* [PATCH] xfs: use GFP_NOFS argument in radix_tree_preload @ 2015-03-23 5:06 Sanidhya Kashyap 2015-03-23 5:24 ` Dave Chinner 0 siblings, 1 reply; 4+ messages in thread From: Sanidhya Kashyap @ 2015-03-23 5:06 UTC (permalink / raw) To: david, xfs, linux-kernel Cc: taesoo, changwoo, sanidhya, blee, csong84, Sanidhya Kashyap From: Byoungyoung Lee <blee@gatech.edu> Following the convention of other file systems, GFP_NOFS should be used as an argument for radix_tree_preload() instead of GFP_KERNEL. Signed-off-by: Byoungyoung Lee <blee@gatech.edu> Signed-off-by: Sanidhya Kashyap <sanidhya.gatech@gmail.com> --- fs/xfs/xfs_mru_cache.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/xfs/xfs_mru_cache.c b/fs/xfs/xfs_mru_cache.c index 30ecca3..f8a674d 100644 --- a/fs/xfs/xfs_mru_cache.c +++ b/fs/xfs/xfs_mru_cache.c @@ -437,7 +437,7 @@ xfs_mru_cache_insert( if (!mru || !mru->lists) return -EINVAL; - if (radix_tree_preload(GFP_KERNEL)) + if (radix_tree_preload(GFP_NOFS)) return -ENOMEM; INIT_LIST_HEAD(&elem->list_node); -- 2.1.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] xfs: use GFP_NOFS argument in radix_tree_preload 2015-03-23 5:06 [PATCH] xfs: use GFP_NOFS argument in radix_tree_preload Sanidhya Kashyap @ 2015-03-23 5:24 ` Dave Chinner 2015-03-23 5:37 ` Taesoo Kim 0 siblings, 1 reply; 4+ messages in thread From: Dave Chinner @ 2015-03-23 5:24 UTC (permalink / raw) To: Sanidhya Kashyap Cc: xfs, linux-kernel, taesoo, changwoo, sanidhya, blee, csong84 On Mon, Mar 23, 2015 at 01:06:23AM -0400, Sanidhya Kashyap wrote: > From: Byoungyoung Lee <blee@gatech.edu> > > Following the convention of other file systems, GFP_NOFS > should be used as an argument for radix_tree_preload() instead > of GFP_KERNEL. "convention of other filesystems" is not a reason for changing from GFP_KERNEL to GFP_NOFS. There are rules for when GFP_NOFS needs to be used, and so we only need to change the code if one of those rules are triggered. i.e. inside a transaction, holding a lock that memory reclaim might require to make progress (e.g. ip->i_ilock, buffer locks, etc). The context in which the allocation is made will tell you whether GFP_KERNEL is safe or not. So while the change probably needs to be made, it needs to be made for the right reasons. I haven't looked at the code, but I have a pretty good idea of the context the allocation is being made under. I'd suggest documenting the call path down to xfs_mru_cache_insert(), because that will tell you exactly what context the allocaiton is being made in and hence tell everyone else the real reason we need to make this change... Call me picky, pendantic and/or annoying, but if you are looking at validating/correcting allocation flags then you need to understand the rules and context in which the allocation is being made... Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xfs: use GFP_NOFS argument in radix_tree_preload 2015-03-23 5:24 ` Dave Chinner @ 2015-03-23 5:37 ` Taesoo Kim 2015-03-23 7:23 ` Dave Chinner 0 siblings, 1 reply; 4+ messages in thread From: Taesoo Kim @ 2015-03-23 5:37 UTC (permalink / raw) To: Dave Chinner Cc: Sanidhya Kashyap, xfs, linux-kernel, changwoo, sanidhya, blee, csong84 Hi Dave, Thank you for letting us know. Since we are not an expert of XFS (nor want to be), we really want to let you guys know it's potential bug that you might miss (we are helping you!). And that's why Sanidhya asked (rather than sending a patch) at the first place. I agree that the comment is misleading and not correct, but probably encouraging a student who spend times to clean up your mistake might be better way to influence him rather than shouting :) Taesoo On 03/23/15 at 04:24pm, Dave Chinner wrote: > On Mon, Mar 23, 2015 at 01:06:23AM -0400, Sanidhya Kashyap wrote: > > From: Byoungyoung Lee <blee@gatech.edu> > > > > Following the convention of other file systems, GFP_NOFS > > should be used as an argument for radix_tree_preload() instead > > of GFP_KERNEL. > > "convention of other filesystems" is not a reason for changing from > GFP_KERNEL to GFP_NOFS. There are rules for when GFP_NOFS needs to > be used, and so we only need to change the code if one of those > rules are triggered. i.e. inside a transaction, holding a lock that > memory reclaim might require to make progress (e.g. ip->i_ilock, > buffer locks, etc). The context in which the allocation is made will > tell you whether GFP_KERNEL is safe or not. > > So while the change probably needs to be made, it needs to be made > for the right reasons. I haven't looked at the code, but I have > a pretty good idea of the context the allocation is being made > under. I'd suggest documenting the call path down to > xfs_mru_cache_insert(), because that will tell you exactly what > context the allocaiton is being made in and hence tell everyone else > the real reason we need to make this change... > > Call me picky, pendantic and/or annoying, but if you are looking at > validating/correcting allocation flags then you need to understand > the rules and context in which the allocation is being made... > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xfs: use GFP_NOFS argument in radix_tree_preload 2015-03-23 5:37 ` Taesoo Kim @ 2015-03-23 7:23 ` Dave Chinner 0 siblings, 0 replies; 4+ messages in thread From: Dave Chinner @ 2015-03-23 7:23 UTC (permalink / raw) To: Taesoo Kim Cc: Sanidhya Kashyap, xfs, linux-kernel, changwoo, sanidhya, blee, csong84 On Mon, Mar 23, 2015 at 01:37:15AM -0400, Taesoo Kim wrote: > Hi Dave, > > Thank you for letting us know. Since we are not an expert of XFS (nor > want to be), we really want to let you guys know it's potential bug > that you might miss (we are helping you!). And that's why Sanidhya > asked (rather than sending a patch) at the first place. The point of sending patches is that it's the best way to ask questions about the code because it first requires the patch submitter to think about the change and to document the reasons for making it. Many, many questions are answered in the process of making a change and writing a decent commit message. And, with it in patch format, if the change and reasoning is good, I can just commit it and move on. > I agree that the comment is misleading and not correct, but probably > encouraging a student who spend times to clean up your mistake > might be better way to influence him rather than shouting :) I think you've taken what I said the wrong way - I'm not sure how much more constructive I can be: > > So while the change probably needs to be made, it needs to be made > > for the right reasons. I haven't looked at the code, but I have > > a pretty good idea of the context the allocation is being made > > under. I'd suggest documenting the call path down to > > xfs_mru_cache_insert(), because that will tell you exactly what > > context the allocaiton is being made in and hence tell everyone else > > the real reason we need to make this change... What I've described is exactly how one goes about finding out whether the allocation context is correct or not, for any allocation in the kernel. Following the rules and techniques I outlined, it takes me less than 15s with cscope to work out whether the code in the patch is correct or not. However, fixing the commit message myself doesn't result in the patch submitter learning what they've done wrong or how to improve the work they've submitted. Fixing it myself is the easy option - it takes me far longer to write an email explaining how to validate the change the right way and document it. However, even though it costs me more time in the short term, I'd much prefer that people learn how to do things the right way because it means I don't end up having to answer the same question for every suspect allocation or fix up the same mistakes in patches over and over again. To further demonstrate I am trying to help, here's exactly what I'd be putting in a commit message explaining the change using methods regularly used by kernel developers to demonstrate context and errors. This is off the top of my head, so the call chain may not be 100% correct, but a commit message I would write for such change is along these lines: ---- xfs: xfs_mru_cache_insert() should use GFP_NOFS xfs_mru_cache_insert() can be called from within transaction context during block allocation like so: write(2) .... xfs_get_blocks xfs_iomap_write_direct start transaction xfs_bmapi_write xfs_bmapi_allocate xfs_bmap_btalloc xfs_bmap_btalloc_filestreams xfs_filestream_new_ag xfs_filestream_pick_ag xfs_mru_cache_insert radix_tree_preload(GFP_KERNEL) In this case, GFP_KERNEL is incorrect and can potentially lead to deadlocks in memory reclaim. It should use GFP_NOFS allocations to avoid lock recursion problems. Signed-off-by.... ----- i.e. explain the bug being fixed, not the convention used to find it. That's what I meant when I said "make the change for the right reasons". This is whay commit messages are important - they explain *why* the code was changed. Hence the code review process is not just about the code - the reviewers need to understand why the code is being changed and the process that lead to the change being proposed. The commit history is going to be the only record of why this code exists in 10 years time, so the explanation needs to be correct.... FWIW, if you want more background on what I'm trying to explain, I highly recommend watching my recent LCA 2015 talk: https://www.youtube.com/watch?v=VpuVDfSXs-g i.e. code review is a key knowledge transfer mechanism in software engineering.... Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-03-23 7:23 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-03-23 5:06 [PATCH] xfs: use GFP_NOFS argument in radix_tree_preload Sanidhya Kashyap 2015-03-23 5:24 ` Dave Chinner 2015-03-23 5:37 ` Taesoo Kim 2015-03-23 7:23 ` Dave Chinner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox