| Message ID | 20181011054038.5428-6-lufq.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Some trivail cleanup about dealyed-refs | expand |
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 greedy. Have you characterized what kind of performance impact this have? > 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, >
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.
On 11.10.2018 15:15, Lu Fengqi wrote: > 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. Right I agree with your. As it stands I will expect that if return_bigger is true to specifically return a bigger entry or if nothing is found to return null. IMO this behavior is higher level and belongs to btrfs_delayed_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. I guess it was used during testing but nothing currently sets it. I.e it might make sense to enable it if BTRFS_DEBUG is set.
On Thu, Oct 11, 2018 at 03:28:15PM +0300, Nikolay Borisov wrote: > > 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. > > I guess it was used during testing but nothing currently sets it. I.e it > might make sense to enable it if BTRFS_DEBUG is set. Agreed, the way the scrambling is supposed to be used does not align very well with the typical testing workflow so adding to ti the BTRFS_DEBUG set is ok, unless there are severe performance problems. The part in btrfs_run_delayed_refs would be better hidden in a function similar to btrfs_debug_check_extent_io_range or btrfs_leak_debug_check.
On Thu, Oct 11, 2018 at 03:28:15PM +0300, Nikolay Borisov wrote: > > >On 11.10.2018 15:15, Lu Fengqi wrote: >> 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. > >Right I agree with your. As it stands I will expect that if >return_bigger is true to specifically return a bigger entry or if >nothing is found to return null. IMO this behavior is higher level and This is also exactly what I want. The patch is on the way. >belongs to btrfs_delayed_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. > >I guess it was used during testing but nothing currently sets it. I.e it >might make sense to enable it if BTRFS_DEBUG is set. > Make sense.
On Thu, Oct 11, 2018 at 02:45:04PM +0200, David Sterba wrote: >On Thu, Oct 11, 2018 at 03:28:15PM +0300, Nikolay Borisov wrote: >> > 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. >> >> I guess it was used during testing but nothing currently sets it. I.e it >> might make sense to enable it if BTRFS_DEBUG is set. > >Agreed, the way the scrambling is supposed to be used does not align >very well with the typical testing workflow so adding to ti the >BTRFS_DEBUG set is ok, unless there are severe performance problems. I will add it to the BTRFS_DEBUG set, and test if it has severe performance problems. > >The part in btrfs_run_delayed_refs would be better hidden in a function >similar to btrfs_debug_check_extent_io_range or btrfs_leak_debug_check. Got it.
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) { 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,
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. Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com> --- fs/btrfs/delayed-ref.c | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-)