From: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
To: Nikolay Borisov <nborisov@suse.com>
Cc: <linux-btrfs@vger.kernel.org>
Subject: Re: [PATCH 5/6] btrfs: simplify btrfs_select_ref_head and cleanup some local variables
Date: Thu, 11 Oct 2018 20:15:58 +0800 [thread overview]
Message-ID: <20181011121558.GF5899@fnst.localdomain> (raw)
In-Reply-To: <e5bb27d0-fea5-30c6-afd4-aea2eae1871e@suse.com>
On Thu, Oct 11, 2018 at 09:40:52AM +0300, Nikolay Borisov wrote:
>
>
>On 11.10.2018 08:40, Lu Fengqi wrote:
>> If the return value of find_ref_head() is NULL, the only possibility is
>> that delayed_refs' head ref rbtree is empty. Hence, the second
>> find_ref_head() is pointless.
>> > Besides, the local variables loop and start are unnecessary, just remove
>> them.
>
>So the objective of that function is to get a reference to the first
>delayed head which is not processed. This is done by essentially keeping
>track of the last range that was processed in
>delayed_refs->run_delayed_start
>>
>> Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
>> ---
>> fs/btrfs/delayed-ref.c | 17 +++--------------
>> 1 file changed, 3 insertions(+), 14 deletions(-)
>>
>> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
>> index 885581852bea..2726d2fb4bbe 100644
>> --- a/fs/btrfs/delayed-ref.c
>> +++ b/fs/btrfs/delayed-ref.c
>> @@ -354,20 +354,11 @@ struct btrfs_delayed_ref_head *
>> btrfs_select_ref_head(struct btrfs_delayed_ref_root *delayed_refs)
>> {
>> struct btrfs_delayed_ref_head *head;
>> - u64 start;
>> - bool loop = false;
>>
>> again:
>> - start = delayed_refs->run_delayed_start;
>> - head = find_ref_head(delayed_refs, start, 1);
>> - if (!head && !loop) {
>> + head = find_ref_head(delayed_refs, delayed_refs->run_delayed_start, 1);
>> + if (!head) {
>> delayed_refs->run_delayed_start = 0;
>> - start = 0;
>> - loop = true;
>> - head = find_ref_head(delayed_refs, start, 1);
>> - if (!head)
>> - return NULL;
>> - } else if (!head && loop) {
>
>I believe this will have a negative impact since it actually will
>prevent finding a head which was added BEFORE the last processed head.
>So when a ref head is selected in btrfs_obtain_ref_head then the
>delayed_refs->lock is dropped and the given head is locked and
>delayed_refs->run_delayed_start points to the end of the selected range
>that the head represents. At this point it's possible that another
>thread modifies a different range which is before the one we have
>selected so graphically it will be something like:
>
>
>---[HEAD2]----->[HEAD1]------
>0 N
>
>Where HEAD1 is the head returned from first invocation of
>btrfs_obtain_ref_head. Once btrfs_obtain_ref_head is called the 2nd
>time it will not find HEAD2 so will just reset run_delayed_start to 0
>and return. So it will be up to another run of the delayed refs to
>actually find head2. Essentially you made btrfs_obtain_ref_head less
Not exactly. In fact, find_ref_head hides such a logic. When
return_bigger is set, if there is no larger entry to return, the first
entry will be returned. Please see the comment I add in the PATCH 6.
Hence, the 2nd invocation of btrfs_obtain_ref_head still will return
HEAD2. There is no functional change here.
However, your question makes me consider whether such hidden logic
should be extracted from find_ref_head to btrfs_select_ref_head.
>greedy. Have you characterized what kind of performance impact this have?
I noticed that there is a macro called SCRAMBLE_DELAYED_REFS in the
extent-tree.c. I am a bit curious whether it has been forgotten by
everyone, I have not found any test results about its performance impact.
--
Thanks,
Lu
>
>
>
>
>> return NULL;
>> }
>>
>> @@ -376,11 +367,9 @@ btrfs_select_ref_head(struct btrfs_delayed_ref_root *delayed_refs)
>>
>> node = rb_next(&head->href_node);
>> if (!node) {
>> - if (loop)
>> + if (delayed_refs->run_delayed_start == 0)
>> return NULL;
>> delayed_refs->run_delayed_start = 0;
>> - start = 0;
>> - loop = true;
>> goto again;
>> }
>> head = rb_entry(node, struct btrfs_delayed_ref_head,
>>
>
>
next prev parent reply other threads:[~2018-10-11 12:17 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-11 5:40 [PATCH 0/6] Some trivail cleanup about dealyed-refs Lu Fengqi
2018-10-11 5:40 ` [PATCH 1/6] btrfs: delayed-ref: pass delayed_refs directly to btrfs_select_ref_head() Lu Fengqi
2018-10-11 5:40 ` [PATCH 2/6] btrfs: delayed-ref: pass delayed_refs directly to btrfs_delayed_ref_lock() Lu Fengqi
2018-10-11 5:40 ` [PATCH 3/6] btrfs: remove fs_info from btrfs_check_space_for_delayed_refs Lu Fengqi
2018-10-11 5:40 ` [PATCH 4/6] btrfs: remove fs_info from btrfs_should_throttle_delayed_refs Lu Fengqi
2018-10-11 5:40 ` [PATCH 5/6] btrfs: simplify btrfs_select_ref_head and cleanup some local variables Lu Fengqi
2018-10-11 6:40 ` Nikolay Borisov
2018-10-11 12:15 ` Lu Fengqi [this message]
2018-10-11 12:28 ` Nikolay Borisov
2018-10-11 12:45 ` David Sterba
2018-10-15 2:32 ` Lu Fengqi
2018-10-15 2:09 ` Lu Fengqi
2018-10-11 5:40 ` [PATCH 6/6] btrfs: switch return_bigger to bool in find_ref_head Lu Fengqi
2018-10-11 6:41 ` [PATCH 0/6] Some trivail cleanup about dealyed-refs Nikolay Borisov
2018-10-11 11:51 ` David Sterba
2018-10-15 2:39 ` Lu Fengqi
2018-10-15 13:26 ` David Sterba
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=20181011121558.GF5899@fnst.localdomain \
--to=lufq.fnst@cn.fujitsu.com \
--cc=linux-btrfs@vger.kernel.org \
--cc=nborisov@suse.com \
/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).