xfs
[Top] [All Lists]

Re: [PATCH 3/7] XFS: Refactor node format directory lookup/addname

To: Barry Naujok <bnaujok@xxxxxxx>
Subject: Re: [PATCH 3/7] XFS: Refactor node format directory lookup/addname
From: "Josef 'Jeff' Sipek" <jeffpc@xxxxxxxxxxxxxx>
Date: Wed, 2 Apr 2008 21:51:22 -0400
Cc: xfs@xxxxxxxxxxx, linux-fsdevel@xxxxxxxxxxxxxxx
In-reply-to: <20080402062708.380299192@xxxxxxxxxxxxxxxxxxxxxxx>
References: <20080402062508.017738664@xxxxxxxxxxxxxxxxxxxxxxx> <20080402062708.380299192@xxxxxxxxxxxxxxxxxxxxxxx>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Mutt/1.5.16 (2007-06-11)
On Wed, Apr 02, 2008 at 04:25:11PM +1000, Barry Naujok wrote:
...
> --- kern_ci.orig/fs/xfs/xfs_dir2_node.c
> +++ kern_ci/fs/xfs/xfs_dir2_node.c
...
> @@ -432,27 +429,15 @@ xfs_dir2_leafn_lookup_int(
>       /*
>        * Do we have a buffer coming in?
>        */
> -     if (state->extravalid)
> -             curbp = state->extrablk.bp;
> -     else
> -             curbp = NULL;
> +     curbp = state->extravalid ? state->extrablk.bp : NULL;
>       /*
>        * For addname, it's a free block buffer, get the block number.
>        */
> -     if (args->addname) {
> -             curfdb = curbp ? state->extrablk.blkno : -1;
> -             curdb = -1;
> -             length = xfs_dir2_data_entsize(args->namelen);
> -             if ((free = (curbp ? curbp->data : NULL)))
> -                     ASSERT(be32_to_cpu(free->hdr.magic) == 
> XFS_DIR2_FREE_MAGIC);
> -     }
> -     /*
> -      * For others, it's a data block buffer, get the block number.
> -      */
> -     else {
> -             curfdb = -1;
> -             curdb = curbp ? state->extrablk.blkno : -1;
> -     }
> +     curfdb = curbp ? state->extrablk.blkno : -1;
> +     free = curbp ? curbp->data : NULL;

The previous 3 lines can be cleaned up as:

if (state->extravalid)
        curbp = state->extrablk.bp;
else
        curbp = NULL;

if (curbp) {
        curfdb = state->extrablk.blkno;
        free = curbp->data;
} else {
        curfdb = -1;
        free = NULL;
}


or, if (state->extravalid && state->extrablk.bp == NULL) is _ALWAYS_ false
(which seems to be the case), you can do:


if (state->extravalid) {
        curbp = state->extrablk.bp;
        curfdb = state->extrablk.blkno;
        free = curbp->data;
} else {
        curbp = NULL;
        curfdb = -1;
        free = NULL;
}

...
> +static int
> +xfs_dir2_leafn_lookup_for_entry(
> +     xfs_dabuf_t             *bp,            /* leaf buffer */
> +     xfs_da_args_t           *args,          /* operation arguments */
> +     int                     *indexp,        /* out: leaf entry index */
> +     xfs_da_state_t          *state)         /* state to fill in */
> +{
> +     xfs_dabuf_t             *curbp;         /* current data/free buffer */
> +     xfs_dir2_db_t           curdb;          /* current data block number */
> +     xfs_dir2_data_entry_t   *dep;           /* data block entry */
> +     xfs_inode_t             *dp;            /* incore directory inode */
> +     int                     error;          /* error return value */
> +     int                     index;          /* leaf entry index */
> +     xfs_dir2_leaf_t         *leaf;          /* leaf structure */
> +     xfs_dir2_leaf_entry_t   *lep;           /* leaf entry */
> +     xfs_mount_t             *mp;            /* filesystem mount point */
> +     xfs_dir2_db_t           newdb;          /* new data block number */
> +     xfs_trans_t             *tp;            /* transaction pointer */
> +     xfs_dacmp_t             cmp;            /* comparison result */
> +     xfs_dabuf_t             *ci_bp = NULL;  /* buffer with CI match */

Did you try to check the stack usage (scripts/checkstack.pl)?

> +     dp = args->dp;
> +     tp = args->trans;
> +     mp = dp->i_mount;
> +     leaf = bp->data;
> +     ASSERT(be16_to_cpu(leaf->hdr.info.magic) == XFS_DIR2_LEAFN_MAGIC);
> +#ifdef __KERNEL__
> +     ASSERT(be16_to_cpu(leaf->hdr.count) > 0);
> +#endif

What's this #ifdef for?

> +             dep = (xfs_dir2_data_entry_t *)((char *)curbp->data +
> +                     xfs_dir2_dataptr_to_off(mp, be32_to_cpu(lep->address)));

Perhaps a static inline to do this calculation more cleanly (assuming it's
done elsewhere as well).

...
> +             /*
> +              * Compare the entry, return it if it matches.
> +              */
> +             cmp = args->oknoent ?
> +                     xfs_dir_compname(dp, dep->name, dep->namelen,
> +                                     args->name, args->namelen):
> +                     xfs_da_compname(dep->name, dep->namelen,
> +                                     args->name, args->namelen);

same as comment for 1/7.

Josef 'Jeff' Sipek.

-- 
My public GPG key can be found at
http://www.josefsipek.net/gpg/public-0xC7958FFE.txt


<Prev in Thread] Current Thread [Next in Thread>