Re: [PATCH 04/13] xfs: arrange all unlinked inodes into one list

From: Dave Chinner <david@fromorbit.com>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 04/13] xfs: arrange all unlinked inodes into one list
Date: Wed, 19 Aug 2020 10:45:31 +1000	[thread overview]
Message-ID: <20200819004531.GF21744@dread.disaster.area> (raw)
In-Reply-To: <20200818235959.GR6096@magnolia>

On Tue, Aug 18, 2020 at 04:59:59PM -0700, Darrick J. Wong wrote:
> On Wed, Aug 12, 2020 at 07:25:47PM +1000, Dave Chinner wrote:
> > From: Gao Xiang <hsiangkao@redhat.com>
> > 
> > We currently keep unlinked lists short on disk by hashing the inodes
> > across multiple buckets. We don't need to ikeep them short anymore
> > as we no longer need to traverse the entire to remove an inode from
> > it. The in-memory back reference index provides the previous inode
> > in the list for us instead.
> > 
> > Log recovery still has to handle existing filesystems that use all
> > 64 on-disk buckets so we detect and handle this case specially so
> > that so inode eviction can still work properly in recovery.
> > 
> > [dchinner: imported into parent patch series early on and modified
> > to fit cleanly. ]
> > 
> > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/xfs_inode.c | 49 +++++++++++++++++++++++++++-------------------
> >  1 file changed, 29 insertions(+), 20 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index f2f502b65691..fa92bdf6e0da 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -33,6 +33,7 @@
> >  #include "xfs_symlink.h"
> >  #include "xfs_trans_priv.h"
> >  #include "xfs_log.h"
> > +#include "xfs_log_priv.h"
> >  #include "xfs_bmap_btree.h"
> >  #include "xfs_reflink.h"
> >  
> > @@ -2092,25 +2093,32 @@ xfs_iunlink_update_bucket(
> >  	struct xfs_trans	*tp,
> >  	xfs_agnumber_t		agno,
> >  	struct xfs_buf		*agibp,
> > -	unsigned int		bucket_index,
> > +	xfs_agino_t		old_agino,
> >  	xfs_agino_t		new_agino)
> >  {
> > +	struct xlog		*log = tp->t_mountp->m_log;
> >  	struct xfs_agi		*agi = agibp->b_addr;
> >  	xfs_agino_t		old_value;
> > -	int			offset;
> > +	unsigned int		bucket_index;
> > +	int                     offset;
> >  
> >  	ASSERT(xfs_verify_agino_or_null(tp->t_mountp, agno, new_agino));
> >  
> > +	bucket_index = 0;
> > +	/* During recovery, the old multiple bucket index can be applied */
> > +	if (!log || log->l_flags & XLOG_RECOVERY_NEEDED) {
> 
> Does the flag test need parentheses?

Yes, will fix.

> It feels a little funny that we pass in old_agino (having gotten it from
> agi_unlinked) and then compare it with agi_unlinked, but as the commit
> log points out, I guess this is a wart of having to support the old
> unlinked list behavior.  It makes sense to me that if we're going to
> change the unlinked list behavior we could be a little more careful
> about double-checking things.
> 
> Question: if a newer kernel crashes with a super-long unlinked list and
> the fs gets recovered on an old kernel, will this lead to insanely high
> recovery times?  I think the answer is no, because recovery is single
> threaded and the hash only existed to reduce AGI contention during
> normal unlinking operations?

Right, the answer is no because log recovery even on old kernels
always recovers the inode at the head of the list. It does no
traversal, so it doesn't matter if it's recovering one list or 64
lists, the recovery time is the same.

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com