Btrfs: allocate ulists in btrfs_check_shared() using GFP_KERNEL
diff mbox series

Message ID 20190415082910.2073-1-fdmanana@kernel.org
State New
Headers show
Series
  • Btrfs: allocate ulists in btrfs_check_shared() using GFP_KERNEL
Related show

Commit Message

Filipe Manana April 15, 2019, 8:29 a.m. UTC
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(-)

Comments

David Sterba April 15, 2019, 2:35 p.m. UTC | #1
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>
Filipe Manana April 15, 2019, 2:44 p.m. UTC | #2
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.
David Sterba April 15, 2019, 2:58 p.m. UTC | #3
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.

Patch
diff mbox series

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);