| Message ID | 201711242103.FIH57396.FOFVJtQFLSMOHO@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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 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. -- 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
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.
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; }