[PATCH 0/2] btrfs: relocation bug fix and a cleanup

[PATCH 0/2] btrfs: relocation bug fix and a cleanup

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Details in the change logs.

Filipe Manana (2):
  btrfs: fix clearing of BTRFS_FS_RELOC_RUNNING if relocation already running
  btrfs: use single return value variable in btrfs_relocate_block_group()

 fs/btrfs/relocation.c | 51 +++++++++++++++++++------------------------
 1 file changed, 22 insertions(+), 29 deletions(-)

-- 
2.47.2

[PATCH 1/2] btrfs: fix clearing of BTRFS_FS_RELOC_RUNNING if relocation already running

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

When starting relocation, at reloc_chunk_start(), if we happen to find
the flag BTRFS_FS_RELOC_RUNNING is already set we return an error
(-EINPROGRESS) to the callers, however the callers call reloc_chunk_end()
which will clear the flag BTRFS_FS_RELOC_RUNNING, which is wrong since
relocation was started by another task and still running.

Finding the BTRFS_FS_RELOC_RUNNING flag already set is an unexpected
scenario, but still our current behaviour is not correct.

Fix this by never calling reloc_chunk_end() if reloc_chunk_start() has
returned an error, which is what logically makes sense, since the general
widespread pattern is to have end functions called only if the counterpart
start functions succeeded. This requires changing reloc_chunk_start() to
clear BTRFS_FS_RELOC_RUNNING if there's a pending cancel request.

Fixes: 907d2710d727 ("btrfs: add cancellable chunk relocation support")
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/relocation.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 8dd8de6b9fb8..acce4d813153 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -3780,6 +3780,7 @@ static noinline_for_stack struct inode *create_reloc_inode(
 /*
  * Mark start of chunk relocation that is cancellable. Check if the cancellation
  * has been requested meanwhile and don't start in that case.
+ * NOTE: if this returns an error, reloc_chunk_end() must not be called.
  *
  * Return:
  *   0             success
@@ -3796,10 +3797,8 @@ static int reloc_chunk_start(struct btrfs_fs_info *fs_info)
 
 	if (atomic_read(&fs_info->reloc_cancel_req) > 0) {
 		btrfs_info(fs_info, "chunk relocation canceled on start");
-		/*
-		 * On cancel, clear all requests but let the caller mark
-		 * the end after cleanup operations.
-		 */
+		/* On cancel, clear all requests. */
+		clear_and_wake_up_bit(BTRFS_FS_RELOC_RUNNING, &fs_info->flags);
 		atomic_set(&fs_info->reloc_cancel_req, 0);
 		return -ECANCELED;
 	}
@@ -3808,6 +3807,7 @@ static int reloc_chunk_start(struct btrfs_fs_info *fs_info)
 
 /*
  * Mark end of chunk relocation that is cancellable and wake any waiters.
+ * NOTE: call only if a previous call to reloc_chunk_start() succeeded.
  */
 static void reloc_chunk_end(struct btrfs_fs_info *fs_info)
 {
@@ -4023,9 +4023,9 @@ int btrfs_relocate_block_group(struct btrfs_fs_info *fs_info, u64 group_start,
 	if (err && rw)
 		btrfs_dec_block_group_ro(rc->block_group);
 	iput(rc->data_inode);
+	reloc_chunk_end(fs_info);
 out_put_bg:
 	btrfs_put_block_group(bg);
-	reloc_chunk_end(fs_info);
 	free_reloc_control(rc);
 	return err;
 }
@@ -4208,8 +4208,8 @@ int btrfs_recover_relocation(struct btrfs_fs_info *fs_info)
 		ret = ret2;
 out_unset:
 	unset_reloc_control(rc);
-out_end:
 	reloc_chunk_end(fs_info);
+out_end:
 	free_reloc_control(rc);
 out:
 	free_reloc_roots(&reloc_roots);
-- 
2.47.2

[PATCH 2/2] btrfs: use single return value variable in btrfs_relocate_block_group()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

We are using 'ret' and 'err' variables to track return values and errors,
which is pattern that is error prone and we had quite some bugs due to
this pattern in the past.

Simplify this and use a single variable, named 'ret', to track errors and
the return value.

Also rename the variable 'rw' to 'bg_is_ro' which is more meaningful name,
and change its type from int to bool.

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

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index acce4d813153..16f625aaaf69 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -3881,8 +3881,7 @@ int btrfs_relocate_block_group(struct btrfs_fs_info *fs_info, u64 group_start,
 	struct inode *inode;
 	struct btrfs_path *path;
 	int ret;
-	int rw = 0;
-	int err = 0;
+	bool bg_is_ro = false;
 
 	/*
 	 * This only gets set if we had a half-deleted snapshot on mount.  We
@@ -3924,24 +3923,20 @@ int btrfs_relocate_block_group(struct btrfs_fs_info *fs_info, u64 group_start,
 	}
 
 	ret = reloc_chunk_start(fs_info);
-	if (ret < 0) {
-		err = ret;
+	if (ret < 0)
 		goto out_put_bg;
-	}
 
 	rc->extent_root = extent_root;
 	rc->block_group = bg;
 
 	ret = btrfs_inc_block_group_ro(rc->block_group, true);
-	if (ret) {
-		err = ret;
+	if (ret)
 		goto out;
-	}
-	rw = 1;
+	bg_is_ro = true;
 
 	path = btrfs_alloc_path();
 	if (!path) {
-		err = -ENOMEM;
+		ret = -ENOMEM;
 		goto out;
 	}
 
@@ -3953,14 +3948,12 @@ int btrfs_relocate_block_group(struct btrfs_fs_info *fs_info, u64 group_start,
 	else
 		ret = PTR_ERR(inode);
 
-	if (ret && ret != -ENOENT) {
-		err = ret;
+	if (ret && ret != -ENOENT)
 		goto out;
-	}
 
 	rc->data_inode = create_reloc_inode(rc->block_group);
 	if (IS_ERR(rc->data_inode)) {
-		err = PTR_ERR(rc->data_inode);
+		ret = PTR_ERR(rc->data_inode);
 		rc->data_inode = NULL;
 		goto out;
 	}
@@ -3981,8 +3974,6 @@ int btrfs_relocate_block_group(struct btrfs_fs_info *fs_info, u64 group_start,
 		mutex_lock(&fs_info->cleaner_mutex);
 		ret = relocate_block_group(rc);
 		mutex_unlock(&fs_info->cleaner_mutex);
-		if (ret < 0)
-			err = ret;
 
 		finishes_stage = rc->stage;
 		/*
@@ -3995,16 +3986,18 @@ int btrfs_relocate_block_group(struct btrfs_fs_info *fs_info, u64 group_start,
 		 * out of the loop if we hit an error.
 		 */
 		if (rc->stage == MOVE_DATA_EXTENTS && rc->found_file_extent) {
-			ret = btrfs_wait_ordered_range(BTRFS_I(rc->data_inode), 0,
-						       (u64)-1);
-			if (ret)
-				err = ret;
+			int wb_ret;
+
+			wb_ret = btrfs_wait_ordered_range(BTRFS_I(rc->data_inode), 0,
+							  (u64)-1);
+			if (wb_ret && ret == 0)
+				ret = wb_ret;
 			invalidate_mapping_pages(rc->data_inode->i_mapping,
 						 0, -1);
 			rc->stage = UPDATE_DATA_PTRS;
 		}
 
-		if (err < 0)
+		if (ret < 0)
 			goto out;
 
 		if (rc->extents_found == 0)
@@ -4020,14 +4013,14 @@ int btrfs_relocate_block_group(struct btrfs_fs_info *fs_info, u64 group_start,
 	WARN_ON(rc->block_group->reserved > 0);
 	WARN_ON(rc->block_group->used > 0);
 out:
-	if (err && rw)
+	if (ret && bg_is_ro)
 		btrfs_dec_block_group_ro(rc->block_group);
 	iput(rc->data_inode);
 	reloc_chunk_end(fs_info);
 out_put_bg:
 	btrfs_put_block_group(bg);
 	free_reloc_control(rc);
-	return err;
+	return ret;
 }
 
 static noinline_for_stack int mark_garbage_root(struct btrfs_root *root)
-- 
2.47.2

Re: [PATCH 0/2] btrfs: relocation bug fix and a cleanup

[Boris Burkov]

On Wed, Sep 24, 2025 at 05:54:54PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Details in the change logs.

Reviewed-by: Boris Burkov <boris@bur.io>

> 
> Filipe Manana (2):
>   btrfs: fix clearing of BTRFS_FS_RELOC_RUNNING if relocation already running
>   btrfs: use single return value variable in btrfs_relocate_block_group()
> 
>  fs/btrfs/relocation.c | 51 +++++++++++++++++++------------------------
>  1 file changed, 22 insertions(+), 29 deletions(-)
> 
> -- 
> 2.47.2
> 

Re: [PATCH 1/2] btrfs: fix clearing of BTRFS_FS_RELOC_RUNNING if relocation already running

[Johannes Thumshirn]

On 9/24/25 6:55 PM, fdmanana@kernel.org wrote:
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index 8dd8de6b9fb8..acce4d813153 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -3780,6 +3780,7 @@ static noinline_for_stack struct inode *create_reloc_inode(
>   /*
>    * Mark start of chunk relocation that is cancellable. Check if the cancellation
>    * has been requested meanwhile and don't start in that case.
> + * NOTE: if this returns an error, reloc_chunk_end() must not be called.
>    *
>    * Return:
>    *   0             success
> @@ -3796,10 +3797,8 @@ static int reloc_chunk_start(struct btrfs_fs_info *fs_info)
>   
>   	if (atomic_read(&fs_info->reloc_cancel_req) > 0) {
>   		btrfs_info(fs_info, "chunk relocation canceled on start");
> -		/*
> -		 * On cancel, clear all requests but let the caller mark
> -		 * the end after cleanup operations.
> -		 */
> +		/* On cancel, clear all requests. */
> +		clear_and_wake_up_bit(BTRFS_FS_RELOC_RUNNING, &fs_info->flags);
>   		atomic_set(&fs_info->reloc_cancel_req, 0);
>   		return -ECANCELED;

Would it make sense to add an ASSERT(test_bit(BRTFS_FS_RELOC_RUNNING, 
&fs_info->flags)); in reloc_chunk_end()?

Re: [PATCH 0/2] btrfs: relocation bug fix and a cleanup

[Johannes Thumshirn]

On 9/24/25 6:55 PM, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> Details in the change logs.
>
> Filipe Manana (2):
>    btrfs: fix clearing of BTRFS_FS_RELOC_RUNNING if relocation already running
>    btrfs: use single return value variable in btrfs_relocate_block_group()
>
>   fs/btrfs/relocation.c | 51 +++++++++++++++++++------------------------
>   1 file changed, 22 insertions(+), 29 deletions(-)
>
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 0/2] btrfs: relocation bug fix and a cleanup

[Qu Wenruo]

在 2025/9/25 02:24, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Details in the change logs.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> 
> Filipe Manana (2):
>    btrfs: fix clearing of BTRFS_FS_RELOC_RUNNING if relocation already running
>    btrfs: use single return value variable in btrfs_relocate_block_group()
> 
>   fs/btrfs/relocation.c | 51 +++++++++++++++++++------------------------
>   1 file changed, 22 insertions(+), 29 deletions(-)
> 

Re: [PATCH 1/2] btrfs: fix clearing of BTRFS_FS_RELOC_RUNNING if relocation already running

[Filipe Manana]

On Thu, Sep 25, 2025 at 6:54 AM Johannes Thumshirn
<Johannes.Thumshirn@wdc.com> wrote:
>
> On 9/24/25 6:55 PM, fdmanana@kernel.org wrote:
> > diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> > index 8dd8de6b9fb8..acce4d813153 100644
> > --- a/fs/btrfs/relocation.c
> > +++ b/fs/btrfs/relocation.c
> > @@ -3780,6 +3780,7 @@ static noinline_for_stack struct inode *create_reloc_inode(
> >   /*
> >    * Mark start of chunk relocation that is cancellable. Check if the cancellation
> >    * has been requested meanwhile and don't start in that case.
> > + * NOTE: if this returns an error, reloc_chunk_end() must not be called.
> >    *
> >    * Return:
> >    *   0             success
> > @@ -3796,10 +3797,8 @@ static int reloc_chunk_start(struct btrfs_fs_info *fs_info)
> >
> >       if (atomic_read(&fs_info->reloc_cancel_req) > 0) {
> >               btrfs_info(fs_info, "chunk relocation canceled on start");
> > -             /*
> > -              * On cancel, clear all requests but let the caller mark
> > -              * the end after cleanup operations.
> > -              */
> > +             /* On cancel, clear all requests. */
> > +             clear_and_wake_up_bit(BTRFS_FS_RELOC_RUNNING, &fs_info->flags);
> >               atomic_set(&fs_info->reloc_cancel_req, 0);
> >               return -ECANCELED;
>
> Would it make sense to add an ASSERT(test_bit(BRTFS_FS_RELOC_RUNNING,
> &fs_info->flags)); in reloc_chunk_end()?

Yes, that's good to have.
I've added it and pushed it to for-next.

Thanks.