Date: Mon, 17 Nov 2014 16:14:04 +0000 From: Steven Hartland <steven@multiplay.co.uk> To: Andriy Gapon <avg@FreeBSD.org>, 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 Message-ID: <546A1ECC.7000301@multiplay.co.uk> In-Reply-To: <546A0F1E.5050305@FreeBSD.org> References: <201411171445.sAHEjgbE096937@svn.freebsd.org> <546A0ED8.6020104@freebsd.org> <546A0F1E.5050305@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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. >> 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; >>> >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?546A1ECC.7000301>