Re: [PATCH 2/3] xfs: clean up xfs_bui_item_recover iget/trans_alloc/ilock ordering

From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 2/3] xfs: clean up xfs_bui_item_recover iget/trans_alloc/ilock ordering
Date: Wed, 16 Sep 2020 23:47:27 -0700	[thread overview]
Message-ID: <20200917064727.GT7955@magnolia> (raw)
In-Reply-To: <20200917051333.GF12131@dread.disaster.area>

On Thu, Sep 17, 2020 at 03:13:33PM +1000, Dave Chinner wrote:
> On Wed, Sep 16, 2020 at 08:29:36PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > In most places in XFS, we have a specific order in which we gather
> > resources: grab the inode, allocate a transaction, then lock the inode.
> > xfs_bui_item_recover doesn't do it in that order, so fix it to be more
> > consistent.  This also makes the error bailout code a bit less weird.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/xfs_bmap_item.c |   40 +++++++++++++++++++++-------------------
> >  1 file changed, 21 insertions(+), 19 deletions(-)
> 
> This probably needs to go before the xfs_qm_dqattach() fix, or
> the dqattach fix need to come after this....

<nod> I'll fix the previous patch.

> > 
> > diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> > index 877afe76d76a..6f589f04f358 100644
> > --- a/fs/xfs/xfs_bmap_item.c
> > +++ b/fs/xfs/xfs_bmap_item.c
> > @@ -475,25 +475,26 @@ xfs_bui_item_recover(
> >  	    (bmap->me_flags & ~XFS_BMAP_EXTENT_FLAGS))
> >  		goto garbage;
> >  
> > -	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate,
> > -			XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK), 0, 0, &tp);
> > -	if (error)
> > -		return error;
> > -
> > -	budp = xfs_trans_get_bud(tp, buip);
> > -
> >  	/* Grab the inode. */
> > -	error = xfs_iget(mp, tp, bmap->me_owner, 0, XFS_ILOCK_EXCL, &ip);
> > +	error = xfs_iget(mp, NULL, bmap->me_owner, 0, 0, &ip);
> >  	if (error)
> > -		goto err_inode;
> > +		return error;
> >  
> >  	error = xfs_qm_dqattach(ip);
> >  	if (error)
> > -		goto err_inode;
> > +		goto err_rele;
> >  
> >  	if (VFS_I(ip)->i_nlink == 0)
> >  		xfs_iflags_set(ip, XFS_IRECOVERY);
> >  
> > +	/* Allocate transaction and do the work. */
> > +	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate,
> > +			XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK), 0, 0, &tp);
> > +	if (error)
> > +		goto err_rele;
> 
> Hmmmm - don't all the error cased before we call xfs_trans_get_bud()
> need to release the bui?

Yes, I think so.  Come to think of it, the other intent items seem like
they have the same bug.

--D

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