[PATCH] device-mapper: [3/4] Fix deadlocks in core

[Alasdair G Kergon]

This patch is an attempt to fix deadlocks discovered in the core dm.

The problems boil down to md->lock having to be held in too many
places, so I've split it into two: md->suspend_lock and md->io_lock.

suspend_lock is now held throughout dm_suspended() as well as
dm_resume() and dm_swap_table() so that these functions cannot
run concurrently: there's no requirement for that and it added
complexity.

DMF_FS_LOCKED becomes redundant: DMF_SUSPENDED provides adequate
protection.

Signed-Off-By: Alasdair G Kergon <agk@xxxxxxxxxx>

--- diff/drivers/md/dm.c	2005-07-11 22:57:58.000000000 +0100
+++ source/drivers/md/dm.c	2005-07-11 23:03:21.000000000 +0100
@@ -55,10 +55,10 @@
  */
 #define DMF_BLOCK_IO 0
 #define DMF_SUSPENDED 1
-#define DMF_FS_LOCKED 2
 
 struct mapped_device {
-	struct rw_semaphore lock;
+	struct rw_semaphore io_lock;
+	struct semaphore suspend_lock;
 	rwlock_t map_lock;
 	atomic_t holders;
 
@@ -248,16 +248,16 @@
  */
 static int queue_io(struct mapped_device *md, struct bio *bio)
 {
-	down_write(&md->lock);
+	down_write(&md->io_lock);
 
 	if (!test_bit(DMF_BLOCK_IO, &md->flags)) {
-		up_write(&md->lock);
+		up_write(&md->io_lock);
 		return 1;
 	}
 
 	bio_list_add(&md->deferred, bio);
 
-	up_write(&md->lock);
+	up_write(&md->io_lock);
 	return 0;		/* deferred successfully */
 }
 
@@ -568,14 +568,14 @@
 	int r;
 	struct mapped_device *md = q->queuedata;
 
-	down_read(&md->lock);
+	down_read(&md->io_lock);
 
 	/*
 	 * If we're suspended we have to queue
 	 * this io for later.
 	 */
 	while (test_bit(DMF_BLOCK_IO, &md->flags)) {
-		up_read(&md->lock);
+		up_read(&md->io_lock);
 
 		if (bio_rw(bio) == READA) {
 			bio_io_error(bio, bio->bi_size);
@@ -594,11 +594,11 @@
 		 * We're in a while loop, because someone could suspend
 		 * before we get to the following read lock.
 		 */
-		down_read(&md->lock);
+		down_read(&md->io_lock);
 	}
 
 	__split_bio(md, bio);
-	up_read(&md->lock);
+	up_read(&md->io_lock);
 	return 0;
 }
 
@@ -747,7 +747,8 @@
 		goto bad1;
 
 	memset(md, 0, sizeof(*md));
-	init_rwsem(&md->lock);
+	init_rwsem(&md->io_lock);
+	init_MUTEX(&md->suspend_lock);
 	rwlock_init(&md->map_lock);
 	atomic_set(&md->holders, 1);
 	atomic_set(&md->event_nr, 0);
@@ -844,13 +845,14 @@
 	if (size == 0)
 		return 0;
 
+	dm_table_get(t);
+	dm_table_event_callback(t, event_callback, md);
+
 	write_lock(&md->map_lock);
 	md->map = t;
+	dm_table_set_restrictions(t, q);
 	write_unlock(&md->map_lock);
 
-	dm_table_get(t);
-	dm_table_event_callback(t, event_callback, md);
-	dm_table_set_restrictions(t, q);
 	return 0;
 }
 
@@ -963,7 +965,7 @@
 {
 	int r = -EINVAL;
 
-	down_write(&md->lock);
+	down(&md->suspend_lock);
 
 	/* device must be suspended */
 	if (!dm_suspended(md))
@@ -973,7 +975,7 @@
 	r = __bind(md, table);
 
 out:
-	up_write(&md->lock);
+	up(&md->suspend_lock);
 	return r;
 }
 
@@ -981,16 +983,13 @@
  * Functions to lock and unlock any filesystem running on the
  * device.
  */
-static int __lock_fs(struct mapped_device *md)
+static int lock_fs(struct mapped_device *md)
 {
 	int r = -ENOMEM;
 
-	if (test_and_set_bit(DMF_FS_LOCKED, &md->flags))
-		return 0;
-
 	md->frozen_bdev = bdget_disk(md->disk, 0);
 	if (!md->frozen_bdev) {
-		DMWARN("bdget failed in __lock_fs");
+		DMWARN("bdget failed in lock_fs");
 		goto out;
 	}
 
@@ -1004,7 +1003,7 @@
 
 	/* don't bdput right now, we don't want the bdev
 	 * to go away while it is locked.  We'll bdput
-	 * in __unlock_fs
+	 * in unlock_fs
 	 */
 	return 0;
 
@@ -1013,15 +1012,11 @@
 	md->frozen_sb = NULL;
 	md->frozen_bdev = NULL;
 out:
-	clear_bit(DMF_FS_LOCKED, &md->flags);
 	return r;
 }
 
-static void __unlock_fs(struct mapped_device *md)
+static void unlock_fs(struct mapped_device *md)
 {
-	if (!test_and_clear_bit(DMF_FS_LOCKED, &md->flags))
-		return;
-
 	thaw_bdev(md->frozen_bdev, md->frozen_sb);
 	bdput(md->frozen_bdev);
 
@@ -1038,13 +1033,14 @@
  */
 int dm_suspend(struct mapped_device *md)
 {
-	struct dm_table *map;
+	struct dm_table *map = NULL;
 	DECLARE_WAITQUEUE(wait, current);
 	int r = -EINVAL;
 
-	down_read(&md->lock);
-	if (test_bit(DMF_BLOCK_IO, &md->flags))
-		goto out_read_unlock;
+	down(&md->suspend_lock);
+
+	if (dm_suspended(md))
+		goto out;
 
 	map = dm_get_table(md);
 
@@ -1052,36 +1048,22 @@
 	dm_table_presuspend_targets(map);
 
 	/* Flush I/O to the device. */
-	r = __lock_fs(md);
-	if (r) {
-		dm_table_put(map);
-		goto out_read_unlock;
-	}
-
-	up_read(&md->lock);
+	r = lock_fs(md);
+	if (r)
+		goto out;
 
 	/*
 	 * First we set the BLOCK_IO flag so no more ios will be mapped.
-	 *
-	 * If the flag is already set we know another thread is trying to
-	 * suspend as well, so we leave the fs locked for this thread.
 	 */
-	r = -EINVAL;
-	down_write(&md->lock);
-	if (test_and_set_bit(DMF_BLOCK_IO, &md->flags)) {
-		if (map)
-			dm_table_put(map);
-		goto out_write_unlock;
-	}
+	down_write(&md->io_lock);
+	set_bit(DMF_BLOCK_IO, &md->flags);
 
 	add_wait_queue(&md->wait, &wait);
-	up_write(&md->lock);
+	up_write(&md->io_lock);
 
 	/* unplug */
-	if (map) {
+	if (map)
 		dm_table_unplug_all(map);
-		dm_table_put(map);
-	}
 
 	/*
 	 * Then we wait for the already mapped ios to
@@ -1097,32 +1079,28 @@
 	}
 	set_current_state(TASK_RUNNING);
 
-	down_write(&md->lock);
+	down_write(&md->io_lock);
 	remove_wait_queue(&md->wait, &wait);
 
 	/* were we interrupted ? */
 	r = -EINTR;
-	if (atomic_read(&md->pending))
-		goto out_unfreeze;
-
-	set_bit(DMF_SUSPENDED, &md->flags);
+	if (atomic_read(&md->pending)) {
+		up_write(&md->io_lock);
+		unlock_fs(md);
+		clear_bit(DMF_BLOCK_IO, &md->flags);
+		goto out;
+	}
+	up_write(&md->io_lock);
 
-	map = dm_get_table(md);
 	dm_table_postsuspend_targets(map);
-	dm_table_put(map);
-	up_write(&md->lock);
 
-	return 0;
+	set_bit(DMF_SUSPENDED, &md->flags);
 
-out_unfreeze:
-	__unlock_fs(md);
-	clear_bit(DMF_BLOCK_IO, &md->flags);
-out_write_unlock:
-	up_write(&md->lock);
-	return r;
+	r = 0;
 
-out_read_unlock:
-	up_read(&md->lock);
+out:
+	dm_table_put(map);
+	up(&md->suspend_lock);
 	return r;
 }
 
@@ -1132,31 +1110,35 @@
 	struct bio *def;
 	struct dm_table *map = NULL;
 
-	down_write(&md->lock);
-	if (!dm_suspended(md)) {
-		up_write(&md->lock);
+	down(&md->suspend_lock);
+	if (!dm_suspended(md))
 		goto out;
-	}
 
 	map = dm_get_table(md);
-	if (!map || !dm_table_get_size(map)) {
-		up_write(&md->lock);
+	if (!map || !dm_table_get_size(map))
 		goto out;
-	}
 
 	dm_table_resume_targets(map);
-	clear_bit(DMF_SUSPENDED, &md->flags);
+
+	down_write(&md->io_lock);
 	clear_bit(DMF_BLOCK_IO, &md->flags);
 
 	def = bio_list_get(&md->deferred);
 	__flush_deferred_io(md, def);
-	up_write(&md->lock);
-	__unlock_fs(md);
+	up_write(&md->io_lock);
+
+	unlock_fs(md);
+
+	clear_bit(DMF_SUSPENDED, &md->flags);
+
 	dm_table_unplug_all(map);
 
 	r = 0;
+
 out:
 	dm_table_put(map);
+	up(&md->suspend_lock);
+
 	return r;
 }
 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/