[dm-devel] [PATCH v2] dm mpath: fix race condition between multipath_dtr and pg_init_done

[Mike Snitzer]

On Tue, Nov 05 2013 at 10:56am -0500,
Alasdair G Kergon <agk at redhat.com> wrote:

> On Thu, Oct 31, 2013 at 10:48:13AM -0400, Mike Snitzer wrote:
> > --- linux.orig/drivers/md/dm-mpath.c
> > +++ linux/drivers/md/dm-mpath.c
> 
> > +	unsigned pg_init_disabled:1;	/* pginit is currently not allowed */
> 
> > +	if (m->pg_init_required && !m->pg_init_in_progress && pgpath &&
> > +	    !m->pg_init_disabled)
> 
> So we're accepting that pg_init might be both "disabled" and "required"
> simultaneously (otherwise we wouldn't need this test)?

Yes, needs to be independent of pg_init_required.

> > @@ -942,10 +944,20 @@ static void multipath_wait_for_pg_init_c
> >  static void flush_multipath_work(struct multipath *m)
> >  {
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&m->lock, flags);
> > +	m->pg_init_disabled = 1;
> > +	spin_unlock_irqrestore(&m->lock, flags);
> > +
> >  	flush_workqueue(kmpath_handlerd);
> >  	multipath_wait_for_pg_init_completion(m);
> 
> So this implies pg_init could even still be running while it is disabled?

pg_init_disabled doesn't allow pg_init to be initiated once set (even if
pg_init_required is).  Hence pg_init is disabled.

> If the logic is correct, then I find the new variable name quite confusing and
> think it should be changed, or as a minimum have inline comments explaining
> the apparent contradictions!

I'm not seeing a contradiction.