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

[Andriy Gapon]

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