Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 14 Aug 2015 14:19:39 +0300
From:      Andriy Gapon <avg@FreeBSD.org>
To:        Alexander Motin <mav@FreeBSD.org>, svn-src-all@FreeBSD.org, svn-src-head@FreeBSD.org, "src-committers@freebsd.org" <src-committers@FreeBSD.org>
Subject:   Re: svn commit: r286570 - in head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs: . sys
Message-ID:  <55CDCECB.1020103@FreeBSD.org>
In-Reply-To: <201508101034.t7AAYOtk074606@repo.freebsd.org>
References:  <201508101034.t7AAYOtk074606@repo.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On 10/08/2015 13:34, Alexander Motin wrote:
> Author: mav
> Date: Mon Aug 10 10:34:23 2015
> New Revision: 286570
> URL: https://svnweb.freebsd.org/changeset/base/286570
> 
> Log:
>   MFV r277426: 5408 managing ZFS cache devices requires lots of RAM
>   Reviewed by: Christopher Siden <christopher.siden@delphix.com>
>   Reviewed by: George Wilson <george.wilson@delphix.com>
>   Reviewed by: Matthew Ahrens <mahrens@delphix.com>
>   Reviewed by: Don Brady <dev.fs.zfs@gmail.com>
>   Reviewed by: Josef 'Jeff' Sipek <josef.sipek@nexenta.com>
>   Approved by: Garrett D'Amore <garrett@damore.org>
>   Author: Chris Williamson <Chris.Williamson@delphix.com>
>   
>   illumos/illumos-gate@89c86e32293a30cdd7af530c38b2073fee01411c
>   
>   Currently, every buffer cached in the L2ARC is accompanied by a 240-byte
>   header in memory, leading to very high memory consumption when using very
>   large cache devices. These changes significantly reduce this overhead.
>   
>   Currently:
>   
>   L1-only header = 176 bytes
>   L1 + L2 or L2-only header = 176 bytes + 32 byte checksum + 32 byte l2hdr
>       = 240 bytes
>   
>   Memory-optimized:
>   
>   L1-only header = 176 bytes
>   L1 + L2 header = 176 bytes + 32 byte checksum = 208 bytes
>   L2-only header = 96 bytes + 32 byte checksum = 128 bytes
>   
>   So overall:
>   
>             Trunk  Optimized
>           +-----------------+
>   L1-only | 176 B  | 176 B  | (same)
>           +-----------------+
>   L1 & L2 | 240 B  | 208 B  | (saved 32 bytes)
>           +-----------------+
>   L2-only | 240 B  | 128 B  | (saved 116 bytes)
>           +-----------------+
>   
>   For an average blocksize of 8KB, this means that for the L2ARC, the ratio
>   of metadata to data has gone down from about 2.92% to 1.56%.  For a
>   'storage optimized' EC2 instance with 1600GB of SSD and 60GB of RAM, this
>   means that we expect a completely full L2ARC to use (1600 GB * 0.0156) /
>   60GB = 41% of the available memory, down from 78%.
> 
> Modified:
>   head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/arc.c
>   head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/arc.h
> 

Looking at the code in arc_buf_l2_cdata_free() function it seems that
this MFV contains element of both 5408 change and the later "5497 lock
contention on arcs_mtx" change, because that's the change where
arc_buf_l2_cdata_free() was actually added in illumos.
It's hard to review such a composite change.
I think that you used a better approach when you merged 5497 where you
first reverted our local changes and then merged the upstream changes.
I think that it would make sense to use the same approach with this
change as well.
For example, in ZoL they did just that and it was much easier to
understand and review:
https://github.com/zfsonlinux/zfs/pull/3481/commits
Note the revert commits one of which is Revert "fix l2arc compression
buffers leak".

P.S. The original illumos commit also made small changes to ztest.c that
do not seem to be merged.

P.P.S. it sucks that svn is not git :-)
-- 
Andriy Gapon



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?55CDCECB.1020103>