On Sun, Nov 17, 2013 at 1:42 PM, Steven Hartland <[email protected]>wrote:
> Finally found some time today to port the work I did > on FreeBSD to improve ZFS N-way mirror read performance > by using location information. > > Webrev: > http://cr.illumos.org/~webrev/steveh/illumos-4334/ Thanks for taking on this work, Steven. Here are some comments on the code: vdev_impl.h: Choose a different name for vq_lastoffset, so it is not confused with the existing vq_last_offset. I believe this is referring to the last *queued* i/o, as opposed to the last *issued* i/o. Why can't this be tracked in (i.e. set by) the vdev_queue layer? Please use an enum for vdev_rotation_state 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_"? mc_load: I assume this is here for debugging only? could you add a comment to that effect?` can you explain what mm_preferred points to? looks like it starts with "off the end of the array", which seems like a questionable decision. Oh, I see it is off the end of the array but you allocate a little more space for it. That's pretty trick / confusing. Is this measurably better performing than doing something straightforward like (a) allocating it in vdev_mirror_child_select(), or (b) walk mm_children again to find the nth child with mc_load==lowest_load? Another way to do this even more efficiently and (probably) straightforwardly would be to start the loop on a random child, and go through the loop twice. Then you don't need mm_preferred or mm_preferred_cnt at all, you can just go with the first (or last) child with the lowest load. rotating_seek_offset: what's a "reduced rotating media seek increment"? Could you rewrite the comment to be something like: "If we issue an i/o which is more than rotating_seek_offset bytes from the previous i/o, and the vdev is VDEV_RATE_ROTATING, then the device's load metric will be increased by rotating_seek_offset. Otherwise it will be increased by non_rotating_seek_inc." vdev_mirror_load: please add a comment above this function explaining what it is trying to do. i.e. calculate and return load on the vdev if a new io were to be added at zio_offset. calculation is based on X and Y, used for Z... Without the comment there is cognitive dissonance as this function does not load a vdev_mirror (annoying verbs that are also nouns!) 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. 325: Oh, that's why we bother with the whole preferred_cnt stuff? I didn't realize that SSDs have a limited number of reads. Can you point me to some specs for that (how many reads before it wears out)? People on the internet seem to disagree: http://superuser.com/questions/440171/will-reading-data-cause-ssds-to-wear-out --matt > > > Issue: > https://www.illumos.org/issues/4334 > > It's currently missing non-rotating media detection > from vdev_rotation_rate. > > I'm not at all familar with illumos device code but I've > looked and the underlying subsystems seem to have a number > of possible ways of detecting this. > > tg_attribute has: media_is_solid_state > cmlb_geom_t has: g_rpm > > Unfortunately as far as I can tell none of these are > currently exposed via an ioctl which I'm guessing would > be required for code at the ZFS layer to detect these > settings. > > Any advice on this would be appreciated so I can complete > this port. > > Regards > Steve > > ================================================ > 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]. > > > > ------------------------------------------- > illumos-zfs > Archives: https://www.listbox.com/member/archive/182191/=now > RSS Feed: https://www.listbox.com/member/archive/rss/182191/ > 21635000-ebd1d460 > Modify Your Subscription: https://www.listbox.com/ > member/?member_id=21635000&id_secret=21635000-73dc201a > Powered by Listbox: http://www.listbox.com >
_______________________________________________ developer mailing list [email protected] http://lists.open-zfs.org/mailman/listinfo/developer
