On Thu, Aug 29, 2019 at 08:47:07PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> ---
> 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?
AFAICT the only way that would happen is if either the dir geometry
changes out from under us (??) or if someone messed with *dbno (???)
right?
Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
--D
> + xfs_alert(mp,
> +"%s: dir ino %llu needed freesp block %lld for data block %lld, got %lld",
> + __func__, (unsigned long long)dp->i_ino,
> + (long long)dp->d_ops->db_to_fdb(args->geo, *dbno),
> + (long long)*dbno, (long long)fbno);
> + if (fblk) {
> + xfs_alert(mp,
> + " fblk "PTR_FMT" blkno %llu index %d magic 0x%x",
> + fblk, (unsigned long long)fblk->blkno,
> + fblk->index, fblk->magic);
> + } else {
> + xfs_alert(mp, " ... fblk is NULL");
> + }
> + XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
> + return -EFSCORRUPTED;
> + }
> +
> + /* Get a buffer for the new block. */
> + error = xfs_dir3_free_get_buf(args, fbno, &fbp);
> + if (error)
> + return error;
> + free = fbp->b_addr;
> + bests = dp->d_ops->free_bests_p(free);
> + dp->d_ops->free_hdr_from_disk(&freehdr, free);
> +
> + /* Remember the first slot as our empty slot. */
> + freehdr.firstdb = (fbno - xfs_dir2_byte_to_db(args->geo,
> + XFS_DIR2_FREE_OFFSET)) *
> + dp->d_ops->free_max_bests(args->geo);
> + } else {
> + free = fbp->b_addr;
> + bests = dp->d_ops->free_bests_p(free);
> + dp->d_ops->free_hdr_from_disk(&freehdr, free);
> + }
> +
> + /* Set the freespace block index from the data block number. */
> + *findex = dp->d_ops->db_to_fdindex(args->geo, *dbno);
> +
> + /* Extend the freespace table if the new data block is off the end. */
> + if (*findex >= freehdr.nvalid) {
> + ASSERT(*findex < dp->d_ops->free_max_bests(args->geo));
> + freehdr.nvalid = *findex + 1;
> + bests[*findex] = cpu_to_be16(NULLDATAOFF);
> + }
> +
> + /*
> + * If this entry was for an empty data block (this should always be
> + * true) then update the header.
> + */
> + if (bests[*findex] == cpu_to_be16(NULLDATAOFF)) {
> + freehdr.nused++;
> + dp->d_ops->free_hdr_to_disk(fbp->b_addr, &freehdr);
> + xfs_dir2_free_log_header(args, fbp);
> + }
> +
> + /* Update the freespace value for the new block in the table. */
> + hdr = dbp->b_addr;
> + bf = dp->d_ops->data_bestfree_p(hdr);
> + bests[*findex] = bf[0].length;
> +
> + *dbpp = dbp;
> + *fbpp = fbp;
> + return 0;
> +}
> +
> /*
> * Add the data entry for a node-format directory name addition.
> * The leaf entry is added in xfs_dir2_leafn_add.
> @@ -1632,10 +1755,9 @@ xfs_dir2_node_addname_int(
> 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; /* need to log free entry */
> - xfs_mount_t *mp; /* filesystem mount point */
> - int needlog; /* need to log data header */
> - int needscan; /* need to rescan data frees */
> + 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;
> @@ -1644,7 +1766,6 @@ xfs_dir2_node_addname_int(
> xfs_dir2_data_aoff_t aoff;
>
> dp = args->dp;
> - mp = dp->i_mount;
> tp = args->trans;
> length = dp->d_ops->data_entsize(args->namelen);
> /*
> @@ -1673,6 +1794,7 @@ xfs_dir2_node_addname_int(
> 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.
> @@ -1774,168 +1896,46 @@ xfs_dir2_node_addname_int(
> }
> }
> }
> +
> /*
> * If we don't have a data block, we need to allocate one and make
> * the freespace entries refer to it.
> */
> - if (unlikely(dbno == -1)) {
> - /*
> - * 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.
> - */
> - if (unlikely((error = xfs_dir2_grow_inode(args,
> - XFS_DIR2_DATA_SPACE,
> - &dbno)) ||
> - (error = xfs_dir3_data_init(args, dbno, &dbp))))
> - return error;
> -
> - /*
> - * If (somehow) we have a freespace block, get rid of it.
> - */
> - if (fbp)
> - xfs_trans_brelse(tp, fbp);
> - if (fblk && fblk->bp)
> - fblk->bp = NULL;
> -
> - /*
> - * 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 (dbno == -1) {
> + error = xfs_dir2_node_add_datablk(args, fblk, &dbno, &dbp, &fbp,
> + &findex);
> 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) {
> - xfs_alert(mp,
> -"%s: dir ino %llu needed freesp block %lld for data block %lld, got %lld ifbno %llu lastfbno %d",
> - __func__, (unsigned long long)dp->i_ino,
> - (long long)dp->d_ops->db_to_fdb(
> - args->geo, dbno),
> - (long long)dbno, (long long)fbno,
> - (unsigned long long)ifbno, lastfbno);
> - if (fblk) {
> - xfs_alert(mp,
> - " fblk "PTR_FMT" blkno %llu index %d magic 0x%x",
> - fblk,
> - (unsigned long long)fblk->blkno,
> - fblk->index,
> - fblk->magic);
> - } else {
> - xfs_alert(mp, " ... fblk is NULL");
> - }
> - XFS_ERROR_REPORT("xfs_dir2_node_addname_int",
> - XFS_ERRLEVEL_LOW, mp);
> - return -EFSCORRUPTED;
> - }
> -
> - /*
> - * Get a buffer for the new block.
> - */
> - error = xfs_dir3_free_get_buf(args, fbno, &fbp);
> - if (error)
> - return error;
> - free = fbp->b_addr;
> - bests = dp->d_ops->free_bests_p(free);
> - dp->d_ops->free_hdr_from_disk(&freehdr, free);
> -
> - /*
> - * Remember the first slot as our empty slot.
> - */
> - freehdr.firstdb =
> - (fbno - xfs_dir2_byte_to_db(args->geo,
> - XFS_DIR2_FREE_OFFSET)) *
> - dp->d_ops->free_max_bests(args->geo);
> - } else {
> - free = fbp->b_addr;
> - bests = dp->d_ops->free_bests_p(free);
> - dp->d_ops->free_hdr_from_disk(&freehdr, free);
> - }
> + /* setup current free block buffer */
> + free = fbp->b_addr;
>
> - /*
> - * Set the freespace block index from the data block number.
> - */
> - findex = dp->d_ops->db_to_fdindex(args->geo, dbno);
> - /*
> - * If it's after the end of the current entries in the
> - * freespace block, extend that table.
> - */
> - if (findex >= freehdr.nvalid) {
> - ASSERT(findex < dp->d_ops->free_max_bests(args->geo));
> - freehdr.nvalid = findex + 1;
> - /*
> - * Tag new entry so nused will go up.
> - */
> - bests[findex] = cpu_to_be16(NULLDATAOFF);
> - }
> - /*
> - * If this entry was for an empty data block
> - * (this should always be true) then update the header.
> - */
> - if (bests[findex] == cpu_to_be16(NULLDATAOFF)) {
> - freehdr.nused++;
> - dp->d_ops->free_hdr_to_disk(fbp->b_addr, &freehdr);
> - xfs_dir2_free_log_header(args, fbp);
> - }
> - /*
> - * Update the real value in the table.
> - * We haven't allocated the data entry yet so this will
> - * change again.
> - */
> - hdr = dbp->b_addr;
> - bf = dp->d_ops->data_bestfree_p(hdr);
> - bests[findex] = bf[0].length;
> + /* we're going to have to log the free block index later */
> logfree = 1;
> - }
> - /*
> - * We had a data block so we don't have to make a new one.
> - */
> - else {
> - /*
> - * If just checking, we succeeded.
> - */
> + } else {
> +found_block:
> + /* If just checking, we succeeded. */
> if (args->op_flags & XFS_DA_OP_JUSTCHECK)
> return 0;
>
> - /*
> - * Read the data block in.
> - */
> + /* 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;
> - hdr = dbp->b_addr;
> - bf = dp->d_ops->data_bestfree_p(hdr);
> - logfree = 0;
> }
> +
> + /* setup for data block up now */
> + hdr = dbp->b_addr;
> + bf = dp->d_ops->data_bestfree_p(hdr);
> ASSERT(be16_to_cpu(bf[0].length) >= length);
> - /*
> - * Point to the existing unused space.
> - */
> +
> + /* Point to the existing unused space. */
> dup = (xfs_dir2_data_unused_t *)
> ((char *)hdr + be16_to_cpu(bf[0].offset));
> - needscan = needlog = 0;
> - /*
> - * Mark the first part of the unused space, inuse for us.
> - */
> +
> + /* Mark the first part of the unused space, inuse for us. */
> aoff = (xfs_dir2_data_aoff_t)((char *)dup - (char *)hdr);
> error = xfs_dir2_data_use_free(args, dbp, dup, aoff, length,
> &needlog, &needscan);
> @@ -1943,9 +1943,8 @@ xfs_dir2_node_addname_int(
> xfs_trans_brelse(tp, dbp);
> return error;
> }
> - /*
> - * Fill in the new entry and log it.
> - */
> +
> + /* Fill in the new entry and log it. */
> dep = (xfs_dir2_data_entry_t *)dup;
> dep->inumber = cpu_to_be64(args->inumber);
> dep->namelen = args->namelen;
> @@ -1954,32 +1953,25 @@ xfs_dir2_node_addname_int(
> tagp = dp->d_ops->data_entry_tag_p(dep);
> *tagp = cpu_to_be16((char *)dep - (char *)hdr);
> xfs_dir2_data_log_entry(args, dbp, dep);
> - /*
> - * Rescan the block for bestfree if needed.
> - */
> +
> + /* Rescan the freespace and log the data block if needed. */
> if (needscan)
> xfs_dir2_data_freescan(dp, hdr, &needlog);
> - /*
> - * Log the data block header if needed.
> - */
> if (needlog)
> xfs_dir2_data_log_header(args, dbp);
> - /*
> - * If the freespace entry is now wrong, update it.
> - */
> - bests = dp->d_ops->free_bests_p(free); /* gcc is so stupid */
> - if (be16_to_cpu(bests[findex]) != be16_to_cpu(bf[0].length)) {
> +
> + /* If the freespace block entry is now wrong, update it. */
> + bests = dp->d_ops->free_bests_p(free);
> + if (bests[findex] != bf[0].length) {
> bests[findex] = bf[0].length;
> logfree = 1;
> }
> - /*
> - * Log the freespace entry if needed.
> - */
> +
> + /* Log the freespace entry if needed. */
> if (logfree)
> xfs_dir2_free_log_bests(args, fbp, findex, findex);
> - /*
> - * Return the data block and offset in args, then drop the data block.
> - */
> +
> + /* Return the data block and offset in args. */
> args->blkno = (xfs_dablk_t)dbno;
> args->index = be16_to_cpu(*tagp);
> return 0;
> --
> 2.23.0.rc1
>
