| Message ID | 20210218165348.4754-10-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: Delayed Attributes | expand |
On Thu, Feb 18, 2021 at 09:53:35AM -0700, Allison Henderson wrote: > This patch hoists xfs_attr_leaf_addname into the calling function. The > goal being to get all the code that will require state management into > the same scope. This isn't particuarly asetheic right away, but it is a > preliminary step to to manageing the state machine code. > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > --- > fs/xfs/libxfs/xfs_attr.c | 209 ++++++++++++++++++++++------------------------- > 1 file changed, 96 insertions(+), 113 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index 19a532a..bfd4466 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c ... > @@ -286,10 +287,101 @@ xfs_attr_set_args( ... > return error; > > + xfs_attr3_leaf_remove(bp, args); > + > + /* > + * If the result is small enough, shrink it all into the inode. > + */ > + forkoff = xfs_attr_shortform_allfit(bp, dp); > + if (forkoff) > + error = xfs_attr3_leaf_to_shortform(bp, args, forkoff); > + /* bp is gone due to xfs_da_shrink_inode */ > + > + return error; > +node: Hmm, I'm not a fan of this unconditional return followed by a jump label in the middle of the function. It's a pretty clear indication that this is just two functions smashed together, so I'm not sure what the advantage of this is. I'll continue on to see what falls out of the next patches.. Brian > /* > * Promote the attribute list to the Btree format. > */ > @@ -731,115 +823,6 @@ xfs_attr_leaf_try_add( > return retval; > } > > - > -/* > - * 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 > - * later routines can manage their own transactions. > - */ > - error = xfs_trans_roll_inode(&args->trans, dp); > - if (error) > - return error; > - > - /* > - * If there was an out-of-line value, allocate the blocks we > - * identified for its storage and copy the value. This is done > - * after we create the attribute so that we don't overflow the > - * maximum size of a transaction and/or hit a deadlock. > - */ > - if (args->rmtblkno > 0) { > - error = xfs_attr_rmtval_set(args); > - if (error) > - return error; > - } > - > - if (!(args->op_flags & XFS_DA_OP_RENAME)) { > - /* > - * Added a "remote" value, just clear the incomplete flag. > - */ > - if (args->rmtblkno > 0) > - error = xfs_attr3_leaf_clearflag(args); > - > - return error; > - } > - > - /* > - * If this is an atomic rename operation, we must "flip" the incomplete > - * flags on the "new" and "old" attribute/value pairs so that one > - * disappears and one appears atomically. Then we must remove the "old" > - * attribute/value pair. > - * > - * In a separate transaction, set the incomplete flag on the "old" attr > - * and clear the incomplete flag on the "new" attr. > - */ > - > - error = xfs_attr3_leaf_flipflags(args); > - if (error) > - return error; > - /* > - * Commit the flag value change and start the next trans in series. > - */ > - error = xfs_trans_roll_inode(&args->trans, args->dp); > - if (error) > - return error; > - > - /* > - * Dismantle the "old" attribute/value pair by removing a "remote" value > - * (if it exists). > - */ > - xfs_attr_restore_rmt_blk(args); > - > - if (args->rmtblkno) { > - error = xfs_attr_rmtval_invalidate(args); > - if (error) > - return error; > - > - error = xfs_attr_rmtval_remove(args); > - if (error) > - return error; > - } > - > - /* > - * Read in the block containing the "old" attr, then remove the "old" > - * attr from that block (neat, huh!) > - */ > - error = xfs_attr3_leaf_read(args->trans, args->dp, args->blkno, > - &bp); > - if (error) > - return error; > - > - xfs_attr3_leaf_remove(bp, args); > - > - /* > - * If the result is small enough, shrink it all into the inode. > - */ > - forkoff = xfs_attr_shortform_allfit(bp, dp); > - if (forkoff) > - error = xfs_attr3_leaf_to_shortform(bp, args, forkoff); > - /* bp is gone due to xfs_da_shrink_inode */ > - > - return error; > -} > - > /* > * Return EEXIST if attr is found, or ENOATTR if not > */ > -- > 2.7.4 >
On 2/24/21 11:42 AM, Brian Foster wrote: > On Thu, Feb 18, 2021 at 09:53:35AM -0700, Allison Henderson wrote: >> This patch hoists xfs_attr_leaf_addname into the calling function. The >> goal being to get all the code that will require state management into >> the same scope. This isn't particuarly asetheic right away, but it is a >> preliminary step to to manageing the state machine code. >> >> Signed-off-by: Allison Henderson <allison.henderson@oracle.com> >> --- >> fs/xfs/libxfs/xfs_attr.c | 209 ++++++++++++++++++++++------------------------- >> 1 file changed, 96 insertions(+), 113 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index 19a532a..bfd4466 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c > ... >> @@ -286,10 +287,101 @@ xfs_attr_set_args( > ... >> return error; >> >> + xfs_attr3_leaf_remove(bp, args); >> + >> + /* >> + * If the result is small enough, shrink it all into the inode. >> + */ >> + forkoff = xfs_attr_shortform_allfit(bp, dp); >> + if (forkoff) >> + error = xfs_attr3_leaf_to_shortform(bp, args, forkoff); >> + /* bp is gone due to xfs_da_shrink_inode */ >> + >> + return error; >> +node: > > Hmm, I'm not a fan of this unconditional return followed by a jump label > in the middle of the function. It's a pretty clear indication that this > is just two functions smashed together, so I'm not sure what the > advantage of this is. I'll continue on to see what falls out of the next > patches.. > > Brian Yes, it does kinda look a little displaced, but the point of it is to bring code that will require state management into the same scope so that the state switch can span all the operations it affects. Which seemed to be what the RFC was striving for? Looking ahead at the other reviews, I think it came together for you? Allison > >> /* >> * Promote the attribute list to the Btree format. >> */ >> @@ -731,115 +823,6 @@ xfs_attr_leaf_try_add( >> return retval; >> } >> >> - >> -/* >> - * 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 >> - * later routines can manage their own transactions. >> - */ >> - error = xfs_trans_roll_inode(&args->trans, dp); >> - if (error) >> - return error; >> - >> - /* >> - * If there was an out-of-line value, allocate the blocks we >> - * identified for its storage and copy the value. This is done >> - * after we create the attribute so that we don't overflow the >> - * maximum size of a transaction and/or hit a deadlock. >> - */ >> - if (args->rmtblkno > 0) { >> - error = xfs_attr_rmtval_set(args); >> - if (error) >> - return error; >> - } >> - >> - if (!(args->op_flags & XFS_DA_OP_RENAME)) { >> - /* >> - * Added a "remote" value, just clear the incomplete flag. >> - */ >> - if (args->rmtblkno > 0) >> - error = xfs_attr3_leaf_clearflag(args); >> - >> - return error; >> - } >> - >> - /* >> - * If this is an atomic rename operation, we must "flip" the incomplete >> - * flags on the "new" and "old" attribute/value pairs so that one >> - * disappears and one appears atomically. Then we must remove the "old" >> - * attribute/value pair. >> - * >> - * In a separate transaction, set the incomplete flag on the "old" attr >> - * and clear the incomplete flag on the "new" attr. >> - */ >> - >> - error = xfs_attr3_leaf_flipflags(args); >> - if (error) >> - return error; >> - /* >> - * Commit the flag value change and start the next trans in series. >> - */ >> - error = xfs_trans_roll_inode(&args->trans, args->dp); >> - if (error) >> - return error; >> - >> - /* >> - * Dismantle the "old" attribute/value pair by removing a "remote" value >> - * (if it exists). >> - */ >> - xfs_attr_restore_rmt_blk(args); >> - >> - if (args->rmtblkno) { >> - error = xfs_attr_rmtval_invalidate(args); >> - if (error) >> - return error; >> - >> - error = xfs_attr_rmtval_remove(args); >> - if (error) >> - return error; >> - } >> - >> - /* >> - * Read in the block containing the "old" attr, then remove the "old" >> - * attr from that block (neat, huh!) >> - */ >> - error = xfs_attr3_leaf_read(args->trans, args->dp, args->blkno, >> - &bp); >> - if (error) >> - return error; >> - >> - xfs_attr3_leaf_remove(bp, args); >> - >> - /* >> - * If the result is small enough, shrink it all into the inode. >> - */ >> - forkoff = xfs_attr_shortform_allfit(bp, dp); >> - if (forkoff) >> - error = xfs_attr3_leaf_to_shortform(bp, args, forkoff); >> - /* bp is gone due to xfs_da_shrink_inode */ >> - >> - return error; >> -} >> - >> /* >> * Return EEXIST if attr is found, or ENOATTR if not >> */ >> -- >> 2.7.4 >> >
On Wed, Feb 24, 2021 at 11:19:41PM -0700, Allison Henderson wrote: > > > On 2/24/21 11:42 AM, Brian Foster wrote: > > On Thu, Feb 18, 2021 at 09:53:35AM -0700, Allison Henderson wrote: > > > This patch hoists xfs_attr_leaf_addname into the calling function. The > > > goal being to get all the code that will require state management into > > > the same scope. This isn't particuarly asetheic right away, but it is a > > > preliminary step to to manageing the state machine code. > > > > > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > > > --- > > > fs/xfs/libxfs/xfs_attr.c | 209 ++++++++++++++++++++++------------------------- > > > 1 file changed, 96 insertions(+), 113 deletions(-) > > > > > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > > > index 19a532a..bfd4466 100644 > > > --- a/fs/xfs/libxfs/xfs_attr.c > > > +++ b/fs/xfs/libxfs/xfs_attr.c > > ... > > > @@ -286,10 +287,101 @@ xfs_attr_set_args( > > ... > > > return error; > > > + xfs_attr3_leaf_remove(bp, args); > > > + > > > + /* > > > + * If the result is small enough, shrink it all into the inode. > > > + */ > > > + forkoff = xfs_attr_shortform_allfit(bp, dp); > > > + if (forkoff) > > > + error = xfs_attr3_leaf_to_shortform(bp, args, forkoff); > > > + /* bp is gone due to xfs_da_shrink_inode */ > > > + > > > + return error; > > > +node: > > > > Hmm, I'm not a fan of this unconditional return followed by a jump label > > in the middle of the function. It's a pretty clear indication that this > > is just two functions smashed together, so I'm not sure what the > > advantage of this is. I'll continue on to see what falls out of the next > > patches.. > > > > Brian > > Yes, it does kinda look a little displaced, but the point of it is to bring > code that will require state management into the same scope so that the > state switch can span all the operations it affects. Which seemed to be > what the RFC was striving for? Looking ahead at the other reviews, I think > it came together for you? > Yes, I think so. If the subsequent patches line everything up nicely in that single switch statement in one place (which it looks like we're pretty close to), then I can probably live with this transient quirk. That said, I might wait until at least the attr set path looks pretty much finalized and come back and take a last look at this one, if nothing else just to think about if there's any simple/unintrusive way to avoid it.. Brian > Allison > > > > > > /* > > > * Promote the attribute list to the Btree format. > > > */ > > > @@ -731,115 +823,6 @@ xfs_attr_leaf_try_add( > > > return retval; > > > } > > > - > > > -/* > > > - * 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 > > > - * later routines can manage their own transactions. > > > - */ > > > - error = xfs_trans_roll_inode(&args->trans, dp); > > > - if (error) > > > - return error; > > > - > > > - /* > > > - * If there was an out-of-line value, allocate the blocks we > > > - * identified for its storage and copy the value. This is done > > > - * after we create the attribute so that we don't overflow the > > > - * maximum size of a transaction and/or hit a deadlock. > > > - */ > > > - if (args->rmtblkno > 0) { > > > - error = xfs_attr_rmtval_set(args); > > > - if (error) > > > - return error; > > > - } > > > - > > > - if (!(args->op_flags & XFS_DA_OP_RENAME)) { > > > - /* > > > - * Added a "remote" value, just clear the incomplete flag. > > > - */ > > > - if (args->rmtblkno > 0) > > > - error = xfs_attr3_leaf_clearflag(args); > > > - > > > - return error; > > > - } > > > - > > > - /* > > > - * If this is an atomic rename operation, we must "flip" the incomplete > > > - * flags on the "new" and "old" attribute/value pairs so that one > > > - * disappears and one appears atomically. Then we must remove the "old" > > > - * attribute/value pair. > > > - * > > > - * In a separate transaction, set the incomplete flag on the "old" attr > > > - * and clear the incomplete flag on the "new" attr. > > > - */ > > > - > > > - error = xfs_attr3_leaf_flipflags(args); > > > - if (error) > > > - return error; > > > - /* > > > - * Commit the flag value change and start the next trans in series. > > > - */ > > > - error = xfs_trans_roll_inode(&args->trans, args->dp); > > > - if (error) > > > - return error; > > > - > > > - /* > > > - * Dismantle the "old" attribute/value pair by removing a "remote" value > > > - * (if it exists). > > > - */ > > > - xfs_attr_restore_rmt_blk(args); > > > - > > > - if (args->rmtblkno) { > > > - error = xfs_attr_rmtval_invalidate(args); > > > - if (error) > > > - return error; > > > - > > > - error = xfs_attr_rmtval_remove(args); > > > - if (error) > > > - return error; > > > - } > > > - > > > - /* > > > - * Read in the block containing the "old" attr, then remove the "old" > > > - * attr from that block (neat, huh!) > > > - */ > > > - error = xfs_attr3_leaf_read(args->trans, args->dp, args->blkno, > > > - &bp); > > > - if (error) > > > - return error; > > > - > > > - xfs_attr3_leaf_remove(bp, args); > > > - > > > - /* > > > - * If the result is small enough, shrink it all into the inode. > > > - */ > > > - forkoff = xfs_attr_shortform_allfit(bp, dp); > > > - if (forkoff) > > > - error = xfs_attr3_leaf_to_shortform(bp, args, forkoff); > > > - /* bp is gone due to xfs_da_shrink_inode */ > > > - > > > - return error; > > > -} > > > - > > > /* > > > * Return EEXIST if attr is found, or ENOATTR if not > > > */ > > > -- > > > 2.7.4 > > > > > >
On Thu, Feb 18, 2021 at 09:53:35AM -0700, Allison Henderson wrote: > This patch hoists xfs_attr_leaf_addname into the calling function. The > goal being to get all the code that will require state management into > the same scope. This isn't particuarly asetheic right away, but it is a "aesthetic" > preliminary step to to manageing the state machine code. "to merging in" ? The goto label is ugly, but afaict this patch moves code and the next one rearranges it the way you ultimately want it, right? With spelling fixed, Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > --- > fs/xfs/libxfs/xfs_attr.c | 209 ++++++++++++++++++++++------------------------- > 1 file changed, 96 insertions(+), 113 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index 19a532a..bfd4466 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -44,9 +44,9 @@ STATIC int xfs_attr_shortform_addname(xfs_da_args_t *args); > * Internal routines when attribute list is one block. > */ > STATIC int xfs_attr_leaf_get(xfs_da_args_t *args); > -STATIC int xfs_attr_leaf_addname(xfs_da_args_t *args); > STATIC int xfs_attr_leaf_removename(xfs_da_args_t *args); > STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp); > +STATIC int xfs_attr_leaf_try_add(struct xfs_da_args *args, struct xfs_buf *bp); > > /* > * Internal routines when attribute list is more than one block. > @@ -269,8 +269,9 @@ xfs_attr_set_args( > struct xfs_da_args *args) > { > struct xfs_inode *dp = args->dp; > + struct xfs_buf *bp = NULL; > struct xfs_da_state *state = NULL; > - int error = 0; > + int forkoff, error = 0; > int retval = 0; > > /* > @@ -286,10 +287,101 @@ xfs_attr_set_args( > } > > if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) { > - error = xfs_attr_leaf_addname(args); > - if (error != -ENOSPC) > + error = xfs_attr_leaf_try_add(args, bp); > + if (error == -ENOSPC) > + goto node; > + else if (error) > + return error; > + > + /* > + * Commit the transaction that added the attr name so that > + * later routines can manage their own transactions. > + */ > + error = xfs_trans_roll_inode(&args->trans, dp); > + if (error) > + return error; > + > + /* > + * If there was an out-of-line value, allocate the blocks we > + * identified for its storage and copy the value. This is done > + * after we create the attribute so that we don't overflow the > + * maximum size of a transaction and/or hit a deadlock. > + */ > + if (args->rmtblkno > 0) { > + error = xfs_attr_rmtval_set(args); > + if (error) > + return error; > + } > + > + if (!(args->op_flags & XFS_DA_OP_RENAME)) { > + /* > + * Added a "remote" value, just clear the incomplete > + *flag. > + */ > + if (args->rmtblkno > 0) > + error = xfs_attr3_leaf_clearflag(args); > + > + return error; > + } > + > + /* > + * If this is an atomic rename operation, we must "flip" the > + * incomplete flags on the "new" and "old" attribute/value pairs > + * so that one disappears and one appears atomically. Then we > + * must remove the "old" attribute/value pair. > + * > + * In a separate transaction, set the incomplete flag on the > + * "old" attr and clear the incomplete flag on the "new" attr. > + */ > + > + error = xfs_attr3_leaf_flipflags(args); > + if (error) > + return error; > + /* > + * Commit the flag value change and start the next trans in > + * series. > + */ > + error = xfs_trans_roll_inode(&args->trans, args->dp); > + if (error) > + return error; > + > + /* > + * Dismantle the "old" attribute/value pair by removing a > + * "remote" value (if it exists). > + */ > + xfs_attr_restore_rmt_blk(args); > + > + if (args->rmtblkno) { > + error = xfs_attr_rmtval_invalidate(args); > + if (error) > + return error; > + > + error = xfs_attr_rmtval_remove(args); > + if (error) > + return error; > + } > + > + /* > + * Read in the block containing the "old" attr, then remove the > + * "old" attr from that block (neat, huh!) > + */ > + error = xfs_attr3_leaf_read(args->trans, args->dp, args->blkno, > + &bp); > + if (error) > return error; > > + xfs_attr3_leaf_remove(bp, args); > + > + /* > + * If the result is small enough, shrink it all into the inode. > + */ > + forkoff = xfs_attr_shortform_allfit(bp, dp); > + if (forkoff) > + error = xfs_attr3_leaf_to_shortform(bp, args, forkoff); > + /* bp is gone due to xfs_da_shrink_inode */ > + > + return error; > +node: > /* > * Promote the attribute list to the Btree format. > */ > @@ -731,115 +823,6 @@ xfs_attr_leaf_try_add( > return retval; > } > > - > -/* > - * 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 > - * later routines can manage their own transactions. > - */ > - error = xfs_trans_roll_inode(&args->trans, dp); > - if (error) > - return error; > - > - /* > - * If there was an out-of-line value, allocate the blocks we > - * identified for its storage and copy the value. This is done > - * after we create the attribute so that we don't overflow the > - * maximum size of a transaction and/or hit a deadlock. > - */ > - if (args->rmtblkno > 0) { > - error = xfs_attr_rmtval_set(args); > - if (error) > - return error; > - } > - > - if (!(args->op_flags & XFS_DA_OP_RENAME)) { > - /* > - * Added a "remote" value, just clear the incomplete flag. > - */ > - if (args->rmtblkno > 0) > - error = xfs_attr3_leaf_clearflag(args); > - > - return error; > - } > - > - /* > - * If this is an atomic rename operation, we must "flip" the incomplete > - * flags on the "new" and "old" attribute/value pairs so that one > - * disappears and one appears atomically. Then we must remove the "old" > - * attribute/value pair. > - * > - * In a separate transaction, set the incomplete flag on the "old" attr > - * and clear the incomplete flag on the "new" attr. > - */ > - > - error = xfs_attr3_leaf_flipflags(args); > - if (error) > - return error; > - /* > - * Commit the flag value change and start the next trans in series. > - */ > - error = xfs_trans_roll_inode(&args->trans, args->dp); > - if (error) > - return error; > - > - /* > - * Dismantle the "old" attribute/value pair by removing a "remote" value > - * (if it exists). > - */ > - xfs_attr_restore_rmt_blk(args); > - > - if (args->rmtblkno) { > - error = xfs_attr_rmtval_invalidate(args); > - if (error) > - return error; > - > - error = xfs_attr_rmtval_remove(args); > - if (error) > - return error; > - } > - > - /* > - * Read in the block containing the "old" attr, then remove the "old" > - * attr from that block (neat, huh!) > - */ > - error = xfs_attr3_leaf_read(args->trans, args->dp, args->blkno, > - &bp); > - if (error) > - return error; > - > - xfs_attr3_leaf_remove(bp, args); > - > - /* > - * If the result is small enough, shrink it all into the inode. > - */ > - forkoff = xfs_attr_shortform_allfit(bp, dp); > - if (forkoff) > - error = xfs_attr3_leaf_to_shortform(bp, args, forkoff); > - /* bp is gone due to xfs_da_shrink_inode */ > - > - return error; > -} > - > /* > * Return EEXIST if attr is found, or ENOATTR if not > */ > -- > 2.7.4 >
On 3/1/21 11:19 AM, Darrick J. Wong wrote: > On Thu, Feb 18, 2021 at 09:53:35AM -0700, Allison Henderson wrote: >> This patch hoists xfs_attr_leaf_addname into the calling function. The >> goal being to get all the code that will require state management into >> the same scope. This isn't particuarly asetheic right away, but it is a > > "aesthetic" > >> preliminary step to to manageing the state machine code. > > "to merging in" ? > > The goto label is ugly, but afaict this patch moves code and the next > one rearranges it the way you ultimately want it, right? > > With spelling fixed, > Reviewed-by: Darrick J. Wong <djwong@kernel.org> > Ok, will fix spelling nits. Thank you! Allison > --D > > >> Signed-off-by: Allison Henderson <allison.henderson@oracle.com> >> --- >> fs/xfs/libxfs/xfs_attr.c | 209 ++++++++++++++++++++++------------------------- >> 1 file changed, 96 insertions(+), 113 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index 19a532a..bfd4466 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -44,9 +44,9 @@ STATIC int xfs_attr_shortform_addname(xfs_da_args_t *args); >> * Internal routines when attribute list is one block. >> */ >> STATIC int xfs_attr_leaf_get(xfs_da_args_t *args); >> -STATIC int xfs_attr_leaf_addname(xfs_da_args_t *args); >> STATIC int xfs_attr_leaf_removename(xfs_da_args_t *args); >> STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp); >> +STATIC int xfs_attr_leaf_try_add(struct xfs_da_args *args, struct xfs_buf *bp); >> >> /* >> * Internal routines when attribute list is more than one block. >> @@ -269,8 +269,9 @@ xfs_attr_set_args( >> struct xfs_da_args *args) >> { >> struct xfs_inode *dp = args->dp; >> + struct xfs_buf *bp = NULL; >> struct xfs_da_state *state = NULL; >> - int error = 0; >> + int forkoff, error = 0; >> int retval = 0; >> >> /* >> @@ -286,10 +287,101 @@ xfs_attr_set_args( >> } >> >> if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) { >> - error = xfs_attr_leaf_addname(args); >> - if (error != -ENOSPC) >> + error = xfs_attr_leaf_try_add(args, bp); >> + if (error == -ENOSPC) >> + goto node; >> + else if (error) >> + return error; >> + >> + /* >> + * Commit the transaction that added the attr name so that >> + * later routines can manage their own transactions. >> + */ >> + error = xfs_trans_roll_inode(&args->trans, dp); >> + if (error) >> + return error; >> + >> + /* >> + * If there was an out-of-line value, allocate the blocks we >> + * identified for its storage and copy the value. This is done >> + * after we create the attribute so that we don't overflow the >> + * maximum size of a transaction and/or hit a deadlock. >> + */ >> + if (args->rmtblkno > 0) { >> + error = xfs_attr_rmtval_set(args); >> + if (error) >> + return error; >> + } >> + >> + if (!(args->op_flags & XFS_DA_OP_RENAME)) { >> + /* >> + * Added a "remote" value, just clear the incomplete >> + *flag. >> + */ >> + if (args->rmtblkno > 0) >> + error = xfs_attr3_leaf_clearflag(args); >> + >> + return error; >> + } >> + >> + /* >> + * If this is an atomic rename operation, we must "flip" the >> + * incomplete flags on the "new" and "old" attribute/value pairs >> + * so that one disappears and one appears atomically. Then we >> + * must remove the "old" attribute/value pair. >> + * >> + * In a separate transaction, set the incomplete flag on the >> + * "old" attr and clear the incomplete flag on the "new" attr. >> + */ >> + >> + error = xfs_attr3_leaf_flipflags(args); >> + if (error) >> + return error; >> + /* >> + * Commit the flag value change and start the next trans in >> + * series. >> + */ >> + error = xfs_trans_roll_inode(&args->trans, args->dp); >> + if (error) >> + return error; >> + >> + /* >> + * Dismantle the "old" attribute/value pair by removing a >> + * "remote" value (if it exists). >> + */ >> + xfs_attr_restore_rmt_blk(args); >> + >> + if (args->rmtblkno) { >> + error = xfs_attr_rmtval_invalidate(args); >> + if (error) >> + return error; >> + >> + error = xfs_attr_rmtval_remove(args); >> + if (error) >> + return error; >> + } >> + >> + /* >> + * Read in the block containing the "old" attr, then remove the >> + * "old" attr from that block (neat, huh!) >> + */ >> + error = xfs_attr3_leaf_read(args->trans, args->dp, args->blkno, >> + &bp); >> + if (error) >> return error; >> >> + xfs_attr3_leaf_remove(bp, args); >> + >> + /* >> + * If the result is small enough, shrink it all into the inode. >> + */ >> + forkoff = xfs_attr_shortform_allfit(bp, dp); >> + if (forkoff) >> + error = xfs_attr3_leaf_to_shortform(bp, args, forkoff); >> + /* bp is gone due to xfs_da_shrink_inode */ >> + >> + return error; >> +node: >> /* >> * Promote the attribute list to the Btree format. >> */ >> @@ -731,115 +823,6 @@ xfs_attr_leaf_try_add( >> return retval; >> } >> >> - >> -/* >> - * 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 >> - * later routines can manage their own transactions. >> - */ >> - error = xfs_trans_roll_inode(&args->trans, dp); >> - if (error) >> - return error; >> - >> - /* >> - * If there was an out-of-line value, allocate the blocks we >> - * identified for its storage and copy the value. This is done >> - * after we create the attribute so that we don't overflow the >> - * maximum size of a transaction and/or hit a deadlock. >> - */ >> - if (args->rmtblkno > 0) { >> - error = xfs_attr_rmtval_set(args); >> - if (error) >> - return error; >> - } >> - >> - if (!(args->op_flags & XFS_DA_OP_RENAME)) { >> - /* >> - * Added a "remote" value, just clear the incomplete flag. >> - */ >> - if (args->rmtblkno > 0) >> - error = xfs_attr3_leaf_clearflag(args); >> - >> - return error; >> - } >> - >> - /* >> - * If this is an atomic rename operation, we must "flip" the incomplete >> - * flags on the "new" and "old" attribute/value pairs so that one >> - * disappears and one appears atomically. Then we must remove the "old" >> - * attribute/value pair. >> - * >> - * In a separate transaction, set the incomplete flag on the "old" attr >> - * and clear the incomplete flag on the "new" attr. >> - */ >> - >> - error = xfs_attr3_leaf_flipflags(args); >> - if (error) >> - return error; >> - /* >> - * Commit the flag value change and start the next trans in series. >> - */ >> - error = xfs_trans_roll_inode(&args->trans, args->dp); >> - if (error) >> - return error; >> - >> - /* >> - * Dismantle the "old" attribute/value pair by removing a "remote" value >> - * (if it exists). >> - */ >> - xfs_attr_restore_rmt_blk(args); >> - >> - if (args->rmtblkno) { >> - error = xfs_attr_rmtval_invalidate(args); >> - if (error) >> - return error; >> - >> - error = xfs_attr_rmtval_remove(args); >> - if (error) >> - return error; >> - } >> - >> - /* >> - * Read in the block containing the "old" attr, then remove the "old" >> - * attr from that block (neat, huh!) >> - */ >> - error = xfs_attr3_leaf_read(args->trans, args->dp, args->blkno, >> - &bp); >> - if (error) >> - return error; >> - >> - xfs_attr3_leaf_remove(bp, args); >> - >> - /* >> - * If the result is small enough, shrink it all into the inode. >> - */ >> - forkoff = xfs_attr_shortform_allfit(bp, dp); >> - if (forkoff) >> - error = xfs_attr3_leaf_to_shortform(bp, args, forkoff); >> - /* bp is gone due to xfs_da_shrink_inode */ >> - >> - return error; >> -} >> - >> /* >> * Return EEXIST if attr is found, or ENOATTR if not >> */ >> -- >> 2.7.4 >>
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index 19a532a..bfd4466 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -44,9 +44,9 @@ STATIC int xfs_attr_shortform_addname(xfs_da_args_t *args); * Internal routines when attribute list is one block. */ STATIC int xfs_attr_leaf_get(xfs_da_args_t *args); -STATIC int xfs_attr_leaf_addname(xfs_da_args_t *args); STATIC int xfs_attr_leaf_removename(xfs_da_args_t *args); STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp); +STATIC int xfs_attr_leaf_try_add(struct xfs_da_args *args, struct xfs_buf *bp); /* * Internal routines when attribute list is more than one block. @@ -269,8 +269,9 @@ xfs_attr_set_args( struct xfs_da_args *args) { struct xfs_inode *dp = args->dp; + struct xfs_buf *bp = NULL; struct xfs_da_state *state = NULL; - int error = 0; + int forkoff, error = 0; int retval = 0; /* @@ -286,10 +287,101 @@ xfs_attr_set_args( } if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) { - error = xfs_attr_leaf_addname(args); - if (error != -ENOSPC) + error = xfs_attr_leaf_try_add(args, bp); + if (error == -ENOSPC) + goto node; + else if (error) + return error; + + /* + * Commit the transaction that added the attr name so that + * later routines can manage their own transactions. + */ + error = xfs_trans_roll_inode(&args->trans, dp); + if (error) + return error; + + /* + * If there was an out-of-line value, allocate the blocks we + * identified for its storage and copy the value. This is done + * after we create the attribute so that we don't overflow the + * maximum size of a transaction and/or hit a deadlock. + */ + if (args->rmtblkno > 0) { + error = xfs_attr_rmtval_set(args); + if (error) + return error; + } + + if (!(args->op_flags & XFS_DA_OP_RENAME)) { + /* + * Added a "remote" value, just clear the incomplete + *flag. + */ + if (args->rmtblkno > 0) + error = xfs_attr3_leaf_clearflag(args); + + return error; + } + + /* + * If this is an atomic rename operation, we must "flip" the + * incomplete flags on the "new" and "old" attribute/value pairs + * so that one disappears and one appears atomically. Then we + * must remove the "old" attribute/value pair. + * + * In a separate transaction, set the incomplete flag on the + * "old" attr and clear the incomplete flag on the "new" attr. + */ + + error = xfs_attr3_leaf_flipflags(args); + if (error) + return error; + /* + * Commit the flag value change and start the next trans in + * series. + */ + error = xfs_trans_roll_inode(&args->trans, args->dp); + if (error) + return error; + + /* + * Dismantle the "old" attribute/value pair by removing a + * "remote" value (if it exists). + */ + xfs_attr_restore_rmt_blk(args); + + if (args->rmtblkno) { + error = xfs_attr_rmtval_invalidate(args); + if (error) + return error; + + error = xfs_attr_rmtval_remove(args); + if (error) + return error; + } + + /* + * Read in the block containing the "old" attr, then remove the + * "old" attr from that block (neat, huh!) + */ + error = xfs_attr3_leaf_read(args->trans, args->dp, args->blkno, + &bp); + if (error) return error; + xfs_attr3_leaf_remove(bp, args); + + /* + * If the result is small enough, shrink it all into the inode. + */ + forkoff = xfs_attr_shortform_allfit(bp, dp); + if (forkoff) + error = xfs_attr3_leaf_to_shortform(bp, args, forkoff); + /* bp is gone due to xfs_da_shrink_inode */ + + return error; +node: /* * Promote the attribute list to the Btree format. */ @@ -731,115 +823,6 @@ xfs_attr_leaf_try_add( return retval; } - -/* - * 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 - * later routines can manage their own transactions. - */ - error = xfs_trans_roll_inode(&args->trans, dp); - if (error) - return error; - - /* - * If there was an out-of-line value, allocate the blocks we - * identified for its storage and copy the value. This is done - * after we create the attribute so that we don't overflow the - * maximum size of a transaction and/or hit a deadlock. - */ - if (args->rmtblkno > 0) { - error = xfs_attr_rmtval_set(args); - if (error) - return error; - } - - if (!(args->op_flags & XFS_DA_OP_RENAME)) { - /* - * Added a "remote" value, just clear the incomplete flag. - */ - if (args->rmtblkno > 0) - error = xfs_attr3_leaf_clearflag(args); - - return error; - } - - /* - * If this is an atomic rename operation, we must "flip" the incomplete - * flags on the "new" and "old" attribute/value pairs so that one - * disappears and one appears atomically. Then we must remove the "old" - * attribute/value pair. - * - * In a separate transaction, set the incomplete flag on the "old" attr - * and clear the incomplete flag on the "new" attr. - */ - - error = xfs_attr3_leaf_flipflags(args); - if (error) - return error; - /* - * Commit the flag value change and start the next trans in series. - */ - error = xfs_trans_roll_inode(&args->trans, args->dp); - if (error) - return error; - - /* - * Dismantle the "old" attribute/value pair by removing a "remote" value - * (if it exists). - */ - xfs_attr_restore_rmt_blk(args); - - if (args->rmtblkno) { - error = xfs_attr_rmtval_invalidate(args); - if (error) - return error; - - error = xfs_attr_rmtval_remove(args); - if (error) - return error; - } - - /* - * Read in the block containing the "old" attr, then remove the "old" - * attr from that block (neat, huh!) - */ - error = xfs_attr3_leaf_read(args->trans, args->dp, args->blkno, - &bp); - if (error) - return error; - - xfs_attr3_leaf_remove(bp, args); - - /* - * If the result is small enough, shrink it all into the inode. - */ - forkoff = xfs_attr_shortform_allfit(bp, dp); - if (forkoff) - error = xfs_attr3_leaf_to_shortform(bp, args, forkoff); - /* bp is gone due to xfs_da_shrink_inode */ - - return error; -} - /* * Return EEXIST if attr is found, or ENOATTR if not */
This patch hoists xfs_attr_leaf_addname into the calling function. The goal being to get all the code that will require state management into the same scope. This isn't particuarly asetheic right away, but it is a preliminary step to to manageing the state machine code. Signed-off-by: Allison Henderson <allison.henderson@oracle.com> --- fs/xfs/libxfs/xfs_attr.c | 209 ++++++++++++++++++++++------------------------- 1 file changed, 96 insertions(+), 113 deletions(-)