Non-l2arc pool reads shouldn't be l2arc misses

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 #10921

6 files changed, 119 insertions, 9 deletions

diff --git a/module/zfs/arc.c b/module/zfs/arc.c
index 4dca688eb..52a926569 100644
--- a/module/zfs/arc.c
+++ b/module/zfs/arc.c
@@ -6305,7 +6305,11 @@ top:
 			    metadata, misses);
 		}
 
-		if (vd != NULL && l2arc_ndev != 0 && !(l2arc_norw && devw)) {
+		/* Check if the spa even has l2 configured */
+		const boolean_t spa_has_l2 = l2arc_ndev != 0 &&
+		    spa->spa_l2cache.sav_count > 0;
+
+		if (vd != NULL && spa_has_l2 && !(l2arc_norw && devw)) {
 			/*
 			 * Read from the L2ARC if the following are true:
 			 * 1. The L2ARC vdev was previously cached.
@@ -6406,15 +6410,24 @@ top:
 		} else {
 			if (vd != NULL)
 				spa_config_exit(spa, SCL_L2ARC, vd);
+
 			/*
-			 * Skip ARC stat bump for block pointers with
-			 * embedded data. The data are read from the blkptr
-			 * itself via decode_embedded_bp_compressed().
+			 * Only a spa with l2 should contribute to l2
+			 * miss stats.  (Including the case of having a
+			 * faulted cache device - that's also a miss.)
 			 */
-			if (l2arc_ndev != 0 && !embedded_bp) {
-				DTRACE_PROBE1(l2arc__miss,
-				    arc_buf_hdr_t *, hdr);
-				ARCSTAT_BUMP(arcstat_l2_misses);
+			if (spa_has_l2) {
+				/*
+				 * Skip ARC stat bump for block pointers with
+				 * embedded data. The data are read from the
+				 * blkptr itself via
+				 * decode_embedded_bp_compressed().
+				 */
+				if (!embedded_bp) {
+					DTRACE_PROBE1(l2arc__miss,
+					    arc_buf_hdr_t *, hdr);
+					ARCSTAT_BUMP(arcstat_l2_misses);
+				}
 			}
 		}
 
diff --git a/tests/runfiles/common.run b/tests/runfiles/common.run
index d935f5b04..ef4858cc7 100644
--- a/tests/runfiles/common.run
+++ b/tests/runfiles/common.run
@@ -899,7 +899,7 @@ post =
 tags = ['functional', 'log_spacemap']
 
 [tests/functional/l2arc]
-tests = ['l2arc_arcstats_pos', 'l2arc_mfuonly_pos',
+tests = ['l2arc_arcstats_pos', 'l2arc_mfuonly_pos', 'l2arc_l2miss_pos',
     'persist_l2arc_001_pos', 'persist_l2arc_002_pos',
     'persist_l2arc_003_neg', 'persist_l2arc_004_pos', 'persist_l2arc_005_pos',
     'persist_l2arc_006_pos', 'persist_l2arc_007_pos', 'persist_l2arc_008_pos']
diff --git a/tests/zfs-tests/tests/functional/l2arc/Makefile.am b/tests/zfs-tests/tests/functional/l2arc/Makefile.am
index cac85fa46..9baf580ee 100644
--- a/tests/zfs-tests/tests/functional/l2arc/Makefile.am
+++ b/tests/zfs-tests/tests/functional/l2arc/Makefile.am
@@ -3,6 +3,7 @@ dist_pkgdata_SCRIPTS = \
 	cleanup.ksh \
 	setup.ksh \
 	l2arc_arcstats_pos.ksh \
+	l2arc_l2miss_pos.ksh \
 	l2arc_mfuonly_pos.ksh \
 	persist_l2arc_001_pos.ksh \
 	persist_l2arc_002_pos.ksh \
diff --git a/tests/zfs-tests/tests/functional/l2arc/l2arc.cfg b/tests/zfs-tests/tests/functional/l2arc/l2arc.cfg
index cd79af034..0302392f4 100644
--- a/tests/zfs-tests/tests/functional/l2arc/l2arc.cfg
+++ b/tests/zfs-tests/tests/functional/l2arc/l2arc.cfg
@@ -24,6 +24,7 @@ export SIZE=1G
 export VDIR=$TESTDIR/disk.l2arc
 export VDEV="$VDIR/a"
 export VDEV_CACHE="$VDIR/b"
+export VDEV1="$VDIR/c"
 
 # fio options
 export DIRECTORY=/$TESTPOOL
diff --git a/tests/zfs-tests/tests/functional/l2arc/l2arc_l2miss_pos.ksh b/tests/zfs-tests/tests/functional/l2arc/l2arc_l2miss_pos.ksh
new file mode 100755
index 000000000..783484f52
--- /dev/null
+++ b/tests/zfs-tests/tests/functional/l2arc/l2arc_l2miss_pos.ksh
@@ -0,0 +1,94 @@
+#!/bin/ksh -p
+#
+# CDDL HEADER START
+#
+# This file and its contents are supplied under the terms of the
+# Common Development and Distribution License ("CDDL"), version 1.0.
+# You may only use this file in accordance with the terms of version
+# 1.0 of the CDDL.
+#
+# A full copy of the text of the CDDL should have accompanied this
+# source.  A copy of the CDDL is also available via the Internet at
+# http://www.illumos.org/license/CDDL.
+#
+# CDDL HEADER END
+#
+
+#
+# Copyright (c) 2020, Adam Moss. All rights reserved.
+#
+
+. $STF_SUITE/include/libtest.shlib
+. $STF_SUITE/tests/functional/l2arc/l2arc.cfg
+
+#
+# DESCRIPTION:
+#	l2arc_misses does not increment upon reads from a pool without l2arc
+#
+# STRATEGY:
+#	1. Create pool with a cache device.
+#	2. Create pool without a cache device.
+#	3. Create a random file in the no-cache-device pool,
+#		and random read for 10 sec.
+#	4. Check that l2arc_misses hasn't risen
+#	5. Create a random file in the pool with the cache device,
+#		and random read for 10 sec.
+#	6. Check that l2arc_misses has risen
+#
+
+verify_runnable "global"
+
+log_assert "l2arc_misses does not increment upon reads from a pool without l2arc."
+
+function cleanup
+{
+	if poolexists $TESTPOOL ; then
+		destroy_pool $TESTPOOL
+	fi
+	if poolexists $TESTPOOL1 ; then
+		destroy_pool $TESTPOOL1
+	fi
+}
+log_onexit cleanup
+
+typeset fill_mb=800
+typeset cache_sz=$(( 1.4 * $fill_mb ))
+export FILE_SIZE=$(( floor($fill_mb / $NUMJOBS) ))M
+
+log_must truncate -s ${cache_sz}M $VDEV_CACHE
+
+log_must zpool create -O compression=off -f $TESTPOOL $VDEV cache $VDEV_CACHE
+log_must zpool create -O compression=off -f $TESTPOOL1 $VDEV1
+
+# I/O to pool without l2arc - expect that l2_misses stays constant
+export DIRECTORY=/$TESTPOOL1
+log_must fio $FIO_SCRIPTS/mkfiles.fio
+log_must fio $FIO_SCRIPTS/random_reads.fio
+# attempt to remove entries for pool from ARC so we would try
+#    to hit the nonexistent L2ARC for subsequent reads
+log_must zpool export $TESTPOOL1
+log_must zpool import $TESTPOOL1 -d $VDEV1
+
+typeset starting_miss_count=$(get_arcstat l2_misses)
+
+log_must fio $FIO_SCRIPTS/random_reads.fio
+log_must test $(get_arcstat l2_misses) -eq $starting_miss_count
+
+# I/O to pool with l2arc - expect that l2_misses rises
+export DIRECTORY=/$TESTPOOL
+log_must fio $FIO_SCRIPTS/mkfiles.fio
+log_must fio $FIO_SCRIPTS/random_reads.fio
+# wait for L2ARC writes to actually happen
+arcstat_quiescence_noecho l2_size
+# attempt to remove entries for pool from ARC so we would try
+#    to hit L2ARC for subsequent reads
+log_must zpool export $TESTPOOL
+log_must zpool import $TESTPOOL -d $VDEV
+
+log_must fio $FIO_SCRIPTS/random_reads.fio
+log_must test $(get_arcstat l2_misses) -gt $starting_miss_count
+
+log_must zpool destroy -f $TESTPOOL
+log_must zpool destroy -f $TESTPOOL1
+
+log_pass "l2arc_misses does not increment upon reads from a pool without l2arc."
diff --git a/tests/zfs-tests/tests/functional/l2arc/setup.ksh b/tests/zfs-tests/tests/functional/l2arc/setup.ksh
index b7a68c134..0df61a9d2 100755
--- a/tests/zfs-tests/tests/functional/l2arc/setup.ksh
+++ b/tests/zfs-tests/tests/functional/l2arc/setup.ksh
@@ -25,5 +25,6 @@ verify_runnable "global"
 log_must rm -rf $VDIR
 log_must mkdir -p $VDIR
 log_must mkfile $SIZE $VDEV
+log_must mkfile $SIZE $VDEV1
 
 log_pass