* [PATCH 0/2] dm zoned fixes for 5.8 @ 2020-07-08 0:20 Damien Le Moal 2020-07-08 0:20 ` [PATCH 1/2] dm zoned: Fix zone reclaim trigger Damien Le Moal 2020-07-08 0:20 ` [PATCH 2/2] dm zoned: Remove set but unused variables Damien Le Moal 0 siblings, 2 replies; 7+ messages in thread From: Damien Le Moal @ 2020-07-08 0:20 UTC (permalink / raw) To: dm-devel, Mike Snitzer; +Cc: Shin'ichiro Kawasaki, Damien Le Moal Mike, A couple of fixes for dm-zoned for this cycle. The first patch fixes a possible hang under intensive write workload with a setup that has a very limited number of cache zones. The second patch fixes some compilation warnings showing when compiling with W=1. Damien Le Moal (2): dm zoned: Fix zone reclaim trigger dm zoned: Remove set but unused variables drivers/md/dm-zoned-metadata.c | 9 ++++++++- drivers/md/dm-zoned-reclaim.c | 7 +++---- drivers/md/dm-zoned-target.c | 10 +--------- 3 files changed, 12 insertions(+), 14 deletions(-) -- 2.26.2 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] dm zoned: Fix zone reclaim trigger 2020-07-08 0:20 [PATCH 0/2] dm zoned fixes for 5.8 Damien Le Moal @ 2020-07-08 0:20 ` Damien Le Moal 2020-07-08 7:06 ` Hannes Reinecke 2020-07-08 0:20 ` [PATCH 2/2] dm zoned: Remove set but unused variables Damien Le Moal 1 sibling, 1 reply; 7+ messages in thread From: Damien Le Moal @ 2020-07-08 0:20 UTC (permalink / raw) To: dm-devel, Mike Snitzer; +Cc: Shin'ichiro Kawasaki, Damien Le Moal Triggerring reclaim only based on the percentage of unmapped cache zones can fail to detect cases where reclaim is needed, e.g. if the target has only 2 or 3 cache zones and only one unmapped cache zone, the percentage of free cache zone is higher than DMZ_RECLAIM_LOW_UNMAP_ZONES (30%) and reclaim does not trigger. This problem, combined with the fact that dmz_schedule_reclaim() is called from dmz_handle_bio() without the map lock held leads to a race between zone allocation and dmz_should_reclaim() result. Depending on the workload applied, this race can lead to the write path forever waiting for a free zone without reclaim being triggerred. Fix this by moving dmz_schedule_reclaim() inside dmz_alloc_zone() under the map lock, checking the need for zone reclaim whenever a new data or buffer zone needs to be allocated. Also fix dmz_reclaim_percentage() to always return 0 if the number of unmapped cache (or random) zone is less than or equal to 1. Suggested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> --- drivers/md/dm-zoned-metadata.c | 9 ++++++++- drivers/md/dm-zoned-reclaim.c | 2 ++ drivers/md/dm-zoned-target.c | 10 +--------- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/drivers/md/dm-zoned-metadata.c b/drivers/md/dm-zoned-metadata.c index 5cf6f5f552e0..b298fefb022e 100644 --- a/drivers/md/dm-zoned-metadata.c +++ b/drivers/md/dm-zoned-metadata.c @@ -2217,8 +2217,15 @@ struct dm_zone *dmz_alloc_zone(struct dmz_metadata *zmd, unsigned int dev_idx, { struct list_head *list; struct dm_zone *zone; - int i = 0; + int i; + + /* Schedule reclaim to ensure free zones are available */ + if (!(flags & DMZ_ALLOC_RECLAIM)) { + for (i = 0; i < zmd->nr_devs; i++) + dmz_schedule_reclaim(zmd->dev[i].reclaim); + } + i = 0; again: if (flags & DMZ_ALLOC_CACHE) list = &zmd->unmap_cache_list; diff --git a/drivers/md/dm-zoned-reclaim.c b/drivers/md/dm-zoned-reclaim.c index dd1eebf6e50f..9c6e264465bc 100644 --- a/drivers/md/dm-zoned-reclaim.c +++ b/drivers/md/dm-zoned-reclaim.c @@ -456,6 +456,8 @@ static unsigned int dmz_reclaim_percentage(struct dmz_reclaim *zrc) nr_zones = dmz_nr_rnd_zones(zmd, zrc->dev_idx); nr_unmap = dmz_nr_unmap_rnd_zones(zmd, zrc->dev_idx); } + if (nr_unmap <= 1) + return 0; return nr_unmap * 100 / nr_zones; } diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c index cf915009c306..42aa5139df7c 100644 --- a/drivers/md/dm-zoned-target.c +++ b/drivers/md/dm-zoned-target.c @@ -400,15 +400,7 @@ static void dmz_handle_bio(struct dmz_target *dmz, struct dm_chunk_work *cw, dm_per_bio_data(bio, sizeof(struct dmz_bioctx)); struct dmz_metadata *zmd = dmz->metadata; struct dm_zone *zone; - int i, ret; - - /* - * Write may trigger a zone allocation. So make sure the - * allocation can succeed. - */ - if (bio_op(bio) == REQ_OP_WRITE) - for (i = 0; i < dmz->nr_ddevs; i++) - dmz_schedule_reclaim(dmz->dev[i].reclaim); + int ret; dmz_lock_metadata(zmd); -- 2.26.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] dm zoned: Fix zone reclaim trigger 2020-07-08 0:20 ` [PATCH 1/2] dm zoned: Fix zone reclaim trigger Damien Le Moal @ 2020-07-08 7:06 ` Hannes Reinecke 0 siblings, 0 replies; 7+ messages in thread From: Hannes Reinecke @ 2020-07-08 7:06 UTC (permalink / raw) To: Damien Le Moal, dm-devel, Mike Snitzer; +Cc: Shin'ichiro Kawasaki On 7/8/20 2:20 AM, Damien Le Moal wrote: > Triggerring reclaim only based on the percentage of unmapped cache > zones can fail to detect cases where reclaim is needed, e.g. if the > target has only 2 or 3 cache zones and only one unmapped cache zone, > the percentage of free cache zone is higher than > DMZ_RECLAIM_LOW_UNMAP_ZONES (30%) and reclaim does not trigger. > This problem, combined with the fact that dmz_schedule_reclaim() is > called from dmz_handle_bio() without the map lock held leads to a race > between zone allocation and dmz_should_reclaim() result. Depending on > the workload applied, this race can lead to the write path forever > waiting for a free zone without reclaim being triggerred. > > Fix this by moving dmz_schedule_reclaim() inside dmz_alloc_zone() > under the map lock, checking the need for zone reclaim whenever a new > data or buffer zone needs to be allocated. > > Also fix dmz_reclaim_percentage() to always return 0 if the number of > unmapped cache (or random) zone is less than or equal to 1. > > Suggested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > --- > drivers/md/dm-zoned-metadata.c | 9 ++++++++- > drivers/md/dm-zoned-reclaim.c | 2 ++ > drivers/md/dm-zoned-target.c | 10 +--------- > 3 files changed, 11 insertions(+), 10 deletions(-) > > diff --git a/drivers/md/dm-zoned-metadata.c b/drivers/md/dm-zoned-metadata.c > index 5cf6f5f552e0..b298fefb022e 100644 > --- a/drivers/md/dm-zoned-metadata.c > +++ b/drivers/md/dm-zoned-metadata.c > @@ -2217,8 +2217,15 @@ struct dm_zone *dmz_alloc_zone(struct dmz_metadata *zmd, unsigned int dev_idx, > { > struct list_head *list; > struct dm_zone *zone; > - int i = 0; > + int i; > + > + /* Schedule reclaim to ensure free zones are available */ > + if (!(flags & DMZ_ALLOC_RECLAIM)) { > + for (i = 0; i < zmd->nr_devs; i++) > + dmz_schedule_reclaim(zmd->dev[i].reclaim); > + } > > + i = 0; > again: > if (flags & DMZ_ALLOC_CACHE) > list = &zmd->unmap_cache_list; > diff --git a/drivers/md/dm-zoned-reclaim.c b/drivers/md/dm-zoned-reclaim.c > index dd1eebf6e50f..9c6e264465bc 100644 > --- a/drivers/md/dm-zoned-reclaim.c > +++ b/drivers/md/dm-zoned-reclaim.c > @@ -456,6 +456,8 @@ static unsigned int dmz_reclaim_percentage(struct dmz_reclaim *zrc) > nr_zones = dmz_nr_rnd_zones(zmd, zrc->dev_idx); > nr_unmap = dmz_nr_unmap_rnd_zones(zmd, zrc->dev_idx); > } > + if (nr_unmap <= 1) > + return 0; > return nr_unmap * 100 / nr_zones; > } > > diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c > index cf915009c306..42aa5139df7c 100644 > --- a/drivers/md/dm-zoned-target.c > +++ b/drivers/md/dm-zoned-target.c > @@ -400,15 +400,7 @@ static void dmz_handle_bio(struct dmz_target *dmz, struct dm_chunk_work *cw, > dm_per_bio_data(bio, sizeof(struct dmz_bioctx)); > struct dmz_metadata *zmd = dmz->metadata; > struct dm_zone *zone; > - int i, ret; > - > - /* > - * Write may trigger a zone allocation. So make sure the > - * allocation can succeed. > - */ > - if (bio_op(bio) == REQ_OP_WRITE) > - for (i = 0; i < dmz->nr_ddevs; i++) > - dmz_schedule_reclaim(dmz->dev[i].reclaim); > + int ret; > > dmz_lock_metadata(zmd); > > I seem to have run into this during my testing, too, but then as I'd arguably had programming errors at that time I didn't manage to recreate it. Thanks for tracking it down. Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking hare@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] dm zoned: Remove set but unused variables 2020-07-08 0:20 [PATCH 0/2] dm zoned fixes for 5.8 Damien Le Moal 2020-07-08 0:20 ` [PATCH 1/2] dm zoned: Fix zone reclaim trigger Damien Le Moal @ 2020-07-08 0:20 ` Damien Le Moal 2020-07-08 7:06 ` Hannes Reinecke 2020-07-08 14:12 ` Mike Snitzer 1 sibling, 2 replies; 7+ messages in thread From: Damien Le Moal @ 2020-07-08 0:20 UTC (permalink / raw) To: dm-devel, Mike Snitzer; +Cc: Shin'ichiro Kawasaki, Damien Le Moal In dmz_reclaim_work(), the variables nr_unmap_rnd and nr_rnd are set but unused. Remove them. Fixes: f97809aec589 ("dm zoned: per-device reclaim") Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> --- drivers/md/dm-zoned-reclaim.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/drivers/md/dm-zoned-reclaim.c b/drivers/md/dm-zoned-reclaim.c index 9c6e264465bc..9c0ecc9568a4 100644 --- a/drivers/md/dm-zoned-reclaim.c +++ b/drivers/md/dm-zoned-reclaim.c @@ -503,7 +503,7 @@ static void dmz_reclaim_work(struct work_struct *work) { struct dmz_reclaim *zrc = container_of(work, struct dmz_reclaim, work.work); struct dmz_metadata *zmd = zrc->metadata; - unsigned int p_unmap, nr_unmap_rnd = 0, nr_rnd = 0; + unsigned int p_unmap; int ret; if (dmz_dev_is_dying(zmd)) @@ -529,9 +529,6 @@ static void dmz_reclaim_work(struct work_struct *work) zrc->kc_throttle.throttle = min(75U, 100U - p_unmap / 2); } - nr_unmap_rnd = dmz_nr_unmap_rnd_zones(zmd, zrc->dev_idx); - nr_rnd = dmz_nr_rnd_zones(zmd, zrc->dev_idx); - DMDEBUG("(%s/%u): Reclaim (%u): %s, %u%% free zones (%u/%u cache %u/%u random)", dmz_metadata_label(zmd), zrc->dev_idx, zrc->kc_throttle.throttle, -- 2.26.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] dm zoned: Remove set but unused variables 2020-07-08 0:20 ` [PATCH 2/2] dm zoned: Remove set but unused variables Damien Le Moal @ 2020-07-08 7:06 ` Hannes Reinecke 2020-07-08 14:12 ` Mike Snitzer 1 sibling, 0 replies; 7+ messages in thread From: Hannes Reinecke @ 2020-07-08 7:06 UTC (permalink / raw) To: Damien Le Moal, dm-devel, Mike Snitzer; +Cc: Shin'ichiro Kawasaki On 7/8/20 2:20 AM, Damien Le Moal wrote: > In dmz_reclaim_work(), the variables nr_unmap_rnd and nr_rnd are set but > unused. Remove them. > > Fixes: f97809aec589 ("dm zoned: per-device reclaim") > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > --- > drivers/md/dm-zoned-reclaim.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/md/dm-zoned-reclaim.c b/drivers/md/dm-zoned-reclaim.c > index 9c6e264465bc..9c0ecc9568a4 100644 > --- a/drivers/md/dm-zoned-reclaim.c > +++ b/drivers/md/dm-zoned-reclaim.c > @@ -503,7 +503,7 @@ static void dmz_reclaim_work(struct work_struct *work) > { > struct dmz_reclaim *zrc = container_of(work, struct dmz_reclaim, work.work); > struct dmz_metadata *zmd = zrc->metadata; > - unsigned int p_unmap, nr_unmap_rnd = 0, nr_rnd = 0; > + unsigned int p_unmap; > int ret; > > if (dmz_dev_is_dying(zmd)) > @@ -529,9 +529,6 @@ static void dmz_reclaim_work(struct work_struct *work) > zrc->kc_throttle.throttle = min(75U, 100U - p_unmap / 2); > } > > - nr_unmap_rnd = dmz_nr_unmap_rnd_zones(zmd, zrc->dev_idx); > - nr_rnd = dmz_nr_rnd_zones(zmd, zrc->dev_idx); > - > DMDEBUG("(%s/%u): Reclaim (%u): %s, %u%% free zones (%u/%u cache %u/%u random)", > dmz_metadata_label(zmd), zrc->dev_idx, > zrc->kc_throttle.throttle, > Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking hare@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] dm zoned: Remove set but unused variables 2020-07-08 0:20 ` [PATCH 2/2] dm zoned: Remove set but unused variables Damien Le Moal 2020-07-08 7:06 ` Hannes Reinecke @ 2020-07-08 14:12 ` Mike Snitzer 2020-07-09 1:44 ` Damien Le Moal 1 sibling, 1 reply; 7+ messages in thread From: Mike Snitzer @ 2020-07-08 14:12 UTC (permalink / raw) To: Damien Le Moal; +Cc: Shin'ichiro Kawasaki, dm-devel On Tue, Jul 07 2020 at 8:20pm -0400, Damien Le Moal <damien.lemoal@wdc.com> wrote: > In dmz_reclaim_work(), the variables nr_unmap_rnd and nr_rnd are set but > unused. Remove them. > > Fixes: f97809aec589 ("dm zoned: per-device reclaim") > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > --- > drivers/md/dm-zoned-reclaim.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/md/dm-zoned-reclaim.c b/drivers/md/dm-zoned-reclaim.c > index 9c6e264465bc..9c0ecc9568a4 100644 > --- a/drivers/md/dm-zoned-reclaim.c > +++ b/drivers/md/dm-zoned-reclaim.c > @@ -503,7 +503,7 @@ static void dmz_reclaim_work(struct work_struct *work) > { > struct dmz_reclaim *zrc = container_of(work, struct dmz_reclaim, work.work); > struct dmz_metadata *zmd = zrc->metadata; > - unsigned int p_unmap, nr_unmap_rnd = 0, nr_rnd = 0; > + unsigned int p_unmap; > int ret; > > if (dmz_dev_is_dying(zmd)) > @@ -529,9 +529,6 @@ static void dmz_reclaim_work(struct work_struct *work) > zrc->kc_throttle.throttle = min(75U, 100U - p_unmap / 2); > } > > - nr_unmap_rnd = dmz_nr_unmap_rnd_zones(zmd, zrc->dev_idx); > - nr_rnd = dmz_nr_rnd_zones(zmd, zrc->dev_idx); > - > DMDEBUG("(%s/%u): Reclaim (%u): %s, %u%% free zones (%u/%u cache %u/%u random)", > dmz_metadata_label(zmd), zrc->dev_idx, > zrc->kc_throttle.throttle, > -- > 2.26.2 > I picked up the same change, that was submitted earlier, for 5.8: https://patchwork.kernel.org/patch/11641031/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] dm zoned: Remove set but unused variables 2020-07-08 14:12 ` Mike Snitzer @ 2020-07-09 1:44 ` Damien Le Moal 0 siblings, 0 replies; 7+ messages in thread From: Damien Le Moal @ 2020-07-09 1:44 UTC (permalink / raw) To: Mike Snitzer; +Cc: Shinichiro Kawasaki, dm-devel On 2020/07/09 0:14, Mike Snitzer wrote: > On Tue, Jul 07 2020 at 8:20pm -0400, > Damien Le Moal <damien.lemoal@wdc.com> wrote: > >> In dmz_reclaim_work(), the variables nr_unmap_rnd and nr_rnd are set but >> unused. Remove them. >> >> Fixes: f97809aec589 ("dm zoned: per-device reclaim") >> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> >> --- >> drivers/md/dm-zoned-reclaim.c | 5 +---- >> 1 file changed, 1 insertion(+), 4 deletions(-) >> >> diff --git a/drivers/md/dm-zoned-reclaim.c b/drivers/md/dm-zoned-reclaim.c >> index 9c6e264465bc..9c0ecc9568a4 100644 >> --- a/drivers/md/dm-zoned-reclaim.c >> +++ b/drivers/md/dm-zoned-reclaim.c >> @@ -503,7 +503,7 @@ static void dmz_reclaim_work(struct work_struct *work) >> { >> struct dmz_reclaim *zrc = container_of(work, struct dmz_reclaim, work.work); >> struct dmz_metadata *zmd = zrc->metadata; >> - unsigned int p_unmap, nr_unmap_rnd = 0, nr_rnd = 0; >> + unsigned int p_unmap; >> int ret; >> >> if (dmz_dev_is_dying(zmd)) >> @@ -529,9 +529,6 @@ static void dmz_reclaim_work(struct work_struct *work) >> zrc->kc_throttle.throttle = min(75U, 100U - p_unmap / 2); >> } >> >> - nr_unmap_rnd = dmz_nr_unmap_rnd_zones(zmd, zrc->dev_idx); >> - nr_rnd = dmz_nr_rnd_zones(zmd, zrc->dev_idx); >> - >> DMDEBUG("(%s/%u): Reclaim (%u): %s, %u%% free zones (%u/%u cache %u/%u random)", >> dmz_metadata_label(zmd), zrc->dev_idx, >> zrc->kc_throttle.throttle, >> -- >> 2.26.2 >> > > I picked up the same change, that was submitted earlier, for 5.8: > https://patchwork.kernel.org/patch/11641031/ Oops. Forgot about this one... Thanks ! > > -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-07-09 1:44 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-08 0:20 [PATCH 0/2] dm zoned fixes for 5.8 Damien Le Moal 2020-07-08 0:20 ` [PATCH 1/2] dm zoned: Fix zone reclaim trigger Damien Le Moal 2020-07-08 7:06 ` Hannes Reinecke 2020-07-08 0:20 ` [PATCH 2/2] dm zoned: Remove set but unused variables Damien Le Moal 2020-07-08 7:06 ` Hannes Reinecke 2020-07-08 14:12 ` Mike Snitzer 2020-07-09 1:44 ` Damien Le Moal
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.