Re: [PATCH v3 03/17] xfs: simplify inode flush error handling

[Brian Foster <bfoster@redhat.com>]

On Fri, May 01, 2020 at 02:17:30AM -0700, Christoph Hellwig wrote:
> On Thu, Apr 30, 2020 at 11:37:03AM -0700, Darrick J. Wong wrote:
> > TBH I've long wondered why we flush one inode and only then check that
> > the icluster buffer is pinned and if so force the log?  Did we do that
> > for some sort of forward progress guarantee?
> > 
> > I looked at a3f74ffb6d144 (aka when the log force moved here from after
> > the iflush_cluster call) but that didn't help me figure out if there's
> > some subtlety here I'm missing, or if the ordering here was weird but
> > the weirdness didn't matter?
> > 
> > TLDR: I can't tell why it's ok to move the xfs_iflush_int call down past
> > the log force. :/
> 
> As far as I can tell the log force is to avoid waiting for the buffer
> to be unpinned.  This is mostly bad when using xfs_bwrite, which we
> still do for the xfs_reclaim_inode case, given that xfs_inode_item_push
> alredy checks for the pinned inode earlier, and lets the xfsaild handle
> the log pushing.
> 

Right, that was my impression of the force as well. Note that it's async
and we already own the buffer lock at this point, so I don't see why
ordering would matter that much functionally. My impression of the
earlier patch is this landed before the cluster flush because that's
heavier weight and simply provides the bulk of the optimization.

> Which means doing the log_force earlier is actually a (practially not
> relevant) micro-optimization as it gives the log code a few more
> instructions worth of time to push out and complete the log buffer.
> 
> Maybe this wants to be split out into a prep patch to better document
> the change.
> 
> 

The intent was more just to clean up the function than to provide any
sort of additional optimization. It seems cleaner to check pinned status
as soon as we grab the buffer vs. somewhere between inode flushes. I
don't think this warrants a separate patch unless we took it further to
somehow avoid the log force in the non-reclaim case. My understanding is
that Dave's upcoming work would eliminate the need for this force in the
reclaim case, so I'm not sure there's value in swizzling it around in
the meantime.

Brian