Date: Wed, 17 Sep 2014 13:38:05 +0300 From: Andriy Gapon <avg@FreeBSD.org> To: Xin LI <delphij@FreeBSD.org>, src-committers@FreeBSD.org, svn-src-all@FreeBSD.org, svn-src-head@FreeBSD.org Cc: "Justin T. Gibbs" <gibbs@FreeBSD.org> Subject: Re: svn commit: r269093 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs Message-ID: <5419648D.2030305@FreeBSD.org> In-Reply-To: <201407251841.s6PIfvUm002202@svn.freebsd.org> References: <201407251841.s6PIfvUm002202@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5419648D.2030305>