| Message ID | 20210218165348.4754-18-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xfs: Delayed Attributes | expand |
On Thu, Feb 18, 2021 at 09:53:43AM -0700, Allison Henderson wrote: > This is a clean up patch that skips the flip flag logic for delayed attr > renames. Since the log replay keeps the inode locked, we do not need to > worry about race windows with attr lookups. So we can skip over > flipping the flag and the extra transaction roll for it > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> I wonder, have you done much performance analysis of the old vs. new xattr code paths? Does skipping the extra step + roll make attr operations faster? This looks pretty straightforward though: Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > fs/xfs/libxfs/xfs_attr.c | 51 +++++++++++++++++++++++++------------------ > fs/xfs/libxfs/xfs_attr_leaf.c | 3 ++- > 2 files changed, 32 insertions(+), 22 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index e4c1b4b..666cc69 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -337,6 +337,7 @@ xfs_attr_set_iter( > struct xfs_da_state *state = NULL; > int forkoff, error = 0; > int retval = 0; > + struct xfs_mount *mp = args->dp->i_mount; > > /* State machine switch */ > switch (dac->dela_state) { > @@ -470,16 +471,21 @@ xfs_attr_set_iter( > * "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. > - */ > - dac->dela_state = XFS_DAS_FLIP_LFLAG; > - trace_xfs_attr_set_iter_return(dac->dela_state, args->dp); > - return -EAGAIN; > + if (!xfs_hasdelattr(mp)) { > + error = xfs_attr3_leaf_flipflags(args); > + if (error) > + return error; > + /* > + * Commit the flag value change and start the next trans > + * in series. > + */ > + dac->dela_state = XFS_DAS_FLIP_LFLAG; > + trace_xfs_attr_set_iter_return(dac->dela_state, > + args->dp); > + return -EAGAIN; > + } > + > + /* fallthrough */ > case XFS_DAS_FLIP_LFLAG: > /* > * Dismantle the "old" attribute/value pair by removing a > @@ -588,17 +594,21 @@ xfs_attr_set_iter( > * 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 > - */ > - dac->dela_state = XFS_DAS_FLIP_NFLAG; > - trace_xfs_attr_set_iter_return(dac->dela_state, args->dp); > - return -EAGAIN; > + if (!xfs_hasdelattr(mp)) { > + error = xfs_attr3_leaf_flipflags(args); > + if (error) > + goto out; > + /* > + * Commit the flag value change and start the next trans > + * in series > + */ > + dac->dela_state = XFS_DAS_FLIP_NFLAG; > + trace_xfs_attr_set_iter_return(dac->dela_state, > + args->dp); > + return -EAGAIN; > + } > > + /* fallthrough */ > case XFS_DAS_FLIP_NFLAG: > /* > * Dismantle the "old" attribute/value pair by removing a > @@ -1277,7 +1287,6 @@ int xfs_attr_node_addname_work( > * 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(args); > state->inleaf = 0; > error = xfs_da3_node_lookup_int(state, &retval); > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c > index 3780141..ec707bd 100644 > --- a/fs/xfs/libxfs/xfs_attr_leaf.c > +++ b/fs/xfs/libxfs/xfs_attr_leaf.c > @@ -1486,7 +1486,8 @@ xfs_attr3_leaf_add_work( > if (tmp) > entry->flags |= XFS_ATTR_LOCAL; > if (args->op_flags & XFS_DA_OP_RENAME) { > - entry->flags |= XFS_ATTR_INCOMPLETE; > + if (!xfs_hasdelattr(mp)) > + entry->flags |= XFS_ATTR_INCOMPLETE; > if ((args->blkno2 == args->blkno) && > (args->index2 <= args->index)) { > args->index2++; > -- > 2.7.4 >
On 2/25/21 10:02 PM, Darrick J. Wong wrote: > On Thu, Feb 18, 2021 at 09:53:43AM -0700, Allison Henderson wrote: >> This is a clean up patch that skips the flip flag logic for delayed attr >> renames. Since the log replay keeps the inode locked, we do not need to >> worry about race windows with attr lookups. So we can skip over >> flipping the flag and the extra transaction roll for it >> >> Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > > I wonder, have you done much performance analysis of the old vs. new > xattr code paths? Does skipping the extra step + roll make attr > operations faster? I dont have any analysis right now, but maybe I could put some together. I'm sure there's some impact, but not sure how much. If it does, I suspect it will become of more interest when we bring in pptrs since the code path with be in heavier use then. > > This looks pretty straightforward though: > Reviewed-by: Darrick J. Wong <djwong@kernel.org> Thank you! Allison > > --D > >> --- >> fs/xfs/libxfs/xfs_attr.c | 51 +++++++++++++++++++++++++------------------ >> fs/xfs/libxfs/xfs_attr_leaf.c | 3 ++- >> 2 files changed, 32 insertions(+), 22 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index e4c1b4b..666cc69 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -337,6 +337,7 @@ xfs_attr_set_iter( >> struct xfs_da_state *state = NULL; >> int forkoff, error = 0; >> int retval = 0; >> + struct xfs_mount *mp = args->dp->i_mount; >> >> /* State machine switch */ >> switch (dac->dela_state) { >> @@ -470,16 +471,21 @@ xfs_attr_set_iter( >> * "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. >> - */ >> - dac->dela_state = XFS_DAS_FLIP_LFLAG; >> - trace_xfs_attr_set_iter_return(dac->dela_state, args->dp); >> - return -EAGAIN; >> + if (!xfs_hasdelattr(mp)) { >> + error = xfs_attr3_leaf_flipflags(args); >> + if (error) >> + return error; >> + /* >> + * Commit the flag value change and start the next trans >> + * in series. >> + */ >> + dac->dela_state = XFS_DAS_FLIP_LFLAG; >> + trace_xfs_attr_set_iter_return(dac->dela_state, >> + args->dp); >> + return -EAGAIN; >> + } >> + >> + /* fallthrough */ >> case XFS_DAS_FLIP_LFLAG: >> /* >> * Dismantle the "old" attribute/value pair by removing a >> @@ -588,17 +594,21 @@ xfs_attr_set_iter( >> * 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 >> - */ >> - dac->dela_state = XFS_DAS_FLIP_NFLAG; >> - trace_xfs_attr_set_iter_return(dac->dela_state, args->dp); >> - return -EAGAIN; >> + if (!xfs_hasdelattr(mp)) { >> + error = xfs_attr3_leaf_flipflags(args); >> + if (error) >> + goto out; >> + /* >> + * Commit the flag value change and start the next trans >> + * in series >> + */ >> + dac->dela_state = XFS_DAS_FLIP_NFLAG; >> + trace_xfs_attr_set_iter_return(dac->dela_state, >> + args->dp); >> + return -EAGAIN; >> + } >> >> + /* fallthrough */ >> case XFS_DAS_FLIP_NFLAG: >> /* >> * Dismantle the "old" attribute/value pair by removing a >> @@ -1277,7 +1287,6 @@ int xfs_attr_node_addname_work( >> * 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(args); >> state->inleaf = 0; >> error = xfs_da3_node_lookup_int(state, &retval); >> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c >> index 3780141..ec707bd 100644 >> --- a/fs/xfs/libxfs/xfs_attr_leaf.c >> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c >> @@ -1486,7 +1486,8 @@ xfs_attr3_leaf_add_work( >> if (tmp) >> entry->flags |= XFS_ATTR_LOCAL; >> if (args->op_flags & XFS_DA_OP_RENAME) { >> - entry->flags |= XFS_ATTR_INCOMPLETE; >> + if (!xfs_hasdelattr(mp)) >> + entry->flags |= XFS_ATTR_INCOMPLETE; >> if ((args->blkno2 == args->blkno) && >> (args->index2 <= args->index)) { >> args->index2++; >> -- >> 2.7.4 >>
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index e4c1b4b..666cc69 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -337,6 +337,7 @@ xfs_attr_set_iter( struct xfs_da_state *state = NULL; int forkoff, error = 0; int retval = 0; + struct xfs_mount *mp = args->dp->i_mount; /* State machine switch */ switch (dac->dela_state) { @@ -470,16 +471,21 @@ xfs_attr_set_iter( * "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. - */ - dac->dela_state = XFS_DAS_FLIP_LFLAG; - trace_xfs_attr_set_iter_return(dac->dela_state, args->dp); - return -EAGAIN; + if (!xfs_hasdelattr(mp)) { + error = xfs_attr3_leaf_flipflags(args); + if (error) + return error; + /* + * Commit the flag value change and start the next trans + * in series. + */ + dac->dela_state = XFS_DAS_FLIP_LFLAG; + trace_xfs_attr_set_iter_return(dac->dela_state, + args->dp); + return -EAGAIN; + } + + /* fallthrough */ case XFS_DAS_FLIP_LFLAG: /* * Dismantle the "old" attribute/value pair by removing a @@ -588,17 +594,21 @@ xfs_attr_set_iter( * 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 - */ - dac->dela_state = XFS_DAS_FLIP_NFLAG; - trace_xfs_attr_set_iter_return(dac->dela_state, args->dp); - return -EAGAIN; + if (!xfs_hasdelattr(mp)) { + error = xfs_attr3_leaf_flipflags(args); + if (error) + goto out; + /* + * Commit the flag value change and start the next trans + * in series + */ + dac->dela_state = XFS_DAS_FLIP_NFLAG; + trace_xfs_attr_set_iter_return(dac->dela_state, + args->dp); + return -EAGAIN; + } + /* fallthrough */ case XFS_DAS_FLIP_NFLAG: /* * Dismantle the "old" attribute/value pair by removing a @@ -1277,7 +1287,6 @@ int xfs_attr_node_addname_work( * 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(args); state->inleaf = 0; error = xfs_da3_node_lookup_int(state, &retval); diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c index 3780141..ec707bd 100644 --- a/fs/xfs/libxfs/xfs_attr_leaf.c +++ b/fs/xfs/libxfs/xfs_attr_leaf.c @@ -1486,7 +1486,8 @@ xfs_attr3_leaf_add_work( if (tmp) entry->flags |= XFS_ATTR_LOCAL; if (args->op_flags & XFS_DA_OP_RENAME) { - entry->flags |= XFS_ATTR_INCOMPLETE; + if (!xfs_hasdelattr(mp)) + entry->flags |= XFS_ATTR_INCOMPLETE; if ((args->blkno2 == args->blkno) && (args->index2 <= args->index)) { args->index2++;
This is a clean up patch that skips the flip flag logic for delayed attr renames. Since the log replay keeps the inode locked, we do not need to worry about race windows with attr lookups. So we can skip over flipping the flag and the extra transaction roll for it Signed-off-by: Allison Henderson <allison.henderson@oracle.com> --- fs/xfs/libxfs/xfs_attr.c | 51 +++++++++++++++++++++++++------------------ fs/xfs/libxfs/xfs_attr_leaf.c | 3 ++- 2 files changed, 32 insertions(+), 22 deletions(-)