Date: Mon, 17 Nov 2014 15:06:00 +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: <546A0ED8.6020104@freebsd.org> In-Reply-To: <201411171445.sAHEjgbE096937@svn.freebsd.org> References: <201411171445.sAHEjgbE096937@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Looks like this could have introduced random data corruption, have you seen this in practice? 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?546A0ED8.6020104>