[PATCH] btrfs: push memalloc_nofs_save/restore() out of alloc_bitmap()

[PATCH] btrfs: push memalloc_nofs_save/restore() out of alloc_bitmap()

[rtapadia730]

From: Rajeev Tapadia <rtapadia730@gmail.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,
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.

Signed-off-by: Rajeev Tapadia <rtapadia730@gmail.com>
---

The patch was tested by enabling CONFIG_BTRFS_FS_RUN_SANITY_TESTS
All tests passed while booting the kernel in qemu.

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

Re: [PATCH] btrfs: push memalloc_nofs_save/restore() out of alloc_bitmap()

[Filipe Manana]

On Thu, Oct 2, 2025 at 12:55 PM <rtapadia730@gmail.com> wrote:
>
> From: Rajeev Tapadia <rtapadia730@gmail.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@gmail.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
>
>

Re: [PATCH] btrfs: push memalloc_nofs_save/restore() out of alloc_bitmap()

[rtapadia730]

Thank you for the review and explanation.


> We test btrfs with fstests. The sanity tests aren't enough, they are
> mostly to test internal functions.
> Please don't declare variables in the middle of the code.
> We always declare variables at the top of the current scope.
Okay I will remember this for future patches.


> 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.
I'll send a v2 patch that just removes TODO and update the comment for 
clearity.