FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Redhat > Device-mapper Development

 
 
LinkBack Thread Tools
 
Old 03-06-2010, 08:10 PM
Lars Ellenberg
 
Default dm: max_segments=1 if merge_bvec_fn is not supported

If the lower device exposes a merge_bvec_fn,
dm_set_device_limits() restricts max_sectors
to PAGE_SIZE "just to be safe".

This is not sufficient, however.

If someone uses bio_add_page() to add 8 disjunct 512 byte partial
pages to a bio, it would succeed, but could still cross a border
of whatever restrictions are below us (e.g. raid10 stripe boundary).
An attempted bio_split() would not succeed, because bi_vcnt is 8.

One example that triggered this frequently is the xen io layer.

raid10_make_request bug: can't convert block across chunks or bigger than 64k 209265151 1

Signed-off-by: Lars <lars.ellenberg@linbit.com>

---

Neil: you may want to double check linear.c and raid*.c for similar logic,
even though it is unlikely that someone puts md raid6 on top of something
exposing a merge_bvec_fn.

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

---
drivers/md/dm-table.c | 12 ++++++++++--
1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 4b22feb..c686ff4 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -515,14 +515,22 @@ int dm_set_device_limits(struct dm_target *ti, struct dm_dev *dev,

/*
* Check if merge fn is supported.
- * If not we'll force DM to use PAGE_SIZE or
+ * If not we'll force DM to use single bio_vec of PAGE_SIZE or
* smaller I/O, just to be safe.
*/

- if (q->merge_bvec_fn && !ti->type->merge)
+ if (q->merge_bvec_fn && !ti->type->merge) {
limits->max_sectors =
min_not_zero(limits->max_sectors,
(unsigned int) (PAGE_SIZE >> 9));
+ /* Restricting max_sectors is not enough.
+ * If someone uses bio_add_page to add 8 disjunct 512 byte
+ * partial pages to a bio, it would succeed,
+ * but could still cross a border of whatever restrictions
+ * are below us (e.g. raid0 stripe boundary). An attempted
+ * bio_split() would not succeed, because bi_vcnt is 8. */
+ limits->max_segments = 1;
+ }
return 0;
}
EXPORT_SYMBOL_GPL(dm_set_device_limits);
--
1.6.3.3

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 03-08-2010, 04:33 AM
Neil Brown
 
Default dm: max_segments=1 if merge_bvec_fn is not supported

On Sat, 6 Mar 2010 22:10:13 +0100
Lars Ellenberg <lars.ellenberg@linbit.com> wrote:

> If the lower device exposes a merge_bvec_fn,
> dm_set_device_limits() restricts max_sectors
> to PAGE_SIZE "just to be safe".
>
> This is not sufficient, however.
>
> If someone uses bio_add_page() to add 8 disjunct 512 byte partial
> pages to a bio, it would succeed, but could still cross a border
> of whatever restrictions are below us (e.g. raid10 stripe boundary).
> An attempted bio_split() would not succeed, because bi_vcnt is 8.
>
> One example that triggered this frequently is the xen io layer.
>
> raid10_make_request bug: can't convert block across chunks or bigger than 64k 209265151 1
>
> Signed-off-by: Lars <lars.ellenberg@linbit.com>
>
> ---
>
> Neil: you may want to double check linear.c and raid*.c for similar logic,
> even though it is unlikely that someone puts md raid6 on top of something
> exposing a merge_bvec_fn.
>

Unlikely, but by no means impossible. I have queued up an appropriate fix
for md.

Thanks!

NeilBrown


> 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
>
> ---
> drivers/md/dm-table.c | 12 ++++++++++--
> 1 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> index 4b22feb..c686ff4 100644
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> @@ -515,14 +515,22 @@ int dm_set_device_limits(struct dm_target *ti, struct dm_dev *dev,
>
> /*
> * Check if merge fn is supported.
> - * If not we'll force DM to use PAGE_SIZE or
> + * If not we'll force DM to use single bio_vec of PAGE_SIZE or
> * smaller I/O, just to be safe.
> */
>
> - if (q->merge_bvec_fn && !ti->type->merge)
> + if (q->merge_bvec_fn && !ti->type->merge) {
> limits->max_sectors =
> min_not_zero(limits->max_sectors,
> (unsigned int) (PAGE_SIZE >> 9));
> + /* Restricting max_sectors is not enough.
> + * If someone uses bio_add_page to add 8 disjunct 512 byte
> + * partial pages to a bio, it would succeed,
> + * but could still cross a border of whatever restrictions
> + * are below us (e.g. raid0 stripe boundary). An attempted
> + * bio_split() would not succeed, because bi_vcnt is 8. */
> + limits->max_segments = 1;
> + }
> return 0;
> }
> EXPORT_SYMBOL_GPL(dm_set_device_limits);

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 03-08-2010, 07:35 AM
Mikulas Patocka
 
Default dm: max_segments=1 if merge_bvec_fn is not supported

On Mon, 8 Mar 2010, Neil Brown wrote:

> On Sat, 6 Mar 2010 22:10:13 +0100
> Lars Ellenberg <lars.ellenberg@linbit.com> wrote:
>
> > If the lower device exposes a merge_bvec_fn,
> > dm_set_device_limits() restricts max_sectors
> > to PAGE_SIZE "just to be safe".
> >
> > This is not sufficient, however.
> >
> > If someone uses bio_add_page() to add 8 disjunct 512 byte partial
> > pages to a bio, it would succeed, but could still cross a border
> > of whatever restrictions are below us (e.g. raid10 stripe boundary).
> > An attempted bio_split() would not succeed, because bi_vcnt is 8.
> >
> > One example that triggered this frequently is the xen io layer.
> >
> > raid10_make_request bug: can't convert block across chunks or bigger than 64k 209265151 1
> >
> > Signed-off-by: Lars <lars.ellenberg@linbit.com>
> >
> > ---
> >
> > Neil: you may want to double check linear.c and raid*.c for similar logic,
> > even though it is unlikely that someone puts md raid6 on top of something
> > exposing a merge_bvec_fn.
> >
>
> Unlikely, but by no means impossible. I have queued up an appropriate fix
> for md.
>
> Thanks!
>
> NeilBrown

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.

> > 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.

Mikulas

> > ---
> > drivers/md/dm-table.c | 12 ++++++++++--
> > 1 files changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> > index 4b22feb..c686ff4 100644
> > --- a/drivers/md/dm-table.c
> > +++ b/drivers/md/dm-table.c
> > @@ -515,14 +515,22 @@ int dm_set_device_limits(struct dm_target *ti, struct dm_dev *dev,
> >
> > /*
> > * Check if merge fn is supported.
> > - * If not we'll force DM to use PAGE_SIZE or
> > + * If not we'll force DM to use single bio_vec of PAGE_SIZE or
> > * smaller I/O, just to be safe.
> > */
> >
> > - if (q->merge_bvec_fn && !ti->type->merge)
> > + if (q->merge_bvec_fn && !ti->type->merge) {
> > limits->max_sectors =
> > min_not_zero(limits->max_sectors,
> > (unsigned int) (PAGE_SIZE >> 9));
> > + /* Restricting max_sectors is not enough.
> > + * If someone uses bio_add_page to add 8 disjunct 512 byte
> > + * partial pages to a bio, it would succeed,
> > + * but could still cross a border of whatever restrictions
> > + * are below us (e.g. raid0 stripe boundary). An attempted
> > + * bio_split() would not succeed, because bi_vcnt is 8. */
> > + limits->max_segments = 1;
> > + }
> > return 0;
> > }
> > EXPORT_SYMBOL_GPL(dm_set_device_limits);
>

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 03-08-2010, 12:14 PM
Lars Ellenberg
 
Default dm: max_segments=1 if merge_bvec_fn is not supported

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?



Lars

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 03-18-2010, 05:48 PM
Andrew Morton
 
Default dm: max_segments=1 if merge_bvec_fn is not supported

On Mon, 8 Mar 2010 14:14:49 +0100
Lars Ellenberg <lars.ellenberg@linbit.com> 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?
>

This all seemed to die. Does Neil's mysterypatch fix all these issues?

Neil, was that patch tagged for -stable backporting?

Thanks.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 03-18-2010, 08:48 PM
Neil Brown
 
Default dm: max_segments=1 if merge_bvec_fn is not supported

On Thu, 18 Mar 2010 11:48:58 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Mon, 8 Mar 2010 14:14:49 +0100
> Lars Ellenberg <lars.ellenberg@linbit.com> 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?
> >
>
> This all seemed to die. Does Neil's mysterypatch fix all these issues?
>
> Neil, was that patch tagged for -stable backporting?

The patch at the top of my 'for-linus' branch (which Linus doesn't seem to
have pulled yet) fixes this for md and is tagged for -stable backporting.
I just sets max_segments and seg_boundary_mask. There is no point setting
max_sectors as well. I found that setting merge_bvec_fn, while a perfectly
correct approach, was more cumbersome.

My patch doesn't fix this for dm. I assume the dm developers will do that.

NeilBrown

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 

Thread Tools




All times are GMT. The time now is 09:03 PM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org