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@xxxxxxxxxx>
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
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
