Re: [PATCH v3] fat: make rename recovery safe after metadata write errors

[Krystian Kaniewski <krystianmkaniewski@gmail.com>]

On 10/16/26 15:50, OGAWA Hirofumi wrote:
> Krystian Kaniewski <krystianmkaniewski@gmail.com> writes:
> 
>> On 10/15/26 20:54, OGAWA Hirofumi wrote:
>>
>>>> +	} else if (!rollback_err) {
>>>> +		/* Read the entry block again if a failed write invalidated it. */
>>>> +		rollback_err = bh_read(sinfo.bh, 0);
>>>
>>> Why do this read the bh from on disk?
>>>
>>
>> The bh_read(sinfo.bh, 0) is needed because a failed inode-position write clears BH_Uptodate. fat_remove_entries() modifies and dirties that retained buffer directly, which otherwise triggers the warning. Reading it first refreshes the whole block. If the read fails, the buffer is left untouched.
>>
>> The same reasoning applies to both the VFAT and MS-DOS paths.
> 
> If sinfo.bh is required to read because of failure of I/O error, I guess
> the bh will fail to write again. I think we should give up to rollback
> in that case.
> 
> Thanks.

Thanks. I was trying to handle transient errors here. The old entry still exists at this point, so if the read and removal succeed, the rename can be fully rolled back.

In my tests with a single injected write error, giving up left the new entry pointing at the root directory, and fsck.fat wanted to remove it. Retrying the cleanup left the filesystem clean. If either the read or the cleanup write fails, we stop and report corruption anyway.

Do you still think we should give up here? If you prefer not to retry the new-entry block after its failed write, should we also avoid rereading and rewriting dotdot_bh after a failed .. update?

-- 
Krystian Kaniewski