[illumos-Developer] illumos bug 453

[George Wilson]

Deano,

First of all, it's great that someone is looking into this but I was 
wondering if anybody has looked at why the ioctl() below is not giving 
us the correct information?

         /*
          * Determine the device's minimum transfer size.
          * If the ioctl isn't supported, assume DEV_BSIZE.
          */
         if (ldi_ioctl(dvd->vd_lh, DKIOCGMEDIAINFOEXT, (intptr_t)&dkmext,
             FKIOCTL, kcred, NULL) != 0)
                 dkmext.dki_pbsize = DEV_BSIZE;

         *ashift = highbit(MAX(dkmext.dki_pbsize, SPA_MINBLOCKSIZE)) - 1;

I'm not a big fan of exposing an implementation detail, like 
vdev_ashift, to users as it makes it more difficult to remove later. I'm 
happy to review your code but would first like to make sure we're making 
the right design decision.

If we're after a quick and dirty way to allow users to set their ashift 
because they have some funky drives then we could also provide an 
override via a kernel tunable. This buy us some time and allow us to 
solve the underlying problem with the code above.

Thanks,
George




On 1/9/11 8:24 AM, Deano wrote:
> Hi,
>
> I have a fix for bug 453 ready to be checked in.
>
> Its adds an optional parameter to zpool, such that you can specify the drive
> block size, normally this isn't needed as ZFS reads it from the drive itself
> but recently a number of SATA 4KiB blocks drives have appeared (for>= 2TB
> disks), that either lie or don't bother to export the correct size, which
> can cause performance issues as they are classed as 512B drives by default.
> It is purely a tool level fix, as ASHIFT setting (which it ultimately is) is
> already supported fully by ZFS properties just not exposed to the zpool
> tool.
>
> zpool create block-size 4096 dsk1 dsk2
>
> Which will set the block-size for *all* following drives on the command
> line. block-size 0 will reset to hw detect, and setting block-size again is
> allowed per pool, so that different pools can have different block sizes.
>
> Been reviewed on IRC the other night (I forgot to get the emails of the
> reviewers so just have the IRC names (Triskelios, gisburn, jbk), if I can
> get the emails I can give proper credit).
>
> Passes hg nit except for copyright year warning, left that because not sure
> how illumos as a whole wants to handle copyright update notices. Personally
> I don't mind doing it in my name or assigning to illumos or ??
>
> Webrev with results of code reviews and hg nit fixes can be found at
> http://cloudpixies.com/webrev/bug453
>
> Any problems, just say and I'll get them fixed or if okay would be good to
> get it into the code base :)
>
> Thanks,
> Deano
>
> deano at cloudpixies.com
>
>
>
>
> _______________________________________________
> Developer mailing list
> Developer at lists.illumos.org
> http://lists.illumos.org/m/listinfo/developer