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

[Christoph Hellwig <hch@infradead.org>]

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.