Yield periodically when rebuilding L2ARC. #11116

amotin · 2020-10-27T02:18:52Z

Motivation and Context

L2ARC devices of several terabytes filled with 4KB blocks may take 15 minutes to rebuild. Due to the way L2ARC log reading is implemented it is quite likely that for all that time rebuild thread will never sleep. At least on FreeBSD kernel threads have absolute priority and can not be preempted by threads with lower priorities. If some thread is also bound to that specific CPU it may not get any CPU time for all the 15 minutes.

Description

This patch solves the issue by adding cond_resched() call after processing of every log block.

How Has This Been Tested?

The patch has been tested on FreeBSD by filling large L2ARC with 4KB blocks, re-importing the pool to start the rebuild and attempt to read dev.cpu sysctl tree. Without the patch some of the sysctls there got stuck trying to bind to respective CPU cores. With the patch the sysctl delays are barely noticeable.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the OpenZFS code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

codecov · 2020-10-27T10:34:42Z

Codecov Report

Merging #11116 into master will decrease coverage by 0.03%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##           master   #11116      +/-   ##
==========================================
- Coverage   79.81%   79.77%   -0.04%     
==========================================
  Files         398      398              
  Lines      125754   125758       +4     
==========================================
- Hits       100367   100323      -44     
- Misses      25387    25435      +48

Flag	Coverage Δ
#kernel	`80.44% <100.00%> (-0.04%)`	⬇️
#user	`65.93% <0.00%> (-0.26%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
module/zfs/arc.c	`89.55% <60.00%> (-0.20%)`	⬇️
cmd/zdb/zdb_il.c	`30.86% <0.00%> (-24.08%)`	⬇️
module/zfs/vdev_raidz.c	`89.68% <0.00%> (-4.24%)`	⬇️
module/zfs/spa_errlog.c	`90.83% <0.00%> (-3.06%)`	⬇️
module/os/linux/spl/spl-kmem-cache.c	`88.57% <0.00%> (-1.51%)`	⬇️
module/os/linux/zfs/zio_crypt.c	`79.97% <0.00%> (-1.11%)`	⬇️
module/os/linux/zfs/zvol_os.c	`87.36% <0.00%> (-0.89%)`	⬇️
module/icp/api/kcf_cipher.c	`15.76% <0.00%> (-0.83%)`	⬇️
module/zfs/zvol.c	`84.39% <0.00%> (-0.68%)`	⬇️
module/zfs/vdev_rebuild.c	`96.73% <0.00%> (-0.66%)`	⬇️
... and 53 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c810ac...f493a4b. Read the comment docs.

ghost

Spotted a typo, but great to get to the bottom of this issue 👍

man/man5/zfs-module-parameters.5

module/zfs/arc.c

amotin · 2020-10-27T13:19:11Z

@gamanakis You've done prefetching too good. ;)

gamanakis · 2020-10-27T14:26:37Z

@amotin I am glad you figured it out. Looks good to me!

mjguzik · 2020-10-27T16:49:26Z

Have you tested this with calling maybe_yield instead?

amotin · 2020-10-27T17:16:28Z

Have you tested this with calling maybe_yield instead?

I was thinking about something like this, but missed the priority trick this particular function does. I've tested it and it really seems to work. Thank you very much!

Do you think it should be done instead or in addition to? L2ARC rebuild has nowhere to hurry really. As I understand the process still remains at the top user-space priority.

Do you know maybe_yield()'s counterpart on Linux?

mjguzik · 2020-10-27T17:22:08Z

Used to be cond_resched or so. Note I'm not confident they have equivalent semantics, but maybe_yield should be fine on FreeBSD at least.

behlendorf · 2020-10-27T17:26:55Z

I was going to suggest trying the same thing. You can use cond_resched() which is the Linux function name, there are existing wrappers for both FreeBSD kern_yield(PRI_USER) and userland sched_yield(). We call cond_resched() at the end of arc_evict_state_impl for this same reason.

mjguzik · 2020-10-27T17:35:47Z

Now that you mention it I see 2 definitions of cond_resched which may concern the FreeBSD port:

the zfs repo:

include/os/freebsd/zfs/sys/zfs_context_os.h (lists it twice btw):
#define cond_resched() kern_yield(PRI_USER)

include/sys/zfs_context.h:
#define cond_resched() sched_yield()

Apart from that there is one in the Linux API compat in FreeBSD: compat/linuxkpi/common/include/linux/sched.h
#define cond_resched() do { if (!cold) sched_relinquish(curthread); } while (0)

I don't know what ends up being used in this mess (is hte zfs_context.h header for userspace?), I presume for kernel it's the zfs_context.os.h one.

That said, maybe_yield definitely needs to be tested to give the hoped for result (it should). Past that, looks like the patch should
invoke cond_resched and make sure it does maybe_yield on FreeBSD while removing spurious definitions at least from the port.

tl;dr please try cond_resched with this:

diff --git a/include/os/freebsd/zfs/sys/zfs_context_os.h b/include/os/freebsd/zfs/sys/zfs_context_os.h
index 3118c7d96..2962689bb 100644
--- a/include/os/freebsd/zfs/sys/zfs_context_os.h
+++ b/include/os/freebsd/zfs/sys/zfs_context_os.h
@@ -41,7 +41,7 @@
 #include <sys/ccompat.h>
 #include <linux/types.h>
 
-#define        cond_resched()          kern_yield(PRI_USER)
+#define        cond_resched()          maybe_yield()
 #define        uio_prefaultpages(size, uio) (0)
 
 #define        taskq_create_sysdc(a, b, d, e, p, dc, f) \
@@ -57,7 +57,6 @@
 #define        tsd_set(key, value)     osd_thread_set(curthread, (key), (value))
 #define        fm_panic        panic
 
-#define        cond_resched()          kern_yield(PRI_USER)
 extern int zfs_debug_level;
 extern struct mtx zfs_debug_mtx;
 #define        ZFS_LOG(lvl, ...) do {   \

amotin · 2020-10-27T17:39:55Z

Thank you Brian. I've added that macro call.

Though I see on FreeBSD it maps into kern_yield(PRI_USER), that is more aggressive than maybe_yield(). It should not be a problem now, but maybe_yield() may be easier to use in some scenarios where the rate is unknown and switch is not required by algorithm.

amotin · 2020-10-27T17:46:42Z

@mjguzik I've tested the maybe_yield() already in L2ARC rebuild context and it works just fine, and since the call period is already about 1ms I don't think there is a big deal which one to call. On the other side I see number of other cond_resched() calls in a tight loops in the dnode code, and there maybe_yield() would be a waste of CPU time. On the third side arc_evict_state_impl() I think could really benefit from maybe_yield(), since the loop there should be pretty fast despite the batching.

amotin · 2020-10-29T18:42:01Z

Any more thoughts/objections about this?

mjguzik · 2020-10-29T18:49:28Z

The idiom in the FreeBSD kernel as far as I know is to maybe_yield. That's what the vfs layer is doing fwiw and quick grep reveals other places like the vm. With this in mind I think maybe_yield should be the default on FreeBSD and only deviated from with a good reason, which is part of why I asked if something regressed with using that instead of delay.

Still, I'm not going to insist on one way or the other.

amotin · 2020-10-29T18:53:31Z

@mjguzik I just wanted to say that we may need two separate primitives in OpenZFS due to semantic aspects I've described. It is unrelated to this change, since already existing code would benefit from having two.

For this PR my only question is whether leave the delay() call or somebody think it is overkill?

mjguzik · 2020-10-29T18:56:10Z

The delay thing is what I argued is likely slightly pessimal if maybe_yield is employed. Iow, imo it should be removed unless a problem can be demonstrated.

L2ARC devices of several terabytes filled with 4KB blocks may take 15 minutes to rebuild. Due to the way L2ARC log reading is implemented it is quite likely that for all that time rebuild thread will never sleep. At least on FreeBSD kernel threads have absolute priority and can not be preempted by threads with lower priorities. If some thread is also bound to that specific CPU it may not get any CPU time for all the 15 minutes. Signed-off-by: Alexander Motin <mav@FreeBSD.org>

amotin · 2020-10-29T19:16:02Z

OK, reduced to one-liner.

unless a problem can be demonstrated.

Probably not on a big system, at least it does not block sysctls for me any more as it was. I was thinking about some dual-core system having several L2ARC device, but in that case default throttling would be too low, so all hope it still on the scheduler.

L2ARC devices of several terabytes filled with 4KB blocks may take 15 minutes to rebuild. Due to the way L2ARC log reading is implemented it is quite likely that for all that time rebuild thread will never sleep. At least on FreeBSD kernel threads have absolute priority and can not be preempted by threads with lower priorities. If some thread is also bound to that specific CPU it may not get any CPU time for all the 15 minutes. Reviewed-by: Cedric Berger <cedric@precidata.com> Reviewed-by: Ryan Moeller <freqlabs@FreeBSD.org> Reviewed-by: George Amanakis <gamanakis@gmail.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Alexander Motin <mav@FreeBSD.org> Closes openzfs#11116

L2ARC devices of several terabytes filled with 4KB blocks may take 15 minutes to rebuild. Due to the way L2ARC log reading is implemented it is quite likely that for all that time rebuild thread will never sleep. At least on FreeBSD kernel threads have absolute priority and can not be preempted by threads with lower priorities. If some thread is also bound to that specific CPU it may not get any CPU time for all the 15 minutes. Reviewed-by: Cedric Berger <cedric@precidata.com> Reviewed-by: Ryan Moeller <freqlabs@FreeBSD.org> Reviewed-by: George Amanakis <gamanakis@gmail.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Alexander Motin <mav@FreeBSD.org> Closes #11116

L2ARC devices of several terabytes filled with 4KB blocks may take 15 minutes to rebuild. Due to the way L2ARC log reading is implemented it is quite likely that for all that time rebuild thread will never sleep. At least on FreeBSD kernel threads have absolute priority and can not be preempted by threads with lower priorities. If some thread is also bound to that specific CPU it may not get any CPU time for all the 15 minutes. Reviewed-by: Cedric Berger <cedric@precidata.com> Reviewed-by: Ryan Moeller <freqlabs@FreeBSD.org> Reviewed-by: George Amanakis <gamanakis@gmail.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Alexander Motin <mav@FreeBSD.org> Closes openzfs#11116

amotin added the Status: Code Review Needed Ready for review and testing label Oct 27, 2020

amotin requested a review from a user October 27, 2020 02:18

amotin self-assigned this Oct 27, 2020

ghost approved these changes Oct 27, 2020

View reviewed changes

man/man5/zfs-module-parameters.5 Outdated Show resolved Hide resolved

ceedriic approved these changes Oct 27, 2020

View reviewed changes

man/man5/zfs-module-parameters.5 Outdated Show resolved Hide resolved

module/zfs/arc.c Outdated Show resolved Hide resolved

amotin force-pushed the l2rb_throttle branch 2 times, most recently from 1e55718 to bc40055 Compare October 27, 2020 13:15

amotin force-pushed the l2rb_throttle branch from bc40055 to e1952e4 Compare October 27, 2020 14:26

gamanakis approved these changes Oct 27, 2020

View reviewed changes

amotin force-pushed the l2rb_throttle branch from e1952e4 to 608ad5e Compare October 27, 2020 17:33

amotin force-pushed the l2rb_throttle branch from 608ad5e to f493a4b Compare October 29, 2020 19:13

amotin changed the title ~~Sleep periodically when rebuilding L2ARC.~~ Yield periodically when rebuilding L2ARC. Oct 29, 2020

behlendorf approved these changes Oct 30, 2020

View reviewed changes

behlendorf removed the Status: Code Review Needed Ready for review and testing label Oct 30, 2020

behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Oct 30, 2020

behlendorf merged commit 1199c3e into openzfs:master Oct 30, 2020

amotin deleted the l2rb_throttle branch August 24, 2021 20:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Yield periodically when rebuilding L2ARC. #11116

Yield periodically when rebuilding L2ARC. #11116

amotin commented Oct 27, 2020 •

edited

Loading

codecov bot commented Oct 27, 2020 •

edited

Loading

ghost left a comment

amotin commented Oct 27, 2020

gamanakis commented Oct 27, 2020

mjguzik commented Oct 27, 2020

amotin commented Oct 27, 2020 •

edited

Loading

mjguzik commented Oct 27, 2020 •

edited

Loading

behlendorf commented Oct 27, 2020

mjguzik commented Oct 27, 2020 •

edited

Loading

amotin commented Oct 27, 2020

amotin commented Oct 27, 2020

amotin commented Oct 29, 2020

mjguzik commented Oct 29, 2020

amotin commented Oct 29, 2020

mjguzik commented Oct 29, 2020

amotin commented Oct 29, 2020

Yield periodically when rebuilding L2ARC. #11116

Yield periodically when rebuilding L2ARC. #11116

Conversation

amotin commented Oct 27, 2020 • edited Loading

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

codecov bot commented Oct 27, 2020 • edited Loading

Codecov Report

ghost left a comment

Choose a reason for hiding this comment

amotin commented Oct 27, 2020

gamanakis commented Oct 27, 2020

mjguzik commented Oct 27, 2020

amotin commented Oct 27, 2020 • edited Loading

mjguzik commented Oct 27, 2020 • edited Loading

behlendorf commented Oct 27, 2020

mjguzik commented Oct 27, 2020 • edited Loading

amotin commented Oct 27, 2020

amotin commented Oct 27, 2020

amotin commented Oct 29, 2020

mjguzik commented Oct 29, 2020

amotin commented Oct 29, 2020

mjguzik commented Oct 29, 2020

amotin commented Oct 29, 2020

amotin commented Oct 27, 2020 •

edited

Loading

codecov bot commented Oct 27, 2020 •

edited

Loading

amotin commented Oct 27, 2020 •

edited

Loading

mjguzik commented Oct 27, 2020 •

edited

Loading

mjguzik commented Oct 27, 2020 •

edited

Loading