I'm late to this old thread but I stumbled across it while auditing the
various dm-devel patchwork patches, e.g.:
https://patchwork.kernel.org/patch/83666/
https://patchwork.kernel.org/patch/83932/
On Mon, Mar 08 2010 at 8:14am -0500,
Lars Ellenberg <[email protected]> wrote:
> On Mon, Mar 08, 2010 at 03:35:37AM -0500, Mikulas Patocka wrote:
> > Hi
> >
> > That patch with limits->max_segments = 1; is wrong. It fixes this bug
> > sometimes and sometimes not.
> >
> > The problem is, if someone attempts to create a bio with two vector
> > entries, the first maps the last sector contained in some page and the
> > second maps the first sector of the next physical page: it has one
> > segment, it has size <= PAGE_SIZE, but it still may cross raid stripe and
> > the raid driver will reject it.
>
> Now that you put it that way ;)
> You are right.
>
> My asumption that "single segment" was
> equalvalent in practice with "single bvec"
> does not hold true in that case.
>
> Then, what about adding seg_boundary_mask restrictions as well?
> max_sectors = PAGE_SIZE >> 9;
> max_segments = 1;
> seg_boundary_mask = PAGE_SIZE -1;
> or some such.
>
> > > > This is not the first time this has been patched, btw.
> > > > See https://bugzilla.redhat.com/show_bug.cgi?id=440093
> > > > and the patch by Mikulas:
> > > > https://bugzilla.redhat.com/attachment.cgi?id=342638&action=diff
> >
> > Look at this patch, it is the proper way how to fix it: create a
> > merge_bvec_fn that reject more than one biovec entry.
>
> If adding seg_boundary_mask is still not sufficient,
> lets merge that patch instead?
> Why has it been dropped, respectively never been merged?
> It became obsolete for dm-linear by 7bc3447b,
> but in general the bug is still there, or am I missing something?
No it _should_ be fixed in general given DM's dm_merge_bvec() _but_ I
did uncover what I think is a subtle oversight in its implementation.
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).
Anyway, I think we need the following patch:
--
From: Mike Snitzer <[email protected]>
Subject: dm: check max_sectors in dm_merge_bvec
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.
Signed-off-by: Mike Snitzer <[email protected]>
---
drivers/md/dm.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 7cb1352..e83dcc8 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1358,12 +1358,11 @@ static int dm_merge_bvec(struct request_queue *q,
/*
* 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
+ * queue_max_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)
-
+ else if (queue_max_sectors(q) <= PAGE_SIZE >> 9)
max_size = 0;
out_table:
On Sat, Dec 04, 2010 at 01:43:08AM -0500, Mike Snitzer wrote:
> I'm late to this old thread but I stumbled across it while auditing the
> various dm-devel patchwork patches, e.g.:
> https://patchwork.kernel.org/patch/83666/
> https://patchwork.kernel.org/patch/83932/
>
> On Mon, Mar 08 2010 at 8:14am -0500,
> Lars Ellenberg <[email protected]> wrote:
>
> > On Mon, Mar 08, 2010 at 03:35:37AM -0500, Mikulas Patocka wrote:
> > > Hi
> > >
> > > That patch with limits->max_segments = 1; is wrong. It fixes this bug
> > > sometimes and sometimes not.
> > >
> > > The problem is, if someone attempts to create a bio with two vector
> > > entries, the first maps the last sector contained in some page and the
> > > second maps the first sector of the next physical page: it has one
> > > segment, it has size <= PAGE_SIZE, but it still may cross raid stripe and
> > > the raid driver will reject it.
> >
> > Now that you put it that way ;)
> > You are right.
> >
> > My asumption that "single segment" was
> > equalvalent in practice with "single bvec"
> > does not hold true in that case.
> >
> > Then, what about adding seg_boundary_mask restrictions as well?
> > max_sectors = PAGE_SIZE >> 9;
> > max_segments = 1;
> > seg_boundary_mask = PAGE_SIZE -1;
> > or some such.
> >
> > > > > This is not the first time this has been patched, btw.
> > > > > See https://bugzilla.redhat.com/show_bug.cgi?id=440093
> > > > > and the patch by Mikulas:
> > > > > https://bugzilla.redhat.com/attachment.cgi?id=342638&action=diff
> > >
> > > Look at this patch, it is the proper way how to fix it: create a
> > > merge_bvec_fn that reject more than one biovec entry.
> >
> > If adding seg_boundary_mask is still not sufficient,
> > lets merge that patch instead?
> > Why has it been dropped, respectively never been merged?
> > It became obsolete for dm-linear by 7bc3447b,
> > but in general the bug is still there, or am I missing something?
>
> No it _should_ be fixed in general given DM's dm_merge_bvec() _but_ I
> did uncover what I think is a subtle oversight in its implementation.
>
> 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.
BTW, e.g. o_direct will adhere to max_hw_limits,
but happily ignore max_sectors, I think.
> Signed-off-by: Mike Snitzer <[email protected]>
> ---
> drivers/md/dm.c | 5 ++---
> 1 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 7cb1352..e83dcc8 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -1358,12 +1358,11 @@ static int dm_merge_bvec(struct request_queue *q,
> /*
> * 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
> + * queue_max_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)
> -
> + else if (queue_max_sectors(q) <= PAGE_SIZE >> 9)
> max_size = 0;
>
> out_table:
Lars
On Sat, Dec 04 2010 at 11:03am -0500,
Lars Ellenberg <[email protected]> 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