| Message ID | 20190415082910.2073-1-fdmanana@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Btrfs: allocate ulists in btrfs_check_shared() using GFP_KERNEL | expand |
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>
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.
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.
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);