| Message ID | 20210824224434.968720-6-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Log Attribute Replay | expand |
On 25 Aug 2021 at 04:14, 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 > > RFC: In the last review, folks asked for some performance analysis, so I > did a few perf captures with and with out this patch. What I found was > that there wasnt very much difference at all between having the patch or > not having it. Of the time we do spend in the affected code, the > percentage is small. Most of the time we spend about %0.03 of the time > in this function, with or with out the patch. Occasionally we get a > 0.02%, though not often. So I think this starts to challenge needing > this patch at all. This patch was requested some number of reviews ago, > be perhaps in light of the findings, it may no longer be of interest. > > 0.03% 0.00% fsstress [xfs] [k] xfs_attr_set_iter > > Keep it or drop it? Looks good to me. Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com> > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > Reviewed-by: Darrick J. Wong <djwong@kernel.org> > --- > fs/xfs/libxfs/xfs_attr.c | 54 +++++++++++++++++++++-------------- > fs/xfs/libxfs/xfs_attr_leaf.c | 3 +- > 2 files changed, 35 insertions(+), 22 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index dfff81024e46..fce67c717be2 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -355,6 +355,7 @@ xfs_attr_set_iter( > struct xfs_inode *dp = args->dp; > struct xfs_buf *bp = NULL; > int forkoff, error = 0; > + struct xfs_mount *mp = args->dp->i_mount; > > /* State machine switch */ > switch (dac->dela_state) { > @@ -477,16 +478,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) > - 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_has_larp(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 > @@ -589,17 +595,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_has_larp(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 > @@ -1236,6 +1246,7 @@ xfs_attr_node_addname_clear_incomplete( > { > struct xfs_da_args *args = dac->da_args; > struct xfs_da_state *state = NULL; > + struct xfs_mount *mp = args->dp->i_mount; > int retval = 0; > int error = 0; > > @@ -1243,7 +1254,8 @@ xfs_attr_node_addname_clear_incomplete( > * 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; > + if (!xfs_has_larp(mp)) > + 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 e1d11e314228..a0a352bdea59 100644 > --- a/fs/xfs/libxfs/xfs_attr_leaf.c > +++ b/fs/xfs/libxfs/xfs_attr_leaf.c > @@ -1487,7 +1487,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_has_larp(mp)) > + entry->flags |= XFS_ATTR_INCOMPLETE; > if ((args->blkno2 == args->blkno) && > (args->index2 <= args->index)) { > args->index2++;
On 8/30/21 3:15 AM, Chandan Babu R wrote: > On 25 Aug 2021 at 04:14, 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 >> >> RFC: In the last review, folks asked for some performance analysis, so I >> did a few perf captures with and with out this patch. What I found was >> that there wasnt very much difference at all between having the patch or >> not having it. Of the time we do spend in the affected code, the >> percentage is small. Most of the time we spend about %0.03 of the time >> in this function, with or with out the patch. Occasionally we get a >> 0.02%, though not often. So I think this starts to challenge needing >> this patch at all. This patch was requested some number of reviews ago, >> be perhaps in light of the findings, it may no longer be of interest. >> >> 0.03% 0.00% fsstress [xfs] [k] xfs_attr_set_iter >> >> Keep it or drop it? > > Looks good to me. > > Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com> Ok, thank you! Allison > >> >> Signed-off-by: Allison Henderson <allison.henderson@oracle.com> >> Reviewed-by: Darrick J. Wong <djwong@kernel.org> >> --- >> fs/xfs/libxfs/xfs_attr.c | 54 +++++++++++++++++++++-------------- >> fs/xfs/libxfs/xfs_attr_leaf.c | 3 +- >> 2 files changed, 35 insertions(+), 22 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index dfff81024e46..fce67c717be2 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -355,6 +355,7 @@ xfs_attr_set_iter( >> struct xfs_inode *dp = args->dp; >> struct xfs_buf *bp = NULL; >> int forkoff, error = 0; >> + struct xfs_mount *mp = args->dp->i_mount; >> >> /* State machine switch */ >> switch (dac->dela_state) { >> @@ -477,16 +478,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) >> - 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_has_larp(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 >> @@ -589,17 +595,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_has_larp(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 >> @@ -1236,6 +1246,7 @@ xfs_attr_node_addname_clear_incomplete( >> { >> struct xfs_da_args *args = dac->da_args; >> struct xfs_da_state *state = NULL; >> + struct xfs_mount *mp = args->dp->i_mount; >> int retval = 0; >> int error = 0; >> >> @@ -1243,7 +1254,8 @@ xfs_attr_node_addname_clear_incomplete( >> * 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; >> + if (!xfs_has_larp(mp)) >> + 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 e1d11e314228..a0a352bdea59 100644 >> --- a/fs/xfs/libxfs/xfs_attr_leaf.c >> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c >> @@ -1487,7 +1487,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_has_larp(mp)) >> + entry->flags |= XFS_ATTR_INCOMPLETE; >> if ((args->blkno2 == args->blkno) && >> (args->index2 <= args->index)) { >> args->index2++; > >
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index dfff81024e46..fce67c717be2 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -355,6 +355,7 @@ xfs_attr_set_iter( struct xfs_inode *dp = args->dp; struct xfs_buf *bp = NULL; int forkoff, error = 0; + struct xfs_mount *mp = args->dp->i_mount; /* State machine switch */ switch (dac->dela_state) { @@ -477,16 +478,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) - 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_has_larp(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 @@ -589,17 +595,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_has_larp(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 @@ -1236,6 +1246,7 @@ xfs_attr_node_addname_clear_incomplete( { struct xfs_da_args *args = dac->da_args; struct xfs_da_state *state = NULL; + struct xfs_mount *mp = args->dp->i_mount; int retval = 0; int error = 0; @@ -1243,7 +1254,8 @@ xfs_attr_node_addname_clear_incomplete( * 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; + if (!xfs_has_larp(mp)) + 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 e1d11e314228..a0a352bdea59 100644 --- a/fs/xfs/libxfs/xfs_attr_leaf.c +++ b/fs/xfs/libxfs/xfs_attr_leaf.c @@ -1487,7 +1487,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_has_larp(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 RFC: In the last review, folks asked for some performance analysis, so I did a few perf captures with and with out this patch. What I found was that there wasnt very much difference at all between having the patch or not having it. Of the time we do spend in the affected code, the percentage is small. Most of the time we spend about %0.03 of the time in this function, with or with out the patch. Occasionally we get a 0.02%, though not often. So I think this starts to challenge needing this patch at all. This patch was requested some number of reviews ago, be perhaps in light of the findings, it may no longer be of interest. 0.03% 0.00% fsstress [xfs] [k] xfs_attr_set_iter Keep it or drop it? Signed-off-by: Allison Henderson <allison.henderson@oracle.com> Reviewed-by: Darrick J. Wong <djwong@kernel.org> --- fs/xfs/libxfs/xfs_attr.c | 54 +++++++++++++++++++++-------------- fs/xfs/libxfs/xfs_attr_leaf.c | 3 +- 2 files changed, 35 insertions(+), 22 deletions(-)