Re: [PATCH 3/5] xfs: factor free block index lookup from xfs_dir2_node_addname_int()

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Thu, Aug 29, 2019 at 08:47:08PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Simplify the logic in xfs_dir2_node_addname_int() by factoring out
> the free block index lookup code that finds a block with enough free
> space for the entry to be added. The code that is moved gets a major
> cleanup at the same time, but there is no algorithm change here.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/libxfs/xfs_dir2_node.c | 194 ++++++++++++++++++----------------
>  1 file changed, 102 insertions(+), 92 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2_node.c b/fs/xfs/libxfs/xfs_dir2_node.c
> index cc1f1c505a2b..93254f45a5f9 100644
> --- a/fs/xfs/libxfs/xfs_dir2_node.c
> +++ b/fs/xfs/libxfs/xfs_dir2_node.c
> @@ -1635,7 +1635,7 @@ xfs_dir2_node_add_datablk(
>  	int			error;
>  
>  	/* Not allowed to allocate, return failure. */
> -	if ((args->op_flags & XFS_DA_OP_JUSTCHECK) || args->total == 0)
> +	if (args->total == 0)
>  		return -ENOSPC;
>  
>  	/* Allocate and initialize the new data block.  */
> @@ -1731,43 +1731,29 @@ xfs_dir2_node_add_datablk(
>  	return 0;
>  }
>  
> -/*
> - * Add the data entry for a node-format directory name addition.
> - * The leaf entry is added in xfs_dir2_leafn_add.
> - * We may enter with a freespace block that the lookup found.
> - */
> -static int					/* error */
> -xfs_dir2_node_addname_int(
> -	xfs_da_args_t		*args,		/* operation arguments */
> -	xfs_da_state_blk_t	*fblk)		/* optional freespace block */
> +static int
> +xfs_dir2_node_find_freeblk(
> +	struct xfs_da_args	*args,
> +	struct xfs_da_state_blk	*fblk,
> +	xfs_dir2_db_t		*dbnop,
> +	struct xfs_buf		**fbpp,
> +	int			*findexp,
> +	int			length)
>  {
> -	xfs_dir2_data_hdr_t	*hdr;		/* data block header */
> -	xfs_dir2_db_t		dbno;		/* data block number */
> -	struct xfs_buf		*dbp;		/* data block buffer */
> -	xfs_dir2_data_entry_t	*dep;		/* data entry pointer */
> -	xfs_inode_t		*dp;		/* incore directory inode */
> -	xfs_dir2_data_unused_t	*dup;		/* data unused entry pointer */
> -	int			error;		/* error return value */
> -	xfs_dir2_db_t		fbno;		/* freespace block number */
> -	struct xfs_buf		*fbp;		/* freespace buffer */
> -	int			findex;		/* freespace entry index */
> -	xfs_dir2_free_t		*free=NULL;	/* freespace block structure */
> -	xfs_dir2_db_t		ifbno;		/* initial freespace block no */
> -	xfs_dir2_db_t		lastfbno=0;	/* highest freespace block no */
> -	int			length;		/* length of the new entry */
> -	int			logfree = 0;	/* need to log free entry */
> -	int			needlog = 0;	/* need to log data header */
> -	int			needscan = 0;	/* need to rescan data frees */
> -	__be16			*tagp;		/* data entry tag pointer */
> -	xfs_trans_t		*tp;		/* transaction pointer */
> -	__be16			*bests;
>  	struct xfs_dir3_icfree_hdr freehdr;
> -	struct xfs_dir2_data_free *bf;
> -	xfs_dir2_data_aoff_t	aoff;
> +	struct xfs_dir2_free	*free = NULL;
> +	struct xfs_inode	*dp = args->dp;
> +	struct xfs_trans	*tp = args->trans;
> +	struct xfs_buf		*fbp = NULL;
> +	xfs_dir2_db_t		lastfbno;
> +	xfs_dir2_db_t		ifbno = -1;
> +	xfs_dir2_db_t		dbno = -1;
> +	xfs_dir2_db_t		fbno = -1;
> +	xfs_fileoff_t		fo;
> +	__be16			*bests;
> +	int			findex;
> +	int			error;
>  
> -	dp = args->dp;
> -	tp = args->trans;
> -	length = dp->d_ops->data_entsize(args->namelen);
>  	/*
>  	 * If we came in with a freespace block that means that lookup
>  	 * found an entry with our hash value.  This is the freespace
> @@ -1775,56 +1761,44 @@ xfs_dir2_node_addname_int(
>  	 */
>  	if (fblk) {
>  		fbp = fblk->bp;
> -		/*
> -		 * Remember initial freespace block number.
> -		 */
> -		ifbno = fblk->blkno;
>  		free = fbp->b_addr;
>  		findex = fblk->index;
> -		bests = dp->d_ops->free_bests_p(free);
> -		dp->d_ops->free_hdr_from_disk(&freehdr, free);
> -
> -		/*
> -		 * This means the free entry showed that the data block had
> -		 * space for our entry, so we remembered it.
> -		 * Use that data block.
> -		 */
>  		if (findex >= 0) {
> +			/* caller already found the freespace for us. */
> +			bests = dp->d_ops->free_bests_p(free);
> +			dp->d_ops->free_hdr_from_disk(&freehdr, free);
> +
>  			ASSERT(findex < freehdr.nvalid);
>  			ASSERT(be16_to_cpu(bests[findex]) != NULLDATAOFF);
>  			ASSERT(be16_to_cpu(bests[findex]) >= length);
>  			dbno = freehdr.firstdb + findex;
>  			goto found_block;
> -		} else {
> -			/*
> -			 * The data block looked at didn't have enough room.
> -			 * We'll start at the beginning of the freespace entries.
> -			 */
> -			dbno = -1;
> -			findex = 0;
>  		}
> -	} else {
> +
>  		/*
> -		 * Didn't come in with a freespace block, so no data block.
> +		 * The data block looked at didn't have enough room.
> +		 * We'll start at the beginning of the freespace entries.
>  		 */
> -		ifbno = dbno = -1;
> -		fbp = NULL;
> -		findex = 0;
> +		ifbno = fblk->blkno;
> +		fbno = ifbno;
>  	}
> +	ASSERT(dbno == -1);
> +	findex = 0;
>  
>  	/*
> -	 * If we don't have a data block yet, we're going to scan the
> -	 * freespace blocks looking for one.  Figure out what the
> -	 * highest freespace block number is.
> +	 * If we don't have a data block yet, we're going to scan the freespace
> +	 * blocks looking for one.  Figure out what the highest freespace block
> +	 * number is.
>  	 */
> -	if (dbno == -1) {
> -		xfs_fileoff_t	fo;		/* freespace block number */
> +	error = xfs_bmap_last_offset(dp, &fo, XFS_DATA_FORK);
> +	if (error)
> +		return error;
> +	lastfbno = xfs_dir2_da_to_db(args->geo, (xfs_dablk_t)fo);
> +
> +	/* If we haven't get a search start block, set it now */
> +	if (fbno == -1)
> +		fbno = xfs_dir2_byte_to_db(args->geo, XFS_DIR2_FREE_OFFSET);
>  
> -		if ((error = xfs_bmap_last_offset(dp, &fo, XFS_DATA_FORK)))
> -			return error;
> -		lastfbno = xfs_dir2_da_to_db(args->geo, (xfs_dablk_t)fo);
> -		fbno = ifbno;
> -	}
>  	/*
>  	 * While we haven't identified a data block, search the freeblock
>  	 * data for a good data block.  If we find a null freeblock entry,
> @@ -1835,17 +1809,10 @@ xfs_dir2_node_addname_int(
>  		 * If we don't have a freeblock in hand, get the next one.
>  		 */
>  		if (fbp == NULL) {
> -			/*
> -			 * Happens the first time through unless lookup gave
> -			 * us a freespace block to start with.
> -			 */
> -			if (++fbno == 0)
> -				fbno = xfs_dir2_byte_to_db(args->geo,
> -							XFS_DIR2_FREE_OFFSET);
>  			/*
>  			 * If it's ifbno we already looked at it.
>  			 */
> -			if (fbno == ifbno)
> +			if (++fbno == ifbno)
>  				fbno++;
>  			/*
>  			 * If it's off the end we're done.
> @@ -1896,35 +1863,77 @@ xfs_dir2_node_addname_int(
>  			}
>  		}
>  	}
> +found_block:
> +	*dbnop = dbno;
> +	*fbpp = fbp;
> +	*findexp = findex;
> +	return 0;
> +}
> +
> +
> +/*
> + * Add the data entry for a node-format directory name addition.
> + * The leaf entry is added in xfs_dir2_leafn_add.
> + * We may enter with a freespace block that the lookup found.
> + */
> +static int
> +xfs_dir2_node_addname_int(
> +	struct xfs_da_args	*args,		/* operation arguments */
> +	struct xfs_da_state_blk	*fblk)		/* optional freespace block */
> +{
> +	struct xfs_dir2_data_unused *dup;	/* data unused entry pointer */
> +	struct xfs_dir2_data_entry *dep;	/* data entry pointer */
> +	struct xfs_dir2_data_hdr *hdr;		/* data block header */
> +	struct xfs_dir2_data_free *bf;
> +	struct xfs_dir2_free	*free = NULL;	/* freespace block structure */
> +	struct xfs_trans	*tp = args->trans;
> +	struct xfs_inode	*dp = args->dp;
> +	struct xfs_buf		*dbp;		/* data block buffer */
> +	struct xfs_buf		*fbp;		/* freespace buffer */
> +	xfs_dir2_data_aoff_t	aoff;
> +	xfs_dir2_db_t		dbno;		/* data block number */
> +	int			error;		/* error return value */
> +	int			findex;		/* freespace entry index */
> +	int			length;		/* length of the new entry */
> +	int			logfree = 0;	/* need to log free entry */
> +	int			needlog = 0;	/* need to log data header */
> +	int			needscan = 0;	/* need to rescan data frees */
> +	__be16			*tagp;		/* data entry tag pointer */
> +	__be16			*bests;
> +
> +	length = dp->d_ops->data_entsize(args->namelen);
> +	error = xfs_dir2_node_find_freeblk(args, fblk, &dbno, &fbp, &findex,
> +					   length);
> +	if (error)
> +		return error;
> +
> +	/*
> +	 * Now we know if we must allocate blocks, so if we are checking whether
> +	 * we can insert without allocation then we can return now.
> +	 */
> +	if (args->op_flags & XFS_DA_OP_JUSTCHECK) {
> +		if (dbno == -1)
> +			return -ENOSPC;
> +		return 0;
> +	}
>  
>  	/*
>  	 * If we don't have a data block, we need to allocate one and make
>  	 * the freespace entries refer to it.
>  	 */
>  	if (dbno == -1) {
> -		error = xfs_dir2_node_add_datablk(args, fblk, &dbno, &dbp, &fbp,
> -						  &findex);
> -		if (error)
> -			return error;
> -
> -		/* setup current free block buffer */
> -		free = fbp->b_addr;
> -
>  		/* we're going to have to log the free block index later */
>  		logfree = 1;
> +		error = xfs_dir2_node_add_datablk(args, fblk, &dbno, &dbp, &fbp,
> +						  &findex);
>  	} else {
> -found_block:
> -		/* If just checking, we succeeded. */
> -		if (args->op_flags & XFS_DA_OP_JUSTCHECK)
> -			return 0;
> -
>  		/* Read the data block in. */
>  		error = xfs_dir3_data_read(tp, dp,
>  					   xfs_dir2_db_to_da(args->geo, dbno),
>  					   -1, &dbp);
> -		if (error)
> -			return error;
>  	}
> +	if (error)
> +		return error;
>  
>  	/* setup for data block up now */
>  	hdr = dbp->b_addr;
> @@ -1961,6 +1970,7 @@ xfs_dir2_node_addname_int(
>  		xfs_dir2_data_log_header(args, dbp);
>  
>  	/* If the freespace block entry is now wrong, update it. */
> +	free = fbp->b_addr;
>  	bests = dp->d_ops->free_bests_p(free);
>  	if (bests[findex] != bf[0].length) {
>  		bests[findex] = bf[0].length;
> -- 
> 2.23.0.rc1
>