| Message ID | 150899696463.18095.9642473908739425678.stgit@magnolia (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Wed, Oct 25, 2017 at 10:49:24PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > 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@oracle.com> > --- > 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? > + 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; } 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@vger.kernel.org > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@oracle.com> > > > > 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@oracle.com> > > --- > > 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@vger.kernel.org > > 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@vger.kernel.org > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) { - 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; + 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; + + xfs_trans_brelse(tp, *pbp); + } + +found_leaf: + return error; + +out_corruptbuf: + error = -EFSCORRUPTED; +out_buf: + xfs_trans_brelse(tp, *pbp); + *pbp = NULL; + return error; +} + +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);