ARC reads from pools without an l2arc should not be accounted as l2arc misses #10921

adamdmoss · 2020-09-12T00:12:24Z

The other half of the fix for issue #10913 ; this PR only eliminates l2 arcstat mis-counting for pools without l2arcs. (Versus the intersecting-but-separate PR #10914 which only ignores misses from datasets with secondarycache!=all)

Note, this change doesn't affect situations where all the pools on a system have an l2arc or all the pools on a system don't have l2arc; it only changes the mixed case.

Motivation and Context

IMHO the current l2_misses accounting behavior is a bug; it treats all reads to pools without a configured l2arc as an l2arc miss, IFF there is at least one other pool on the system which does have an l2arc configured.

This makes it extremely hard to tune for an improved l2arc hit/miss ratio because this ratio will be modulated by reads from pools which do not (and should not) have l2arc devices; its upper limit will depend on the ratio of reads from l2arc'd pools and non-l2arc'd pools.

Description

This PR prevents ARC reads affecting l2arc stats (n.b. l2_misses is the only relevant one) where the target spa doesn't have an l2arc.

Note I've added a couple of paragraphs of hand-wringing around arc.c:6170 to clarify(?) exactly what's being checked for / ignored for the sake of cheapness.

How Has This Been Tested?

I've tested on a mix of pools where some have l2arc and some do not. L2 hits and misses are accounted correctly when reading from l2arc-backed pools, L2 misses (and hits) do not affect accounting when reading from non-l2arc-backed pools. An upper limit of ~100% l2_hits/l2_misses ratio is attainable with this change regardless of traffic on pools which do not use l2arc.

I've tested the behavior of secondarycache=... and it remains unaffected. (That possibly-more-controversial change is covered by PR #10914 )

Ubuntu 18.04.05, Ubuntu HWE kernel 5.4.0-42-generic, git zfs zfs-2.0-release @ 8e7fe49 + PR cherrypick.

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.

behlendorf · 2020-09-14T17:11:39Z

@gamanakis would you mind looking at this.

gamanakis · 2020-09-15T02:31:48Z

module/zfs/arc.c

+		/*
+		 * Check if the spa even has l2 configured;
+		 *
+		 * NOTE #1: not holding spa_namespace_lock -


We shouldn't need holding spa_namespace_lock because we are not making any changes to the pool configuration here.
We lock out L2ARC device removal with spa_config_tryenter() a few lines above. I think you can remove NOTE#1 entirely.

Good - I was hoping that was the case, the NOTEs were articulate my concerns so I could be corrected. :D

gamanakis · 2020-09-15T02:32:39Z

module/zfs/arc.c

+		 * only affects accounting.
+		 */
+		const boolean_t spa_has_l2 =
+		    l2arc_ndev != 0 &&


This line can be moved to the previous one.

gamanakis · 2020-09-15T02:33:40Z

module/zfs/arc.c

@@ -6265,17 +6285,26 @@ arc_read(zio_t *pio, spa_t *spa, const blkptr_t *bp,
 				spa_config_exit(spa, SCL_L2ARC, vd);
 			}
 		} else {
+


This empty line is not needed here.

module/zfs/arc.c

gamanakis · 2020-09-15T02:52:57Z

@adamdmoss Thank you for this. I left comments in the code. Since you mention you have test-workflow in the description it would be worthwhile to add a test in tests/zfs-tests/tests/functional/l2arc.

adamdmoss · 2020-09-18T19:37:18Z

Thanks for the review - will address feedback soon.

richardelling · 2020-09-19T18:01:29Z

It is a bit unfortunate that @gamanakis and @adamdmoss were not at the OpenZFS Leadership meeting on 16-Sep-2020. The team had a good discussion around per-pool ARC statistics and discussed several ways to implement them. See https://www.youtube.com/watch?v=fVcNnFHDwHY beginning at 5:10.

adamdmoss · 2020-09-19T18:07:45Z

I believe I've addressed all the review feedback @gamanakis - cheers.

adamdmoss · 2020-09-19T18:17:07Z

n.b. as the reporter of #10913 I'd be happy enough to call that closed with just with PR; I don't feel very strongly about the alternative solution/workaround in #10914

gamanakis · 2020-09-19T23:55:53Z

@richardelling Thank you for triggering this discussion, interesting indeed. I will take a look into the different types of stats mentioned, it will be useful in the case of L2ARC as you pointed out.
Regarding the present PR, I think it is a step in the right direction.

behlendorf

Thanks for addressing this case. I'd also recommend we add a new test case to tests/zfs-tests/functional/l2arc

adamdmoss · 2020-09-21T17:24:14Z

I'd like to add a test case, it'll take a little while again due to the day job; is there a doc out there about running the test suite locally or should I rely on the upstream buildbot?

gmelikov · 2020-09-21T17:36:37Z

@adamdmoss you can run zfs-tests via:

local run https://github.com/openzfs/zfs/tree/master/tests (see README)
(pre-pre-alpha) zfs-tests run via github actions - clone this changes https://github.com/gmelikov/zfs/blob/test/.github/workflows/tests.yml , enable github actions on your fork, push changes, github will start a job on your fork
just rely on buildbot

adamdmoss · 2020-09-21T17:46:52Z

Thanks - I'm scared of borking my local system since I don't have multiple scratch disks, keep meaning to set up a VM for zfs dev. The github action thing is quite intriguing!

gmelikov · 2020-09-21T18:11:43Z

Sorry, I may be more precise, you should use VM on local run, zfs-tests may easily kill something.

gamanakis · 2020-09-21T18:29:09Z

My workflow for running a part of the ZTS suite (in a VM) is the following:

compile: ./autogen.sh; ./configure --enable-debug --enable-debuginfo --enable-debug-kmem --enable-debug-kmem-tracking; make -j8
load modules: sudo ./scripts/zfs.sh -v
run part of ZTS: ./scripts/zfs-tests.sh -T l2arc
unload modules: sudo ./scripts/zfs.sh -vu

Take a look in the tests in the tests/zfs-tests/tests/functional/l2arc directory (this is where the test for this PR should go).

adamdmoss · 2020-09-26T07:52:12Z

(sorry for a bit of churn - github action in my repo was a very mixed success so I'm leaning on buildbot while I iterate on the new test)

Resolves openzfs#10921 Includes new test - l2arc_l2miss_pos.ksh Signed-off-by: Adam Moss <[email protected]>

adamdmoss · 2020-09-29T19:22:23Z

The new test passes on CentOS8 now, FWIW, Any idea on the 'random' other failing tests? If they're relevant I'll fix them, but it looks unlikely on a cursory look.

Is there a test dashboard that'll let me visualize if there's a pattern?

adamdmoss · 2020-09-29T19:24:27Z

Ahh, I see a strong correlation with commonly-failing tests from here http://build.zfsonlinux.org/known-issues.html

tests/zfs-tests/tests/functional/l2arc/l2arc_l2miss_pos.ksh

behlendorf · 2020-09-29T20:46:25Z

If they're relevant I'll fix them

I've gone ahead and resubmitted those builds. It looks like they were caused by an issue with the CI. I suggest focusing on the test case you're adding.

Includes new test - l2arc_l2miss_pos.ksh Signed-off-by: Adam Moss <[email protected]> Closes openzfs#10921

adamdmoss · 2020-09-30T10:37:24Z

buildbot/Fedora 32 x86_64 (TEST) failure appears unrelated - failing to compile!

adamdmoss · 2020-10-01T01:22:57Z

Looks like all test platforms are succeeding apart from Fedora which doesn't compile (unrelated), and CentOS which is failing a lot of stuff (but not this test).

tests/zfs-tests/tests/functional/l2arc/l2arc_l2miss_pos.ksh

Includes new test - l2arc_l2miss_pos.ksh Signed-off-by: Adam Moss <[email protected]> Closes openzfs#10921

tests/zfs-tests/tests/functional/l2arc/l2arc_l2miss_pos.ksh

Includes new test - l2arc_l2miss_pos.ksh Signed-off-by: Adam Moss <[email protected]> Closes openzfs#10921

adamdmoss · 2020-10-20T16:41:06Z

(ping)

gamanakis · 2020-10-20T16:54:04Z

Looks good to me!

The current l2_misses accounting behavior treats all reads to pools without a configured l2arc as an l2arc miss, IFF there is at least one other pool on the system which does have an l2arc configured. This makes it extremely hard to tune for an improved l2arc hit/miss ratio because this ratio will be modulated by reads from pools which do not (and should not) have l2arc devices; its upper limit will depend on the ratio of reads from l2arc'd pools and non-l2arc'd pools. This PR prevents ARC reads affecting l2arc stats (n.b. l2_misses is the only relevant one) where the target spa doesn't have an l2arc. Includes new test - l2arc_l2miss_pos.ksh Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: George Amanakis <[email protected]> Signed-off-by: Adam Moss <[email protected]> Closes openzfs#10921

adamdmoss force-pushed the pr/l2checkmiss branch 3 times, most recently from 434ceb2 to fa1c0c2 Compare September 12, 2020 03:28

behlendorf added the Status: Code Review Needed Ready for review and testing label Sep 12, 2020

gamanakis reviewed Sep 15, 2020

View reviewed changes

module/zfs/arc.c Show resolved Hide resolved

adamdmoss force-pushed the pr/l2checkmiss branch from fa1c0c2 to 640d4a1 Compare September 19, 2020 18:06

adamdmoss mentioned this pull request Sep 19, 2020

draft: Reads from secondarycache=none datasets shouldn't count as l2arc misses. #10914

Closed

11 tasks

behlendorf approved these changes Sep 21, 2020

View reviewed changes

adamdmoss added a commit to adamdmoss/zfs that referenced this pull request Sep 26, 2020

first stab at test for openzfs#10921

6331b13

adamdmoss force-pushed the pr/l2checkmiss branch from 640d4a1 to a9ee28d Compare September 26, 2020 07:49

adamdmoss added a commit to adamdmoss/zfs that referenced this pull request Sep 26, 2020

first stab at test for openzfs#10921

def4339

adamdmoss added a commit to adamdmoss/zfs that referenced this pull request Sep 26, 2020

locally verified test for openzfs#10921 - about to squash

23b2aff

adamdmoss added a commit to adamdmoss/zfs that referenced this pull request Sep 26, 2020

Non-l2arc pool reads shouldn't be l2arc misses.

984f3ce

Resolves openzfs#10921 Includes new test - l2arc_l2miss_pos.ksh Signed-off-by: Adam Moss <[email protected]>

adamdmoss force-pushed the pr/l2checkmiss branch from 23b2aff to 984f3ce Compare September 26, 2020 22:05

gamanakis reviewed Sep 29, 2020

View reviewed changes

tests/zfs-tests/tests/functional/l2arc/l2arc_l2miss_pos.ksh Show resolved Hide resolved

adamdmoss added a commit to adamdmoss/zfs that referenced this pull request Sep 30, 2020

Non-l2arc pool reads shouldn't be l2arc misses.

88a0790

Includes new test - l2arc_l2miss_pos.ksh Signed-off-by: Adam Moss <[email protected]> Closes openzfs#10921

adamdmoss force-pushed the pr/l2checkmiss branch from 7a7cf9e to 88a0790 Compare September 30, 2020 10:19

behlendorf reviewed Oct 1, 2020

View reviewed changes

tests/zfs-tests/tests/functional/l2arc/l2arc_l2miss_pos.ksh Show resolved Hide resolved

tests/zfs-tests/tests/functional/l2arc/l2arc_l2miss_pos.ksh Outdated Show resolved Hide resolved

tests/zfs-tests/tests/functional/l2arc/l2arc_l2miss_pos.ksh Outdated Show resolved Hide resolved

adamdmoss added a commit to adamdmoss/zfs that referenced this pull request Oct 2, 2020

Non-l2arc pool reads shouldn't be l2arc misses.

5439f80

Includes new test - l2arc_l2miss_pos.ksh Signed-off-by: Adam Moss <[email protected]> Closes openzfs#10921

adamdmoss force-pushed the pr/l2checkmiss branch from 88a0790 to 5439f80 Compare October 2, 2020 20:29

adamdmoss added a commit to adamdmoss/zfs that referenced this pull request Oct 7, 2020

Non-l2arc pool reads shouldn't be l2arc misses.

eb7ca77

Includes new test - l2arc_l2miss_pos.ksh Signed-off-by: Adam Moss <[email protected]> Closes openzfs#10921

adamdmoss force-pushed the pr/l2checkmiss branch from 5439f80 to eb7ca77 Compare October 7, 2020 02:26

adamdmoss added a commit to adamdmoss/zfs that referenced this pull request Oct 7, 2020

Non-l2arc pool reads shouldn't be l2arc misses.

990b070

Includes new test - l2arc_l2miss_pos.ksh Signed-off-by: Adam Moss <[email protected]> Closes openzfs#10921

adamdmoss force-pushed the pr/l2checkmiss branch from eb7ca77 to 990b070 Compare October 7, 2020 02:29

adamdmoss added a commit to adamdmoss/zfs that referenced this pull request Oct 10, 2020

Non-l2arc pool reads shouldn't be l2arc misses.

c1ff0c6

Includes new test - l2arc_l2miss_pos.ksh Signed-off-by: Adam Moss <[email protected]> Closes openzfs#10921

adamdmoss force-pushed the pr/l2checkmiss branch from 990b070 to c1ff0c6 Compare October 10, 2020 21:54

adamdmoss added a commit to adamdmoss/zfs that referenced this pull request Oct 12, 2020

Non-l2arc pool reads shouldn't be l2arc misses.

26893f9

Includes new test - l2arc_l2miss_pos.ksh Signed-off-by: Adam Moss <[email protected]> Closes openzfs#10921

adamdmoss force-pushed the pr/l2checkmiss branch from c1ff0c6 to 26893f9 Compare October 12, 2020 00:47

gamanakis reviewed Oct 13, 2020

View reviewed changes

tests/zfs-tests/tests/functional/l2arc/l2arc_l2miss_pos.ksh Outdated Show resolved Hide resolved

Non-l2arc pool reads shouldn't be l2arc misses.

e22e6d8

Includes new test - l2arc_l2miss_pos.ksh Signed-off-by: Adam Moss <[email protected]> Closes openzfs#10921

adamdmoss force-pushed the pr/l2checkmiss branch from 26893f9 to e22e6d8 Compare October 13, 2020 21:17

gamanakis approved these changes Oct 20, 2020

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Oct 20, 2020

behlendorf merged commit 666aa69 into openzfs:master Oct 20, 2020

ARC reads from pools without an l2arc should not be accounted as l2arc misses #10921

ARC reads from pools without an l2arc should not be accounted as l2arc misses #10921

Uh oh!

Conversation

adamdmoss commented Sep 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

behlendorf commented Sep 14, 2020

Uh oh!

gamanakis Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

adamdmoss Sep 19, 2020

Choose a reason for hiding this comment

Uh oh!

gamanakis Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

gamanakis Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gamanakis commented Sep 15, 2020

Uh oh!

adamdmoss commented Sep 18, 2020

Uh oh!

richardelling commented Sep 19, 2020

Uh oh!

adamdmoss commented Sep 19, 2020

Uh oh!

adamdmoss commented Sep 19, 2020

Uh oh!

gamanakis commented Sep 19, 2020

Uh oh!

behlendorf left a comment

Choose a reason for hiding this comment

Uh oh!

adamdmoss commented Sep 21, 2020

Uh oh!

gmelikov commented Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamdmoss commented Sep 21, 2020

Uh oh!

gmelikov commented Sep 21, 2020

Uh oh!

gamanakis commented Sep 21, 2020

Uh oh!

adamdmoss commented Sep 26, 2020

Uh oh!

adamdmoss commented Sep 29, 2020

Uh oh!

adamdmoss commented Sep 29, 2020

Uh oh!

Uh oh!

behlendorf commented Sep 29, 2020

Uh oh!

adamdmoss commented Sep 30, 2020

Uh oh!

adamdmoss commented Oct 1, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamdmoss commented Oct 20, 2020

Uh oh!

gamanakis commented Oct 20, 2020

Uh oh!

Uh oh!

adamdmoss commented Sep 12, 2020 •

edited

Loading

gmelikov commented Sep 21, 2020 •

edited

Loading