Re: [PATCH 3/4] xfs: track unlinked inodes in core inode

[Brian Foster <bfoster@redhat.com>]

On Thu, Jul 02, 2020 at 08:18:39AM +1000, Dave Chinner wrote:
> On Wed, Jul 01, 2020 at 10:31:21AM -0400, Brian Foster wrote:
> > On Tue, Jun 23, 2020 at 07:50:14PM +1000, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > Currently we cache unlinked inode list backrefs through a separate
> > > cache which has to be maintained via memory allocation and a hash
> > > table. When the inode is on the unlinked list, we have an existence
> > > guarantee for the inode in memory.
> > > 
> > > That is, if the inode is on the unlinked list, there must be a
> > > reference to the inode from the core VFS because dropping the last
> > > reference to the inode causes it to be removed from the unlinked
> > > list. Hence if we hold the AGI locked, we guarantee that any inode
> > > on the unlinked list is pinned in memory. That means we can actually
> > > track the entire unlinked list on the inode itself and use
> > > unreferenced inode cache lookups to update the list pointers as
> > > needed.
> > > 
> > > However, we don't use this relationship because log recovery has
> > > no in memory state and so has to work directly from buffers.
> > > However, because unlink recovery only removes from the head of the
> > > list, we can easily fake this in memory state as the inode we read
> > > in from the AGI bucket has a pointer to the next inode. Hence we can
> > > play reference leapfrog in the recovery loop always reading the
> > > second inode on the list and updating pointers before dropping the
> > > reference to the first inode. Hence the in-memory state is always
> > > valid for recovery, too.
> > > 
> > > This means we can tear out the old inode unlinked list cache and
> > > update mechanisms and replace it with a much simpler "insert" and
> > > "remove" functions that use in-memory inode state rather than on
> > > buffer state to track the list. The diffstat speaks for itself.
> > > 
> > > Food for thought: This obliviates the need for the on-disk AGI
> > > unlinked hash - we because we track as a double linked list in
> > > memory, we don't need to keep hash chains on disk short to minimise
> > > previous inode lookup overhead on list removal. Hence we probably
> > > should just convert the unlinked list code to use a single list
> > > on disk...
> > > 
> > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > ---
> > 
> > Looks interesting, but are you planning to break this up into smaller
> > pieces? E.g., perhaps add the new inode pointers and set them in one
> > patch, then replace the whole backref thing with the in-core pointers,
> > then update the insert/remove, then log recovery, etc.
> 
> Likely, yes.
> 
> > I'm sure there's
> > various ways it can or cannot be split, but regardless this patch looks
> > like it could be a series in and of itself.
> 
> This RFC series is largely centered around this single patch, so
> splitting it out into a separate series makes no sense.
> 

I was just speaking generally that this patch looked quite overloaded. I
don't mean to imply it should be separated from the others.

> FWIW, This is basically the same sort of thing that the inode
> flushing patchset started out as - a single patch that I wrote in
> few hours and got working as a whole. It does need to be split up,
> but given that the inode flushing rework took several months to turn
> a few hours of coding into a mergable patchset, I haven't bothered
> to do that for this patch set yet.
> 

Understood.

> I'd kinda like to avoid having this explode into 30 patches as that
> previous patchset did - this is a very self-contained change, so
> there's really only 4-5 pieces it can be split up into. Trying to
> split it more finely than that is going to make it quite hard to
> find clean places to split it...
> 

I'm not expecting 30 patches. :) The quick flow I noted above, perhaps
with the addition of pushing refactoring changes towards the end, lands
right around 4 or 5 and seems like it would improve reviewability quite
a bit. Of course things change that might lead to more or less once you
get into the details/context of breaking things down...

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
>