* [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: [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 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: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: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-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
* 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: 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: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 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 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
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).