[PATCH RFC 0/9] fs: reclaim inodes with I_METADATA_WRITEBACK

[PATCH RFC 0/9] 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 but nothing ever puts it back.

So these inodes can't be reclaimed anymore. They stay around until
something fsyncs them or the filesystem is unmounted. Which is
effectively 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 evict() write out what I_METADATA_WRITEBACK tracks
unless the inode is being deleted. That's what the filesystems already
do for the metadata buffers they track in ->evict_inode().

The first seven patches make ->sync_inode_metadata() of ext2, ext4,
minix and udf look the inode block up in the buffer cache instead of
reading it from disk, the way fat already does. A block that isn't
cached is neither dirty nor under writeback so there's nothing to write
or to wait for. That matters once evict() calls it from reclaim.

The eighth patch is a fat fix in the same area. fat_evict_inode()
updates the directory entry of a file with fallocated clusters and lets
the inode go without writing it out. So the freed clusters can be free
on disk while the directory entry still points at them.

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 and will send it as
an RFC in a minute.

All of this is meant for v7.3 stable. v7.3 still has bfs which sets
I_METADATA_WRITEBACK as well, so the backport also needs a cache-only
lookup in bfs_sync_inode_metadata().

Tested on 65ba8f34ec88 with KASAN and lockdep: the reproducer in the
last patch leaves 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. The
reproducer in the fat patch finds /a pointing at a free cluster on disk
without the series and a clean /a with it. 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.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
Christian Brauner (9):
      ext2: add ext2_inode_block() helper
      ext2: don't read the inode block in ext2_sync_inode_metadata()
      ext4: add ext4_inode_loc_block() helper
      ext4: don't read the inode block in ext4_sync_inode_metadata()
      minix: add minix_inode_block() helper
      minix: don't read the inode block in minix_sync_inode_metadata()
      udf: don't allocate the inode block in udf_sync_inode_metadata()
      fat: sync the inode in fat_free_eofblocks()
      fs: don't strand I_METADATA_WRITEBACK inodes outside the LRU

 fs/ext2/inode.c   | 61 ++++++++++++++++++++++++++++++++++---------------------
 fs/ext4/inode.c   | 48 ++++++++++++++++++++++++++++++-------------
 fs/fat/inode.c    |  2 +-
 fs/inode.c        | 13 +++++++++++-
 fs/minix/bitmap.c | 37 ++++++++++++++++++++-------------
 fs/minix/inode.c  | 16 +++++++--------
 fs/minix/minix.h  |  1 +
 fs/udf/inode.c    | 27 ++++++++++++------------
 8 files changed, 130 insertions(+), 75 deletions(-)
---
base-commit: 65ba8f34ec886ec72b2c07ddc8cf3d815c32869d
change-id: 20261029-work-inode-metadata_writeback-d880a0429e49

[PATCH RFC 1/9] 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
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..940f61a4dde3 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, "ext2_get_inode", "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

Re: [PATCH RFC 1/9] 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
> 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/20261029-work-inode-metadata_writeback-v1-0-7d6c678a585d@kernel.org?part=1

[PATCH RFC 2/9] ext2: don't read the inode block in ext2_sync_inode_metadata()

[Christian Brauner]

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

It's also not something we can afford once inode eviction calls
->sync_inode_metadata() from reclaim. So look the block up with
sb_find_get_block_nonatomic() instead, the same way
fat_sync_inode_metadata() already does.

Cc: stable@vger.kernel.org # v7.3
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 940f61a4dde3..efb50cb48da3 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

Re: [PATCH RFC 2/9] ext2: don't read the inode block in ext2_sync_inode_metadata()

[sashiko-bot]

> ext2_sync_inode_metadata() looks up the inode block with
> ext2_get_inode() which reads it from disk if it isn't cached. But a
> block that isn't in the buffer cache is neither dirty nor under
> writeback. So there's nothing to write or to wait for and reading it in
> is pointless.
> 
> It's also not something we can afford once inode eviction calls
> ->sync_inode_metadata() from reclaim. So look the block up with
> sb_find_get_block_nonatomic() instead, the same way
> fat_sync_inode_metadata() already does.
> 
> 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/20261029-work-inode-metadata_writeback-v1-0-7d6c678a585d@kernel.org?part=2

[PATCH RFC 3/9] ext4: add ext4_inode_loc_block() helper

[Christian Brauner]

Move the lookup of the inode table block out of
ext4_get_inode_loc_noio() into a helper. ext4_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
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/ext4/inode.c | 38 +++++++++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 11 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index b4d581aa5ee1..b34b99d15db6 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5874,16 +5874,14 @@ 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.
+ * Find the inode table block that holds @inode without looking at the block
+ * itself. Fills in iloc->block_group and iloc->offset.
  */
-int ext4_get_inode_loc_noio(struct inode *inode, struct ext4_iloc *iloc)
+static int ext4_inode_loc_block(struct inode *inode, struct ext4_iloc *iloc,
+				ext4_fsblk_t *block)
 {
 	struct super_block *sb = inode->i_sb;
 	struct ext4_group_desc *gdp;
-	struct buffer_head *bh;
-	ext4_fsblk_t block;
 	int inodes_per_block, inode_offset;
 	unsigned long ino = inode->i_ino;
 
@@ -5902,15 +5900,33 @@ int ext4_get_inode_loc_noio(struct inode *inode, struct ext4_iloc *iloc)
 	inode_offset = ((ino - 1) % EXT4_INODES_PER_GROUP(sb));
 	iloc->offset = (inode_offset % inodes_per_block) * EXT4_INODE_SIZE(sb);
 
-	block = ext4_inode_table(sb, gdp);
-	if (block <= le32_to_cpu(EXT4_SB(sb)->s_es->s_first_data_block) ||
-	    block >= ext4_blocks_count(EXT4_SB(sb)->s_es)) {
+	*block = ext4_inode_table(sb, gdp);
+	if (*block <= le32_to_cpu(EXT4_SB(sb)->s_es->s_first_data_block) ||
+	    *block >= ext4_blocks_count(EXT4_SB(sb)->s_es)) {
 		ext4_error(sb,
 			   "Invalid inode table block %llu in block_group %u",
-			   block, iloc->block_group);
+			   *block, iloc->block_group);
 		return -EFSCORRUPTED;
 	}
-	block += inode_offset / inodes_per_block;
+	*block += inode_offset / inodes_per_block;
+	return 0;
+}
+
+/*
+ * 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.
+ */
+int ext4_get_inode_loc_noio(struct inode *inode, struct ext4_iloc *iloc)
+{
+	struct super_block *sb = inode->i_sb;
+	struct buffer_head *bh;
+	ext4_fsblk_t block;
+	int err;
+
+	err = ext4_inode_loc_block(inode, iloc, &block);
+	if (err)
+		return err;
 
 	bh = sb_find_get_block(sb, block);
 	if (!bh)

-- 
2.53.0

Re: [PATCH RFC 3/9] ext4: add ext4_inode_loc_block() helper

[sashiko-bot]

> Move the lookup of the inode table block out of
> ext4_get_inode_loc_noio() into a helper. ext4_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
> 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/20261029-work-inode-metadata_writeback-v1-0-7d6c678a585d@kernel.org?part=3

[PATCH RFC 4/9] ext4: don't read the inode block in ext4_sync_inode_metadata()

[Christian Brauner]

In nojournal mode ext4_sync_inode_metadata() looks up the inode table
block 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 in the buffer cache is neither dirty nor under writeback. So
there's nothing to write or to wait for and reading it in is pointless.

It's also not something we can afford once inode eviction calls
->sync_inode_metadata() from reclaim. So look the block up with
sb_find_get_block_nonatomic() instead.

Note that ext4_get_inode_loc_noio() isn't usable here. It uses the
atomic lookup and that one gives up on a folio that is being migrated.
That's fine for a best-effort fast commit snapshot but not for a data
integrity sync.

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

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index b34b99d15db6..cab26ec01aba 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -6824,28 +6824,32 @@ int ext4_sync_inode_metadata(struct inode *inode, struct writeback_control *wbc)
 {
 	struct ext4_iloc iloc;
 	struct mapping_metadata_bhs *mmb;
+	ext4_fsblk_t block;
 	int err;
 
 	/* We should only get here in nojournal mode */
 	if (WARN_ON_ONCE(EXT4_SB(inode->i_sb)->s_journal))
 		return -EFSCORRUPTED;
 
-	err = __ext4_get_inode_loc_noinmem(inode, &iloc);
+	err = ext4_inode_loc_block(inode, &iloc, &block);
 	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. */
+	iloc.bh = sb_find_get_block_nonatomic(inode->i_sb, block);
+	if (!iloc.bh)
+		return 0;
 	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

Re: [PATCH RFC 4/9] ext4: don't read the inode block in ext4_sync_inode_metadata()

[sashiko-bot]

> In nojournal mode ext4_sync_inode_metadata() looks up the inode table
> block 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 in the buffer cache is neither dirty nor under writeback. So
> there's nothing to write or to wait for and reading it in is pointless.
> 
> It's also not something we can afford once inode eviction calls
> ->sync_inode_metadata() from reclaim. So look the block up with
> sb_find_get_block_nonatomic() instead.
> 
> Note that ext4_get_inode_loc_noio() isn't usable here. It uses the
> atomic lookup and that one gives up on a folio that is being migrated.
> That's fine for a best-effort fast commit snapshot but not for a data
> integrity sync.
> 
> 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/20261029-work-inode-metadata_writeback-v1-0-7d6c678a585d@kernel.org?part=4

[PATCH RFC 5/9] 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
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

Re: [PATCH RFC 5/9] 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
> 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/20261029-work-inode-metadata_writeback-v1-0-7d6c678a585d@kernel.org?part=5

[PATCH RFC 6/9] minix: don't read the inode block in minix_sync_inode_metadata()

[Christian Brauner]

minix_sync_inode_metadata() looks up the inode block 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 in the buffer cache is neither
dirty nor under writeback. So there's nothing to write or to wait for
and reading it in is pointless.

It's also not something we can 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
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

Re: [PATCH RFC 6/9] minix: don't read the inode block in minix_sync_inode_metadata()

[sashiko-bot]

> minix_sync_inode_metadata() looks up the inode block 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 in the buffer cache is neither
> dirty nor under writeback. So there's nothing to write or to wait for
> and reading it in is pointless.
> 
> It's also not something we can 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
> 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/20261029-work-inode-metadata_writeback-v1-0-7d6c678a585d@kernel.org?part=6

[PATCH RFC 7/9] udf: don't allocate the inode block in udf_sync_inode_metadata()

[Christian Brauner]

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

It's also not something we can 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
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/udf/inode.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index bacf2a86161e..59f8e9bde0b1 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -1744,23 +1744,22 @@ int udf_sync_inode_metadata(struct inode *inode, struct writeback_control *wbc)
 	struct buffer_head *bh;
 	int err = 0;
 
-	bh = sb_getblk(inode->i_sb,
+	/* A block that isn't cached is neither dirty nor under writeback. */
+	bh = sb_find_get_block_nonatomic(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;
+	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

Re: [PATCH RFC 7/9] udf: don't allocate the inode block in udf_sync_inode_metadata()

[sashiko-bot]

> udf_sync_inode_metadata() looks up the block that holds the inode with
> sb_getblk() which allocates a buffer for it if it isn't cached. But a
> block that isn't in the buffer cache is neither dirty nor under
> writeback. So there's nothing to write or to wait for and allocating a
> buffer that just gets thrown away again is pointless.
> 
> It's also not something we can 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
> 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/20261029-work-inode-metadata_writeback-v1-0-7d6c678a585d@kernel.org?part=7

[PATCH RFC 8/9] fat: sync the inode in fat_free_eofblocks()

[Christian Brauner]

fat_evict_inode() calls fat_free_eofblocks() for a linked inode to give
back the clusters that were fallocated past the initialized size. For an
empty file that also resets the start cluster in the directory entry. It
truncates the clusters and writes the inode with:

  sync_inode_metadata(inode, inode_needs_sync(inode));

Unless the inode is IS_SYNC that is WB_SYNC_NONE writeback. It copies
the inode into the directory entry buffer, marks the buffer dirty and
sets I_METADATA_WRITEBACK. Then the inode goes away and with it the only
thing that would have told a later fsync() that the directory entry
still has to be written.

FAT doesn't have that problem. Its buffers are tracked on the FAT inode
and fat_file_fsync() writes them out on every fsync() of any file on the
filesystem.

So after the next fsync() the clusters are free on disk while the
directory entry of the empty file still points at them. If another file
reuses them and the machine crashes before the directory entry makes it
to disk, the two files share clusters. That's the corruption the comment
in fat_free_eofblocks() is about.

So wait for the directory entry to reach the disk before the inode goes
away.

Reproducer. The image file shows what reached the disk. The flusher is
turned off so that it doesn't write the directory entry before fsck.fat
looks at the image:

  img=/tmp/fat.img
  truncate -s 64M $img
  mkfs.vfat $img > /dev/null
  dev=$(losetup -f --show $img)
  mount -t vfat $dev /mnt
  echo 0 > /proc/sys/vm/dirty_writeback_centisecs
  xfs_io -f -c 'falloc -k 0 1m' -c fsync /mnt/a
  echo 3 > /proc/sys/vm/drop_caches
  echo 3 > /proc/sys/vm/drop_caches
  xfs_io -c fsync /mnt/a
  fsck.fat -n $img | grep -A1 '^/a'
  umount /mnt
  losetup -d $dev

On v7.3-17318-g65ba8f34ec88 in a KASAN and lockdep vng guest this
prints:

  /a
    Contains a free cluster (3). Assuming EOF.

With this patch fsck.fat finds nothing wrong with /a.

The stable tag is narrower than the Fixes tag. Before v7.3
fat_free_eofblocks() called __fat_write_inode() directly and the fix
there is to pass 1 for wait.

Fixes: b13bb33eacb7 ("fat: add fat_fallocate operation")
Cc: stable@vger.kernel.org # v7.3
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/fat/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index d85c8fbdcca4..fc571ee2b174 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -675,7 +675,7 @@ static void fat_free_eofblocks(struct inode *inode)
 		 * any corruption on the next access to the cluster
 		 * chain for the file.
 		 */
-		err = sync_inode_metadata(inode, inode_needs_sync(inode));
+		err = sync_inode_metadata(inode, 1);
 		if (err) {
 			fat_msg(inode->i_sb, KERN_WARNING, "Failed to "
 					"update on disk inode for unused "

-- 
2.53.0

Re: [PATCH RFC 8/9] fat: sync the inode in fat_free_eofblocks()

[sashiko-bot]

> fat_evict_inode() calls fat_free_eofblocks() for a linked inode to give
> back the clusters that were fallocated past the initialized size. For an
> empty file that also resets the start cluster in the directory entry. It
> truncates the clusters and writes the inode with:
> 
>   sync_inode_metadata(inode, inode_needs_sync(inode));
> 
> Unless the inode is IS_SYNC that is WB_SYNC_NONE writeback. It copies
> the inode into the directory entry buffer, marks the buffer dirty and
> sets I_METADATA_WRITEBACK. Then the inode goes away and with it the only
> thing that would have told a later fsync() that the directory entry
> still has to be written.
> 
> FAT doesn't have that problem. Its buffers are tracked on the FAT inode
> and fat_file_fsync() writes them out on every fsync() of any file on the
> [ ... ]
> there is to pass 1 for wait.
> 
> Fixes: b13bb33eacb7 ("fat: add fat_fallocate operation")
> 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/20261029-work-inode-metadata_writeback-v1-0-7d6c678a585d@kernel.org?part=8

[PATCH RFC 9/9] 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. So a clean, unused inode keeps the bit around.

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. Using the inode
again puts it 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 this is especially bad because
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 writing it out on eviction is exactly what these
filesystems already do for the metadata buffers they track in their
mapping_metadata_bhs.

ext2, ext4, fat, udf and minix all call mmb_sync() from ->evict_inode()
and that is what allowed commit 972b9dd4e418 ("fs: Ignore inode metadata
buffers in inode_lru_isolate()") to take metadata buffers out of
inode_lru_isolate().

So let the shrinker reclaim inodes with I_METADATA_WRITEBACK like any
other clean inode and make evict() call sync_inode_metadata() for them
unless they are being deleted. With the previous patches
->sync_inode_metadata() doesn't read anything from disk anymore. It
writes and waits for whatever is still dirty or under writeback and
usually the flusher has long taken care of that by the time an unused
inode reaches the end of the LRU.

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

For stable this needs the previous patches as well. Without them
evict() ends up reading inode blocks from disk with __GFP_NOFAIL
allocations from reclaim. v7.3 also still has bfs. It sets
I_METADATA_WRITEBACK too and bfs_sync_inode_metadata() reads the inode
block with sb_bread(). So the v7.3 backport needs to switch bfs over to
sb_find_get_block_nonatomic() as well.

Reproducer, one filesystem at a time:

  #!/bin/bash
  # usage: <ext2|ext4|vfat|minix|udf> <dev>
  fs=$1 dev=$2 n=5000
  case $fs in
  ext2)  mkfs.ext2 -q -F $dev ;;
  ext4)  mkfs.ext4 -q -F -O ^has_journal $dev ;;
  vfat)  mkfs.vfat $dev ;;
  minix) mkfs.minix -3 $dev ;;
  udf)   mkudffs --media-type=hd $dev ;;
  esac
  mount -t $fs $dev /mnt && mkdir /mnt/d && sync
  echo 3 > /proc/sys/vm/drop_caches
  before=$(cut -f1 /proc/sys/fs/inode-nr)
  for ((i = 0; i < n; i++)); do echo x > /mnt/d/f$i; done
  sync
  echo 3 > /proc/sys/vm/drop_caches
  echo 3 > /proc/sys/vm/drop_caches
  echo "$fs: $(( $(cut -f1 /proc/sys/fs/inode-nr) - before )) of $n inodes left"
  umount /mnt

On v7.3-17318-g65ba8f34ec88 in a KASAN and lockdep vng guest with brd
devices:

             without   with this series
    ext2       5001     1
    ext4       4944     0
    vfat       5001     0
    minix      4999     0
    udf        4992     4992

udf needs the flusher to come by first. Its inodes with the data in the
ICB are still dirty after sync(2), with or without this series. When
drop_caches runs after the flusher wrote them 453 of 500 udf inodes stay
without this series and none with it.

And creating 60000 files from a task in a memory cgroup with
memory.max=96M and a sync every 2000 files gets the task OOM killed
after 55524 files on ext2 and after 43222 files on ext4 without a
journal. With this series all 60000 files get created and nothing is
killed.

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 | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/fs/inode.c b/fs/inode.c
index 1cb6293b237a..332108961df6 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -818,6 +818,14 @@ static void evict(struct inode *inode)
 	inode_wait_for_writeback(inode);
 	spin_unlock(&inode->i_lock);
 
+	/*
+	 * fsync() can't find what I_METADATA_WRITEBACK tracks anymore once the
+	 * inode is gone, so write it out unless the inode is being deleted.
+	 */
+	if (inode->i_nlink &&
+	    (inode_state_read_once(inode) & I_METADATA_WRITEBACK))
+		sync_inode_metadata(inode, 1);
+
 	if (op->evict_inode) {
 		op->evict_inode(inode);
 	} else {
@@ -947,9 +955,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() 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 9/9] 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. So a clean, unused inode keeps the bit around.
> 
> 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. Using the inode
> again puts it 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
> [ ... ]
> 
> 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/20261029-work-inode-metadata_writeback-v1-0-7d6c678a585d@kernel.org?part=9