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

[Guangshuo Li]

Hi Joseph,

Thanks for the review.

On Mon, 20 Jul 2026 at 11:09, Joseph Qi <joseph.qi@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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