[PATCH RFC v2 00/13] fs: reclaim inodes with I_METADATA_WRITEBACK

[PATCH RFC v2 00/13] fs: reclaim inodes with I_METADATA_WRITEBACK

[Christian Brauner]

Since v7.3 ext2, ext4 without a journal, fat, udf and minix set
I_METADATA_WRITEBACK when they copy an inode into its buffer and only
fsync() and friends clear it again.

inode_lru_isolate() treats it like any other state bit and takes the
inode off the LRU, expecting something to put it back once the bit is
gone. Nothing ever does. So these inodes can't be reclaimed until
something fsyncs them or the filesystem is unmounted. That's a DoS.

On ext4 without a journal an atime update is enough to pin an inode, and
creating 60000 small files in a memory cgroup limited to 96M gets the
task OOM killed after about 43000 of them.

The original provided fix was quite ugly. It handed such inodes from
inode_lru_isolate() to a per-sb work item with a reference held, synced
them there and put them back on the LRU. We've had discussions in that
direction for unrelated reasons and we found that quite terrible. It's a
second inode reclaim offload mechanism next to Jan's deferred inode
reclaim, it needs a new inode state bit, and the reference shows up in
everything that looks for open deleted files.

So instead, let the shrinker reclaim these inodes like any other clean
inode and make the filesystems write out what I_METADATA_WRITEBACK
tracks in ->evict_inode() unless the inode is being deleted. That's
what they already do there for the metadata buffers they track.

The first twelve patches do that per filesystem. ->sync_inode_metadata()
of ext2, ext4, minix and udf looks the inode block up in the buffer
cache instead of reading it from disk, the way fat already does, so
that calling it from reclaim is fine. Then ->evict_inode() of all five
calls it for a linked inode that still has the bit set. The last patch
is the actual fix.

This still writes and waits from reclaim if the inode block is still
dirty. Once Jan's deferred inode reclaim lands these inodes can go to
his workers instead. I have that on top of his v3 (cf. [1]).

All of this is meant for v7.3 stable. v7.3 still has bfs which sets
I_METADATA_WRITEBACK as well, so the backport needs the same two
changes for bfs.

Tested on 65ba8f34ec88 with KASAN and lockdep: creating 5000 files,
sync(2) and two drop_caches leave about 5000 of 5000 inodes around on
ext2, ext4, vfat and minix without this series and at most one with it.
The memory cgroup run creates all 60000 files and nothing gets killed.
Blocks the flusher left dirty get written out on eviction on all five
filesystems. No splats.

udf inodes with their data in the ICB are still dirty after sync(2),
with and without this series. That's a separate issue I didn't care
about.

[1]: https://patch.msgid.link/20261029-work-inode-metadata_writeback-deferred-v1-1-7e49f6c12b60@kernel.org

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
Changes in v2:
- Drop the ext4_inode_loc_block() helper and use
  ext4_get_inode_loc_noio() with the sleeping buffer lookup instead
  (Jan).
- Write out what I_METADATA_WRITEBACK tracks from ->evict_inode() of
  each filesystem instead of from evict() (Jan).
- Drop "fat: sync the inode in fat_free_eofblocks()" (Jan, OGAWA).
- Move the reproducer out of the commit message.
- Link to v1: https://patch.msgid.link/20261029-work-inode-metadata_writeback-v1-0-7d6c678a585d@kernel.org

---
Christian Brauner (13):
      ext2: add ext2_inode_block() helper
      ext2: don't read the inode block in ext2_sync_inode_metadata()
      ext2: sync inode metadata in ext2_evict_inode()
      ext4: use sb_find_get_block_nonatomic() in ext4_get_inode_loc_noio()
      ext4: don't read the inode block in ext4_sync_inode_metadata()
      ext4: sync inode metadata in ext4_evict_inode()
      minix: add minix_inode_block() helper
      minix: don't read the inode block in minix_sync_inode_metadata()
      minix: sync inode metadata in minix_evict_inode()
      udf: don't allocate the inode block in udf_sync_inode_metadata()
      udf: sync inode metadata in udf_evict_inode()
      fat: sync inode metadata in fat_evict_inode()
      fs: don't strand I_METADATA_WRITEBACK inodes outside the LRU

 fs/ext2/inode.c   | 69 ++++++++++++++++++++++++++++++++++++-------------------
 fs/ext4/inode.c   | 33 ++++++++++++++++++--------
 fs/fat/inode.c    | 11 ++++++++-
 fs/inode.c        |  5 +++-
 fs/minix/bitmap.c | 37 ++++++++++++++++++-----------
 fs/minix/inode.c  | 24 ++++++++++++-------
 fs/minix/minix.h  |  1 +
 fs/udf/inode.c    | 45 +++++++++++++++++++++---------------
 8 files changed, 150 insertions(+), 75 deletions(-)
---
base-commit: 65ba8f34ec886ec72b2c07ddc8cf3d815c32869d
change-id: 20261029-work-inode-metadata_writeback-d880a0429e49

[PATCH RFC v2 01/13] ext2: add ext2_inode_block() helper

[Christian Brauner]

Move the lookup of the inode table block that holds an inode out of
ext2_get_inode() into a helper. ext2_sync_inode_metadata() is going to
use it to find the block without reading it from disk.

No functional changes.

Cc: stable@vger.kernel.org # v7.3
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/ext2/inode.c | 49 +++++++++++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 18 deletions(-)

diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index 12ac4cfe1500..aecd8528ea36 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -1272,46 +1272,59 @@ static int ext2_setsize(struct inode *inode, loff_t newsize)
 	return 0;
 }
 
-static struct ext2_inode *ext2_get_inode(struct super_block *sb, ino_t ino,
-					struct buffer_head **p)
+/*
+ * Find the inode table block that holds inode @ino and the offset of the
+ * inode in that block.
+ */
+static int ext2_inode_block(struct super_block *sb, ino_t ino,
+			    unsigned long *block, unsigned long *offset)
 {
-	struct buffer_head * bh;
 	unsigned long block_group;
-	unsigned long block;
-	unsigned long offset;
 	struct ext2_group_desc * gdp;
 
-	*p = NULL;
 	if ((ino != EXT2_ROOT_INO && ino < EXT2_FIRST_INO(sb)) ||
-	    ino > le32_to_cpu(EXT2_SB(sb)->s_es->s_inodes_count))
-		goto Einval;
+	    ino > le32_to_cpu(EXT2_SB(sb)->s_es->s_inodes_count)) {
+		ext2_error(sb, __func__, "bad inode number: %lu",
+			   (unsigned long) ino);
+		return -EINVAL;
+	}
 
 	block_group = (ino - 1) / EXT2_INODES_PER_GROUP(sb);
 	gdp = ext2_get_group_desc(sb, block_group, NULL);
 	if (!gdp)
-		goto Egdp;
+		return -EIO;
 	/*
 	 * Figure out the offset within the block group inode table
 	 */
-	offset = ((ino - 1) % EXT2_INODES_PER_GROUP(sb)) * EXT2_INODE_SIZE(sb);
-	block = le32_to_cpu(gdp->bg_inode_table) +
-		(offset >> EXT2_BLOCK_SIZE_BITS(sb));
+	*offset = ((ino - 1) % EXT2_INODES_PER_GROUP(sb)) * EXT2_INODE_SIZE(sb);
+	*block = le32_to_cpu(gdp->bg_inode_table) +
+		(*offset >> EXT2_BLOCK_SIZE_BITS(sb));
+	*offset &= (EXT2_BLOCK_SIZE(sb) - 1);
+	return 0;
+}
+
+static struct ext2_inode *ext2_get_inode(struct super_block *sb, ino_t ino,
+					struct buffer_head **p)
+{
+	struct buffer_head *bh;
+	unsigned long block;
+	unsigned long offset;
+	int err;
+
+	*p = NULL;
+	err = ext2_inode_block(sb, ino, &block, &offset);
+	if (err)
+		return ERR_PTR(err);
 	if (!(bh = sb_bread(sb, block)))
 		goto Eio;
 
 	*p = bh;
-	offset &= (EXT2_BLOCK_SIZE(sb) - 1);
 	return (struct ext2_inode *) (bh->b_data + offset);
 
-Einval:
-	ext2_error(sb, "ext2_get_inode", "bad inode number: %lu",
-		   (unsigned long) ino);
-	return ERR_PTR(-EINVAL);
 Eio:
 	ext2_error(sb, "ext2_get_inode",
 		   "unable to read inode block - inode=%lu, block=%lu",
 		   (unsigned long) ino, block);
-Egdp:
 	return ERR_PTR(-EIO);
 }
 

-- 
2.53.0

[PATCH RFC v2 02/13] ext2: don't read the inode block in ext2_sync_inode_metadata()

[Christian Brauner]

ext2_sync_inode_metadata() looks the inode block up with
ext2_get_inode() which reads it from disk if it isn't cached. But a
block that isn't cached is neither dirty nor under writeback, so there's
nothing to write or to wait for.

That read is also something we can't afford once inode eviction calls
->sync_inode_metadata() from reclaim. So look the block up with
sb_find_get_block_nonatomic() instead, the way fat_sync_inode_metadata()
already does.

Cc: stable@vger.kernel.org # v7.3
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/ext2/inode.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index aecd8528ea36..9261b666d043 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -1586,19 +1586,22 @@ int ext2_write_inode(struct inode *inode, struct writeback_control *wbc)
 int ext2_sync_inode_metadata(struct inode *inode, struct writeback_control *wbc)
 {
 	struct buffer_head *bh;
-	struct ext2_inode *raw_inode = ext2_get_inode(inode->i_sb, inode->i_ino,
-						      &bh);
+	unsigned long block, offset;
 	int err = 0;
 
-	if (IS_ERR(raw_inode))
+	if (ext2_inode_block(inode->i_sb, inode->i_ino, &block, &offset))
 		return -EIO;
 	err = mmb_sync(&EXT2_I(inode)->i_metadata_bhs);
 	if (err) {
 		ext2_error(inode->i_sb, __func__,
 			"Error syncing inode metadata ino=%lu\n",
 			(unsigned long)inode->i_ino);
-		goto out;
+		return err;
 	}
+	/* A block that isn't cached is neither dirty nor under writeback. */
+	bh = sb_find_get_block_nonatomic(inode->i_sb, block);
+	if (!bh)
+		return 0;
 	sync_dirty_buffer(bh);
 	if (buffer_write_io_error(bh)) {
 		ext2_error(inode->i_sb, __func__,
@@ -1606,7 +1609,6 @@ int ext2_sync_inode_metadata(struct inode *inode, struct writeback_control *wbc)
 			(unsigned long)inode->i_ino);
 		err = -EIO;
 	}
-out:
 	brelse(bh);
 	return err;
 }

-- 
2.53.0

[PATCH RFC v2 03/13] ext2: sync inode metadata in ext2_evict_inode()

[Christian Brauner]

ext2_write_inode() copies the inode into its inode table block, marks
the block dirty and sets I_METADATA_WRITEBACK so that a later fsync()
writes the block out and waits for it. But the bit goes away with the
inode. If the inode is evicted and read back in, fsync() finds a clean
inode and returns while the block might still be dirty or under
writeback.

Right now that can't happen because inode_lru_isolate() never lets go
of an inode with I_METADATA_WRITEBACK set. That is about to change.

So call ext2_sync_inode_metadata() from ext2_evict_inode() for a linked
inode that still has the bit set. It syncs the metadata buffers as well
so it replaces the mmb_sync() there, and since the previous patch it
doesn't read anything from disk.

Suggested-by: Jan Kara <jack@suse.cz>
Cc: stable@vger.kernel.org # v7.3
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/ext2/inode.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index 9261b666d043..4ed727c3d26d 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -91,6 +91,14 @@ void ext2_evict_inode(struct inode * inode)
 		if (inode->i_blocks)
 			ext2_truncate_blocks(inode, 0);
 		ext2_xattr_delete_inode(inode);
+	} else if (inode_state_read_once(inode) & I_METADATA_WRITEBACK) {
+		/*
+		 * fsync() can't find what I_METADATA_WRITEBACK tracks once the
+		 * inode is gone. This syncs the metadata buffers as well.
+		 */
+		ext2_sync_inode_metadata(inode, &(struct writeback_control) {
+			.sync_mode = WB_SYNC_ALL,
+		});
 	} else {
 		mmb_sync(&EXT2_I(inode)->i_metadata_bhs);
 	}

-- 
2.53.0

[PATCH RFC v2 04/13] ext4: use sb_find_get_block_nonatomic() in ext4_get_inode_loc_noio()

[Christian Brauner]

ext4_get_inode_loc_noio() looks the inode table block up with
sb_find_get_block(). That's the atomic lookup. It serializes against
try_to_free_buffers() with i_private_lock instead of the folio lock and
gives up if the folio is being migrated. So it can report the block as
not cached while it is.

For ext4_fc_snapshot_inode() that only means falling back to a full
commit. But ext4_sync_inode_metadata() is going to use
ext4_get_inode_loc_noio() to find the inode block that fsync() has to
write out and there a dirty block must not be skipped.

ext4_fc_snapshot_inode() is the only caller and it can sleep. It runs
under jbd2_journal_lock_updates() and allocates the snapshot with
GFP_NOFS. So use sb_find_get_block_nonatomic() which takes the folio
lock and waits for the migration instead.

Suggested-by: Jan Kara <jack@suse.cz>
Cc: stable@vger.kernel.org # v7.3
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/ext4/inode.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index b4d581aa5ee1..4484c2383eaf 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5876,7 +5876,8 @@ int ext4_get_inode_loc(struct inode *inode, struct ext4_iloc *iloc)
 /*
  * ext4_get_inode_loc_noio() is a best-effort variant of ext4_get_inode_loc().
  * It looks up the inode table block in the buffer cache and returns -EAGAIN if
- * the block is not present or not uptodate, without starting any I/O.
+ * the block is not present or not uptodate, without starting any I/O. It may
+ * sleep on the folio lock of the block.
  */
 int ext4_get_inode_loc_noio(struct inode *inode, struct ext4_iloc *iloc)
 {
@@ -5912,7 +5913,7 @@ int ext4_get_inode_loc_noio(struct inode *inode, struct ext4_iloc *iloc)
 	}
 	block += inode_offset / inodes_per_block;
 
-	bh = sb_find_get_block(sb, block);
+	bh = sb_find_get_block_nonatomic(sb, block);
 	if (!bh)
 		return -EAGAIN;
 	if (!ext4_buffer_uptodate(bh)) {

-- 
2.53.0

[PATCH RFC v2 05/13] ext4: don't read the inode block in ext4_sync_inode_metadata()

[Christian Brauner]

Without a journal ext4_sync_inode_metadata() looks the inode table
block up with __ext4_get_inode_loc_noinmem() which reads it from disk,
with readahead of the inode table on top, if it isn't cached. But a
block that isn't cached is neither dirty nor under writeback, so there's
nothing to write or to wait for.

That read is also something we can't afford once inode eviction calls
->sync_inode_metadata() from reclaim. So look the block up with
ext4_get_inode_loc_noio() instead and treat -EAGAIN as nothing to do.

Note that ext4_get_inode_loc_noio() also returns -EAGAIN for a block
that isn't uptodate. Such a block is being read or failed to be read
and can't be dirty either. A block whose write failed still counts as
uptodate for ext4_buffer_uptodate() so the write error still gets
reported.

Suggested-by: Jan Kara <jack@suse.cz>
Cc: stable@vger.kernel.org # v7.3
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/ext4/inode.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 4484c2383eaf..18362580097f 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -6815,22 +6815,24 @@ int ext4_sync_inode_metadata(struct inode *inode, struct writeback_control *wbc)
 	if (WARN_ON_ONCE(EXT4_SB(inode->i_sb)->s_journal))
 		return -EFSCORRUPTED;
 
-	err = __ext4_get_inode_loc_noinmem(inode, &iloc);
-	if (err)
-		return err;
 	mmb = READ_ONCE(EXT4_I(inode)->i_metadata_bhs);
 	if (mmb) {
 		err = mmb_sync(mmb);
 		if (err)
-			goto out;
+			return err;
 	}
+	/* A block that isn't cached is neither dirty nor under writeback. */
+	err = ext4_get_inode_loc_noio(inode, &iloc);
+	if (err == -EAGAIN)
+		return 0;
+	if (err)
+		return err;
 	sync_dirty_buffer(iloc.bh);
 	if (buffer_write_io_error(iloc.bh)) {
 		ext4_error_inode_block(inode, iloc.bh->b_blocknr, EIO,
 				       "IO error syncing inode");
 		err = -EIO;
 	}
-out:
 	brelse(iloc.bh);
 	return err;
 }

-- 
2.53.0

[PATCH RFC v2 06/13] ext4: sync inode metadata in ext4_evict_inode()

[Christian Brauner]

Without a journal ext4_mark_iloc_dirty() copies the inode into the inode
table block, marks the block dirty and sets I_METADATA_WRITEBACK so that
a later fsync() writes the block out and waits for it. But the bit goes
away with the inode. If the inode is evicted and read back in, fsync()
finds a clean inode and returns while the block might still be dirty or
under writeback.

Right now that can't happen because inode_lru_isolate() never lets go
of an inode with I_METADATA_WRITEBACK set. That is about to change.

So call ext4_sync_inode_metadata() from ext4_evict_inode() for a linked
inode that still has the bit set. It syncs the metadata buffers as well
so it replaces the mmb_sync() there, and since the previous patch it
doesn't read anything from disk. With a journal the bit is never set
and nothing changes.

Suggested-by: Jan Kara <jack@suse.cz>
Cc: stable@vger.kernel.org # v7.3
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/ext4/inode.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 18362580097f..c64cedd86826 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -261,9 +261,19 @@ void ext4_evict_inode(struct inode *inode)
 			ext4_warning_inode(inode, "data will be lost");
 
 		truncate_inode_pages_final(&inode->i_data);
-		mmb = ext4_i_metadata_bhs(inode);
-		if (mmb)
-			mmb_sync(mmb);
+		/*
+		 * 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) {
+			ext4_sync_inode_metadata(inode, &(struct writeback_control) {
+				.sync_mode = WB_SYNC_ALL,
+			});
+		} else {
+			mmb = ext4_i_metadata_bhs(inode);
+			if (mmb)
+				mmb_sync(mmb);
+		}
 		goto no_delete;
 	}
 

-- 
2.53.0

[PATCH RFC v2 07/13] minix: add minix_inode_block() helper

[Christian Brauner]

Move the lookup of the block that holds an inode out of
minix_V1_raw_inode() and minix_V2_raw_inode() into a helper.
minix_sync_inode_metadata() is going to use it to find the block
without reading it from disk.

No functional changes.

Cc: stable@vger.kernel.org # v7.3
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/minix/bitmap.c | 37 +++++++++++++++++++++++--------------
 fs/minix/minix.h  |  1 +
 2 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/fs/minix/bitmap.c b/fs/minix/bitmap.c
index abec438330a7..deac05472bef 100644
--- a/fs/minix/bitmap.c
+++ b/fs/minix/bitmap.c
@@ -103,21 +103,35 @@ unsigned long minix_count_free_blocks(struct super_block *sb)
 		<< sbi->s_log_zone_size);
 }
 
-struct minix_inode *
-minix_V1_raw_inode(struct super_block *sb, ino_t ino, struct buffer_head **bh)
+/* Return the block that holds inode @ino or 0 if @ino is out of range. */
+int minix_inode_block(struct super_block *sb, ino_t ino)
 {
-	int block;
 	struct minix_sb_info *sbi = minix_sb(sb);
-	struct minix_inode *p;
+	int inodes_per_block;
 
 	if (!ino || ino > sbi->s_ninodes) {
 		printk("Bad inode number on dev %s: %ld is out of range\n",
 		       sb->s_id, (long)ino);
-		return NULL;
+		return 0;
 	}
+	if (sbi->s_version == MINIX_V1)
+		inodes_per_block = MINIX_INODES_PER_BLOCK;
+	else
+		inodes_per_block = sb->s_blocksize / sizeof(struct minix2_inode);
+	return 2 + sbi->s_imap_blocks + sbi->s_zmap_blocks +
+		 (ino - 1) / inodes_per_block;
+}
+
+struct minix_inode *
+minix_V1_raw_inode(struct super_block *sb, ino_t ino, struct buffer_head **bh)
+{
+	int block;
+	struct minix_inode *p;
+
+	block = minix_inode_block(sb, ino);
+	if (!block)
+		return NULL;
 	ino--;
-	block = 2 + sbi->s_imap_blocks + sbi->s_zmap_blocks +
-		 ino / MINIX_INODES_PER_BLOCK;
 	*bh = sb_bread(sb, block);
 	if (!*bh) {
 		printk("Unable to read inode block\n");
@@ -131,19 +145,14 @@ struct minix2_inode *
 minix_V2_raw_inode(struct super_block *sb, ino_t ino, struct buffer_head **bh)
 {
 	int block;
-	struct minix_sb_info *sbi = minix_sb(sb);
 	struct minix2_inode *p;
 	int minix2_inodes_per_block = sb->s_blocksize / sizeof(struct minix2_inode);
 
 	*bh = NULL;
-	if (!ino || ino > sbi->s_ninodes) {
-		printk("Bad inode number on dev %s: %ld is out of range\n",
-		       sb->s_id, (long)ino);
+	block = minix_inode_block(sb, ino);
+	if (!block)
 		return NULL;
-	}
 	ino--;
-	block = 2 + sbi->s_imap_blocks + sbi->s_zmap_blocks +
-		 ino / minix2_inodes_per_block;
 	*bh = sb_bread(sb, block);
 	if (!*bh) {
 		printk("Unable to read inode block\n");
diff --git a/fs/minix/minix.h b/fs/minix/minix.h
index db92cf9e0e1b..67d8e2a7dc06 100644
--- a/fs/minix/minix.h
+++ b/fs/minix/minix.h
@@ -47,6 +47,7 @@ void __minix_error_inode(struct inode *inode, const char *function,
 			 unsigned int line, const char *fmt, ...);
 
 struct inode *minix_iget(struct super_block *, unsigned long);
+int minix_inode_block(struct super_block *sb, ino_t ino);
 struct minix_inode *minix_V1_raw_inode(struct super_block *, ino_t, struct buffer_head **);
 struct minix2_inode *minix_V2_raw_inode(struct super_block *, ino_t, struct buffer_head **);
 struct inode *minix_new_inode(const struct inode *, umode_t);

-- 
2.53.0

[PATCH RFC v2 08/13] minix: don't read the inode block in minix_sync_inode_metadata()

[Christian Brauner]

minix_sync_inode_metadata() looks the inode block up with
minix_V1_raw_inode() or minix_V2_raw_inode() which read it from disk if
it isn't cached. But a block that isn't cached is neither dirty nor
under writeback, so there's nothing to write or to wait for.

That read is also something we can't afford once inode eviction calls
->sync_inode_metadata() from reclaim. So look the block up with
sb_find_get_block_nonatomic() instead.

Cc: stable@vger.kernel.org # v7.3
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/minix/inode.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/minix/inode.c b/fs/minix/inode.c
index 670179173645..756bef4dc832 100644
--- a/fs/minix/inode.c
+++ b/fs/minix/inode.c
@@ -702,24 +702,24 @@ static int minix_sync_inode_metadata(struct inode *inode,
 {
 	int err = 0;
 	struct buffer_head *bh;
-	void *raw_inode;
+	int block;
 
-	if (INODE_VERSION(inode) == MINIX_V1)
-		raw_inode = minix_V1_raw_inode(inode->i_sb, inode->i_ino, &bh);
-	else
-		raw_inode = minix_V2_raw_inode(inode->i_sb, inode->i_ino, &bh);
-	if (!raw_inode)
+	block = minix_inode_block(inode->i_sb, inode->i_ino);
+	if (!block)
 		return -EIO;
 	err = mmb_sync(&minix_i(inode)->i_metadata_bhs);
 	if (err)
-		goto out;
+		return err;
+	/* A block that isn't cached is neither dirty nor under writeback. */
+	bh = sb_find_get_block_nonatomic(inode->i_sb, block);
+	if (!bh)
+		return 0;
 	sync_dirty_buffer(bh);
 	if (buffer_write_io_error(bh)) {
 		printk("IO error syncing minix inode [%s:%08llx]\n",
 			inode->i_sb->s_id, inode->i_ino);
 		err = -EIO;
 	}
-out:
 	brelse(bh);
 	return err;
 }

-- 
2.53.0

[PATCH RFC v2 09/13] minix: sync inode metadata in minix_evict_inode()

[Christian Brauner]

minix_write_inode() copies the inode into its block, marks the block
dirty and sets I_METADATA_WRITEBACK so that a later fsync() writes the
block out and waits for it. But the bit goes away with the inode. If
the inode is evicted and read back in, fsync() finds a clean inode and
returns while the block might still be dirty or under writeback.

Right now that can't happen because inode_lru_isolate() never lets go
of an inode with I_METADATA_WRITEBACK set. That is about to change.

So call minix_sync_inode_metadata() from minix_evict_inode() for a
linked inode that still has the bit set. It syncs the metadata buffers
as well so it replaces the mmb_sync() there, and since the previous
patch it doesn't read anything from disk.

Suggested-by: Jan Kara <jack@suse.cz>
Cc: stable@vger.kernel.org # v7.3
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/minix/inode.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/fs/minix/inode.c b/fs/minix/inode.c
index 756bef4dc832..7d6cc49f7126 100644
--- a/fs/minix/inode.c
+++ b/fs/minix/inode.c
@@ -50,6 +50,14 @@ static void minix_evict_inode(struct inode *inode)
 	if (!inode->i_nlink) {
 		inode->i_size = 0;
 		minix_truncate(inode);
+	} else if (inode_state_read_once(inode) & I_METADATA_WRITEBACK) {
+		/*
+		 * fsync() can't find what I_METADATA_WRITEBACK tracks once the
+		 * inode is gone. This syncs the metadata buffers as well.
+		 */
+		minix_sync_inode_metadata(inode, &(struct writeback_control) {
+			.sync_mode = WB_SYNC_ALL,
+		});
 	} else {
 		mmb_sync(&minix_i(inode)->i_metadata_bhs);
 	}

-- 
2.53.0

[PATCH RFC v2 10/13] udf: don't allocate the inode block in udf_sync_inode_metadata()

[Christian Brauner]

udf_sync_inode_metadata() looks the block that holds the inode up with
sb_getblk() which allocates a buffer for it if it isn't cached. But a
block that isn't cached is neither dirty nor under writeback, so there's
nothing to write or to wait for and the buffer just gets thrown away
again.

That allocation is also something we can't afford once inode eviction
calls ->sync_inode_metadata() from reclaim. So look the block up with
sb_find_get_block_nonatomic() instead.

Cc: stable@vger.kernel.org # v7.3
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/udf/inode.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index bacf2a86161e..b3e7c07cb9e7 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -1742,25 +1742,24 @@ void udf_update_extra_perms(struct inode *inode, umode_t mode)
 int udf_sync_inode_metadata(struct inode *inode, struct writeback_control *wbc)
 {
 	struct buffer_head *bh;
+	unsigned long block;
 	int err = 0;
 
-	bh = sb_getblk(inode->i_sb,
-			udf_get_lb_pblock(inode->i_sb,
-					  &UDF_I(inode)->i_location, 0));
-	if (!bh)
-		return -EIO;
-
-	sync_dirty_buffer(bh);
-	if (buffer_write_io_error(bh)) {
-		udf_warn(inode->i_sb, "IO error syncing udf inode [%08llx]\n",
-			 inode->i_ino);
-		err = -EIO;
-		goto out;
+	block = udf_get_lb_pblock(inode->i_sb, &UDF_I(inode)->i_location, 0);
+	/* A block that isn't cached is neither dirty nor under writeback. */
+	bh = sb_find_get_block_nonatomic(inode->i_sb, block);
+	if (bh) {
+		sync_dirty_buffer(bh);
+		if (buffer_write_io_error(bh)) {
+			udf_warn(inode->i_sb, "IO error syncing udf inode [%08llx]\n",
+				 inode->i_ino);
+			err = -EIO;
+		}
+		brelse(bh);
+		if (err)
+			return err;
 	}
-	err = mmb_sync(&UDF_I(inode)->i_metadata_bhs);
-out:
-	brelse(bh);
-	return err;
+	return mmb_sync(&UDF_I(inode)->i_metadata_bhs);
 }
 
 static void udf_adjust_time(struct udf_inode_info *iinfo, struct timespec64 time)

-- 
2.53.0

[PATCH RFC v2 11/13] udf: sync inode metadata in udf_evict_inode()

[Christian Brauner]

udf_write_inode() copies the inode into the block that holds it, marks
the block dirty and sets I_METADATA_WRITEBACK so that a later fsync()
writes the block out and waits for it. But the bit goes away with the
inode. If the inode is evicted and read back in, fsync() finds a clean
inode and returns while the block might still be dirty or under
writeback.

Right now that can't happen because inode_lru_isolate() never lets go
of an inode with I_METADATA_WRITEBACK set. That is about to change.

So call udf_sync_inode_metadata() from udf_evict_inode() for an inode
that isn't deleted and still has the bit set. It syncs the metadata
buffers as well so it replaces the mmb_sync() there, and since the
previous patch it doesn't allocate anything.

Suggested-by: Jan Kara <jack@suse.cz>
Cc: stable@vger.kernel.org # v7.3
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/udf/inode.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index b3e7c07cb9e7..8f45133ce75a 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -152,8 +152,18 @@ void udf_evict_inode(struct inode *inode)
 		}
 	}
 	truncate_inode_pages_final(&inode->i_data);
-	if (!want_delete)
-		mmb_sync(&iinfo->i_metadata_bhs);
+	if (!want_delete) {
+		/*
+		 * 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)
+			udf_sync_inode_metadata(inode, &(struct writeback_control) {
+				.sync_mode = WB_SYNC_ALL,
+			});
+		else
+			mmb_sync(&iinfo->i_metadata_bhs);
+	}
 	mmb_invalidate(&iinfo->i_metadata_bhs);
 	clear_inode(inode);
 	kfree(iinfo->i_data);

-- 
2.53.0

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

[Christian Brauner]

fat_write_inode() copies the inode into its directory entry, marks the
buffer dirty and sets I_METADATA_WRITEBACK so that a later fsync()
writes the entry out and waits for it. But the bit goes away with the
inode. If the inode is evicted and read back in, fsync() finds a clean
inode and returns while the directory entry might still be dirty or
under writeback.

Right now that can't happen because inode_lru_isolate() never lets go
of an inode with I_METADATA_WRITEBACK set. That is about to change.

So call fat_sync_inode_metadata() from fat_evict_inode() for a linked
inode that still has the bit set. It syncs the metadata buffers as well
so it replaces the mmb_sync() there, and it already looks the directory
entry up in the buffer cache only.

Do it before fat_free_eofblocks(). That one updates the directory entry
again for a file with unused fallocated clusters and only waits for it
on a sync mount. That's intentional (cf. [1]) and syncing after it
would turn every such eviction into a synchronous write.

[1]: https://lore.kernel.org/r/87v76co5dv.fsf@mail.parknet.co.jp

Suggested-by: Jan Kara <jack@suse.cz>
Cc: stable@vger.kernel.org # v7.3
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/fat/inode.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index d85c8fbdcca4..df4616636c65 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,
+			});
+		else
+			mmb_sync(&MSDOS_I(inode)->i_metadata_bhs);
 		fat_free_eofblocks(inode);
 	}
 

-- 
2.53.0

[PATCH RFC v2 13/13] fs: don't strand I_METADATA_WRITEBACK inodes outside the LRU

[Christian Brauner]

Since commit c474bc56b6d1 ("fs: Provide way for filesystem to wait for
metadata writeback") ext2, ext4 without a journal, fat, udf and minix
set I_METADATA_WRITEBACK on an inode whose metadata was copied into a
buffer that might not have reached the disk yet. Only data integrity
writeback, i.e. fsync() and friends, clears it again. The flusher and
sync(2) leave it alone.

inode_lru_isolate() treats every state bit other than I_REFERENCED as a
reason not to reclaim the inode right now. It takes the inode off the
LRU and relies on iput(), writeback completion or the removal of the
last page from the page cache to put it back once the reason is gone.

But I_METADATA_WRITEBACK doesn't go away on its own. iput() puts the
inode back on the LRU with the bit still set and the next scan takes it
off again. So neither memory pressure nor drop_caches can free these
inodes. Only fsync() and friends or unmounting the filesystem get rid
of them.

On ext4 without a journal ext4_mark_iloc_dirty() sets the bit on every
ext4_mark_inode_dirty(), so even an atime update is enough. An
unprivileged user creating or touching lots of files grows the inode
cache without bound until the OOM killer comes along.

The inode can't just be thrown away though. An fsync() after the inode
was read back in wouldn't know that its metadata still has to be
written out. But that's no different from the metadata buffers these
filesystems track in their mapping_metadata_bhs. Their ->evict_inode()
calls mmb_sync() and that is what allowed commit 972b9dd4e418 ("fs:
Ignore inode metadata buffers in inode_lru_isolate()") to take those
buffers out of inode_lru_isolate(). With the previous patches
->evict_inode() writes out what I_METADATA_WRITEBACK tracks the same
way, without reading anything from disk.

So let the shrinker reclaim inodes with I_METADATA_WRITEBACK like any
other clean inode.

Handing these inodes to a work item that syncs them and puts them back
on the LRU would work too. But that's a second inode reclaim offload
mechanism next to the deferred inode reclaim that is being worked on,
and it needs a reference on the inode which leaks into remount, freeze
and umount.

For stable this needs the previous patches as well. Without the
->evict_inode() changes an fsync() after the inode was read back in
misses the inode block. Without the lookup changes ->evict_inode() reads
inode blocks from disk with __GFP_NOFAIL allocations from reclaim. v7.3
also still has bfs which sets I_METADATA_WRITEBACK too, so the backport
needs the same two changes for bfs.

Fixes: c474bc56b6d1 ("fs: Provide way for filesystem to wait for metadata writeback")
Cc: stable@vger.kernel.org # v7.3; see patch description
Reported-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/inode.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/inode.c b/fs/inode.c
index 1cb6293b237a..16ebd686d681 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -947,9 +947,12 @@ static enum lru_status inode_lru_isolate(struct list_head *item,
 	 * they're already on the LRU, and this can make them
 	 * unreclaimable for a while. Remove them lazily here; iput,
 	 * sync, or the last page cache deletion will requeue them.
+	 * Nothing requeues them for I_METADATA_WRITEBACK, ->evict_inode()
+	 * writes that out instead.
 	 */
 	if (icount_read(inode) ||
-	    (inode_state_read(inode) & ~I_REFERENCED) ||
+	    (inode_state_read(inode) &
+	     ~(I_REFERENCED | I_METADATA_WRITEBACK)) ||
 	    !mapping_shrinkable(&inode->i_data)) {
 		list_lru_isolate(lru, &inode->i_lru);
 		spin_unlock(&inode->i_lock);

-- 
2.53.0

Re: [PATCH RFC v2 07/13] minix: add minix_inode_block() helper

[sashiko-bot]

> Move the lookup of the block that holds an inode out of
> minix_V1_raw_inode() and minix_V2_raw_inode() into a helper.
> minix_sync_inode_metadata() is going to use it to find the block
> without reading it from disk.
> 
> No functional changes.
> 
> Cc: stable@vger.kernel.org # v7.3
> Reviewed-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>

Sashiko has reviewed this patch and found no issues.

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

Re: [PATCH RFC v2 04/13] ext4: use sb_find_get_block_nonatomic() in ext4_get_inode_loc_noio()

[sashiko-bot]

> ext4_get_inode_loc_noio() looks the inode table block up with
> sb_find_get_block(). That's the atomic lookup. It serializes against
> try_to_free_buffers() with i_private_lock instead of the folio lock and
> gives up if the folio is being migrated. So it can report the block as
> not cached while it is.
> 
> For ext4_fc_snapshot_inode() that only means falling back to a full
> commit. But ext4_sync_inode_metadata() is going to use
> ext4_get_inode_loc_noio() to find the inode block that fsync() has to
> write out and there a dirty block must not be skipped.
> 
> ext4_fc_snapshot_inode() is the only caller and it can sleep. It runs
> under jbd2_journal_lock_updates() and allocates the snapshot with
> GFP_NOFS. So use sb_find_get_block_nonatomic() which takes the folio
> lock and waits for the migration instead.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Cc: stable@vger.kernel.org # v7.3
> Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>

Sashiko has reviewed this patch and found no issues.

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

Re: [PATCH RFC v2 08/13] minix: don't read the inode block in minix_sync_inode_metadata()

[sashiko-bot]

> minix_sync_inode_metadata() looks the inode block up with
> minix_V1_raw_inode() or minix_V2_raw_inode() which read it from disk if
> it isn't cached. But a block that isn't cached is neither dirty nor
> under writeback, so there's nothing to write or to wait for.
> 
> That read is also something we can't afford once inode eviction calls
> ->sync_inode_metadata() from reclaim. So look the block up with
> sb_find_get_block_nonatomic() instead.
> 
> Cc: stable@vger.kernel.org # v7.3
> Reviewed-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>

Sashiko has reviewed this patch and found no issues.

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

Re: [PATCH RFC v2 09/13] minix: sync inode metadata in minix_evict_inode()

[sashiko-bot]

> minix_write_inode() copies the inode into its block, marks the block
> dirty and sets I_METADATA_WRITEBACK so that a later fsync() writes the
> block out and waits for it. But the bit goes away with the inode. If
> the inode is evicted and read back in, fsync() finds a clean inode and
> returns while the block might still be dirty or under writeback.
> 
> Right now that can't happen because inode_lru_isolate() never lets go
> of an inode with I_METADATA_WRITEBACK set. That is about to change.
> 
> So call minix_sync_inode_metadata() from minix_evict_inode() for a
> linked inode that still has the bit set. It syncs the metadata buffers
> as well so it replaces the mmb_sync() there, and since the previous
> patch it doesn't read anything from disk.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Cc: stable@vger.kernel.org # v7.3
> Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>

Sashiko has reviewed this patch and found no issues.

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

Re: [PATCH RFC v2 02/13] ext2: don't read the inode block in ext2_sync_inode_metadata()

[sashiko-bot]

> ext2_sync_inode_metadata() looks the inode block up with
> ext2_get_inode() which reads it from disk if it isn't cached. But a
> block that isn't cached is neither dirty nor under writeback, so there's
> nothing to write or to wait for.
> 
> That read is also something we can't afford once inode eviction calls
> ->sync_inode_metadata() from reclaim. So look the block up with
> sb_find_get_block_nonatomic() instead, the way fat_sync_inode_metadata()
> already does.
> 
> Cc: stable@vger.kernel.org # v7.3
> Reviewed-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>

Sashiko has reviewed this patch and found no issues.

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

Re: [PATCH RFC v2 05/13] ext4: don't read the inode block in ext4_sync_inode_metadata()

[sashiko-bot]

> Without a journal ext4_sync_inode_metadata() looks the inode table
> block up with __ext4_get_inode_loc_noinmem() which reads it from disk,
> with readahead of the inode table on top, if it isn't cached. But a
> block that isn't cached is neither dirty nor under writeback, so there's
> nothing to write or to wait for.
> 
> That read is also something we can't afford once inode eviction calls
> ->sync_inode_metadata() from reclaim. So look the block up with
> ext4_get_inode_loc_noio() instead and treat -EAGAIN as nothing to do.
> 
> Note that ext4_get_inode_loc_noio() also returns -EAGAIN for a block
> that isn't uptodate. Such a block is being read or failed to be read
> and can't be dirty either. A block whose write failed still counts as
> uptodate for ext4_buffer_uptodate() so the write error still gets
> reported.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Cc: stable@vger.kernel.org # v7.3
> Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>

Sashiko has reviewed this patch and found no issues.

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

Re: [PATCH RFC v2 03/13] ext2: sync inode metadata in ext2_evict_inode()

[sashiko-bot]

> ext2_write_inode() copies the inode into its inode table block, marks
> the block dirty and sets I_METADATA_WRITEBACK so that a later fsync()
> writes the block out and waits for it. But the bit goes away with the
> inode. If the inode is evicted and read back in, fsync() finds a clean
> inode and returns while the block might still be dirty or under
> writeback.
> 
> Right now that can't happen because inode_lru_isolate() never lets go
> of an inode with I_METADATA_WRITEBACK set. That is about to change.
> 
> So call ext2_sync_inode_metadata() from ext2_evict_inode() for a linked
> inode that still has the bit set. It syncs the metadata buffers as well
> so it replaces the mmb_sync() there, and since the previous patch it
> doesn't read anything from disk.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Cc: stable@vger.kernel.org # v7.3
> Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>

Sashiko has reviewed this patch and found no issues.

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

Re: [PATCH RFC v2 10/13] udf: don't allocate the inode block in udf_sync_inode_metadata()

[sashiko-bot]

> udf_sync_inode_metadata() looks the block that holds the inode up with
> sb_getblk() which allocates a buffer for it if it isn't cached. But a
> block that isn't cached is neither dirty nor under writeback, so there's
> nothing to write or to wait for and the buffer just gets thrown away
> again.
> 
> That allocation is also something we can't afford once inode eviction
> calls ->sync_inode_metadata() from reclaim. So look the block up with
> sb_find_get_block_nonatomic() instead.
> 
> Cc: stable@vger.kernel.org # v7.3
> Reviewed-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>

Sashiko has reviewed this patch and found no issues.

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

Re: [PATCH RFC v2 01/13] ext2: add ext2_inode_block() helper

[sashiko-bot]

> Move the lookup of the inode table block that holds an inode out of
> ext2_get_inode() into a helper. ext2_sync_inode_metadata() is going to
> use it to find the block without reading it from disk.
> 
> No functional changes.
> 
> Cc: stable@vger.kernel.org # v7.3
> Reviewed-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>

Sashiko has reviewed this patch and found no issues.

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

Re: [PATCH RFC v2 06/13] ext4: sync inode metadata in ext4_evict_inode()

[sashiko-bot]

> Without a journal ext4_mark_iloc_dirty() copies the inode into the inode
> table block, marks the block dirty and sets I_METADATA_WRITEBACK so that
> a later fsync() writes the block out and waits for it. But the bit goes
> away with the inode. If the inode is evicted and read back in, fsync()
> finds a clean inode and returns while the block might still be dirty or
> under writeback.
> 
> Right now that can't happen because inode_lru_isolate() never lets go
> of an inode with I_METADATA_WRITEBACK set. That is about to change.
> 
> So call ext4_sync_inode_metadata() from ext4_evict_inode() for a linked
> inode that still has the bit set. It syncs the metadata buffers as well
> so it replaces the mmb_sync() there, and since the previous patch it
> doesn't read anything from disk. With a journal the bit is never set
> and nothing changes.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Cc: stable@vger.kernel.org # v7.3
> Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>

Sashiko has reviewed this patch and found no issues.

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

Re: [PATCH RFC v2 11/13] udf: sync inode metadata in udf_evict_inode()

[sashiko-bot]

> udf_write_inode() copies the inode into the block that holds it, marks
> the block dirty and sets I_METADATA_WRITEBACK so that a later fsync()
> writes the block out and waits for it. But the bit goes away with the
> inode. If the inode is evicted and read back in, fsync() finds a clean
> inode and returns while the block might still be dirty or under
> writeback.
> 
> Right now that can't happen because inode_lru_isolate() never lets go
> of an inode with I_METADATA_WRITEBACK set. That is about to change.
> 
> So call udf_sync_inode_metadata() from udf_evict_inode() for an inode
> that isn't deleted and still has the bit set. It syncs the metadata
> buffers as well so it replaces the mmb_sync() there, and since the
> previous patch it doesn't allocate anything.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Cc: stable@vger.kernel.org # v7.3
> Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>

Sashiko has reviewed this patch and found no issues.

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

Re: [PATCH RFC v2 13/13] fs: don't strand I_METADATA_WRITEBACK inodes outside the LRU

[sashiko-bot]

> Since commit c474bc56b6d1 ("fs: Provide way for filesystem to wait for
> metadata writeback") ext2, ext4 without a journal, fat, udf and minix
> set I_METADATA_WRITEBACK on an inode whose metadata was copied into a
> buffer that might not have reached the disk yet. Only data integrity
> writeback, i.e. fsync() and friends, clears it again. The flusher and
> sync(2) leave it alone.
> 
> inode_lru_isolate() treats every state bit other than I_REFERENCED as a
> reason not to reclaim the inode right now. It takes the inode off the
> LRU and relies on iput(), writeback completion or the removal of the
> last page from the page cache to put it back once the reason is gone.
> 
> But I_METADATA_WRITEBACK doesn't go away on its own. iput() puts the
> inode back on the LRU with the bit still set and the next scan takes it
> off again. So neither memory pressure nor drop_caches can free these
> [ ... ]
> 
> Fixes: c474bc56b6d1 ("fs: Provide way for filesystem to wait for metadata writeback")
> Cc: stable@vger.kernel.org # v7.3; see patch description
> Reported-by: Josef Bacik <josef@toxicpanda.com>
> Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>

Sashiko has reviewed this patch and found no issues.

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

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

[sashiko-bot]

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