* [PATCH] dm-raid: stack limits instead of overwriting them.
@ 2020-09-24 16:26 Mikulas Patocka
2020-09-24 16:45 ` John Dorminy
2020-09-24 16:56 ` Mike Snitzer
0 siblings, 2 replies; 14+ messages in thread
From: Mikulas Patocka @ 2020-09-24 16:26 UTC (permalink / raw)
To: Mike Snitzer, Zdenek Kabelac; +Cc: dm-devel
This patch fixes a warning WARN_ON_ONCE(!q->limits.discard_granularity).
The reason is that the function raid_io_hints overwrote
limits->discard_granularity with zero. We need to properly stack the
limits instead of overwriting them.
Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Cc: stable@vger.kernel.org
---
drivers/md/dm-raid.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Index: linux-2.6/drivers/md/dm-raid.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-raid.c 2020-09-24 18:16:45.000000000 +0200
+++ linux-2.6/drivers/md/dm-raid.c 2020-09-24 18:16:45.000000000 +0200
@@ -3734,8 +3734,8 @@ static void raid_io_hints(struct dm_targ
* RAID0/4/5/6 don't and process large discard bios properly.
*/
if (rs_is_raid1(rs) || rs_is_raid10(rs)) {
- limits->discard_granularity = chunk_size_bytes;
- limits->max_discard_sectors = rs->md.chunk_sectors;
+ limits->discard_granularity = max(limits->discard_granularity, chunk_size_bytes);
+ limits->max_discard_sectors = min_not_zero(limits->max_discard_sectors, (unsigned)rs->md.chunk_sectors);
}
}
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] dm-raid: stack limits instead of overwriting them.
2020-09-24 16:26 [PATCH] dm-raid: stack limits instead of overwriting them Mikulas Patocka
@ 2020-09-24 16:45 ` John Dorminy
2020-09-24 16:58 ` Mikulas Patocka
2020-09-24 17:00 ` Mike Snitzer
2020-09-24 16:56 ` Mike Snitzer
1 sibling, 2 replies; 14+ messages in thread
From: John Dorminy @ 2020-09-24 16:45 UTC (permalink / raw)
To: Mikulas Patocka; +Cc: Mike Snitzer, device-mapper development, Zdenek Kabelac
I don't understand how this works...
Can chunk_size_bytes be 0? If not, how is discard_granularity being set to 0?
I think also limits is local to the ti in question here, initialized
by blk_set_stacking_limits() via dm-table.c, and therefore has only
default values and not anything to do with the underlying queue. So
setting discard_granularity=max(discard_granularity, chunk_size_bytes)
doesn't seem like it should be working, unless I'm not understanding
what it's there for...
And shouldn't melding in the target's desired io_hints into the
existing queue limits be happening in blk_stack_limits() instead?
(Also, it does lcm_not_zero() for stacking granularity, instead of
max()...)
On Thu, Sep 24, 2020 at 12:29 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
>
> This patch fixes a warning WARN_ON_ONCE(!q->limits.discard_granularity).
> The reason is that the function raid_io_hints overwrote
> limits->discard_granularity with zero. We need to properly stack the
> limits instead of overwriting them.
>
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Cc: stable@vger.kernel.org
>
> ---
> drivers/md/dm-raid.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> Index: linux-2.6/drivers/md/dm-raid.c
> ===================================================================
> --- linux-2.6.orig/drivers/md/dm-raid.c 2020-09-24 18:16:45.000000000 +0200
> +++ linux-2.6/drivers/md/dm-raid.c 2020-09-24 18:16:45.000000000 +0200
> @@ -3734,8 +3734,8 @@ static void raid_io_hints(struct dm_targ
> * RAID0/4/5/6 don't and process large discard bios properly.
> */
> if (rs_is_raid1(rs) || rs_is_raid10(rs)) {
> - limits->discard_granularity = chunk_size_bytes;
> - limits->max_discard_sectors = rs->md.chunk_sectors;
> + limits->discard_granularity = max(limits->discard_granularity, chunk_size_bytes);
> + limits->max_discard_sectors = min_not_zero(limits->max_discard_sectors, (unsigned)rs->md.chunk_sectors);
> }
> }
>
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: dm-raid: stack limits instead of overwriting them.
2020-09-24 16:26 [PATCH] dm-raid: stack limits instead of overwriting them Mikulas Patocka
2020-09-24 16:45 ` John Dorminy
@ 2020-09-24 16:56 ` Mike Snitzer
2020-09-24 17:07 ` Mikulas Patocka
1 sibling, 1 reply; 14+ messages in thread
From: Mike Snitzer @ 2020-09-24 16:56 UTC (permalink / raw)
To: Mikulas Patocka; +Cc: dm-devel, Zdenek Kabelac
On Thu, Sep 24 2020 at 12:26pm -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:
> This patch fixes a warning WARN_ON_ONCE(!q->limits.discard_granularity).
> The reason is that the function raid_io_hints overwrote
> limits->discard_granularity with zero. We need to properly stack the
> limits instead of overwriting them.
>
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Cc: stable@vger.kernel.org
>
> ---
> drivers/md/dm-raid.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> Index: linux-2.6/drivers/md/dm-raid.c
> ===================================================================
> --- linux-2.6.orig/drivers/md/dm-raid.c 2020-09-24 18:16:45.000000000 +0200
> +++ linux-2.6/drivers/md/dm-raid.c 2020-09-24 18:16:45.000000000 +0200
> @@ -3734,8 +3734,8 @@ static void raid_io_hints(struct dm_targ
> * RAID0/4/5/6 don't and process large discard bios properly.
> */
> if (rs_is_raid1(rs) || rs_is_raid10(rs)) {
> - limits->discard_granularity = chunk_size_bytes;
> - limits->max_discard_sectors = rs->md.chunk_sectors;
> + limits->discard_granularity = max(limits->discard_granularity, chunk_size_bytes);
> + limits->max_discard_sectors = min_not_zero(limits->max_discard_sectors, (unsigned)rs->md.chunk_sectors);
> }
> }
>
OK, but how is it that chunk_size_bytes is 0? Oh, raid1 doesn't have a
chunksize does it!?
Relative to MD raid0 and raid10: they don't have dm-stripe like
optimization to handle large discards. So stacking up larger discard
limits (that span multiple chunks) is a non-starter right?
Like dm-raid.c, raid10.c does explicitly set max_discard_sectors to
mddev->chunk_sectors. But it (mistakenly IMHO) just accepts stackd up
discard_granularity.
Looking at raid1.c I see MD is just stacking up the limits without
modification. Maybe dm-raid.c shouldn't be changing these limits at all
for raid1 (just use what was already stacked)?
WAIT... Could it be that raid_io_hints _really_ meant to special case
raid0 and raid10 -- due to their striping/splitting requirements!?
So, not raid1 but raid0?
E.g.:
diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
index 56b723d012ac..6dca932d6f1d 100644
--- a/drivers/md/dm-raid.c
+++ b/drivers/md/dm-raid.c
@@ -3730,10 +3730,10 @@ static void raid_io_hints(struct dm_target *ti,
struct queue_limits *limits)
blk_limits_io_opt(limits, chunk_size_bytes *
mddev_data_stripes(rs));
/*
- * RAID1 and RAID10 personalities require bio splitting,
- * RAID0/4/5/6 don't and process large discard bios properly.
+ * RAID0 and RAID10 personalities require bio splitting,
+ * RAID1/4/5/6 don't and process large discard bios properly.
*/
- if (rs_is_raid1(rs) || rs_is_raid10(rs)) {
+ if (rs_is_raid0(rs) || rs_is_raid10(rs)) {
limits->discard_granularity = chunk_size_bytes;
limits->max_discard_sectors = rs->md.chunk_sectors;
}
Mike
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] dm-raid: stack limits instead of overwriting them.
2020-09-24 16:45 ` John Dorminy
@ 2020-09-24 16:58 ` Mikulas Patocka
2020-09-24 17:00 ` Mike Snitzer
1 sibling, 0 replies; 14+ messages in thread
From: Mikulas Patocka @ 2020-09-24 16:58 UTC (permalink / raw)
To: John Dorminy; +Cc: Mike Snitzer, device-mapper development, Zdenek Kabelac
On Thu, 24 Sep 2020, John Dorminy wrote:
> I don't understand how this works...
>
> Can chunk_size_bytes be 0? If not, how is discard_granularity being set to 0?
Yes - chunk_size_bytes is 0 here.
> I think also limits is local to the ti in question here, initialized
> by blk_set_stacking_limits() via dm-table.c, and therefore has only
> default values and not anything to do with the underlying queue. So
In dm_calculate_queue_limits, there's
blk_set_stacking_limits(limits);
- that will initialize the default limits (discard_granularity == 0)
Then, there's:
ti->type->iterate_devices(ti, dm_set_device_limits, &ti_limits);
- that will unify limits for all RAID legs (in this case, it sets
discard_granularity == 1024)
Then, there's
if (ti->type->io_hints) ti->type->io_hints(ti, &ti_limits);
And that will incorrectly overwrite discard_granularity with zero.
> setting discard_granularity=max(discard_granularity, chunk_size_bytes)
> doesn't seem like it should be working, unless I'm not understanding
> what it's there for...
>
> And shouldn't melding in the target's desired io_hints into the
> existing queue limits be happening in blk_stack_limits() instead?
> (Also, it does lcm_not_zero() for stacking granularity, instead of
> max()...)
Well, you can add blk_stack_limits to raid_io_hints and use it to stack
the limits, but that would just complicate that function.
Mikulas
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: dm-raid: stack limits instead of overwriting them.
2020-09-24 16:45 ` John Dorminy
2020-09-24 16:58 ` Mikulas Patocka
@ 2020-09-24 17:00 ` Mike Snitzer
2020-09-24 17:24 ` John Dorminy
1 sibling, 1 reply; 14+ messages in thread
From: Mike Snitzer @ 2020-09-24 17:00 UTC (permalink / raw)
To: John Dorminy; +Cc: device-mapper development, Mikulas Patocka, Zdenek Kabelac
On Thu, Sep 24 2020 at 12:45pm -0400,
John Dorminy <jdorminy@redhat.com> wrote:
> I don't understand how this works...
>
> Can chunk_size_bytes be 0? If not, how is discard_granularity being set to 0?
Yeah, I had same question.. see the reply I just sent in this thread:
https://www.redhat.com/archives/dm-devel/2020-September/msg00568.html
> I think also limits is local to the ti in question here, initialized
> by blk_set_stacking_limits() via dm-table.c, and therefore has only
> default values and not anything to do with the underlying queue. So
> setting discard_granularity=max(discard_granularity, chunk_size_bytes)
> doesn't seem like it should be working, unless I'm not understanding
> what it's there for...
You're reading the dm-table.c limits stacking wrong. Of course DM stack
up the underlying device(s) limits ;)
>
> And shouldn't melding in the target's desired io_hints into the
> existing queue limits be happening in blk_stack_limits() instead?
> (Also, it does lcm_not_zero() for stacking granularity, instead of
> max()...)
>
DM core does do that, the .io_hints hook in the DM target is reserved
for when the target has additional constraints that blk_stack_limits()
didn't/couldn't factor in.
And blk_stack_limts() does use max() for discard_granularity.
Mike
>
> On Thu, Sep 24, 2020 at 12:29 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
> >
> > This patch fixes a warning WARN_ON_ONCE(!q->limits.discard_granularity).
> > The reason is that the function raid_io_hints overwrote
> > limits->discard_granularity with zero. We need to properly stack the
> > limits instead of overwriting them.
> >
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > Cc: stable@vger.kernel.org
> >
> > ---
> > drivers/md/dm-raid.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > Index: linux-2.6/drivers/md/dm-raid.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/md/dm-raid.c 2020-09-24 18:16:45.000000000 +0200
> > +++ linux-2.6/drivers/md/dm-raid.c 2020-09-24 18:16:45.000000000 +0200
> > @@ -3734,8 +3734,8 @@ static void raid_io_hints(struct dm_targ
> > * RAID0/4/5/6 don't and process large discard bios properly.
> > */
> > if (rs_is_raid1(rs) || rs_is_raid10(rs)) {
> > - limits->discard_granularity = chunk_size_bytes;
> > - limits->max_discard_sectors = rs->md.chunk_sectors;
> > + limits->discard_granularity = max(limits->discard_granularity, chunk_size_bytes);
> > + limits->max_discard_sectors = min_not_zero(limits->max_discard_sectors, (unsigned)rs->md.chunk_sectors);
> > }
> > }
> >
> >
> > --
> > dm-devel mailing list
> > dm-devel@redhat.com
> > https://www.redhat.com/mailman/listinfo/dm-devel
> >
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: dm-raid: stack limits instead of overwriting them.
2020-09-24 16:56 ` Mike Snitzer
@ 2020-09-24 17:07 ` Mikulas Patocka
2020-09-24 18:12 ` Mikulas Patocka
0 siblings, 1 reply; 14+ messages in thread
From: Mikulas Patocka @ 2020-09-24 17:07 UTC (permalink / raw)
To: Mike Snitzer; +Cc: dm-devel, Zdenek Kabelac
On Thu, 24 Sep 2020, Mike Snitzer wrote:
> WAIT... Could it be that raid_io_hints _really_ meant to special case
> raid0 and raid10 -- due to their striping/splitting requirements!?
> So, not raid1 but raid0?
>
> E.g.:
>
> diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
> index 56b723d012ac..6dca932d6f1d 100644
> --- a/drivers/md/dm-raid.c
> +++ b/drivers/md/dm-raid.c
> @@ -3730,10 +3730,10 @@ static void raid_io_hints(struct dm_target *ti,
> struct queue_limits *limits)
> blk_limits_io_opt(limits, chunk_size_bytes *
> mddev_data_stripes(rs));
>
> /*
> - * RAID1 and RAID10 personalities require bio splitting,
> - * RAID0/4/5/6 don't and process large discard bios properly.
> + * RAID0 and RAID10 personalities require bio splitting,
> + * RAID1/4/5/6 don't and process large discard bios properly.
> */
> - if (rs_is_raid1(rs) || rs_is_raid10(rs)) {
> + if (rs_is_raid0(rs) || rs_is_raid10(rs)) {
> limits->discard_granularity = chunk_size_bytes;
> limits->max_discard_sectors = rs->md.chunk_sectors;
> }
>
> Mike
Yes - that's an interesing point.
Mikulas
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: dm-raid: stack limits instead of overwriting them.
2020-09-24 17:00 ` Mike Snitzer
@ 2020-09-24 17:24 ` John Dorminy
2020-09-24 18:56 ` John Dorminy
0 siblings, 1 reply; 14+ messages in thread
From: John Dorminy @ 2020-09-24 17:24 UTC (permalink / raw)
To: Mike Snitzer; +Cc: device-mapper development, Mikulas Patocka, Zdenek Kabelac
I am impressed at how much I read wrong...
On Thu, Sep 24, 2020 at 1:00 PM Mike Snitzer <snitzer@redhat.com> wrote:
>
> On Thu, Sep 24 2020 at 12:45pm -0400,
> John Dorminy <jdorminy@redhat.com> wrote:
>
> > I don't understand how this works...
> >
> > Can chunk_size_bytes be 0? If not, how is discard_granularity being set to 0?
>
> Yeah, I had same question.. see the reply I just sent in this thread:
> https://www.redhat.com/archives/dm-devel/2020-September/msg00568.html
>
> > I think also limits is local to the ti in question here, initialized
> > by blk_set_stacking_limits() via dm-table.c, and therefore has only
> > default values and not anything to do with the underlying queue. So
> > setting discard_granularity=max(discard_granularity, chunk_size_bytes)
> > doesn't seem like it should be working, unless I'm not understanding
> > what it's there for...
>
> You're reading the dm-table.c limits stacking wrong. Of course DM stack
> up the underlying device(s) limits ;)
Yep, I failed to read iterate_devices...
>
> >
> > And shouldn't melding in the target's desired io_hints into the
> > existing queue limits be happening in blk_stack_limits() instead?
> > (Also, it does lcm_not_zero() for stacking granularity, instead of
> > max()...)
> >
>
> DM core does do that, the .io_hints hook in the DM target is reserved
> for when the target has additional constraints that blk_stack_limits()
> didn't/couldn't factor in.
Yes, I had erroneously thought the limit-stacking was after getting
the targets' individual limits, not before.
>
> And blk_stack_limts() does use max() for discard_granularity.
... I'm just terrible at reading this morning.
Thanks for pointing out all the things I misread!
>
> Mike
>
> >
> > On Thu, Sep 24, 2020 at 12:29 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
> > >
> > > This patch fixes a warning WARN_ON_ONCE(!q->limits.discard_granularity).
> > > The reason is that the function raid_io_hints overwrote
> > > limits->discard_granularity with zero. We need to properly stack the
> > > limits instead of overwriting them.
> > >
> > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > > Cc: stable@vger.kernel.org
> > >
> > > ---
> > > drivers/md/dm-raid.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > Index: linux-2.6/drivers/md/dm-raid.c
> > > ===================================================================
> > > --- linux-2.6.orig/drivers/md/dm-raid.c 2020-09-24 18:16:45.000000000 +0200
> > > +++ linux-2.6/drivers/md/dm-raid.c 2020-09-24 18:16:45.000000000 +0200
> > > @@ -3734,8 +3734,8 @@ static void raid_io_hints(struct dm_targ
> > > * RAID0/4/5/6 don't and process large discard bios properly.
> > > */
> > > if (rs_is_raid1(rs) || rs_is_raid10(rs)) {
> > > - limits->discard_granularity = chunk_size_bytes;
> > > - limits->max_discard_sectors = rs->md.chunk_sectors;
> > > + limits->discard_granularity = max(limits->discard_granularity, chunk_size_bytes);
> > > + limits->max_discard_sectors = min_not_zero(limits->max_discard_sectors, (unsigned)rs->md.chunk_sectors);
> > > }
> > > }
> > >
> > >
> > > --
> > > dm-devel mailing list
> > > dm-devel@redhat.com
> > > https://www.redhat.com/mailman/listinfo/dm-devel
> > >
> >
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: dm-raid: stack limits instead of overwriting them.
2020-09-24 17:07 ` Mikulas Patocka
@ 2020-09-24 18:12 ` Mikulas Patocka
2020-09-24 19:07 ` Mike Snitzer
0 siblings, 1 reply; 14+ messages in thread
From: Mikulas Patocka @ 2020-09-24 18:12 UTC (permalink / raw)
To: Mike Snitzer; +Cc: dm-devel, Zdenek Kabelac
On Thu, 24 Sep 2020, Mikulas Patocka wrote:
>
>
> On Thu, 24 Sep 2020, Mike Snitzer wrote:
>
> > WAIT... Could it be that raid_io_hints _really_ meant to special case
> > raid0 and raid10 -- due to their striping/splitting requirements!?
> > So, not raid1 but raid0?
> >
> > E.g.:
> >
> > diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
> > index 56b723d012ac..6dca932d6f1d 100644
> > --- a/drivers/md/dm-raid.c
> > +++ b/drivers/md/dm-raid.c
> > @@ -3730,10 +3730,10 @@ static void raid_io_hints(struct dm_target *ti,
> > struct queue_limits *limits)
> > blk_limits_io_opt(limits, chunk_size_bytes *
> > mddev_data_stripes(rs));
> >
> > /*
> > - * RAID1 and RAID10 personalities require bio splitting,
> > - * RAID0/4/5/6 don't and process large discard bios properly.
> > + * RAID0 and RAID10 personalities require bio splitting,
> > + * RAID1/4/5/6 don't and process large discard bios properly.
> > */
> > - if (rs_is_raid1(rs) || rs_is_raid10(rs)) {
> > + if (rs_is_raid0(rs) || rs_is_raid10(rs)) {
> > limits->discard_granularity = chunk_size_bytes;
> > limits->max_discard_sectors = rs->md.chunk_sectors;
> > }
> >
> > Mike
>
> Yes - that's an interesing point.
>
> Mikulas
But raid0_handle_discard handles discards with arbitrary start/end
sectors.
So, we don't need to set discard_granularity for that.
Mikulas
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: dm-raid: stack limits instead of overwriting them.
2020-09-24 17:24 ` John Dorminy
@ 2020-09-24 18:56 ` John Dorminy
2020-09-24 19:15 ` Mike Snitzer
2020-12-17 23:40 ` [dm-devel] " S. Baerwolf
0 siblings, 2 replies; 14+ messages in thread
From: John Dorminy @ 2020-09-24 18:56 UTC (permalink / raw)
To: Mike Snitzer; +Cc: device-mapper development, Mikulas Patocka, Zdenek Kabelac
On Thu, Sep 24, 2020 at 1:24 PM John Dorminy <jdorminy@redhat.com> wrote:
>
> I am impressed at how much I read wrong...
>
> On Thu, Sep 24, 2020 at 1:00 PM Mike Snitzer <snitzer@redhat.com> wrote:
> >
> > On Thu, Sep 24 2020 at 12:45pm -0400,
> > John Dorminy <jdorminy@redhat.com> wrote:
> >
> > > I don't understand how this works...
> > >
> > > Can chunk_size_bytes be 0? If not, how is discard_granularity being set to 0?
> >
> > Yeah, I had same question.. see the reply I just sent in this thread:
> > https://www.redhat.com/archives/dm-devel/2020-September/msg00568.html
> >
> > > I think also limits is local to the ti in question here, initialized
> > > by blk_set_stacking_limits() via dm-table.c, and therefore has only
> > > default values and not anything to do with the underlying queue. So
> > > setting discard_granularity=max(discard_granularity, chunk_size_bytes)
> > > doesn't seem like it should be working, unless I'm not understanding
> > > what it's there for...
> >
> > You're reading the dm-table.c limits stacking wrong. Of course DM stack
> > up the underlying device(s) limits ;)
>
> Yep, I failed to read iterate_devices...
>
> >
> > >
> > > And shouldn't melding in the target's desired io_hints into the
> > > existing queue limits be happening in blk_stack_limits() instead?
> > > (Also, it does lcm_not_zero() for stacking granularity, instead of
> > > max()...)
> > >
> >
> > DM core does do that, the .io_hints hook in the DM target is reserved
> > for when the target has additional constraints that blk_stack_limits()
> > didn't/couldn't factor in.
> Yes, I had erroneously thought the limit-stacking was after getting
> the targets' individual limits, not before.
>
> >
> > And blk_stack_limts() does use max() for discard_granularity.
> ... I'm just terrible at reading this morning.
>
> Thanks for pointing out all the things I misread!
Actually, though, I don't understand why it should be max instead of
lcm_not_zero(). If the raid's chunk size is 1024 sectors, say, and
you're constructing it on something that has discard_granularity 812
sectors, say, blkdev_issue_discard will be generating 1024 sector IOs
which will work poorly when passed down to the 812-sector-granularity
underlying device. While, if lcm(812,1024) were used, lcm(812,1024)
sector IOs would be compatible with both the chunk size and underlying
device's granularity, perhaps? Maybe I'm missing something, but I read
the doc and code an extra time around this time ;)
>
> >
> > Mike
> >
> > >
> > > On Thu, Sep 24, 2020 at 12:29 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
> > > >
> > > > This patch fixes a warning WARN_ON_ONCE(!q->limits.discard_granularity).
> > > > The reason is that the function raid_io_hints overwrote
> > > > limits->discard_granularity with zero. We need to properly stack the
> > > > limits instead of overwriting them.
> > > >
> > > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > > > Cc: stable@vger.kernel.org
> > > >
> > > > ---
> > > > drivers/md/dm-raid.c | 4 ++--
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > Index: linux-2.6/drivers/md/dm-raid.c
> > > > ===================================================================
> > > > --- linux-2.6.orig/drivers/md/dm-raid.c 2020-09-24 18:16:45.000000000 +0200
> > > > +++ linux-2.6/drivers/md/dm-raid.c 2020-09-24 18:16:45.000000000 +0200
> > > > @@ -3734,8 +3734,8 @@ static void raid_io_hints(struct dm_targ
> > > > * RAID0/4/5/6 don't and process large discard bios properly.
> > > > */
> > > > if (rs_is_raid1(rs) || rs_is_raid10(rs)) {
> > > > - limits->discard_granularity = chunk_size_bytes;
> > > > - limits->max_discard_sectors = rs->md.chunk_sectors;
> > > > + limits->discard_granularity = max(limits->discard_granularity, chunk_size_bytes);
> > > > + limits->max_discard_sectors = min_not_zero(limits->max_discard_sectors, (unsigned)rs->md.chunk_sectors);
> > > > }
> > > > }
> > > >
> > > >
> > > > --
> > > > dm-devel mailing list
> > > > dm-devel@redhat.com
> > > > https://www.redhat.com/mailman/listinfo/dm-devel
> > > >
> > >
> >
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: dm-raid: stack limits instead of overwriting them.
2020-09-24 18:12 ` Mikulas Patocka
@ 2020-09-24 19:07 ` Mike Snitzer
2020-09-25 12:04 ` Mikulas Patocka
0 siblings, 1 reply; 14+ messages in thread
From: Mike Snitzer @ 2020-09-24 19:07 UTC (permalink / raw)
To: Mikulas Patocka; +Cc: dm-devel, Zdenek Kabelac
On Thu, Sep 24 2020 at 2:12pm -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:
>
>
> On Thu, 24 Sep 2020, Mikulas Patocka wrote:
>
> >
> >
> > On Thu, 24 Sep 2020, Mike Snitzer wrote:
> >
> > > WAIT... Could it be that raid_io_hints _really_ meant to special case
> > > raid0 and raid10 -- due to their striping/splitting requirements!?
> > > So, not raid1 but raid0?
> > >
> > > E.g.:
> > >
> > > diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
> > > index 56b723d012ac..6dca932d6f1d 100644
> > > --- a/drivers/md/dm-raid.c
> > > +++ b/drivers/md/dm-raid.c
> > > @@ -3730,10 +3730,10 @@ static void raid_io_hints(struct dm_target *ti,
> > > struct queue_limits *limits)
> > > blk_limits_io_opt(limits, chunk_size_bytes *
> > > mddev_data_stripes(rs));
> > >
> > > /*
> > > - * RAID1 and RAID10 personalities require bio splitting,
> > > - * RAID0/4/5/6 don't and process large discard bios properly.
> > > + * RAID0 and RAID10 personalities require bio splitting,
> > > + * RAID1/4/5/6 don't and process large discard bios properly.
> > > */
> > > - if (rs_is_raid1(rs) || rs_is_raid10(rs)) {
> > > + if (rs_is_raid0(rs) || rs_is_raid10(rs)) {
> > > limits->discard_granularity = chunk_size_bytes;
> > > limits->max_discard_sectors = rs->md.chunk_sectors;
> > > }
> > >
> > > Mike
> >
> > Yes - that's an interesing point.
> >
> > Mikulas
>
> But raid0_handle_discard handles discards with arbitrary start/end
> sectors.
>
> So, we don't need to set discard_granularity for that.
OK, great, I've staged this:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-5.10&id=c1fda10e1123a37cf9d22740486cd66f43c47846
Thanks,
Mike
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: dm-raid: stack limits instead of overwriting them.
2020-09-24 18:56 ` John Dorminy
@ 2020-09-24 19:15 ` Mike Snitzer
2020-12-17 23:40 ` [dm-devel] " S. Baerwolf
1 sibling, 0 replies; 14+ messages in thread
From: Mike Snitzer @ 2020-09-24 19:15 UTC (permalink / raw)
To: John Dorminy
Cc: device-mapper development, Mikulas Patocka, Martin K. Petersen,
Zdenek Kabelac
On Thu, Sep 24 2020 at 2:56pm -0400,
John Dorminy <jdorminy@redhat.com> wrote:
> On Thu, Sep 24, 2020 at 1:24 PM John Dorminy <jdorminy@redhat.com> wrote:
> >
> > I am impressed at how much I read wrong...
> >
> > On Thu, Sep 24, 2020 at 1:00 PM Mike Snitzer <snitzer@redhat.com> wrote:
> > >
> > > On Thu, Sep 24 2020 at 12:45pm -0400,
> > > John Dorminy <jdorminy@redhat.com> wrote:
> > >
> > > > I don't understand how this works...
> > > >
> > > > Can chunk_size_bytes be 0? If not, how is discard_granularity being set to 0?
> > >
> > > Yeah, I had same question.. see the reply I just sent in this thread:
> > > https://www.redhat.com/archives/dm-devel/2020-September/msg00568.html
> > >
> > > > I think also limits is local to the ti in question here, initialized
> > > > by blk_set_stacking_limits() via dm-table.c, and therefore has only
> > > > default values and not anything to do with the underlying queue. So
> > > > setting discard_granularity=max(discard_granularity, chunk_size_bytes)
> > > > doesn't seem like it should be working, unless I'm not understanding
> > > > what it's there for...
> > >
> > > You're reading the dm-table.c limits stacking wrong. Of course DM stack
> > > up the underlying device(s) limits ;)
> >
> > Yep, I failed to read iterate_devices...
> >
> > >
> > > >
> > > > And shouldn't melding in the target's desired io_hints into the
> > > > existing queue limits be happening in blk_stack_limits() instead?
> > > > (Also, it does lcm_not_zero() for stacking granularity, instead of
> > > > max()...)
> > > >
> > >
> > > DM core does do that, the .io_hints hook in the DM target is reserved
> > > for when the target has additional constraints that blk_stack_limits()
> > > didn't/couldn't factor in.
> > Yes, I had erroneously thought the limit-stacking was after getting
> > the targets' individual limits, not before.
> >
> > >
> > > And blk_stack_limts() does use max() for discard_granularity.
> > ... I'm just terrible at reading this morning.
> >
> > Thanks for pointing out all the things I misread!
>
> Actually, though, I don't understand why it should be max instead of
> lcm_not_zero(). If the raid's chunk size is 1024 sectors, say, and
> you're constructing it on something that has discard_granularity 812
> sectors, say, blkdev_issue_discard will be generating 1024 sector IOs
> which will work poorly when passed down to the 812-sector-granularity
> underlying device. While, if lcm(812,1024) were used, lcm(812,1024)
> sector IOs would be compatible with both the chunk size and underlying
> device's granularity, perhaps? Maybe I'm missing something, but I read
> the doc and code an extra time around this time ;)
Martin may correct me if I'm wrong but I _think_ it is because
discard_granularity is unintuitive. The larger the discard_granularity
the more constraining it is (on other devices with more relaxed, or
smaller, discard_granularity). So you need to impose the most
constrained limit for all when stacking.
Mike
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: dm-raid: stack limits instead of overwriting them.
2020-09-24 19:07 ` Mike Snitzer
@ 2020-09-25 12:04 ` Mikulas Patocka
2020-09-25 13:20 ` Mike Snitzer
0 siblings, 1 reply; 14+ messages in thread
From: Mikulas Patocka @ 2020-09-25 12:04 UTC (permalink / raw)
To: Mike Snitzer; +Cc: dm-devel, Zdenek Kabelac
On Thu, 24 Sep 2020, Mike Snitzer wrote:
> On Thu, Sep 24 2020 at 2:12pm -0400,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
>
> >
> >
> > On Thu, 24 Sep 2020, Mikulas Patocka wrote:
> >
> > >
> > >
> > > On Thu, 24 Sep 2020, Mike Snitzer wrote:
> > >
> > > > WAIT... Could it be that raid_io_hints _really_ meant to special case
> > > > raid0 and raid10 -- due to their striping/splitting requirements!?
> > > > So, not raid1 but raid0?
> > > >
> > > > E.g.:
> > > >
> > > > diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
> > > > index 56b723d012ac..6dca932d6f1d 100644
> > > > --- a/drivers/md/dm-raid.c
> > > > +++ b/drivers/md/dm-raid.c
> > > > @@ -3730,10 +3730,10 @@ static void raid_io_hints(struct dm_target *ti,
> > > > struct queue_limits *limits)
> > > > blk_limits_io_opt(limits, chunk_size_bytes *
> > > > mddev_data_stripes(rs));
> > > >
> > > > /*
> > > > - * RAID1 and RAID10 personalities require bio splitting,
> > > > - * RAID0/4/5/6 don't and process large discard bios properly.
> > > > + * RAID0 and RAID10 personalities require bio splitting,
> > > > + * RAID1/4/5/6 don't and process large discard bios properly.
> > > > */
> > > > - if (rs_is_raid1(rs) || rs_is_raid10(rs)) {
> > > > + if (rs_is_raid0(rs) || rs_is_raid10(rs)) {
> > > > limits->discard_granularity = chunk_size_bytes;
> > > > limits->max_discard_sectors = rs->md.chunk_sectors;
> > > > }
> > > >
> > > > Mike
> > >
> > > Yes - that's an interesing point.
> > >
> > > Mikulas
> >
> > But raid0_handle_discard handles discards with arbitrary start/end
> > sectors.
> >
> > So, we don't need to set discard_granularity for that.
>
> OK, great, I've staged this:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-5.10&id=c1fda10e1123a37cf9d22740486cd66f43c47846
>
> Thanks,
> Mike
What if there are multiple targets for a device with different
discard_granularity - we would overwrite the settings made by another
target.
Should there be:
limits->discard_granularity = max(limits->discard_granularity, chunk_size_bytes);
or perhaps:
limits->discard_granularity = lcm_not_zero(limits->discard_granularity, chunk_size_bytes);
Mikulas
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: dm-raid: stack limits instead of overwriting them.
2020-09-25 12:04 ` Mikulas Patocka
@ 2020-09-25 13:20 ` Mike Snitzer
0 siblings, 0 replies; 14+ messages in thread
From: Mike Snitzer @ 2020-09-25 13:20 UTC (permalink / raw)
To: Mikulas Patocka; +Cc: dm-devel, Zdenek Kabelac
On Fri, Sep 25 2020 at 8:04am -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:
>
>
> On Thu, 24 Sep 2020, Mike Snitzer wrote:
>
> > On Thu, Sep 24 2020 at 2:12pm -0400,
> > Mikulas Patocka <mpatocka@redhat.com> wrote:
> >
> > >
> > >
> > > On Thu, 24 Sep 2020, Mikulas Patocka wrote:
> > >
> > > >
> > > >
> > > > On Thu, 24 Sep 2020, Mike Snitzer wrote:
> > > >
> > > > > WAIT... Could it be that raid_io_hints _really_ meant to special case
> > > > > raid0 and raid10 -- due to their striping/splitting requirements!?
> > > > > So, not raid1 but raid0?
> > > > >
> > > > > E.g.:
> > > > >
> > > > > diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
> > > > > index 56b723d012ac..6dca932d6f1d 100644
> > > > > --- a/drivers/md/dm-raid.c
> > > > > +++ b/drivers/md/dm-raid.c
> > > > > @@ -3730,10 +3730,10 @@ static void raid_io_hints(struct dm_target *ti,
> > > > > struct queue_limits *limits)
> > > > > blk_limits_io_opt(limits, chunk_size_bytes *
> > > > > mddev_data_stripes(rs));
> > > > >
> > > > > /*
> > > > > - * RAID1 and RAID10 personalities require bio splitting,
> > > > > - * RAID0/4/5/6 don't and process large discard bios properly.
> > > > > + * RAID0 and RAID10 personalities require bio splitting,
> > > > > + * RAID1/4/5/6 don't and process large discard bios properly.
> > > > > */
> > > > > - if (rs_is_raid1(rs) || rs_is_raid10(rs)) {
> > > > > + if (rs_is_raid0(rs) || rs_is_raid10(rs)) {
> > > > > limits->discard_granularity = chunk_size_bytes;
> > > > > limits->max_discard_sectors = rs->md.chunk_sectors;
> > > > > }
> > > > >
> > > > > Mike
> > > >
> > > > Yes - that's an interesing point.
> > > >
> > > > Mikulas
> > >
> > > But raid0_handle_discard handles discards with arbitrary start/end
> > > sectors.
> > >
> > > So, we don't need to set discard_granularity for that.
> >
> > OK, great, I've staged this:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-5.10&id=c1fda10e1123a37cf9d22740486cd66f43c47846
> >
> > Thanks,
> > Mike
>
> What if there are multiple targets for a device with different
> discard_granularity - we would overwrite the settings made by another
> target.
>
> Should there be:
>
> limits->discard_granularity = max(limits->discard_granularity, chunk_size_bytes);
>
> or perhaps:
>
> limits->discard_granularity = lcm_not_zero(limits->discard_granularity, chunk_size_bytes);
I'd go with max(). So I can fix up the stable@ patch to actually stack
the limits for raid10.
But it should be noted that there is this patch queued for Jens to pull
in (he actually _did_ pull the MD pull request but then rebased without
it):
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-5.10&id=828d14fd7a6cf
I haappened to have seized on it and will need to adjust given the
changes have been dropped at this point:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-5.10&id=dd29d4b556979dae3cb6460d019c36073af7a3fc
Mike
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [dm-devel] dm-raid: stack limits instead of overwriting them.
2020-09-24 18:56 ` John Dorminy
2020-09-24 19:15 ` Mike Snitzer
@ 2020-12-17 23:40 ` S. Baerwolf
1 sibling, 0 replies; 14+ messages in thread
From: S. Baerwolf @ 2020-12-17 23:40 UTC (permalink / raw)
To: dm-devel
Cc: device-mapper development, Mikulas Patocka, John Dorminy,
Mike Snitzer, Zdenek Kabelac
[-- Attachment #1: Type: text/plain, Size: 4442 bytes --]
Hi
I just saw your posts right now. (After posting my text "[PATCH] dm-raid: set discard_granularity non-zero if possible" yesterday)
How about iterating over all meta-devices corresponding to the raid1? Having mixed technology devices (TRIM and nonTRIM) I assume discard should be switched off? So using lcm_with_zero would be more suitable?
BR Stephan
On 24.09.20 18:56, John Dorminy wrote:
> On Thu, Sep 24, 2020 at 1:24 PM John Dorminy <jdorminy@redhat.com> wrote:
>>
>> I am impressed at how much I read wrong...
>>
>> On Thu, Sep 24, 2020 at 1:00 PM Mike Snitzer <snitzer@redhat.com> wrote:
>>>
>>> On Thu, Sep 24 2020 at 12:45pm -0400,
>>> John Dorminy <jdorminy@redhat.com> wrote:
>>>
>>>> I don't understand how this works...
>>>>
>>>> Can chunk_size_bytes be 0? If not, how is discard_granularity being set to 0?
>>>
>>> Yeah, I had same question.. see the reply I just sent in this thread:
>>> https://www.redhat.com/archives/dm-devel/2020-September/msg00568.html
>>>
>>>> I think also limits is local to the ti in question here, initialized
>>>> by blk_set_stacking_limits() via dm-table.c, and therefore has only
>>>> default values and not anything to do with the underlying queue. So
>>>> setting discard_granularity=max(discard_granularity, chunk_size_bytes)
>>>> doesn't seem like it should be working, unless I'm not understanding
>>>> what it's there for...
>>>
>>> You're reading the dm-table.c limits stacking wrong. Of course DM stack
>>> up the underlying device(s) limits ;)
>>
>> Yep, I failed to read iterate_devices...
>>
>>>
>>>>
>>>> And shouldn't melding in the target's desired io_hints into the
>>>> existing queue limits be happening in blk_stack_limits() instead?
>>>> (Also, it does lcm_not_zero() for stacking granularity, instead of
>>>> max()...)
>>>>
>>>
>>> DM core does do that, the .io_hints hook in the DM target is reserved
>>> for when the target has additional constraints that blk_stack_limits()
>>> didn't/couldn't factor in.
>> Yes, I had erroneously thought the limit-stacking was after getting
>> the targets' individual limits, not before.
>>
>>>
>>> And blk_stack_limts() does use max() for discard_granularity.
>> ... I'm just terrible at reading this morning.
>>
>> Thanks for pointing out all the things I misread!
>
> Actually, though, I don't understand why it should be max instead of
> lcm_not_zero(). If the raid's chunk size is 1024 sectors, say, and
> you're constructing it on something that has discard_granularity 812
> sectors, say, blkdev_issue_discard will be generating 1024 sector IOs
> which will work poorly when passed down to the 812-sector-granularity
> underlying device. While, if lcm(812,1024) were used, lcm(812,1024)
> sector IOs would be compatible with both the chunk size and underlying
> device's granularity, perhaps? Maybe I'm missing something, but I read
> the doc and code an extra time around this time ;)
>
>>
>>>
>>> Mike
>>>
>>>>
>>>> On Thu, Sep 24, 2020 at 12:29 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
>>>>>
>>>>> This patch fixes a warning WARN_ON_ONCE(!q->limits.discard_granularity).
>>>>> The reason is that the function raid_io_hints overwrote
>>>>> limits->discard_granularity with zero. We need to properly stack the
>>>>> limits instead of overwriting them.
>>>>>
>>>>> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
>>>>> Cc: stable@vger.kernel.org
>>>>>
>>>>> ---
>>>>> drivers/md/dm-raid.c | 4 ++--
>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> Index: linux-2.6/drivers/md/dm-raid.c
>>>>> ===================================================================
>>>>> --- linux-2.6.orig/drivers/md/dm-raid.c 2020-09-24 18:16:45.000000000 +0200
>>>>> +++ linux-2.6/drivers/md/dm-raid.c 2020-09-24 18:16:45.000000000 +0200
>>>>> @@ -3734,8 +3734,8 @@ static void raid_io_hints(struct dm_targ
>>>>> * RAID0/4/5/6 don't and process large discard bios properly.
>>>>> */
>>>>> if (rs_is_raid1(rs) || rs_is_raid10(rs)) {
>>>>> - limits->discard_granularity = chunk_size_bytes;
>>>>> - limits->max_discard_sectors = rs->md.chunk_sectors;
>>>>> + limits->discard_granularity = max(limits->discard_granularity, chunk_size_bytes);
>>>>> + limits->max_discard_sectors = min_not_zero(limits->max_discard_sectors, (unsigned)rs->md.chunk_sectors);
>>>>> }
>>>>> }
>>>>>
>>>>>
>>>>> --
>>>>> dm-devel mailing list
>>>>> dm-devel@redhat.com
>>>>> https://www.redhat.com/mailman/listinfo/dm-devel
>>>>>
>>>>
>>>
>
[-- Attachment #2: 0001-dm-raid-set-discard_granularity-non-zero-if-possible.patch --]
[-- Type: text/x-patch, Size: 1694 bytes --]
diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
index 8d2b835d7a10..4c769fd93ced 100644
--- a/drivers/md/dm-raid.c
+++ b/drivers/md/dm-raid.c
@@ -3734,8 +3734,36 @@ static void raid_io_hints(struct dm_target *ti, struct queue_limits *limits)
* RAID0/4/5/6 don't and process large discard bios properly.
*/
if (rs_is_raid1(rs) || rs_is_raid10(rs)) {
- limits->discard_granularity = chunk_size_bytes;
- limits->max_discard_sectors = rs->md.chunk_sectors;
+ /* HACK */
+ if (chunk_size_bytes==0) {
+ unsigned int i, chunk_sectors=(UINT_MAX >> SECTOR_SHIFT);
+ struct request_queue *q = NULL;
+
+ DMINFO("chunk_size is 0 for raid1 - preventing issue with TRIM");
+
+ for (i=0;i<rs->raid_disks;i++) {
+ q=bdev_get_queue(rs->dev[i].meta_dev->bdev);
+ if (chunk_sectors > q->limits.max_discard_sectors) {
+ chunk_sectors = q->limits.max_discard_sectors;
+ }
+ if (chunk_size_bytes < q->limits.discard_granularity) {
+ chunk_size_bytes = q->limits.discard_granularity;
+ }
+
+ /* lcm(x,y,...,0) = 0 */
+ if (q->limits.discard_granularity == 0) {
+ chunk_size_bytes = 0;
+ break;
+ }
+ }
+
+ limits->discard_granularity = chunk_size_bytes;
+ limits->max_discard_sectors = chunk_sectors;
+ /* end of HACK (but not of if) */
+ } else {
+ limits->discard_granularity = chunk_size_bytes;
+ limits->max_discard_sectors = rs->md.chunk_sectors;
+ }
}
}
[-- Attachment #3: Type: text/plain, Size: 93 bytes --]
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-12-17 23:47 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-24 16:26 [PATCH] dm-raid: stack limits instead of overwriting them Mikulas Patocka
2020-09-24 16:45 ` John Dorminy
2020-09-24 16:58 ` Mikulas Patocka
2020-09-24 17:00 ` Mike Snitzer
2020-09-24 17:24 ` John Dorminy
2020-09-24 18:56 ` John Dorminy
2020-09-24 19:15 ` Mike Snitzer
2020-12-17 23:40 ` [dm-devel] " S. Baerwolf
2020-09-24 16:56 ` Mike Snitzer
2020-09-24 17:07 ` Mikulas Patocka
2020-09-24 18:12 ` Mikulas Patocka
2020-09-24 19:07 ` Mike Snitzer
2020-09-25 12:04 ` Mikulas Patocka
2020-09-25 13:20 ` Mike Snitzer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).