Re: dm: check max_sectors in dm_merge_bvec (was: Re: dm: max_segments=1 if merge_bvec_fn is not supported)

Mike Snitzer
2010-12-04T19:21:40Z

On Sat, Dec 04 2010 at 11:03am -0500,
Lars Ellenberg <lars.ellenberg@linbit.com> wrote:

> On Sat, Dec 04, 2010 at 01:43:08AM -0500, Mike Snitzer wrote:
>
> > Given dm_set_device_limits() sets q->limits->max_sectors,
> > shouldn't dm_merge_bvec() be using queue_max_sectors rather than
> > queue_max_hw_sectors?
> >
> > blk_queue_max_hw_sectors() establishes that max_hw_sectors is the hard
> > limit and max_sectors the soft. But AFAICT no relation is maintained
> > between the two over time (even though max_sectors <= max_hw_sectors
> > _should_ be enforced; in practice there is no blk_queue_max_sectors
> > setter that uniformly enforces as much).
>
> Just for the record, in case someone finds this in the archives,
> and wants to backport or base his own work on this:
>
> A long time ago, there was no .max_hw_sectors. Then max_hw_sectors got
> introduced, but without accessor function.
>
> Before 2.6.31, there was no blk_queue_max_hw_sectors(),
> only blk_queue_max_sectors(), which set both.
>
> 2.6.31 introduced some blk_queue_max_hw_sectors(), which _only_ set
> max_hw_sectors, and enforced a lower limit of BLK_DEF_MAX_SECTORS, so
> using that only, you have not been able to actually set lower limits
> than 512 kB. With 2.6.31 to 2.6.33, inclusive, you still need to use
> blk_queue_max_sectors() to set your limits.
>
> 2.6.34 finally dropped the newly introduced function again,
> but renamed the other, so starting with 2.6.34 you need to use
> blk_queue_max_hw_sectors(), which now basically has the function body
> blk_queue_max_sectors() had up until 2.6.33.
>
> > dm_set_device_limits() will set q->limits->max_sectors to <= PAGE_SIZE
> > if an underlying device has a merge_bvec_fn. Therefore, dm_merge_bvec()
> > must use queue_max_sectors() rather than queue_max_hw_sectors() to check
> > the appropriate limit.
>
> IMO, you should not do this.
> max_sectors is a user tunable, capped by max_hw_sectors.
> max_hw_sectors is the driver limit.
>
> Please set max_hw_sectors in dm_set_device_limits instead.

Right, good point.. will do (unless I happen upon a reason not to or
someone else shouts).

Thanks,
Mike