| Message ID | 20191212041513.13855-8-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: Delay Ready Attributes | expand |
On Wed, Dec 11, 2019 at 09:15:06PM -0700, Allison Collins wrote: > Factor out new helper function xfs_attr_leaf_try_add. > Because new delayed attribute routines cannot roll > transactions, we carve off the parts of > xfs_attr_leaf_addname that we can use. This will help > to reduce repetitive code later when we introduce > delayed attributes. > > Signed-off-by: Allison Collins <allison.henderson@oracle.com> > --- > fs/xfs/libxfs/xfs_attr.c | 83 ++++++++++++++++++++++++++++-------------------- > 1 file changed, 49 insertions(+), 34 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index ee973d2..36f6a43 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -305,10 +305,30 @@ xfs_attr_set_args( > } > } > > - if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) > + if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) { > error = xfs_attr_leaf_addname(args); > - else > - error = xfs_attr_node_addname(args); > + if (error == 0 || error != -ENOSPC) > + return 0; No need to check for 0 since 0 != -ENOSPC. We also probably want to return error instead of zero. With that fixed up: Reviewed-by: Brian Foster <bfoster@redhat.com> > + > + /* > + * Commit that transaction so that the node_addname() > + * call can manage its own transactions. > + */ > + error = xfs_defer_finish(&args->trans); > + if (error) > + return error; > + > + /* > + * Commit the current trans (including the inode) and > + * start a new one. > + */ > + error = xfs_trans_roll_inode(&args->trans, dp); > + if (error) > + return error; > + > + } > + > + error = xfs_attr_node_addname(args); > return error; > } > > @@ -606,21 +626,12 @@ xfs_attr_shortform_addname(xfs_da_args_t *args) > * External routines when attribute list is one block > *========================================================================*/ > > -/* > - * Add a name to the leaf attribute list structure > - * > - * 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). > - */ > STATIC int > -xfs_attr_leaf_addname( > - struct xfs_da_args *args) > +xfs_attr_leaf_try_add( > + struct xfs_da_args *args, > + struct xfs_buf *bp) > { > - struct xfs_buf *bp; > - int retval, error, forkoff; > - struct xfs_inode *dp = args->dp; > - > - trace_xfs_attr_leaf_addname(args); > + int retval, error; > > /* > * Look up the given attribute in the leaf block. Figure out if > @@ -666,31 +677,35 @@ xfs_attr_leaf_addname( > retval = xfs_attr3_leaf_add(bp, args); > if (retval == -ENOSPC) { > /* > - * Promote the attribute list to the Btree format, then > - * Commit that transaction so that the node_addname() call > - * can manage its own transactions. > + * Promote the attribute list to the Btree format. > + * Unless an error occurs, retain the -ENOSPC retval > */ > error = xfs_attr3_leaf_to_node(args); > if (error) > return error; > - error = xfs_defer_finish(&args->trans); > - if (error) > - return error; > + } > + return retval; > +} > > - /* > - * Commit the current trans (including the inode) and start > - * a new one. > - */ > - error = xfs_trans_roll_inode(&args->trans, dp); > - if (error) > - return error; > > - /* > - * Fob the whole rest of the problem off on the Btree code. > - */ > - error = xfs_attr_node_addname(args); > +/* > + * Add a name to the leaf attribute list structure > + * > + * 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). > + */ > +STATIC int > +xfs_attr_leaf_addname(struct xfs_da_args *args) > +{ > + int error, forkoff; > + struct xfs_buf *bp = NULL; > + struct xfs_inode *dp = args->dp; > + > + trace_xfs_attr_leaf_addname(args); > + > + error = xfs_attr_leaf_try_add(args, bp); > + if (error) > return error; > - } > > /* > * Commit the transaction that added the attr name so that > -- > 2.7.4 >
On 12/13/19 7:15 AM, Brian Foster wrote: > On Wed, Dec 11, 2019 at 09:15:06PM -0700, Allison Collins wrote: >> Factor out new helper function xfs_attr_leaf_try_add. >> Because new delayed attribute routines cannot roll >> transactions, we carve off the parts of >> xfs_attr_leaf_addname that we can use. This will help >> to reduce repetitive code later when we introduce >> delayed attributes. >> >> Signed-off-by: Allison Collins <allison.henderson@oracle.com> >> --- >> fs/xfs/libxfs/xfs_attr.c | 83 ++++++++++++++++++++++++++++-------------------- >> 1 file changed, 49 insertions(+), 34 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index ee973d2..36f6a43 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -305,10 +305,30 @@ xfs_attr_set_args( >> } >> } >> >> - if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) >> + if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) { >> error = xfs_attr_leaf_addname(args); >> - else >> - error = xfs_attr_node_addname(args); >> + if (error == 0 || error != -ENOSPC) >> + return 0; > > No need to check for 0 since 0 != -ENOSPC. We also probably want to > return error instead of zero. With that fixed up: > > Reviewed-by: Brian Foster <bfoster@redhat.com> Ok will fix. Thank you!! Allison > >> + >> + /* >> + * Commit that transaction so that the node_addname() >> + * call can manage its own transactions. >> + */ >> + error = xfs_defer_finish(&args->trans); >> + if (error) >> + return error; >> + >> + /* >> + * Commit the current trans (including the inode) and >> + * start a new one. >> + */ >> + error = xfs_trans_roll_inode(&args->trans, dp); >> + if (error) >> + return error; >> + >> + } >> + >> + error = xfs_attr_node_addname(args); >> return error; >> } >> >> @@ -606,21 +626,12 @@ xfs_attr_shortform_addname(xfs_da_args_t *args) >> * External routines when attribute list is one block >> *========================================================================*/ >> >> -/* >> - * Add a name to the leaf attribute list structure >> - * >> - * 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). >> - */ >> STATIC int >> -xfs_attr_leaf_addname( >> - struct xfs_da_args *args) >> +xfs_attr_leaf_try_add( >> + struct xfs_da_args *args, >> + struct xfs_buf *bp) >> { >> - struct xfs_buf *bp; >> - int retval, error, forkoff; >> - struct xfs_inode *dp = args->dp; >> - >> - trace_xfs_attr_leaf_addname(args); >> + int retval, error; >> >> /* >> * Look up the given attribute in the leaf block. Figure out if >> @@ -666,31 +677,35 @@ xfs_attr_leaf_addname( >> retval = xfs_attr3_leaf_add(bp, args); >> if (retval == -ENOSPC) { >> /* >> - * Promote the attribute list to the Btree format, then >> - * Commit that transaction so that the node_addname() call >> - * can manage its own transactions. >> + * Promote the attribute list to the Btree format. >> + * Unless an error occurs, retain the -ENOSPC retval >> */ >> error = xfs_attr3_leaf_to_node(args); >> if (error) >> return error; >> - error = xfs_defer_finish(&args->trans); >> - if (error) >> - return error; >> + } >> + return retval; >> +} >> >> - /* >> - * Commit the current trans (including the inode) and start >> - * a new one. >> - */ >> - error = xfs_trans_roll_inode(&args->trans, dp); >> - if (error) >> - return error; >> >> - /* >> - * Fob the whole rest of the problem off on the Btree code. >> - */ >> - error = xfs_attr_node_addname(args); >> +/* >> + * Add a name to the leaf attribute list structure >> + * >> + * 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). >> + */ >> +STATIC int >> +xfs_attr_leaf_addname(struct xfs_da_args *args) >> +{ >> + int error, forkoff; >> + struct xfs_buf *bp = NULL; >> + struct xfs_inode *dp = args->dp; >> + >> + trace_xfs_attr_leaf_addname(args); >> + >> + error = xfs_attr_leaf_try_add(args, bp); >> + if (error) >> return error; >> - } >> >> /* >> * Commit the transaction that added the attr name so that >> -- >> 2.7.4 >> >
On Wed, Dec 11, 2019 at 09:15:06PM -0700, Allison Collins wrote: > Factor out new helper function xfs_attr_leaf_try_add. > Because new delayed attribute routines cannot roll > transactions, we carve off the parts of > xfs_attr_leaf_addname that we can use. This will help > to reduce repetitive code later when we introduce > delayed attributes. I have a hard time relating the subject to what is happening here, maybe because the patch does too many things at once. One thing is plitting a xfs_attr_leaf_try_add from xfs_attr_leaf_addname, which seems pretty sensible, but then it also moves code from xfs_attr_node_addname into the only caller. That probably should be a separate patch with a proper description.
On 12/24/19 5:22 AM, Christoph Hellwig wrote: > On Wed, Dec 11, 2019 at 09:15:06PM -0700, Allison Collins wrote: >> Factor out new helper function xfs_attr_leaf_try_add. >> Because new delayed attribute routines cannot roll >> transactions, we carve off the parts of >> xfs_attr_leaf_addname that we can use. This will help >> to reduce repetitive code later when we introduce >> delayed attributes. > > I have a hard time relating the subject to what is happening here, > maybe because the patch does too many things at once. One thing > is plitting a xfs_attr_leaf_try_add from xfs_attr_leaf_addname, which > seems pretty sensible, but then it also moves code from > xfs_attr_node_addname into the only caller. That probably should be > a separate patch with a proper description. > Sure, maybe it might help to look at it this way though: the goal of all this refactoring is to get all the transactions up into the xfs_attr_*_args routines. Once we have them pretty much corralled, we replace them with a sort of state machine like mechanic. This produces the "return EAGAIN for new a transaction" behavior that we need for the .finish_item callback. In this case, xfs_attr_leaf_addname is a subroutine with a transaction in the middle. So we split it into two functions. Kind of like a top half and bottom half, and then the transaction moves up. While I can separate the split and the move into separate patches, it didnt really feel to me like the transaction or the node logic really fit with either of the helpers. They are supposed to be about leaves, not nodes. It's not a big deal I suppose to split up the patches, but I thought doing so creates a sort of transient patch with functions that have logic not particularly appropriate for their scope. Thoughts? Also, perhaps I need to remove the line in the commit header about reducing repetitive code. It might be misleading you (apologies). This is left over from an earlier version of the series where I tried to avoid "monster function" by having two code paths: one for inline attrs and a separate one for delayed attrs. It generated a lot of repetitive code with subtle differences though, and i think folks preferred the code paths to stay merged. The goal being to first establish what "monster function" even looks like, and then simplify it into new helpers from there. This is admittedly a very difficult series to review. People need to understand patches 1 - 12 to really understand what 13 and 14 are doing. But really, 13 and 14 are kind of driving the rest of the series. Like literally an entire patch fell out of the last set because of some changes we made in the end of the series. I know 13 and 14 are the hardest to look at, but they very much dictate what the lower patches end up doing. So I encourage people to try and focus attention there before loosing to much sanity on scaffolding. Certainty it all needs to get reviewed of course, but it may help to make more efficient use of peoples review time. :-) Hope that helps! Thanks for looking at it! Allison
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index ee973d2..36f6a43 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -305,10 +305,30 @@ xfs_attr_set_args( } } - if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) + if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) { error = xfs_attr_leaf_addname(args); - else - error = xfs_attr_node_addname(args); + if (error == 0 || error != -ENOSPC) + return 0; + + /* + * Commit that transaction so that the node_addname() + * call can manage its own transactions. + */ + error = xfs_defer_finish(&args->trans); + if (error) + return error; + + /* + * Commit the current trans (including the inode) and + * start a new one. + */ + error = xfs_trans_roll_inode(&args->trans, dp); + if (error) + return error; + + } + + error = xfs_attr_node_addname(args); return error; } @@ -606,21 +626,12 @@ xfs_attr_shortform_addname(xfs_da_args_t *args) * External routines when attribute list is one block *========================================================================*/ -/* - * Add a name to the leaf attribute list structure - * - * 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). - */ STATIC int -xfs_attr_leaf_addname( - struct xfs_da_args *args) +xfs_attr_leaf_try_add( + struct xfs_da_args *args, + struct xfs_buf *bp) { - struct xfs_buf *bp; - int retval, error, forkoff; - struct xfs_inode *dp = args->dp; - - trace_xfs_attr_leaf_addname(args); + int retval, error; /* * Look up the given attribute in the leaf block. Figure out if @@ -666,31 +677,35 @@ xfs_attr_leaf_addname( retval = xfs_attr3_leaf_add(bp, args); if (retval == -ENOSPC) { /* - * Promote the attribute list to the Btree format, then - * Commit that transaction so that the node_addname() call - * can manage its own transactions. + * Promote the attribute list to the Btree format. + * Unless an error occurs, retain the -ENOSPC retval */ error = xfs_attr3_leaf_to_node(args); if (error) return error; - error = xfs_defer_finish(&args->trans); - if (error) - return error; + } + return retval; +} - /* - * Commit the current trans (including the inode) and start - * a new one. - */ - error = xfs_trans_roll_inode(&args->trans, dp); - if (error) - return error; - /* - * Fob the whole rest of the problem off on the Btree code. - */ - error = xfs_attr_node_addname(args); +/* + * Add a name to the leaf attribute list structure + * + * 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). + */ +STATIC int +xfs_attr_leaf_addname(struct xfs_da_args *args) +{ + int error, forkoff; + struct xfs_buf *bp = NULL; + struct xfs_inode *dp = args->dp; + + trace_xfs_attr_leaf_addname(args); + + error = xfs_attr_leaf_try_add(args, bp); + if (error) return error; - } /* * Commit the transaction that added the attr name so that
Factor out new helper function xfs_attr_leaf_try_add. Because new delayed attribute routines cannot roll transactions, we carve off the parts of xfs_attr_leaf_addname that we can use. This will help to reduce repetitive code later when we introduce delayed attributes. Signed-off-by: Allison Collins <allison.henderson@oracle.com> --- fs/xfs/libxfs/xfs_attr.c | 83 ++++++++++++++++++++++++++++-------------------- 1 file changed, 49 insertions(+), 34 deletions(-)