xfs: use GFP_NOFS argument in radix_tree_preload

From: Byoungyoung Lee <[email protected]>

Following the convention of other file systems, GFP_NOFS should be used as an argument for radix_tree_preload() instead of GFP_KERNEL.

---
 fs/xfs/xfs_mru_cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 30ecca3..f8a674d 100644
@@ -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

"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
[email protected]

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

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

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 think you've taken what I said the wrong way - I'm not sure how much more constructive I can be:

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
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs