N-way mirror read speedup in zfsonlinux

[Matthew Ahrens]

On Wed, Aug 14, 2013 at 7:48 PM, Steven Hartland <killing at multiplay.co.uk>wrote:

> ----- Original Message ----- From: "Matthew Ahrens" <mahrens at delphix.com>
>
>  This patch looks reasonable to me.  I shared Alexander's skepticism of the
>> time-based metric.  Just a couple of specific notes:
>>
>> I believe that mm_preferred is only used for reads.  You might not bother
>> with the load balancing logic if this is not a read.
>>
>
> Good idea, done.
>
>
>  You might add accessor functions for avl_numnodes(&vq->vq_pending_**tree)
>> and vq_lastoffset, rather than having vdev_disk.c reach into the queue
>> implementation.  It looks like technically you could get away without
>> vq_lock for avl_numnodes(), as it's just loading a "long".  But that is
>> really relying on the implementation details.  More significantly, you
>> have
>> no locking for vq_lastoffset.  At a minimum, we should have some comments
>> explaining that this is OK.  It seems like it could get the wrong result
>> on
>> 32-bit systems (where load/store of 64 bit values is not atomic), but I
>> guess that's OK since it will only cause a tiny performance impact.
>>
>
> Accessors added to vdev_queue including comment about lack of locking.
>
>
>  zfs_mirror_by_load -- could this be removed in favor of setting
>> zfs_vdev_mirror_locality_bonus = 0 to turn off the bonus?  If not, seems
>> like it should at least be a boolean_t.
>>
>
> zfs_mirror_by_load changes the entire read distribution method not just
> the locality bonus with 0 = current striped balancer, 1 = new load balancer
> (default) so I don't think thats appropriate.
>

Ah, yes, I was misreading that code.  Even with locality_bonus=0, we'd take
into account the load.


>
> It was also added for testing convenience as much as anything, however all
> being well it could be removed unless there's objections to that?


I'm fine with removing the old algorithm and the switch
(zfs_mirror_by_load).


>
>
>  vdev_mirror_load() -- if the vdev is not rotating, you always apply the
>> locality bonus.  It was slightly difficult to follow the comment and logic
>> around this.  Would a "seek penalty" which is applied if (rotating &&
>> lastoffset != offset) be easier to understand?
>>
>
> It might do but that would make the common case slightly more expensive
> what do others think?


How would it be more expensive?

--matt


>
>
>    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 postmaster at multiplay.co.uk.
>