Hi Dave,
Comments below. Basically just some nits about asserts, comments, variable
names, and looks like an off-by-one in xfs_dir3_leaf_log_header. I would like
to know your plan for xfs_da_root_split and xfs_dir3_leaf_check_int.
Nice find in xfs_dir2_block_to_leaf! That should be a separate patch.
Regards,
Ben
On Wed, Apr 03, 2013 at 04:11:23PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> This addition follows the same pattern as the dir2 block CRCs.
> Seeing as both LEAF1 and LEAFN types need to changed at the same
> time, this is a pretty large amount of change. leaf block headers
> neeed to be abstracted away from the on-disk structures (struct
need
> xfs_dir3_icleaf_hdr), as do the base leaf entry locations.
>
> This header abstract allows the in-core header and leaf entry
> location to be passed around instead of the leaf block itself. This
> saves a lot of converting individual variables from on-disk format
> to host format where they are used, so there's a good chance that
> the compiler will be able to produce much more optimal code as it's
> not having to byteswap variables all over the place.
>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
...
> @@ -396,11 +397,18 @@ xfs_da_root_split(xfs_da_state_t *state, xfs_da_state_blk_t *blk1,
> size = (int)((char *)&oldroot->btree[be16_to_cpu(oldroot->hdr.count)] -
> (char *)oldroot);
> } else {
> - ASSERT(oldroot->hdr.info.magic == cpu_to_be16(XFS_DIR2_LEAFN_MAGIC));
> + struct xfs_dir3_icleaf_hdr leafhdr;
> + struct xfs_dir2_leaf_entry *ents;
> +
> leaf = (xfs_dir2_leaf_t *)oldroot;
> - size = (int)((char *)&leaf->ents[be16_to_cpu(leaf->hdr.count)] -
> - (char *)leaf);
> + xfs_dir3_leaf_hdr_from_disk(&leafhdr, leaf);
> + ents = xfs_dir3_leaf_ents_p(leaf);
> +
> + ASSERT(leafhdr.magic == XFS_DIR2_LEAFN_MAGIC ||
> + leafhdr.magic == XFS_DIR3_LEAFN_MAGIC);
> + size = (int)((char *)&ents[leafhdr.count] - (char *)leaf);
> }
> + /* XXX: can't just copy CRC headers from one block to another */
What is your plan for resolving that?
> @@ -2281,10 +2299,17 @@ xfs_da_read_buf(
> XFS_TEST_ERROR((magic != XFS_DA_NODE_MAGIC) &&
> (magic != XFS_ATTR_LEAF_MAGIC) &&
> (magic != XFS_DIR2_LEAF1_MAGIC) &&
> + (magic != XFS_DIR3_LEAF1_MAGIC) &&
> (magic != XFS_DIR2_LEAFN_MAGIC) &&
> + (magic != XFS_DIR3_LEAFN_MAGIC) &&
> (magic1 != XFS_DIR2_BLOCK_MAGIC) &&
> + (magic1 != XFS_DIR3_BLOCK_MAGIC) &&
Did this DIR3_BLOCK_MAGIC change sneak in from another patch?
> (magic1 != XFS_DIR2_DATA_MAGIC) &&
> - (free->hdr.magic != cpu_to_be32(XFS_DIR2_FREE_MAGIC)),
> + (magic1 != XFS_DIR3_DATA_MAGIC) &&
> + (free->hdr.magic !=
> + cpu_to_be32(XFS_DIR2_FREE_MAGIC)) &&
> + (free->hdr.magic !=
> + cpu_to_be32(XFS_DIR3_FREE_MAGIC)),
Do DIRx_DATA_MAGIC and DIRx_FREE_MAGIC belong in a different patch?
> /*
> * Get address of the bestcount field in the single-leaf block.
> */
> +static inline struct xfs_dir2_leaf_entry *
> +xfs_dir3_leaf_ents_p(struct xfs_dir2_leaf *lp)
> +{
> + if (lp->hdr.info.magic == cpu_to_be16(XFS_DIR3_LEAF1_MAGIC) ||
> + lp->hdr.info.magic == cpu_to_be16(XFS_DIR3_LEAFN_MAGIC)) {
> + struct xfs_dir3_leaf *lp3 = (struct xfs_dir3_leaf *)lp;
> + return lp3->__ents;
> + }
> + return lp->__ents;
> +}
> +
> +/*
> + * Get address of the bestcount field in the single-leaf block.
> + */
I appreciate the addition of the comment.
> +bool
> +xfs_dir3_leaf_check_int(
> + struct xfs_mount *mp,
> + struct xfs_dir3_icleaf_hdr *hdr,
> + struct xfs_dir2_leaf *leaf)
> +{
> + struct xfs_dir2_leaf_entry *ents;
> + xfs_dir2_leaf_tail_t *ltp;
> + int stale;
> + int i;
> +
> + ents = xfs_dir3_leaf_ents_p(leaf);
> + ltp = xfs_dir2_leaf_tail_p(mp, leaf);
> +
> + /*
> + * This value is not restrictive enough.
> + * Should factor in the size of the bests table as well.
> + * We can deduce a value for that from di_size.
> + */
^^^^^
That should probably be marked with a TODO.
> +static bool
> +xfs_dir3_leaf_verify(
> struct xfs_buf *bp,
> - __be16 magic)
> + __uint16_t magic)
> +{
> + struct xfs_mount *mp = bp->b_target->bt_mount;
> + struct xfs_dir2_leaf *leaf = bp->b_addr;
> + struct xfs_dir3_icleaf_hdr leafhdr;
> +
> + ASSERT(magic == XFS_DIR2_LEAF1_MAGIC || magic == XFS_DIR2_LEAFN_MAGIC);
Using v2 magics to choose between leaf1 and leafn is ok, but not as clear as
using a non-version-specific #define or enum might be for this purpose.
> @@ -169,27 +432,34 @@ xfs_dir2_block_to_leaf(
> /*
> * Initialize the leaf block, get a buffer for it.
> */
> - if ((error = xfs_dir2_leaf_init(args, ldb, &lbp, XFS_DIR2_LEAF1_MAGIC))) {
> + error = xfs_dir3_leaf_get_buf(args, ldb, &lbp, XFS_DIR2_LEAF1_MAGIC);
> + if (error)
> return error;
> - }
> - ASSERT(lbp != NULL);
> +
> leaf = lbp->b_addr;
> hdr = dbp->b_addr;
> xfs_dir3_data_check(dp, dbp);
> btp = xfs_dir2_block_tail_p(mp, hdr);
> blp = xfs_dir2_block_leaf_p(btp);
> bf = xfs_dir3_data_bestfree_p(hdr);
> + ents = xfs_dir3_leaf_ents_p(leaf);
> +
> /*
> * Set the counts in the leaf header.
> + *
> + * XXX: are these actually logged, or just gathered by chance?
Nice find. I'm not seeing where that header is being logged. Worth a separate
patch.
> void
> -xfs_dir2_leaf_log_header(
> +xfs_dir3_leaf_log_header(
> struct xfs_trans *tp,
> struct xfs_buf *bp)
> {
> - xfs_dir2_leaf_t *leaf; /* leaf structure */
> + struct xfs_dir2_leaf *leaf = bp->b_addr;
>
> - leaf = bp->b_addr;
> ASSERT(leaf->hdr.info.magic == cpu_to_be16(XFS_DIR2_LEAF1_MAGIC) ||
> - leaf->hdr.info.magic == cpu_to_be16(XFS_DIR2_LEAFN_MAGIC));
> + leaf->hdr.info.magic == cpu_to_be16(XFS_DIR3_LEAF1_MAGIC) ||
> + leaf->hdr.info.magic == cpu_to_be16(XFS_DIR2_LEAFN_MAGIC) ||
> + leaf->hdr.info.magic == cpu_to_be16(XFS_DIR3_LEAFN_MAGIC));
> +
> xfs_trans_log_buf(tp, bp, (uint)((char *)&leaf->hdr - (char *)leaf),
> - (uint)(sizeof(leaf->hdr) - 1));
> + xfs_dir3_leaf_hdr_size(leaf));
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Should be xfs_dir3_leaf_hdr_size(leaf) - 1, I think.
> @@ -1958,36 +2171,40 @@ xfs_dir2_node_to_leaf(
> * Now see if the leafn and free data will fit in a leaf1.
> * If not, release the buffer and give up.
> */
> - if (xfs_dir2_leaf_size(&leaf->hdr, freehdr.nvalid) > mp->m_dirblksize) {
> + if (xfs_dir3_leaf_size(&leafhdr, freehdr.nvalid) > mp->m_dirblksize) {
> xfs_trans_brelse(tp, fbp);
> return 0;
> }
>
> /*
> * If the leaf has any stale entries in it, compress them out.
> - * The compact routine will log the header.
> */
> - if (be16_to_cpu(leaf->hdr.stale))
> - xfs_dir2_leaf_compact(args, lbp);
> - else
> - xfs_dir2_leaf_log_header(tp, lbp);
> + if (leafhdr.stale)
> + xfs_dir3_leaf_compact(args, &leafhdr, lbp);
>
> - lbp->b_ops = &xfs_dir2_leaf1_buf_ops;
> - leaf->hdr.info.magic = cpu_to_be16(XFS_DIR2_LEAF1_MAGIC);
> + lbp->b_ops = &xfs_dir3_leaf1_buf_ops;
> + leafhdr.magic = (leafhdr.magic == XFS_DIR2_LEAFN_MAGIC)
> + ? XFS_DIR2_LEAF1_MAGIC
> + : XFS_DIR3_LEAF1_MAGIC;
An assert might be nice here:
if (leafhdr.magic == XFS_DIR2_LEAFN_MAGIC)
leafhdr.magic = XFS_DIR2_LEAF1_MAGIC;
else {
ASSERT(leafhdr.magic == XFS_DIR3_LEAFN_MAGIC);
leafhdr.magic = XFS_DIR3_LEAF1_MAGIC;
}
> static bool
> xfs_dir3_free_verify(
> struct xfs_buf *bp)
> @@ -360,11 +385,19 @@ xfs_dir2_leaf_to_node(
> xfs_dir2_free_log_bests(tp, fbp, 0, freehdr.nvalid - 1);
> xfs_dir2_free_log_header(tp, fbp);
>
> - /* convert the leaf to a leafnode */
> - leaf->hdr.info.magic = cpu_to_be16(XFS_DIR2_LEAFN_MAGIC);
> - lbp->b_ops = &xfs_dir2_leafn_buf_ops;
> - xfs_dir2_leaf_log_header(tp, lbp);
> - xfs_dir2_leafn_check(dp, lbp);
> + /*
> + * Converting the leaf to a leafnode is just a matter of changing the
> + * magic number and the ops. Do the change directly to the buffer as
> + * it's less work (and less code) than decoding the header to host
> + * format and back again.
> + */
> + if (leaf->hdr.info.magic == cpu_to_be16(XFS_DIR2_LEAF1_MAGIC))
> + leaf->hdr.info.magic = cpu_to_be16(XFS_DIR2_LEAFN_MAGIC);
> + else
> + leaf->hdr.info.magic = cpu_to_be16(XFS_DIR3_LEAFN_MAGIC);
Here too might be a good spot for an assert in the else.
> static void
> xfs_dir2_free_hdr_check(
> struct xfs_mount *mp,
> @@ -510,15 +520,18 @@ xfs_dir2_leafn_lasthash(
> struct xfs_buf *bp, /* leaf buffer */
> int *count) /* count of entries in leaf */
> {
> - xfs_dir2_leaf_t *leaf; /* leaf structure */
> + struct xfs_dir2_leaf *leaf = bp->b_addr;
> + struct xfs_dir2_leaf_entry *ents;
> + struct xfs_dir3_icleaf_hdr leafhdr;
> +
> + xfs_dir3_leaf_hdr_from_disk(&leafhdr, leaf);
> + ents = xfs_dir3_leaf_ents_p(leaf);
>
> - leaf = bp->b_addr;
> - ASSERT(leaf->hdr.info.magic == cpu_to_be16(XFS_DIR2_LEAFN_MAGIC));
Looks like this assert has gone missing. It was intentionally more specific
than the one in xfs_dir3_leaf_hdr_from_disk, right?
> @@ -547,16 +560,19 @@ xfs_dir2_leafn_lookup_for_addname(
> xfs_dir2_db_t newdb; /* new data block number */
> xfs_dir2_db_t newfdb; /* new free block number */
> xfs_trans_t *tp; /* transaction pointer */
> + struct xfs_dir2_leaf_entry *ents;
> + struct xfs_dir3_icleaf_hdr leafhdr;
>
> dp = args->dp;
> tp = args->trans;
> mp = dp->i_mount;
> leaf = bp->b_addr;
> - ASSERT(leaf->hdr.info.magic == cpu_to_be16(XFS_DIR2_LEAFN_MAGIC));
Here the assert is also missing, but it looks like it is handled below in
xfs_dir3_leaf_check. Maybe the above is similar.
> -#ifdef __KERNEL__
> - ASSERT(be16_to_cpu(leaf->hdr.count) > 0);
> -#endif
> - xfs_dir2_leafn_check(dp, bp);
> + xfs_dir3_leaf_hdr_from_disk(&leafhdr, leaf);
> + ents = xfs_dir3_leaf_ents_p(leaf);
> +
> + xfs_dir3_leaf_check(mp, bp);
...
> @@ -694,16 +710,19 @@ xfs_dir2_leafn_lookup_for_entry(
> xfs_dir2_db_t newdb; /* new data block number */
> xfs_trans_t *tp; /* transaction pointer */
> enum xfs_dacmp cmp; /* comparison result */
> + struct xfs_dir2_leaf_entry *ents;
> + struct xfs_dir3_icleaf_hdr leafhdr;
>
> dp = args->dp;
> tp = args->trans;
> mp = dp->i_mount;
> leaf = bp->b_addr;
> - ASSERT(leaf->hdr.info.magic == cpu_to_be16(XFS_DIR2_LEAFN_MAGIC));
Again. But you've also got xfs_dir3_leaf_check below, so maybe the first one is
the only issue.
> -#ifdef __KERNEL__
> - ASSERT(be16_to_cpu(leaf->hdr.count) > 0);
> -#endif
> - xfs_dir2_leafn_check(dp, bp);
> + xfs_dir3_leaf_hdr_from_disk(&leafhdr, leaf);
> + ents = xfs_dir3_leaf_ents_p(leaf);
> +
> + xfs_dir3_leaf_check(mp, bp);
...
> +xfs_dir3_leafn_moveents(
> + xfs_da_args_t *args, /* operation arguments */
> + struct xfs_buf *bp_s, /* source */
> + struct xfs_dir3_icleaf_hdr *shdr,
> + struct xfs_dir2_leaf_entry *sents,
> + int start_s,/* source leaf index */
> + struct xfs_buf *bp_d, /* destination */
> + struct xfs_dir3_icleaf_hdr *dhdr,
> + struct xfs_dir2_leaf_entry *dents,
> + int start_d,/* destination leaf index */
> + int count) /* count of leaves to copy */
> {
> - xfs_dir2_leaf_t *leaf_d; /* destination leaf structure */
> - xfs_dir2_leaf_t *leaf_s; /* source leaf structure */
> - int stale; /* count stale leaves copied */
> - xfs_trans_t *tp; /* transaction pointer */
> + int stale;
I agree
/* transaction pointer */
is a bit overkill but
/* count stale leaves copied */
does help a new reader of this code.
> @@ -922,21 +936,25 @@ xfs_dir2_leafn_moveents(
> */
> int /* sort order */
> xfs_dir2_leafn_order(
> - struct xfs_buf *leaf1_bp, /* leaf1 buffer */
> - struct xfs_buf *leaf2_bp) /* leaf2 buffer */
> + struct xfs_buf *leaf1_bp, /* leaf1 buffer */
> + struct xfs_buf *leaf2_bp) /* leaf2 buffer */
> {
> - xfs_dir2_leaf_t *leaf1; /* leaf1 structure */
> - xfs_dir2_leaf_t *leaf2; /* leaf2 structure */
> -
> - leaf1 = leaf1_bp->b_addr;
> - leaf2 = leaf2_bp->b_addr;
> - ASSERT(leaf1->hdr.info.magic == cpu_to_be16(XFS_DIR2_LEAFN_MAGIC));
> - ASSERT(leaf2->hdr.info.magic == cpu_to_be16(XFS_DIR2_LEAFN_MAGIC));
These asserts are more specific than those in xfs_dir3_leaf_hdr_from_disk so
they should probably stay and have dirv3 magic added.
> @@ -1153,6 +1193,8 @@ xfs_dir2_leafn_remove(
> int needscan; /* need to rescan data frees */
> xfs_trans_t *tp; /* transaction pointer */
> struct xfs_dir2_data_free *bf; /* bestfree table */
> + struct xfs_dir3_icleaf_hdr leafhdr;
> + struct xfs_dir2_leaf_entry *ents;
>
> trace_xfs_dir2_leafn_remove(args, index);
>
> @@ -1160,11 +1202,14 @@ xfs_dir2_leafn_remove(
> tp = args->trans;
> mp = dp->i_mount;
> leaf = bp->b_addr;
> - ASSERT(leaf->hdr.info.magic == cpu_to_be16(XFS_DIR2_LEAFN_MAGIC));
Another assert that is more specific than what it is replaced with, but you do
have a leafn check, so that's something.
> @@ -1366,6 +1413,8 @@ xfs_dir2_leafn_toosmall(
> xfs_da_blkinfo_t *info; /* leaf block header */
Looks like you can get rid of the info pointer now.
> @@ -1374,10 +1423,12 @@ xfs_dir2_leafn_toosmall(
> */
> blk = &state->path.blk[state->path.active - 1];
> info = blk->bp->b_addr;
> - ASSERT(info->magic == cpu_to_be16(XFS_DIR2_LEAFN_MAGIC));
Another assert that is more specific than it's replacement.
> @@ -1428,13 +1481,15 @@ xfs_dir2_leafn_toosmall(
> /*
> * Count bytes in the two blocks combined.
> */
> - leaf = (xfs_dir2_leaf_t *)info;
> - count = be16_to_cpu(leaf->hdr.count) - be16_to_cpu(leaf->hdr.stale);
> + count = leafhdr.count - leafhdr.stale;
> bytes = state->blocksize - (state->blocksize >> 2);
> +
> leaf = bp->b_addr;
> - ASSERT(leaf->hdr.info.magic == cpu_to_be16(XFS_DIR2_LEAFN_MAGIC));
> - count += be16_to_cpu(leaf->hdr.count) - be16_to_cpu(leaf->hdr.stale);
> - bytes -= count * (uint)sizeof(leaf->ents[0]);
^^^^^
I wonder why that (uint) was added.
> @@ -1481,34 +1536,52 @@ xfs_dir2_leafn_unbalance(
> xfs_da_args_t *args; /* operation arguments */
> xfs_dir2_leaf_t *drop_leaf; /* dead leaf structure */
> xfs_dir2_leaf_t *save_leaf; /* surviving leaf structure */
> + struct xfs_dir3_icleaf_hdr shdr;
> + struct xfs_dir3_icleaf_hdr dhdr;
> + struct xfs_dir2_leaf_entry *sents;
> + struct xfs_dir2_leaf_entry *dents;
As I read this I really had a hard time not thinking 'source' and 'dest'
instead of 'save' and 'drop'. This would be more readable as the latter.
>
> args = state->args;
> ASSERT(drop_blk->magic == XFS_DIR2_LEAFN_MAGIC);
> ASSERT(save_blk->magic == XFS_DIR2_LEAFN_MAGIC);
> drop_leaf = drop_blk->bp->b_addr;
> save_leaf = save_blk->bp->b_addr;
> - ASSERT(drop_leaf->hdr.info.magic == cpu_to_be16(XFS_DIR2_LEAFN_MAGIC));
> - ASSERT(save_leaf->hdr.info.magic == cpu_to_be16(XFS_DIR2_LEAFN_MAGIC));
More specific assert than its replacement.
> +
> + xfs_dir3_leaf_hdr_from_disk(&shdr, save_leaf);
> + xfs_dir3_leaf_hdr_from_disk(&dhdr, drop_leaf);
> + sents = xfs_dir3_leaf_ents_p(save_leaf);
> + dents = xfs_dir3_leaf_ents_p(drop_leaf);
> +
> /*
> * If there are any stale leaf entries, take this opportunity
> * to purge them.
> */
> - if (drop_leaf->hdr.stale)
> - xfs_dir2_leaf_compact(args, drop_blk->bp);
> - if (save_leaf->hdr.stale)
> - xfs_dir2_leaf_compact(args, save_blk->bp);
> + if (dhdr.stale)
> + xfs_dir3_leaf_compact(args, &dhdr, drop_blk->bp);
> + if (shdr.stale)
> + xfs_dir3_leaf_compact(args, &shdr, save_blk->bp);
> +
> /*
> * Move the entries from drop to the appropriate end of save.
> */
> - drop_blk->hashval = be32_to_cpu(drop_leaf->ents[be16_to_cpu(drop_leaf->hdr.count) - 1].hashval);
> + drop_blk->hashval = be32_to_cpu(dents[dhdr.count - 1].hashval);
> if (xfs_dir2_leafn_order(save_blk->bp, drop_blk->bp))
> - xfs_dir2_leafn_moveents(args, drop_blk->bp, 0, save_blk->bp, 0,
> - be16_to_cpu(drop_leaf->hdr.count));
> + xfs_dir3_leafn_moveents(args, drop_blk->bp, &dhdr, dents, 0,
> + save_blk->bp, &shdr, sents, 0,
> + dhdr.count);
> else
> - xfs_dir2_leafn_moveents(args, drop_blk->bp, 0, save_blk->bp,
> - be16_to_cpu(save_leaf->hdr.count), be16_to_cpu(drop_leaf->hdr.count));
> - save_blk->hashval = be32_to_cpu(save_leaf->ents[be16_to_cpu(save_leaf->hdr.count) - 1].hashval);
> - xfs_dir2_leafn_check(args->dp, save_blk->bp);
> + xfs_dir3_leafn_moveents(args, drop_blk->bp, &dhdr, dents, 0,
> + save_blk->bp, &shdr, sents,
> + shdr.count, dhdr.count);
> + save_blk->hashval = be32_to_cpu(sents[shdr.count - 1].hashval);
> +
> + /* log the changes made when moving the entries */
> + xfs_dir3_leaf_hdr_to_disk(save_leaf, &shdr);
> + xfs_dir3_leaf_hdr_to_disk(drop_leaf, &dhdr);
> + xfs_dir3_leaf_log_header(args->trans, save_blk->bp);
> + xfs_dir3_leaf_log_header(args->trans, drop_blk->bp);
> +
> + xfs_dir3_leaf_check(args->dp->i_mount, save_blk->bp);
xfs_dir3_leaf_check(args->dp->i_mount, drop_block->bp);
We used to check it in debug code. Might as well continue to do so.
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
