Re: [PATCH v4 1/2] dm: prevent table type changes after initial table load

[Mike Snitzer <snitzer@redhat.com>]

On Tue, Jun 29 2010 at  7:40am -0400,
Alasdair G Kergon <agk@redhat.com> wrote:

> On Thu, May 27, 2010 at 08:47:48AM -0400, Mike Snitzer wrote:
> > Introduce 'type_lock' in mapped_device structure and use it to protect
> > md->type access.  table_load() sets md->type without concern for:
> 
> > - another table_load() racing to set conflicting md->type.
> which leads to non-deterministic behaviour that is of no practical use and
> so there is no need to support it.
> 
> > - do_resume() making a conflicting table live.
> Any table being made live must already have passed through table_load so I
> don't see how there can be conflicting types.

As we talked about on irc.  There is potential for do_resume() to
destage the "inactive" table slot, in the process of making the table
"live", and in parallel another table load can arrive to stage another
"inactive" table (which can have a conflicting type).  Without a
critical section there is room for this to occur.

And the critical section must span more than setting md->type; as
md->queue initialization may fail (elevator memory allocation fails
ENOMEM).  So competing table_load needs protection from concurrent queue
initialization.

(do understand that I know you're commenting on this patch header in
isolation; but I figured I'd give context for "why the mutex").  I have
an updated patch header too:

Before table_load() stages an inactive table check if its type
conflicts with a previously established device type (md->type).

Allowed md->type transitions:
DM_TYPE_NONE => DM_TYPE_BIO_BASED
DM_TYPE_NONE => DM_TYPE_REQUEST_BASED

Once a table load occurs the DM device's type is completely immutable.
This prevents a table reload from switching the inactive table directly
to a conflicting type (even if the table is explicitly cleared before
load).

Introduce 'type_lock' in mapped_device structure and use it to protect
md->type access.  Use of mutex prepares for follow-on DM device queue
initialization changes that will allocate memory while md->type_lock
is held.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
Acked-by: Kiyoshi Ueda <k-ueda@ct.jp.nec.com>


> The mutex here seems to be overkill: instead of protecting one field to
> cope with a useless race, we should be making the output of the race
> deterministic (e.g. perhaps giving an error on the parallel table_load
> attempt).

We're protecting 2 fields in the end (md->type and md->queue): once 2/2
is applied.

I agree 100% on improving the DM core to be trained to error out on
competing table loads (at a higher level).. given the parallelism that
we have right now in DM operations (across devices) we get this awkward
inability to _know_ the state transitions of:
no table -> inactive table -> live table

As you pointed out that is an unwanted side-effect of supporting
operations we do want.

> Anyway, I can take this patch for now and we can deal with the mutex/race
> differently later.

OK, sounds good.

Mike