Re: [PATCH 2/5] xfs: factor data block addition from xfs_dir2_node_addname_int()

[Dave Chinner <david@fromorbit.com>]

On Thu, Aug 29, 2019 at 02:02:25PM -0700, Darrick J. Wong wrote:
> On Thu, Aug 29, 2019 at 08:47:07PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Factor out the code that adds a data block to a directory from
> > xfs_dir2_node_addname_int(). This makes the code flow cleaner and
> > more obvious and provides clear isolation of upcoming optimsations.
> > 
> > Signed-off-By: Dave Chinner <dchinner@redhat.com>
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  fs/xfs/libxfs/xfs_dir2_node.c | 324 +++++++++++++++++-----------------
> >  1 file changed, 158 insertions(+), 166 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_dir2_node.c b/fs/xfs/libxfs/xfs_dir2_node.c
> > index e40986cc0759..cc1f1c505a2b 100644
> > --- a/fs/xfs/libxfs/xfs_dir2_node.c
> > +++ b/fs/xfs/libxfs/xfs_dir2_node.c
> > @@ -1608,6 +1608,129 @@ xfs_dir2_leafn_unbalance(
> >  	xfs_dir3_leaf_check(dp, drop_blk->bp);
> >  }
> >  
> > +/*
> > + * Add a new data block to the directory at the free space index that the caller
> > + * has specified.
> > + */
> > +static int
> > +xfs_dir2_node_add_datablk(
> > +	struct xfs_da_args	*args,
> > +	struct xfs_da_state_blk	*fblk,
> > +	xfs_dir2_db_t		*dbno,
> > +	struct xfs_buf		**dbpp,
> > +	struct xfs_buf		**fbpp,
> > +	int			*findex)
> > +{
> > +	struct xfs_inode	*dp = args->dp;
> > +	struct xfs_trans	*tp = args->trans;
> > +	struct xfs_mount	*mp = dp->i_mount;
> > +	struct xfs_dir3_icfree_hdr freehdr;
> > +	struct xfs_dir2_data_free *bf;
> > +	struct xfs_dir2_data_hdr *hdr;
> > +	struct xfs_dir2_free	*free = NULL;
> > +	xfs_dir2_db_t		fbno;
> > +	struct xfs_buf		*fbp;
> > +	struct xfs_buf		*dbp;
> > +	__be16			*bests = NULL;
> > +	int			error;
> > +
> > +	/* Not allowed to allocate, return failure. */
> > +	if ((args->op_flags & XFS_DA_OP_JUSTCHECK) || args->total == 0)
> > +		return -ENOSPC;
> > +
> > +	/* Allocate and initialize the new data block.  */
> > +	error = xfs_dir2_grow_inode(args, XFS_DIR2_DATA_SPACE, dbno);
> > +	if (error)
> > +		return error;
> > +	error = xfs_dir3_data_init(args, *dbno, &dbp);
> > +	if (error)
> > +		return error;
> > +
> > +	/*
> > +	 * Get the freespace block corresponding to the data block
> > +	 * that was just allocated.
> > +	 */
> > +	fbno = dp->d_ops->db_to_fdb(args->geo, *dbno);
> > +	error = xfs_dir2_free_try_read(tp, dp,
> > +			       xfs_dir2_db_to_da(args->geo, fbno), &fbp);
> > +	if (error)
> > +		return error;
> > +
> > +	/*
> > +	 * If there wasn't a freespace block, the read will
> > +	 * return a NULL fbp.  Allocate and initialize a new one.
> > +	 */
> > +	if (!fbp) {
> > +		error = xfs_dir2_grow_inode(args, XFS_DIR2_FREE_SPACE, &fbno);
> > +		if (error)
> > +			return error;
> > +
> > +		if (dp->d_ops->db_to_fdb(args->geo, *dbno) != fbno) {
> 
> As a straight "cut this huge function into smaller pieces, no functional
> changes" patch I guess this is fine, but ... when can this happen?

No idea. We're growing the free space segment, and it's checking
that we allocated a new block over the free block index for the
data segment block we just allocated. It's a sanity check more than
anything, I think, because if we have a hole in the free block index
for a give data block bad things are going to happen later...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com