[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAL3q7H6BQpWAtfkA+tnW-fBkdqSDb4udAeJvdgHfKSKfpYWAOA@mail.gmail.com>
Date: Thu, 2 Oct 2025 13:06:15 +0100
From: Filipe Manana <fdmanana@...nel.org>
To: rtapadia730@...il.com
Cc: dsterba@...e.com, clm@...com, fdmanana@...e.com,
linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org,
skhan@...uxfoundation.org, khalid@...nel.org, david.hunter.linux@...il.com
Subject: Re: [PATCH] btrfs: push memalloc_nofs_save/restore() out of alloc_bitmap()
On Thu, Oct 2, 2025 at 12:55 PM <rtapadia730@...il.com> wrote:
>
> From: Rajeev Tapadia <rtapadia730@...il.com>
>
> alloc_bitmap() currently wraps its allocation in memalloc_nofs_save
> /restore(), but this hides allocation context from callers. GFP_NOFS is
> required to avoid recursion into the filesystem during transaction commits,
Not just during transaction commits, but because in every context we
call alloc_bitmap() we are holding a transaction handle open, so if
the memory allocation recurses into the filesystem and triggers the
current transaction's commit, we deadlock - that's why we need NOFS.
> but the correct place to enforce that is at the call sites where we know
> recursion is unsafe.
>
> So now alloc_bitmap() just allocates a bitmap. Also completing the TODO
> comment.
No. Think for a moment...
alloc_bitmap() only has two callers, and in both of them we need a
NOFS allocation since we are holding a transaction handle...
So it only makes sense to leave the setup of NOFS in alloc_bitmap() -
do not move it to the callers and duplicate code...
Just fix the comment, removing the TODO and mention that the reason we
can't recurse is because we are holding a transaction handle open - it
doesn't matter if we are in critical section of a transaction commit
or not - all it matters is that we are holding a transaction handle
open, so a GFP_KERNEL allocation results in a deadlock if it recurses
to the filesystem to commit the transaction.
>
> Signed-off-by: Rajeev Tapadia <rtapadia730@...il.com>
> ---
>
> The patch was tested by enabling CONFIG_BTRFS_FS_RUN_SANITY_TESTS
> All tests passed while booting the kernel in qemu.
We test btrfs with fstests. The sanity tests aren't enough, they are
mostly to test internal functions.
>
> fs/btrfs/free-space-tree.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
> index dad0b492a663..abdbdc74edf8 100644
> --- a/fs/btrfs/free-space-tree.c
> +++ b/fs/btrfs/free-space-tree.c
> @@ -159,8 +159,6 @@ static inline u32 free_space_bitmap_size(const struct btrfs_fs_info *fs_info,
>
> static unsigned long *alloc_bitmap(u32 bitmap_size)
> {
> - unsigned long *ret;
> - unsigned int nofs_flag;
> u32 bitmap_rounded_size = round_up(bitmap_size, sizeof(unsigned long));
>
> /*
> @@ -168,13 +166,11 @@ static unsigned long *alloc_bitmap(u32 bitmap_size)
> * into the filesystem as the free space bitmap can be modified in the
> * critical section of a transaction commit.
> *
> - * TODO: push the memalloc_nofs_{save,restore}() to the caller where we
> - * know that recursion is unsafe.
> + * This function's caller is responsible for setting the appropriate
> + * allocation context (e.g., using memalloc_nofs_save/restore())
> + * to prevent recursion.
> */
> - nofs_flag = memalloc_nofs_save();
> - ret = kvzalloc(bitmap_rounded_size, GFP_KERNEL);
> - memalloc_nofs_restore(nofs_flag);
> - return ret;
> + return kvzalloc(bitmap_rounded_size, GFP_KERNEL);
> }
>
> static void le_bitmap_set(unsigned long *map, unsigned int start, int len)
> @@ -217,7 +213,9 @@ int btrfs_convert_free_space_to_bitmaps(struct btrfs_trans_handle *trans,
> int ret;
>
> bitmap_size = free_space_bitmap_size(fs_info, block_group->length);
> + unsigned int nofs_flag = memalloc_nofs_save();
Please don't declare variables in the middle of the code.
We always declare variables at the top of the current scope.
> bitmap = alloc_bitmap(bitmap_size);
> + memalloc_nofs_restore(nofs_flag);
> if (unlikely(!bitmap)) {
> ret = -ENOMEM;
> btrfs_abort_transaction(trans, ret);
> @@ -360,7 +358,9 @@ int btrfs_convert_free_space_to_extents(struct btrfs_trans_handle *trans,
> int ret;
>
> bitmap_size = free_space_bitmap_size(fs_info, block_group->length);
> + unsigned int nofs_flag = memalloc_nofs_save();
> bitmap = alloc_bitmap(bitmap_size);
> + memalloc_nofs_restore(nofs_flag);
> if (unlikely(!bitmap)) {
> ret = -ENOMEM;
> btrfs_abort_transaction(trans, ret);
> --
> 2.51.0
>
>
Powered by blists - more mailing lists