Re: [PATCH 04/30] xfs: mark inode buffers in cache

[Dave Chinner <david@fromorbit.com>]

On Tue, Jun 02, 2020 at 12:45:35PM -0400, Brian Foster wrote:
> On Tue, Jun 02, 2020 at 07:42:25AM +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.
> > 
> 
> Ok, I was initially thinking this patch looked incomplete in that we
> continue to set ->b_iodone() on inode buffers even though we'd never
> call it. Looking ahead, I see that the next few patches continue to
> clean that up to eventually remove ->b_iodone(), so that addresses that.
> 
> My only other curiosity is that while there may not be any functional
> difference, this technically changes callback behavior in that we set
> the new flag in some contexts that don't currently attach anything to
> the buffer, right? E.g., xfs_trans_inode_alloc_buf() sets the flag on
> inode chunk init, which means we can write out an inode buffer without
> any attached/flushed inodes.

Yes, it can happen, and it happens before this patch, too, because
the AIL can push the buffer log item directly and that does not
flush dirty inodes to the buffer before it writes back(*).

As it is, xfs_buf_inode_iodone() on a buffer with no inode attached
if functionally identical to the existing xfs_buf_iodone() callback
that would otherwise be done. i.e. it just runs the buffer log item
completion callback. Hence the change here rearranges code, but it
does not change behaviour at all.

(*) this is a double-write bug that this patch set does not address.
i.e. buffer log item flushes the buffer without flushing inodes, IO
compeletes, then inode flush to the buffer and we do another IO to
clean them.  This is addressed by a follow-on patchset that tracks
dirty inodes via ordered cluster buffers, such that pushing the
buffer always triggers xfs_iflush_cluster() on buffers tagged
_XBF_INODES...

> Is the intent of that to support future
> changes? If so, a note about that in the commit log would be helpful.

That's part of it, as you can see from the (*) above. But the commit
log already says "..., and makes future modifications much simpler."
Was that insufficient to indicate that it will be used later on?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com