Re: [PATCH 3/4] xfs: remove most in-flight buffer accounting

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Feb 19, 2025 at 06:35:27AM +0100, Christoph Hellwig wrote:
> On Tue, Feb 18, 2025 at 12:23:27PM -0800, Darrick J. Wong wrote:
> > On Mon, Feb 17, 2025 at 10:31:28AM +0100, Christoph Hellwig wrote:
> > > The buffer cache keeps a bt_io_count per-CPU counter to track all
> > > in-flight I/O, which is used to ensure no I/O is in flight when
> > > unmounting the file system.
> > > 
> > > For most I/O we already keep track of inflight I/O at higher levels:
> > > 
> > >  - for synchronous I/O (xfs_buf_read/xfs_bwrite/xfs_buf_delwri_submit),
> > >    the caller has a reference and waits for I/O completions using
> > >    xfs_buf_iowait
> > >  - for xfs_buf_delwri_submit the only caller (AIL writeback) tracks the
> > 
> > Do you mean xfs_buf_delwri_submit_nowait here?
> 
> Yes.
> 
> > IOWs, only asynchronous readahead needs an explicit counter in the
> > xfs_buf to prevent unmount because:
> > 
> > 0. Anything done in mount/unmount/freeze holds s_umount
> > 1. Buffer reads done on behalf of a file hold the file open and pin the
> >    mount
> > 2. Dirty buffers have log items, and we can't unmount until those are
> >    dealt with
> > 3. Fsck holds an open fd and hence pins the mount
> > 4. Unmount blocks until background gc finishes
> > 
> > Right?
> 
> Yes.
> 
> > I almost wonder if you could just have a percpu counter in the
> > xfs_mount but that sounds pretty hot.
> 
> Well, that would remove the nice xfs_buftarg_wait() abstraction.
> Givne that we don't even allocate an extra buftrag unless we use
> it that doesn't seem very useful.

Heheh.  Anyway, the changes look good to me so with that cleared up
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

> > > +	/* there are currently no valid flags for xfs_buf_get_uncached */
> > > +	ASSERT(flags == 0);
> > 
> > Can we just get rid of flags then?  AFAICT nobody uses it either here or
> > in xfsprogs, and in fact I think there's a nasty bug in the userspace
> > rtsb code:
> 
> See my reply to the last patch: I actually have a patch to remove it,
> but it conflicts with the zoned series.  So for now I'll defer it until
> that is merged.

<nod>

--D