| Message ID | 20200430225016.4287-6-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: Delay Ready Attributes | expand |
On Thu, Apr 30, 2020 at 03:49:57PM -0700, Allison Collins wrote: > Split out new helper function xfs_attr_leaf_try_add from > xfs_attr_leaf_addname. Because new delayed attribute routines cannot > roll transactions, we split off the parts of xfs_attr_leaf_addname that > we can use, and move the commit into the calling function. > > Signed-off-by: Allison Collins <allison.henderson@oracle.com> > Reviewed-by: Brian Foster <bfoster@redhat.com> > Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com> <groan> I just replied to the xfsprogs version of this patch by accident. :( Modulo the comments I made in that reply, the rest of the patch looks ok. --D > --- > fs/xfs/libxfs/xfs_attr.c | 94 ++++++++++++++++++++++++++++++------------------ > 1 file changed, 60 insertions(+), 34 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index dcbc475..b3b21a7 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -256,10 +256,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 != -ENOSPC) > + 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; > + > + } > + > + error = xfs_attr_node_addname(args); > return error; > } > > @@ -507,20 +527,21 @@ xfs_attr_shortform_addname(xfs_da_args_t *args) > *========================================================================*/ > > /* > - * Add a name to the leaf attribute list structure > + * Tries to add an attribute to an inode in leaf form > * > - * 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). > + * This function is meant to execute as part of a delayed operation and leaves > + * the transaction handling to the caller. On success the attribute is added > + * and the inode and transaction are left dirty. If there is not enough space, > + * the attr data is converted to node format and -ENOSPC is returned. Caller is > + * responsible for handling the dirty inode and transaction or adding the attr > + * in node format. > */ > 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 > @@ -562,31 +583,39 @@ 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; > +out_brelse: > + xfs_trans_brelse(args->trans, bp); > + 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 > @@ -681,9 +710,6 @@ xfs_attr_leaf_addname( > error = xfs_attr3_leaf_clearflag(args); > } > return error; > -out_brelse: > - xfs_trans_brelse(args->trans, bp); > - return retval; > } > > /* > -- > 2.7.4 >
On 5/4/20 10:33 AM, Darrick J. Wong wrote: > On Thu, Apr 30, 2020 at 03:49:57PM -0700, Allison Collins wrote: >> Split out new helper function xfs_attr_leaf_try_add from >> xfs_attr_leaf_addname. Because new delayed attribute routines cannot >> roll transactions, we split off the parts of xfs_attr_leaf_addname that >> we can use, and move the commit into the calling function. >> >> Signed-off-by: Allison Collins <allison.henderson@oracle.com> >> Reviewed-by: Brian Foster <bfoster@redhat.com> >> Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com> > > <groan> I just replied to the xfsprogs version of this patch by accident. :( > > Modulo the comments I made in that reply, the rest of the patch looks > ok. > > --D No worries, will apply same feed back here :-) Allison > >> --- >> fs/xfs/libxfs/xfs_attr.c | 94 ++++++++++++++++++++++++++++++------------------ >> 1 file changed, 60 insertions(+), 34 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index dcbc475..b3b21a7 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -256,10 +256,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 != -ENOSPC) >> + 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; >> + >> + } >> + >> + error = xfs_attr_node_addname(args); >> return error; >> } >> >> @@ -507,20 +527,21 @@ xfs_attr_shortform_addname(xfs_da_args_t *args) >> *========================================================================*/ >> >> /* >> - * Add a name to the leaf attribute list structure >> + * Tries to add an attribute to an inode in leaf form >> * >> - * 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). >> + * This function is meant to execute as part of a delayed operation and leaves >> + * the transaction handling to the caller. On success the attribute is added >> + * and the inode and transaction are left dirty. If there is not enough space, >> + * the attr data is converted to node format and -ENOSPC is returned. Caller is >> + * responsible for handling the dirty inode and transaction or adding the attr >> + * in node format. >> */ >> 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 >> @@ -562,31 +583,39 @@ 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; >> +out_brelse: >> + xfs_trans_brelse(args->trans, bp); >> + 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 >> @@ -681,9 +710,6 @@ xfs_attr_leaf_addname( >> error = xfs_attr3_leaf_clearflag(args); >> } >> return error; >> -out_brelse: >> - xfs_trans_brelse(args->trans, bp); >> - return retval; >> } >> >> /* >> -- >> 2.7.4 >>
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index dcbc475..b3b21a7 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -256,10 +256,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 != -ENOSPC) + 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; + + } + + error = xfs_attr_node_addname(args); return error; } @@ -507,20 +527,21 @@ xfs_attr_shortform_addname(xfs_da_args_t *args) *========================================================================*/ /* - * Add a name to the leaf attribute list structure + * Tries to add an attribute to an inode in leaf form * - * 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). + * This function is meant to execute as part of a delayed operation and leaves + * the transaction handling to the caller. On success the attribute is added + * and the inode and transaction are left dirty. If there is not enough space, + * the attr data is converted to node format and -ENOSPC is returned. Caller is + * responsible for handling the dirty inode and transaction or adding the attr + * in node format. */ 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 @@ -562,31 +583,39 @@ 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; +out_brelse: + xfs_trans_brelse(args->trans, bp); + 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 @@ -681,9 +710,6 @@ xfs_attr_leaf_addname( error = xfs_attr3_leaf_clearflag(args); } return error; -out_brelse: - xfs_trans_brelse(args->trans, bp); - return retval; } /*