[PATCH] dm-raid: stack limits instead of overwriting them.

[PATCH] dm-raid: stack limits instead of overwriting them.

[Mikulas Patocka]

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);
 	}
 }
 

Re: [PATCH] dm-raid: stack limits instead of overwriting them.

[John Dorminy]

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
>

Re: dm-raid: stack limits instead of overwriting them.

[Mike Snitzer]

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

Re: [PATCH] dm-raid: stack limits instead of overwriting them.

[Mikulas Patocka]

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

Re: dm-raid: stack limits instead of overwriting them.

[Mike Snitzer]

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

Re: dm-raid: stack limits instead of overwriting them.

[Mikulas Patocka]

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

Re: dm-raid: stack limits instead of overwriting them.

[John Dorminy]

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

Re: dm-raid: stack limits instead of overwriting them.

[Mikulas Patocka]

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

Re: dm-raid: stack limits instead of overwriting them.

[John Dorminy]

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

Re: dm-raid: stack limits instead of overwriting them.

[Mike Snitzer]

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

Re: dm-raid: stack limits instead of overwriting them.

[Mike Snitzer]

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

Re: dm-raid: stack limits instead of overwriting them.

[Mikulas Patocka]

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

Re: dm-raid: stack limits instead of overwriting them.

[Mike Snitzer]

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

Re: [dm-devel] dm-raid: stack limits instead of overwriting them.

[S. Baerwolf]

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