[8/14] barriers

Commit Message

Mikulas Patocka Feb. 23, 2009, 7:21 p.m. UTC

Move wait queue declaration and unplug to dm_wait_for_completion.

The purpose is to minimize duplicate code in the further patches.

The patch reorders functions a little bit. It doesn't change any
functionality. For proper non-deadlock operation, it is required 
hat add_wait_queue happens before set_current_state(interruptible)
and before the test for !atomic_read(&md->pending) --- and this
patch follows that.

Signed-off-by: Mikulas Patocka <[e-mail]>

---
 drivers/md/dm.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)


--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Comments

Alasdair G Kergon March 26, 2009, 11:11 a.m. UTC | #1

On Mon, Feb 23, 2009 at 02:21:41PM -0500, Mikulas Patocka wrote:
> Move wait queue declaration and unplug to dm_wait_for_completion.
> The patch reorders functions a little bit. It doesn't change any
> functionality. For proper non-deadlock operation, it is required 
> hat add_wait_queue happens before set_current_state(interruptible)
> and before the test for !atomic_read(&md->pending) --- and this
> patch follows that.
 
Can you check md->wait is sufficiently protected and the reordering
in this patch doesn't increase the likelihood of a race with its
contents changing outside a lock?

Alasdair

Patch

Index: linux-2.6.29-rc3-devel/drivers/md/dm.c
===================================================================
--- linux-2.6.29-rc3-devel.orig/drivers/md/dm.c	2009-02-05 05:52:18.000000000 +0100
+++ linux-2.6.29-rc3-devel/drivers/md/dm.c	2009-02-05 05:52:19.000000000 +0100
@@ -1375,6 +1375,11 @@  EXPORT_SYMBOL_GPL(dm_put);
 static int dm_wait_for_completion(struct mapped_device *md)
 {
 	int r = 0;
+	DECLARE_WAITQUEUE(wait, current);
+
+	dm_unplug_all(md->queue);
+
+	add_wait_queue(&md->wait, &wait);
 
 	while (1) {
 		set_current_state(TASK_INTERRUPTIBLE);
@@ -1392,6 +1397,8 @@  static int dm_wait_for_completion(struct
 	}
 	set_current_state(TASK_RUNNING);
 
+	remove_wait_queue(&md->wait, &wait);
+
 	return r;
 }
 
@@ -1497,7 +1504,6 @@  static void unlock_fs(struct mapped_devi
 int dm_suspend(struct mapped_device *md, unsigned suspend_flags)
 {
 	struct dm_table *map = NULL;
-	DECLARE_WAITQUEUE(wait, current);
 	int r = 0;
 	int do_lockfs = suspend_flags & DM_SUSPEND_LOCKFS_FLAG ? 1 : 0;
 	int noflush = suspend_flags & DM_SUSPEND_NOFLUSH_FLAG ? 1 : 0;
@@ -1540,20 +1546,14 @@  int dm_suspend(struct mapped_device *md,
 	down_write(&md->io_lock);
 	set_bit(DMF_BLOCK_IO, &md->flags);
 
-	add_wait_queue(&md->wait, &wait);
 	up_write(&md->io_lock);
 
-	/* unplug */
-	if (map)
-		dm_table_unplug_all(map);
-
 	/*
 	 * Wait for the already-mapped ios to complete.
 	 */
 	r = dm_wait_for_completion(md);
 
 	down_write(&md->io_lock);
-	remove_wait_queue(&md->wait, &wait);
 
 	if (noflush)
 		__merge_pushback_list(md);