On Tue, Oct 03, 2017 at 01:42:42PM -0700, Darrick J. Wong wrote:
> +/* Find an entry at a certain level in a da btree. */
> +STATIC void *
> +xfs_scrub_da_btree_entry(
> + struct xfs_scrub_da_btree *ds,
> + int level,
> + int rec)
> +{
> + char *ents;
> + void *(*fn)(void *);
> + size_t sz;
> + struct xfs_da_state_blk *blk;
> +
> + /* Dispatch the entry finding function. */
> + blk = &ds->state->path.blk[level];
> + switch (blk->magic) {
> + case XFS_ATTR_LEAF_MAGIC:
> + case XFS_ATTR3_LEAF_MAGIC:
> + fn = (xfs_da_leaf_ents_fn)xfs_attr3_leaf_entryp;
> + sz = sizeof(struct xfs_attr_leaf_entry);
> + break;
> + case XFS_DIR2_LEAFN_MAGIC:
> + case XFS_DIR3_LEAFN_MAGIC:
> + fn = (xfs_da_leaf_ents_fn)ds->dargs.dp->d_ops->leaf_ents_p;
> + sz = sizeof(struct xfs_dir2_leaf_entry);
> + break;
> + case XFS_DIR2_LEAF1_MAGIC:
> + case XFS_DIR3_LEAF1_MAGIC:
> + fn = (xfs_da_leaf_ents_fn)ds->dargs.dp->d_ops->leaf_ents_p;
> + sz = sizeof(struct xfs_dir2_leaf_entry);
> + break;
> + case XFS_DA_NODE_MAGIC:
> + case XFS_DA3_NODE_MAGIC:
> + fn = (xfs_da_leaf_ents_fn)ds->dargs.dp->d_ops->node_tree_p;
> + sz = sizeof(struct xfs_da_node_entry);
> + break;
> + default:
> + return NULL;
> + }
> +
> + ents = fn(blk->bp->b_addr);
> + return ents + (sz * rec);
> +}
This looks kinda unnecesarily abstracted.
case XFS_ATTR_LEAF_MAGIC:
case XFS_ATTR3_LEAF_MAGIC:
ents = xfs_attr3_leaf_entryp(blk->bp->b_addr);
return ents + (rec * sizeof(struct xfs_attr_leaf_entry));
case XFS_DIR2_LEAF1_MAGIC:
case XFS_DIR3_LEAF1_MAGIC:
case XFS_DIR2_LEAFN_MAGIC:
case XFS_DIR3_LEAFN_MAGIC:
ents = ds->dargs.dp->d_ops->leaf_ents_p(blk->bp->b_addr);
return ents + (rec * sizeof(struct xfs_dir2_leaf_entry));
case XFS_DA_NODE_MAGIC:
case XFS_DA3_NODE_MAGIC:
ents = ds->dargs.dp->d_ops->node_tree_p(blk->bp->b_addr)
return ents + (rec * sizeof(struct xfs_da_node_entry));
> +
> +/* Scrub a da btree hash (key). */
> +int
> +xfs_scrub_da_btree_hash(
> + struct xfs_scrub_da_btree *ds,
> + int level,
> + __be32 *hashp)
> +{
> + struct xfs_da_state_blk *blks;
> + struct xfs_da_node_entry *btree;
*entry?
> + xfs_dahash_t hash;
> + xfs_dahash_t parent_hash;
> +
> + /* Is this hash in order? */
> + hash = be32_to_cpu(*hashp);
> + if (hash < ds->hashes[level])
> + xfs_scrub_da_set_corrupt(ds, level);
> + ds->hashes[level] = hash;
> +
> + if (level == 0)
> + return 0;
> +
> + /* Is this hash no larger than the parent hash? */
> + blks = ds->state->path.blk;
> + btree = xfs_scrub_da_btree_entry(ds, level - 1, blks[level - 1].index);
entry = ?
> + parent_hash = be32_to_cpu(btree->hashval);
> + if (parent_hash < hash)
> + xfs_scrub_da_set_corrupt(ds, level);
> +
> + return 0;
> +}
> +
> +/*
> + * Check a da btree pointer. Returns true if it's ok to use this
> + * pointer.
> + */
> +STATIC bool
> +xfs_scrub_da_btree_ptr_ok(
> + struct xfs_scrub_da_btree *ds,
> + int level,
> + xfs_dablk_t blkno)
> +{
> + if (blkno < ds->lowest || (ds->highest != 0 && blkno >= ds->highest)) {
> + xfs_scrub_da_set_corrupt(ds, level);
> + return false;
> + }
Not sure what lowest and highest are here - the structure definition
is not commented. I /think/ it's the offset within the dierctory
address space for the leaf pointers (i.e. XFS_DIR2_LEAF_OFFSET ->
XFS_DIR2_FREE_OFFSET for directories), but I'm mostly guessing from
context here...
> +
> + return true;
> +}
> +
> +/*
> + * The da btree scrubber can handle leaf1 blocks as a degenerate
> + * form of da btree. Since the regular da code doesn't handle
degenerate form of LEAFN blocks?
> +
> +/* Check a block's sibling. */
> +STATIC int
> +xfs_scrub_da_btree_block_check_sibling(
> + struct xfs_scrub_da_btree *ds,
> + int level,
> + int direction,
> + xfs_dablk_t sibling)
> +{
> + int retval;
> + int error;
> +
> + if (!sibling)
> + return 0;
> +
> + /* Move the alternate cursor back one block. */
Move the alternate cursor one block in the direction specified?
> + memcpy(&ds->state->altpath, &ds->state->path,
> + sizeof(ds->state->altpath));
> + error = xfs_da3_path_shift(ds->state, &ds->state->altpath,
> + direction, false, &retval);
> + if (!xfs_scrub_da_op_ok(ds, level, &error))
> + return error;
> + if (retval) {
> + xfs_scrub_da_set_corrupt(ds, level);
> + return error;
> + }
> +
> + if (ds->state->altpath.blk[level].blkno != sibling)
> + xfs_scrub_da_set_corrupt(ds, level);
> + xfs_trans_brelse(ds->dargs.trans, ds->state->altpath.blk[level].bp);
> + return error;
> +}
> +
> +/* Check a block's sibling pointers. */
> +STATIC int
> +xfs_scrub_da_btree_block_check_siblings(
> + struct xfs_scrub_da_btree *ds,
> + int level,
> + struct xfs_da_blkinfo *hdr)
> +{
> + xfs_dablk_t forw;
> + xfs_dablk_t back;
> + int error = 0;
> +
> + forw = be32_to_cpu(hdr->forw);
> + back = be32_to_cpu(hdr->back);
> +
> + /* Top level blocks should not have sibling pointers. */
> + if (level == 0) {
> + if (forw != 0 || back != 0)
> + xfs_scrub_da_set_corrupt(ds, level);
> + return error;
Error is always zero here?
> + }
> +
> + /*
> + * Check back (left) and forw (right) pointers. These functions
> + * absorb error codes for us.
> + */
> + error = xfs_scrub_da_btree_block_check_sibling(ds, level, 0, back);
> + if (error)
> + goto out;
> + error = xfs_scrub_da_btree_block_check_sibling(ds, level, 1, forw);
> +
> +out:
> + memset(&ds->state->altpath, 0, sizeof(ds->state->altpath));
> + return error;
> +}
> +
> +/* Load a dir/attribute block from a btree. */
> +STATIC int
> +xfs_scrub_da_btree_block(
> + struct xfs_scrub_da_btree *ds,
> + int level,
> + xfs_dablk_t blkno)
> +{
> + struct xfs_da_state_blk *blk;
> + struct xfs_da_intnode *node;
> + struct xfs_da_node_entry *btree;
> + struct xfs_da3_blkinfo *hdr3;
> + struct xfs_da_args *dargs = &ds->dargs;
> + struct xfs_inode *ip = ds->dargs.dp;
> + xfs_ino_t owner;
> + int *pmaxrecs;
> + struct xfs_da3_icnode_hdr nodehdr;
> + int error;
> +
> + blk = &ds->state->path.blk[level];
> + ds->state->path.active = level + 1;
> +
> + /* Release old block. */
> + if (blk->bp) {
> + xfs_trans_brelse(dargs->trans, blk->bp);
> + blk->bp = NULL;
> + }
> +
> + /* Check the pointer. */
> + blk->blkno = blkno;
> + if (!xfs_scrub_da_btree_ptr_ok(ds, level, blkno))
> + goto out_nobuf;
> +
> + /* Read the buffer. */
> + error = xfs_da_read_buf(dargs->trans, dargs->dp, blk->blkno, -2,
> + &blk->bp, dargs->whichfork,
> + &xfs_scrub_da_btree_buf_ops);
Hmmm - this verifier only special cases LEAF1 blocks, no comments as
to why it treats everything else as a with the node verifier. DOn't
we have to special case the attr leaf blocks here as well?
> + if (!xfs_scrub_da_op_ok(ds, level, &error))
> + goto out_nobuf;
> +
> + /* It's ok for a directory not to have a da btree in it. */
> + if (ds->dargs.whichfork == XFS_DATA_FORK && level == 0 &&
> + blk->bp == NULL)
> + goto out_nobuf;
What case is that? single block form? Need a magic number check
here if that's the case?
> +/* Visit all nodes and leaves of a da btree. */
> +int
> +xfs_scrub_da_btree(
> + struct xfs_scrub_context *sc,
> + int whichfork,
> + xfs_scrub_da_btree_rec_fn scrub_fn)
> +{
> + struct xfs_scrub_da_btree ds;
> + struct xfs_mount *mp = sc->mp;
> + struct xfs_da_state_blk *blks;
> + struct xfs_da_node_entry *key;
> + void *rec;
> + xfs_dablk_t blkno;
> + bool is_attr;
> + int level;
> + int error;
> +
> + memset(&ds, 0, sizeof(ds));
I almost missed this - had to go looking later for why the
ds.maxrecs[] started off at zero. Can we change this to be
initialised to zero at declaration like so:
struct xfs_scrub_da_btree ds = {};
> + /* Skip short format data structures; no btree to scan. */
> + if (XFS_IFORK_FORMAT(sc->ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
> + XFS_IFORK_FORMAT(sc->ip, whichfork) != XFS_DINODE_FMT_BTREE)
> + return 0;
> +
> + /* Set up initial da state. */
> + is_attr = whichfork == XFS_ATTR_FORK;
> + ds.dargs.geo = is_attr ? mp->m_attr_geo : mp->m_dir_geo;
> + ds.dargs.dp = sc->ip;
> + ds.dargs.whichfork = whichfork;
> + ds.dargs.trans = sc->tp;
> + ds.dargs.op_flags = XFS_DA_OP_OKNOENT;
> + ds.state = xfs_da_state_alloc();
> + ds.state->args = &ds.dargs;
> + ds.state->mp = mp;
> + ds.sc = sc;
> + blkno = ds.lowest = is_attr ? 0 : ds.dargs.geo->leafblk;
> + ds.highest = is_attr ? 0 : ds.dargs.geo->freeblk;
> + level = 0;
bit hard to read with all the ?: constructs. Can we make this:
if (whichfork == XFS_ATTR_FORK) {
ds.dargs.geo = ...
ds.lowest = ..
ds.highest = ...
} else {
....
}
......
blkno = ds.lowest;
> +
> + /* Find the root of the da tree, if present. */
> + blks = ds.state->path.blk;
> + error = xfs_scrub_da_btree_block(&ds, level, blkno);
> + if (error)
> + goto out_state;
> + if (blks[level].bp == NULL)
> + goto out_state;
So for a single block directory, we'll jump out here because it's
at block zero and there's nothing at mp->m_dir_geo.leafblk.
That means the loop will only ever handle LEAF1/LEAFN format
directory structures. Correct? (comment?)
> + blks[level].index = 0;
> + while (level >= 0 && level < XFS_DA_NODE_MAXDEPTH) {
> + /* Handle leaf block. */
> + if (blks[level].magic != XFS_DA_NODE_MAGIC) {
> + /* End of leaf, pop back towards the root. */
> + if (blks[level].index >= ds.maxrecs[level]) {
> + if (level > 0)
> + blks[level - 1].index++;
> + ds.tree_level++;
> + level--;
> + continue;
> + }
> +
> + /* Dispatch record scrubbing. */
> + rec = xfs_scrub_da_btree_entry(&ds, level,
> + blks[level].index);
> + error = scrub_fn(&ds, level, rec);
> + if (error < 0 ||
> + error == XFS_BTREE_QUERY_RANGE_ABORT)
When would we get a XFS_BTREE_QUERY_RANGE_ABORT error?
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
