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

[Julian Sun <sunjunchao@bytedance.com>]

On 9/28/26 2:24 PM, Christoph Hellwig wrote:

Hi, Christoph.

Thanks for your review and suggestions.

> On Fri, Sep 25, 2026 at 12:34:52PM +0200, Jan Kara wrote:
>> On Thu 24-09-26 23:52:33, Christoph Hellwig wrote:
>>> On Fri, Sep 25, 2026 at 02:44:42PM +0800, Julian Sun wrote:
>>>> Add bdev_flush_by_dev() to submit page-cache writeback for a device
>>>> identified by dev_t without requiring callers to hold a persistent
>>>> device reference.
>>>
>>> This isn't really a flush is it?  This is writeback.
>>
>> So bdev_writeback_by_dev()? Fine by me.
> 
> Yes.
> 
>> Well, Julian is explaining that in the cover letter. We don't want memcgs
>> to hold bdev references just for foreign flush tracking. There's no easy
>> way to get rid of them so they could pin bdevs for a long time leading to
>> strange artifacts.
> 
> I have to admit I didn't get it when flying over the cover letter.
> But this also really belongs into the patch where it is more obvious,
> or even better into code comments.
> 
>>
>> As you write, we could just hold a reference to the bdev inode. Those get
>> unhashed when the device dies (__del_gendisk()) so memcgs would be just
>> wasting some memory by holding these inodes alive. But still, transitioning
>> from inode to proper bdev reference verifying bdev is still alive will add
>> a bit of hairy code (essentially what blkdev_get_no_open() does plus
>> verification inode is still hashed). Plus when replacing foreign flush
>> entries you're under irqsafe xa_lock so doing iput() from there is kind of
>> a nogo.
>>
>> So I think tracking devices by dev_t is a good way of dealing with these
>> problems. But then when flushing you have to transition from dev_t to
>> struct block_device and blkdev_get_no_open() is the canonical way of doing
>> that (and note this use is just internal to block/bdev.c).
> 
> I'd really prefer not to grow more blkdev_get_no_open users.
> 
> But the more I look at this I wonder why we even bother.  Trying to
> attribute individual bits of dirty metadata to cgroups is pretty much
> insane.  Why don't we bypass memcg accounting for buffer_heads like we
> always did for the XFS buffer cache, and what btrfs switched to last
> year? See commit b55102826d7d ("btrfs: set AS_KERNEL_FILE on the
> btree_inode") for the btrfs side.

That sounds reasonable. However, IMHO, the main difference is that ext4
stores this metadata in the bdev page cache rather than a separate metadata
cache as btrfs and XFS do. The same bdev->bd_mapping is also used for buffered
reads of the raw block device from userspace. Setting AS_KERNEL_FILE on that
mapping would therefore also charge newly allocated page-cache folios from
those reads to the root memcg rather than the reader's memcg, bypassing its
memory limits.

If we pass a flag from ext4's metadata buffer-cache allocation sites and
check it in grow_buffers() to charge new folios to the root memcg, accounting
them as kernel-file pages would still require persistent per-folio state.
When a folio is removed from the page cache, the allocation context is gone,
so we cannot tell whether it was counted as a kernel-file folio.

Separating ext4's metadata cache from the bdev page cache would address this,
but that seems like a much larger, longer-term change.


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