| Message ID | 20190828042350.6062-2-david@fromorbit.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfs: allocate xattr buffer on demand | expand |
On Wed, Aug 28, 2019 at 02:23:48PM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > Shortform, leaf and remote value attr value retrieval return > different values for success. This makes it more complex to handle > actual errors xfs_attr_get() as some errors mean success and some > mean failure. Make the return values consistent for success and > failure consistent for all attribute formats. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > fs/xfs/libxfs/xfs_attr.c | 57 +++++++++++++++++++++------------ > fs/xfs/libxfs/xfs_attr_leaf.c | 15 ++++++--- > fs/xfs/libxfs/xfs_attr_remote.c | 2 ++ > fs/xfs/scrub/attr.c | 2 -- > 4 files changed, 49 insertions(+), 27 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index d48fcf11cc35..776343c4f22b 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -97,7 +97,10 @@ xfs_inode_hasattr( > * Overall external interface routines. > *========================================================================*/ > > -/* Retrieve an extended attribute and its value. Must have ilock. */ > +/* > + * Retrieve an extended attribute and its value. Must have ilock. > + * Returns 0 on successful retrieval, otherwise an error. > + */ > int > xfs_attr_get_ilocked( > struct xfs_inode *ip, > @@ -147,7 +150,7 @@ xfs_attr_get( > xfs_iunlock(ip, lock_mode); > > *valuelenp = args.valuelen; > - return error == -EEXIST ? 0 : error; > + return error; > } > > /* > @@ -768,6 +771,8 @@ xfs_attr_leaf_removename( > * > * This leaf block cannot have a "remote" value, we only call this routine > * if bmap_one_block() says there is only one block (ie: no remote blks). > + * > + * Returns 0 on successful retrieval, otherwise an error. > */ > STATIC int > xfs_attr_leaf_get(xfs_da_args_t *args) > @@ -789,10 +794,15 @@ xfs_attr_leaf_get(xfs_da_args_t *args) > } > error = xfs_attr3_leaf_getvalue(bp, args); > xfs_trans_brelse(args->trans, bp); > - if (!error && (args->rmtblkno > 0) && !(args->flags & ATTR_KERNOVAL)) { > - error = xfs_attr_rmtval_get(args); > - } > - return error; > + if (error) > + return error; > + > + /* check if we have to retrieve a remote attribute to get the value */ > + if (args->flags & ATTR_KERNOVAL) > + return 0; > + if (!args->rmtblkno) > + return 0; > + return xfs_attr_rmtval_get(args); > } > > /*======================================================================== > @@ -1268,11 +1278,13 @@ xfs_attr_refillstate(xfs_da_state_t *state) > } > > /* > - * Look up a filename in a node attribute list. > + * Retreive the attribute data from a node attribute list. "Retrieve" > * > * This routine gets called for any attribute fork that has more than one > * block, ie: both true Btree attr lists and for single-leaf-blocks with > * "remote" values taking up more blocks. > + * > + * Returns 0 on successful retrieval, otherwise an error. > */ > STATIC int > xfs_attr_node_get(xfs_da_args_t *args) > @@ -1289,29 +1301,32 @@ xfs_attr_node_get(xfs_da_args_t *args) > state->mp = args->dp->i_mount; > > /* > - * Search to see if name exists, and get back a pointer to it. > + Search to see if name exists, and get back a pointer to it. Comment damage here? Meh whatever will just fix it. Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > */ > error = xfs_da3_node_lookup_int(state, &retval); > if (error) { > retval = error; > - } else if (retval == -EEXIST) { > - blk = &state->path.blk[ state->path.active-1 ]; > - ASSERT(blk->bp != NULL); > - ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC); > - > - /* > - * Get the value, local or "remote" > - */ > - retval = xfs_attr3_leaf_getvalue(blk->bp, args); > - if (!retval && (args->rmtblkno > 0) > - && !(args->flags & ATTR_KERNOVAL)) { > - retval = xfs_attr_rmtval_get(args); > - } > + goto out_release; > } > + if (retval != -EEXIST) > + goto out_release; > + > + /* > + * Get the value, local or "remote" > + */ > + blk = &state->path.blk[state->path.active - 1]; > + retval = xfs_attr3_leaf_getvalue(blk->bp, args); > + if (retval) > + goto out_release; > + if (args->flags & ATTR_KERNOVAL) > + goto out_release; > + if (args->rmtblkno > 0) > + retval = xfs_attr_rmtval_get(args); > > /* > * If not in a transaction, we have to release all the buffers. > */ > +out_release: > for (i = 0; i < state->path.active; i++) { > xfs_trans_brelse(args->trans, state->path.blk[i].bp); > state->path.blk[i].bp = NULL; > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c > index 70eb941d02e4..d056767b5c53 100644 > --- a/fs/xfs/libxfs/xfs_attr_leaf.c > +++ b/fs/xfs/libxfs/xfs_attr_leaf.c > @@ -720,9 +720,12 @@ xfs_attr_shortform_lookup(xfs_da_args_t *args) > } > > /* > - * Look up a name in a shortform attribute list structure. > + * Retreive the attribute value and length. > + * > + * If ATTR_KERNOVAL is specified, only the length needs to be returned. > + * Unlike a lookup, we only return an error if the attribute does not > + * exist or we can't retrieve the value. > */ > -/*ARGSUSED*/ > int > xfs_attr_shortform_getvalue(xfs_da_args_t *args) > { > @@ -743,7 +746,7 @@ xfs_attr_shortform_getvalue(xfs_da_args_t *args) > continue; > if (args->flags & ATTR_KERNOVAL) { > args->valuelen = sfe->valuelen; > - return -EEXIST; > + return 0; > } > if (args->valuelen < sfe->valuelen) { > args->valuelen = sfe->valuelen; > @@ -752,7 +755,7 @@ xfs_attr_shortform_getvalue(xfs_da_args_t *args) > args->valuelen = sfe->valuelen; > memcpy(args->value, &sfe->nameval[args->namelen], > args->valuelen); > - return -EEXIST; > + return 0; > } > return -ENOATTR; > } > @@ -2350,6 +2353,10 @@ xfs_attr3_leaf_lookup_int( > /* > * Get the value associated with an attribute name from a leaf attribute > * list structure. > + * > + * If ATTR_KERNOVAL is specified, only the length needs to be returned. > + * Unlike a lookup, we only return an error if the attribute does not > + * exist or we can't retrieve the value. > */ > int > xfs_attr3_leaf_getvalue( > diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c > index 4eb30d357045..3e39b7d40f25 100644 > --- a/fs/xfs/libxfs/xfs_attr_remote.c > +++ b/fs/xfs/libxfs/xfs_attr_remote.c > @@ -358,6 +358,8 @@ xfs_attr_rmtval_copyin( > /* > * Read the value associated with an attribute from the out-of-line buffer > * that we stored it in. > + * > + * Returns 0 on successful retrieval, otherwise an error. > */ > int > xfs_attr_rmtval_get( > diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c > index 1afc58bf71dd..e9248ad4f842 100644 > --- a/fs/xfs/scrub/attr.c > +++ b/fs/xfs/scrub/attr.c > @@ -163,8 +163,6 @@ xchk_xattr_listent( > args.valuelen = valuelen; > > error = xfs_attr_get_ilocked(context->dp, &args); > - if (error == -EEXIST) > - error = 0; > if (!xchk_fblock_process_error(sx->sc, XFS_ATTR_FORK, args.blkno, > &error)) > goto fail_xref; > -- > 2.23.0.rc1 >
On Wed, Aug 28, 2019 at 02:23:48PM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > Shortform, leaf and remote value attr value retrieval return > different values for success. This makes it more complex to handle > actual errors xfs_attr_get() as some errors mean success and some > mean failure. Make the return values consistent for success and > failure consistent for all attribute formats. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > fs/xfs/libxfs/xfs_attr.c | 57 +++++++++++++++++++++------------ > fs/xfs/libxfs/xfs_attr_leaf.c | 15 ++++++--- > fs/xfs/libxfs/xfs_attr_remote.c | 2 ++ > fs/xfs/scrub/attr.c | 2 -- > 4 files changed, 49 insertions(+), 27 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index d48fcf11cc35..776343c4f22b 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -97,7 +97,10 @@ xfs_inode_hasattr( > * Overall external interface routines. > *========================================================================*/ > > -/* Retrieve an extended attribute and its value. Must have ilock. */ > +/* > + * Retrieve an extended attribute and its value. Must have ilock. > + * Returns 0 on successful retrieval, otherwise an error. > + */ > int > xfs_attr_get_ilocked( > struct xfs_inode *ip, > @@ -147,7 +150,7 @@ xfs_attr_get( > xfs_iunlock(ip, lock_mode); > > *valuelenp = args.valuelen; > - return error == -EEXIST ? 0 : error; > + return error; > } > > /* > @@ -768,6 +771,8 @@ xfs_attr_leaf_removename( > * > * This leaf block cannot have a "remote" value, we only call this routine > * if bmap_one_block() says there is only one block (ie: no remote blks). > + * > + * Returns 0 on successful retrieval, otherwise an error. > */ > STATIC int > xfs_attr_leaf_get(xfs_da_args_t *args) > @@ -789,10 +794,15 @@ xfs_attr_leaf_get(xfs_da_args_t *args) > } > error = xfs_attr3_leaf_getvalue(bp, args); > xfs_trans_brelse(args->trans, bp); > - if (!error && (args->rmtblkno > 0) && !(args->flags & ATTR_KERNOVAL)) { > - error = xfs_attr_rmtval_get(args); > - } > - return error; > + if (error) > + return error; > + > + /* check if we have to retrieve a remote attribute to get the value */ > + if (args->flags & ATTR_KERNOVAL) > + return 0; > + if (!args->rmtblkno) > + return 0; > + return xfs_attr_rmtval_get(args); > } > > /*======================================================================== > @@ -1268,11 +1278,13 @@ xfs_attr_refillstate(xfs_da_state_t *state) > } > > /* > - * Look up a filename in a node attribute list. > + * Retreive the attribute data from a node attribute list. > * > * This routine gets called for any attribute fork that has more than one > * block, ie: both true Btree attr lists and for single-leaf-blocks with > * "remote" values taking up more blocks. > + * > + * Returns 0 on successful retrieval, otherwise an error. > */ > STATIC int > xfs_attr_node_get(xfs_da_args_t *args) > @@ -1289,29 +1301,32 @@ xfs_attr_node_get(xfs_da_args_t *args) > state->mp = args->dp->i_mount; > > /* > - * Search to see if name exists, and get back a pointer to it. > + Search to see if name exists, and get back a pointer to it. > */ > error = xfs_da3_node_lookup_int(state, &retval); > if (error) { > retval = error; Given that you are cleaning up this mess, can you check if there is any point in the weird xfs_da3_node_lookup_int calling conventions? It looks like it can return errnos in both the return value and *revtval, and from a quick check it seems like all callers treat them more or less the same.
On Thu, Aug 29, 2019 at 12:41:39AM -0700, Christoph Hellwig wrote: > On Wed, Aug 28, 2019 at 02:23:48PM +1000, Dave Chinner wrote: > > @@ -1289,29 +1301,32 @@ xfs_attr_node_get(xfs_da_args_t *args) > > state->mp = args->dp->i_mount; > > > > /* > > - * Search to see if name exists, and get back a pointer to it. > > + Search to see if name exists, and get back a pointer to it. > > */ > > error = xfs_da3_node_lookup_int(state, &retval); > > if (error) { > > retval = error; > > Given that you are cleaning up this mess, can you check if there > is any point in the weird xfs_da3_node_lookup_int calling conventions? retval propagates down into child functions like xfs_da3_path_shift(), xfs_dir2_leafn_lookup_int(), etc, so untangling that mess is non-trivial. > It looks like it can return errnos in both the return value and > *revtval, and from a quick check it seems like all callers treat them > more or less the same. Maybe so, but I don't have the time to do a deep dive into both the directory and the attribute code to determine what such a cleanup might look like. I think it's way out of scope for the problem being solved by this patchset... Cheers, Dave.
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index d48fcf11cc35..776343c4f22b 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -97,7 +97,10 @@ xfs_inode_hasattr( * Overall external interface routines. *========================================================================*/ -/* Retrieve an extended attribute and its value. Must have ilock. */ +/* + * Retrieve an extended attribute and its value. Must have ilock. + * Returns 0 on successful retrieval, otherwise an error. + */ int xfs_attr_get_ilocked( struct xfs_inode *ip, @@ -147,7 +150,7 @@ xfs_attr_get( xfs_iunlock(ip, lock_mode); *valuelenp = args.valuelen; - return error == -EEXIST ? 0 : error; + return error; } /* @@ -768,6 +771,8 @@ xfs_attr_leaf_removename( * * This leaf block cannot have a "remote" value, we only call this routine * if bmap_one_block() says there is only one block (ie: no remote blks). + * + * Returns 0 on successful retrieval, otherwise an error. */ STATIC int xfs_attr_leaf_get(xfs_da_args_t *args) @@ -789,10 +794,15 @@ xfs_attr_leaf_get(xfs_da_args_t *args) } error = xfs_attr3_leaf_getvalue(bp, args); xfs_trans_brelse(args->trans, bp); - if (!error && (args->rmtblkno > 0) && !(args->flags & ATTR_KERNOVAL)) { - error = xfs_attr_rmtval_get(args); - } - return error; + if (error) + return error; + + /* check if we have to retrieve a remote attribute to get the value */ + if (args->flags & ATTR_KERNOVAL) + return 0; + if (!args->rmtblkno) + return 0; + return xfs_attr_rmtval_get(args); } /*======================================================================== @@ -1268,11 +1278,13 @@ xfs_attr_refillstate(xfs_da_state_t *state) } /* - * Look up a filename in a node attribute list. + * Retreive the attribute data from a node attribute list. * * This routine gets called for any attribute fork that has more than one * block, ie: both true Btree attr lists and for single-leaf-blocks with * "remote" values taking up more blocks. + * + * Returns 0 on successful retrieval, otherwise an error. */ STATIC int xfs_attr_node_get(xfs_da_args_t *args) @@ -1289,29 +1301,32 @@ xfs_attr_node_get(xfs_da_args_t *args) state->mp = args->dp->i_mount; /* - * Search to see if name exists, and get back a pointer to it. + Search to see if name exists, and get back a pointer to it. */ error = xfs_da3_node_lookup_int(state, &retval); if (error) { retval = error; - } else if (retval == -EEXIST) { - blk = &state->path.blk[ state->path.active-1 ]; - ASSERT(blk->bp != NULL); - ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC); - - /* - * Get the value, local or "remote" - */ - retval = xfs_attr3_leaf_getvalue(blk->bp, args); - if (!retval && (args->rmtblkno > 0) - && !(args->flags & ATTR_KERNOVAL)) { - retval = xfs_attr_rmtval_get(args); - } + goto out_release; } + if (retval != -EEXIST) + goto out_release; + + /* + * Get the value, local or "remote" + */ + blk = &state->path.blk[state->path.active - 1]; + retval = xfs_attr3_leaf_getvalue(blk->bp, args); + if (retval) + goto out_release; + if (args->flags & ATTR_KERNOVAL) + goto out_release; + if (args->rmtblkno > 0) + retval = xfs_attr_rmtval_get(args); /* * If not in a transaction, we have to release all the buffers. */ +out_release: for (i = 0; i < state->path.active; i++) { xfs_trans_brelse(args->trans, state->path.blk[i].bp); state->path.blk[i].bp = NULL; diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c index 70eb941d02e4..d056767b5c53 100644 --- a/fs/xfs/libxfs/xfs_attr_leaf.c +++ b/fs/xfs/libxfs/xfs_attr_leaf.c @@ -720,9 +720,12 @@ xfs_attr_shortform_lookup(xfs_da_args_t *args) } /* - * Look up a name in a shortform attribute list structure. + * Retreive the attribute value and length. + * + * If ATTR_KERNOVAL is specified, only the length needs to be returned. + * Unlike a lookup, we only return an error if the attribute does not + * exist or we can't retrieve the value. */ -/*ARGSUSED*/ int xfs_attr_shortform_getvalue(xfs_da_args_t *args) { @@ -743,7 +746,7 @@ xfs_attr_shortform_getvalue(xfs_da_args_t *args) continue; if (args->flags & ATTR_KERNOVAL) { args->valuelen = sfe->valuelen; - return -EEXIST; + return 0; } if (args->valuelen < sfe->valuelen) { args->valuelen = sfe->valuelen; @@ -752,7 +755,7 @@ xfs_attr_shortform_getvalue(xfs_da_args_t *args) args->valuelen = sfe->valuelen; memcpy(args->value, &sfe->nameval[args->namelen], args->valuelen); - return -EEXIST; + return 0; } return -ENOATTR; } @@ -2350,6 +2353,10 @@ xfs_attr3_leaf_lookup_int( /* * Get the value associated with an attribute name from a leaf attribute * list structure. + * + * If ATTR_KERNOVAL is specified, only the length needs to be returned. + * Unlike a lookup, we only return an error if the attribute does not + * exist or we can't retrieve the value. */ int xfs_attr3_leaf_getvalue( diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c index 4eb30d357045..3e39b7d40f25 100644 --- a/fs/xfs/libxfs/xfs_attr_remote.c +++ b/fs/xfs/libxfs/xfs_attr_remote.c @@ -358,6 +358,8 @@ xfs_attr_rmtval_copyin( /* * Read the value associated with an attribute from the out-of-line buffer * that we stored it in. + * + * Returns 0 on successful retrieval, otherwise an error. */ int xfs_attr_rmtval_get( diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c index 1afc58bf71dd..e9248ad4f842 100644 --- a/fs/xfs/scrub/attr.c +++ b/fs/xfs/scrub/attr.c @@ -163,8 +163,6 @@ xchk_xattr_listent( args.valuelen = valuelen; error = xfs_attr_get_ilocked(context->dp, &args); - if (error == -EEXIST) - error = 0; if (!xchk_fblock_process_error(sx->sc, XFS_ATTR_FORK, args.blkno, &error)) goto fail_xref;