svn commit: r274628 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs

[Andriy Gapon]

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