All of lore.kernel.org
 help / color / mirror / Atom feed
From: Chandra Seetharaman <sekharan@us.ibm.com>
To: device-mapper development <dm-devel@redhat.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>,
	mpatocka@redhat.com, Alasdair G Kergon <agk@redhat.com>,
	Milan Broz <mbroz@redhat.com>
Subject: Re: updated Chandra's patch for dm_any_congested
Date: Mon, 10 Nov 2008 18:17:51 -0800	[thread overview]
Message-ID: <1226369871.14830.1186.camel@chandra-ubuntu> (raw)
In-Reply-To: <1226338466.5014.29.camel@localhost.localdomain>


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;
 }
 

      reply	other threads:[~2008-11-11  2:17 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-11-10 17:14 updated Chandra's patch for dm_any_congested Mikulas Patocka
2008-11-10 17:34 ` James Bottomley
2008-11-11  2:17   ` Chandra Seetharaman [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1226369871.14830.1186.camel@chandra-ubuntu \
    --to=sekharan@us.ibm.com \
    --cc=James.Bottomley@HansenPartnership.com \
    --cc=agk@redhat.com \
    --cc=dm-devel@redhat.com \
    --cc=mbroz@redhat.com \
    --cc=mpatocka@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.