updated Chandra's patch for dm_any_congested

updated Chandra's patch for dm_any_congested

[Mikulas Patocka]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 572 bytes --]

Hi

This is the updated patch for dm_any_congested, the only change is that
atomic_dec(&md->pending) is changed into
if (!atomic_dec_return(&md->pending)) wake_up(&md->wait);

--- so that there is not a possibility to deadlock device suspend that 
wait for pending variable to drop to zero.

You can put it into your series as a quick "stop crash" patch, until we 
find a better solution as we talked about today.

BTW. if you some times ago wondered why I'm using yield() and not waiting 
queues in performance non-critical paths, it's to avoid bugs like this :)

Mikulas

[-- Attachment #2: Type: TEXT/PLAIN, Size: 2036 bytes --]

From: Chandra Seetharaman <sekharan@us.ibm.com>

dm_any_congested() just checks for the DMF_BLOCK_IO and has no
code to make sure that suspend waits for dm_any_congested() to
complete.  This patch adds such a check.

Without it, a race can occur with dm_table_put() attempting to
destroying the table in the wrong thread, the one running
dm_any_congested() which is meant to be quick and return
immediately.

Two examples of problems:
1. Sleeping functions called from congested code, the caller
   of which holds a spin lock.
2. An ABBA deadlock between pdflush and multipathd. The two locks
   in contention are inode lock and kernel lock.

[AGK: Is the return value always correct when the table is skipped?]

Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>
Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Signed-off-by: Alasdair G Kergon <agk@redhat.com>
---
 drivers/md/dm.c |   20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

Index: linux-2.6.28-rc3-devel/drivers/md/dm.c
===================================================================
--- linux-2.6.28-rc3-devel.orig/drivers/md/dm.c	2008-11-05 09:19:18.000000000 +0100
+++ linux-2.6.28-rc3-devel/drivers/md/dm.c	2008-11-10 18:05:24.000000000 +0100
@@ -941,16 +941,22 @@ static void dm_unplug_all(struct request
 
 static int dm_any_congested(void *congested_data, int bdi_bits)
 {
-	int r;
+	int r = bdi_bits;
 	struct mapped_device *md = (struct mapped_device *) congested_data;
-	struct dm_table *map = dm_get_table(md);
+	struct dm_table *map;
 
-	if (!map || test_bit(DMF_BLOCK_IO, &md->flags))
-		r = bdi_bits;
-	else
-		r = dm_table_any_congested(map, bdi_bits);
+	atomic_inc(&md->pending);
+	if (test_bit(DMF_BLOCK_IO, &md->flags))
+		goto done;
 
-	dm_table_put(map);
+	map = dm_get_table(md);
+	if (map) {
+		r = dm_table_any_congested(map, bdi_bits);
+		dm_table_put(map);
+	}
+done:
+	if (!atomic_dec_return(&md->pending))
+		wake_up(&md->wait);
 	return r;
 }
 

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: updated Chandra's patch for dm_any_congested

[James Bottomley]

On Mon, 2008-11-10 at 12:14 -0500, Mikulas Patocka wrote:
> --- so that there is not a possibility to deadlock device suspend
> that 
> wait for pending variable to drop to zero.
> 
> You can put it into your series as a quick "stop crash" patch, until
> we 
> find a better solution as we talked about today.
> 
> BTW. if you some times ago wondered why I'm using yield() and not
> waiting 
> queues in performance non-critical paths, it's to avoid bugs like
> this :)


> From: Chandra Seetharaman <sekharan@us.ibm.com>
> 
> dm_any_congested() just checks for the DMF_BLOCK_IO and has no
> code to make sure that suspend waits for dm_any_congested() to
> complete.  This patch adds such a check.
> 
> Without it, a race can occur with dm_table_put() attempting to
> destroying the table in the wrong thread, the one running
> dm_any_congested() which is meant to be quick and return
> immediately.
> 
> Two examples of problems:
> 1. Sleeping functions called from congested code, the caller
>    of which holds a spin lock.
> 2. An ABBA deadlock between pdflush and multipathd. The two locks
>    in contention are inode lock and kernel lock.
> 
> [AGK: Is the return value always correct when the table is skipped?]
> 
> Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

Stylistically, there needs to be a brief description of your alteration
in the text above (traditionally in square brackets preceeded by your
email address so we know how the patch got altered).

> Signed-off-by: Alasdair G Kergon <agk@redhat.com>
> ---
>  drivers/md/dm.c |   20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> Index: linux-2.6.28-rc3-devel/drivers/md/dm.c
> ===================================================================
> --- linux-2.6.28-rc3-devel.orig/drivers/md/dm.c 2008-11-05
> 09:19:18.000000000 +0100
> +++ linux-2.6.28-rc3-devel/drivers/md/dm.c      2008-11-10
> 18:05:24.000000000 +0100
> @@ -941,16 +941,22 @@ static void dm_unplug_all(struct request
>  
>  static int dm_any_congested(void *congested_data, int bdi_bits)
>  {
> -       int r;
> +       int r = bdi_bits;
>         struct mapped_device *md = (struct mapped_device *)
> congested_data;
> -       struct dm_table *map = dm_get_table(md);
> +       struct dm_table *map;
>  
> -       if (!map || test_bit(DMF_BLOCK_IO, &md->flags))
> -               r = bdi_bits;
> -       else
> -               r = dm_table_any_congested(map, bdi_bits);
> +       atomic_inc(&md->pending);
> +       if (test_bit(DMF_BLOCK_IO, &md->flags))
> +               goto done;
>  
> -       dm_table_put(map);
> +       map = dm_get_table(md);
> +       if (map) {
> +               r = dm_table_any_congested(map, bdi_bits);
> +               dm_table_put(map);
> +       }
> +done:

This isn't really the best use of goto.  Intentation isn't going wild
here, so it should be

if (!test_bit(DMF_BLOCK_IO, &md->flags)) {
	... all the code
}
... code after done

If there were more than one goto done; for the error out case, then
fine, but there's only this one.

James

Re: updated Chandra's patch for dm_any_congested

[Chandra Seetharaman]

Here is an updated version based on James's comments:

From: Chandra Seetharaman <sekharan@us.ibm.com>

dm_any_congested() just checks for the DMF_BLOCK_IO and has no
code to make sure that suspend waits for dm_any_congested() to
complete.  This patch adds such a check.

Without it, a race can occur with dm_table_put() attempting to
destroying the table in the wrong thread, the one running
dm_any_congested() which is meant to be quick and return
immediately.

Two examples of problems:
1. Sleeping functions called from congested code, the caller
   of which holds a spin lock.
2. An ABBA deadlock between pdflush and multipathd. The two locks
   in contention are inode lock and kernel lock.
----------------
Updates from the original patch:
 - Mikulas changed 
	atomic_dec(&md->pending));
   to
	if (!atomic_dec_return(&md->pending))
		wake_up(&md->wait);
 - Chandra changed the code organization to get rid of the goto
   as per James Bottemley' suggestion.
----------------

[AGK: Is the return value always correct when the table is skipped?]

Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>
Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Signed-off-by: Alasdair G Kergon <agk@redhat.com>
--------

Index: linux-2.6.28-rc3/drivers/md/dm.c
===================================================================
--- linux-2.6.28-rc3.orig/drivers/md/dm.c
+++ linux-2.6.28-rc3/drivers/md/dm.c
@@ -937,16 +937,20 @@ static void dm_unplug_all(struct request
 
 static int dm_any_congested(void *congested_data, int bdi_bits)
 {
-	int r;
+	int r = bdi_bits;
 	struct mapped_device *md = (struct mapped_device *) congested_data;
-	struct dm_table *map = dm_get_table(md);
-
-	if (!map || test_bit(DMF_BLOCK_IO, &md->flags))
-		r = bdi_bits;
-	else
-		r = dm_table_any_congested(map, bdi_bits);
+	struct dm_table *map;
 
-	dm_table_put(map);
+	atomic_inc(&md->pending);
+	if (!test_bit(DMF_BLOCK_IO, &md->flags)) {
+		map = dm_get_table(md);
+		if (map) {
+			r = dm_table_any_congested(map, bdi_bits);
+			dm_table_put(map);
+		}
+	}
+	if (!atomic_dec_return(&md->pending))
+		wake_up(&md->wait);
 	return r;
 }