[PATCH] Btrfs: allocate ulists in btrfs_check_shared() using GFP_KERNEL

[PATCH] Btrfs: allocate ulists in btrfs_check_shared() using GFP_KERNEL

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

As of commit bb739cf08e8f32 ("btrfs: btrfs_check_shared should manage its
own transaction") btrfs_check_shared() manages its own transaction, it
no longer receives a transaction handle as parameter. Since it is used
only in the fiemap path, the ulists it needs to use are allocated before
it grabs a transaction handle and we are not holding any locks that could
cause a deadlock in case reclaim happens during a memory allocation, we
can use the GFP_KERNEL flag to allocate the ulists instead of GFP_NOFS.
So do that switch.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/backref.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index 876e6bb93797..377978bb8845 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -1481,8 +1481,8 @@ int btrfs_check_shared(struct btrfs_root *root, u64 inum, u64 bytenr)
 		.share_count = 0,
 	};
 
-	tmp = ulist_alloc(GFP_NOFS);
-	roots = ulist_alloc(GFP_NOFS);
+	tmp = ulist_alloc(GFP_KERNEL);
+	roots = ulist_alloc(GFP_KERNEL);
 	if (!tmp || !roots) {
 		ulist_free(tmp);
 		ulist_free(roots);
-- 
2.11.0

Re: [PATCH] Btrfs: allocate ulists in btrfs_check_shared() using GFP_KERNEL

[David Sterba]

On Mon, Apr 15, 2019 at 09:29:10AM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> As of commit bb739cf08e8f32 ("btrfs: btrfs_check_shared should manage its
> own transaction") btrfs_check_shared() manages its own transaction, it
> no longer receives a transaction handle as parameter. Since it is used
> only in the fiemap path, the ulists it needs to use are allocated before
> it grabs a transaction handle and we are not holding any locks that could
> cause a deadlock in case reclaim happens during a memory allocation, we
> can use the GFP_KERNEL flag to allocate the ulists instead of GFP_NOFS.
> So do that switch.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: David Sterba <dsterba@suse.com>

Re: [PATCH] Btrfs: allocate ulists in btrfs_check_shared() using GFP_KERNEL

[Filipe Manana]

On Mon, Apr 15, 2019 at 3:33 PM David Sterba <dsterba@suse.cz> wrote:
>
> On Mon, Apr 15, 2019 at 09:29:10AM +0100, fdmanana@kernel.org wrote:
> > From: Filipe Manana <fdmanana@suse.com>
> >
> > As of commit bb739cf08e8f32 ("btrfs: btrfs_check_shared should manage its
> > own transaction") btrfs_check_shared() manages its own transaction, it
> > no longer receives a transaction handle as parameter. Since it is used
> > only in the fiemap path, the ulists it needs to use are allocated before
> > it grabs a transaction handle and we are not holding any locks that could
> > cause a deadlock in case reclaim happens during a memory allocation, we
> > can use the GFP_KERNEL flag to allocate the ulists instead of GFP_NOFS.
> > So do that switch.
> >
> > Signed-off-by: Filipe Manana <fdmanana@suse.com>
>
> Reviewed-by: David Sterba <dsterba@suse.com>

Actually no, this isn't safe, drop it please.
An inode's range is locked, the allocations can trigger writeback for
that inode, and writepages() needs to lock the range as well.

Thanks.

Re: [PATCH] Btrfs: allocate ulists in btrfs_check_shared() using GFP_KERNEL

[David Sterba]

On Mon, Apr 15, 2019 at 02:44:19PM +0000, Filipe Manana wrote:
> On Mon, Apr 15, 2019 at 3:33 PM David Sterba <dsterba@suse.cz> wrote:
> >
> > On Mon, Apr 15, 2019 at 09:29:10AM +0100, fdmanana@kernel.org wrote:
> > > From: Filipe Manana <fdmanana@suse.com>
> > >
> > > As of commit bb739cf08e8f32 ("btrfs: btrfs_check_shared should manage its
> > > own transaction") btrfs_check_shared() manages its own transaction, it
> > > no longer receives a transaction handle as parameter. Since it is used
> > > only in the fiemap path, the ulists it needs to use are allocated before
> > > it grabs a transaction handle and we are not holding any locks that could
> > > cause a deadlock in case reclaim happens during a memory allocation, we
> > > can use the GFP_KERNEL flag to allocate the ulists instead of GFP_NOFS.
> > > So do that switch.
> > >
> > > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> >
> > Reviewed-by: David Sterba <dsterba@suse.com>
> 
> Actually no, this isn't safe, drop it please.
> An inode's range is locked, the allocations can trigger writeback for
> that inode, and writepages() needs to lock the range as well.

Ah, right, lock_extent_bits. I think the ulist allocations can be moved
outside of the section to extent_fiemap, each call of btrfs_check_shared
allocates and frees.