[PATCH 0/13] fs: Various I_DIRTY_* state handling fixes

[PATCH 0/13] fs: Various I_DIRTY_* state handling fixes

[Jan Kara]

Hello,

this series is a result of my audit of I_DIRTY_* state handling fixes. Over the
years some pointless code or code with smaller or larger bugs has accumulated
in filesystems around handling of inode dirty state. In particular testing
for I_DIRTY_PAGES is very subtle (and racy without holding i_lock), some places
forget that I_DIRTY_TIME also means inode has dirty metadata etc. So this
patch set removes direct checks of dirtiness state from filesystems and either
change them to directly testing PAGECACHE_TAG_DIRTY or to a helper function
which performs appropriate dirtiness check reliably.

								Honza

[PATCH 01/13] bdev: Drop pointless loop from blkdev_flush_mapping()

[Jan Kara]

bdev_write_inode() is really odd loop calling write_inode_now(). The
bdev doesn't have any pages by now and if there are any dirty bits in
the inode state, they are stale. Just call write_inode_now() once to
clear them.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 block/bdev.c | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/block/bdev.c b/block/bdev.c
index f20f91b3176e..9fa72df4f47a 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -60,24 +60,6 @@ struct block_device *file_bdev(struct file *bdev_file)
 }
 EXPORT_SYMBOL(file_bdev);
 
-static void bdev_write_inode(struct block_device *bdev)
-{
-	struct inode *inode = BD_INODE(bdev);
-	int ret;
-
-	spin_lock(&inode->i_lock);
-	while (inode_state_read(inode) & I_DIRTY) {
-		spin_unlock(&inode->i_lock);
-		ret = write_inode_now(inode, true);
-		if (ret)
-			pr_warn_ratelimited(
-	"VFS: Dirty inode writeback failed for block device %pg (err=%d).\n",
-				bdev, ret);
-		spin_lock(&inode->i_lock);
-	}
-	spin_unlock(&inode->i_lock);
-}
-
 /* Kill _all_ buffers and pagecache , dirty or not.. */
 static void kill_bdev(struct block_device *bdev)
 {
@@ -755,7 +737,12 @@ static void blkdev_flush_mapping(struct block_device *bdev)
 	WARN_ON_ONCE(bdev->bd_holders);
 	sync_blockdev(bdev);
 	kill_bdev(bdev);
-	bdev_write_inode(bdev);
+	/*
+	 * All data is written out and evicted but inode can have some stale
+	 * I_DIRTY bits set. Call write_inode_now() to get rid of them so that
+	 * the inode can be evicted.
+	 */
+	write_inode_now(BD_INODE(bdev), 1);
 }
 
 static void blkdev_put_whole(struct block_device *bdev)
-- 
2.53.0

[PATCH 02/13] xfs: Drop pointless I_DIRTY_PAGES check in xfs_can_free_cowblocks()

[Jan Kara]

PAGECACHE_TAG_DIRTY is the authoritative source of information about any
folio being dirty in the mapping. I_DIRTY_PAGES is just an approximation
used by writeback code that can get stale under certain situations. So
just rely on the authoritative check.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/xfs/xfs_reflink.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
index 683c1841e640..1310cc9fe829 100644
--- a/fs/xfs/xfs_reflink.h
+++ b/fs/xfs/xfs_reflink.h
@@ -17,8 +17,7 @@ xfs_can_free_cowblocks(struct xfs_inode *ip)
 {
 	struct inode *inode = VFS_I(ip);
 
-	if ((inode_state_read_once(inode) & I_DIRTY_PAGES) ||
-	    mapping_tagged(inode->i_mapping, PAGECACHE_TAG_DIRTY) ||
+	if (mapping_tagged(inode->i_mapping, PAGECACHE_TAG_DIRTY) ||
 	    mapping_tagged(inode->i_mapping, PAGECACHE_TAG_WRITEBACK) ||
 	    atomic_read(&inode->i_dio_count))
 		return false;
-- 
2.53.0

[PATCH 03/13] orangefs: Check PAGECACHE_TAG_DIRTY instead of I_DIRTY_PAGES inode state

[Jan Kara]

PAGECACHE_TAG_DIRTY is the authoritative source of dirty folios present
in the mapping. Use that instead of looking at I_DIRTY_PAGES inode state
which can get stale (e.g. when folios are written directly with
filemap_fdatawrite()).

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/orangefs/orangefs-utils.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/orangefs/orangefs-utils.c b/fs/orangefs/orangefs-utils.c
index 9ab1119ebd28..4c4036f470cd 100644
--- a/fs/orangefs/orangefs-utils.c
+++ b/fs/orangefs/orangefs-utils.c
@@ -247,7 +247,8 @@ int orangefs_inode_getattr(struct inode *inode, int flags)
 	spin_lock(&inode->i_lock);
 	/* Must have all the attributes in the mask and be within cache time. */
 	if ((!flags && time_before(jiffies, orangefs_inode->getattr_time)) ||
-	    orangefs_inode->attr_valid || inode_state_read(inode) & I_DIRTY_PAGES) {
+	    orangefs_inode->attr_valid ||
+	    mapping_tagged(inode->i_mapping, PAGECACHE_TAG_DIRTY)) {
 		if (orangefs_inode->attr_valid) {
 			spin_unlock(&inode->i_lock);
 			write_inode_now(inode, 1);
@@ -281,13 +282,14 @@ int orangefs_inode_getattr(struct inode *inode, int flags)
 	spin_lock(&inode->i_lock);
 	/* Must have all the attributes in the mask and be within cache time. */
 	if ((!flags && time_before(jiffies, orangefs_inode->getattr_time)) ||
-	    orangefs_inode->attr_valid || inode_state_read(inode) & I_DIRTY_PAGES) {
+	    orangefs_inode->attr_valid ||
+	    mapping_tagged(inode->i_mapping, PAGECACHE_TAG_DIRTY)) {
 		if (orangefs_inode->attr_valid) {
 			spin_unlock(&inode->i_lock);
 			write_inode_now(inode, 1);
 			goto again2;
 		}
-		if (inode_state_read(inode) & I_DIRTY_PAGES) {
+		if (mapping_tagged(inode->i_mapping, PAGECACHE_TAG_DIRTY)) {
 			ret = 0;
 			goto out_unlock;
 		}
-- 
2.53.0

[PATCH 04/13] fs: Provide inode_is_dirty() helper

[Jan Kara]

Provide inode_is_dirty() helper for use by filesystems (mostly fsync
methods).

Signed-off-by: Jan Kara <jack@suse.cz>
---
 include/linux/fs.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 99caef43ff3c..025f5d890689 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2217,6 +2217,17 @@ static inline void mark_inode_dirty_sync(struct inode *inode)
 	__mark_inode_dirty(inode, I_DIRTY_SYNC);
 }
 
+/*
+ * Check whether inode itself needs to be written out either at all or for
+ * the purposes of datasync if @datasync is set.
+ */
+static inline bool inode_is_dirty(struct inode *inode, bool datasync)
+{
+	int mask = datasync ? I_DIRTY_DATASYNC : (I_DIRTY_INODE | I_DIRTY_TIME);
+
+	return inode_state_read_once(inode) & mask;
+}
+
 static inline void set_inode_metadata_writeback(struct inode *inode)
 {
 	spin_lock(&inode->i_lock);
-- 
2.53.0

[PATCH 05/13] jfs: Use inode_is_dirty() in jfs_fsync()

[Jan Kara]

Use inode_is_dirty() helper in jfs_fsync() instead of explicit i_state
check. This slightly changes behavior in that we no longer trigger inode
commit if the inode is clean except it still has I_DIRTY_PAGES set after
data writeout but that is actually a desirable behavior because commit
is pointless if only folios are dirty.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/jfs/file.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/jfs/file.c b/fs/jfs/file.c
index 2f5bb79c0591..90001d3d90fc 100644
--- a/fs/jfs/file.c
+++ b/fs/jfs/file.c
@@ -27,8 +27,7 @@ int jfs_fsync(struct file *file, loff_t start, loff_t end, int datasync)
 		return rc;
 
 	inode_lock(inode);
-	if (!(inode_state_read_once(inode) & I_DIRTY_ALL) ||
-	    (datasync && !(inode_state_read_once(inode) & I_DIRTY_DATASYNC))) {
+	if (!inode_is_dirty(inode, datasync)) {
 		/* Make sure committed changes hit the disk */
 		jfs_flush_journal(JFS_SBI(inode->i_sb)->log, 1);
 		inode_unlock(inode);
-- 
2.53.0

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

[Jan Kara]

Check for I_DIRTY_PAGES state in gfs2_fsync() is racy because
__writeback_single_inode() unconditionally clears the flag and
reinstates it only later based on PAGECACHE_TAG_DIRTY tag. If
gfs2_fsync() unlocked read of i_state hits in between, it will not see
I_DIRTY_PAGES although there may be dirty folios in the mapping. Check
for PAGECACHE_TAG_DIRTY instead and use inode_is_dirty() helper for
inode metadata dirtiness check.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/gfs2/file.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
index 1efd0679badd..9e52f1832673 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;
-- 
2.53.0

[PATCH 07/13] fuse: Use inode_is_dirty() check in fuse_evict_inode()

[Jan Kara]

Use inode_is_dirty() helper instead of explicit i_state check. This will
result in additionally warning if we have unwritten timestamp updates
when lazytime is enabled but that is actually desirable.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/fuse/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 1341b681d0ed..ef88ba7bf682 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -143,7 +143,7 @@ static void fuse_evict_inode(struct inode *inode)
 	struct fuse_inode *fi = get_fuse_inode(inode);
 
 	/* Will write inode on close/munmap and in all other dirtiers */
-	WARN_ON(inode_state_read_once(inode) & I_DIRTY_INODE);
+	WARN_ON(inode_is_dirty(inode, false));
 
 	if (IS_DAX(inode))
 		dax_break_layout_final(inode);
-- 
2.53.0

[PATCH 08/13] fs: Provide inode_or_folios_dirty() helper

[Jan Kara]

Provide a helper function checking whether inode itself or some of its
folios are dirty. We'll use it in filesystems instead of direct i_state
checks. Note that this check returns true also if only timestamps are
dirty when lazytime is enabled. This is what users really want.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 include/linux/fs.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 025f5d890689..f602deacf953 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2228,6 +2228,13 @@ static inline bool inode_is_dirty(struct inode *inode, bool datasync)
 	return inode_state_read_once(inode) & mask;
 }
 
+/* Check whether anything in the inode itself or its folios need writing */
+static inline bool inode_or_folios_dirty(struct inode *inode)
+{
+	return inode_state_read_once(inode) & I_DIRTY_ALL ||
+		mapping_tagged(inode->i_mapping, PAGECACHE_TAG_DIRTY);
+}
+
 static inline void set_inode_metadata_writeback(struct inode *inode)
 {
 	spin_lock(&inode->i_lock);
-- 
2.53.0

[PATCH 09/13] gfs2: Fix dirtiness checks in gfs2_write_inode() and evict_linked_inode()

[Jan Kara]

The checks are bogus because they check against inode->i_flags instead
of inode->i_state. Use the inode_or_folios_dirty() helper which actually
checks what was intended.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/gfs2/super.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index 04bb4cf787d4..9b723dd591a5 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -460,7 +460,7 @@ static int gfs2_write_inode(struct inode *inode, struct writeback_control *wbc)
 		mark_inode_dirty_sync(inode);
 	else {
 		spin_lock(&inode->i_lock);
-		if (!(inode->i_flags & I_DIRTY))
+		if (!inode_or_folios_dirty(inode))
 			gfs2_ordered_del_inode(ip);
 		spin_unlock(&inode->i_lock);
 	}
@@ -1418,7 +1418,8 @@ static int evict_linked_inode(struct inode *inode, struct gfs2_holder *gh)
 	struct address_space *metamapping = gfs2_glock2aspace(gl);
 	int ret;
 
-	if (!(test_bit(GLF_DIRTY, &gl->gl_flags) || inode->i_flags & I_DIRTY))
+	if (!(test_bit(GLF_DIRTY, &gl->gl_flags) ||
+	      inode_or_folios_dirty(inode)))
 		goto clean;
 
 	/* The inode glock must be held exclusively and be instantiated. */
-- 
2.53.0

[PATCH 10/13] afs: Use inode_or_folios_dirty() in afs_evict_inode()

[Jan Kara]

Use the helper instead of direct inode I_DIRTY check to abstract details
of inode dirtiness handling. This also properly triggers when only
timestamps are dirty and lazytime is enabled (although I'm not sure if
it really matters for this particular case in AFS).

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/afs/inode.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/afs/inode.c b/fs/afs/inode.c
index 0733abff77a5..69982e7d7020 100644
--- a/fs/afs/inode.c
+++ b/fs/afs/inode.c
@@ -667,10 +667,8 @@ void afs_evict_inode(struct inode *inode)
 
 	ASSERTCMP(inode->i_ino, ==, vnode->fid.vnode);
 
-	if ((S_ISDIR(inode->i_mode) ||
-	     S_ISLNK(inode->i_mode)) &&
-	    (inode_state_read_once(inode) & I_DIRTY) &&
-	    !sbi->dyn_root) {
+	if ((S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode)) &&
+	    inode_or_folios_dirty(inode) && !sbi->dyn_root) {
 		struct writeback_control wbc = {
 			.sync_mode = WB_SYNC_ALL,
 			.for_sync = true,
-- 
2.53.0

[PATCH 11/13] nfsd: Use inode_or_folios_dirty() for dirtiness check in wait_for_concurrent_writes()

[Jan Kara]

The check for dirtiness in wait_for_concurrent_writes() is racy because
I_DIRTY_PAGES flag can be temporarily cleared by
__writeback_single_inode(). Use inode_or_folios_dirty() helper to avoid
that. This also makes sure inode timestamps properly persisted when
lazytime is enabled.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/nfsd/vfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 56c49b5bc5bb..a314ca39272f 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1249,7 +1249,7 @@ static int wait_for_concurrent_writes(struct file *file)
 		dprintk("nfsd: write resume %d\n", task_pid_nr(current));
 	}
 
-	if (inode_state_read_once(inode) & I_DIRTY) {
+	if (inode_or_folios_dirty(inode)) {
 		dprintk("nfsd: write sync %d\n", task_pid_nr(current));
 		err = vfs_fsync(file, 0);
 	}
-- 
2.53.0

[PATCH 12/13] fscrypt: Use inode_or_folios_dirty() instead of explicit state check

[Jan Kara]

Use inode_or_folios_dirty() helper instead of explicit inode state
check. No functional change.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/crypto/keysetup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index a77b889d7578..1cd40663b251 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -839,7 +839,7 @@ int fscrypt_drop_inode(struct inode *inode)
 	 * userspace is still using the files, inodes can be dirtied between
 	 * then and now.  We mustn't lose any writes, so skip dirty inodes here.
 	 */
-	if (inode_state_read(inode) & I_DIRTY_ALL)
+	if (inode_or_folios_dirty(inode))
 		return 0;
 
 	/*
-- 
2.53.0

[PATCH 13/13] fs: Use inode_or_folios_dirty() in __inode_lru_list_add()

[Jan Kara]

Use inode_or_folios_dirty() helper instead of explicit i_state check in
__inode_lru_list_add(). No functional change.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/inode.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/inode.c b/fs/inode.c
index f63535ee3629..73afa57fe385 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -552,7 +552,9 @@ static void __inode_lru_list_add(struct inode *inode, bool rotate)
 {
 	lockdep_assert_held(&inode->i_lock);
 
-	if (inode_state_read(inode) & (I_DIRTY_ALL | I_SYNC | I_FREEING | I_WILL_FREE))
+	if (inode_state_read(inode) & (I_SYNC | I_FREEING | I_WILL_FREE))
+		return;
+	if (inode_or_folios_dirty(inode))
 		return;
 	if (icount_read(inode))
 		return;
-- 
2.53.0

Re: [PATCH 09/13] gfs2: Fix dirtiness checks in gfs2_write_inode() and evict_linked_inode()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] gfs2: missing Fixes trailers for broken i_flags dirtiness checks
--

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

> gfs2: Fix dirtiness checks in gfs2_write_inode() and evict_linked_inode()
>
> The checks are bogus because they check against inode->i_flags instead
> of inode->i_state. Use the inode_or_folios_dirty() helper which actually
> checks what was intended.
>
> Signed-off-by: Jan Kara <jack@suse.cz>

[Severity: Low]
Should this include Fixes: tags for the commits that introduced the
inode->i_flags & I_DIRTY checks in gfs2_write_inode() and
evict_linked_inode()?

  Fixes: 957a7acd46e6 ("gfs2: Remove inode from ordered write list in gfs2_write_inode()")
  Fixes: bd67f17718cc ("gfs2: Fix data loss during inode evict")

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

Re: [PATCH 11/13] nfsd: Use inode_or_folios_dirty() for dirtiness check in wait_for_concurrent_writes()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Medium] nfsd: data race on static variables in
  wait_for_concurrent_writes() [1]
--

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

nfsd: Use inode_or_folios_dirty() for dirtiness check in wait_for_concurrent_writes()

This commit replaces the racy I_DIRTY check in wait_for_concurrent_writes()
with inode_or_folios_dirty() so that concurrent writeback clearing
I_DIRTY_PAGES or lazytime timestamp updates do not cause vfs_fsync() to be
skipped.

> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 6905d3285ce10..f271b474e76ce 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1431,7 +1431,7 @@ static int wait_for_concurrent_writes(struct file *file)
>  		dprintk("nfsd: write resume %d\n", task_pid_nr(current));
>  	}
>  
> -	if (inode_state_read_once(inode) & I_DIRTY) {
> +	if (inode_or_folios_dirty(inode)) {
>  		dprintk("nfsd: write sync %d\n", task_pid_nr(current));
>  		err = vfs_fsync(file, 0);
>  	}

[Severity: Medium]
This problem wasn't introduced by this patch, but can concurrent NFSD
threads cause a data race on the static variables last_ino and last_dev in
wait_for_concurrent_writes() [1]?

fs/nfsd/vfs.c:wait_for_concurrent_writes() {
	struct inode *inode = file_inode(file);
	static ino_t last_ino;
	static dev_t last_dev;
	int err = 0;

	if (atomic_read(&inode->i_writecount) > 1
	    || (last_ino == inode->i_ino && last_dev == inode->i_sb->s_dev)) {
		...
		msleep(10);
		...
	}
	...
	last_ino = inode->i_ino;
	last_dev = inode->i_sb->s_dev;
	return err;
}

Because last_ino and last_dev are shared across all NFSD threads and read
and written without synchronization, concurrent write requests can observe
torn accesses or mismatched inode and device pairs, causing spurious
msleep(10) delays or missed write deferrals.

[1] https://sashiko.dev/#/bug/linux-f2ccec51-4c11-45a7-a336-775a80d79f1c

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

Re: [PATCH 03/13] orangefs: Check PAGECACHE_TAG_DIRTY instead of I_DIRTY_PAGES inode state

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] orangefs: stale i_size overwrite during writeback in
  orangefs_inode_getattr()
--

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

orangefs: Check PAGECACHE_TAG_DIRTY instead of I_DIRTY_PAGES inode state

This commit replaces checks of the I_DIRTY_PAGES inode state flag in
orangefs_inode_getattr() with mapping_tagged(..., PAGECACHE_TAG_DIRTY).

> diff --git a/fs/orangefs/orangefs-utils.c b/fs/orangefs/orangefs-utils.c
> index 9ab1119ebd28a..4c4036f470cdd 100644
> --- a/fs/orangefs/orangefs-utils.c
> +++ b/fs/orangefs/orangefs-utils.c
> @@ -247,7 +247,8 @@ int orangefs_inode_getattr(struct inode *inode, int flags)
>  	spin_lock(&inode->i_lock);
>  	/* Must have all the attributes in the mask and be within cache time. */
>  	if ((!flags && time_before(jiffies, orangefs_inode->getattr_time)) ||
> -	    orangefs_inode->attr_valid || inode_state_read(inode) & I_DIRTY_PAGES) {
> +	    orangefs_inode->attr_valid ||
> +	    mapping_tagged(inode->i_mapping, PAGECACHE_TAG_DIRTY)) {
>  		if (orangefs_inode->attr_valid) {
>  			spin_unlock(&inode->i_lock);
>  			write_inode_now(inode, 1);
> @@ -281,13 +282,14 @@ int orangefs_inode_getattr(struct inode *inode, int flags)
>  	spin_lock(&inode->i_lock);
>  	/* Must have all the attributes in the mask and be within cache time. */
>  	if ((!flags && time_before(jiffies, orangefs_inode->getattr_time)) ||
> -	    orangefs_inode->attr_valid || inode_state_read(inode) & I_DIRTY_PAGES) {
> +	    orangefs_inode->attr_valid ||
> +	    mapping_tagged(inode->i_mapping, PAGECACHE_TAG_DIRTY)) {
>  		if (orangefs_inode->attr_valid) {
>  			spin_unlock(&inode->i_lock);
>  			write_inode_now(inode, 1);
>  			goto again2;
>  		}
> -		if (inode_state_read(inode) & I_DIRTY_PAGES) {
> +		if (mapping_tagged(inode->i_mapping, PAGECACHE_TAG_DIRTY)) {
>  			ret = 0;
>  			goto out_unlock;
>  		}

[Severity: High]
Can orangefs_inode_getattr() overwrite inode->i_size with a stale size from
the server while pagecache writeback is in flight?

When a buffered write extends a file in orangefs_write_end(), inode->i_size
is updated in memory and the folio is marked dirty, while the server only
learns the new size once the writeback RPC in wait_for_direct_io()
completes.  Previously, I_DIRTY_PAGES stayed set in
__writeback_single_inode() across the entire do_writepages() call.
However, folio_start_writeback() clears PAGECACHE_TAG_DIRTY and sets
PAGECACHE_TAG_WRITEBACK before wait_for_direct_io() sends the write to the
server:

CPU0                                  CPU1
orangefs_writepage_locked()
  folio_start_writeback(folio)
    /* clears PAGECACHE_TAG_DIRTY,
       sets PAGECACHE_TAG_WRITEBACK */
                                      orangefs_getattr()
                                        orangefs_inode_getattr(...,
                                            ORANGEFS_GETATTR_SIZE)
                                          /* PAGECACHE_TAG_DIRTY is false */
                                          service_operation(GETATTR)
                                          inode->i_size = ...; <-- stale size
  len = i_size_read(inode);
  wait_for_direct_io(ORANGEFS_IO_WRITE, ...)

As a result, any concurrent getattr with non-zero flags (such as
orangefs_getattr() or orangefs_file_llseek()) fetches the pre-write size
from the server and clobbers inode->i_size, which also causes
orangefs_writepage_locked() (or a subsequent batch in
orangefs_writepages_work()) to clamp the write length to the stale size and
truncate the buffered write.

Should these checks also test mapping_tagged(inode->i_mapping,
PAGECACHE_TAG_WRITEBACK) (or mapping_writably_mapped() / a helper covering
both PAGECACHE_TAG_DIRTY and PAGECACHE_TAG_WRITEBACK)?

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

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

[sashiko-bot]

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

Re: [PATCH 09/13] gfs2: Fix dirtiness checks in gfs2_write_inode() and evict_linked_inode()

[Jan Kara]

On Fri 13-11-26 08:56:48, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue to consider:
> - [Low] gfs2: missing Fixes trailers for broken i_flags dirtiness checks
> --
> 
> commit 7de49fbf42de584cd1afa16382bbb192cfbdaf7c
> Author: Jan Kara <jack@suse.cz>
> 
> > gfs2: Fix dirtiness checks in gfs2_write_inode() and evict_linked_inode()
> >
> > The checks are bogus because they check against inode->i_flags instead
> > of inode->i_state. Use the inode_or_folios_dirty() helper which actually
> > checks what was intended.
> >
> > Signed-off-by: Jan Kara <jack@suse.cz>
> 
> [Severity: Low]
> Should this include Fixes: tags for the commits that introduced the
> inode->i_flags & I_DIRTY checks in gfs2_write_inode() and
> evict_linked_inode()?
> 
>   Fixes: 957a7acd46e6 ("gfs2: Remove inode from ordered write list in gfs2_write_inode()")
>   Fixes: bd67f17718cc ("gfs2: Fix data loss during inode evict")

Yeah, these Fixes tags would be adequate.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Jan Kara]

On Fri 13-11-26 09:08:22, sashiko-bot@kernel.org wrote:
> 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.

So I'm aware the behavior of gfs2_fsync() is changed and on a second
thought I agree that the mapping_tagged() check should probably happen
before the filemap_fdatawrite_range() as it used to. But frankly I find the
code in gfs2_fsync() with multiple filemap writeouts and waits confusing so
I'd like some feedback from GFS2 folks about the intended behavior to be
sure.

								Honza

> 
> 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
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 03/13] orangefs: Check PAGECACHE_TAG_DIRTY instead of I_DIRTY_PAGES inode state

[Jan Kara]

On Fri 13-11-26 09:00:01, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue to consider:
> - [High] orangefs: stale i_size overwrite during writeback in
>   orangefs_inode_getattr()

...

> > diff --git a/fs/orangefs/orangefs-utils.c b/fs/orangefs/orangefs-utils.c
> > index 9ab1119ebd28a..4c4036f470cdd 100644
> > --- a/fs/orangefs/orangefs-utils.c
> > +++ b/fs/orangefs/orangefs-utils.c
> > @@ -247,7 +247,8 @@ int orangefs_inode_getattr(struct inode *inode, int flags)
> >  	spin_lock(&inode->i_lock);
> >  	/* Must have all the attributes in the mask and be within cache time. */
> >  	if ((!flags && time_before(jiffies, orangefs_inode->getattr_time)) ||
> > -	    orangefs_inode->attr_valid || inode_state_read(inode) & I_DIRTY_PAGES) {
> > +	    orangefs_inode->attr_valid ||
> > +	    mapping_tagged(inode->i_mapping, PAGECACHE_TAG_DIRTY)) {
> >  		if (orangefs_inode->attr_valid) {
> >  			spin_unlock(&inode->i_lock);
> >  			write_inode_now(inode, 1);
> > @@ -281,13 +282,14 @@ int orangefs_inode_getattr(struct inode *inode, int flags)
> >  	spin_lock(&inode->i_lock);
> >  	/* Must have all the attributes in the mask and be within cache time. */
> >  	if ((!flags && time_before(jiffies, orangefs_inode->getattr_time)) ||
> > -	    orangefs_inode->attr_valid || inode_state_read(inode) & I_DIRTY_PAGES) {
> > +	    orangefs_inode->attr_valid ||
> > +	    mapping_tagged(inode->i_mapping, PAGECACHE_TAG_DIRTY)) {
> >  		if (orangefs_inode->attr_valid) {
> >  			spin_unlock(&inode->i_lock);
> >  			write_inode_now(inode, 1);
> >  			goto again2;
> >  		}
> > -		if (inode_state_read(inode) & I_DIRTY_PAGES) {
> > +		if (mapping_tagged(inode->i_mapping, PAGECACHE_TAG_DIRTY)) {
> >  			ret = 0;
> >  			goto out_unlock;
> >  		}
> 
> [Severity: High]
> Can orangefs_inode_getattr() overwrite inode->i_size with a stale size from
> the server while pagecache writeback is in flight?
> 
> When a buffered write extends a file in orangefs_write_end(), inode->i_size
> is updated in memory and the folio is marked dirty, while the server only
> learns the new size once the writeback RPC in wait_for_direct_io()
> completes.  Previously, I_DIRTY_PAGES stayed set in
> __writeback_single_inode() across the entire do_writepages() call.
> However, folio_start_writeback() clears PAGECACHE_TAG_DIRTY and sets
> PAGECACHE_TAG_WRITEBACK before wait_for_direct_io() sends the write to the
> server:
> 
> CPU0                                  CPU1
> orangefs_writepage_locked()
>   folio_start_writeback(folio)
>     /* clears PAGECACHE_TAG_DIRTY,
>        sets PAGECACHE_TAG_WRITEBACK */
>                                       orangefs_getattr()
>                                         orangefs_inode_getattr(...,
>                                             ORANGEFS_GETATTR_SIZE)
>                                           /* PAGECACHE_TAG_DIRTY is false */
>                                           service_operation(GETATTR)
>                                           inode->i_size = ...; <-- stale size
>   len = i_size_read(inode);
>   wait_for_direct_io(ORANGEFS_IO_WRITE, ...)

Yeah, that's a good catch but I think that this is pre-existing issue. When
we don't wait for writeback to complete in __writeback_single_inode()
(which happens for sync(2) or for WB_SYNC_NONE writeback), we can clear
I_DIRTY_PAGES before writeback completes as well. So this patch just makes
the race window larger.

But I agree with adding a check for PAGECACHE_TAG_WRITEBACK into the
conditions as well to close the race window altogether. Another option
would be to sample i_size before calling folio_start_writeback() but I find
that too subtle, it would require some barriers etc., so I won't do that
unless orangefs guys insist.

								Honza

> 
> As a result, any concurrent getattr with non-zero flags (such as
> orangefs_getattr() or orangefs_file_llseek()) fetches the pre-write size
> from the server and clobbers inode->i_size, which also causes
> orangefs_writepage_locked() (or a subsequent batch in
> orangefs_writepages_work()) to clamp the write length to the stale size and
> truncate the buffered write.
> 
> Should these checks also test mapping_tagged(inode->i_mapping,
> PAGECACHE_TAG_WRITEBACK) (or mapping_writably_mapped() / a helper covering
> both PAGECACHE_TAG_DIRTY and PAGECACHE_TAG_WRITEBACK)?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261113083336.15152-1-jack@suse.cz?part=3
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR