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

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

[Guangshuo Li]

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 <lgs201920130244@gmail.com>
---
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;
-- 
2.43.0

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

[Joseph Qi]

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 <lgs201920130244@gmail.com>

Looks fine.
Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com>

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

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

[Andrew Morton]

On Mon, 20 Jul 2026 22:19:43 +0800 Guangshuo Li <lgs201920130244@gmail.com> 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.

Thanks.  AI review has flagged a couple of possible issues with this
change.  Please check?

	https://sashiko.dev/#/patchset/20260720141944.485212-1-lgs201920130244@gmail.com

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

[Joseph Qi]

On 7/21/26 9:15 AM, Andrew Morton wrote:
> On Mon, 20 Jul 2026 22:19:43 +0800 Guangshuo Li <lgs201920130244@gmail.com> 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.
> 
> Thanks.  AI review has flagged a couple of possible issues with this
> change.  Please check?
> 
> 	https://sashiko.dev/#/patchset/20260720141944.485212-1-lgs201920130244@gmail.com

Thanks sashiko. Yes, it indeed introduces a regression.

In data=writeback mode, ocfs2_should_order_data() returns 0, so
ocfs2_jbd2_inode_add_write() is never called. The dirty buffers are not
flushed at commit, they survive, still mapped to new_block. The patch's
need_free path frees new_block immediately.  The VM later writes those
dirty buffers to new_block at an arbitrary time — after it may have been
reallocated to another inode. That's the cross-file corruption.

Before this changes, the cluster were leaked, so new_block stayed allocated
forever and the stale writeback was harmless.

Thanks,
Joseph