From 666aa69f32ff2558ef9e9a27bc4cb5559e21d795 Mon Sep 17 00:00:00 2001 From: "Adam D. Moss" Date: Tue, 20 Oct 2020 11:39:52 -0700 Subject: [PATCH] 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 Reviewed-by: George Amanakis Signed-off-by: Adam Moss Closes #10921 --- module/zfs/arc.c | 29 ++++-- tests/runfiles/common.run | 2 +- .../tests/functional/l2arc/Makefile.am | 1 + .../tests/functional/l2arc/l2arc.cfg | 1 + .../functional/l2arc/l2arc_l2miss_pos.ksh | 94 +++++++++++++++++++ .../tests/functional/l2arc/setup.ksh | 1 + 6 files changed, 119 insertions(+), 9 deletions(-) create mode 100755 tests/zfs-tests/tests/functional/l2arc/l2arc_l2miss_pos.ksh diff --git a/module/zfs/arc.c b/module/zfs/arc.c index 4dca688ebb..52a926569e 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 d935f5b042..ef4858cc7f 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 cac85fa46e..9baf580eea 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 cd79af034a..0302392f4c 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 0000000000..783484f52c --- /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 b7a68c1345..0df61a9d27 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