| Message ID | 20200430225016.4287-21-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: Delay Ready Attributes | expand |
On Thu, Apr 30, 2020 at 03:50:12PM -0700, Allison Collins wrote: > Quick patch to unnest the rename logic in the node code path. This will > help simplify delayed attr logic later. > > Signed-off-by: Allison Collins <allison.henderson@oracle.com> > --- > fs/xfs/libxfs/xfs_attr.c | 131 +++++++++++++++++++++++------------------------ > 1 file changed, 64 insertions(+), 67 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index 1810f90..9171895 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -1030,83 +1030,80 @@ xfs_attr_node_addname( > 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. > - */ > - if (args->op_flags & XFS_DA_OP_RENAME) { > - /* > - * 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) > - goto out; > + if ((args->op_flags & XFS_DA_OP_RENAME) == 0) { Same comments about the commit message and the if test as the previous patch. Admittedly I'm now starting to wonder if this patch and the previous one should really just hoist the taken and not-taken branches into separate functions, but maybe you've already gone around and around on that, and everyone else already thought we have too many small functions... --D > /* > - * Commit the flag value change and start the next trans in > - * series > + * Added a "remote" value, just clear the incomplete flag. > */ > - error = xfs_trans_roll_inode(&args->trans, args->dp); > - if (error) > - goto out; > + if (args->rmtblkno > 0) > + error = xfs_attr3_leaf_clearflag(args); > + retval = error; > + goto out; > + } > > - /* > - * Dismantle the "old" attribute/value pair by removing > - * a "remote" value (if it exists). > - */ > - xfs_attr_restore_rmt_blk(args); > + /* > + * 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) > + goto out; > + /* > + * Commit the flag value change and start the next trans in series > + */ > + error = xfs_trans_roll_inode(&args->trans, args->dp); > + if (error) > + goto out; > > - if (args->rmtblkno) { > - error = xfs_attr_rmtval_invalidate(args); > - if (error) > - return error; > + /* > + * Dismantle the "old" attribute/value pair by removing a "remote" value > + * (if it exists). > + */ > + xfs_attr_restore_rmt_blk(args); > > - error = xfs_attr_rmtval_remove(args); > - if (error) > - return error; > - } > + if (args->rmtblkno) { > + error = xfs_attr_rmtval_invalidate(args); > + if (error) > + return error; > > - /* > - * Re-find the "old" attribute entry after any split ops. > - * The INCOMPLETE flag means that we will find the "old" > - * attr, not the "new" one. > - */ > - args->attr_filter |= XFS_ATTR_INCOMPLETE; > - state = xfs_da_state_alloc(); > - state->args = args; > - state->mp = mp; > - state->inleaf = 0; > - error = xfs_da3_node_lookup_int(state, &retval); > + error = xfs_attr_rmtval_remove(args); > if (error) > - goto out; > + return error; > + } > > - /* > - * Remove the name and update the hashvals in the tree. > - */ > - blk = &state->path.blk[ state->path.active-1 ]; > - ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC); > - error = xfs_attr3_leaf_remove(blk->bp, args); > - xfs_da3_fixhashpath(state, &state->path); > + /* > + * Re-find the "old" attribute entry after any split ops. The INCOMPLETE > + * flag means that we will find the "old" attr, not the "new" one. > + */ > + args->attr_filter |= XFS_ATTR_INCOMPLETE; > + state = xfs_da_state_alloc(); > + state->args = args; > + state->mp = mp; > + state->inleaf = 0; > + error = xfs_da3_node_lookup_int(state, &retval); > + if (error) > + goto out; > > - /* > - * Check to see if the tree needs to be collapsed. > - */ > - if (retval && (state->path.active > 1)) { > - error = xfs_da3_join(state); > - if (error) > - goto out; > - error = xfs_defer_finish(&args->trans); > - if (error) > - goto out; > - } > + /* > + * Remove the name and update the hashvals in the tree. > + */ > + blk = &state->path.blk[state->path.active-1]; > + ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC); > + error = xfs_attr3_leaf_remove(blk->bp, args); > + xfs_da3_fixhashpath(state, &state->path); > > - } else if (args->rmtblkno > 0) { > - /* > - * Added a "remote" value, just clear the incomplete flag. > - */ > - error = xfs_attr3_leaf_clearflag(args); > + /* > + * Check to see if the tree needs to be collapsed. > + */ > + if (retval && (state->path.active > 1)) { > + error = xfs_da3_join(state); > + if (error) > + goto out; > + error = xfs_defer_finish(&args->trans); > if (error) > goto out; > } > -- > 2.7.4 >
On 5/4/20 12:06 PM, Darrick J. Wong wrote: > On Thu, Apr 30, 2020 at 03:50:12PM -0700, Allison Collins wrote: >> Quick patch to unnest the rename logic in the node code path. This will >> help simplify delayed attr logic later. >> >> Signed-off-by: Allison Collins <allison.henderson@oracle.com> >> --- >> fs/xfs/libxfs/xfs_attr.c | 131 +++++++++++++++++++++++------------------------ >> 1 file changed, 64 insertions(+), 67 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index 1810f90..9171895 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -1030,83 +1030,80 @@ xfs_attr_node_addname( >> 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. >> - */ >> - if (args->op_flags & XFS_DA_OP_RENAME) { >> - /* >> - * 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) >> - goto out; >> + if ((args->op_flags & XFS_DA_OP_RENAME) == 0) { > > Same comments about the commit message and the if test as the previous > patch. > > Admittedly I'm now starting to wonder if this patch and the previous one > should really just hoist the taken and not-taken branches into separate > functions, but maybe you've already gone around and around on that, and > everyone else already thought we have too many small functions... Yes, I had played with that and even suggested it myself in the last review. What I didnt like about it after prototyping it, was having to plumb around the leaf_bp that is still needed for a rename, and also the state machine logic ends up getting plumbed downward, instead of trying to keep it up in one high level function. Really the initial motivation for this patch was to just avoid putting xfs_das_* labels in the middle of if{} statements or loops when we get further down the set. With that goal in mind, this solution seemed like the least amount of required surgery. Though the patch itself tends to suggest a helper, I figure I'd let people consider this first before jumping to the later option which will end up with more plumbing. Allison > > --D > >> /* >> - * Commit the flag value change and start the next trans in >> - * series >> + * Added a "remote" value, just clear the incomplete flag. >> */ >> - error = xfs_trans_roll_inode(&args->trans, args->dp); >> - if (error) >> - goto out; >> + if (args->rmtblkno > 0) >> + error = xfs_attr3_leaf_clearflag(args); >> + retval = error; >> + goto out; >> + } >> >> - /* >> - * Dismantle the "old" attribute/value pair by removing >> - * a "remote" value (if it exists). >> - */ >> - xfs_attr_restore_rmt_blk(args); >> + /* >> + * 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) >> + goto out; >> + /* >> + * Commit the flag value change and start the next trans in series >> + */ >> + error = xfs_trans_roll_inode(&args->trans, args->dp); >> + if (error) >> + goto out; >> >> - if (args->rmtblkno) { >> - error = xfs_attr_rmtval_invalidate(args); >> - if (error) >> - return error; >> + /* >> + * Dismantle the "old" attribute/value pair by removing a "remote" value >> + * (if it exists). >> + */ >> + xfs_attr_restore_rmt_blk(args); >> >> - error = xfs_attr_rmtval_remove(args); >> - if (error) >> - return error; >> - } >> + if (args->rmtblkno) { >> + error = xfs_attr_rmtval_invalidate(args); >> + if (error) >> + return error; >> >> - /* >> - * Re-find the "old" attribute entry after any split ops. >> - * The INCOMPLETE flag means that we will find the "old" >> - * attr, not the "new" one. >> - */ >> - args->attr_filter |= XFS_ATTR_INCOMPLETE; >> - state = xfs_da_state_alloc(); >> - state->args = args; >> - state->mp = mp; >> - state->inleaf = 0; >> - error = xfs_da3_node_lookup_int(state, &retval); >> + error = xfs_attr_rmtval_remove(args); >> if (error) >> - goto out; >> + return error; >> + } >> >> - /* >> - * Remove the name and update the hashvals in the tree. >> - */ >> - blk = &state->path.blk[ state->path.active-1 ]; >> - ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC); >> - error = xfs_attr3_leaf_remove(blk->bp, args); >> - xfs_da3_fixhashpath(state, &state->path); >> + /* >> + * Re-find the "old" attribute entry after any split ops. The INCOMPLETE >> + * flag means that we will find the "old" attr, not the "new" one. >> + */ >> + args->attr_filter |= XFS_ATTR_INCOMPLETE; >> + state = xfs_da_state_alloc(); >> + state->args = args; >> + state->mp = mp; >> + state->inleaf = 0; >> + error = xfs_da3_node_lookup_int(state, &retval); >> + if (error) >> + goto out; >> >> - /* >> - * Check to see if the tree needs to be collapsed. >> - */ >> - if (retval && (state->path.active > 1)) { >> - error = xfs_da3_join(state); >> - if (error) >> - goto out; >> - error = xfs_defer_finish(&args->trans); >> - if (error) >> - goto out; >> - } >> + /* >> + * Remove the name and update the hashvals in the tree. >> + */ >> + blk = &state->path.blk[state->path.active-1]; >> + ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC); >> + error = xfs_attr3_leaf_remove(blk->bp, args); >> + xfs_da3_fixhashpath(state, &state->path); >> >> - } else if (args->rmtblkno > 0) { >> - /* >> - * Added a "remote" value, just clear the incomplete flag. >> - */ >> - error = xfs_attr3_leaf_clearflag(args); >> + /* >> + * Check to see if the tree needs to be collapsed. >> + */ >> + if (retval && (state->path.active > 1)) { >> + error = xfs_da3_join(state); >> + if (error) >> + goto out; >> + error = xfs_defer_finish(&args->trans); >> if (error) >> goto out; >> } >> -- >> 2.7.4 >>
On Thu, Apr 30, 2020 at 03:50:12PM -0700, Allison Collins wrote: > Quick patch to unnest the rename logic in the node code path. This will > help simplify delayed attr logic later. > > Signed-off-by: Allison Collins <allison.henderson@oracle.com> > --- > fs/xfs/libxfs/xfs_attr.c | 131 +++++++++++++++++++++++------------------------ > 1 file changed, 64 insertions(+), 67 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index 1810f90..9171895 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -1030,83 +1030,80 @@ xfs_attr_node_addname( > 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. > - */ > - if (args->op_flags & XFS_DA_OP_RENAME) { > - /* > - * 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) > - goto out; > + if ((args->op_flags & XFS_DA_OP_RENAME) == 0) { > /* > - * Commit the flag value change and start the next trans in > - * series > + * Added a "remote" value, just clear the incomplete flag. > */ > - error = xfs_trans_roll_inode(&args->trans, args->dp); > - if (error) > - goto out; > + if (args->rmtblkno > 0) > + error = xfs_attr3_leaf_clearflag(args); > + retval = error; Can we just init retval to 0 and avoid this assignment? With that and similar fixups to the previous patch: Reviewed-by: Brian Foster <bfoster@redhat.com> Note that I'm also of the mind to break this down into smaller functions depending on what later patches look like, but if there's more to consider around that this seems like a good step in that direction. Brian > + goto out; > + } > > - /* > - * Dismantle the "old" attribute/value pair by removing > - * a "remote" value (if it exists). > - */ > - xfs_attr_restore_rmt_blk(args); > + /* > + * 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) > + goto out; > + /* > + * Commit the flag value change and start the next trans in series > + */ > + error = xfs_trans_roll_inode(&args->trans, args->dp); > + if (error) > + goto out; > > - if (args->rmtblkno) { > - error = xfs_attr_rmtval_invalidate(args); > - if (error) > - return error; > + /* > + * Dismantle the "old" attribute/value pair by removing a "remote" value > + * (if it exists). > + */ > + xfs_attr_restore_rmt_blk(args); > > - error = xfs_attr_rmtval_remove(args); > - if (error) > - return error; > - } > + if (args->rmtblkno) { > + error = xfs_attr_rmtval_invalidate(args); > + if (error) > + return error; > > - /* > - * Re-find the "old" attribute entry after any split ops. > - * The INCOMPLETE flag means that we will find the "old" > - * attr, not the "new" one. > - */ > - args->attr_filter |= XFS_ATTR_INCOMPLETE; > - state = xfs_da_state_alloc(); > - state->args = args; > - state->mp = mp; > - state->inleaf = 0; > - error = xfs_da3_node_lookup_int(state, &retval); > + error = xfs_attr_rmtval_remove(args); > if (error) > - goto out; > + return error; > + } > > - /* > - * Remove the name and update the hashvals in the tree. > - */ > - blk = &state->path.blk[ state->path.active-1 ]; > - ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC); > - error = xfs_attr3_leaf_remove(blk->bp, args); > - xfs_da3_fixhashpath(state, &state->path); > + /* > + * Re-find the "old" attribute entry after any split ops. The INCOMPLETE > + * flag means that we will find the "old" attr, not the "new" one. > + */ > + args->attr_filter |= XFS_ATTR_INCOMPLETE; > + state = xfs_da_state_alloc(); > + state->args = args; > + state->mp = mp; > + state->inleaf = 0; > + error = xfs_da3_node_lookup_int(state, &retval); > + if (error) > + goto out; > > - /* > - * Check to see if the tree needs to be collapsed. > - */ > - if (retval && (state->path.active > 1)) { > - error = xfs_da3_join(state); > - if (error) > - goto out; > - error = xfs_defer_finish(&args->trans); > - if (error) > - goto out; > - } > + /* > + * Remove the name and update the hashvals in the tree. > + */ > + blk = &state->path.blk[state->path.active-1]; > + ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC); > + error = xfs_attr3_leaf_remove(blk->bp, args); > + xfs_da3_fixhashpath(state, &state->path); > > - } else if (args->rmtblkno > 0) { > - /* > - * Added a "remote" value, just clear the incomplete flag. > - */ > - error = xfs_attr3_leaf_clearflag(args); > + /* > + * Check to see if the tree needs to be collapsed. > + */ > + if (retval && (state->path.active > 1)) { > + error = xfs_da3_join(state); > + if (error) > + goto out; > + error = xfs_defer_finish(&args->trans); > if (error) > goto out; > } > -- > 2.7.4 >
On 5/5/20 6:12 AM, Brian Foster wrote: > On Thu, Apr 30, 2020 at 03:50:12PM -0700, Allison Collins wrote: >> Quick patch to unnest the rename logic in the node code path. This will >> help simplify delayed attr logic later. >> >> Signed-off-by: Allison Collins <allison.henderson@oracle.com> >> --- >> fs/xfs/libxfs/xfs_attr.c | 131 +++++++++++++++++++++++------------------------ >> 1 file changed, 64 insertions(+), 67 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index 1810f90..9171895 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -1030,83 +1030,80 @@ xfs_attr_node_addname( >> 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. >> - */ >> - if (args->op_flags & XFS_DA_OP_RENAME) { >> - /* >> - * 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) >> - goto out; >> + if ((args->op_flags & XFS_DA_OP_RENAME) == 0) { >> /* >> - * Commit the flag value change and start the next trans in >> - * series >> + * Added a "remote" value, just clear the incomplete flag. >> */ >> - error = xfs_trans_roll_inode(&args->trans, args->dp); >> - if (error) >> - goto out; >> + if (args->rmtblkno > 0) >> + error = xfs_attr3_leaf_clearflag(args); >> + retval = error; > > Can we just init retval to 0 and avoid this assignment? With that and > similar fixups to the previous patch: > Sure, will do. > Reviewed-by: Brian Foster <bfoster@redhat.com> > > Note that I'm also of the mind to break this down into smaller functions > depending on what later patches look like, but if there's more to > consider around that this seems like a good step in that direction. Sure, and that's fine too if we change our minds later. I just figured I see what people thought of this first. Thanks for the reviews! Allison > > Brian > >> + goto out; >> + } >> >> - /* >> - * Dismantle the "old" attribute/value pair by removing >> - * a "remote" value (if it exists). >> - */ >> - xfs_attr_restore_rmt_blk(args); >> + /* >> + * 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) >> + goto out; >> + /* >> + * Commit the flag value change and start the next trans in series >> + */ >> + error = xfs_trans_roll_inode(&args->trans, args->dp); >> + if (error) >> + goto out; >> >> - if (args->rmtblkno) { >> - error = xfs_attr_rmtval_invalidate(args); >> - if (error) >> - return error; >> + /* >> + * Dismantle the "old" attribute/value pair by removing a "remote" value >> + * (if it exists). >> + */ >> + xfs_attr_restore_rmt_blk(args); >> >> - error = xfs_attr_rmtval_remove(args); >> - if (error) >> - return error; >> - } >> + if (args->rmtblkno) { >> + error = xfs_attr_rmtval_invalidate(args); >> + if (error) >> + return error; >> >> - /* >> - * Re-find the "old" attribute entry after any split ops. >> - * The INCOMPLETE flag means that we will find the "old" >> - * attr, not the "new" one. >> - */ >> - args->attr_filter |= XFS_ATTR_INCOMPLETE; >> - state = xfs_da_state_alloc(); >> - state->args = args; >> - state->mp = mp; >> - state->inleaf = 0; >> - error = xfs_da3_node_lookup_int(state, &retval); >> + error = xfs_attr_rmtval_remove(args); >> if (error) >> - goto out; >> + return error; >> + } >> >> - /* >> - * Remove the name and update the hashvals in the tree. >> - */ >> - blk = &state->path.blk[ state->path.active-1 ]; >> - ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC); >> - error = xfs_attr3_leaf_remove(blk->bp, args); >> - xfs_da3_fixhashpath(state, &state->path); >> + /* >> + * Re-find the "old" attribute entry after any split ops. The INCOMPLETE >> + * flag means that we will find the "old" attr, not the "new" one. >> + */ >> + args->attr_filter |= XFS_ATTR_INCOMPLETE; >> + state = xfs_da_state_alloc(); >> + state->args = args; >> + state->mp = mp; >> + state->inleaf = 0; >> + error = xfs_da3_node_lookup_int(state, &retval); >> + if (error) >> + goto out; >> >> - /* >> - * Check to see if the tree needs to be collapsed. >> - */ >> - if (retval && (state->path.active > 1)) { >> - error = xfs_da3_join(state); >> - if (error) >> - goto out; >> - error = xfs_defer_finish(&args->trans); >> - if (error) >> - goto out; >> - } >> + /* >> + * Remove the name and update the hashvals in the tree. >> + */ >> + blk = &state->path.blk[state->path.active-1]; >> + ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC); >> + error = xfs_attr3_leaf_remove(blk->bp, args); >> + xfs_da3_fixhashpath(state, &state->path); >> >> - } else if (args->rmtblkno > 0) { >> - /* >> - * Added a "remote" value, just clear the incomplete flag. >> - */ >> - error = xfs_attr3_leaf_clearflag(args); >> + /* >> + * Check to see if the tree needs to be collapsed. >> + */ >> + if (retval && (state->path.active > 1)) { >> + error = xfs_da3_join(state); >> + if (error) >> + goto out; >> + error = xfs_defer_finish(&args->trans); >> if (error) >> goto out; >> } >> -- >> 2.7.4 >> >
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index 1810f90..9171895 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -1030,83 +1030,80 @@ xfs_attr_node_addname( 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. - */ - if (args->op_flags & XFS_DA_OP_RENAME) { - /* - * 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) - goto out; + if ((args->op_flags & XFS_DA_OP_RENAME) == 0) { /* - * Commit the flag value change and start the next trans in - * series + * Added a "remote" value, just clear the incomplete flag. */ - error = xfs_trans_roll_inode(&args->trans, args->dp); - if (error) - goto out; + if (args->rmtblkno > 0) + error = xfs_attr3_leaf_clearflag(args); + retval = error; + goto out; + } - /* - * Dismantle the "old" attribute/value pair by removing - * a "remote" value (if it exists). - */ - xfs_attr_restore_rmt_blk(args); + /* + * 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) + goto out; + /* + * Commit the flag value change and start the next trans in series + */ + error = xfs_trans_roll_inode(&args->trans, args->dp); + if (error) + goto out; - if (args->rmtblkno) { - error = xfs_attr_rmtval_invalidate(args); - if (error) - return error; + /* + * Dismantle the "old" attribute/value pair by removing a "remote" value + * (if it exists). + */ + xfs_attr_restore_rmt_blk(args); - error = xfs_attr_rmtval_remove(args); - if (error) - return error; - } + if (args->rmtblkno) { + error = xfs_attr_rmtval_invalidate(args); + if (error) + return error; - /* - * Re-find the "old" attribute entry after any split ops. - * The INCOMPLETE flag means that we will find the "old" - * attr, not the "new" one. - */ - args->attr_filter |= XFS_ATTR_INCOMPLETE; - state = xfs_da_state_alloc(); - state->args = args; - state->mp = mp; - state->inleaf = 0; - error = xfs_da3_node_lookup_int(state, &retval); + error = xfs_attr_rmtval_remove(args); if (error) - goto out; + return error; + } - /* - * Remove the name and update the hashvals in the tree. - */ - blk = &state->path.blk[ state->path.active-1 ]; - ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC); - error = xfs_attr3_leaf_remove(blk->bp, args); - xfs_da3_fixhashpath(state, &state->path); + /* + * Re-find the "old" attribute entry after any split ops. The INCOMPLETE + * flag means that we will find the "old" attr, not the "new" one. + */ + args->attr_filter |= XFS_ATTR_INCOMPLETE; + state = xfs_da_state_alloc(); + state->args = args; + state->mp = mp; + state->inleaf = 0; + error = xfs_da3_node_lookup_int(state, &retval); + if (error) + goto out; - /* - * Check to see if the tree needs to be collapsed. - */ - if (retval && (state->path.active > 1)) { - error = xfs_da3_join(state); - if (error) - goto out; - error = xfs_defer_finish(&args->trans); - if (error) - goto out; - } + /* + * Remove the name and update the hashvals in the tree. + */ + blk = &state->path.blk[state->path.active-1]; + ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC); + error = xfs_attr3_leaf_remove(blk->bp, args); + xfs_da3_fixhashpath(state, &state->path); - } else if (args->rmtblkno > 0) { - /* - * Added a "remote" value, just clear the incomplete flag. - */ - error = xfs_attr3_leaf_clearflag(args); + /* + * Check to see if the tree needs to be collapsed. + */ + if (retval && (state->path.active > 1)) { + error = xfs_da3_join(state); + if (error) + goto out; + error = xfs_defer_finish(&args->trans); if (error) goto out; }
Quick patch to unnest the rename logic in the node code path. This will help simplify delayed attr logic later. Signed-off-by: Allison Collins <allison.henderson@oracle.com> --- fs/xfs/libxfs/xfs_attr.c | 131 +++++++++++++++++++++++------------------------ 1 file changed, 64 insertions(+), 67 deletions(-)