On Sun, Nov 17, 2013 at 5:43 PM, Steven Hartland <[email protected]>wrote:
> ----- Original Message ----- From: "Matthew Ahrens" <[email protected]> > > Please use an enum for vdev_rotation_state >>> >>>> >>>> >>> There's no such variable, are you refering to vdev_rotation_rate? >>> >> >> yes >> >> If so this was initially a bool for non-rotating but it was deemed using >>> rotation_rate would be more helpful moving forward, as this value could >>> be used to make more intelligent decisions. >>> >> >> >> Yes, changing it to a bool would probably be even better. Going forward, >> it will be easy to change this to a different type when necessary. >> > > I'd prefer to keep it as the actual rate not an enum or bool so if we > look to make improvements in the future we have rotation rate of the > device and dont have to implement yet another struct member to store > this. > > An example of this could be calculate lower loads for vdevs with a higher > rotational rate. > > To clarify as per ATA and SCSI specs: > 0 = Rate unknown > 1 = Rate non-rotationl > 2-INT16_MAX = rotational rate I don't see the point of having a variable called "rotation_rate" that doesn't store the rotation rate and is always set to UNKNOWN. Please update the webrev when you've implemented this (setting this variable based on the rotation rate and taking it into account in the i/o scheduling logic), or removed the dead code. > vdev_mirror.c: >>> >>>> unfortunately I think we have to declare tunables as non-static, >>>> otherwise >>>> gcc can optimize them away. perhaps prefix the variable names with >>>> "zfs_mirror_"? >>>> >>>> >>> These are tunables in FreeBSD and Linux does illumos have such things? >>> >> >> yes, they can be set by /etc/system or mdb. >> >> If cc optimises them away is that an issue? >>> >> >> yes, because then they can't be tuned. >> > > Ah so you can poke those values directly using /etc/system and you > dont need any special handlers like sysctl in FreeBSD? Yes. --matt > > > 138: I don't understand this comment, or what code it is referring to; >>> can >>> >>>> you elaborate? I don't see any check for resilvering before returning >>>> UINT_MAX above. >>>> >>>> >>> In a previous revision INT_MAX was returned if vdev_resliver_txg != 0 >>> as it was assumed that avoiding the device that was reslivering would >>> improve performance. Justin found this assumption didn't hold true, hence >>> the early return was removed and the comment added so others are aware of >>> this in case they thought of the same thing. >>> >> >> >> So you'll remove the comment? >> > > I'm a little confused, its not my current intention to remove the comment > as removing it wouldn't allow someone looking at the code to know this had > be tried and it doesn't help. Does that make sense? > > > > ================================================ > This e.mail is private and confidential between Multiplay (UK) Ltd. and > the person or entity to whom it is addressed. In the event of misdirection, > the recipient is prohibited from using, copying, printing or otherwise > disseminating it or any information contained in it. > In the event of misdirection, illegible or incomplete transmission please > telephone +44 845 868 1337 > or return the E.mail to [email protected]. > >
_______________________________________________ developer mailing list [email protected] http://lists.open-zfs.org/mailman/listinfo/developer
