From owner-svn-src-all@freebsd.org Mon Apr 16 07:36:42 2018 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 15F8BF9149C for ; Mon, 16 Apr 2018 07:36:42 +0000 (UTC) (envelope-from steven.hartland@multiplay.co.uk) Received: from mail-wr0-x242.google.com (mail-wr0-x242.google.com [IPv6:2a00:1450:400c:c0c::242]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 84B247690F for ; Mon, 16 Apr 2018 07:36:41 +0000 (UTC) (envelope-from steven.hartland@multiplay.co.uk) Received: by mail-wr0-x242.google.com with SMTP id l49so22997609wrl.4 for ; Mon, 16 Apr 2018 00:36:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=multiplay-co-uk.20150623.gappssmtp.com; s=20150623; h=subject:to:references:from:message-id:date:user-agent:mime-version :in-reply-to:content-language; bh=ENCw/PNP2dDC2OvRdLXb6F38XId+C+xvawrgWmGuIn0=; b=tS5EjC5XkuB0cJ1KKnloQIsba0Ltrc8z9/apT7r6g0fHSEp83ISZvH+yd8vtxEkUP+ 1k3J0ZmMfyl3xiIQcdggsrIzULtzk4z0fKMQ2aWF4nNw/7a7W9kBHcPACnewFKN35cGZ fINitgzKjFcyGIgmpibEa25ynjIuwWsGmfpAimrPM74FS18EIFjehbrKIr9KcFj5D3cC yJ4EO56Yn9x+kQA2KQqXfc1qL4k1QgdIvJDdyv1oWLQwYb1kRNsKDTnw9wIa7GR/Fj54 isoy1BkQVZWqc/fCt1N9YMbajetQXwjvmdsQ0k6KBVpdJz1fizE6PHhfuFDy/a09hTNF KgPA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language; bh=ENCw/PNP2dDC2OvRdLXb6F38XId+C+xvawrgWmGuIn0=; b=Ezg3qO4GKH9WbjjswWXj0Dl169w4wIFE4Koc1VTs1gNWvjQqkLbS1rIOnYrp8XLkRZ SkJXJ48Je1P+vMKDATHluTocwqYk5LTRfWQtk0fgGgrDYokpoZSGDckryU+Lk5rjk/c2 V4T8n0ZRCNkCBlPn2CuRK58zPOFXd7sff6u7ioK+Z+HcdKr1S/IaFKC3Hz5h/1kem1fG 3pX3SGcyOkUrm+WuujcoeeDTvk/FT5XAdZI5OEdn9x0nqYvoXt/cN3ALcNaD3fGhrvce 8H4q6JW8qcGdmb04TwRpyDW/3vmk0B7V1WLwpTJpWK/HCkvsR+P1wdvkOiQv7ywnNahe Obrg== X-Gm-Message-State: ALQs6tAheCMjTrnyl7wjMsYD+s3GWTHurvAUdZtF9WLs1Dp91TFZcv3B q1t8YNPXjfxE6zJNdeodQWXj1Q== X-Google-Smtp-Source: AIpwx4+gPMo4VQYJVcgToRdyl+GypojOdWY5Jalv0g7msg1OgBu8e7b4mausO1NAZynVrwIhVda3yQ== X-Received: by 10.28.7.76 with SMTP id 73mr10034363wmh.128.1523864200314; Mon, 16 Apr 2018 00:36:40 -0700 (PDT) Received: from [10.10.1.111] ([185.97.61.1]) by smtp.gmail.com with ESMTPSA id n49sm18622454wrn.50.2018.04.16.00.36.39 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 16 Apr 2018 00:36:39 -0700 (PDT) Subject: Re: svn commit: r332523 - in head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs: . sys To: Alexander Motin , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org References: <201804160054.w3G0swF1030058@repo.freebsd.org> From: Steven Hartland Message-ID: Date: Mon, 16 Apr 2018 08:36:38 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <201804160054.w3G0swF1030058@repo.freebsd.org> Content-Language: en-US Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.25 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 16 Apr 2018 07:36:42 -0000 Hey Mav, this seems like an important one to get in for 11.2 so just wanted to check if that was your intention as there's no MFC tag on the commit? On 16/04/2018 01:54, Alexander Motin wrote: > Author: mav > Date: Mon Apr 16 00:54:58 2018 > New Revision: 332523 > URL: https://svnweb.freebsd.org/changeset/base/332523 > > Log: > 9433 Fix ARC hit rate > > When the compressed ARC feature was added in commit d3c2ae1 > the method of reference counting in the ARC was modified. As > part of this accounting change the arc_buf_add_ref() function > was removed entirely. > > This would have be fine but the arc_buf_add_ref() function > served a second undocumented purpose of updating the ARC access > information when taking a hold on a dbuf. Without this logic > in place a cached dbuf would not migrate its associated > arc_buf_hdr_t to the MFU list. This would negatively impact > the ARC hit rate, particularly on systems with a small ARC. > > This change reinstates the missing call to arc_access() from > dbuf_hold() by implementing a new arc_buf_access() function. > > Reviewed-by: Giuseppe Di Natale > Reviewed-by: Tony Hutter > Reviewed-by: Tim Chase > Reviewed by: George Wilson > Reviewed-by: George Melikov > Signed-off-by: Brian Behlendorf > > Modified: > head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/arc.c > head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dbuf.c > head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/arc.h > > Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/arc.c > ============================================================================== > --- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/arc.c Mon Apr 16 00:42:45 2018 (r332522) > +++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/arc.c Mon Apr 16 00:54:58 2018 (r332523) > @@ -540,8 +540,13 @@ typedef struct arc_stats { > */ > kstat_named_t arcstat_mutex_miss; > /* > + * Number of buffers skipped when updating the access state due to the > + * header having already been released after acquiring the hash lock. > + */ > + kstat_named_t arcstat_access_skip; > + /* > * Number of buffers skipped because they have I/O in progress, are > - * indrect prefetch buffers that have not lived long enough, or are > + * indirect prefetch buffers that have not lived long enough, or are > * not from the spa we're trying to evict from. > */ > kstat_named_t arcstat_evict_skip; > @@ -796,6 +801,7 @@ static arc_stats_t arc_stats = { > { "allocated", KSTAT_DATA_UINT64 }, > { "deleted", KSTAT_DATA_UINT64 }, > { "mutex_miss", KSTAT_DATA_UINT64 }, > + { "access_skip", KSTAT_DATA_UINT64 }, > { "evict_skip", KSTAT_DATA_UINT64 }, > { "evict_not_enough", KSTAT_DATA_UINT64 }, > { "evict_l2_cached", KSTAT_DATA_UINT64 }, > @@ -5063,6 +5069,51 @@ arc_access(arc_buf_hdr_t *hdr, kmutex_t *hash_lock) > } else { > ASSERT(!"invalid arc state"); > } > +} > + > +/* > + * This routine is called by dbuf_hold() to update the arc_access() state > + * which otherwise would be skipped for entries in the dbuf cache. > + */ > +void > +arc_buf_access(arc_buf_t *buf) > +{ > + mutex_enter(&buf->b_evict_lock); > + arc_buf_hdr_t *hdr = buf->b_hdr; > + > + /* > + * Avoid taking the hash_lock when possible as an optimization. > + * The header must be checked again under the hash_lock in order > + * to handle the case where it is concurrently being released. > + */ > + if (hdr->b_l1hdr.b_state == arc_anon || HDR_EMPTY(hdr)) { > + mutex_exit(&buf->b_evict_lock); > + ARCSTAT_BUMP(arcstat_access_skip); > + return; > + } > + > + kmutex_t *hash_lock = HDR_LOCK(hdr); > + mutex_enter(hash_lock); > + > + if (hdr->b_l1hdr.b_state == arc_anon || HDR_EMPTY(hdr)) { > + mutex_exit(hash_lock); > + mutex_exit(&buf->b_evict_lock); > + ARCSTAT_BUMP(arcstat_access_skip); > + return; > + } > + > + mutex_exit(&buf->b_evict_lock); > + > + ASSERT(hdr->b_l1hdr.b_state == arc_mru || > + hdr->b_l1hdr.b_state == arc_mfu); > + > + DTRACE_PROBE1(arc__hit, arc_buf_hdr_t *, hdr); > + arc_access(hdr, hash_lock); > + mutex_exit(hash_lock); > + > + ARCSTAT_BUMP(arcstat_hits); > + ARCSTAT_CONDSTAT(!HDR_PREFETCH(hdr), > + demand, prefetch, !HDR_ISTYPE_METADATA(hdr), data, metadata, hits); > } > > /* a generic arc_done_func_t which you can use */ > > Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dbuf.c > ============================================================================== > --- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dbuf.c Mon Apr 16 00:42:45 2018 (r332522) > +++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dbuf.c Mon Apr 16 00:54:58 2018 (r332523) > @@ -2579,8 +2579,10 @@ top: > return (SET_ERROR(ENOENT)); > } > > - if (db->db_buf != NULL) > + if (db->db_buf != NULL) { > + arc_buf_access(db->db_buf); > ASSERT3P(db->db.db_data, ==, db->db_buf->b_data); > + } > > ASSERT(db->db_buf == NULL || arc_referenced(db->db_buf)); > > > Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/arc.h > ============================================================================== > --- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/arc.h Mon Apr 16 00:42:45 2018 (r332522) > +++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/arc.h Mon Apr 16 00:54:58 2018 (r332523) > @@ -169,6 +169,7 @@ void arc_loan_inuse_buf(arc_buf_t *buf, void *tag); > void arc_buf_destroy(arc_buf_t *buf, void *tag); > int arc_buf_size(arc_buf_t *buf); > int arc_buf_lsize(arc_buf_t *buf); > +void arc_buf_access(arc_buf_t *buf); > void arc_release(arc_buf_t *buf, void *tag); > int arc_released(arc_buf_t *buf); > void arc_buf_freeze(arc_buf_t *buf); >