DM-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [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; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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	[flat|nested] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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
  0 siblings, 1 reply; 13+ 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] 13+ 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; 13+ 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] 13+ 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
  0 siblings, 0 replies; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ messages in thread

end of thread, back to index

Thread overview: 13+ 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-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

DM-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dm-devel/0 dm-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dm-devel dm-devel/ https://lore.kernel.org/dm-devel \
		dm-devel@redhat.com
	public-inbox-index dm-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/com.redhat.dm-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git