Re: [PATCH 1/2] dm-multipath: push back requests instead of queueing

[Hannes Reinecke <hare@suse.de>]

On 01/20/2014 12:57 PM, Junichi Nomura wrote:
> On 01/17/14 19:42, Hannes Reinecke wrote:
>> @@ -1256,7 +1188,8 @@ static void pg_init_done(void *data, int errors)
>>  		m->queue_io = 0;
>>  
>>  	m->pg_init_delay_retry = delay_retry;
>> -	queue_work(kmultipathd, &m->process_queued_ios);
>> +	if (!m->queue_io)
>> +		dm_table_run_queue(m->ti->table);
>>  
>>  	/*
>>  	 * Wake up any thread waiting to suspend.
> 
> Does pg_init retry still work with this change?
> 
> I suspect it doesn't. When a retry is requested in pg_init_done(),
> m->queue_io is still 0 and somebody has to kick pg_init.
> 
> Instead of replacing "process_queued_ios" work completely,
> how about keeping it around and just replacing dispatch_queued_ios() by
> dm_table_run_queue()?
> 
I'd rather prefer to schedule the activate_path() workqueue item
directly; no need to involve yet another workqueue here.
And we already know which path to activate.

But yeah, it looks as if we need to reschedule activate_path() here.

>> @@ -1606,7 +1540,7 @@ static int multipath_ioctl(struct dm_target *ti, unsigned int cmd,
>>  
>>  	spin_lock_irqsave(&m->lock, flags);
>>  
>> -	if (!m->current_pgpath)
>> +	if (!m->current_pgpath || !m->queue_io)
>>  		__choose_pgpath(m, 0);
>>  
>>  	pgpath = m->current_pgpath;
> 
> Why is !m->queue_io check added here?
> 
You are right, this is a different patch.
I'll be splitting it up.

>> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
>> index 0704c52..291491b 100644
>> --- a/drivers/md/dm.c
>> +++ b/drivers/md/dm.c
>> @@ -1912,6 +1912,19 @@ static int dm_any_congested(void *congested_data, int bdi_bits)
>>  	return r;
>>  }
>>  
>> +void dm_table_run_queue(struct dm_table *t)
>> +{
>> +	struct mapped_device *md = dm_table_get_md(t);
>> +	unsigned long flags;
>> +
>> +	if (md->queue) {
>> +		spin_lock_irqsave(md->queue->queue_lock, flags);
>> +		blk_run_queue_async(md->queue);
>> +		spin_unlock_irqrestore(md->queue->queue_lock, flags);
>> +	}
>> +}
>> +EXPORT_SYMBOL_GPL(dm_table_run_queue);
>> +
> 
> I think this funcion fits better in dm-table.c.
> 
So I've thought, but struct mapped_device is internal to dm.c
And having yet another wrapper just to circumvent the layering
seemed a bit excessive.

But I do whatever deemed necessary by the powers that be ..

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@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)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel