| Message ID | 20210727062053.11129-17-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Delayed Attributes | expand |
On Mon, Jul 26, 2021 at 11:20:53PM -0700, Allison Henderson wrote: > This patch adds a helper function xfs_attr_leaf_addname. While this > does help to break down xfs_attr_set_iter, it does also hoist out some > of the state management. This patch has been moved to the end of the > clean up series for further discussion. > > Suggested-by: Darrick J. Wong <djwong@kernel.org> > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > Reviewed-by: Darrick J. Wong <djwong@kernel.org> > --- > fs/xfs/libxfs/xfs_attr.c | 108 ++++++++++++++++++++++++++--------------------- > fs/xfs/xfs_trace.h | 1 + > 2 files changed, 61 insertions(+), 48 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index 811288d..acb995b 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -285,6 +285,65 @@ xfs_attr_sf_addname( > return -EAGAIN; > } > > +STATIC int > +xfs_attr_leaf_addname( > + struct xfs_attr_item *attr) > +{ > + struct xfs_da_args *args = attr->xattri_da_args; > + struct xfs_buf *leaf_bp = attr->xattri_leaf_bp; > + struct xfs_inode *dp = args->dp; > + int error; > + > + if (xfs_attr_is_leaf(dp)) { > + error = xfs_attr_leaf_try_add(args, leaf_bp); > + if (error == -ENOSPC) { > + error = xfs_attr3_leaf_to_node(args); > + if (error) > + return error; > + > + /* > + * Finish any deferred work items and roll the > + * transaction once more. The goal here is to call > + * node_addname with the inode and transaction in the > + * same state (inode locked and joined, transaction > + * clean) no matter how we got to this step. > + * > + * At this point, we are still in XFS_DAS_UNINIT, but > + * when we come back, we'll be a node, so we'll fall > + * down into the node handling code below > + */ > + trace_xfs_attr_set_iter_return( > + attr->xattri_dela_state, args->dp); > + return -EAGAIN; > + } > + > + if (error) > + return error; > + > + attr->xattri_dela_state = XFS_DAS_FOUND_LBLK; Hmm. I know I reviewed this once before, but on second thought it's a little strange to be calling the node addname function from the leaf addname function. Can you reduce the leaf addname function's scope like this: STATIC int xfs_attr_leaf_addname( struct xfs_attr_item *attr) struct xfs_da_args *args = attr->xattri_da_args; struct xfs_buf *leaf_bp = attr->xattri_leaf_bp; struct xfs_inode *dp = args->dp; int error; error = xfs_attr_leaf_try_add(args, leaf_bp); if (error == 0) { attr->xattri_dela_state = XFS_DAS_FOUND_LBLK; trace_xfs_attr_leaf_addname_return(attr->xattri_dela_state, args->dp); return -EAGAIN; } if (error != -ENOSPC) return error; /* No room in leaf; convert to node format and try again. */ error = xfs_attr3_leaf_to_node(args); if (error) return error; /* * Finish any deferred work items and roll the transaction once * more. The goal here is to call node_addname with the inode * and transaction in the same state (inode locked and joined, * transaction clean) no matter how we got to this step. * * At this point, we are still in XFS_DAS_UNINIT, but when we * come back, we'll be a node, so we'll fall down into the node * handling code below */ trace_xfs_attr_set_iter_return(attr->xattri_dela_state, args->dp); return -EAGAIN; } Then the callsite up in xfs_attr_set_iter looks like: case XFS_DAS_UNINIT: if (xfs_attr_is_shortform(dp)) return xfs_attr_sf_addname(dac, leaf_bp); if (*leaf_bp != NULL) { xfs_trans_bhold_release(args->trans, *leaf_bp); *leaf_bp = NULL; } if (xfs_attr_is_leaf(dp)) return xfs_attr_leaf_addname(...); /* node format */ error = xfs_attr_node_addname_find_attr(attr); if (error) return error; error = xfs_attr_node_addname(attr); if (error) return error; dac->dela_state = XFS_DAS_FOUND_NBLK; return -EAGAIN; case XFS_DAS_FOUND_LBLK: --D > + } else { > + error = xfs_attr_node_addname_find_attr(attr); > + if (error) > + return error; > + > + error = xfs_attr_node_addname(attr); > + if (error) > + return error; > + > + /* > + * If addname was successful, and we dont need to alloc or > + * remove anymore blks, we're done. > + */ > + if (!args->rmtblkno && !args->rmtblkno2) > + return 0; > + > + attr->xattri_dela_state = XFS_DAS_FOUND_NBLK; > + } > + > + trace_xfs_attr_leaf_addname_return(attr->xattri_dela_state, args->dp); > + return -EAGAIN; > +} > + > /* > * Set the attribute specified in @args. > * This routine is meant to function as a delayed operation, and may return > @@ -320,55 +379,8 @@ xfs_attr_set_iter( > *leaf_bp = NULL; > } > > - if (xfs_attr_is_leaf(dp)) { > - error = xfs_attr_leaf_try_add(args, *leaf_bp); > - if (error == -ENOSPC) { > - error = xfs_attr3_leaf_to_node(args); > - if (error) > - return error; > - > - /* > - * Finish any deferred work items and roll the > - * transaction once more. The goal here is to > - * call node_addname with the inode and > - * transaction in the same state (inode locked > - * and joined, transaction clean) no matter how > - * we got to this step. > - * > - * At this point, we are still in > - * XFS_DAS_UNINIT, but when we come back, we'll > - * be a node, so we'll fall down into the node > - * handling code below > - */ > - trace_xfs_attr_set_iter_return( > - attr->xattri_dela_state, args->dp); > - return -EAGAIN; > - } else if (error) { > - return error; > - } > - > - attr->xattri_dela_state = XFS_DAS_FOUND_LBLK; > - } else { > - error = xfs_attr_node_addname_find_attr(attr); > - if (error) > - return error; > + return xfs_attr_leaf_addname(attr); > > - error = xfs_attr_node_addname(attr); > - if (error) > - return error; > - > - /* > - * If addname was successful, and we dont need to alloc > - * or remove anymore blks, we're done. > - */ > - if (!args->rmtblkno && !args->rmtblkno2) > - return 0; > - > - attr->xattri_dela_state = XFS_DAS_FOUND_NBLK; > - } > - trace_xfs_attr_set_iter_return(attr->xattri_dela_state, > - args->dp); > - return -EAGAIN; > case XFS_DAS_FOUND_LBLK: > /* > * If there was an out-of-line value, allocate the blocks we > diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h > index f9840dd..cf8bd3a 100644 > --- a/fs/xfs/xfs_trace.h > +++ b/fs/xfs/xfs_trace.h > @@ -4008,6 +4008,7 @@ DEFINE_EVENT(xfs_das_state_class, name, \ > TP_ARGS(das, ip)) > DEFINE_DAS_STATE_EVENT(xfs_attr_sf_addname_return); > DEFINE_DAS_STATE_EVENT(xfs_attr_set_iter_return); > +DEFINE_DAS_STATE_EVENT(xfs_attr_leaf_addname_return); > DEFINE_DAS_STATE_EVENT(xfs_attr_node_addname_return); > DEFINE_DAS_STATE_EVENT(xfs_attr_remove_iter_return); > DEFINE_DAS_STATE_EVENT(xfs_attr_rmtval_remove_return); > -- > 2.7.4 >
On 7/28/21 12:52 PM, Darrick J. Wong wrote: > On Mon, Jul 26, 2021 at 11:20:53PM -0700, Allison Henderson wrote: >> This patch adds a helper function xfs_attr_leaf_addname. While this >> does help to break down xfs_attr_set_iter, it does also hoist out some >> of the state management. This patch has been moved to the end of the >> clean up series for further discussion. >> >> Suggested-by: Darrick J. Wong <djwong@kernel.org> >> Signed-off-by: Allison Henderson <allison.henderson@oracle.com> >> Reviewed-by: Darrick J. Wong <djwong@kernel.org> >> --- >> fs/xfs/libxfs/xfs_attr.c | 108 ++++++++++++++++++++++++++--------------------- >> fs/xfs/xfs_trace.h | 1 + >> 2 files changed, 61 insertions(+), 48 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index 811288d..acb995b 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -285,6 +285,65 @@ xfs_attr_sf_addname( >> return -EAGAIN; >> } >> >> +STATIC int >> +xfs_attr_leaf_addname( >> + struct xfs_attr_item *attr) >> +{ >> + struct xfs_da_args *args = attr->xattri_da_args; >> + struct xfs_buf *leaf_bp = attr->xattri_leaf_bp; >> + struct xfs_inode *dp = args->dp; >> + int error; >> + >> + if (xfs_attr_is_leaf(dp)) { >> + error = xfs_attr_leaf_try_add(args, leaf_bp); >> + if (error == -ENOSPC) { >> + error = xfs_attr3_leaf_to_node(args); >> + if (error) >> + return error; >> + >> + /* >> + * Finish any deferred work items and roll the >> + * transaction once more. The goal here is to call >> + * node_addname with the inode and transaction in the >> + * same state (inode locked and joined, transaction >> + * clean) no matter how we got to this step. >> + * >> + * At this point, we are still in XFS_DAS_UNINIT, but >> + * when we come back, we'll be a node, so we'll fall >> + * down into the node handling code below >> + */ >> + trace_xfs_attr_set_iter_return( >> + attr->xattri_dela_state, args->dp); >> + return -EAGAIN; >> + } >> + >> + if (error) >> + return error; >> + >> + attr->xattri_dela_state = XFS_DAS_FOUND_LBLK; > > Hmm. I know I reviewed this once before, but on second thought it's a > little strange to be calling the node addname function from the leaf > addname function. Can you reduce the leaf addname function's scope like > this: > > STATIC int > xfs_attr_leaf_addname( > struct xfs_attr_item *attr) > > struct xfs_da_args *args = attr->xattri_da_args; > struct xfs_buf *leaf_bp = attr->xattri_leaf_bp; > struct xfs_inode *dp = args->dp; > int error; > > error = xfs_attr_leaf_try_add(args, leaf_bp); > if (error == 0) { > attr->xattri_dela_state = XFS_DAS_FOUND_LBLK; > trace_xfs_attr_leaf_addname_return(attr->xattri_dela_state, > args->dp); > return -EAGAIN; > } > if (error != -ENOSPC) > return error; > > /* No room in leaf; convert to node format and try again. */ > error = xfs_attr3_leaf_to_node(args); > if (error) > return error; > > /* > * Finish any deferred work items and roll the transaction once > * more. The goal here is to call node_addname with the inode > * and transaction in the same state (inode locked and joined, > * transaction clean) no matter how we got to this step. > * > * At this point, we are still in XFS_DAS_UNINIT, but when we > * come back, we'll be a node, so we'll fall down into the node > * handling code below > */ > trace_xfs_attr_set_iter_return(attr->xattri_dela_state, > args->dp); > return -EAGAIN; > } > > Then the callsite up in xfs_attr_set_iter looks like: > > case XFS_DAS_UNINIT: > if (xfs_attr_is_shortform(dp)) > return xfs_attr_sf_addname(dac, leaf_bp); > if (*leaf_bp != NULL) { > xfs_trans_bhold_release(args->trans, *leaf_bp); > *leaf_bp = NULL; > } > > if (xfs_attr_is_leaf(dp)) > return xfs_attr_leaf_addname(...); > > /* node format */ > error = xfs_attr_node_addname_find_attr(attr); > if (error) > return error; > > error = xfs_attr_node_addname(attr); > if (error) > return error; > > dac->dela_state = XFS_DAS_FOUND_NBLK; > return -EAGAIN; > > case XFS_DAS_FOUND_LBLK: > Sure, I can pull some of that back up, should be ok. Allison > --D > >> + } else { >> + error = xfs_attr_node_addname_find_attr(attr); >> + if (error) >> + return error; >> + >> + error = xfs_attr_node_addname(attr); >> + if (error) >> + return error; >> + >> + /* >> + * If addname was successful, and we dont need to alloc or >> + * remove anymore blks, we're done. >> + */ >> + if (!args->rmtblkno && !args->rmtblkno2) >> + return 0; >> + >> + attr->xattri_dela_state = XFS_DAS_FOUND_NBLK; >> + } >> + >> + trace_xfs_attr_leaf_addname_return(attr->xattri_dela_state, args->dp); >> + return -EAGAIN; >> +} >> + >> /* >> * Set the attribute specified in @args. >> * This routine is meant to function as a delayed operation, and may return >> @@ -320,55 +379,8 @@ xfs_attr_set_iter( >> *leaf_bp = NULL; >> } >> >> - if (xfs_attr_is_leaf(dp)) { >> - error = xfs_attr_leaf_try_add(args, *leaf_bp); >> - if (error == -ENOSPC) { >> - error = xfs_attr3_leaf_to_node(args); >> - if (error) >> - return error; >> - >> - /* >> - * Finish any deferred work items and roll the >> - * transaction once more. The goal here is to >> - * call node_addname with the inode and >> - * transaction in the same state (inode locked >> - * and joined, transaction clean) no matter how >> - * we got to this step. >> - * >> - * At this point, we are still in >> - * XFS_DAS_UNINIT, but when we come back, we'll >> - * be a node, so we'll fall down into the node >> - * handling code below >> - */ >> - trace_xfs_attr_set_iter_return( >> - attr->xattri_dela_state, args->dp); >> - return -EAGAIN; >> - } else if (error) { >> - return error; >> - } >> - >> - attr->xattri_dela_state = XFS_DAS_FOUND_LBLK; >> - } else { >> - error = xfs_attr_node_addname_find_attr(attr); >> - if (error) >> - return error; >> + return xfs_attr_leaf_addname(attr); >> >> - error = xfs_attr_node_addname(attr); >> - if (error) >> - return error; >> - >> - /* >> - * If addname was successful, and we dont need to alloc >> - * or remove anymore blks, we're done. >> - */ >> - if (!args->rmtblkno && !args->rmtblkno2) >> - return 0; >> - >> - attr->xattri_dela_state = XFS_DAS_FOUND_NBLK; >> - } >> - trace_xfs_attr_set_iter_return(attr->xattri_dela_state, >> - args->dp); >> - return -EAGAIN; >> case XFS_DAS_FOUND_LBLK: >> /* >> * If there was an out-of-line value, allocate the blocks we >> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h >> index f9840dd..cf8bd3a 100644 >> --- a/fs/xfs/xfs_trace.h >> +++ b/fs/xfs/xfs_trace.h >> @@ -4008,6 +4008,7 @@ DEFINE_EVENT(xfs_das_state_class, name, \ >> TP_ARGS(das, ip)) >> DEFINE_DAS_STATE_EVENT(xfs_attr_sf_addname_return); >> DEFINE_DAS_STATE_EVENT(xfs_attr_set_iter_return); >> +DEFINE_DAS_STATE_EVENT(xfs_attr_leaf_addname_return); >> DEFINE_DAS_STATE_EVENT(xfs_attr_node_addname_return); >> DEFINE_DAS_STATE_EVENT(xfs_attr_remove_iter_return); >> DEFINE_DAS_STATE_EVENT(xfs_attr_rmtval_remove_return); >> -- >> 2.7.4 >>
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index 811288d..acb995b 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -285,6 +285,65 @@ xfs_attr_sf_addname( return -EAGAIN; } +STATIC int +xfs_attr_leaf_addname( + struct xfs_attr_item *attr) +{ + struct xfs_da_args *args = attr->xattri_da_args; + struct xfs_buf *leaf_bp = attr->xattri_leaf_bp; + struct xfs_inode *dp = args->dp; + int error; + + if (xfs_attr_is_leaf(dp)) { + error = xfs_attr_leaf_try_add(args, leaf_bp); + if (error == -ENOSPC) { + error = xfs_attr3_leaf_to_node(args); + if (error) + return error; + + /* + * Finish any deferred work items and roll the + * transaction once more. The goal here is to call + * node_addname with the inode and transaction in the + * same state (inode locked and joined, transaction + * clean) no matter how we got to this step. + * + * At this point, we are still in XFS_DAS_UNINIT, but + * when we come back, we'll be a node, so we'll fall + * down into the node handling code below + */ + trace_xfs_attr_set_iter_return( + attr->xattri_dela_state, args->dp); + return -EAGAIN; + } + + if (error) + return error; + + attr->xattri_dela_state = XFS_DAS_FOUND_LBLK; + } else { + error = xfs_attr_node_addname_find_attr(attr); + if (error) + return error; + + error = xfs_attr_node_addname(attr); + if (error) + return error; + + /* + * If addname was successful, and we dont need to alloc or + * remove anymore blks, we're done. + */ + if (!args->rmtblkno && !args->rmtblkno2) + return 0; + + attr->xattri_dela_state = XFS_DAS_FOUND_NBLK; + } + + trace_xfs_attr_leaf_addname_return(attr->xattri_dela_state, args->dp); + return -EAGAIN; +} + /* * Set the attribute specified in @args. * This routine is meant to function as a delayed operation, and may return @@ -320,55 +379,8 @@ xfs_attr_set_iter( *leaf_bp = NULL; } - if (xfs_attr_is_leaf(dp)) { - error = xfs_attr_leaf_try_add(args, *leaf_bp); - if (error == -ENOSPC) { - error = xfs_attr3_leaf_to_node(args); - if (error) - return error; - - /* - * Finish any deferred work items and roll the - * transaction once more. The goal here is to - * call node_addname with the inode and - * transaction in the same state (inode locked - * and joined, transaction clean) no matter how - * we got to this step. - * - * At this point, we are still in - * XFS_DAS_UNINIT, but when we come back, we'll - * be a node, so we'll fall down into the node - * handling code below - */ - trace_xfs_attr_set_iter_return( - attr->xattri_dela_state, args->dp); - return -EAGAIN; - } else if (error) { - return error; - } - - attr->xattri_dela_state = XFS_DAS_FOUND_LBLK; - } else { - error = xfs_attr_node_addname_find_attr(attr); - if (error) - return error; + return xfs_attr_leaf_addname(attr); - error = xfs_attr_node_addname(attr); - if (error) - return error; - - /* - * If addname was successful, and we dont need to alloc - * or remove anymore blks, we're done. - */ - if (!args->rmtblkno && !args->rmtblkno2) - return 0; - - attr->xattri_dela_state = XFS_DAS_FOUND_NBLK; - } - trace_xfs_attr_set_iter_return(attr->xattri_dela_state, - args->dp); - return -EAGAIN; case XFS_DAS_FOUND_LBLK: /* * If there was an out-of-line value, allocate the blocks we diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h index f9840dd..cf8bd3a 100644 --- a/fs/xfs/xfs_trace.h +++ b/fs/xfs/xfs_trace.h @@ -4008,6 +4008,7 @@ DEFINE_EVENT(xfs_das_state_class, name, \ TP_ARGS(das, ip)) DEFINE_DAS_STATE_EVENT(xfs_attr_sf_addname_return); DEFINE_DAS_STATE_EVENT(xfs_attr_set_iter_return); +DEFINE_DAS_STATE_EVENT(xfs_attr_leaf_addname_return); DEFINE_DAS_STATE_EVENT(xfs_attr_node_addname_return); DEFINE_DAS_STATE_EVENT(xfs_attr_remove_iter_return); DEFINE_DAS_STATE_EVENT(xfs_attr_rmtval_remove_return);