Message ID | 1391459326-21569-5-git-send-email-snitzer@redhat.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
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"?
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
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.
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
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.
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 --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); }