* [PATCH] xfs: handle register_shrinker error @ 2017-11-23 12:08 Michal Hocko 2017-11-23 13:26 ` Christoph Hellwig 0 siblings, 1 reply; 18+ messages in thread From: Michal Hocko @ 2017-11-23 12:08 UTC (permalink / raw) To: Darrick J. Wong; +Cc: Dave Chinner, Tetsuo Handa, linux-xfs, LKML, Michal Hocko From: Michal Hocko <mhocko@suse.com> xfs_alloc_buftarg doesn't handle register_shrinker error path. While it is unlikely to trigger it is not impossible especially with large NUMAs. Let's handle the failure to make the code more robust. Signed-off-by: Michal Hocko <mhocko@suse.com> --- Hi, this is not tested but it looks quite straightforward. There is one more unchecked register_shrinker in xfs_qm_init_quotainfo but my absolute lack of familiarity with the code didn't allow me to come up with a mechanical fix like this one. fs/xfs/xfs_buf.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c index 4db6e8d780f6..dd0e18af990c 100644 --- a/fs/xfs/xfs_buf.c +++ b/fs/xfs/xfs_buf.c @@ -1827,7 +1827,10 @@ xfs_alloc_buftarg( 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)) { + percpu_counter_destroy(&btp->bt_io_count); + goto error; + } return btp; error: -- 2.15.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] xfs: handle register_shrinker error 2017-11-23 12:08 [PATCH] xfs: handle register_shrinker error Michal Hocko @ 2017-11-23 13:26 ` Christoph Hellwig 2017-11-23 13:41 ` Michal Hocko 0 siblings, 1 reply; 18+ messages in thread From: Christoph Hellwig @ 2017-11-23 13:26 UTC (permalink / raw) To: Michal Hocko Cc: Darrick J. Wong, Dave Chinner, Tetsuo Handa, linux-xfs, LKML, Michal Hocko Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> I can take a stab at the quota one. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] xfs: handle register_shrinker error 2017-11-23 13:26 ` Christoph Hellwig @ 2017-11-23 13:41 ` Michal Hocko 2017-11-23 16:01 ` Tetsuo Handa 0 siblings, 1 reply; 18+ messages in thread From: Michal Hocko @ 2017-11-23 13:41 UTC (permalink / raw) To: Christoph Hellwig Cc: Darrick J. Wong, Dave Chinner, Tetsuo Handa, linux-xfs, LKML On Thu 23-11-17 05:26:33, Christoph Hellwig wrote: > Looks good, > > Reviewed-by: Christoph Hellwig <hch@lst.de> Thanks! > I can take a stab at the quota one. That would be really great! -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] xfs: handle register_shrinker error 2017-11-23 13:41 ` Michal Hocko @ 2017-11-23 16:01 ` Tetsuo Handa 2017-11-23 16:11 ` Michal Hocko 0 siblings, 1 reply; 18+ messages in thread From: Tetsuo Handa @ 2017-11-23 16:01 UTC (permalink / raw) To: mhocko, hch; +Cc: darrick.wong, david, linux-xfs, linux-kernel Michal Hocko wrote: > On Thu 23-11-17 05:26:33, Christoph Hellwig wrote: > > Looks good, > > > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > 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(). ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] xfs: handle register_shrinker error 2017-11-23 16:01 ` Tetsuo Handa @ 2017-11-23 16:11 ` Michal Hocko 2017-11-23 16:17 ` Tetsuo Handa 2017-11-23 22:00 ` Dave Chinner 0 siblings, 2 replies; 18+ messages in thread From: Michal Hocko @ 2017-11-23 16:11 UTC (permalink / raw) To: Tetsuo Handa; +Cc: hch, darrick.wong, david, linux-xfs, linux-kernel 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 <hch@lst.de> > > > > 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? --- 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; } -- Michal Hocko SUSE Labs ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] xfs: handle register_shrinker error 2017-11-23 16:11 ` Michal Hocko @ 2017-11-23 16:17 ` Tetsuo Handa 2017-11-23 16:31 ` Michal Hocko 2017-11-23 22:00 ` Dave Chinner 1 sibling, 1 reply; 18+ messages in thread From: Tetsuo Handa @ 2017-11-23 16:17 UTC (permalink / raw) To: mhocko; +Cc: hch, darrick.wong, david, linux-xfs, linux-kernel Michal Hocko wrote: > 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? Well, xfs_alloc_buftarg() needs to be more careful. xfs_alloc_buftarg( struct xfs_mount *mp, struct block_device *bdev, struct dax_device *dax_dev) { xfs_buftarg_t *btp; btp = kmem_zalloc(sizeof(*btp), KM_SLEEP | KM_NOFS); // This is GFP_NOFS context. But... btp->bt_mount = mp; btp->bt_dev = bdev->bd_dev; btp->bt_bdev = bdev; btp->bt_daxdev = dax_dev; if (xfs_setsize_buftarg_early(btp, bdev)) goto error; if (list_lru_init(&btp->bt_lru)) // This is GFP_KERNEL context. goto error; if (percpu_counter_init(&btp->bt_io_count, 0, GFP_KERNEL)) // This is GFP_KERNEL context. goto error; // Need to undo list_lru_init() before kmem_free(). 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); // This is GFP_KERNEL context. return btp; error: kmem_free(btp); return NULL; } ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] xfs: handle register_shrinker error 2017-11-23 16:17 ` Tetsuo Handa @ 2017-11-23 16:31 ` Michal Hocko 0 siblings, 0 replies; 18+ messages in thread From: Michal Hocko @ 2017-11-23 16:31 UTC (permalink / raw) To: Tetsuo Handa; +Cc: hch, darrick.wong, david, linux-xfs, linux-kernel On Fri 24-11-17 01:17:12, Tetsuo Handa wrote: > Michal Hocko wrote: > > 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? > > Well, xfs_alloc_buftarg() needs to be more careful. [...] > btp = kmem_zalloc(sizeof(*btp), KM_SLEEP | KM_NOFS); // This is GFP_NOFS context. But... [...] > if (list_lru_init(&btp->bt_lru)) // This is GFP_KERNEL context. this sounds like a separate thing to cleanup or document. -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] xfs: handle register_shrinker error 2017-11-23 16:11 ` Michal Hocko 2017-11-23 16:17 ` Tetsuo Handa @ 2017-11-23 22:00 ` Dave Chinner 2017-11-24 7:39 ` [PATCH v2] " Michal Hocko 1 sibling, 1 reply; 18+ messages in thread From: Dave Chinner @ 2017-11-23 22:00 UTC (permalink / raw) To: Michal Hocko; +Cc: Tetsuo Handa, hch, darrick.wong, linux-xfs, linux-kernel 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 <hch@lst.de> > > > > > > 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 :/ > --- > 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 <dchinner@redhat.com> Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2] xfs: handle register_shrinker error 2017-11-23 22:00 ` Dave Chinner @ 2017-11-24 7:39 ` Michal Hocko 2017-11-24 12:03 ` Tetsuo Handa 2017-11-27 17:44 ` Darrick J. Wong 0 siblings, 2 replies; 18+ messages in thread From: Michal Hocko @ 2017-11-24 7:39 UTC (permalink / raw) To: Dave Chinner; +Cc: Tetsuo Handa, hch, darrick.wong, linux-xfs, linux-kernel 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 <hch@lst.de> > > > > > > > > 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 <dchinner@redhat.com> Thanks. Updated patch below --- >From 1009db61988c48c9a9e327a9d076945b29b02eee Mon Sep 17 00:00:00 2001 From: Michal Hocko <mhocko@suse.com> 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 <dchinner@redhat.com> Noticed-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Signed-off-by: Michal Hocko <mhocko@suse.com> --- 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2] xfs: handle register_shrinker error 2017-11-24 7:39 ` [PATCH v2] " Michal Hocko @ 2017-11-24 12:03 ` Tetsuo Handa 2017-11-24 12:09 ` Michal Hocko 2017-11-25 23:34 ` Dave Chinner 2017-11-27 17:44 ` Darrick J. Wong 1 sibling, 2 replies; 18+ messages in thread From: Tetsuo Handa @ 2017-11-24 12:03 UTC (permalink / raw) To: mhocko, david; +Cc: hch, darrick.wong, linux-xfs, linux-kernel Michal Hocko wrote: > Thanks. Updated patch below > --- > From 1009db61988c48c9a9e327a9d076945b29b02eee Mon Sep 17 00:00:00 2001 > From: Michal Hocko <mhocko@suse.com> > 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2] xfs: handle register_shrinker error 2017-11-24 12:03 ` Tetsuo Handa @ 2017-11-24 12:09 ` Michal Hocko 2017-11-25 23:34 ` Dave Chinner 1 sibling, 0 replies; 18+ messages in thread From: Michal Hocko @ 2017-11-24 12:09 UTC (permalink / raw) To: Tetsuo Handa; +Cc: david, hch, darrick.wong, linux-xfs, linux-kernel 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 <mhocko@suse.com> > > 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] xfs: handle register_shrinker error 2017-11-24 12:03 ` Tetsuo Handa 2017-11-24 12:09 ` Michal Hocko @ 2017-11-25 23:34 ` Dave Chinner 2017-11-26 2:14 ` Tetsuo Handa 1 sibling, 1 reply; 18+ messages in thread From: Dave Chinner @ 2017-11-25 23:34 UTC (permalink / raw) To: Tetsuo Handa; +Cc: mhocko, hch, darrick.wong, linux-xfs, linux-kernel 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 <mhocko@suse.com> > > 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 david@fromorbit.com ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] xfs: handle register_shrinker error 2017-11-25 23:34 ` Dave Chinner @ 2017-11-26 2:14 ` Tetsuo Handa 2017-11-27 8:05 ` Michal Hocko 0 siblings, 1 reply; 18+ messages in thread From: Tetsuo Handa @ 2017-11-26 2:14 UTC (permalink / raw) To: david; +Cc: mhocko, hch, darrick.wong, linux-xfs, linux-kernel 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] xfs: handle register_shrinker error 2017-11-26 2:14 ` Tetsuo Handa @ 2017-11-27 8:05 ` Michal Hocko 0 siblings, 0 replies; 18+ messages in thread From: Michal Hocko @ 2017-11-27 8:05 UTC (permalink / raw) To: Tetsuo Handa; +Cc: david, hch, darrick.wong, linux-xfs, linux-kernel 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] xfs: handle register_shrinker error 2017-11-24 7:39 ` [PATCH v2] " Michal Hocko 2017-11-24 12:03 ` Tetsuo Handa @ 2017-11-27 17:44 ` Darrick J. Wong 2017-11-28 9:35 ` Michal Hocko 1 sibling, 1 reply; 18+ messages in thread From: Darrick J. Wong @ 2017-11-27 17:44 UTC (permalink / raw) To: Michal Hocko; +Cc: Dave Chinner, Tetsuo Handa, hch, linux-xfs, linux-kernel 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 <hch@lst.de> > > > > > > > > > > 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 <dchinner@redhat.com> > > Thanks. Updated patch below > --- > From 1009db61988c48c9a9e327a9d076945b29b02eee Mon Sep 17 00:00:00 2001 > From: Michal Hocko <mhocko@suse.com> > 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 <dchinner@redhat.com> > Noticed-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Signed-off-by: Michal Hocko <mhocko@suse.com> > --- > 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 <darrick.wong@oracle.com> > } > -- > 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] xfs: handle register_shrinker error 2017-11-27 17:44 ` Darrick J. Wong @ 2017-11-28 9:35 ` Michal Hocko 2017-11-28 16:39 ` Darrick J. Wong 0 siblings, 1 reply; 18+ messages in thread From: Michal Hocko @ 2017-11-28 9:35 UTC (permalink / raw) To: Darrick J. Wong; +Cc: Dave Chinner, Tetsuo Handa, hch, linux-xfs, linux-kernel On Mon 27-11-17 09:44:53, Darrick J. Wong wrote: [...] > > From 1009db61988c48c9a9e327a9d076945b29b02eee Mon Sep 17 00:00:00 2001 > > From: Michal Hocko <mhocko@suse.com> > > 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 <dchinner@redhat.com> > > Noticed-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > > Signed-off-by: Michal Hocko <mhocko@suse.com> > > --- > > 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 <darrick.wong@oracle.com> Are you going to apply the patch or should I re-send it with acks/reviewed-by? -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] xfs: handle register_shrinker error 2017-11-28 9:35 ` Michal Hocko @ 2017-11-28 16:39 ` Darrick J. Wong 2017-11-28 19:40 ` Michal Hocko 0 siblings, 1 reply; 18+ messages in thread From: Darrick J. Wong @ 2017-11-28 16:39 UTC (permalink / raw) To: Michal Hocko; +Cc: Dave Chinner, Tetsuo Handa, hch, linux-xfs, linux-kernel 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 <mhocko@suse.com> > > > 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 <dchinner@redhat.com> > > > Noticed-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > > > Signed-off-by: Michal Hocko <mhocko@suse.com> > > > --- > > > 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 <darrick.wong@oracle.com> > > 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 <penguin-kernel@I-love.SAKURA.ne.jp> Signed-off-by: Michal Hocko <mhocko@suse.com> Acked-by: Dave Chinner <dchinner@redhat.com> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> --D > -- > Michal Hocko > SUSE Labs > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] xfs: handle register_shrinker error 2017-11-28 16:39 ` Darrick J. Wong @ 2017-11-28 19:40 ` Michal Hocko 0 siblings, 0 replies; 18+ messages in thread From: Michal Hocko @ 2017-11-28 19:40 UTC (permalink / raw) To: Darrick J. Wong; +Cc: Dave Chinner, Tetsuo Handa, hch, linux-xfs, linux-kernel 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 <penguin-kernel@I-love.SAKURA.ne.jp> > Signed-off-by: Michal Hocko <mhocko@suse.com> > Acked-by: Dave Chinner <dchinner@redhat.com> > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> Thanks! -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2017-11-28 19:40 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-11-23 12:08 [PATCH] xfs: handle register_shrinker error Michal Hocko 2017-11-23 13:26 ` Christoph Hellwig 2017-11-23 13:41 ` Michal Hocko 2017-11-23 16:01 ` Tetsuo Handa 2017-11-23 16:11 ` Michal Hocko 2017-11-23 16:17 ` Tetsuo Handa 2017-11-23 16:31 ` Michal Hocko 2017-11-23 22:00 ` Dave Chinner 2017-11-24 7:39 ` [PATCH v2] " Michal Hocko 2017-11-24 12:03 ` Tetsuo Handa 2017-11-24 12:09 ` Michal Hocko 2017-11-25 23:34 ` Dave Chinner 2017-11-26 2:14 ` Tetsuo Handa 2017-11-27 8:05 ` Michal Hocko 2017-11-27 17:44 ` Darrick J. Wong 2017-11-28 9:35 ` Michal Hocko 2017-11-28 16:39 ` Darrick J. Wong 2017-11-28 19:40 ` Michal Hocko
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.