[illumos-Developer] illumos bug 453

[George Wilson]

It's probably also worth noting that setting ZPOOL_CONFIG_ASHIFT, as is 
being proposed, only works if it's a higher value than what ZFS 
discovers when it first open the device. In other words if a user sets 
blocksize=2048 on a working 4K sector sized device then the blocksize 
value will simply be ignored.

- George

On 1/9/11 8:56 AM, Eric Schrock wrote:
> - This is a property, so please follow the standard methodology for 
> the ZFS tools:
>
>         zpool create blocksize=4096 ...
>
> This includes an '=' sign between property and value, as well as a 
> name that follows convention (it's "recordsize", not "record-size", so 
> it should be "blocksize").
>
> - While the implementation can use '0' as auto-detect, please expose 
> this to the user as "blocksize=auto" (0 should be illegal).
>
> - What are the ramifications of this being detected incorrectly?  Is 
> there any need to include observability into what the current setting 
> is (I could imagine 'zpool status -V' showing some per-vdev 
> configuration parameters, for example).
>
> - How does ZFS handle mixed ashift on non-leaf vdevs?  I thought that 
> the ashift was dynamically computed (i.e. max of children), so what 
> does it mean to manually specify an ashift as you do at line 1348?
>
> - Please update the usage message
>
> - What is the illumos manpage strategy?  This should be changed in the 
> zpool(1M) manpage as well.
>
> 1188: Add a block comment explaining the semantics being used
>
> 1191: incorrect indentation
>
> 1200: format this comment as a sentence
>
> 1208: ditch "(%i specified)".  Besides missing space around the 
> parenthesis and using the wrong format character, it's redundant.
>
> 1213: Use proper multi-line comments, remove "for now at least!"
>
> 1218: Rephrase this error message to be useful - no user knows what 
> this "sanity check" is.  Perhaps something like "blocksize is too large".
>
> 1218: Get rid of "(%i specified)" (see above)
>
> 1224: I don't see why this is an error.  If the defined behavior is 
> that optional properties affect any following vdevs, why is it illegal 
> to have no vdevs following it?
>
> Thanks,
>
> - Eric
>
> On Sun, Jan 9, 2011 at 11:24 AM, Deano <deano at rattie.demon.co.uk 
> <mailto:deano at rattie.demon.co.uk>> 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 <mailto:deano at cloudpixies.com>
>
>
>
>
>     _______________________________________________
>     Developer mailing list
>     Developer at lists.illumos.org <mailto:Developer at lists.illumos.org>
>     http://lists.illumos.org/m/listinfo/developer
>
>
>
>
> -- 
> Eric Schrock, Delphix
>
>
> _______________________________________________
> Developer mailing list
> Developer at lists.illumos.org
> http://lists.illumos.org/m/listinfo/developer