[PATCH] device-mapper: fix md->lock deadlocks in core

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@redhat.com>
Signed-off-by: Andrew Morton <akpm@osdl.org>
Signed-off-by: Linus Torvalds <torvalds@osdl.org>

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 88f1f27..d487d9d 100644 (file)
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -55,10 +55,10 @@ union map_info *dm_get_mapinfo(struct bio *bio)
  */
 #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 inline void free_tio(struct mapped_device *md, struct target_io *tio)
  */
 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 @@ static int dm_request(request_queue_t *q, struct bio *bio)
        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 @@ static int dm_request(request_queue_t *q, struct bio *bio)
                 * 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 @@ static struct mapped_device *alloc_dev(unsigned int minor, int persistent)
                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 @@ static int __bind(struct mapped_device *md, struct dm_table *t)
        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 dm_swap_table(struct mapped_device *md, struct dm_table *table)
 {
        int r = -EINVAL;
 
-       down_write(&md->lock);
+       down(&md->suspend_lock);
 
        /* device must be suspended */
        if (!dm_suspended(md))
@@ -973,7 +975,7 @@ int dm_swap_table(struct mapped_device *md, struct dm_table *table)
        r = __bind(md, table);
 
 out:
-       up_write(&md->lock);
+       up(&md->suspend_lock);
        return r;
 }
 
@@ -981,16 +983,13 @@ out:
  * 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 @@ static int __lock_fs(struct mapped_device *md)
 
        /* 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 @@ out_bdput:
        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 @@ static void __unlock_fs(struct mapped_device *md)
  */
 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 @@ int dm_suspend(struct mapped_device *md)
        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 @@ int dm_suspend(struct mapped_device *md)
        }
        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 @@ int dm_resume(struct mapped_device *md)
        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;
 }