Re: [PATCH 06/13] gfs2: Avoid racy dirtiness check in gfs2_fsync()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] gfs2: gfs2_fsync() checks PAGECACHE_TAG_DIRTY after
  filemap_fdatawrite_range()
--

commit a74a8e373d1c581c53bf8dbda94cd97b2bf0e19f
Author: Jan Kara <jack@suse.cz>

gfs2: Avoid racy dirtiness check in gfs2_fsync()

This commit replaces the unlocked I_DIRTY check in gfs2_fsync() with
inode_is_dirty() and a PAGECACHE_TAG_DIRTY check on the mapping for jdata
inodes.

> diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
> index 1efd0679badd9..9e52f18326737 100644
> --- a/fs/gfs2/file.c
> +++ b/fs/gfs2/file.c
> @@ -749,7 +749,6 @@ static int gfs2_fsync(struct file *file, loff_t start, loff_t end,
>  {
>  	struct address_space *mapping = file->f_mapping;
>  	struct inode *inode = mapping->host;
> -	int sync_state = inode_state_read_once(inode) & I_DIRTY;
>  	struct gfs2_glock *gl = gfs2_inode_glock(inode);
>  	struct gfs2_inode *ip = GFS2_I(inode);
>  	int ret = 0, ret1 = 0;
> @@ -760,12 +759,9 @@ static int gfs2_fsync(struct file *file, loff_t start, loff_t end,
>  			return ret1;
>  	}
>  
> -	if (!gfs2_is_jdata(ip))
> -		sync_state &= ~I_DIRTY_PAGES;
> -	if (datasync)
> -		sync_state &= ~I_DIRTY_SYNC;
> -
> -	if (sync_state) {
> +	if (inode_is_dirty(inode, datasync) ||
> +	    (gfs2_is_jdata(ip) && mapping_tagged(inode->i_mapping,
> +						 PAGECACHE_TAG_DIRTY))) {
>  		ret = sync_inode_metadata(inode, 1);
>  		if (ret)
>  			return ret;

[Severity: High]
Does checking mapping_tagged(inode->i_mapping, PAGECACHE_TAG_DIRTY) after
filemap_fdatawrite_range(mapping, start, end) miss dirty jdata pages?

Previously, sync_state was sampled before filemap_fdatawrite_range().  For a
jdata inode, filemap_fdatawrite_range() runs gfs2_jdata_writepages() with
WB_SYNC_ALL, which flushes the log and calls gfs2_write_cache_jdata() ->
gfs2_write_jdata_batch() -> folio_clear_dirty_for_io(), clearing
PAGECACHE_TAG_DIRTY on the folios in [start, end] before this check is
reached.

When overwriting already-allocated blocks of an unstuffed jdata file,
gfs2_iomap_put_folio() only increments tr->tr_num_databuf_new (leaving
tr->tr_num_buf_new == 0), so I_DIRTY_DATASYNC is not set:

fs/gfs2/bmap.c:gfs2_iomap_put_folio() {
	...
	if (gfs2_is_jdata(ip) || gfs2_is_stuffed(ip)) {
		if (tr->tr_num_buf_new)
			__mark_inode_dirty(inode, I_DIRTY_DATASYNC);
		gfs2_trans_end(sdp);
	}
	...
}

On fdatasync() (or fsync() when timestamps did not change), both
inode_is_dirty(inode, datasync) and mapping_tagged(inode->i_mapping,
PAGECACHE_TAG_DIRTY) then evaluate to false, skipping sync_inode_metadata(),
file_write_and_wait(), and gfs2_ail_flush(gl, 1) and leaving the jdata AIL
buffers unrevoked in the journal.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113083336.15152-1-jack@suse.cz?part=6