Re: [PATCH v3] ocfs2: free unused clusters on defrag move errors

On 7/20/26 10:19 PM, Guangshuo Li wrote:
> ocfs2_defrag_extent() claims new clusters before calling
> __ocfs2_move_extent(). If the move fails before ocfs2_split_extent()
> succeeds, the claimed clusters are not referenced by the inode and must
> be released.
> 
> The current error path only logs the error and continues to
> ocfs2_cow_sync_writeback(), which can overwrite the original error with
> zero and leave the claimed clusters allocated.
> 
> Not every __ocfs2_move_extent() error can free the new clusters. Once
> ocfs2_split_extent() succeeds, the extent tree references them even if
> ocfs2_decrease_refcount() or ocfs2_truncate_log_append() subsequently
> fails. Freeing the clusters in that case would leave the extent tree
> pointing to clusters marked free.
> 
> context->new_phys_cpos is updated immediately after a successful extent
> split. Compare it with the newly claimed physical cluster on error. If
> they differ, the split for the current move did not complete and the
> claimed clusters can be freed. If they match, leave the clusters
> allocated because the extent tree already references them.
> 
> Return move errors through the transaction cleanup path so that the
> original error is preserved instead of being overwritten by writeback.
> 
> Fixes: 202ee5facb2c ("Ocfs2/move_extents: defrag a range of extent.")
> Signed-off-by: Guangshuo Li <[email protected]>

Looks fine.
Reviewed-by: Joseph Qi <[email protected]>

> ---
> v3:
>   - Fix the duplicate split_started declaration.
>   - Reuse context->new_phys_cpos to determine whether the extent split
>     succeeded, as suggested by Joseph Qi.
>   - Avoid changing the __ocfs2_move_extent() prototype.
> 
> v2:
>   - Do not free the new clusters after the extent-tree update has
>     started, as pointed out by Joseph Qi.
>   - Track whether the new clusters remain unused on error.
>   - Preserve __ocfs2_move_extent() errors instead of allowing the
>     subsequent writeback call to overwrite them.
> 
>  fs/ocfs2/move_extents.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ocfs2/move_extents.c b/fs/ocfs2/move_extents.c
> index ad1678ee7cc4..7820df90a262 100644
> --- a/fs/ocfs2/move_extents.c
> +++ b/fs/ocfs2/move_extents.c
> @@ -310,8 +310,12 @@ static int ocfs2_defrag_extent(struct ocfs2_move_extents_context *context,
>  
>  	ret = __ocfs2_move_extent(handle, context, cpos, new_len, phys_cpos,
>  				  new_phys_cpos, ext_flags);
> -	if (ret)
> +	if (ret) {
>  		mlog_errno(ret);
> +		if (context->new_phys_cpos != new_phys_cpos)
> +			need_free = 1;
> +		goto out_commit;
> +	}
>  
>  	if (partial && (new_len != *len))
>  		*len = new_len;