[dm-devel] [PATCH] dm: Deal with merge_bvec_fn in component devices better

[Ben Hutchings]

On Mon, 2013-09-02 at 10:32 -0400, Mikulas Patocka wrote:
> Hi
> 
> This was already fixed in 2.6.31 with commit 
> 8cbeb67ad50f7d68e5e83be2cb2284de8f9c03b5.
> 
> See this piece of code in dm.c:
> 
> /*
>  * If the target doesn't support merge method and some of the devices
>  * provided their merge_bvec method (we know this by looking at
>  * queue_max_hw_sectors), then we can't allow bios with multiple vector
>  * entries.  So always set max_size to 0, and the code below allows
>  * just one page.
>  */
> else if (queue_max_hw_sectors(q) <= PAGE_SIZE >> 9)
>         max_size = 0;

Thanks, I suspected it might have been fixed somehow.

Ben.

> Mikulas
> 
> 
> On Sun, 1 Sep 2013, Ben Hutchings wrote:
> 
> > This is analogous to commit 627a2d3c29427637f4c5d31ccc7fcbd8d312cd71
> > ('md: deal with merge_bvec_fn in component devices better.'),
> > which does the same for md-devices at the top of the stack.  The
> > following explanation is taken from that commit.  Thanks to Neil Brown
> > <neilb at suse.de> for the advice.
> > 
> > If a component device has a merge_bvec_fn then as we never call it
> > we must ensure we never need to.  Currently this is done by setting
> > max_sector to 1 PAGE, however this does not stop a bio being created
> > with several sub-page iovecs that would violate the merge_bvec_fn.
> > 
> > So instead set max_segments to 1 and set the segment boundary to the
> > same as a page boundary to ensure there is only ever one single-page
> > segment of IO requested at a time.
> > 
> > This can particularly be an issue when 'xen' is used as it is
> > known to submit multiple small buffers in a single bio.
> > 
> > References: http://bugs.debian.org/604457
> > Signed-off-by: Ben Hutchings <ben at decadent.org.uk>
> > ---
> > We've been carrying this in Debian for about 3 years and it's about time
> > it got reviewed...  It's basically Neil's work so should probably have
> > his sign-off as well.
> > 
> > Ben.
> > 
> > --- a/drivers/md/dm-table.c
> > +++ b/drivers/md/dm-table.c
> > @@ -536,13 +536,15 @@ int dm_set_device_limits(struct dm_target *ti, struct dm_dev *dev,
> >  		       (unsigned long long) start << SECTOR_SHIFT);
> >  
> >  	/*
> > -	 * Check if merge fn is supported.
> > -	 * If not we'll force DM to use PAGE_SIZE or
> > -	 * smaller I/O, just to be safe.
> > +	 * If we don't call merge_bvec_fn, we must never risk
> > +	 * violating it, so limit max_phys_segments to 1 lying within
> > +	 * a single page.
> >  	 */
> > -	if (dm_queue_merge_is_compulsory(q) && !ti->type->merge)
> > -		blk_limits_max_hw_sectors(limits,
> > -					  (unsigned int) (PAGE_SIZE >> 9));
> > +	if (dm_queue_merge_is_compulsory(q) && !ti->type->merge) {
> > +		limits->max_segments = 1;
> > +		limits->seg_boundary_mask = PAGE_CACHE_SIZE - 1;
> > +	}
> > +
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(dm_set_device_limits);
> > 
> > -- 
> > Ben Hutchings
> > The most exhausting thing in life is being insincere. - Anne Morrow Lindberg
> > 

-- 
Ben Hutchings
If you seem to know what you are doing, you'll be given more to do.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 828 bytes
Desc: This is a digitally signed message part
URL: <http://listman.redhat.com/archives/dm-devel/attachments/20130903/fa987138/attachment.sig>