[illumos-Developer] illumos bug 453

[Garrett D'Amore]

I believe that B151 and other builds *after* the code was closed have a 
way to override (perhaps via a whitelist) the discovered ashift in sd.  
I'd generally agree that I'd prefer to handle this in sd rather than in 
ZFS.   Its a device problem, and the fix belongs with the device, IMO.

   - Garrett


On 01/ 9/11 09:15 AM, George Wilson wrote:
> 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
>
>
> _______________________________________________
> Developer mailing list
> Developer at lists.illumos.org
> http://lists.illumos.org/m/listinfo/developer