Re: [PATCH RFC v2 12/13] fat: sync inode metadata in fat_evict_inode()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] fat: fat_evict_inode() syncs stale dir entry before fat_free_eofblocks()
- [Medium] fat: fat_evict_inode() skips mmb_sync() when fat_sync_inode_metadata() fails
- [Medium] fat: fat_iget() skips I_FREEING inodes while fat_evict_inode() syncs metadata
--

commit bd339e5e48c232fb0520331a07810a35de6e7ce5
Author: Christian Brauner <brauner@kernel.org>

fat: sync inode metadata in fat_evict_inode()

This commit updates fat_evict_inode() to call fat_sync_inode_metadata() when
evicting a linked inode that has I_METADATA_WRITEBACK set, ensuring dirty
directory entry and metadata buffers are written out before the inode and its
state bit disappear.

> diff --git a/fs/fat/inode.c b/fs/fat/inode.c
> index d85c8fbdcca46..df4616636c659 100644
> --- a/fs/fat/inode.c
> +++ b/fs/fat/inode.c
> @@ -693,7 +693,16 @@ static void fat_evict_inode(struct inode *inode)
>  		inode->i_size = 0;
>  		fat_truncate_blocks(inode, 0);
>  	} else {
> -		mmb_sync(&MSDOS_I(inode)->i_metadata_bhs);
> +		/*
> +		 * fsync() can't find what I_METADATA_WRITEBACK tracks once the
> +		 * inode is gone. This syncs the metadata buffers as well.
> +		 */
> +		if (inode_state_read_once(inode) & I_METADATA_WRITEBACK)
> +			fat_sync_inode_metadata(inode, &(struct writeback_control) {
> +				.sync_mode = WB_SYNC_ALL,
> +			});

[Severity: Medium]
Can a concurrent lookup reopen the file and complete fsync() before
fat_evict_inode() finishes syncing the old inode's metadata?

When reclaim isolates the inode, I_FREEING is set before dispose_list()
calls evict() -> fat_evict_inode(). Unlike filesystems that look up inodes
via iget_locked() and wait in __wait_on_freeing_inode(), FAT assigns a
synthetic i_ino via iunique() and looks up existing inodes by i_pos in
fat_iget():

fs/fat/inode.c:fat_iget() {
	...
	hlist_for_each_entry(i, head, i_fat_hash) {
		BUG_ON(i->vfs_inode.i_sb != sb);
		if (i->i_pos != i_pos)
			continue;
		inode = igrab(&i->vfs_inode);
		if (inode)
			break;
	}
	...
}

Because igrab() returns NULL when I_FREEING is set, fat_iget() returns NULL
without waiting for the freeing inode to finish eviction.

If another thread looks up the file while the old inode is queued on the
dispose list or sleeping inside fat_sync_inode_metadata() before
fat_detach() runs, fat_build_inode() allocates a new clean inode for the
same i_pos without I_METADATA_WRITEBACK set. Wouldn't a subsequent
fat_file_fsync() -> simple_fsync_noflush() on that new inode see a clean
inode and return before the old inode's directory entry buffer writeback has
completed?

> +		else
> +			mmb_sync(&MSDOS_I(inode)->i_metadata_bhs);

[Severity: Medium]
What happens to MSDOS_I(inode)->i_metadata_bhs if fat_sync_inode_metadata()
encounters a buffer write error on the inode's directory entry block?

In fat_sync_inode_metadata(), if buffer_write_io_error(bh) is true after
sync_dirty_buffer(bh), the function returns -EIO early before reaching the
sync_bhs label:

fs/fat/inode.c:fat_sync_inode_metadata() {
	...
	if (bh && buffer_uptodate(bh)) {
		sync_dirty_buffer(bh);
		if (buffer_write_io_error(bh)) {
			brelse(bh);
			return -EIO;
		}
	}
	brelse(bh);
sync_bhs:
	return mmb_sync(&MSDOS_I(inode)->i_metadata_bhs);
}

When evicting a subdirectory inode that has I_METADATA_WRITEBACK set,
replacing the unconditional mmb_sync() with fat_sync_inode_metadata() means
an I/O error on the parent directory entry buffer skips mmb_sync() for the
child directory buffers tracked in i_metadata_bhs, right before
fat_evict_inode() detaches them via mmb_invalidate(). Should
fat_sync_inode_metadata() still run mmb_sync() before returning an error, or
should fat_evict_inode() call mmb_sync() unconditionally?

>  		fat_free_eofblocks(inode);

[Severity: High]
Could calling fat_sync_inode_metadata() before fat_free_eofblocks() write a
stale directory entry to disk and leave the post-truncation directory entry
unsynced without I_METADATA_WRITEBACK?

When clusters are preallocated via fallocate() with FALLOC_FL_KEEP_SIZE on a
zero-byte file, fat_chain_add() updates i_start and i_logstart and marks the
inode dirty:

fs/fat/misc.c:fat_chain_add() {
	...
	} else {
		MSDOS_I(inode)->i_start = new_dclus;
		MSDOS_I(inode)->i_logstart = new_dclus;
		mark_inode_dirty(inode);
	...
}

Before the inode can be isolated from the LRU and evicted, background
writeback runs fat_write_inode() -> __fat_write_inode(), which writes the
non-zero i_logstart into the directory entry buffer (with size still 0),
marks the buffer dirty, and sets I_METADATA_WRITEBACK on the inode.

When fat_evict_inode() then runs on a normal async mount:

1. Because I_METADATA_WRITEBACK is set, fat_sync_inode_metadata()
   synchronously writes the pre-truncation directory entry buffer (pointing
   to the fallocated clusters on a zero-byte file) to disk.

2. Next, fat_free_eofblocks() truncates the unused fallocated clusters back
   to mmu_private:

fs/fat/inode.c:fat_free_eofblocks() {
	if ((inode->i_blocks << 9) >
			round_up(MSDOS_I(inode)->mmu_private,
				MSDOS_SB(inode->i_sb)->cluster_size)) {
		int err;

		fat_truncate_blocks(inode, MSDOS_I(inode)->mmu_private);
		...
		err = sync_inode_metadata(inode, inode_needs_sync(inode));
		...
	}
}

3. On an async mount, sync_inode_metadata(inode, 0) calls
   __fat_write_inode(), which updates the directory entry buffer in memory
   back to i_logstart = 0, marks the buffer dirty again, and sets
   I_METADATA_WRITEBACK on the dying inode without syncing the buffer.

4. fat_evict_inode() immediately finishes and calls clear_inode(), losing
   the newly set I_METADATA_WRITEBACK bit while the updated directory entry
   buffer remains dirty in memory.

If the file is reopened from the cached buffer and fsync() is called,
simple_fsync_noflush() sees a clean inode without I_METADATA_WRITEBACK and
skips syncing the directory entry buffer, while fat_file_fsync() flushes the
FAT table updates where those clusters were freed or reallocated, leaving
the on-disk zero-byte directory entry pointing at freed clusters if a crash
occurs.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-work-inode-metadata_writeback-v2-0-48628c60c336@kernel.org?part=12