On Thu, Oct 26, 2017 at 09:16:37AM -0400, Brian Foster wrote:
> On Wed, Oct 25, 2017 at 10:49:24PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >
> > When we're iterating the attribute list and we can't find our previous
> > location based off the attribute cursor, we'll instead walk down the
> > attribute btree from the root trying to find where we left off. Move
> > this code into a separate function for later cleanups.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > ---
> > fs/xfs/xfs_attr_list.c | 136 ++++++++++++++++++++++++++++++------------------
> > 1 file changed, 84 insertions(+), 52 deletions(-)
> >
> >
> > diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
> > index 5816786..48423eb 100644
> > --- a/fs/xfs/xfs_attr_list.c
> > +++ b/fs/xfs/xfs_attr_list.c
> > @@ -204,19 +204,89 @@ xfs_attr_shortform_list(xfs_attr_list_context_t *context)
> > return 0;
> > }
> >
> > +/*
> > + * We didn't find the block & hash mentioned in the cursor state, so
> > + * walk down the attr btree looking for the hash.
> > + */
> > STATIC int
> > -xfs_attr_node_list(xfs_attr_list_context_t *context)
> > +xfs_attr_node_list_lookup(
> > + struct xfs_attr_list_context *context,
> > + struct attrlist_cursor_kern *cursor,
> > + struct xfs_buf **pbp)
>
> Looks Ok, but the label usage at the bottom seems a bit gratuitous...
>
> > {
> > - attrlist_cursor_kern_t *cursor;
> > - xfs_attr_leafblock_t *leaf;
> > - xfs_da_intnode_t *node;
> > - struct xfs_attr3_icleaf_hdr leafhdr;
> > - struct xfs_da3_icnode_hdr nodehdr;
> > - struct xfs_da_node_entry *btree;
> > - int error, i;
> > - struct xfs_buf *bp;
> > - struct xfs_inode *dp = context->dp;
> > - struct xfs_mount *mp = dp->i_mount;
> > + struct xfs_da3_icnode_hdr nodehdr;
> > + struct xfs_da_intnode *node;
> > + struct xfs_da_node_entry *btree;
> > + struct xfs_inode *dp = context->dp;
> > + struct xfs_mount *mp = dp->i_mount;
> > + struct xfs_trans *tp = context->tp;
> > + int i;
> > + int error = 0;
> > + uint16_t magic;
> > +
> > + ASSERT(*pbp == NULL);
> > + cursor->blkno = 0;
> > + for (;;) {
> > + error = xfs_da3_node_read(tp, dp, cursor->blkno, -1, pbp,
> > + XFS_ATTR_FORK);
> > + if (error)
> > + return error;
> > + node = (*pbp)->b_addr;
> > + magic = be16_to_cpu(node->hdr.info.magic);
> > + switch (magic) {
> > + case XFS_ATTR_LEAF_MAGIC:
> > + case XFS_ATTR3_LEAF_MAGIC:
> > + goto found_leaf;
>
> The label after the loop suggests a break should be sufficient, but the
> switch statement conflicts with that. So why not just use the if logic
> from the original code and kill that label?
Ok.
> > + case XFS_DA_NODE_MAGIC:
> > + case XFS_DA3_NODE_MAGIC:
> > + /* process btree node below */
> > + break;
> > + default:
> > + XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
> > + node);
> > + goto out_corruptbuf;
> > + }
> > +
> > + dp->d_ops->node_hdr_from_disk(&nodehdr, node);
> > +
> > + btree = dp->d_ops->node_tree_p(node);
> > + for (i = 0; i < nodehdr.count; btree++, i++) {
> > + if (cursor->hashval <= be32_to_cpu(btree->hashval)) {
> > + cursor->blkno = be32_to_cpu(btree->before);
> > + trace_xfs_attr_list_node_descend(context,
> > + btree);
> > + break;
> > + }
> > + }
> > + if (i == nodehdr.count)
> > + goto out_buf;
>
> We can move this after the line below to pick up the brelse(). Also, if
> we use a local bp pointer and assign pbp = &bp in the one case we know
> we found something, that eliminates the need to clear pbp in the
> multiple other cases. It also means we can now just return directly from
> any checks after the brelse() in the loop.
>
> > +
> > + xfs_trans_brelse(tp, *pbp);
> > + }
> > +
> > +found_leaf:
> > + return error;
> > +
> > +out_corruptbuf:
> > + error = -EFSCORRUPTED;
>
> Might as well just return -EFSCORRUPTED. We already return directly from
> the only place error is potentially assigned anything else.
>
> > +out_buf:
> > + xfs_trans_brelse(tp, *pbp);
> > + *pbp = NULL;
> > + return error;
>
> With all of that and from taking a quick look at the next patch, I think
> we should be able to end up with something like the following logic with
> only a single label for the corrupted buf case, which is now common
> between the existing magic val corruption check and the new level
> checks.
>
> ...
> }
>
> if (expected_level != 0)
> goto out_corruptbp;
> *pbp = bp;
> return 0;
>
> out_corruptbp:
> xfs_trans_brelse(tp, bp);
> return -EFSCORRUPTED;
Ok, will do.
--D
> }
>
> Brian
>
> > +}
> > +
> > +STATIC int
> > +xfs_attr_node_list(
> > + struct xfs_attr_list_context *context)
> > +{
> > + struct xfs_attr3_icleaf_hdr leafhdr;
> > + struct attrlist_cursor_kern *cursor;
> > + struct xfs_attr_leafblock *leaf;
> > + struct xfs_da_intnode *node;
> > + struct xfs_buf *bp;
> > + struct xfs_inode *dp = context->dp;
> > + struct xfs_mount *mp = dp->i_mount;
> > + int error;
> >
> > trace_xfs_attr_node_list(context);
> >
> > @@ -277,47 +347,9 @@ xfs_attr_node_list(xfs_attr_list_context_t *context)
> > * Note that start of node block is same as start of leaf block.
> > */
> > if (bp == NULL) {
> > - cursor->blkno = 0;
> > - for (;;) {
> > - uint16_t magic;
> > -
> > - error = xfs_da3_node_read(context->tp, dp,
> > - cursor->blkno, -1, &bp,
> > - XFS_ATTR_FORK);
> > - if (error)
> > - return error;
> > - node = bp->b_addr;
> > - magic = be16_to_cpu(node->hdr.info.magic);
> > - if (magic == XFS_ATTR_LEAF_MAGIC ||
> > - magic == XFS_ATTR3_LEAF_MAGIC)
> > - break;
> > - if (magic != XFS_DA_NODE_MAGIC &&
> > - magic != XFS_DA3_NODE_MAGIC) {
> > - XFS_CORRUPTION_ERROR("xfs_attr_node_list(3)",
> > - XFS_ERRLEVEL_LOW,
> > - context->dp->i_mount,
> > - node);
> > - xfs_trans_brelse(context->tp, bp);
> > - return -EFSCORRUPTED;
> > - }
> > -
> > - dp->d_ops->node_hdr_from_disk(&nodehdr, node);
> > - btree = dp->d_ops->node_tree_p(node);
> > - for (i = 0; i < nodehdr.count; btree++, i++) {
> > - if (cursor->hashval
> > - <= be32_to_cpu(btree->hashval)) {
> > - cursor->blkno = be32_to_cpu(btree->before);
> > - trace_xfs_attr_list_node_descend(context,
> > - btree);
> > - break;
> > - }
> > - }
> > - if (i == nodehdr.count) {
> > - xfs_trans_brelse(context->tp, bp);
> > - return 0;
> > - }
> > - xfs_trans_brelse(context->tp, bp);
> > - }
> > + error = xfs_attr_node_list_lookup(context, cursor, &bp);
> > + if (error || !bp)
> > + return error;
> > }
> > ASSERT(bp != NULL);
> >
> >
> > --
> > 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
> --
> 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
--
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
