Re: [RFC PATCH v3.2 5/6] btrfs: qgroup: Introduce extent changeset for qgroup reserve functions

[David Sterba <dsterba@suse.cz>]

On Thu, May 18, 2017 at 08:24:26AM +0800, Qu Wenruo wrote:
> >> +static inline void extent_changeset_init(struct extent_changeset *changeset)
> >> +{
> >> +	changeset->bytes_changed = 0;
> >> +	ulist_init(&changeset->range_changed);
> >> +}
> >> +
> >> +static inline struct extent_changeset *extent_changeset_alloc(void)
> >> +{
> >> +	struct extent_changeset *ret;
> >> +
> >> +	ret = kmalloc(sizeof(*ret), GFP_KERNEL);
> > 
> > I don't remember if we'd discussed this before, but have you evaluated
> > if GFP_KERNEL is ok to use in this context?
> 
> IIRC you have informed me that I shouldn't abuse GFP_NOFS.

Use of GFP_NOFS or _KERNEL has to be evaluated case by case. So if it's
"let's use NOFS because everybody else does" or "he said I should not
use NOFS, then I'll use KERNEL", then it's wrong and I'll complain.

A short notice in the changelog or a comment above the allocation would
better signify that the patch author spent some time thinking about the
consequences.

Sometimes it can become pretty hard to find the potential deadlock
scenarios. Using GFP_NOFS in such case is a matter of precaution, but at
least would be nice to be explictly stated somewhere.

The hard cases help to understand the callchain patterns and it's easier
to detect them in the future. For example, in your patch I already knew
that it's a problem when I saw lock_extent_bits, because I had seen this
pattern in a patch doing allocation in FIEMAP. Commit
afce772e87c36c7f07f230a76d525025aaf09e41, discussion in thread
http://lkml.kernel.org/r/1465362783-27078-1-git-send-email-lufq.fnst@cn.fujitsu.com