linux-xfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Allison Henderson <allison.henderson@oracle.com>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH v22 10/16] RFC xfs: Skip flip flags for delayed attrs
Date: Mon, 2 Aug 2021 00:47:23 -0700	[thread overview]
Message-ID: <f8cfd6ed-4cdb-2d68-e41e-e7964c4b5432@oracle.com> (raw)
In-Reply-To: <95b364d5-053a-45cc-1bcb-8a0346b5c324@oracle.com>



On 7/30/21 10:11 PM, Allison Henderson wrote:
> 
> 
> On 7/28/21 12:18 PM, Darrick J. Wong wrote:
>> On Mon, Jul 26, 2021 at 11:20:47PM -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
>>>
>>> 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>
>>
>> /me hates it when he notices things after review... :/
>>
>>> ---
>>>   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 11d8081..eee219c6 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) {
>>> @@ -476,16 +477,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_hasdelattr(mp)) {
>>
>> More nitpicking: this should be gated directly on the log incompat
>> feature check, not the LARP knob...
> I think they're equivelent functionally right now.  Looking forward, if 
> we assume that the knob will one day disappear, it might be a good idea 
> to leave it wrapping any code that would disappear along with it.  Just 
> as a sort of reminder to clean it out.
> 
> So for example, if we ever decide that delayed attrs are just alway on, 
> this whole chunk could just go away with it.  OTOTH, if there is an 
> internal need to run attrs without the logging, it would make sense to 
> check the feature bit here, as this code would still be needed even when 
> the knob is gone.
> 
> So i guess this is a question of what we think we will need in the future?
> 
>>
>>         if (!xfs_sb_version_haslogxattrs(&mp->m_sb)) {
>>
>>> +            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
>>> @@ -587,17 +593,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)) {
>>
>> ...and here...
>>
>>> +            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
>>> @@ -1241,7 +1251,6 @@ 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;
>>
>> Why is this removed from the query arguments?  If we're going to skip
>> the INCOMPLETE flag dance, I would have thought that you'd change the
>> XFS_DAS_CLR_FLAG case to omit xfs_attr_node_addname_clear_incomplete if
>> the logged xattr feature is set?

Sorry, i inadvertently hit send before I finished all the replies.  It 
took me a bit to remember why i ended doing this few months ago:

I don't think you can just skip the clear incomplete.  It does more than 
clear the flag, it calls xfs_attr_node_removename that actually does the 
remove in the case of nodes.  The reason the filter is removed is 
because we would no longer match the attributes we are looking for, and 
so the lookup fails.  Which turns into failures in xfs/067 and xfs/298.

Alternately, I tried simply omitting 
xfs_attr_node_addname_clear_incomplete instead, but I seem to get the 
same failures in the same two tests.

Allison

>>
>>>       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 b910bd2..a9116ee 100644
>>> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
>>> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
>>> @@ -1482,7 +1482,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))
>>
>> Same change here as above.
>>
>> --D
>>
>>> +            entry->flags |= XFS_ATTR_INCOMPLETE;
>>>           if ((args->blkno2 == args->blkno) &&
>>>               (args->index2 <= args->index)) {
>>>               args->index2++;
>>> -- 
>>> 2.7.4
>>>

  reply	other threads:[~2021-08-02  7:47 UTC|newest]

Thread overview: 67+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-27  6:20 [PATCH v22 00/16] Delayed Attributes Allison Henderson
2021-07-27  6:20 ` [PATCH v22 01/16] xfs: allow setting and clearing of log incompat feature flags Allison Henderson
2021-07-27 12:24   ` Chandan Babu R
2021-07-28  9:01     ` Allison Henderson
2021-07-27  6:20 ` [PATCH v22 02/16] xfs: clear log incompat feature bits when the log is idle Allison Henderson
2021-07-27 12:46   ` Chandan Babu R
2021-07-28  9:02     ` Allison Henderson
2021-07-27  6:20 ` [PATCH v22 03/16] xfs: refactor xfs_iget calls from log intent recovery Allison Henderson
2021-07-28 11:54   ` Chandan Babu R
2021-07-30  9:17     ` Allison Henderson
2021-07-27  6:20 ` [PATCH v22 04/16] xfs: Return from xfs_attr_set_iter if there are no more rmtblks to process Allison Henderson
2021-07-27 23:30   ` Darrick J. Wong
2021-07-28  9:01     ` Allison Henderson
2021-07-28 12:18   ` Chandan Babu R
2021-07-30  9:17     ` Allison Henderson
2021-08-09 17:24   ` Darrick J. Wong
2021-07-27  6:20 ` [PATCH v22 05/16] xfs: Add state machine tracepoints Allison Henderson
2021-07-28 13:42   ` Chandan Babu R
2021-07-30  9:17     ` Allison Henderson
2021-07-27  6:20 ` [PATCH v22 06/16] xfs: Rename __xfs_attr_rmtval_remove Allison Henderson
2021-07-29  7:56   ` Chandan Babu R
2021-07-30  9:17     ` Allison Henderson
2021-07-27  6:20 ` [PATCH v22 07/16] xfs: Handle krealloc errors in xlog_recover_add_to_cont_trans Allison Henderson
2021-07-29  8:27   ` Chandan Babu R
2021-07-30  9:17     ` Allison Henderson
2021-07-27  6:20 ` [PATCH v22 08/16] xfs: Set up infrastructure for deferred attribute operations Allison Henderson
2021-07-28  0:56   ` Darrick J. Wong
2021-07-28  9:04     ` Allison Henderson
2021-07-30  4:46   ` Chandan Babu R
2021-07-30  9:17     ` Allison Henderson
2021-07-27  6:20 ` [PATCH v22 09/16] xfs: Implement attr logging and replay Allison Henderson
2021-07-27  9:38   ` Chandan Babu R
2021-07-28  9:01     ` Allison Henderson
2021-07-28  0:39   ` Darrick J. Wong
2021-07-28  9:05     ` Allison Henderson
2021-07-30 12:21   ` Chandan Babu R
2021-08-02  8:33     ` Allison Henderson
2021-07-27  6:20 ` [PATCH v22 10/16] RFC xfs: Skip flip flags for delayed attrs Allison Henderson
2021-07-28 19:18   ` Darrick J. Wong
2021-07-31  5:11     ` Allison Henderson
2021-08-02  7:47       ` Allison Henderson [this message]
2021-07-30 14:40   ` Chandan Babu R
2021-08-02  8:33     ` Allison Henderson
2021-07-27  6:20 ` [PATCH v22 11/16] xfs: Add xfs_attr_set_deferred and xfs_attr_remove_deferred Allison Henderson
2021-07-28 19:24   ` Darrick J. Wong
2021-08-02  8:18     ` Allison Henderson
2021-07-30 14:58   ` Chandan Babu R
2021-08-02  8:33     ` Allison Henderson
2021-07-27  6:20 ` [PATCH v22 12/16] xfs: Remove unused xfs_attr_*_args Allison Henderson
2021-07-28 19:31   ` Darrick J. Wong
2021-08-02  8:11     ` Allison Henderson
2021-08-02  3:26   ` Chandan Babu R
2021-08-02  8:33     ` Allison Henderson
2021-07-27  6:20 ` [PATCH v22 13/16] xfs: Add delayed attributes error tag Allison Henderson
2021-08-02  3:27   ` Chandan Babu R
2021-08-02  8:33     ` Allison Henderson
2021-07-27  6:20 ` [PATCH v22 14/16] xfs: Add delattr mount option Allison Henderson
2021-07-28  0:47   ` Darrick J. Wong
2021-07-28  2:13     ` Dave Chinner
2021-07-28  9:05       ` Allison Henderson
2021-07-28  9:02     ` Allison Henderson
2021-07-27  6:20 ` [PATCH v22 15/16] xfs: Merge xfs_delattr_context into xfs_attr_item Allison Henderson
2021-08-02  3:27   ` Chandan Babu R
2021-08-02  8:33     ` Allison Henderson
2021-07-27  6:20 ` [PATCH v22 16/16] xfs: Add helper function xfs_attr_leaf_addname Allison Henderson
2021-07-28 19:52   ` Darrick J. Wong
2021-08-02  8:18     ` Allison Henderson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f8cfd6ed-4cdb-2d68-e41e-e7964c4b5432@oracle.com \
    --to=allison.henderson@oracle.com \
    --cc=djwong@kernel.org \
    --cc=linux-xfs@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).