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

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

Hi Joseph,

Thanks for the review.

On Mon, 20 Jul 2026 at 11:09, Joseph Qi <joseph.qi@linux.alibaba.com> wrote:
>
>
>
> 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 <lgs201920130244@gmail.com>
> > ---
> > 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;
>

You are right about the duplicate declaration. Reusing
context->new_phys_cpos is also simpler and avoids changing the
__ocfs2_move_extent() prototype.

I will update the patch and send v3.

Thanks,
Guangshuo