From owner-svn-src-head@FreeBSD.ORG Mon Nov 17 16:29:35 2014 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 8E5941CA; Mon, 17 Nov 2014 16:29:35 +0000 (UTC) Received: from citadel.icyb.net.ua (citadel.icyb.net.ua [212.40.38.140]) by mx1.freebsd.org (Postfix) with ESMTP id 8D4E7FC1; Mon, 17 Nov 2014 16:29:34 +0000 (UTC) Received: from porto.starpoint.kiev.ua (porto-e.starpoint.kiev.ua [212.40.38.100]) by citadel.icyb.net.ua (8.8.8p3/ICyb-2.3exp) with ESMTP id SAA11508; Mon, 17 Nov 2014 18:31:25 +0200 (EET) (envelope-from avg@FreeBSD.org) Received: from localhost ([127.0.0.1]) by porto.starpoint.kiev.ua with esmtp (Exim 4.34 (FreeBSD)) id 1XqPBE-00021m-DC; Mon, 17 Nov 2014 18:29:32 +0200 Message-ID: <546A2248.5000400@FreeBSD.org> Date: Mon, 17 Nov 2014 18:28:56 +0200 From: Andriy Gapon User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: Steven Hartland , src-committers@FreeBSD.org, svn-src-all@FreeBSD.org, svn-src-head@FreeBSD.org Subject: Re: svn commit: r274628 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs References: <201411171445.sAHEjgbE096937@svn.freebsd.org> <546A0ED8.6020104@freebsd.org> <546A0F1E.5050305@FreeBSD.org> <546A1ECC.7000301@multiplay.co.uk> In-Reply-To: <546A1ECC.7000301@multiplay.co.uk> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 17 Nov 2014 16:29:35 -0000 On 17/11/2014 18:14, Steven Hartland wrote: > > On 17/11/2014 15:07, Andriy Gapon wrote: >> On 17/11/2014 17:06, Steven Hartland wrote: >>> Looks like this could have introduced random data corruption, have you seen this >>> in practice? >> Which part exactly? > The part where it zeroed beyond the allocated buffer. Oh, maybe I worded the commit message badly, but I wanted to say was that *if* I replaced SPA_MINBLOCKSIZE with vdev_ashift, but left the zeroing code where it was before, then there would be zeroing beyond the buffer. There was no corruption with the previous code. >>> On 17/11/2014 14:45, Andriy Gapon wrote: >>>> Author: avg >>>> Date: Mon Nov 17 14:45:42 2014 >>>> New Revision: 274628 >>>> URL: https://svnweb.freebsd.org/changeset/base/274628 >>>> >>>> Log: >>>> l2arc: restore correct rounding up of asize of compressed data >>>> This rounding up was lost in a mismerge of illumos code. >>>> See r268075 MFV r267565. >>>> After that commit zio_compress_data() no longer performs any compressed >>>> size adjustment, so it needs to be done externally. On FreeBSD we round >>>> up the size using vdev_ashift rather than SPA_MINBLOCKSIZE so that 4KB >>>> devices are properly supported. >>>> Additionally, zero out the buffer tail only if compression succeeds. >>>> The compression is considered successful if the size of compressed >>>> data after rounding up to account for the vdev ashift is less than the >>>> original data size. It does not make sense to have the data compressed >>>> if all the savings are lost to rounding up. >>>> With the new zio_compress_data() it could have been possible that the >>>> rounded compressed size would be greater than the original size and thus >>>> we could zero beyond the allocated buffer if the zeroing code was kept >>>> at the original place. >>>> Discussed with: delphij, gibbs >>>> MFC after: 2 weeks >>>> X-MFC with: r274627 >>>> >>>> Modified: >>>> head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/arc.c >>>> >>>> Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/arc.c >>>> ============================================================================== >>>> --- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/arc.c Mon Nov 17 >>>> 14:16:02 2014 (r274627) >>>> +++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/arc.c Mon Nov 17 >>>> 14:45:42 2014 (r274628) >>>> @@ -5326,12 +5326,6 @@ l2arc_compress_buf(l2arc_buf_hdr_t *l2hd >>>> csize = zio_compress_data(ZIO_COMPRESS_LZ4, l2hdr->b_tmp_cdata, >>>> cdata, l2hdr->b_asize); >>>> - rounded = P2ROUNDUP(csize, (size_t)SPA_MINBLOCKSIZE); >>>> - if (rounded > csize) { >>>> - bzero((char *)cdata + csize, rounded - csize); >>>> - csize = rounded; >>>> - } >>>> - >>>> if (csize == 0) { >>>> /* zero block, indicate that there's nothing to write */ >>>> zio_data_buf_free(cdata, len); >>>> @@ -5340,11 +5334,19 @@ l2arc_compress_buf(l2arc_buf_hdr_t *l2hd >>>> l2hdr->b_tmp_cdata = NULL; >>>> ARCSTAT_BUMP(arcstat_l2_compress_zeros); >>>> return (B_TRUE); >>>> - } else if (csize > 0 && csize < len) { >>>> + } >>>> + >>>> + rounded = P2ROUNDUP(csize, >>>> + (size_t)1 << l2hdr->b_dev->l2ad_vdev->vdev_ashift); >>>> + if (rounded < len) { >>>> /* >>>> * Compression succeeded, we'll keep the cdata around for >>>> * writing and release it afterwards. >>>> */ >>>> + if (rounded > csize) { >>>> + bzero((char *)cdata + csize, rounded - csize); >>>> + csize = rounded; >>>> + } >>>> l2hdr->b_compress = ZIO_COMPRESS_LZ4; >>>> l2hdr->b_asize = csize; >>>> l2hdr->b_tmp_cdata = cdata; >>>> >> > -- Andriy Gapon