[illumos-Developer] illumos bug 453
George Wilson
gwilson at zfsmail.com
Sun Jan 9 10:04:41 PST 2011
I totally agree that the fix should move into the driver somehow. The
approach mentioned by Garrett and Schrock should be investigated.
- George
On 1/9/11 9:33 AM, Garrett D'Amore wrote:
> Note also this bug:
>
> http://bugs.opensolaris.org/bugdatabase/view_bug.do?bug_id=6947063
>
> Of course, not having to perform RMW at all would be the best
> solution. :-)
>
> - Garrett
>
> On 01/ 9/11 09:31 AM, Eric Schrock wrote:
>> There should be something similar for controlling WCE behavior
>> already in the open code that can be used as a model for this
>> behavior (it allows tweaking WCE behavior based on VPD data). Having
>> something tunable in sd.conf strikes a decent balance between
>> allowing users to control behavior with unknown drives, and avoiding
>> exposing an implementation detail. I don't know if this type of
>> stuff is documented in Solaris Express at all, but it would be nice
>> to at least choose the same property name and semantics, even if the
>> implementation is different. The danger of pushing this to zpool(1M)
>> is that users then think it's something they should "tune" (i.e. why
>> not just set it to 1MB, won't that make things faster?).
>>
>> - Eric
>>
>> On Sun, Jan 9, 2011 at 12:26 PM, Garrett D'Amore <garrett at nexenta.com
>> <mailto:garrett at nexenta.com>> wrote:
>>
>> 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>
>> <mailto: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>
>> <mailto:deano at cloudpixies.com <mailto:deano at cloudpixies.com>>
>>
>>
>>
>>
>> _______________________________________________
>> Developer mailing list
>> Developer at lists.illumos.org
>> <mailto:Developer at lists.illumos.org>
>> <mailto: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
>> <mailto:Developer at lists.illumos.org>
>> http://lists.illumos.org/m/listinfo/developer
>>
>>
>>
>> _______________________________________________
>> Developer mailing list
>> Developer at lists.illumos.org <mailto:Developer at lists.illumos.org>
>> http://lists.illumos.org/m/listinfo/developer
>>
>>
>>
>> _______________________________________________
>> 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 <mailto: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
More information about the Developer
mailing list