Re: [PATCH v2] xfs: handle register_shrinker error
On Tue 28-11-17 08:39:19, Darrick J. Wong wrote: > On Tue, Nov 28, 2017 at 10:35:51AM +0100, Michal Hocko wrote: [...] > > Are you going to apply the patch or should I re-send it with > > acks/reviewed-by? > > I injected all of those when I added the patch to my tree: > > Noticed-by: Tetsuo Handa > Signed-off-by: Michal Hocko > Acked-by: Dave Chinner > Reviewed-by: Darrick J. Wong > Signed-off-by: Darrick J. Wong Thanks! -- Michal Hocko SUSE Labs
Re: [PATCH v2] xfs: handle register_shrinker error
On Tue, Nov 28, 2017 at 10:35:51AM +0100, Michal Hocko wrote: > On Mon 27-11-17 09:44:53, Darrick J. Wong wrote: > [...] > > > From 1009db61988c48c9a9e327a9d076945b29b02eee Mon Sep 17 00:00:00 2001 > > > From: Michal Hocko > > > Date: Thu, 23 Nov 2017 17:13:40 +0100 > > > Subject: [PATCH] xfs: fortify xfs_alloc_buftarg error handling > > > > > > percpu_counter_init failure path doesn't clean up &btp->bt_lru list. > > > Call list_lru_destroy in that error path. Similarly register_shrinker > > > error path is not handled. > > > > > > While it is unlikely to trigger these error path, it is not impossible > > > especially the later might fail with large NUMAs. Let's handle the > > > failure to make the code more robust. > > > > > > Acked-by: Dave Chinner > > > Noticed-by: Tetsuo Handa > > > Signed-off-by: Michal Hocko > > > --- > > > fs/xfs/xfs_buf.c | 15 ++- > > > 1 file changed, 10 insertions(+), 5 deletions(-) > > > > > > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c > > > index 4db6e8d780f6..4c6e86d861fd 100644 > > > --- a/fs/xfs/xfs_buf.c > > > +++ b/fs/xfs/xfs_buf.c > > > @@ -1815,22 +1815,27 @@ xfs_alloc_buftarg( > > > btp->bt_daxdev = dax_dev; > > > > > > if (xfs_setsize_buftarg_early(btp, bdev)) > > > - goto error; > > > + goto error_free; > > > > > > if (list_lru_init(&btp->bt_lru)) > > > - goto error; > > > + goto error_free; > > > > > > if (percpu_counter_init(&btp->bt_io_count, 0, GFP_KERNEL)) > > > - goto error; > > > + goto error_lru; > > > > > > btp->bt_shrinker.count_objects = xfs_buftarg_shrink_count; > > > btp->bt_shrinker.scan_objects = xfs_buftarg_shrink_scan; > > > btp->bt_shrinker.seeks = DEFAULT_SEEKS; > > > btp->bt_shrinker.flags = SHRINKER_NUMA_AWARE; > > > - register_shrinker(&btp->bt_shrinker); > > > + if (register_shrinker(&btp->bt_shrinker)) > > > + goto error_pcpu; > > > return btp; > > > > > > -error: > > > +error_pcpu: > > > + percpu_counter_destroy(&btp->bt_io_count); > > > +error_lru: > > > + list_lru_destroy(&btp->bt_lru); > > > +error_free: > > > kmem_free(btp); > > > return NULL; > > > > This part looks ok, > > Reviewed-by: Darrick J. Wong > > Are you going to apply the patch or should I re-send it with > acks/reviewed-by? I injected all of those when I added the patch to my tree: Noticed-by: Tetsuo Handa Signed-off-by: Michal Hocko Acked-by: Dave Chinner Reviewed-by: Darrick J. Wong Signed-off-by: Darrick J. Wong --D > -- > Michal Hocko > SUSE Labs > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] xfs: handle register_shrinker error
On Mon 27-11-17 09:44:53, Darrick J. Wong wrote: [...] > > From 1009db61988c48c9a9e327a9d076945b29b02eee Mon Sep 17 00:00:00 2001 > > From: Michal Hocko > > Date: Thu, 23 Nov 2017 17:13:40 +0100 > > Subject: [PATCH] xfs: fortify xfs_alloc_buftarg error handling > > > > percpu_counter_init failure path doesn't clean up &btp->bt_lru list. > > Call list_lru_destroy in that error path. Similarly register_shrinker > > error path is not handled. > > > > While it is unlikely to trigger these error path, it is not impossible > > especially the later might fail with large NUMAs. Let's handle the > > failure to make the code more robust. > > > > Acked-by: Dave Chinner > > Noticed-by: Tetsuo Handa > > Signed-off-by: Michal Hocko > > --- > > fs/xfs/xfs_buf.c | 15 ++- > > 1 file changed, 10 insertions(+), 5 deletions(-) > > > > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c > > index 4db6e8d780f6..4c6e86d861fd 100644 > > --- a/fs/xfs/xfs_buf.c > > +++ b/fs/xfs/xfs_buf.c > > @@ -1815,22 +1815,27 @@ xfs_alloc_buftarg( > > btp->bt_daxdev = dax_dev; > > > > if (xfs_setsize_buftarg_early(btp, bdev)) > > - goto error; > > + goto error_free; > > > > if (list_lru_init(&btp->bt_lru)) > > - goto error; > > + goto error_free; > > > > if (percpu_counter_init(&btp->bt_io_count, 0, GFP_KERNEL)) > > - goto error; > > + goto error_lru; > > > > btp->bt_shrinker.count_objects = xfs_buftarg_shrink_count; > > btp->bt_shrinker.scan_objects = xfs_buftarg_shrink_scan; > > btp->bt_shrinker.seeks = DEFAULT_SEEKS; > > btp->bt_shrinker.flags = SHRINKER_NUMA_AWARE; > > - register_shrinker(&btp->bt_shrinker); > > + if (register_shrinker(&btp->bt_shrinker)) > > + goto error_pcpu; > > return btp; > > > > -error: > > +error_pcpu: > > + percpu_counter_destroy(&btp->bt_io_count); > > +error_lru: > > + list_lru_destroy(&btp->bt_lru); > > +error_free: > > kmem_free(btp); > > return NULL; > > This part looks ok, > Reviewed-by: Darrick J. Wong Are you going to apply the patch or should I re-send it with acks/reviewed-by? -- Michal Hocko SUSE Labs
Re: [PATCH v2] xfs: handle register_shrinker error
On Fri, Nov 24, 2017 at 08:39:57AM +0100, Michal Hocko wrote: > On Fri 24-11-17 09:00:46, Dave Chinner wrote: > > On Thu, Nov 23, 2017 at 05:11:37PM +0100, Michal Hocko wrote: > > > On Fri 24-11-17 01:01:10, Tetsuo Handa wrote: > > > > Michal Hocko wrote: > > > > > On Thu 23-11-17 05:26:33, Christoph Hellwig wrote: > > > > > > Looks good, > > > > > > > > > > > > Reviewed-by: Christoph Hellwig > > > > > > > > > > Thanks! > > > > > > > > > > > I can take a stab at the quota one. > > > > > > > > > > That would be really great! > > > > > > > > > Again, it does not look good. Since kmem_free() does only kvfree(), > > > > nothing will release memory allocated by list_lru_init(). > > > > > > Hmm, you are right. I have (blindly) followed the current code flow > > > which is wrong as well. The following should do the trick. Should I > > > split that into two patches? > > > > One is fine by me - if we're need to backport one fix, then we need > > to backport both :/ > > OK > > > > --- > > > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c > > > index dd0e18af990c..4c6e86d861fd 100644 > > > --- a/fs/xfs/xfs_buf.c > > > +++ b/fs/xfs/xfs_buf.c > > > @@ -1815,25 +1815,27 @@ xfs_alloc_buftarg( > > > btp->bt_daxdev = dax_dev; > > > > > > if (xfs_setsize_buftarg_early(btp, bdev)) > > > - goto error; > > > + goto error_free; > > > > > > if (list_lru_init(&btp->bt_lru)) > > > - goto error; > > > + goto error_free; > > > > > > if (percpu_counter_init(&btp->bt_io_count, 0, GFP_KERNEL)) > > > - goto error; > > > + goto error_lru; > > > > > > btp->bt_shrinker.count_objects = xfs_buftarg_shrink_count; > > > btp->bt_shrinker.scan_objects = xfs_buftarg_shrink_scan; > > > btp->bt_shrinker.seeks = DEFAULT_SEEKS; > > > btp->bt_shrinker.flags = SHRINKER_NUMA_AWARE; > > > - if (register_shrinker(&btp->bt_shrinker)) { > > > - percpu_counter_destroy(&btp->bt_io_count); > > > - goto error; > > > - } > > > + if (register_shrinker(&btp->bt_shrinker)) > > > + goto error_pcpu; > > > return btp; > > > > > > -error: > > > +error_pcpu: > > > + percpu_counter_destroy(&btp->bt_io_count); > > > +error_lru: > > > + list_lru_destroy(&btp->bt_lru); > > > +error_free: > > > kmem_free(btp); > > > return NULL; > > > > That should do the trick. > > > > Acked-by: Dave Chinner > > Thanks. Updated patch below > --- > From 1009db61988c48c9a9e327a9d076945b29b02eee Mon Sep 17 00:00:00 2001 > From: Michal Hocko > Date: Thu, 23 Nov 2017 17:13:40 +0100 > Subject: [PATCH] xfs: fortify xfs_alloc_buftarg error handling > > percpu_counter_init failure path doesn't clean up &btp->bt_lru list. > Call list_lru_destroy in that error path. Similarly register_shrinker > error path is not handled. > > While it is unlikely to trigger these error path, it is not impossible > especially the later might fail with large NUMAs. Let's handle the > failure to make the code more robust. > > Acked-by: Dave Chinner > Noticed-by: Tetsuo Handa > Signed-off-by: Michal Hocko > --- > fs/xfs/xfs_buf.c | 15 ++- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c > index 4db6e8d780f6..4c6e86d861fd 100644 > --- a/fs/xfs/xfs_buf.c > +++ b/fs/xfs/xfs_buf.c > @@ -1815,22 +1815,27 @@ xfs_alloc_buftarg( > btp->bt_daxdev = dax_dev; > > if (xfs_setsize_buftarg_early(btp, bdev)) > - goto error; > + goto error_free; > > if (list_lru_init(&btp->bt_lru)) > - goto error; > + goto error_free; > > if (percpu_counter_init(&btp->bt_io_count, 0, GFP_KERNEL)) > - goto error; > + goto error_lru; > > btp->bt_shrinker.count_objects = xfs_buftarg_shrink_count; > btp->bt_shrinker.scan_objects = xfs_buftarg_shrink_scan; > btp->bt_shrinker.seeks = DEFAULT_SEEKS; > btp->bt_shrinker.flags = SHRINKER_NUMA_AWARE; > - register_shrinker(&btp->bt_shrinker); > + if (register_shrinker(&btp->bt_shrinker)) > + goto error_pcpu; > return btp; > > -error: > +error_pcpu: > + percpu_counter_destroy(&btp->bt_io_count); > +error_lru: > + list_lru_destroy(&btp->bt_lru); > +error_free: > kmem_free(btp); > return NULL; This part looks ok, Reviewed-by: Darrick J. Wong > } > -- > 2.15.0 > > -- > Michal Hocko > SUSE Labs > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] xfs: handle register_shrinker error
On Sun 26-11-17 11:14:25, Tetsuo Handa wrote: > Dave Chinner wrote: > > IOWs, we don't actually need to touch this code, but if you really > > must, just remove the KM_NOFS tag. > > OK. Then, please remove KM_NOFS. GFP_KERNEL is safer than GFP_NOFS > in the sense that it won't cause OOM lockup due to unable to invoke > the OOM killer. I agree that we should remove nofs request if they are not really needed. But arguing your usual OOM lockup is (again) over exaggerating. As a rule of thumb, it is almost always better to have the full reclaim context rather than reduced one because the later one can influence other parts of the system as they might need to do more work. -- Michal Hocko SUSE Labs
Re: [PATCH v2] xfs: handle register_shrinker error
Dave Chinner wrote: > IOWs, we don't actually need to touch this code, but if you really > must, just remove the KM_NOFS tag. OK. Then, please remove KM_NOFS. GFP_KERNEL is safer than GFP_NOFS in the sense that it won't cause OOM lockup due to unable to invoke the OOM killer.
Re: [PATCH v2] xfs: handle register_shrinker error
On Fri, Nov 24, 2017 at 09:03:28PM +0900, Tetsuo Handa wrote: > Michal Hocko wrote: > > Thanks. Updated patch below > > --- > > From 1009db61988c48c9a9e327a9d076945b29b02eee Mon Sep 17 00:00:00 2001 > > From: Michal Hocko > > Date: Thu, 23 Nov 2017 17:13:40 +0100 > > Subject: [PATCH] xfs: fortify xfs_alloc_buftarg error handling > > Do we need below patch on top of Michal's patch? > KM_NOFS was added by commit b17cb364dbbbf65a ("xfs: fix missing KM_NOFS > tags to keep lockdep happy"). If not needed, some comment is expected. Quite frankly, if the fix is "sprinkle magic undocumented memalloc_nofs_save() calls around", then you need to think a little more about the things you just read and the context we're operating on here. IOWs: > --- > fs/xfs/xfs_buf.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c > index 4c6e86d..b73fc76 100644 > --- a/fs/xfs/xfs_buf.c > +++ b/fs/xfs/xfs_buf.c > @@ -1806,6 +1806,7 @@ struct xfs_buf * > struct dax_device *dax_dev) > { > xfs_buftarg_t *btp; > + unsigned int nofs_flag = memalloc_nofs_save(); > > btp = kmem_zalloc(sizeof(*btp), KM_SLEEP | KM_NOFS); xfs_alloc_buftarg() isn't called from transaction context, so this KM_NOFS flag wasn't added to prevent reclaim deadlocks - it was added to avoid stupid lockdep false positives (as was stated in the commit you quoted). IOWs, GFP_KERNEL allocations in this function used to trigger lockdep false positives. So - think for a minute rather than bashing on the keyboard. Why aren't the other GFP_KERNEL allocations from this function causing lockdep to trigger warnings? Yeah - lockdep is a lot smarter these days and the false positive trigger has clearly been fixed. i.e. there's no false positive detection occurring here any more under GFP_KERNEL allocations, so we don't need the KM_NOFS flag anymore. IOWs, we don't actually need to touch this code, but if you really must, just remove the KM_NOFS tag. -Dave. -- Dave Chinner da...@fromorbit.com
Re: [PATCH v2] xfs: handle register_shrinker error
On Fri 24-11-17 21:03:28, Tetsuo Handa wrote: > Michal Hocko wrote: > > Thanks. Updated patch below > > --- > > From 1009db61988c48c9a9e327a9d076945b29b02eee Mon Sep 17 00:00:00 2001 > > From: Michal Hocko > > Date: Thu, 23 Nov 2017 17:13:40 +0100 > > Subject: [PATCH] xfs: fortify xfs_alloc_buftarg error handling > > Do we need below patch on top of Michal's patch? > KM_NOFS was added by commit b17cb364dbbbf65a ("xfs: fix missing KM_NOFS > tags to keep lockdep happy"). If not needed, some comment is expected. I am not not familiar with the code but git blame says that the whole point of NOFS here was to make lockdep happy. We do have means to silence the warning if the original concern still applies. -- Michal Hocko SUSE Labs
Re: [PATCH v2] xfs: handle register_shrinker error
Michal Hocko wrote: > Thanks. Updated patch below > --- > From 1009db61988c48c9a9e327a9d076945b29b02eee Mon Sep 17 00:00:00 2001 > From: Michal Hocko > Date: Thu, 23 Nov 2017 17:13:40 +0100 > Subject: [PATCH] xfs: fortify xfs_alloc_buftarg error handling Do we need below patch on top of Michal's patch? KM_NOFS was added by commit b17cb364dbbbf65a ("xfs: fix missing KM_NOFS tags to keep lockdep happy"). If not needed, some comment is expected. --- fs/xfs/xfs_buf.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c index 4c6e86d..b73fc76 100644 --- a/fs/xfs/xfs_buf.c +++ b/fs/xfs/xfs_buf.c @@ -1806,6 +1806,7 @@ struct xfs_buf * struct dax_device *dax_dev) { xfs_buftarg_t *btp; + unsigned int nofs_flag = memalloc_nofs_save(); btp = kmem_zalloc(sizeof(*btp), KM_SLEEP | KM_NOFS); @@ -1829,6 +1830,7 @@ struct xfs_buf * btp->bt_shrinker.flags = SHRINKER_NUMA_AWARE; if (register_shrinker(&btp->bt_shrinker)) goto error_pcpu; + memalloc_nofs_restore(nofs_flag); return btp; error_pcpu: @@ -1837,6 +1839,7 @@ struct xfs_buf * list_lru_destroy(&btp->bt_lru); error_free: kmem_free(btp); + memalloc_nofs_restore(nofs_flag); return NULL; } -- 1.8.3.1
[PATCH v2] xfs: handle register_shrinker error
On Fri 24-11-17 09:00:46, Dave Chinner wrote: > On Thu, Nov 23, 2017 at 05:11:37PM +0100, Michal Hocko wrote: > > On Fri 24-11-17 01:01:10, Tetsuo Handa wrote: > > > Michal Hocko wrote: > > > > On Thu 23-11-17 05:26:33, Christoph Hellwig wrote: > > > > > Looks good, > > > > > > > > > > Reviewed-by: Christoph Hellwig > > > > > > > > Thanks! > > > > > > > > > I can take a stab at the quota one. > > > > > > > > That would be really great! > > > > > > > Again, it does not look good. Since kmem_free() does only kvfree(), > > > nothing will release memory allocated by list_lru_init(). > > > > Hmm, you are right. I have (blindly) followed the current code flow > > which is wrong as well. The following should do the trick. Should I > > split that into two patches? > > One is fine by me - if we're need to backport one fix, then we need > to backport both :/ OK > > --- > > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c > > index dd0e18af990c..4c6e86d861fd 100644 > > --- a/fs/xfs/xfs_buf.c > > +++ b/fs/xfs/xfs_buf.c > > @@ -1815,25 +1815,27 @@ xfs_alloc_buftarg( > > btp->bt_daxdev = dax_dev; > > > > if (xfs_setsize_buftarg_early(btp, bdev)) > > - goto error; > > + goto error_free; > > > > if (list_lru_init(&btp->bt_lru)) > > - goto error; > > + goto error_free; > > > > if (percpu_counter_init(&btp->bt_io_count, 0, GFP_KERNEL)) > > - goto error; > > + goto error_lru; > > > > btp->bt_shrinker.count_objects = xfs_buftarg_shrink_count; > > btp->bt_shrinker.scan_objects = xfs_buftarg_shrink_scan; > > btp->bt_shrinker.seeks = DEFAULT_SEEKS; > > btp->bt_shrinker.flags = SHRINKER_NUMA_AWARE; > > - if (register_shrinker(&btp->bt_shrinker)) { > > - percpu_counter_destroy(&btp->bt_io_count); > > - goto error; > > - } > > + if (register_shrinker(&btp->bt_shrinker)) > > + goto error_pcpu; > > return btp; > > > > -error: > > +error_pcpu: > > + percpu_counter_destroy(&btp->bt_io_count); > > +error_lru: > > + list_lru_destroy(&btp->bt_lru); > > +error_free: > > kmem_free(btp); > > return NULL; > > That should do the trick. > > Acked-by: Dave Chinner Thanks. Updated patch below --- >From 1009db61988c48c9a9e327a9d076945b29b02eee Mon Sep 17 00:00:00 2001 From: Michal Hocko Date: Thu, 23 Nov 2017 17:13:40 +0100 Subject: [PATCH] xfs: fortify xfs_alloc_buftarg error handling percpu_counter_init failure path doesn't clean up &btp->bt_lru list. Call list_lru_destroy in that error path. Similarly register_shrinker error path is not handled. While it is unlikely to trigger these error path, it is not impossible especially the later might fail with large NUMAs. Let's handle the failure to make the code more robust. Acked-by: Dave Chinner Noticed-by: Tetsuo Handa Signed-off-by: Michal Hocko --- fs/xfs/xfs_buf.c | 15 ++- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c index 4db6e8d780f6..4c6e86d861fd 100644 --- a/fs/xfs/xfs_buf.c +++ b/fs/xfs/xfs_buf.c @@ -1815,22 +1815,27 @@ xfs_alloc_buftarg( btp->bt_daxdev = dax_dev; if (xfs_setsize_buftarg_early(btp, bdev)) - goto error; + goto error_free; if (list_lru_init(&btp->bt_lru)) - goto error; + goto error_free; if (percpu_counter_init(&btp->bt_io_count, 0, GFP_KERNEL)) - goto error; + goto error_lru; btp->bt_shrinker.count_objects = xfs_buftarg_shrink_count; btp->bt_shrinker.scan_objects = xfs_buftarg_shrink_scan; btp->bt_shrinker.seeks = DEFAULT_SEEKS; btp->bt_shrinker.flags = SHRINKER_NUMA_AWARE; - register_shrinker(&btp->bt_shrinker); + if (register_shrinker(&btp->bt_shrinker)) + goto error_pcpu; return btp; -error: +error_pcpu: + percpu_counter_destroy(&btp->bt_io_count); +error_lru: + list_lru_destroy(&btp->bt_lru); +error_free: kmem_free(btp); return NULL; } -- 2.15.0 -- Michal Hocko SUSE Labs