diff mbox

[4/7] dm mpath: remove process_queued_ios()

Message ID 1391459326-21569-5-git-send-email-snitzer@redhat.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Mike Snitzer Feb. 3, 2014, 8:28 p.m. UTC 
From: Hannes Reinecke <hare@suse.de>

process_queued_ios() has served 3 functions:
  1) select pg and pgpath if none is selected
  2) start pg_init if requested
  3) dispatch queued IOs when pg is ready

Basically, a call to queue_work(process_queued_ios) can be replaced by
dm_table_run_md_queue_async(), which runs request queue and ends up
calling map_io(), which does 1), 2) and 3).

Exception is when !pg_ready() (which means either pg_init is running or
requested), then multipath_busy() prevents map_io() being called from
request_fn.

If pg_init is running, it should be ok as long as pg_init_done() does
the right thing when pg_init is completed, I.e.: restart pg_init if
!pg_ready() or call dm_table_run_md_queue_async() to kick map_io().

If pg_init is requested, we have to make sure the request is detected
and pg_init will be started.  pg_init is requested in 3 places:
  a) __choose_pgpath() in map_io()
  b) __choose_pgpath() in multipath_ioctl()
  c) pg_init retry in pg_init_done()
a) is ok because map_io() calls __pg_init_all_paths(), which does 2).
b) needs a call to __pg_init_all_paths(), which does 2).
c) needs a call to __pg_init_all_paths(), which does 2).

So this patch removes process_queued_ios() and ensures that
__pg_init_all_paths() is called at the appropriate locations.

We only need to take care to add a small delay when calling
__pg_init_all_paths() to move processing off to a workqueue; otherwise
pg_init_done() might end up calling scsi_dh_activate() directly, which
might use non-atomic memory allocations or issue I/O.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Cc: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-mpath.c | 63 ++++++++++++++++++---------------------------------
 1 file changed, 22 insertions(+), 41 deletions(-)

Comments

Junichi Nomura Feb. 4, 2014, 3:24 a.m. UTC | #1 
On 02/04/14 05:28, Mike Snitzer wrote:
> @@ -1216,9 +1185,12 @@ static void pg_init_done(void *data, int errors)
>  
>  	if (!m->pg_init_required)
>  		m->queue_io = 0;
> -
> -	m->pg_init_delay_retry = delay_retry;
> -	queue_work(kmultipathd, &m->process_queued_ios);
> +	else if (m->current_pg) {
> +		m->pg_init_delay_retry = delay_retry;
> +		/* Use a small delay to force the use of workqueue context */
> +		__pg_init_all_paths(m, 50/HZ);
> +		goto out;
> +	}

I think the patch is still broken.

When "m->pg_init_required && !m->current_pg",
it ends up with !pg_ready() and no pg_init running.
So map_io() will not be called and IO will stall.

What do you think about my suggestion here:
https://www.redhat.com/archives/dm-devel/2014-February/msg00013.html

Also it's not yet clear why the second parameter of __pg_init_all_paths()
is needed. (At least, "50/HZ" is typo or something.)

> @@ -1591,8 +1563,17 @@ static int multipath_ioctl(struct dm_target *ti, unsigned int cmd,
...
> +		if (m->current_pg && m->pg_init_required)
> +			__pg_init_all_paths(m, 0);
> +		spin_unlock_irqrestore(&m->lock, flags);
> +		dm_table_run_md_queue_async(m->ti->table);
> +	}

What happens if "!m->current_pg && m->pg_init_required"?
Hannes Reinecke Feb. 4, 2014, 8:18 a.m. UTC | #2 
On 02/04/2014 04:24 AM, Junichi Nomura wrote:
> On 02/04/14 05:28, Mike Snitzer wrote:
>> @@ -1216,9 +1185,12 @@ static void pg_init_done(void *data, int errors)
>>  
>>  	if (!m->pg_init_required)
>>  		m->queue_io = 0;
>> -
>> -	m->pg_init_delay_retry = delay_retry;
>> -	queue_work(kmultipathd, &m->process_queued_ios);
>> +	else if (m->current_pg) {
>> +		m->pg_init_delay_retry = delay_retry;
>> +		/* Use a small delay to force the use of workqueue context */
>> +		__pg_init_all_paths(m, 50/HZ);
>> +		goto out;
>> +	}
> 
> I think the patch is still broken.
> 
> When "m->pg_init_required && !m->current_pg",
> it ends up with !pg_ready() and no pg_init running.
> So map_io() will not be called and IO will stall.
> 
> What do you think about my suggestion here:
> https://www.redhat.com/archives/dm-devel/2014-February/msg00013.html
> 
Sigh. pg_init_required again...

> Also it's not yet clear why the second parameter of __pg_init_all_paths()
> is needed. (At least, "50/HZ" is typo or something.)
> 
>> @@ -1591,8 +1563,17 @@ static int multipath_ioctl(struct dm_target *ti, unsigned int cmd,
> ...
>> +		if (m->current_pg && m->pg_init_required)
>> +			__pg_init_all_paths(m, 0);
>> +		spin_unlock_irqrestore(&m->lock, flags);
>> +		dm_table_run_md_queue_async(m->ti->table);
>> +	}
> 
> What happens if "!m->current_pg && m->pg_init_required"?
> 
>From the current logic it means that no valid pg was found, so
calling pg_init would be pointless.
We're calling __choose_pgpath() before that, so if that returns
with current_pg == NULL all paths are down, and calling
pg_init would be pointless.

But I think I see to have pg_init_required handling cleared up;
I'll be doing a patch to unset it at the start of __choose_pgpath(),
this we we can be sure that it'll be set correctly.

Cheers,

Hannes
Junichi Nomura Feb. 4, 2014, 8:55 a.m. UTC | #3 
On 02/04/14 17:18, Hannes Reinecke wrote:
>>> @@ -1591,8 +1563,17 @@ static int multipath_ioctl(struct dm_target *ti, unsigned int cmd,
>> ...
>>> +		if (m->current_pg && m->pg_init_required)
>>> +			__pg_init_all_paths(m, 0);
>>> +		spin_unlock_irqrestore(&m->lock, flags);
>>> +		dm_table_run_md_queue_async(m->ti->table);
>>> +	}
>>
>> What happens if "!m->current_pg && m->pg_init_required"?
>>
>>From the current logic it means that no valid pg was found, so
> calling pg_init would be pointless.
> We're calling __choose_pgpath() before that, so if that returns
> with current_pg == NULL all paths are down, and calling
> pg_init would be pointless.
> 
> But I think I see to have pg_init_required handling cleared up;
> I'll be doing a patch to unset it at the start of __choose_pgpath(),
> this we we can be sure that it'll be set correctly.

I think it is possible that __choose_pgpath() being called twice
before pg_init_required is checked.

For example,

  multipath_ioctl()
    __choose_pgpath()
      clear pg_init_required
      select a new pg
      __switch_pg()
        set pg_init_required

  map_io()
    __choose_pgpath()
      clear pg_init_required
      select the same pg
      (pg_init_required is not set)
      ...

In this case, pg_init should be submitted to the pg but not.
Hannes Reinecke Feb. 4, 2014, 9:08 a.m. UTC | #4 
On 02/04/2014 09:55 AM, Junichi Nomura wrote:
> On 02/04/14 17:18, Hannes Reinecke wrote:
>>>> @@ -1591,8 +1563,17 @@ static int multipath_ioctl(struct dm_target *ti, unsigned int cmd,
>>> ...
>>>> +		if (m->current_pg && m->pg_init_required)
>>>> +			__pg_init_all_paths(m, 0);
>>>> +		spin_unlock_irqrestore(&m->lock, flags);
>>>> +		dm_table_run_md_queue_async(m->ti->table);
>>>> +	}
>>>
>>> What happens if "!m->current_pg && m->pg_init_required"?
>>>
>> >From the current logic it means that no valid pg was found, so
>> calling pg_init would be pointless.
>> We're calling __choose_pgpath() before that, so if that returns
>> with current_pg == NULL all paths are down, and calling
>> pg_init would be pointless.
>>
>> But I think I see to have pg_init_required handling cleared up;
>> I'll be doing a patch to unset it at the start of __choose_pgpath(),
>> this we we can be sure that it'll be set correctly.
> 
> I think it is possible that __choose_pgpath() being called twice
> before pg_init_required is checked.
> 
> For example,
> 
>   multipath_ioctl()
>     __choose_pgpath()
>       clear pg_init_required
>       select a new pg
>       __switch_pg()
>         set pg_init_required
> 
>   map_io()
>     __choose_pgpath()
>       clear pg_init_required
>       select the same pg
>       (pg_init_required is not set)
>       ...
> 
But why should 'map_io' calling __choose_pgpath()?

Either __choose_path() from ioctl was able to set ->current_pg
(in which case __choose_pgpath() wouldn't be called in map_io)
or it was not, in which case pg_init_required would need to be reset
during __choose_pgpath() when called from map_io().

Am I missing something?

Cheers,

Hannes
Junichi Nomura Feb. 4, 2014, 9:27 a.m. UTC | #5 
On 02/04/14 18:08, Hannes Reinecke wrote:
> On 02/04/2014 09:55 AM, Junichi Nomura wrote:
>> On 02/04/14 17:18, Hannes Reinecke wrote:
>>>>> @@ -1591,8 +1563,17 @@ static int multipath_ioctl(struct dm_target *ti, unsigned int cmd,
>>>> ...
>>>>> +		if (m->current_pg && m->pg_init_required)
>>>>> +			__pg_init_all_paths(m, 0);
>>>>> +		spin_unlock_irqrestore(&m->lock, flags);
>>>>> +		dm_table_run_md_queue_async(m->ti->table);
>>>>> +	}
>>>>
>>>> What happens if "!m->current_pg && m->pg_init_required"?
>>>>
>>> >From the current logic it means that no valid pg was found, so
>>> calling pg_init would be pointless.
>>> We're calling __choose_pgpath() before that, so if that returns
>>> with current_pg == NULL all paths are down, and calling
>>> pg_init would be pointless.
>>>
>>> But I think I see to have pg_init_required handling cleared up;
>>> I'll be doing a patch to unset it at the start of __choose_pgpath(),
>>> this we we can be sure that it'll be set correctly.
>>
>> I think it is possible that __choose_pgpath() being called twice
>> before pg_init_required is checked.
>>
>> For example,
>>
>>   multipath_ioctl()
>>     __choose_pgpath()
>>       clear pg_init_required
>>       select a new pg
>>       __switch_pg()
>>         set pg_init_required
>>
>>   map_io()
>>     __choose_pgpath()
>>       clear pg_init_required
>>       select the same pg
>>       (pg_init_required is not set)
>>       ...
>>
> But why should 'map_io' calling __choose_pgpath()?
> 
> Either __choose_path() from ioctl was able to set ->current_pg
> (in which case __choose_pgpath() wouldn't be called in map_io)
> or it was not, in which case pg_init_required would need to be reset
> during __choose_pgpath() when called from map_io().

__choose_pgpath() is a function to select "path".
Even if current_pg is set, map_io() may call the function
to select a path. (Please look at the repeat_count check)

So, I suggested the followings:
  Call __pg_init_all_paths() regardless of current_pg.
  __pg_init_all_paths() returns whether pg_init has started.
  If !current_pg, it returns 0.
  (https://www.redhat.com/archives/dm-devel/2014-February/msg00013.html)

The semantics is clear:
  - if pg_init_required, call __pg_init_all_paths()
  - __pg_init_all_paths() might fail to start pg_init (= returns 0).
    Then caller should take some action or give up.
Hannes Reinecke Feb. 4, 2014, 9:45 a.m. UTC | #6 
On 02/04/2014 10:27 AM, Junichi Nomura wrote:
> On 02/04/14 18:08, Hannes Reinecke wrote:
>> On 02/04/2014 09:55 AM, Junichi Nomura wrote:
>>> On 02/04/14 17:18, Hannes Reinecke wrote:
>>>>>> @@ -1591,8 +1563,17 @@ static int multipath_ioctl(struct dm_target *ti, unsigned int cmd,
>>>>> ...
>>>>>> +		if (m->current_pg && m->pg_init_required)
>>>>>> +			__pg_init_all_paths(m, 0);
>>>>>> +		spin_unlock_irqrestore(&m->lock, flags);
>>>>>> +		dm_table_run_md_queue_async(m->ti->table);
>>>>>> +	}
>>>>>
>>>>> What happens if "!m->current_pg && m->pg_init_required"?
>>>>>
>>>> >From the current logic it means that no valid pg was found, so
>>>> calling pg_init would be pointless.
>>>> We're calling __choose_pgpath() before that, so if that returns
>>>> with current_pg == NULL all paths are down, and calling
>>>> pg_init would be pointless.
>>>>
>>>> But I think I see to have pg_init_required handling cleared up;
>>>> I'll be doing a patch to unset it at the start of __choose_pgpath(),
>>>> this we we can be sure that it'll be set correctly.
>>>
>>> I think it is possible that __choose_pgpath() being called twice
>>> before pg_init_required is checked.
>>>
>>> For example,
>>>
>>>   multipath_ioctl()
>>>     __choose_pgpath()
>>>       clear pg_init_required
>>>       select a new pg
>>>       __switch_pg()
>>>         set pg_init_required
>>>
>>>   map_io()
>>>     __choose_pgpath()
>>>       clear pg_init_required
>>>       select the same pg
>>>       (pg_init_required is not set)
>>>       ...
>>>
>> But why should 'map_io' calling __choose_pgpath()?
>>
>> Either __choose_path() from ioctl was able to set ->current_pg
>> (in which case __choose_pgpath() wouldn't be called in map_io)
>> or it was not, in which case pg_init_required would need to be reset
>> during __choose_pgpath() when called from map_io().
> 
> __choose_pgpath() is a function to select "path".
> Even if current_pg is set, map_io() may call the function
> to select a path. (Please look at the repeat_count check)
> 
... But only if 'queue_io' is not set, ie no pg_init is running.
At which point 'pg_init_required' should be set to '0' anyway.

What I'm arguing here is an inconsistency in __choose_pg():
__choose_pg() might or might not set 'current_pg', but if
'current_pg' is not set the status of 'pg_init_required'
is undefined upon return.

This makes it (in my view) unnecessarily complicated to
check if we need to initialize the paths, as we have to
check for both, current_pg _and_ pg_init_required.
If it were _that_ easy we wouldn't have this discussion :-)

> So, I suggested the followings:
>   Call __pg_init_all_paths() regardless of current_pg.
>   __pg_init_all_paths() returns whether pg_init has started.
>   If !current_pg, it returns 0.
>   (https://www.redhat.com/archives/dm-devel/2014-February/msg00013.html)
> 
> The semantics is clear:
>   - if pg_init_required, call __pg_init_all_paths()
>   - __pg_init_all_paths() might fail to start pg_init (= returns 0).
>     Then caller should take some action or give up.
> 
All right, will be modifying the patch.

Cheers,

Hannes
diff mbox

Patch

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 89c0997..b99cff0 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -93,8 +93,6 @@  struct multipath {
 	unsigned pg_init_count;		/* Number of times pg_init called */
 	unsigned pg_init_delay_msecs;	/* Number of msecs before pg_init retry */
 
-	struct work_struct process_queued_ios;
-
 	struct work_struct trigger_event;
 
 	/*
@@ -119,7 +117,6 @@  typedef int (*action_fn) (struct pgpath *pgpath);
 static struct kmem_cache *_mpio_cache;
 
 static struct workqueue_struct *kmultipathd, *kmpath_handlerd;
-static void process_queued_ios(struct work_struct *work);
 static void trigger_event(struct work_struct *work);
 static void activate_path(struct work_struct *work);
 static int __pgpath_busy(struct pgpath *pgpath);
@@ -197,7 +194,6 @@  static struct multipath *alloc_multipath(struct dm_target *ti)
 		spin_lock_init(&m->lock);
 		m->queue_io = 1;
 		m->pg_init_delay_msecs = DM_PG_INIT_DELAY_DEFAULT;
-		INIT_WORK(&m->process_queued_ios, process_queued_ios);
 		INIT_WORK(&m->trigger_event, trigger_event);
 		init_waitqueue_head(&m->pg_init_wait);
 		mutex_init(&m->work_mutex);
@@ -254,10 +250,10 @@  static void clear_mapinfo(struct multipath *m, union map_info *info)
  * Path selection
  *-----------------------------------------------*/
 
-static void __pg_init_all_paths(struct multipath *m)
+static void __pg_init_all_paths(struct multipath *m, unsigned long min_delay)
 {
 	struct pgpath *pgpath;
-	unsigned long pg_init_delay = 0;
+	unsigned long pg_init_delay = min_delay;
 
 	if (m->pg_init_in_progress || m->pg_init_disabled)
 		return;
@@ -406,7 +402,7 @@  static int map_io(struct multipath *m, struct request *clone,
 							      &pgpath->path,
 							      nr_bytes);
 		} else {
-			__pg_init_all_paths(m);
+			__pg_init_all_paths(m, 0);
 			r = DM_MAPIO_REQUEUE;
 		}
 	} else {
@@ -438,40 +434,13 @@  static int queue_if_no_path(struct multipath *m, unsigned queue_if_no_path,
 		m->saved_queue_if_no_path = queue_if_no_path;
 	m->queue_if_no_path = queue_if_no_path;
 	if (!m->queue_if_no_path)
-		queue_work(kmultipathd, &m->process_queued_ios);
+		dm_table_run_md_queue_async(m->ti->table);
 
 	spin_unlock_irqrestore(&m->lock, flags);
 
 	return 0;
 }
 
-static void process_queued_ios(struct work_struct *work)
-{
-	struct multipath *m =
-		container_of(work, struct multipath, process_queued_ios);
-	struct pgpath *pgpath = NULL;
-	unsigned must_queue = 1;
-	unsigned long flags;
-
-	spin_lock_irqsave(&m->lock, flags);
-
-	if (!m->current_pgpath)
-		__choose_pgpath(m, 0);
-
-	pgpath = m->current_pgpath;
-
-	if ((pgpath && !m->queue_io) ||
-	    (!pgpath && !m->queue_if_no_path))
-		must_queue = 0;
-
-	if (pgpath && m->pg_init_required)
-		__pg_init_all_paths(m);
-
-	spin_unlock_irqrestore(&m->lock, flags);
-	if (!must_queue)
-		dm_table_run_md_queue_async(m->ti->table);
-}
-
 /*
  * An event is triggered whenever a path is taken out of use.
  * Includes path failure and PG bypass.
@@ -1018,7 +987,7 @@  static int reinstate_path(struct pgpath *pgpath)
 
 	if (!m->nr_valid_paths++) {
 		m->current_pgpath = NULL;
-		queue_work(kmultipathd, &m->process_queued_ios);
+		dm_table_run_md_queue_async(m->ti->table);
 	} else if (m->hw_handler_name && (m->current_pg == pgpath->pg)) {
 		if (queue_work(kmpath_handlerd, &pgpath->activate_path.work))
 			m->pg_init_in_progress++;
@@ -1216,9 +1185,12 @@  static void pg_init_done(void *data, int errors)
 
 	if (!m->pg_init_required)
 		m->queue_io = 0;
-
-	m->pg_init_delay_retry = delay_retry;
-	queue_work(kmultipathd, &m->process_queued_ios);
+	else if (m->current_pg) {
+		m->pg_init_delay_retry = delay_retry;
+		/* Use a small delay to force the use of workqueue context */
+		__pg_init_all_paths(m, 50/HZ);
+		goto out;
+	}
 
 	/*
 	 * Wake up any thread waiting to suspend.
@@ -1591,8 +1563,17 @@  static int multipath_ioctl(struct dm_target *ti, unsigned int cmd,
 	if (!r && ti->len != i_size_read(bdev->bd_inode) >> SECTOR_SHIFT)
 		r = scsi_verify_blk_ioctl(NULL, cmd);
 
-	if (r == -ENOTCONN && !fatal_signal_pending(current))
-		queue_work(kmultipathd, &m->process_queued_ios);
+	if (r == -ENOTCONN && !fatal_signal_pending(current)) {
+		spin_lock_irqsave(&m->lock, flags);
+		if (!m->current_pg) {
+			/* Path status changed, redo selection */
+			__choose_pgpath(m, 0);
+		}
+		if (m->current_pg && m->pg_init_required)
+			__pg_init_all_paths(m, 0);
+		spin_unlock_irqrestore(&m->lock, flags);
+		dm_table_run_md_queue_async(m->ti->table);
+	}
 
 	return r ? : __blkdev_driver_ioctl(bdev, mode, cmd, arg);
 }