Re: [PATCH 00/12] xfs: flush related error handling cleanups

[Dave Chinner <david@fromorbit.com>]

On Mon, Apr 20, 2020 at 10:06:04AM -0400, Brian Foster wrote:
> On Mon, Apr 20, 2020 at 08:53:06AM +1000, Dave Chinner wrote:
> > On Fri, Apr 17, 2020 at 11:08:47AM -0400, Brian Foster wrote:
> > > Hi all,
> > > 
> > > This actually started as what I intended to be a cleanup of xfsaild
> > > error handling and the fact that unexpected errors are kind of lost in
> > > the ->iop_push() handlers of flushable log items. Some discussion with
> > > Dave on that is available here[1]. I was thinking of genericizing the
> > > behavior, but I'm not so sure that is possible now given the error
> > > handling requirements of the associated items.
> > > 
> > > While thinking through that, I ended up incorporating various cleanups
> > > in the somewhat confusing and erratic error handling on the periphery of
> > > xfsaild, such as the flush handlers. Most of these are straightforward
> > > cleanups except for patch 9, which I think requires careful review and
> > > is of debatable value. I have used patch 12 to run an hour or so of
> > > highly concurrent fsstress load against it and will execute a longer run
> > > over the weekend now that fstests has completed.
> > > 
> > > Thoughts, reviews, flames appreciated.
> > 
> > I'll need to do something thinking on this patchset - I have a
> > patchset that touches a lot of the same code I'm working on right
> > now to pin inode cluster buffers in memory when the inode is dirtied
> > so we don't get RMW cycles in AIL flushing.
> > 
> > That code gets rid of xfs_iflush() completely, removes dirty inodes
> > from the AIL and tracks only ordered cluster buffers in the AIL for
> > inode writeback (i.e. reduces AIL tracked log items by up to 30x).
> > It also only does inode writeback from the ordered cluster buffers.
> > 
> 
> Ok. I could see that being reason enough to drop the iflush iodone
> patch, given that it depends on a bit of a rework/hack. A cleaner
> solution requires more thought and it might not be worth the time if the
> code is going away. Most of the rest are straightforward cleanups though
> so I wouldn't expect complex conflict resolution. It's hard to say
> for sure without seeing the code, of course..

Yeah, now I've been though most of it there isn't a huge impact on
my patchset. Mainly just the conflicts in the mods to xfs_iflush and
friends.

> > The idea behind this is to make inode flushing completely
> > non-blocking, and to simply inode cluster flushing to simply iterate
> > all the dirty inodes attached to the buffer. This gets rid of radix
> > tree lookups and races with reclaim, and gets rid of having to
> > special case a locked inode in the cluster iteration code.
> > 
> 
> Sounds interesting, but it's not really clear to me what the general
> flushing dynamic looks like in this model. I.e., you mention
> xfs_iflush() goes away, but cluster flushing still exists in some form,
> so I can't really tell if xfs_iflush() going away is tied to a
> functional change or primarily a refactoring/cleanup. Anyways, no need
> to go into the weeds if the code will eventually clarify..

It's primarily a clean-up to try to reduce AIL pushing overhead as
I'm regularly seeing the xfsaild CPU bound trying to push inodes
that are already on their way to disk. So I'm trying to reduce
cluster flushing to be driven by a buffer item push rather than by
pushing repeatedly on every inode item that is attached to the
buffer. 

> > Do you have a git tree I could pull this from to see how bad the
> > conflicts are?
> > 
> 
> I don't have a public tree. I suppose I could look into getting
> kernel.org access if somebody could point me in the right
> direction for that. :) In the meantime I could make a private tree
> accessible to you directly if that's helpful..

Send a request for an account and git tree to helpdesk@kernel.org
and cc Darrick, Eric and myself so we can ACK the request.

Details here:

https://korg.wiki.kernel.org/userdoc/accounts

and all the userdoc is here:

https://korg.wiki.kernel.org/start

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com