From owner-svn-src-all@FreeBSD.ORG Wed Sep 17 10:39:05 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 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id C70D36C0; Wed, 17 Sep 2014 10:39:05 +0000 (UTC) Received: from citadel.icyb.net.ua (citadel.icyb.net.ua [212.40.38.140]) by mx1.freebsd.org (Postfix) with ESMTP id 33454B39; Wed, 17 Sep 2014 10:39:03 +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 NAA26459; Wed, 17 Sep 2014 13:39:01 +0300 (EEST) (envelope-from avg@FreeBSD.org) Received: from localhost ([127.0.0.1]) by porto.starpoint.kiev.ua with esmtp (Exim 4.34 (FreeBSD)) id 1XUCdZ-00096J-4K; Wed, 17 Sep 2014 13:39:01 +0300 Message-ID: <5419648D.2030305@FreeBSD.org> Date: Wed, 17 Sep 2014 13:38:05 +0300 From: Andriy Gapon User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:31.0) Gecko/20100101 Thunderbird/31.1.0 MIME-Version: 1.0 To: Xin LI , src-committers@FreeBSD.org, svn-src-all@FreeBSD.org, svn-src-head@FreeBSD.org Subject: Re: svn commit: r269093 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs References: <201407251841.s6PIfvUm002202@svn.freebsd.org> In-Reply-To: <201407251841.s6PIfvUm002202@svn.freebsd.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: "Justin T. Gibbs" 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: Wed, 17 Sep 2014 10:39:05 -0000 On 25/07/2014 21:41, Xin LI wrote: > Author: delphij > Date: Fri Jul 25 18:41:56 2014 > New Revision: 269093 > URL: http://svnweb.freebsd.org/changeset/base/269093 > > Log: > Transform the I/O when vdev_physical_ashift is greater than > SPA_MINBLOCKSHIFT. This commit seems illogical to me. I do not see why a fact that the _physical_ sector size is greater than 512B (e.g. 4KB) should force a data buffer of a "physical" zio to be rounded up using ashift. To give an example. ZFS knows that a vdev has physical sector size of 4KB, but a logical sector size of 512B. ZFS uses ashift of 12 for the vdev. Nevertheless in some situations ZFS wants to make a physical / special write of 512B (taking advantage of RMW). Why should we force that write to become 4KB with data beyond the first 512B being all zeros? It would be logical to check for a logical sector size and to enforce it, but that is not what this commit does. This commit is against the spirit of commit r268855. It was specifically to allow ZFS physical writes to have smaller alignments than otherwise would be dictated by ashift. I think that this commit should be reverted. If ZFS wants to make a physical I/O operation with a particular alignment then it must be allowed to. If that operation is invalid for a certain vdev, then it is higher level code that must be modified to account for the logical sector size. The physical zio must be _created_ correctly, it should not be silently transformed. > MFC after: 2 weeks > > Modified: > head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zio.c > > Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zio.c > ============================================================================== > --- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zio.c Fri Jul 25 18:20:56 2014 (r269092) > +++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zio.c Fri Jul 25 18:41:56 2014 (r269093) > @@ -2622,7 +2622,8 @@ zio_vdev_io_start(zio_t **ziop) > > align = 1ULL << vd->vdev_top->vdev_ashift; > > - if (!(zio->io_flags & ZIO_FLAG_PHYSICAL) && > + if ((!(zio->io_flags & ZIO_FLAG_PHYSICAL) || > + (vd->vdev_top->vdev_physical_ashift > SPA_MINBLOCKSHIFT)) && > P2PHASE(zio->io_size, align) != 0) { > /* Transform logical writes to be a full physical block size. */ > uint64_t asize = P2ROUNDUP(zio->io_size, align); > -- Andriy Gapon