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

[Krystian Kaniewski]

On 10/16/26 17:23, OGAWA Hirofumi wrote:
> Krystian Kaniewski <krystianmkaniewski@xxxxxxxxx> writes:
> 
>> On 10/16/26 15:50, OGAWA Hirofumi wrote:
>>> Krystian Kaniewski <krystianmkaniewski@xxxxxxxxx> 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?
> 
> Basically that path is not the failure of sinfo.bh, true? If so, we will
> not need to re-read sinfo.bh here, just removing the added entries would
> be ok. 
> 
> If we have to read sinfo.bh, the sinfo.bh may be failed the write and
> cleared the uptodate. If so, I wonder why succeed by retry?
> 
> The above is my understand.
> 
> Thanks.
Thanks. error_inode can also be reached when a different metadata buffer fails. In that case, sinfo.bh is still uptodate and bh_read() returns 1 without doing any I/O, so the removal proceeds directly.

The test I mentioned did fail the write of sinfo.bh itself. The trace shows __sync_dirty_buffer() called from fat_sync_inode_metadata() failing on the new-entry block. The retry succeeded because fault injection was configured with times=1, so only that single write failed. bh_read() then reloaded the block and the cleanup write succeeded.

That is why I kept the read. It is a no-op when sinfo.bh is still uptodate, and it allows a complete rollback after a transient write error. Would you still prefer to give up when the failed write cleared BH_Uptodate?

-- 
Krystian Kaniewski