Re: [PATCH 03/24] xfs: mark inode buffers in cache

Dave Chinner
2020-05-25T00:06:28Z

On Sat, May 23, 2020 at 01:48:27AM -0700, Christoph Hellwig wrote:
> On Fri, May 22, 2020 at 01:50:08PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > Inode buffers always have write IO callbacks, so by marking them
> > directly we can avoid needing to attach ->b_iodone functions to
> > them. This avoids an indirect call, and makes future modifications
> > much simpler.
> >
> > This is largely a rearrangement of the code at this point - no IO
> > completion functionality changes at this point, just how the
> > code is run is modified.
>
> So I've looked over the whole series where this leads, and I really like
> killing off li_cb and sorting out the mess around b_iodone. But I think
> the series ends up moving too much out of xfs_buf.c. I'd rather keep
> a minimal b_write_done callback rather than the checking of the inode
> and dquote flags, and keep most of the logic in xfs_buf.c. This is the
> patch I cooked up on top of your series to show what I mean - it removes
> the need for both flags and kills about 100 lines of code:

Let's deal with further cleanups after this patchset and the
follow-on patchsets that I have are sorted out. All cleanups will do
right now is invalidate all the stuff I'm still working on that sits
on top of this patchset..

> diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> index e5f58a4b13eee..4220fee1c9ddb 100644
> --- a/fs/xfs/libxfs/xfs_trans_inode.c
> +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> @@ -169,7 +169,7 @@ xfs_trans_log_inode(
> xfs_buf_hold(bp);
> spin_lock(&iip->ili_lock);
> iip->ili_item.li_buf = bp;
> - bp->b_flags |= _XBF_INODES;
> + bp->b_write_done = xfs_iflush_done;

I originally tried this, but then I ended up with places in
follow-on patchsets where I had to do this sort of check:

if (bp->b_write_done == xfs_iflush_done)

To detect a buffer that already had inodes attached to it. Which
clearly told me that there should be a state flag to indicate that
the buffer has attached inodes....

Cheers,

Dave.
--
Dave Chinner
david@fromorbit.com