[dm-devel] [PATCH] dm-mpath: push back requests instead of queueing

[Hannes Reinecke]

On 11/12/2013 09:43 AM, Junichi Nomura wrote:
> On 11/12/13 17:17, Hannes Reinecke wrote:
>> On 11/12/2013 08:48 AM, Junichi Nomura wrote:
>>> On 11/08/13 18:02, Hannes Reinecke wrote:
>>>> In doing so we can also remove the ->busy check as a requeue is identical
>>>> to ->busy returning 'true' from the callers perspective. This simplifies
>>>> the code by quite a lot.
>>>
>>> They are not identical.
>>> ->busy returns true when underlying path cannot dispatch a request
>>> immediately. In that case it's better to keep the request in queue
>>> to allow merges.  (It used to have real impact on buffered sequential
>>> write + fsync workload, though the performance impact might become
>>> smaller in recent kernels due to plugging change.)
>>> Also ->busy can be used by upper layer (dm_table_any_busy_target).
>>> So you can't just remove it.
>>>
>> But they are identical from the callers perspective:
>> drivers/md/dm.c:dm_request_fn()
>>
>> 		if (ti->type->busy && ti->type->busy(ti))
>> 			goto delay_and_out;
>>
>> 		clone = dm_start_request(md, rq);
>>
>> 		spin_unlock(q->queue_lock);
>> 		if (map_request(ti, clone, md))
>> 			goto requeued;
>>
>> 		BUG_ON(!irqs_disabled());
>> 		spin_lock(q->queue_lock);
>> 	}
>>
>> 	goto out;
>>
>> requeued:
>> 	BUG_ON(!irqs_disabled());
>> 	spin_lock(q->queue_lock);
>>
>> delay_and_out:
>> 	blk_delay_queue(q, HZ / 10);
>>
>> So the only difference between ->busy and return 'DM_MAPIO_REQUEUE'
>> is that 'busy' runs under the queue_lock.
> 
> A difference is whether the request is once dequeued or not.
> I think requeued request does not accept any merge.
> 
Hmm. Now _that_ is a valid point.
But I would've thought all possible merges are already done by
the time request_fn() is called.
If not multipathing would be working suboptimal anyway, as a
potential merge has been missed even during normal I/O.

> But the point is not there; if you remove ->busy, nobody checks whether
> underlying device is busy and DM_MAPIO_REQUEUE won't be returned.
> So the other option might be moving ->busy check in request_fn to
> inside of map function and let it return DM_MAPIO_REQUEUE.
> 
???

But that's precisely what the patch is doing, no?
The only thing we're not doing in map_io() is to check for
pgpath_busy(), but that would be pointless as a busy pgpath
would return DM_MAPIO_REQUEUE anyway.

We _could_ optimize this in __switch_pg(), to call pgpath_busy()
when selecting the paths. But that should be done by the path selector.
So for that we would need to separate the functionality of the
path selector and __switch_pg; currently it's unclear whether
we need to call pgpath_busy() in __switch_pg or not.

The main reason for removing 'busy' is that it's really hard
to do right for the queue_if_no_path case.
We can easily do a ->busy check during pg_init (by just checking
->queue_io), but the queue_if_no_path _condition_ is only
established after we've called __switch_pg(). Which is precisely
what we do _not_ want here.
Maybe we should add a flag here; 'all_paths_down' or something.
That could be easily checked from ->busy. Plus it can only be
unset on 'reinstate_path'. Hmm.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare at suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)