[PATCH] btrfs: abort transaction in do_remap_reloc_trans() on failure

[PATCH] btrfs: abort transaction in do_remap_reloc_trans() on failure

[Mark Harmstone]

If one of the calls made by do_remap_reloc_trans() fails, we can leave
the remap tree in an inconsistent state. Abort the transaction if this
happens, to prevent the corrupt state from reaching the disk.

Signed-off-by: Mark Harmstone <mark@harmstone.com>
---
 fs/btrfs/relocation.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 1c42c5180bddd5..277a0042967665 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -5089,6 +5089,7 @@ static int do_remap_reloc_trans(struct btrfs_fs_info *fs_info,
 	btrfs_free_reserved_extent(fs_info, new_addr, length, 0);
 
 	mutex_unlock(&fs_info->remap_mutex);
+	btrfs_abort_transaction(trans, ret);
 	btrfs_end_transaction(trans);
 
 	return ret;
-- 
2.52.0

Re: [PATCH] btrfs: abort transaction in do_remap_reloc_trans() on failure

[Boris Burkov]

On Tue, Apr 07, 2026 at 05:16:56PM +0100, Mark Harmstone wrote:
> If one of the calls made by do_remap_reloc_trans() fails, we can leave
> the remap tree in an inconsistent state. Abort the transaction if this
> happens, to prevent the corrupt state from reaching the disk.
> 
> Signed-off-by: Mark Harmstone <mark@harmstone.com>
> ---
>  fs/btrfs/relocation.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index 1c42c5180bddd5..277a0042967665 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -5089,6 +5089,7 @@ static int do_remap_reloc_trans(struct btrfs_fs_info *fs_info,
>  	btrfs_free_reserved_extent(fs_info, new_addr, length, 0);
>  
>  	mutex_unlock(&fs_info->remap_mutex);
> +	btrfs_abort_transaction(trans, ret);

Counterintuitively to the usual pattern of sharing code in fail: labels,
I think it is quite nice to put a separate abort at each failing code
path that requires an abort. That way we get better diagnostic
information on what actually went wrong.

e.g. a recent patch from Filipe unfolding it across a function call:
https://lore.kernel.org/linux-btrfs/99743ac0b4b9a69dcaa8f9adcaf301bd1a0f1a89.1769163248.git.fdmanana@suse.com/

Thanks,
Boris

>  	btrfs_end_transaction(trans);
>  
>  	return ret;
> -- 
> 2.52.0
> 

Re: [PATCH] btrfs: abort transaction in do_remap_reloc_trans() on failure

[Mark Harmstone]

On 07/04/2026 10.31 pm, Boris Burkov wrote:
> On Tue, Apr 07, 2026 at 05:16:56PM +0100, Mark Harmstone wrote:
>> If one of the calls made by do_remap_reloc_trans() fails, we can leave
>> the remap tree in an inconsistent state. Abort the transaction if this
>> happens, to prevent the corrupt state from reaching the disk.
>>
>> Signed-off-by: Mark Harmstone <mark@harmstone.com>
>> ---
>>   fs/btrfs/relocation.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
>> index 1c42c5180bddd5..277a0042967665 100644
>> --- a/fs/btrfs/relocation.c
>> +++ b/fs/btrfs/relocation.c
>> @@ -5089,6 +5089,7 @@ static int do_remap_reloc_trans(struct btrfs_fs_info *fs_info,
>>   	btrfs_free_reserved_extent(fs_info, new_addr, length, 0);
>>   
>>   	mutex_unlock(&fs_info->remap_mutex);
>> +	btrfs_abort_transaction(trans, ret);
> 
> Counterintuitively to the usual pattern of sharing code in fail: labels,
> I think it is quite nice to put a separate abort at each failing code
> path that requires an abort. That way we get better diagnostic
> information on what actually went wrong.
> 
> e.g. a recent patch from Filipe unfolding it across a function call:
> https://lore.kernel.org/linux-btrfs/99743ac0b4b9a69dcaa8f9adcaf301bd1a0f1a89.1769163248.git.fdmanana@suse.com/

Thanks Boris - if aborting a transaction logs the line number, that's a 
very good reason to do it in place. This also shows that the call to 
btrfs_add_to_free_space_tree after add_remap_entry fails is pointless, 
as the transaction is about to be aborted.

> Thanks,
> Boris
> 
>>   	btrfs_end_transaction(trans);
>>   
>>   	return ret;
>> -- 
>> 2.52.0
>>