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

On 7/21/26 9:15 AM, Andrew Morton wrote:
> On Mon, 20 Jul 2026 22:19:43 +0800 Guangshuo Li <[email protected]> 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/[email protected]

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