On Thu, Aug 29, 2019 at 08:47:08PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
--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
>
