From owner-svn-src-all@FreeBSD.ORG Mon Nov 17 15:08:11 2014 Return-Path: Delivered-To: svn-src-all@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 380742A9; Mon, 17 Nov 2014 15:08:11 +0000 (UTC) Received: from citadel.icyb.net.ua (citadel.icyb.net.ua [212.40.38.140]) by mx1.freebsd.org (Postfix) with ESMTP id 0724413A; Mon, 17 Nov 2014 15:08:09 +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 RAA10576; Mon, 17 Nov 2014 17:10:00 +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 1XqNuQ-0001wZ-Nb; Mon, 17 Nov 2014 17:08:06 +0200 Message-ID: <546A0F1E.5050305@FreeBSD.org> Date: Mon, 17 Nov 2014 17:07:10 +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> In-Reply-To: <546A0ED8.6020104@freebsd.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.18-1 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, 17 Nov 2014 15:08:11 -0000 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? > 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