[6/8] xfs: hoist unlinked list search and mapping to a separate function
diff mbox series

Message ID 154897670777.26065.9017810401812180528.stgit@magnolia
State New
Headers show
Series
  • xfs: incore unlinked list
Related show

Commit Message

Darrick J. Wong Jan. 31, 2019, 11:18 p.m. UTC
From: Darrick J. Wong <darrick.wong@oracle.com>

There's a loop that searches an unlinked bucket list to find the inode
that points to a given inode.  Hoist this into a separate function;
later we'll use our iunlink backref cache to bypass the slow list
operation.  No functional changes.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_inode.c |  136 ++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 99 insertions(+), 37 deletions(-)

Comments

Brian Foster Feb. 1, 2019, 7:01 p.m. UTC | #1
On Thu, Jan 31, 2019 at 03:18:27PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> There's a loop that searches an unlinked bucket list to find the inode
> that points to a given inode.  Hoist this into a separate function;
> later we'll use our iunlink backref cache to bypass the slow list
> operation.  No functional changes.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_inode.c |  136 ++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 99 insertions(+), 37 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index ea38b66fbc59..d5b3f8fdac7e 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -2084,6 +2084,100 @@ xfs_iunlink(
>  	return error;
>  }
>  
> +/* Return the imap, dinode pointer, and buffer for an inode. */
> +STATIC int
> +xfs_iunlink_map_ino(
> +	struct xfs_trans	*tp,
> +	xfs_ino_t		next_ino,

Might be good to clean up some of these variable names as this code gets
factored out. E.g., "next_ino" doesn't mean much more than "ino" in the
context of this helper.

> +	struct xfs_imap		*imap,
> +	struct xfs_dinode	**dipp,
> +	struct xfs_buf		**bpp)
> +{
> +	struct xfs_mount	*mp = tp->t_mountp;
> +	int			error;
> +
> +	imap->im_blkno = 0;
> +	error = xfs_imap(mp, tp, next_ino, imap, 0);
> +	if (error) {
> +		xfs_warn(mp, "%s: xfs_imap returned error %d.",
> +				__func__, error);
> +		return error;
> +	}
> +
> +	error = xfs_imap_to_bp(mp, tp, imap, dipp, bpp, 0, 0);
> +	if (error) {
> +		xfs_warn(mp, "%s: xfs_imap_to_bp returned error %d.",
> +				__func__, error);
> +		return error;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Walk the unlinked chain from @head_agino until we find the inode that
> + * points to @target_ino.  Return the inode number, map, dinode pointer,
> + * and inode cluster buffer of that inode as @ino, @imap, @dipp, and @bpp.
> + *
> + * Do not call this function if @target_agino is the head of the list.
> + */
> +STATIC int
> +xfs_iunlink_map_prev(
> +	struct xfs_trans	*tp,
> +	struct xfs_perag	*pag,
> +	xfs_agino_t		head_agino,
> +	xfs_agino_t		target_agino,
> +	xfs_ino_t		*ino,
> +	struct xfs_imap		*imap,
> +	struct xfs_dinode	**dipp,
> +	struct xfs_buf		**bpp)
> +{

It would also be nice to have some oneline comments next to the params
of some of these functions.

> +	struct xfs_imap		last_imap;
> +	struct xfs_mount	*mp = tp->t_mountp;
> +	struct xfs_buf		*last_ibp = NULL;
> +	struct xfs_dinode	*last_dip;
> +	xfs_ino_t		next_ino = NULLFSINO;
> +	xfs_agino_t		next_agino;
> +	int			error;
> +
> +	ASSERT(head_agino != target_agino);
> +
> +	next_agino = head_agino;
> +	while (next_agino != target_agino) {
> +		xfs_agino_t	unlinked_agino;
> +
> +		if (last_ibp)
> +			xfs_trans_brelse(tp, last_ibp);
> +
> +		next_ino = XFS_AGINO_TO_INO(mp, pag->pag_agno, next_agino);
> +		error = xfs_iunlink_map_ino(tp, next_ino, &last_imap,
> +				&last_dip, &last_ibp);
> +		if (error)
> +			return error;
> +
> +		unlinked_agino = be32_to_cpu(last_dip->di_next_unlinked);
> +		if (!xfs_verify_agino(mp, pag->pag_agno, next_agino) ||

Should the next_agino above be unlinked_agino? Also is the == check
below a loop check (comment pls)?

> +		    next_agino == unlinked_agino) {
> +			XFS_CORRUPTION_ERROR(__func__,
> +					XFS_ERRLEVEL_LOW, mp,
> +					last_dip, sizeof(*last_dip));
> +			error = -EFSCORRUPTED;
> +			return error;
> +		}
> +		next_agino = unlinked_agino;
> +	}
> +
> +	/* Should never happen, but don't return garbage. */
> +	if (next_ino == NULLFSINO)
> +		return -EFSCORRUPTED;
> +
> +	*ino = next_ino;
> +	memcpy(imap, &last_imap, sizeof(last_imap));
> +	*dipp = last_dip;
> +	*bpp = last_ibp;
> +	return 0;
> +}
> +
>  /*
>   * Pull the on-disk inode from the AGI unlinked list.
>   */
> @@ -2153,43 +2247,11 @@ xfs_iunlink_remove(
>  	} else {
>  		struct xfs_imap	imap;
>  
> -		/*
> -		 * We need to search the list for the inode being freed.
> -		 */
> -		last_ibp = NULL;
> -		while (next_agino != agino) {
> -			if (last_ibp)
> -				xfs_trans_brelse(tp, last_ibp);
> -
> -			imap.im_blkno = 0;
> -			next_ino = XFS_AGINO_TO_INO(mp, agno, next_agino);
> -
> -			error = xfs_imap(mp, tp, next_ino, &imap, 0);
> -			if (error) {
> -				xfs_warn(mp,
> -	"%s: xfs_imap returned error %d.",
> -					 __func__, error);
> -				goto out_unlock;
> -			}
> -
> -			error = xfs_imap_to_bp(mp, tp, &imap, &last_dip,
> -					       &last_ibp, 0, 0);
> -			if (error) {
> -				xfs_warn(mp,
> -	"%s: xfs_imap_to_bp returned error %d.",
> -					__func__, error);
> -				goto out_unlock;
> -			}
> -
> -			next_agino = be32_to_cpu(last_dip->di_next_unlinked);
> -			if (!xfs_verify_agino(mp, agno, next_agino)) {
> -				XFS_CORRUPTION_ERROR(__func__,
> -						XFS_ERRLEVEL_LOW, mp,
> -						last_dip, sizeof(*last_dip));
> -				error = -EFSCORRUPTED;
> -				goto out_unlock;
> -			}
> -		}
> +		/* We need to search the list for the inode being freed. */
> +		error = xfs_iunlink_map_prev(tp, pag, next_agino, agino,
> +				&next_ino, &imap, &last_dip, &last_ibp);

Another variable nit... next_ino is kind of confusing here, particularly
where we still use next_agino as well. The former is actually the "prev"
ino output, right?

Brian

> +		if (error)
> +			goto out_unlock;
>  
>  		/*
>  		 * Now last_ibp points to the buffer previous to us on the
>
Christoph Hellwig Feb. 2, 2019, 4:30 p.m. UTC | #2
Nitpick: why hoist here and refactor in the previous patches subject?

> +	xfs_agino_t		target_agino,
> +	xfs_ino_t		*ino,
> +	struct xfs_imap		*imap,
> +	struct xfs_dinode	**dipp,
> +	struct xfs_buf		**bpp)
> +{
> +	struct xfs_imap		last_imap;
> +	struct xfs_mount	*mp = tp->t_mountp;
> +	struct xfs_buf		*last_ibp = NULL;
> +	struct xfs_dinode	*last_dip;
> +	xfs_ino_t		next_ino = NULLFSINO;
> +	xfs_agino_t		next_agino;
> +	int			error;
> +
> +	ASSERT(head_agino != target_agino);
> +
> +	next_agino = head_agino;
> +	while (next_agino != target_agino) {
> +		xfs_agino_t	unlinked_agino;
> +
> +		if (last_ibp)
> +			xfs_trans_brelse(tp, last_ibp);
> +
> +		next_ino = XFS_AGINO_TO_INO(mp, pag->pag_agno, next_agino);
> +		error = xfs_iunlink_map_ino(tp, next_ino, &last_imap,
> +				&last_dip, &last_ibp);

And reason we stop formatting directly into the passed in imap,
as the old code did?
Christoph Hellwig Feb. 2, 2019, 4:51 p.m. UTC | #3
> +/* Return the imap, dinode pointer, and buffer for an inode. */
> +STATIC int
> +xfs_iunlink_map_ino(
> +	struct xfs_trans	*tp,
> +	xfs_ino_t		next_ino,
> +	struct xfs_imap		*imap,
> +	struct xfs_dinode	**dipp,
> +	struct xfs_buf		**bpp)
> +{
> +	struct xfs_mount	*mp = tp->t_mountp;
> +	int			error;
> +
> +	imap->im_blkno = 0;
> +	error = xfs_imap(mp, tp, next_ino, imap, 0);

Looking at the next patch it actually would be nicer if we'd pass
agno/agino instead of the inode number to this function,
as currently both callers need to duplicate the caculation of the
ino.

In fact the first thing xfs_imap does is to split the ino into agno
and agino again, so maybe we should throw in a prep patch to change
the xfs_imap calling convention as well.
Darrick J. Wong Feb. 2, 2019, 8:42 p.m. UTC | #4
On Sat, Feb 02, 2019 at 08:30:07AM -0800, Christoph Hellwig wrote:
> Nitpick: why hoist here and refactor in the previous patches subject?

Will change.

> > +	xfs_agino_t		target_agino,
> > +	xfs_ino_t		*ino,
> > +	struct xfs_imap		*imap,
> > +	struct xfs_dinode	**dipp,
> > +	struct xfs_buf		**bpp)
> > +{
> > +	struct xfs_imap		last_imap;
> > +	struct xfs_mount	*mp = tp->t_mountp;
> > +	struct xfs_buf		*last_ibp = NULL;
> > +	struct xfs_dinode	*last_dip;
> > +	xfs_ino_t		next_ino = NULLFSINO;
> > +	xfs_agino_t		next_agino;
> > +	int			error;
> > +
> > +	ASSERT(head_agino != target_agino);
> > +
> > +	next_agino = head_agino;
> > +	while (next_agino != target_agino) {
> > +		xfs_agino_t	unlinked_agino;
> > +
> > +		if (last_ibp)
> > +			xfs_trans_brelse(tp, last_ibp);
> > +
> > +		next_ino = XFS_AGINO_TO_INO(mp, pag->pag_agno, next_agino);
> > +		error = xfs_iunlink_map_ino(tp, next_ino, &last_imap,
> > +				&last_dip, &last_ibp);
> 
> And reason we stop formatting directly into the passed in imap,
> as the old code did?

General prinicple of not formatting into the caller's variables until
we're sure we're returning 0 but I'll change it to format directly since
it'll save stack space.

--D
Darrick J. Wong Feb. 2, 2019, 8:46 p.m. UTC | #5
On Fri, Feb 01, 2019 at 02:01:38PM -0500, Brian Foster wrote:
> On Thu, Jan 31, 2019 at 03:18:27PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > There's a loop that searches an unlinked bucket list to find the inode
> > that points to a given inode.  Hoist this into a separate function;
> > later we'll use our iunlink backref cache to bypass the slow list
> > operation.  No functional changes.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/xfs_inode.c |  136 ++++++++++++++++++++++++++++++++++++++--------------
> >  1 file changed, 99 insertions(+), 37 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index ea38b66fbc59..d5b3f8fdac7e 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -2084,6 +2084,100 @@ xfs_iunlink(
> >  	return error;
> >  }
> >  
> > +/* Return the imap, dinode pointer, and buffer for an inode. */
> > +STATIC int
> > +xfs_iunlink_map_ino(
> > +	struct xfs_trans	*tp,
> > +	xfs_ino_t		next_ino,
> 
> Might be good to clean up some of these variable names as this code gets
> factored out. E.g., "next_ino" doesn't mean much more than "ino" in the
> context of this helper.

Ok.

> > +	struct xfs_imap		*imap,
> > +	struct xfs_dinode	**dipp,
> > +	struct xfs_buf		**bpp)
> > +{
> > +	struct xfs_mount	*mp = tp->t_mountp;
> > +	int			error;
> > +
> > +	imap->im_blkno = 0;
> > +	error = xfs_imap(mp, tp, next_ino, imap, 0);
> > +	if (error) {
> > +		xfs_warn(mp, "%s: xfs_imap returned error %d.",
> > +				__func__, error);
> > +		return error;
> > +	}
> > +
> > +	error = xfs_imap_to_bp(mp, tp, imap, dipp, bpp, 0, 0);
> > +	if (error) {
> > +		xfs_warn(mp, "%s: xfs_imap_to_bp returned error %d.",
> > +				__func__, error);
> > +		return error;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Walk the unlinked chain from @head_agino until we find the inode that
> > + * points to @target_ino.  Return the inode number, map, dinode pointer,
> > + * and inode cluster buffer of that inode as @ino, @imap, @dipp, and @bpp.
> > + *
> > + * Do not call this function if @target_agino is the head of the list.
> > + */
> > +STATIC int
> > +xfs_iunlink_map_prev(
> > +	struct xfs_trans	*tp,
> > +	struct xfs_perag	*pag,
> > +	xfs_agino_t		head_agino,
> > +	xfs_agino_t		target_agino,
> > +	xfs_ino_t		*ino,
> > +	struct xfs_imap		*imap,
> > +	struct xfs_dinode	**dipp,
> > +	struct xfs_buf		**bpp)
> > +{
> 
> It would also be nice to have some oneline comments next to the params
> of some of these functions.

In addition to the description given above the function?

> > +	struct xfs_imap		last_imap;
> > +	struct xfs_mount	*mp = tp->t_mountp;
> > +	struct xfs_buf		*last_ibp = NULL;
> > +	struct xfs_dinode	*last_dip;
> > +	xfs_ino_t		next_ino = NULLFSINO;
> > +	xfs_agino_t		next_agino;
> > +	int			error;
> > +
> > +	ASSERT(head_agino != target_agino);
> > +
> > +	next_agino = head_agino;
> > +	while (next_agino != target_agino) {
> > +		xfs_agino_t	unlinked_agino;
> > +
> > +		if (last_ibp)
> > +			xfs_trans_brelse(tp, last_ibp);
> > +
> > +		next_ino = XFS_AGINO_TO_INO(mp, pag->pag_agno, next_agino);
> > +		error = xfs_iunlink_map_ino(tp, next_ino, &last_imap,
> > +				&last_dip, &last_ibp);
> > +		if (error)
> > +			return error;
> > +
> > +		unlinked_agino = be32_to_cpu(last_dip->di_next_unlinked);
> > +		if (!xfs_verify_agino(mp, pag->pag_agno, next_agino) ||
> 
> Should the next_agino above be unlinked_agino? Also is the == check
> below a loop check (comment pls)?

Yes, fixed.

/*
 * Make sure this pointer is valid and isn't an obvious
 * infinite loop.
 */


> > +		    next_agino == unlinked_agino) {
> > +			XFS_CORRUPTION_ERROR(__func__,
> > +					XFS_ERRLEVEL_LOW, mp,
> > +					last_dip, sizeof(*last_dip));
> > +			error = -EFSCORRUPTED;
> > +			return error;
> > +		}
> > +		next_agino = unlinked_agino;
> > +	}
> > +
> > +	/* Should never happen, but don't return garbage. */
> > +	if (next_ino == NULLFSINO)
> > +		return -EFSCORRUPTED;
> > +
> > +	*ino = next_ino;
> > +	memcpy(imap, &last_imap, sizeof(last_imap));
> > +	*dipp = last_dip;
> > +	*bpp = last_ibp;
> > +	return 0;
> > +}
> > +
> >  /*
> >   * Pull the on-disk inode from the AGI unlinked list.
> >   */
> > @@ -2153,43 +2247,11 @@ xfs_iunlink_remove(
> >  	} else {
> >  		struct xfs_imap	imap;
> >  
> > -		/*
> > -		 * We need to search the list for the inode being freed.
> > -		 */
> > -		last_ibp = NULL;
> > -		while (next_agino != agino) {
> > -			if (last_ibp)
> > -				xfs_trans_brelse(tp, last_ibp);
> > -
> > -			imap.im_blkno = 0;
> > -			next_ino = XFS_AGINO_TO_INO(mp, agno, next_agino);
> > -
> > -			error = xfs_imap(mp, tp, next_ino, &imap, 0);
> > -			if (error) {
> > -				xfs_warn(mp,
> > -	"%s: xfs_imap returned error %d.",
> > -					 __func__, error);
> > -				goto out_unlock;
> > -			}
> > -
> > -			error = xfs_imap_to_bp(mp, tp, &imap, &last_dip,
> > -					       &last_ibp, 0, 0);
> > -			if (error) {
> > -				xfs_warn(mp,
> > -	"%s: xfs_imap_to_bp returned error %d.",
> > -					__func__, error);
> > -				goto out_unlock;
> > -			}
> > -
> > -			next_agino = be32_to_cpu(last_dip->di_next_unlinked);
> > -			if (!xfs_verify_agino(mp, agno, next_agino)) {
> > -				XFS_CORRUPTION_ERROR(__func__,
> > -						XFS_ERRLEVEL_LOW, mp,
> > -						last_dip, sizeof(*last_dip));
> > -				error = -EFSCORRUPTED;
> > -				goto out_unlock;
> > -			}
> > -		}
> > +		/* We need to search the list for the inode being freed. */
> > +		error = xfs_iunlink_map_prev(tp, pag, next_agino, agino,
> > +				&next_ino, &imap, &last_dip, &last_ibp);
> 
> Another variable nit... next_ino is kind of confusing here, particularly
> where we still use next_agino as well. The former is actually the "prev"
> ino output, right?

Yep, will fix.

--D

> Brian
> 
> > +		if (error)
> > +			goto out_unlock;
> >  
> >  		/*
> >  		 * Now last_ibp points to the buffer previous to us on the
> >
Brian Foster Feb. 4, 2019, 1:18 p.m. UTC | #6
On Sat, Feb 02, 2019 at 12:46:00PM -0800, Darrick J. Wong wrote:
> On Fri, Feb 01, 2019 at 02:01:38PM -0500, Brian Foster wrote:
> > On Thu, Jan 31, 2019 at 03:18:27PM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > There's a loop that searches an unlinked bucket list to find the inode
> > > that points to a given inode.  Hoist this into a separate function;
> > > later we'll use our iunlink backref cache to bypass the slow list
> > > operation.  No functional changes.
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > >  fs/xfs/xfs_inode.c |  136 ++++++++++++++++++++++++++++++++++++++--------------
> > >  1 file changed, 99 insertions(+), 37 deletions(-)
> > > 
> > > 
> > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > > index ea38b66fbc59..d5b3f8fdac7e 100644
> > > --- a/fs/xfs/xfs_inode.c
> > > +++ b/fs/xfs/xfs_inode.c
> > > @@ -2084,6 +2084,100 @@ xfs_iunlink(
> > >  	return error;
> > >  }
> > >  
> > > +/* Return the imap, dinode pointer, and buffer for an inode. */
> > > +STATIC int
> > > +xfs_iunlink_map_ino(
> > > +	struct xfs_trans	*tp,
> > > +	xfs_ino_t		next_ino,
> > 
> > Might be good to clean up some of these variable names as this code gets
> > factored out. E.g., "next_ino" doesn't mean much more than "ino" in the
> > context of this helper.
> 
> Ok.
> 
> > > +	struct xfs_imap		*imap,
> > > +	struct xfs_dinode	**dipp,
> > > +	struct xfs_buf		**bpp)
> > > +{
> > > +	struct xfs_mount	*mp = tp->t_mountp;
> > > +	int			error;
> > > +
> > > +	imap->im_blkno = 0;
> > > +	error = xfs_imap(mp, tp, next_ino, imap, 0);
> > > +	if (error) {
> > > +		xfs_warn(mp, "%s: xfs_imap returned error %d.",
> > > +				__func__, error);
> > > +		return error;
> > > +	}
> > > +
> > > +	error = xfs_imap_to_bp(mp, tp, imap, dipp, bpp, 0, 0);
> > > +	if (error) {
> > > +		xfs_warn(mp, "%s: xfs_imap_to_bp returned error %d.",
> > > +				__func__, error);
> > > +		return error;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/*
> > > + * Walk the unlinked chain from @head_agino until we find the inode that
> > > + * points to @target_ino.  Return the inode number, map, dinode pointer,
> > > + * and inode cluster buffer of that inode as @ino, @imap, @dipp, and @bpp.
> > > + *
> > > + * Do not call this function if @target_agino is the head of the list.
> > > + */
> > > +STATIC int
> > > +xfs_iunlink_map_prev(
> > > +	struct xfs_trans	*tp,
> > > +	struct xfs_perag	*pag,
> > > +	xfs_agino_t		head_agino,
> > > +	xfs_agino_t		target_agino,
> > > +	xfs_ino_t		*ino,
> > > +	struct xfs_imap		*imap,
> > > +	struct xfs_dinode	**dipp,
> > > +	struct xfs_buf		**bpp)
> > > +{
> > 
> > It would also be nice to have some oneline comments next to the params
> > of some of these functions.
> 
> In addition to the description given above the function?
> 

I missed the header comment.. perhaps just a one liner that indicates
ino and everything after are output params would be helpful here. BTW,
s/@target_ino/@target_agino/ in the header comment?

Brian

> > > +	struct xfs_imap		last_imap;
> > > +	struct xfs_mount	*mp = tp->t_mountp;
> > > +	struct xfs_buf		*last_ibp = NULL;
> > > +	struct xfs_dinode	*last_dip;
> > > +	xfs_ino_t		next_ino = NULLFSINO;
> > > +	xfs_agino_t		next_agino;
> > > +	int			error;
> > > +
> > > +	ASSERT(head_agino != target_agino);
> > > +
> > > +	next_agino = head_agino;
> > > +	while (next_agino != target_agino) {
> > > +		xfs_agino_t	unlinked_agino;
> > > +
> > > +		if (last_ibp)
> > > +			xfs_trans_brelse(tp, last_ibp);
> > > +
> > > +		next_ino = XFS_AGINO_TO_INO(mp, pag->pag_agno, next_agino);
> > > +		error = xfs_iunlink_map_ino(tp, next_ino, &last_imap,
> > > +				&last_dip, &last_ibp);
> > > +		if (error)
> > > +			return error;
> > > +
> > > +		unlinked_agino = be32_to_cpu(last_dip->di_next_unlinked);
> > > +		if (!xfs_verify_agino(mp, pag->pag_agno, next_agino) ||
> > 
> > Should the next_agino above be unlinked_agino? Also is the == check
> > below a loop check (comment pls)?
> 
> Yes, fixed.
> 
> /*
>  * Make sure this pointer is valid and isn't an obvious
>  * infinite loop.
>  */
> 
> 
> > > +		    next_agino == unlinked_agino) {
> > > +			XFS_CORRUPTION_ERROR(__func__,
> > > +					XFS_ERRLEVEL_LOW, mp,
> > > +					last_dip, sizeof(*last_dip));
> > > +			error = -EFSCORRUPTED;
> > > +			return error;
> > > +		}
> > > +		next_agino = unlinked_agino;
> > > +	}
> > > +
> > > +	/* Should never happen, but don't return garbage. */
> > > +	if (next_ino == NULLFSINO)
> > > +		return -EFSCORRUPTED;
> > > +
> > > +	*ino = next_ino;
> > > +	memcpy(imap, &last_imap, sizeof(last_imap));
> > > +	*dipp = last_dip;
> > > +	*bpp = last_ibp;
> > > +	return 0;
> > > +}
> > > +
> > >  /*
> > >   * Pull the on-disk inode from the AGI unlinked list.
> > >   */
> > > @@ -2153,43 +2247,11 @@ xfs_iunlink_remove(
> > >  	} else {
> > >  		struct xfs_imap	imap;
> > >  
> > > -		/*
> > > -		 * We need to search the list for the inode being freed.
> > > -		 */
> > > -		last_ibp = NULL;
> > > -		while (next_agino != agino) {
> > > -			if (last_ibp)
> > > -				xfs_trans_brelse(tp, last_ibp);
> > > -
> > > -			imap.im_blkno = 0;
> > > -			next_ino = XFS_AGINO_TO_INO(mp, agno, next_agino);
> > > -
> > > -			error = xfs_imap(mp, tp, next_ino, &imap, 0);
> > > -			if (error) {
> > > -				xfs_warn(mp,
> > > -	"%s: xfs_imap returned error %d.",
> > > -					 __func__, error);
> > > -				goto out_unlock;
> > > -			}
> > > -
> > > -			error = xfs_imap_to_bp(mp, tp, &imap, &last_dip,
> > > -					       &last_ibp, 0, 0);
> > > -			if (error) {
> > > -				xfs_warn(mp,
> > > -	"%s: xfs_imap_to_bp returned error %d.",
> > > -					__func__, error);
> > > -				goto out_unlock;
> > > -			}
> > > -
> > > -			next_agino = be32_to_cpu(last_dip->di_next_unlinked);
> > > -			if (!xfs_verify_agino(mp, agno, next_agino)) {
> > > -				XFS_CORRUPTION_ERROR(__func__,
> > > -						XFS_ERRLEVEL_LOW, mp,
> > > -						last_dip, sizeof(*last_dip));
> > > -				error = -EFSCORRUPTED;
> > > -				goto out_unlock;
> > > -			}
> > > -		}
> > > +		/* We need to search the list for the inode being freed. */
> > > +		error = xfs_iunlink_map_prev(tp, pag, next_agino, agino,
> > > +				&next_ino, &imap, &last_dip, &last_ibp);
> > 
> > Another variable nit... next_ino is kind of confusing here, particularly
> > where we still use next_agino as well. The former is actually the "prev"
> > ino output, right?
> 
> Yep, will fix.
> 
> --D
> 
> > Brian
> > 
> > > +		if (error)
> > > +			goto out_unlock;
> > >  
> > >  		/*
> > >  		 * Now last_ibp points to the buffer previous to us on the
> > >
Darrick J. Wong Feb. 4, 2019, 4:31 p.m. UTC | #7
On Mon, Feb 04, 2019 at 08:18:45AM -0500, Brian Foster wrote:
> On Sat, Feb 02, 2019 at 12:46:00PM -0800, Darrick J. Wong wrote:
> > On Fri, Feb 01, 2019 at 02:01:38PM -0500, Brian Foster wrote:
> > > On Thu, Jan 31, 2019 at 03:18:27PM -0800, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > 
> > > > There's a loop that searches an unlinked bucket list to find the inode
> > > > that points to a given inode.  Hoist this into a separate function;
> > > > later we'll use our iunlink backref cache to bypass the slow list
> > > > operation.  No functional changes.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > ---
> > > >  fs/xfs/xfs_inode.c |  136 ++++++++++++++++++++++++++++++++++++++--------------
> > > >  1 file changed, 99 insertions(+), 37 deletions(-)
> > > > 
> > > > 
> > > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > > > index ea38b66fbc59..d5b3f8fdac7e 100644
> > > > --- a/fs/xfs/xfs_inode.c
> > > > +++ b/fs/xfs/xfs_inode.c
> > > > @@ -2084,6 +2084,100 @@ xfs_iunlink(
> > > >  	return error;
> > > >  }
> > > >  
> > > > +/* Return the imap, dinode pointer, and buffer for an inode. */
> > > > +STATIC int
> > > > +xfs_iunlink_map_ino(
> > > > +	struct xfs_trans	*tp,
> > > > +	xfs_ino_t		next_ino,
> > > 
> > > Might be good to clean up some of these variable names as this code gets
> > > factored out. E.g., "next_ino" doesn't mean much more than "ino" in the
> > > context of this helper.
> > 
> > Ok.
> > 
> > > > +	struct xfs_imap		*imap,
> > > > +	struct xfs_dinode	**dipp,
> > > > +	struct xfs_buf		**bpp)
> > > > +{
> > > > +	struct xfs_mount	*mp = tp->t_mountp;
> > > > +	int			error;
> > > > +
> > > > +	imap->im_blkno = 0;
> > > > +	error = xfs_imap(mp, tp, next_ino, imap, 0);
> > > > +	if (error) {
> > > > +		xfs_warn(mp, "%s: xfs_imap returned error %d.",
> > > > +				__func__, error);
> > > > +		return error;
> > > > +	}
> > > > +
> > > > +	error = xfs_imap_to_bp(mp, tp, imap, dipp, bpp, 0, 0);
> > > > +	if (error) {
> > > > +		xfs_warn(mp, "%s: xfs_imap_to_bp returned error %d.",
> > > > +				__func__, error);
> > > > +		return error;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Walk the unlinked chain from @head_agino until we find the inode that
> > > > + * points to @target_ino.  Return the inode number, map, dinode pointer,
> > > > + * and inode cluster buffer of that inode as @ino, @imap, @dipp, and @bpp.
> > > > + *
> > > > + * Do not call this function if @target_agino is the head of the list.
> > > > + */
> > > > +STATIC int
> > > > +xfs_iunlink_map_prev(
> > > > +	struct xfs_trans	*tp,
> > > > +	struct xfs_perag	*pag,
> > > > +	xfs_agino_t		head_agino,
> > > > +	xfs_agino_t		target_agino,
> > > > +	xfs_ino_t		*ino,
> > > > +	struct xfs_imap		*imap,
> > > > +	struct xfs_dinode	**dipp,
> > > > +	struct xfs_buf		**bpp)
> > > > +{
> > > 
> > > It would also be nice to have some oneline comments next to the params
> > > of some of these functions.
> > 
> > In addition to the description given above the function?
> > 
> 
> I missed the header comment.. perhaps just a one liner that indicates
> ino and everything after are output params would be helpful here. BTW,
> s/@target_ino/@target_agino/ in the header comment?

Ok, will fix.  Thanks for the review!

--D

> Brian
> 
> > > > +	struct xfs_imap		last_imap;
> > > > +	struct xfs_mount	*mp = tp->t_mountp;
> > > > +	struct xfs_buf		*last_ibp = NULL;
> > > > +	struct xfs_dinode	*last_dip;
> > > > +	xfs_ino_t		next_ino = NULLFSINO;
> > > > +	xfs_agino_t		next_agino;
> > > > +	int			error;
> > > > +
> > > > +	ASSERT(head_agino != target_agino);
> > > > +
> > > > +	next_agino = head_agino;
> > > > +	while (next_agino != target_agino) {
> > > > +		xfs_agino_t	unlinked_agino;
> > > > +
> > > > +		if (last_ibp)
> > > > +			xfs_trans_brelse(tp, last_ibp);
> > > > +
> > > > +		next_ino = XFS_AGINO_TO_INO(mp, pag->pag_agno, next_agino);
> > > > +		error = xfs_iunlink_map_ino(tp, next_ino, &last_imap,
> > > > +				&last_dip, &last_ibp);
> > > > +		if (error)
> > > > +			return error;
> > > > +
> > > > +		unlinked_agino = be32_to_cpu(last_dip->di_next_unlinked);
> > > > +		if (!xfs_verify_agino(mp, pag->pag_agno, next_agino) ||
> > > 
> > > Should the next_agino above be unlinked_agino? Also is the == check
> > > below a loop check (comment pls)?
> > 
> > Yes, fixed.
> > 
> > /*
> >  * Make sure this pointer is valid and isn't an obvious
> >  * infinite loop.
> >  */
> > 
> > 
> > > > +		    next_agino == unlinked_agino) {
> > > > +			XFS_CORRUPTION_ERROR(__func__,
> > > > +					XFS_ERRLEVEL_LOW, mp,
> > > > +					last_dip, sizeof(*last_dip));
> > > > +			error = -EFSCORRUPTED;
> > > > +			return error;
> > > > +		}
> > > > +		next_agino = unlinked_agino;
> > > > +	}
> > > > +
> > > > +	/* Should never happen, but don't return garbage. */
> > > > +	if (next_ino == NULLFSINO)
> > > > +		return -EFSCORRUPTED;
> > > > +
> > > > +	*ino = next_ino;
> > > > +	memcpy(imap, &last_imap, sizeof(last_imap));
> > > > +	*dipp = last_dip;
> > > > +	*bpp = last_ibp;
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  /*
> > > >   * Pull the on-disk inode from the AGI unlinked list.
> > > >   */
> > > > @@ -2153,43 +2247,11 @@ xfs_iunlink_remove(
> > > >  	} else {
> > > >  		struct xfs_imap	imap;
> > > >  
> > > > -		/*
> > > > -		 * We need to search the list for the inode being freed.
> > > > -		 */
> > > > -		last_ibp = NULL;
> > > > -		while (next_agino != agino) {
> > > > -			if (last_ibp)
> > > > -				xfs_trans_brelse(tp, last_ibp);
> > > > -
> > > > -			imap.im_blkno = 0;
> > > > -			next_ino = XFS_AGINO_TO_INO(mp, agno, next_agino);
> > > > -
> > > > -			error = xfs_imap(mp, tp, next_ino, &imap, 0);
> > > > -			if (error) {
> > > > -				xfs_warn(mp,
> > > > -	"%s: xfs_imap returned error %d.",
> > > > -					 __func__, error);
> > > > -				goto out_unlock;
> > > > -			}
> > > > -
> > > > -			error = xfs_imap_to_bp(mp, tp, &imap, &last_dip,
> > > > -					       &last_ibp, 0, 0);
> > > > -			if (error) {
> > > > -				xfs_warn(mp,
> > > > -	"%s: xfs_imap_to_bp returned error %d.",
> > > > -					__func__, error);
> > > > -				goto out_unlock;
> > > > -			}
> > > > -
> > > > -			next_agino = be32_to_cpu(last_dip->di_next_unlinked);
> > > > -			if (!xfs_verify_agino(mp, agno, next_agino)) {
> > > > -				XFS_CORRUPTION_ERROR(__func__,
> > > > -						XFS_ERRLEVEL_LOW, mp,
> > > > -						last_dip, sizeof(*last_dip));
> > > > -				error = -EFSCORRUPTED;
> > > > -				goto out_unlock;
> > > > -			}
> > > > -		}
> > > > +		/* We need to search the list for the inode being freed. */
> > > > +		error = xfs_iunlink_map_prev(tp, pag, next_agino, agino,
> > > > +				&next_ino, &imap, &last_dip, &last_ibp);
> > > 
> > > Another variable nit... next_ino is kind of confusing here, particularly
> > > where we still use next_agino as well. The former is actually the "prev"
> > > ino output, right?
> > 
> > Yep, will fix.
> > 
> > --D
> > 
> > > Brian
> > > 
> > > > +		if (error)
> > > > +			goto out_unlock;
> > > >  
> > > >  		/*
> > > >  		 * Now last_ibp points to the buffer previous to us on the
> > > >

Patch
diff mbox series

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index ea38b66fbc59..d5b3f8fdac7e 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -2084,6 +2084,100 @@  xfs_iunlink(
 	return error;
 }
 
+/* Return the imap, dinode pointer, and buffer for an inode. */
+STATIC int
+xfs_iunlink_map_ino(
+	struct xfs_trans	*tp,
+	xfs_ino_t		next_ino,
+	struct xfs_imap		*imap,
+	struct xfs_dinode	**dipp,
+	struct xfs_buf		**bpp)
+{
+	struct xfs_mount	*mp = tp->t_mountp;
+	int			error;
+
+	imap->im_blkno = 0;
+	error = xfs_imap(mp, tp, next_ino, imap, 0);
+	if (error) {
+		xfs_warn(mp, "%s: xfs_imap returned error %d.",
+				__func__, error);
+		return error;
+	}
+
+	error = xfs_imap_to_bp(mp, tp, imap, dipp, bpp, 0, 0);
+	if (error) {
+		xfs_warn(mp, "%s: xfs_imap_to_bp returned error %d.",
+				__func__, error);
+		return error;
+	}
+
+	return 0;
+}
+
+/*
+ * Walk the unlinked chain from @head_agino until we find the inode that
+ * points to @target_ino.  Return the inode number, map, dinode pointer,
+ * and inode cluster buffer of that inode as @ino, @imap, @dipp, and @bpp.
+ *
+ * Do not call this function if @target_agino is the head of the list.
+ */
+STATIC int
+xfs_iunlink_map_prev(
+	struct xfs_trans	*tp,
+	struct xfs_perag	*pag,
+	xfs_agino_t		head_agino,
+	xfs_agino_t		target_agino,
+	xfs_ino_t		*ino,
+	struct xfs_imap		*imap,
+	struct xfs_dinode	**dipp,
+	struct xfs_buf		**bpp)
+{
+	struct xfs_imap		last_imap;
+	struct xfs_mount	*mp = tp->t_mountp;
+	struct xfs_buf		*last_ibp = NULL;
+	struct xfs_dinode	*last_dip;
+	xfs_ino_t		next_ino = NULLFSINO;
+	xfs_agino_t		next_agino;
+	int			error;
+
+	ASSERT(head_agino != target_agino);
+
+	next_agino = head_agino;
+	while (next_agino != target_agino) {
+		xfs_agino_t	unlinked_agino;
+
+		if (last_ibp)
+			xfs_trans_brelse(tp, last_ibp);
+
+		next_ino = XFS_AGINO_TO_INO(mp, pag->pag_agno, next_agino);
+		error = xfs_iunlink_map_ino(tp, next_ino, &last_imap,
+				&last_dip, &last_ibp);
+		if (error)
+			return error;
+
+		unlinked_agino = be32_to_cpu(last_dip->di_next_unlinked);
+		if (!xfs_verify_agino(mp, pag->pag_agno, next_agino) ||
+		    next_agino == unlinked_agino) {
+			XFS_CORRUPTION_ERROR(__func__,
+					XFS_ERRLEVEL_LOW, mp,
+					last_dip, sizeof(*last_dip));
+			error = -EFSCORRUPTED;
+			return error;
+		}
+		next_agino = unlinked_agino;
+	}
+
+	/* Should never happen, but don't return garbage. */
+	if (next_ino == NULLFSINO)
+		return -EFSCORRUPTED;
+
+	*ino = next_ino;
+	memcpy(imap, &last_imap, sizeof(last_imap));
+	*dipp = last_dip;
+	*bpp = last_ibp;
+	return 0;
+}
+
 /*
  * Pull the on-disk inode from the AGI unlinked list.
  */
@@ -2153,43 +2247,11 @@  xfs_iunlink_remove(
 	} else {
 		struct xfs_imap	imap;
 
-		/*
-		 * We need to search the list for the inode being freed.
-		 */
-		last_ibp = NULL;
-		while (next_agino != agino) {
-			if (last_ibp)
-				xfs_trans_brelse(tp, last_ibp);
-
-			imap.im_blkno = 0;
-			next_ino = XFS_AGINO_TO_INO(mp, agno, next_agino);
-
-			error = xfs_imap(mp, tp, next_ino, &imap, 0);
-			if (error) {
-				xfs_warn(mp,
-	"%s: xfs_imap returned error %d.",
-					 __func__, error);
-				goto out_unlock;
-			}
-
-			error = xfs_imap_to_bp(mp, tp, &imap, &last_dip,
-					       &last_ibp, 0, 0);
-			if (error) {
-				xfs_warn(mp,
-	"%s: xfs_imap_to_bp returned error %d.",
-					__func__, error);
-				goto out_unlock;
-			}
-
-			next_agino = be32_to_cpu(last_dip->di_next_unlinked);
-			if (!xfs_verify_agino(mp, agno, next_agino)) {
-				XFS_CORRUPTION_ERROR(__func__,
-						XFS_ERRLEVEL_LOW, mp,
-						last_dip, sizeof(*last_dip));
-				error = -EFSCORRUPTED;
-				goto out_unlock;
-			}
-		}
+		/* We need to search the list for the inode being freed. */
+		error = xfs_iunlink_map_prev(tp, pag, next_agino, agino,
+				&next_ino, &imap, &last_dip, &last_ibp);
+		if (error)
+			goto out_unlock;
 
 		/*
 		 * Now last_ibp points to the buffer previous to us on the