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

[Mike Snitzer <snitzer@redhat.com>]

On Tue, May 25 2010 at  7:16am -0400,
Kiyoshi Ueda <k-ueda@ct.jp.nec.com> wrote:

> Hi Mike,
> 
> On 05/25/2010 08:46 AM +0900, Mike Snitzer wrote:
> > Before table_load() stages an inactive table atomically check if its
> > type conflicts with a previously established device type (md->type).
> > 
> > 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.
> > - do_resume() making a conflicting table live.
> > 
> > Allowed transitions:
> > UNKNOWN_MD_TYPE => BIO_BASED_MD_TYPE
> > UNKNOWN_MD_TYPE => REQUEST_BASED_MD_TYPE
> > 
> > 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).
> > 
> > Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> 
> I basically agree with this design.
> But I have some comments.  Please see below.
> 
> By the way, I can't make enough time to review until tomorrow.
> So the following comments may not be all.
> Sorry for the inconvenience.

No problem, thanks again for all your review.

> > Index: linux-2.6/drivers/md/dm-ioctl.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/md/dm-ioctl.c
> > +++ linux-2.6/drivers/md/dm-ioctl.c
> > @@ -1176,12 +1176,39 @@ static int table_load(struct dm_ioctl *p
> >  		goto out;
> >  	}
> >  
> > +	/*
> > +	 * Protect md->type against concurrent table loads.
> > +	 * Locking strategy:
> > +	 * + Leverage fact that md's type cannot change after initial table load.
> > +	 *   - Only protect type in table_load() -- not in do_resume().
> > +	 *
> > +	 * + Protect type while working to stage an inactive table:
> > +	 *   - check if table's type conflicts with md->type
> > +	 *     (holding: md->type_lock)
> > +	 *   - stage inactive table (hc->new_map)
> > +	 *     (holding: md->type_lock + _hash_lock)
> > +	 */
> 
> I think you don't need to hold md->type_lock while you set the table
> to hc->new_map; If 2 tables of the same type are racing here, we can
> have either table.  If they are different type, the former one will win
> and the latter one will be rejected.

Correct.  This v2 patchset has significantly reduced the complexity (no
longer need to be concerned with md->type transitioning back to
UNKNOWN_MD_TYPE).  I just missed the fact that the locking could be
simplified a bit too.

> > +	dm_lock_md_type(md);
> > +
> > +	if (dm_unknown_md_type(md)) {
> > +		/* initial table load, set md's type based on table's type */
> > +		dm_set_md_type(md, t);
> > +	} else if (!dm_md_type_matches_table(md, t)) {
> > +		DMWARN("can't change device type after initial table load.");
> > +		dm_table_destroy(t);
> > +		dm_unlock_md_type(md);
> > +		r = -EINVAL;
> > +		goto out;
> > +	}
> 
> So you should be able to unlock md->type_lock here.

Yes.

> > Index: linux-2.6/drivers/md/dm.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/md/dm.c
> > +++ linux-2.6/drivers/md/dm.c
> > @@ -111,6 +111,15 @@ EXPORT_SYMBOL_GPL(dm_get_rq_mapinfo);
> >  #define DMF_QUEUE_IO_TO_THREAD 6
> >  
> >  /*
> > + * Type for md->type field.
> > + */
> > +enum mapped_device_type {
> > +	UNKNOWN_MD_TYPE,
> > +	BIO_BASED_MD_TYPE,
> > +	REQUEST_BASED_MD_TYPE,
> > +};
> 
> You can use the defined types in drivers/md/dm.h below instead;
> /*
>  * Type of table and mapped_device's mempool
>  */
> #define DM_TYPE_NONE            0
> #define DM_TYPE_BIO_BASED       1
> #define DM_TYPE_REQUEST_BASED   2
> 
> Using them should make type matching easier than making another definition.
> E.g. see below;
> 
> > +bool dm_md_type_matches_table(struct mapped_device *md, struct dm_table* t)
> > +{
> > +	if (dm_request_based_md_type(md))
> > +		return dm_table_request_based(t);
> > +	else if (dm_bio_based_md_type(md))
> > +		return !dm_table_request_based(t);
> > +
> > +	return 0;
> > +}
> 
> You can match type using "md->type == dm_table_get_type(t)" if you use
> the already defined types above.

Ah, yes that'll clean things up nicely.  No idea how I missed the
existing definitions.

Thanks,
Mike