On Thu, Jun 02, 2016 at 04:56:06PM -0400, Jeff Mahoney wrote: > 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.
There's nothing here I disagree with. I'm not seeing a strong technical justification, which is what I want (being called from an ioctl means nothing in this case). --Mark -- Mark Fasheh -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html