Re: [PATCH v9 1/3] block: introduce bdev_flush_by_dev()

[Julian Sun <sunjunchao@bytedance.com>]

On 10/6/26 12:25 AM, Jan Kara wrote:
> On Mon 05-10-26 01:26:28, Christoph Hellwig wrote:
>> On Fri, Oct 02, 2026 at 01:10:42PM +0200, Jan Kara wrote:
>>> FWIW I share Julian's concern here. It would be fine to use AS_KERNEL_FILE
>>> for ext4 metadata but I think just unconditionally setting AS_KERNEL_FILE
>>> for bdev mappings will cause issues because some users may be using bdevs
>>> directly for their workloads and they could still expect proper memcg
>>> accounting to work in that case.
>>
>> Agreed that it should not set unconditionally.
>>
>>> We could set AS_KERNEL_FILE when opening bdev for a filesystem (and remove
>>> it when releasing bdev) but that would have to make sure there are no
>>> folios in the bdev mapping when changing the flag as otherwise the
>>> accounting would go wrong. Looks it might be doable but getting all the
>>> cornercases right will be hairy and overall not very appealing to me...
>>
>> I'd rather not support special case writeback code just for this legacy
>> fs abuses bdev buffer cache case.  And we basically need to tear
>> down pagecache at unmount anyway, as i_blkbits can change, so while
>> we do need to be careful, I don't think it really is a major issue.
> 
> OK, after some more thought yes, I think we can make that work.
> 
>> We might be able to restrict to setting it when
>> CONFIG_BLK_DEV_WRITE_MOUNTED is disabled to avoid the problem of non-fs
>> shared mmap writers, as anyone using a modern kernel and cgroups really
>> should have that disabled.
> 
> I don't think that's really needed. If such writes are happening, they are
> *very* limited and done by a system administrator only (as they are very
> dangerous) so the fact the writes will get charged to the root cgroup is
> not an issue.

Hi, Jan, Christoph.

Thanks for your suggestions.

One remaining concern: while ext4 is mounted, buffered reads of the raw
block device would also charge page-cache allocations to the root memcg,
bypassing the reader's memcg memory limits. Is that acceptable?

I've attached a preliminary diff. Does this approach look reasonable?
If so, I'll split this into a formal patch series.

diff --git a/block/bdev.c b/block/bdev.c
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -79,7 +79,7 @@ static void bdev_write_inode(struct bloc
 }

 /* Kill _all_ buffers and pagecache , dirty or not.. */
-static void kill_bdev(struct block_device *bdev)
+void kill_bdev(struct block_device *bdev)
 {
 	struct address_space *mapping = bdev->bd_mapping;

@@ -89,6 +89,7 @@ static void kill_bdev(struct block_devic
 	invalidate_bh_lrus();
 	truncate_inode_pages(mapping, 0);
 }
+EXPORT_SYMBOL_GPL(kill_bdev);

 /* Invalidate clean unused buffers and pagecache. */
 void invalidate_bdev(struct block_device *bdev)
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1706,6 +1706,7 @@ bool disk_live(struct gendisk *disk);
 unsigned int block_size(struct block_device *bdev);

 #ifdef CONFIG_BLOCK
+void kill_bdev(struct block_device *bdev);
 void invalidate_bdev(struct block_device *bdev);
 int sync_blockdev(struct block_device *bdev);
 int sync_blockdev_range(struct block_device *bdev, loff_t lstart, loff_t lend);
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1279,6 +1279,56 @@ static void ext4_flex_groups_free(struct
 	}
 }

+/*
+ * Account ext4 metadata cache to the root cgroup. This filesystem-wide
+ * metadata is shared across users, so it is not naturally attributable
+ * to a single user cgroup.
+ *
+ * A bdev inode can contain metadata pages charged to many memory cgroups.
+ * Dirtying these pages can trigger foreign writeback of its entire owner wb,
+ * including unrelated file data. Premature writeback of buffered overwrites
+ * reduces write coalescing, increasing disk traffic and consuming bandwidth
+ * that could serve foreground direct I/O. This can hurt performance,
+ * particularly on bandwidth-limited devices such as HDDs.
+ *
+ * This does not depend on CONFIG_BLK_DEV_WRITE_MOUNTED. Writing directly to
+ * a mounted block device is dangerous and intended for system administration.
+ * Charging the cache from these writes to the root cgroup is acceptable.
+ */
+static void ext4_bdev_set_kernel_file(struct block_device *bdev, bool enable)
+{
+	struct address_space *mapping = bdev->bd_mapping;
+	struct inode *inode = mapping->host;
+
+	inode_lock(inode);
+	filemap_invalidate_lock(mapping);
+
+	if (!!test_bit(AS_KERNEL_FILE, &mapping->flags) == enable)
+		goto out;
+
+	/*
+	 * Folio removal uses the mapping's current accounting mode. Remove old
+	 * folios before changing it, excluding new raw-I/O cache allocations.
+	 */
+	sync_blockdev(bdev);
+	kill_bdev(bdev);
+
+	if (enable)
+		set_bit(AS_KERNEL_FILE, &mapping->flags);
+	else
+		clear_bit(AS_KERNEL_FILE, &mapping->flags);
+out:
+	filemap_invalidate_unlock(mapping);
+	inode_unlock(inode);
+}
+
 static void ext4_put_super(struct super_block *sb)
 {
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
@@ -1387,6 +1437,7 @@ static void ext4_put_super(struct super_
 #if IS_ENABLED(CONFIG_UNICODE)
 	utf8_unload(sb->s_encoding);
 #endif
+	ext4_bdev_set_kernel_file(sb->s_bdev, false);
 	kfree(sbi);
 }

@@ -5852,6 +5903,8 @@ static int ext4_fill_super(struct super_
 	if (ctx->spec & EXT4_SPEC_s_sb_block)
 		sbi->s_sb_block = ctx->s_sb_block;

+	ext4_bdev_set_kernel_file(sb->s_bdev, true);
+
 	ret = __ext4_fill_super(fc, sb);
 	if (ret < 0)
 		goto free_sbi;
@@ -5877,6 +5930,7 @@ static int ext4_fill_super(struct super_
 	return 0;

 free_sbi:
+	ext4_bdev_set_kernel_file(sb->s_bdev, false);
 	ext4_free_sbi(sbi);
 	fc->s_fs_info = NULL;
 	return ret;


> 
> 								Honza

Thanks,
-- 
Julian Sun <sunjunchao@bytedance.com>