| Message ID | 20191107012801.22863-9-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: Delay Ready Attributes | expand |
On Wed, Nov 06, 2019 at 06:27:52PM -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 | 84 +++++++++++++++++++++++++++++------------------- > 1 file changed, 51 insertions(+), 33 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index 212995f..dda2eba 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -305,10 +305,33 @@ 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 > + if (error == -ENOSPC) { > + /* > + * 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; > + > + /* > + * Fob the rest of the problem off on the Btree code. > + */ > + error = xfs_attr_node_addname(args); > + } > + } else { > error = xfs_attr_node_addname(args); > + } > return error; > } > > @@ -601,21 +624,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( (total stream of consciousness here...) AFAICT the old _addname function's responsibilities were: 1 Try to add a new attr key entry to the leaf block, with INCOMPLETE set if it's a rename op or we need to set a remote value. 2 If there wasn't space in the leaf block, convert to node format, call the node version of this function, and exit. 3 Allocating blocks for the remote attr value and writing them, if applicable 4 If it's a rename operation, clearing the INCOMPLETE flag on the new entry; setting it on the old entry; and then removing the old entry. 5 Clearing the INCOMPLETE flag on the new entry when we're done writing a remote value (if applicable) I think we arrive at this split so that we don't have a transaction roll in the middle of the function, right? And also to make the "convert to node format and roll" bits go elsewhere? The way I'm thinking about how to accomplish this is... xfs_attr_leaf_addname should be renamed xfs_attr_leaf_setname, and then hoist (1) into a separate function, move (2) into xfs_attr_set_args, and hoist (4) into a separate function. ...ok, so let's test how closely my understanding fits the changes made in this patch: _try_add is basically (1). Most of (2) happened, though the call to xfs_attr3_leaf_to_node ought to go into the caller so that the conversion stays with the defer_finish and roll. (4) could still be done, maybe as a separate prep patch. Hm, ok, I think I understand what this patch does. The call site in xfs_attr_set_args would be clearer (and less indenty) if it looked like: if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) { error = xfs_attr_leaf_addname(args); if (error == 0 || error != -ENOSPC) return error; /* Promote the attribute list to node format. */ error = xfs_attr3_leaf_to_node(args); if (error) return error; /* * 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; } return xfs_attr_node_addname(args); But otherwise it looks decent, assuming I understood any of it. :) --D > + 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 > @@ -661,31 +675,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 11/8/19 1:57 PM, Darrick J. Wong wrote: > On Wed, Nov 06, 2019 at 06:27:52PM -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 | 84 +++++++++++++++++++++++++++++------------------- >> 1 file changed, 51 insertions(+), 33 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index 212995f..dda2eba 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -305,10 +305,33 @@ 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 >> + if (error == -ENOSPC) { >> + /* >> + * 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; >> + >> + /* >> + * Fob the rest of the problem off on the Btree code. >> + */ >> + error = xfs_attr_node_addname(args); >> + } >> + } else { >> error = xfs_attr_node_addname(args); >> + } >> return error; > > >> } >> >> @@ -601,21 +624,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( > > (total stream of consciousness here...) > > AFAICT the old _addname function's responsibilities were: > > 1 Try to add a new attr key entry to the leaf block, with INCOMPLETE set > if it's a rename op or we need to set a remote value. > 2 If there wasn't space in the leaf block, convert to node format, call > the node version of this function, and exit. > 3 Allocating blocks for the remote attr value and writing them, if > applicable > 4 If it's a rename operation, clearing the INCOMPLETE flag on the new > entry; setting it on the old entry; and then removing the old entry. > 5 Clearing the INCOMPLETE flag on the new entry when we're done writing > a remote value (if applicable) > > I think we arrive at this split so that we don't have a transaction roll > in the middle of the function, right? And also to make the "convert to > node format and roll" bits go elsewhere? > > The way I'm thinking about how to accomplish this is... > > xfs_attr_leaf_addname should be renamed xfs_attr_leaf_setname, and then > hoist (1) into a separate function, move (2) into xfs_attr_set_args, and > hoist (4) into a separate function. > > ...ok, so let's test how closely my understanding fits the changes made > in this patch: > > _try_add is basically (1). > > Most of (2) happened, though the call to xfs_attr3_leaf_to_node ought to > go into the caller so that the conversion stays with the defer_finish > and roll. > > (4) could still be done, maybe as a separate prep patch. > I think you're on the right track. Perhaps the diff made it look a little crazier than it seems. Maybe its easier to describe it as this: I renamed xfs_attr_leaf_addname to xfs_attr_leaf_try_add, and then deleted (2) out of the body of the function. The upper half (1) became the helper function, and lower half (3, 4, and 5) became the new xfs_attr_leaf_addname, which now calls the helper at the start of the routine. Finally (2) got factored up into the caller. Hope that helps some :-) The point of all this being to jigsaw out (1) into a stand alone routine that we can use later in delayed attrs. > Hm, ok, I think I understand what this patch does. The call site in > xfs_attr_set_args would be clearer (and less indenty) if it looked like: > > if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) { > error = xfs_attr_leaf_addname(args); > if (error == 0 || error != -ENOSPC) > return error; > > /* Promote the attribute list to node format. */ > error = xfs_attr3_leaf_to_node(args); > if (error) > return error; I think this part here is already taken care of in xfs_attr_leaf_addname, but otherwise, I think the rest of this is equivalent. Will update in the next set. Thanks for the reviews! 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; > } > > return xfs_attr_node_addname(args); > > But otherwise it looks decent, assuming I understood any of it. :) > > --D > >> + 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 >> @@ -661,31 +675,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 >>
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index 212995f..dda2eba 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -305,10 +305,33 @@ 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 + if (error == -ENOSPC) { + /* + * 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; + + /* + * Fob the rest of the problem off on the Btree code. + */ + error = xfs_attr_node_addname(args); + } + } else { error = xfs_attr_node_addname(args); + } return error; } @@ -601,21 +624,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 @@ -661,31 +675,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 | 84 +++++++++++++++++++++++++++++------------------- 1 file changed, 51 insertions(+), 33 deletions(-)