dm: restrict conflicting table loads and improve queue initialization

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 <[email protected]>

Index: linux-2.6/drivers/md/dm-ioctl.c
===================================================================

drivers/md/dm-ioctl.c +2 -0

@@ -1176,12 +1176,39 @@ static int table_load(struct dm_ioctl *p
 		goto out;
 	}
 
+	/*
+	 * Protect md->type against concurrent table loads.

Index: linux-2.6/drivers/md/dm.c
===================================================================

Hi Mike,

Quoted 18 lines · Mike Snitzer

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

Quoted 1 line

Signed-off-by Mike Snitzer <[email protected]>

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.

Quoted 17 lines

Index: linux-2.6/drivers/md/dm-ioctl.c
===================================================================

drivers/md/dm-ioctl.c +9 -0

@@ -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)

drivers/md/dm-ioctl.c static int table_load(struct dm_ioctl *p · line 1190

Quoted 3 lines

drivers/md/dm-ioctl.c +3 -0

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

Quoted 6 lines

drivers/md/dm-ioctl.c +6 -0

+	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)) {

drivers/md/dm-ioctl.c static int table_load(struct dm_ioctl *p · line 1202

Quoted 6 lines

drivers/md/dm-ioctl.c +6 -0

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

Quoted 5 lines

Index: linux-2.6/drivers/md/dm.c
===================================================================

drivers/md/dm.c EXPORT_SYMBOL_GPL(dm_get_rq_mapinfo); · line 120

Quoted 10 lines

drivers/md/dm.c +7 -0

@@ -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;

drivers/md/dm.c int dm_create(int minor, struct mapped_d · line 2193

Quoted 9 lines

drivers/md/dm.c +9 -0

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

Index: linux-2.6/drivers/md/dm-ioctl.c
===================================================================

drivers/md/dm-ioctl.c +0 -0

@@ -1201,6 +1203,15 @@ static int table_load(struct dm_ioctl *p
 		goto out;
 	}

drivers/md/dm-ioctl.c +18 -0

+static void dm_clear_request_based_queue(struct mapped_device *md)
+{
+	if (dm_bio_based_md_queue(md))
+		return; /* queue already bio-based */
+
+	/* Unregister elevator from sysfs and clear ->request_fn */
+	elv_unregister_queue(md->queue);
+	md->queue->request_fn = NULL;
+}
+
+/*
+ * Setup the DM device's queue based on md's type
+ */
+int dm_setup_md_queue(struct mapped_device *md)
+{
+	BUG_ON(!mutex_is_locked(&md->type_lock));
+	BUG_ON(dm_unknown_md_type(md));
+

Hi Mike,

Quoted 8 lines · Mike Snitzer

Index: linux-2.6/drivers/md/dm-ioctl.c
===================================================================

drivers/md/dm-ioctl.c +0 -0

@@ -1201,6 +1203,15 @@ static int table_load(struct dm_ioctl *p
 		goto out;
 	}
 

drivers/md/dm-ioctl.c static int table_load(struct dm_ioctl *p · line 1214

Quoted 9 lines

drivers/md/dm-ioctl.c +9 -0

+	/* setup md->queue to reflect md's and table's type (may block) */
+	r = dm_setup_md_queue(md);
+	if (r) {
+		DMWARN("unable to setup device queue for this table.");
+		dm_table_destroy(t);
+		dm_unlock_md_type(md);
+		goto out;
+	}
+

dm_setup_md_queue() should be put just after dm_set_md_type() of patch#1. Then, you can make sure that dm_setup_md_queue() is called only once and dm_setup_md_queue() should be much simpler.

Quoted 8 lines

drivers/md/dm-ioctl.c +8 -0

+/*
+ * Fully initialize a request-based queue (->elevator, ->request_fn, etc).
+ */
+static int dm_init_request_based_queue(struct mapped_device *md)
+{
+	struct request_queue *q = NULL;
+
+	/* Avoid re-initializing the queue if already fully initialized */

drivers/md/dm-ioctl.c

Quoted 5 lines

drivers/md/dm-ioctl.c +5 -0

+	if (!md->queue->elevator) {
+		/* Fully initialize the queue */
+		q = blk_init_allocated_queue(md->queue, dm_request_fn, NULL);
+		if (!q)
+			return 0;

When blk_init_allocated_queue() fails, the block-layer seems not to guarantee that the queue is still available.

You should create appropriate block-layer interfaces and/or take proper recovery action here.
However, if the failure is not realistic, rather than complicating this patch, you can just put a big comment and WARN_ON() for now, and fix it in a separate patch-set.

Quoted 23 lines

drivers/md/dm-ioctl.c +23 -0

+static void dm_clear_request_based_queue(struct mapped_device *md)
+{
+	if (dm_bio_based_md_queue(md))
+		return; /* queue already bio-based */
+
+	/* Unregister elevator from sysfs and clear ->request_fn */
+	elv_unregister_queue(md->queue);
+	md->queue->request_fn = NULL;
+}
+
+/*
+ * Setup the DM device's queue based on md's type
+ */
+int dm_setup_md_queue(struct mapped_device *md)
+{
+	BUG_ON(!mutex_is_locked(&md->type_lock));
+	BUG_ON(dm_unknown_md_type(md));
+
+	if (dm_request_based_md_type(md)) {
+		if (!dm_init_request_based_queue(md)) {
+			DMWARN("Cannot initialize queue for Request-based dm");
+			return -EINVAL;
+		}

drivers/md/dm-ioctl.c

Quoted 5 lines

drivers/md/dm-ioctl.c +5 -0

+	} else if (dm_bio_based_md_type(md))
+		dm_clear_request_based_queue(md);
+
+	return 0;
+}

These unregister/clear works shouldn't be needed if dm_setup_md_queue() is called only once as I mentioned above.

Quoted 8 lines · Kiyoshi Ueda

Quoted 8 lines

(quoted diff) +8 -0

+/*
+ * Fully initialize a request-based queue (->elevator, ->request_fn, etc).
+ */
+static int dm_init_request_based_queue(struct mapped_device *md)
+{
+	struct request_queue *q = NULL;
+
+	/* Avoid re-initializing the queue if already fully initialized */

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+	if (!md->queue->elevator) {
+		/* Fully initialize the queue */
+		q = blk_init_allocated_queue(md->queue, dm_request_fn, NULL);
+		if (!q)
+			return 0;

When blk_init_allocated_queue() fails, the block-layer seems not to guarantee that the queue is still available.

Ouch, yes this portion of blk_init_allocated_queue_node() is certainly problematic:

        if (blk_init_free_list(q)) {
                kmem_cache_free(blk_requestq_cachep, q);
                return NULL;
        }

Cc'ing Jens as I think it would be advantageous for us to push the above kmem_cache_free() into the callers where it really makes sense, e.g.:
blk_init_queue_node().

So on blk_init_allocated_queue_node() failure blk_init_queue_node() will take care to cleanup the queue that it assumes it is managing completely.

My patch (linux-2.6-block.git's commit: 01effb0) that split out blk_init_allocated_queue_node() from blk_init_queue_node() opened up this issue. I'm fairly confident we'll get it fixed by the time 2.6.35 ships.

Quoted 18 lines · Mike Snitzer

Quoted 16 lines · Kiyoshi Ueda

Review

Quoted 13 lines

(quoted diff) +13 -0

+/*
+ * Fully initialize a request-based queue (->elevator, ->request_fn, etc).
+ */
+static int dm_init_request_based_queue(struct mapped_device *md)
+{
+	struct request_queue *q = NULL;
+
+	/* Avoid re-initializing the queue if already fully initialized */
+	if (!md->queue->elevator) {
+		/* Fully initialize the queue */
+		q = blk_init_allocated_queue(md->queue, dm_request_fn, NULL);
+		if (!q)
+			return 0;

When blk_init_allocated_queue() fails, the block-layer seems not to guarantee that the queue is still available.

Quoted 7 lines

Ouch, yes this portion of blk_init_allocated_queue_node() is certainly problematic:

        if (blk_init_free_list(q)) {
                kmem_cache_free(blk_requestq_cachep, q);
                return NULL;
        }

Hi Mike,

Quoted 19 lines · Mike Snitzer

Quoted 18 lines · Mike Snitzer

Quoted 16 lines · Kiyoshi Ueda

Review

Quoted 13 lines

(quoted diff) +13 -0

+/*
+ * Fully initialize a request-based queue (->elevator, ->request_fn, etc).
+ */
+static int dm_init_request_based_queue(struct mapped_device *md)
+{
+	struct request_queue *q = NULL;
+
+	/* Avoid re-initializing the queue if already fully initialized */
+	if (!md->queue->elevator) {
+		/* Fully initialize the queue */
+		q = blk_init_allocated_queue(md->queue, dm_request_fn, NULL);
+		if (!q)
+			return 0;

When blk_init_allocated_queue() fails, the block-layer seems not to guarantee that the queue is still available.

Quoted 7 lines

Quoted 7 lines

Ouch, yes this portion of blk_init_allocated_queue_node() is certainly problematic:

        if (blk_init_free_list(q)) {
                kmem_cache_free(blk_requestq_cachep, q);
                return NULL;
        }

Not only that. The blk_put_queue() in blk_init_allocated_queue_node() will also free the queue:

	if (!elevator_init(q, NULL)) {
		blk_queue_congestion_threshold(q);
		return q;
	}

	blk_put_queue(q);
	return NULL;

block: avoid unconditionally freeing previously allocated request_queue

On blk_init_allocated_queue_node failure, only free the request_queue if it is wasn't previously allocated outside the block layer (e.g. blk_init_queue_node was blk_init_allocated_queue_node caller).

This addresses an interface bug introduced by the following commit:
01effb0 block: allow initialization of previously allocated request_queue

Otherwise the request_queue may be free'd out from underneath a caller that is managing the request_queue directly (e.g. caller uses blk_alloc_queue + blk_init_allocated_queue_node).