On 6/2/16 3:08 PM, Mark Fasheh wrote:
> On Thu, Jun 02, 2016 at 07:07:32PM +0200, David Sterba wrote:
>> On Wed, Jun 01, 2016 at 02:15:22PM -0700, Mark Fasheh wrote:
>>>> +/* dynamically allocate and initialize a ref_root */
>>>> +static struct ref_root *ref_root_alloc(void)
>>>> +{
>>>> + struct ref_root *ref_tree;
>>>> +
>>>> + ref_tree = kmalloc(sizeof(*ref_tree), GFP_KERNEL);
>>>
>>> I'm pretty sure we want GFP_NOFS here.
>>
>> Then please explain to me why/where the reasoning below is wrong:
>
> The general reasoning of when to use GFP_NOFS below is fine, I don't
> disagree with that at all. If there is no way a recursion back into btrfs
> can't happen at that allocation site then we can use GFP_KERNEL.
>
> That said, have you closely audited this path? Does the allocation happen
> completely outside any locks that might be shared with the writeout path?
> What happens if we have to do writeout of the inode being fiemapped in order
> to allocate space? If the answer to all my questions is "there is no way
> this can deadlock" then by all means, we should use GFP_KERNEL. Otherwise
> GFP_NOFS is a sensible guard against possible future deadlocks.This is exactly the situation we discussed at LSF/MM this year. The MM folks are pushing back because the fs folks tend to use GFP_NOFS as a talisman. The audit needs to happen, otherwise that last sentence is another talisman. -Jeff -- Jeff Mahoney SUSE Labs
signature.asc
Description: OpenPGP digital signature
