[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>
This commit is contained in:
parent
4e90188be4
commit
2ca3310e78
142
drivers/md/dm.c
142
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;
|
||||
|
||||
write_lock(&md->map_lock);
|
||||
md->map = t;
|
||||
write_unlock(&md->map_lock);
|
||||
|
||||
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);
|
||||
|
||||
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;
|
||||
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);
|
||||
|
||||
dm_table_postsuspend_targets(map);
|
||||
|
||||
set_bit(DMF_SUSPENDED, &md->flags);
|
||||
|
||||
map = dm_get_table(md);
|
||||
dm_table_postsuspend_targets(map);
|
||||
r = 0;
|
||||
|
||||
out:
|
||||
dm_table_put(map);
|
||||
up_write(&md->lock);
|
||||
|
||||
return 0;
|
||||
|
||||
out_unfreeze:
|
||||
__unlock_fs(md);
|
||||
clear_bit(DMF_BLOCK_IO, &md->flags);
|
||||
out_write_unlock:
|
||||
up_write(&md->lock);
|
||||
return r;
|
||||
|
||||
out_read_unlock:
|
||||
up_read(&md->lock);
|
||||
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;
|
||||
}
|
||||
|
||||
|
|
Loading…
Reference in New Issue