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

[Kiyoshi Ueda]

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 at 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.


> 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.

> +	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.


> 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.

Thanks,
Kiyoshi Ueda