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

On 7/14/26 7:35 PM, Guangshuo Li wrote:
> ocfs2_defrag_extent() claims new clusters before calling
> __ocfs2_move_extent(). If the move fails before the extent tree update
> is attempted, 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. The claimed clusters are also left allocated.
> 
> Not every __ocfs2_move_extent() error can free the new clusters,
> however. Once ocfs2_split_extent() has been called, the extent tree may
> already reference them even if a later operation fails. Freeing them in
> that case would leave the extent tree pointing to clusters marked free.
> 
> Track whether the extent split has been started. On errors before that
> point, report that the new clusters remain unused so the defrag caller
> can release them. Once the split has started, leave the clusters
> allocated. Return all move errors through the transaction cleanup path
> without allowing writeback to overwrite them.
> 
> Fixes: 202ee5facb2c ("Ocfs2/move_extents: defrag a range of extent.")
> Signed-off-by: Guangshuo Li <[email protected]>
> ---
> 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 | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ocfs2/move_extents.c b/fs/ocfs2/move_extents.c
> index ad1678ee7cc4..138c65d21668 100644
> --- a/fs/ocfs2/move_extents.c
> +++ b/fs/ocfs2/move_extents.c
> @@ -49,9 +49,10 @@ struct ocfs2_move_extents_context {
>  static int __ocfs2_move_extent(handle_t *handle,
>  			       struct ocfs2_move_extents_context *context,
>  			       u32 cpos, u32 len, u32 p_cpos, u32 new_p_cpos,
> -			       int ext_flags)
> +			       int ext_flags, bool *new_clusters_unused)
>  {
>  	int ret = 0, index;
> +	bool split_started = false;
>  	struct inode *inode = context->inode;
>  	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
>  	struct ocfs2_extent_rec *rec, replace_rec;
> @@ -59,6 +60,10 @@ static int __ocfs2_move_extent(handle_t *handle,
>  	struct ocfs2_extent_list *el;
>  	u64 ino = ocfs2_metadata_cache_owner(context->et.et_ci);
>  	u64 old_blkno = ocfs2_clusters_to_blocks(inode->i_sb, p_cpos);
> +	bool split_started = false;

Duplicate declaration?

> +
> +	if (new_clusters_unused)
> +		*new_clusters_unused = false;
>  
>  	ret = ocfs2_duplicate_clusters_by_page(handle, inode, cpos,
>  					       p_cpos, new_p_cpos, len);
> @@ -111,6 +116,7 @@ static int __ocfs2_move_extent(handle_t *handle,
>  	 */
>  	replace_rec.e_flags = ext_flags & ~OCFS2_EXT_REFCOUNTED;
>  
> +	split_started = true;
>  	ret = ocfs2_split_extent(handle, &context->et, path, index,
>  				 &replace_rec, context->meta_ac,
>  				 &context->dealloc);
> @@ -138,6 +144,9 @@ static int __ocfs2_move_extent(handle_t *handle,
>  
>  	ocfs2_update_inode_fsync_trans(handle, inode, 0);
>  out:
> +	if (ret && !split_started && new_clusters_unused)
> +		*new_clusters_unused = true;
> +
>  	ocfs2_free_path(path);
>  	return ret;
>  }
> @@ -209,6 +218,7 @@ static int ocfs2_defrag_extent(struct ocfs2_move_extents_context *context,
>  	struct ocfs2_refcount_tree *ref_tree = NULL;
>  	u32 new_phys_cpos, new_len;
>  	u64 phys_blkno = ocfs2_clusters_to_blocks(inode->i_sb, phys_cpos);
> +	bool new_clusters_unused;
>  	int need_free = 0;
>  
>  	if ((ext_flags & OCFS2_EXT_REFCOUNTED) && *len) {
> @@ -309,9 +319,14 @@ static int ocfs2_defrag_extent(struct ocfs2_move_extents_context *context,
>  	     phys_cpos, new_phys_cpos);
>  
>  	ret = __ocfs2_move_extent(handle, context, cpos, new_len, phys_cpos,
> -				  new_phys_cpos, ext_flags);
> -	if (ret)
> +				  new_phys_cpos, ext_flags,
> +				  &new_clusters_unused);

Seems we can reuse 'context->new_phys_cpos' to check if need free without
changing __ocfs2_move_extent() prototype.

Thanks,
Joseph

> +	if (ret) {
>  		mlog_errno(ret);
> +		if (new_clusters_unused)
> +			need_free = 1;
> +		goto out_commit;
> +	}
>  
>  	if (partial && (new_len != *len))
>  		*len = new_len;
> @@ -684,7 +699,7 @@ static int ocfs2_move_extent(struct ocfs2_move_extents_context *context,
>  	}
>  
>  	ret = __ocfs2_move_extent(handle, context, cpos, len, phys_cpos,
> -				  *new_phys_cpos, ext_flags);
> +				  *new_phys_cpos, ext_flags, NULL);
>  	if (ret) {
>  		mlog_errno(ret);
>  		goto out_commit;