[PATCH RFC v3 00/14] fs: reclaim inodes with I_METADATA_WRITEBACK

[PATCH RFC v3 00/14] 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 evict() write out what I_METADATA_WRITEBACK tracks
unless the inode is being deleted.

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, so that calling it from
reclaim is fine. The eighth sets I_METADATA_WRITEBACK from
mmb_mark_buffer_dirty() so that the bit covers the tracked metadata
buffers as well. The ninth is the actual fix. The last five drop the
mmb_sync() calls from ->evict_inode() that evict() makes redundant.

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 cache-only
lookup for bfs too.

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 none 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 v3:
- Set I_METADATA_WRITEBACK from mmb_mark_buffer_dirty() and write out
  what it tracks from evict() again instead of from every
  ->evict_inode() (Jan).
- Drop the mmb_sync() calls from the ->evict_inode() of ext2, ext4,
  minix, udf and fat.
- Jan's Reviewed-by on the fix isn't carried over since the patch
  changed.
- Link to v2: https://patch.msgid.link/20261105-work-inode-metadata_writeback-v2-0-48628c60c336@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 (14):
      ext2: add ext2_inode_block() helper
      ext2: don't read the inode block in ext2_sync_inode_metadata()
      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()
      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()
      fs: set I_METADATA_WRITEBACK in mmb_mark_buffer_dirty()
      fs: don't strand I_METADATA_WRITEBACK inodes outside the LRU
      ext2: drop mmb_sync() from ext2_evict_inode()
      ext4: drop mmb_sync() from ext4_evict_inode()
      minix: drop mmb_sync() from minix_evict_inode()
      udf: drop mmb_sync() from udf_evict_inode()
      fat: drop mmb_sync() from fat_evict_inode()

 fs/buffer.c       | 16 ++++++++++++--
 fs/ext2/inode.c   | 63 +++++++++++++++++++++++++++++++++----------------------
 fs/ext4/inode.c   | 22 +++++++++----------
 fs/fat/inode.c    |  1 -
 fs/inode.c        | 13 +++++++++++-
 fs/minix/bitmap.c | 37 +++++++++++++++++++-------------
 fs/minix/inode.c  | 18 +++++++---------
 fs/minix/minix.h  |  1 +
 fs/udf/inode.c    | 33 +++++++++++++----------------
 9 files changed, 121 insertions(+), 83 deletions(-)
---
base-commit: 65ba8f34ec886ec72b2c07ddc8cf3d815c32869d
change-id: 20261029-work-inode-metadata_writeback-d880a0429e49

[PATCH RFC v3 01/14] 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

Re: [PATCH RFC v3 01/14] 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/20261109-work-inode-metadata_writeback-v3-0-91ac6c024018@kernel.org?part=1

[PATCH RFC v3 02/14] 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

Re: [PATCH RFC v3 02/14] 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/20261109-work-inode-metadata_writeback-v3-0-91ac6c024018@kernel.org?part=2

[PATCH RFC v3 03/14] 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
Reviewed-by: Jan Kara <jack@suse.cz>
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

Re: [PATCH RFC v3 03/14] 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
> 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/20261109-work-inode-metadata_writeback-v3-0-91ac6c024018@kernel.org?part=3

[PATCH RFC v3 04/14] 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
Reviewed-by: Jan Kara <jack@suse.cz>
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

Re: [PATCH RFC v3 04/14] 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
> 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/20261109-work-inode-metadata_writeback-v3-0-91ac6c024018@kernel.org?part=4

[PATCH RFC v3 05/14] 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

Re: [PATCH RFC v3 05/14] 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/20261109-work-inode-metadata_writeback-v3-0-91ac6c024018@kernel.org?part=5

[PATCH RFC v3 06/14] 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

Re: [PATCH RFC v3 06/14] 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/20261109-work-inode-metadata_writeback-v3-0-91ac6c024018@kernel.org?part=6

[PATCH RFC v3 07/14] 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

Re: [PATCH RFC v3 07/14] 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/20261109-work-inode-metadata_writeback-v3-0-91ac6c024018@kernel.org?part=7

[PATCH RFC v3 08/14] fs: set I_METADATA_WRITEBACK in mmb_mark_buffer_dirty()

[Christian Brauner]

ext2, ext4 without a journal, fat, udf and minix track the metadata
buffers of an inode in a mapping_metadata_bhs list and sync them from
->sync_inode_metadata() and ->evict_inode(). The inode block itself is
only tracked by I_METADATA_WRITEBACK which ->write_inode() sets after
copying the inode into its buffer.

Set the bit in mmb_mark_buffer_dirty() as well so that it covers all
the metadata an inode may still have to write out. Set it after the
buffer is on the list. An fsync() that sees the bit then also finds the
buffer when it walks the list, and one that cleared the bit before we
set it leaves it set for the next one.

The inode itself may never have been dirtied. fat for example tracks
the FAT buffers on a private inode that nothing marks dirty. So attach
the inode to a wb before setting the bit, the way ext4_mark_iloc_dirty()
does, or writeback_single_inode() has no wb to lock once evict() syncs
it.

That lets evict() write out everything the bit tracks in one place
instead of every ->evict_inode() calling mmb_sync() itself.

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

diff --git a/fs/buffer.c b/fs/buffer.c
index 5d3852b726d6..f951559e7d32 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -621,12 +621,15 @@ void write_boundary_block(struct block_device *bdev,
  * @bh: The buffer to mark dirty.
  * @mmb: The list of buffers to add the buffer to.
  *
- * Mark the buffer dirty and add it to the list if it is not already on
- * a list.
+ * Mark the buffer dirty, add it to the list if it is not already on a list
+ * and set I_METADATA_WRITEBACK on the inode so that fsync() and eviction
+ * write it out.
  */
 void mmb_mark_buffer_dirty(struct buffer_head *bh,
 			   struct mapping_metadata_bhs *mmb)
 {
+	struct inode *inode = mmb->mapping->host;
+
 	mark_buffer_dirty(bh);
 	if (!bh->b_mmb) {
 		spin_lock(&mmb->lock);
@@ -641,6 +644,15 @@ void mmb_mark_buffer_dirty(struct buffer_head *bh,
 		list_move_tail(&bh->b_assoc_buffers, &mmb->list);
 		spin_unlock(&mmb->lock);
 	}
+	/*
+	 * The inode itself may never have been dirtied, so attach it to a wb
+	 * before setting the bit or writeback_single_inode() has nothing to
+	 * lock when evict() syncs it.
+	 */
+	spin_lock(&inode->i_lock);
+	inode_attach_wb(inode, NULL);
+	inode_state_set(inode, I_METADATA_WRITEBACK);
+	spin_unlock(&inode->i_lock);
 }
 EXPORT_SYMBOL(mmb_mark_buffer_dirty);
 

-- 
2.53.0

Re: [PATCH RFC v3 08/14] fs: set I_METADATA_WRITEBACK in mmb_mark_buffer_dirty()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] ext4: crash in fastcommit replay when mmb_mark_buffer_dirty()
  sets writeback
- [Medium] fs/buffer: early return in mmb_mark_buffer_dirty() skips
  I_METADATA_WRITEBACK
- [Medium] fs/buffer: mmb_mark_buffer_dirty() sets b_mmb before adding
  bh to mmb->list
--

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

fs: set I_METADATA_WRITEBACK in mmb_mark_buffer_dirty()

This commit attaches the inode to a writeback structure and sets
I_METADATA_WRITEBACK in mmb_mark_buffer_dirty() so that fsync() and
inode eviction write out dirty metadata buffers tracked on the mapping.

> diff --git a/fs/buffer.c b/fs/buffer.c
> index 5d3852b726d6b..f951559e7d32c 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -621,12 +621,15 @@ void write_boundary_block(struct block_device *bdev,
>   * @bh: The buffer to mark dirty.
>   * @mmb: The list of buffers to add the buffer to.
>   *
> - * Mark the buffer dirty and add it to the list if it is not already on
> - * a list.
> + * Mark the buffer dirty, add it to the list if it is not already on a list
> + * and set I_METADATA_WRITEBACK on the inode so that fsync() and eviction
> + * write it out.
>   */
>  void mmb_mark_buffer_dirty(struct buffer_head *bh,
>  			   struct mapping_metadata_bhs *mmb)
>  {
> +	struct inode *inode = mmb->mapping->host;
> +
>  	mark_buffer_dirty(bh);
>  	if (!bh->b_mmb) {
>  		spin_lock(&mmb->lock);
> @@ -641,6 +644,15 @@ void mmb_mark_buffer_dirty(struct buffer_head *bh,

[Severity: Medium]
Can a concurrent mmb_mark_buffer_dirty() set I_METADATA_WRITEBACK before
bh is linked onto mmb->list?

CPU0                                    CPU1
mmb_mark_buffer_dirty(bh, mmb)
  spin_lock(&mmb->lock)
  cmpxchg(&bh->b_mmb, NULL, mmb)
                                        mmb_mark_buffer_dirty(bh, mmb)
                                          if (!bh->b_mmb) // false
                                          inode_state_set(...,
                                            I_METADATA_WRITEBACK)
                                        fsync()
                                          __writeback_single_inode()
                                            inode_state_clear(...,
                                              I_METADATA_WRITEBACK)
                                            mmb_sync(mmb)
                                              if (!mmb_has_buffers(mmb))
                                                return 0;  <-- skips bh
  list_move_tail(..., &mmb->list)
  spin_unlock(&mmb->lock)

Because cmpxchg() sets bh->b_mmb before list_move_tail() adds bh to
mmb->list, CPU1 sees bh->b_mmb != NULL without taking mmb->lock, sets
I_METADATA_WRITEBACK, and returns while mmb->list is still empty.  An
intervening fsync() can then clear I_METADATA_WRITEBACK and return early
from mmb_sync() when its lockless mmb_has_buffers() check sees an empty
list, leaving bh unsynced.

[Severity: Medium]
Can the early return on cmpxchg(&bh->b_mmb, NULL, mmb) != NULL skip
setting I_METADATA_WRITEBACK when racing with mmb_sync()?

CPU0                                    CPU1
__writeback_single_inode()
  inode_state_clear(...,
    I_METADATA_WRITEBACK)
  mmb_sync(mmb)
    spin_lock(&mmb->lock)
    __remove_assoc_queue(mmb, bh)
      bh->b_mmb = NULL
                                        mmb_mark_buffer_dirty(bh, mmb)
                                          mark_buffer_dirty(bh)
                                          if (!bh->b_mmb) // true
    smp_mb()
    if (buffer_dirty(bh))
      list_add(..., &mmb->list)
      bh->b_mmb = mmb
    spin_unlock(&mmb->lock)
                                          spin_lock(&mmb->lock)
                                          cmpxchg(&bh->b_mmb, NULL, mmb)
                                            // returns mmb != NULL
                                          spin_unlock(&mmb->lock)
                                          return;  <-- skips setting bit

__writeback_single_inode() clears I_METADATA_WRITEBACK before calling
sync_inode_metadata() -> mmb_sync().  In the second pass of mmb_sync(),
__remove_assoc_queue() transiently clears bh->b_mmb and then restores
bh->b_mmb = mmb after seeing BH_Dirty from CPU1.  CPU1 then fails the
cmpxchg() and returns early without setting I_METADATA_WRITEBACK, leaving
the redirtied buffer on mmb->list with I_METADATA_WRITEBACK clear on the
inode so a subsequent fsync() or evict() will not write it out.

>  		list_move_tail(&bh->b_assoc_buffers, &mmb->list);
>  		spin_unlock(&mmb->lock);
>  	}
> +	/*
> +	 * The inode itself may never have been dirtied, so attach it to a wb
> +	 * before setting the bit or writeback_single_inode() has nothing to
> +	 * lock when evict() syncs it.
> +	 */
> +	spin_lock(&inode->i_lock);
> +	inode_attach_wb(inode, NULL);
> +	inode_state_set(inode, I_METADATA_WRITEBACK);
> +	spin_unlock(&inode->i_lock);
>  }
>  EXPORT_SYMBOL(mmb_mark_buffer_dirty);

[Severity: High]
Does setting I_METADATA_WRITEBACK here cause a NULL pointer dereference
during ext4 fastcommit replay?

0ecd56573c1f ("ext4: Avoid entering writeback paths during fastcommit
replay") guarded ext4_mark_iloc_dirty() against EXT4_FC_REPLAY so it
would not set I_METADATA_WRITEBACK while SB_ACTIVE is still clear and
s_writepages_rwsem is not yet initialized.  However, fastcommit replay
paths such as ext4_fc_replay_create() and ext4_fc_replay_add_range()
dirty directory or extent blocks with handle == NULL and a non-NULL
inode, which reaches mmb_mark_buffer_dirty():

fs/ext4/ext4_jbd2.c:__ext4_handle_dirty_metadata() {
    ...
	} else {
		if (inode) {
			if (!ext4_i_metadata_bhs(inode))
				ext4_inode_attach_mmb(inode);
			mmb_mark_buffer_dirty(bh, ext4_i_metadata_bhs(inode));
    ...
}

When replay finishes with the inode and calls iput(), iput_final() sees
!(sb->s_flags & SB_ACTIVE) and calls write_inode_now(inode, 1).  With
I_METADATA_WRITEBACK now set, __writeback_single_inode() runs
do_writepages() -> ext4_writepages() -> percpu_down_read() on the
uninitialized s_writepages_rwsem, crashing the kernel, and also calls
ext4_sync_inode_metadata(), which triggers
WARN_ON_ONCE(EXT4_SB(inode->i_sb)->s_journal) and returns -EFSCORRUPTED.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-work-inode-metadata_writeback-v3-0-91ac6c024018@kernel.org?part=8

Re: [PATCH RFC v3 08/14] fs: set I_METADATA_WRITEBACK in mmb_mark_buffer_dirty()

[Jan Kara]

On Mon 09-11-26 17:51:23, Christian Brauner wrote:
> ext2, ext4 without a journal, fat, udf and minix track the metadata
> buffers of an inode in a mapping_metadata_bhs list and sync them from
> ->sync_inode_metadata() and ->evict_inode(). The inode block itself is
> only tracked by I_METADATA_WRITEBACK which ->write_inode() sets after
> copying the inode into its buffer.
> 
> Set the bit in mmb_mark_buffer_dirty() as well so that it covers all
> the metadata an inode may still have to write out. Set it after the
> buffer is on the list. An fsync() that sees the bit then also finds the
> buffer when it walks the list, and one that cleared the bit before we
> set it leaves it set for the next one.
> 
> The inode itself may never have been dirtied. fat for example tracks
> the FAT buffers on a private inode that nothing marks dirty. So attach
> the inode to a wb before setting the bit, the way ext4_mark_iloc_dirty()
> does, or writeback_single_inode() has no wb to lock once evict() syncs
> it.
> 
> That lets evict() write out everything the bit tracks in one place
> instead of every ->evict_inode() calling mmb_sync() itself.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Cc: stable@vger.kernel.org # v7.3
> Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>

Looks good, just one nit below. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

> @@ -641,6 +644,15 @@ void mmb_mark_buffer_dirty(struct buffer_head *bh,
>  		list_move_tail(&bh->b_assoc_buffers, &mmb->list);
>  		spin_unlock(&mmb->lock);
>  	}
> +	/*
> +	 * The inode itself may never have been dirtied, so attach it to a wb
> +	 * before setting the bit or writeback_single_inode() has nothing to
> +	 * lock when evict() syncs it.
> +	 */
> +	spin_lock(&inode->i_lock);
> +	inode_attach_wb(inode, NULL);
> +	inode_state_set(inode, I_METADATA_WRITEBACK);
> +	spin_unlock(&inode->i_lock);
>  }

I'd wrap this all in:

	if (!(inode_state_read_once(inode) & I_METADATA_WRITEBACK))

so that we save i_lock round-trip for the common case where
I_METADATA_WRITEBACK is already set.

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

[PATCH RFC v3 09/14] 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. So let the shrinker reclaim inodes with
I_METADATA_WRITEBACK like any other clean inode and make evict() write
out what the bit tracks unless the inode is being deleted. Since the
previous patch the bit covers the tracked metadata buffers as well, and
since the lookup patches ->sync_inode_metadata() doesn't read anything
from disk for that. It writes and waits for whatever is still dirty or
under writeback. The mmb_sync() calls in ->evict_inode() are redundant
with that and the following patches drop them.

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 lookup
changes evict() ends up reading inode blocks from disk with
__GFP_NOFAIL allocations from reclaim. v7.3 also still has bfs which
sets I_METADATA_WRITEBACK too and reads the inode block in
bfs_sync_inode_metadata(), so the backport needs the cache-only lookup
for bfs as well.

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 v3 09/14] 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/20261109-work-inode-metadata_writeback-v3-0-91ac6c024018@kernel.org?part=9

Re: [PATCH RFC v3 09/14] fs: don't strand I_METADATA_WRITEBACK inodes outside the LRU

[Jan Kara]

On Mon 09-11-26 17:51:24, Christian Brauner wrote:
> 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. So let the shrinker reclaim inodes with
> I_METADATA_WRITEBACK like any other clean inode and make evict() write
> out what the bit tracks unless the inode is being deleted. Since the
> previous patch the bit covers the tracked metadata buffers as well, and
> since the lookup patches ->sync_inode_metadata() doesn't read anything
> from disk for that. It writes and waits for whatever is still dirty or
> under writeback. The mmb_sync() calls in ->evict_inode() are redundant
> with that and the following patches drop them.
> 
> 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 lookup
> changes evict() ends up reading inode blocks from disk with
> __GFP_NOFAIL allocations from reclaim. v7.3 also still has bfs which
> sets I_METADATA_WRITEBACK too and reads the inode block in
> bfs_sync_inode_metadata(), so the backport needs the cache-only lookup
> for bfs as well.
> 
> 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>

Looka good! Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

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

[PATCH RFC v3 10/14] ext2: drop mmb_sync() from ext2_evict_inode()

[Christian Brauner]

evict() writes out what I_METADATA_WRITEBACK tracks before it calls
->evict_inode() and since mmb_mark_buffer_dirty() sets the bit that
includes the tracked metadata buffers. So there's nothing left for
ext2_evict_inode() to sync.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/ext2/inode.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index 9261b666d043..4c38a3a8ea4d 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -91,8 +91,6 @@ void ext2_evict_inode(struct inode * inode)
 		if (inode->i_blocks)
 			ext2_truncate_blocks(inode, 0);
 		ext2_xattr_delete_inode(inode);
-	} else {
-		mmb_sync(&EXT2_I(inode)->i_metadata_bhs);
 	}
 	mmb_invalidate(&EXT2_I(inode)->i_metadata_bhs);
 	clear_inode(inode);

-- 
2.53.0

Re: [PATCH RFC v3 10/14] ext2: drop mmb_sync() from ext2_evict_inode()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] ext2: unsynced metadata buffers in ext2_evict_inode() on
  mmb_sync() race
--

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

ext2: drop mmb_sync() from ext2_evict_inode()

This commit removes the mmb_sync() call from ext2_evict_inode(), relying on
evict() to write out metadata buffers tracked via I_METADATA_WRITEBACK.

> diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> index 9261b666d043a..4c38a3a8ea4d7 100644
> --- a/fs/ext2/inode.c
> +++ b/fs/ext2/inode.c
> @@ -91,8 +91,6 @@ void ext2_evict_inode(struct inode * inode)
>  		if (inode->i_blocks)
>  			ext2_truncate_blocks(inode, 0);
>  		ext2_xattr_delete_inode(inode);
> -	} else {
> -		mmb_sync(&EXT2_I(inode)->i_metadata_bhs);
>  	}
>  	mmb_invalidate(&EXT2_I(inode)->i_metadata_bhs);
>  	clear_inode(inode);

[Severity: Medium]
Can ext2_evict_inode() invalidate dirty metadata buffers on i_metadata_bhs
without writing them out if mmb_mark_buffer_dirty() races with mmb_sync()?

In __ext2_truncate_blocks(), ext2_free_branches() modifies
partial->bh->b_data and calls mark_inode_dirty(inode) before calling
mmb_mark_buffer_dirty().  If a concurrent writeback clears I_DIRTY and
I_METADATA_WRITEBACK in __writeback_single_inode() and enters mmb_sync(),
mmb_sync() temporarily sets bh->b_mmb to NULL in __remove_assoc_queue() and
restores bh->b_mmb = mmb if the buffer was dirtied again:

CPU0                                  CPU1
__ext2_truncate_blocks()
  ext2_free_branches()
    mark_inode_dirty(inode)
                                      __writeback_single_inode()
                                        inode_state_clear(...,
                                          I_METADATA_WRITEBACK)
                                        ext2_sync_inode_metadata()
                                          mmb_sync()
                                            spin_lock(&mmb->lock)
                                            __remove_assoc_queue()
                                              bh->b_mmb = NULL
  mmb_mark_buffer_dirty()
    mark_buffer_dirty(bh)
    if (!bh->b_mmb) // true
                                            if (buffer_dirty(bh)) {
                                              list_add(..., &mmb->list)
                                              bh->b_mmb = mmb
                                            }
                                            spin_unlock(&mmb->lock)
      spin_lock(&mmb->lock)
      if (cmpxchg(&bh->b_mmb,
                  NULL, mmb) != NULL) {
        spin_unlock(&mmb->lock)
        return;  <-- skips setting I_METADATA_WRITEBACK
      }

Because mmb_sync() only waits on in-flight I/O in its second loop without
submitting new I/O for the re-dirtied buffer, and mmb_mark_buffer_dirty()
returns early without setting I_METADATA_WRITEBACK, the dirty buffer remains
on i_metadata_bhs with I_METADATA_WRITEBACK clear on the inode.

With mmb_sync() removed from ext2_evict_inode(), evict() skips
sync_inode_metadata() and ext2_evict_inode() detaches the dirty buffer via
mmb_invalidate().  If the inode is later re-read from disk and fsync()ed
before background block device writeback flushes the buffer, fsync() will no
longer see the buffer on i_metadata_bhs and can return before the indirect
block is persisted to disk.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-work-inode-metadata_writeback-v3-0-91ac6c024018@kernel.org?part=10

Re: [PATCH RFC v3 10/14] ext2: drop mmb_sync() from ext2_evict_inode()

[Jan Kara]

On Mon 09-11-26 17:51:25, Christian Brauner wrote:
> evict() writes out what I_METADATA_WRITEBACK tracks before it calls
> ->evict_inode() and since mmb_mark_buffer_dirty() sets the bit that
> includes the tracked metadata buffers. So there's nothing left for
> ext2_evict_inode() to sync.
> 
> Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext2/inode.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> index 9261b666d043..4c38a3a8ea4d 100644
> --- a/fs/ext2/inode.c
> +++ b/fs/ext2/inode.c
> @@ -91,8 +91,6 @@ void ext2_evict_inode(struct inode * inode)
>  		if (inode->i_blocks)
>  			ext2_truncate_blocks(inode, 0);
>  		ext2_xattr_delete_inode(inode);
> -	} else {
> -		mmb_sync(&EXT2_I(inode)->i_metadata_bhs);
>  	}
>  	mmb_invalidate(&EXT2_I(inode)->i_metadata_bhs);
>  	clear_inode(inode);
> 
> -- 
> 2.53.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH RFC v3 11/14] ext4: drop mmb_sync() from ext4_evict_inode()

[Christian Brauner]

evict() writes out what I_METADATA_WRITEBACK tracks before it calls
->evict_inode() and since mmb_mark_buffer_dirty() sets the bit that
includes the tracked metadata buffers. So there's nothing left for
ext4_evict_inode() to sync without a journal, and with a journal there
are no tracked buffers.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/ext4/inode.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 18362580097f..b6c8c11525a8 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -250,8 +250,6 @@ void ext4_evict_inode(struct inode *inode)
 	if (EXT4_I(inode)->i_flags & EXT4_EA_INODE_FL)
 		ext4_evict_ea_inode(inode);
 	if (inode->i_nlink) {
-		struct mapping_metadata_bhs *mmb;
-
 		/*
 		 * If there's dirty page will lead to data loss, user
 		 * could see stale data.
@@ -261,9 +259,6 @@ 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);
 		goto no_delete;
 	}
 

-- 
2.53.0

Re: [PATCH RFC v3 11/14] ext4: drop mmb_sync() from ext4_evict_inode()

[sashiko-bot]

> evict() writes out what I_METADATA_WRITEBACK tracks before it calls
> ->evict_inode() and since mmb_mark_buffer_dirty() sets the bit that
> includes the tracked metadata buffers. So there's nothing left for
> ext4_evict_inode() to sync without a journal, and with a journal there
> are no tracked buffers.
> 
> 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/20261109-work-inode-metadata_writeback-v3-0-91ac6c024018@kernel.org?part=11

Re: [PATCH RFC v3 11/14] ext4: drop mmb_sync() from ext4_evict_inode()

[Jan Kara]

On Mon 09-11-26 17:51:26, Christian Brauner wrote:
> evict() writes out what I_METADATA_WRITEBACK tracks before it calls
> ->evict_inode() and since mmb_mark_buffer_dirty() sets the bit that
> includes the tracked metadata buffers. So there's nothing left for
> ext4_evict_inode() to sync without a journal, and with a journal there
> are no tracked buffers.
> 
> Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>

Looks good! Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/inode.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 18362580097f..b6c8c11525a8 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -250,8 +250,6 @@ void ext4_evict_inode(struct inode *inode)
>  	if (EXT4_I(inode)->i_flags & EXT4_EA_INODE_FL)
>  		ext4_evict_ea_inode(inode);
>  	if (inode->i_nlink) {
> -		struct mapping_metadata_bhs *mmb;
> -
>  		/*
>  		 * If there's dirty page will lead to data loss, user
>  		 * could see stale data.
> @@ -261,9 +259,6 @@ 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);
>  		goto no_delete;
>  	}
>  
> 
> -- 
> 2.53.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH RFC v3 12/14] minix: drop mmb_sync() from minix_evict_inode()

[Christian Brauner]

evict() writes out what I_METADATA_WRITEBACK tracks before it calls
->evict_inode() and since mmb_mark_buffer_dirty() sets the bit that
includes the tracked metadata buffers. So there's nothing left for
minix_evict_inode() to sync.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/minix/inode.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/minix/inode.c b/fs/minix/inode.c
index 756bef4dc832..9fb938ec5868 100644
--- a/fs/minix/inode.c
+++ b/fs/minix/inode.c
@@ -50,8 +50,6 @@ static void minix_evict_inode(struct inode *inode)
 	if (!inode->i_nlink) {
 		inode->i_size = 0;
 		minix_truncate(inode);
-	} else {
-		mmb_sync(&minix_i(inode)->i_metadata_bhs);
 	}
 	mmb_invalidate(&minix_i(inode)->i_metadata_bhs);
 	clear_inode(inode);

-- 
2.53.0

Re: [PATCH RFC v3 12/14] minix: drop mmb_sync() from minix_evict_inode()

[sashiko-bot]

> evict() writes out what I_METADATA_WRITEBACK tracks before it calls
> ->evict_inode() and since mmb_mark_buffer_dirty() sets the bit that
> includes the tracked metadata buffers. So there's nothing left for
> minix_evict_inode() to sync.
> 
> 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/20261109-work-inode-metadata_writeback-v3-0-91ac6c024018@kernel.org?part=12

Re: [PATCH RFC v3 12/14] minix: drop mmb_sync() from minix_evict_inode()

[Jan Kara]

On Mon 09-11-26 17:51:27, Christian Brauner wrote:
> evict() writes out what I_METADATA_WRITEBACK tracks before it calls
> ->evict_inode() and since mmb_mark_buffer_dirty() sets the bit that
> includes the tracked metadata buffers. So there's nothing left for
> minix_evict_inode() to sync.
> 
> Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>

Looks good! Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/minix/inode.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/minix/inode.c b/fs/minix/inode.c
> index 756bef4dc832..9fb938ec5868 100644
> --- a/fs/minix/inode.c
> +++ b/fs/minix/inode.c
> @@ -50,8 +50,6 @@ static void minix_evict_inode(struct inode *inode)
>  	if (!inode->i_nlink) {
>  		inode->i_size = 0;
>  		minix_truncate(inode);
> -	} else {
> -		mmb_sync(&minix_i(inode)->i_metadata_bhs);
>  	}
>  	mmb_invalidate(&minix_i(inode)->i_metadata_bhs);
>  	clear_inode(inode);
> 
> -- 
> 2.53.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH RFC v3 13/14] udf: drop mmb_sync() from udf_evict_inode()

[Christian Brauner]

evict() writes out what I_METADATA_WRITEBACK tracks before it calls
->evict_inode() and since mmb_mark_buffer_dirty() sets the bit that
includes the tracked metadata buffers. So there's nothing left for
udf_evict_inode() to sync for an inode that isn't deleted.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/udf/inode.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index b3e7c07cb9e7..a6c0b9d70ffb 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -152,8 +152,6 @@ void udf_evict_inode(struct inode *inode)
 		}
 	}
 	truncate_inode_pages_final(&inode->i_data);
-	if (!want_delete)
-		mmb_sync(&iinfo->i_metadata_bhs);
 	mmb_invalidate(&iinfo->i_metadata_bhs);
 	clear_inode(inode);
 	kfree(iinfo->i_data);

-- 
2.53.0

Re: [PATCH RFC v3 13/14] udf: drop mmb_sync() from udf_evict_inode()

[sashiko-bot]

> evict() writes out what I_METADATA_WRITEBACK tracks before it calls
> ->evict_inode() and since mmb_mark_buffer_dirty() sets the bit that
> includes the tracked metadata buffers. So there's nothing left for
> udf_evict_inode() to sync for an inode that isn't deleted.
> 
> 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/20261109-work-inode-metadata_writeback-v3-0-91ac6c024018@kernel.org?part=13

Re: [PATCH RFC v3 13/14] udf: drop mmb_sync() from udf_evict_inode()

[Jan Kara]

On Mon 09-11-26 17:51:28, Christian Brauner wrote:
> evict() writes out what I_METADATA_WRITEBACK tracks before it calls
> ->evict_inode() and since mmb_mark_buffer_dirty() sets the bit that
> includes the tracked metadata buffers. So there's nothing left for
> udf_evict_inode() to sync for an inode that isn't deleted.
> 
> Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>

Looks good! Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/udf/inode.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/udf/inode.c b/fs/udf/inode.c
> index b3e7c07cb9e7..a6c0b9d70ffb 100644
> --- a/fs/udf/inode.c
> +++ b/fs/udf/inode.c
> @@ -152,8 +152,6 @@ void udf_evict_inode(struct inode *inode)
>  		}
>  	}
>  	truncate_inode_pages_final(&inode->i_data);
> -	if (!want_delete)
> -		mmb_sync(&iinfo->i_metadata_bhs);
>  	mmb_invalidate(&iinfo->i_metadata_bhs);
>  	clear_inode(inode);
>  	kfree(iinfo->i_data);
> 
> -- 
> 2.53.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH RFC v3 14/14] fat: drop mmb_sync() from fat_evict_inode()

[Christian Brauner]

evict() writes out what I_METADATA_WRITEBACK tracks before it calls
->evict_inode() and since mmb_mark_buffer_dirty() sets the bit that
includes the tracked metadata buffers. So there's nothing left for
fat_evict_inode() to sync. The write-out happens before
fat_free_eofblocks() updates the directory entry again, so that update
stays as asynchronous as it is now.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/fat/inode.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index d85c8fbdcca4..243b18949164 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -693,7 +693,6 @@ 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);
 		fat_free_eofblocks(inode);
 	}
 

-- 
2.53.0

Re: [PATCH RFC v3 14/14] fat: drop mmb_sync() from fat_evict_inode()

[sashiko-bot]

> evict() writes out what I_METADATA_WRITEBACK tracks before it calls
> ->evict_inode() and since mmb_mark_buffer_dirty() sets the bit that
> includes the tracked metadata buffers. So there's nothing left for
> fat_evict_inode() to sync. The write-out happens before
> fat_free_eofblocks() updates the directory entry again, so that update
> stays as asynchronous as it is now.
> 
> 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/20261109-work-inode-metadata_writeback-v3-0-91ac6c024018@kernel.org?part=14

Re: [PATCH RFC v3 14/14] fat: drop mmb_sync() from fat_evict_inode()

[Jan Kara]

On Mon 09-11-26 17:51:29, Christian Brauner wrote:
> evict() writes out what I_METADATA_WRITEBACK tracks before it calls
> ->evict_inode() and since mmb_mark_buffer_dirty() sets the bit that
> includes the tracked metadata buffers. So there's nothing left for
> fat_evict_inode() to sync. The write-out happens before
> fat_free_eofblocks() updates the directory entry again, so that update
> stays as asynchronous as it is now.
> 
> Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>

Looks good! Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/fat/inode.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fs/fat/inode.c b/fs/fat/inode.c
> index d85c8fbdcca4..243b18949164 100644
> --- a/fs/fat/inode.c
> +++ b/fs/fat/inode.c
> @@ -693,7 +693,6 @@ 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);
>  		fat_free_eofblocks(inode);
>  	}
>  
> 
> -- 
> 2.53.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR