Date: Sat, 6 Mar 2004 14:40:18 +0100
From: Miquel van Smoorenburg <miquels@cistron.nl>
To: Andrew Morton <akpm@osdl.org>
Cc: Joe Thornber <thornber@redhat.com>
Subject: Re: [PATCH] dm-rwlock.patch (Re: 2.6.4-rc1-mm1: queue-congestion-dm-implementation patch)
Message-ID: <20040306134018.GA13030@cistron.nl>
References: <cistron.200403011400.51008.kevcorry@us.ibm.com> <20040302130137.GA9941@cistron.nl> <200403020826.52448.kevcorry@us.ibm.com> <20040303211624.GA11008@cistron.nl> <20040305135544.318de1a6.akpm@osdl.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20040305135544.318de1a6.akpm@osdl.org>
User-Agent: Mutt/1.5.4i
Sender: Miquel van Smoorenburg <miquels@subspace.cistron-office.nl>

According to Andrew Morton:
> Miquel van Smoorenburg <miquels@cistron.nl> wrote:
> >
> > I posted a patch in the wrong thread earlier today, and the patch also
> > wasn't so good. This should be better, but I'll let Joe be the
> > judge of that. At least it survived my testing.
> 
> Joe just sent me a dm update which broke your patch big-time.
> 
> I've dropped a snapshot of my tree at
> http://www.zipworld.com.au/~akpm/linux/patches/stuff/x.gz (against rc2). 
> Would you have time to update and retest the rwlock patch?

Sure. I'm taking a new approach, since the rwlock patch was still
not correct - sleeping functions could still be called under
the rwlock, for example dm-crypt does that in its mapping function.
Fixing that would get way too complicated. So:

dm-maplock.patch

This patch adds a rwlock_t maplock to the struct mapped_device.
The ->map member is only updated with that lock held. dm_any_congested
takes the lock, and so can be sure that the table under ->map won't change.

This patch is much smaller and simpler than dm-rwlock.patch.

--- linux-2.6.4-rc2-mmX.orig/drivers/md/dm.c	2004-03-06 04:00:02.000000000 +0100
+++ linux-2.6.4-rc2-mmX/drivers/md/dm.c	2004-03-06 13:24:20.000000000 +0100
@@ -49,6 +49,7 @@
 
 struct mapped_device {
 	struct rw_semaphore lock;
+	rwlock_t maplock;
 	atomic_t holders;
 
 	unsigned long flags;
@@ -564,9 +565,12 @@
 	int r;
 	struct mapped_device *md = congested_data;
 
-	down_read(&md->lock);
-	r = dm_table_any_congested(md->map, bdi_bits);
-	up_read(&md->lock);
+	read_lock(&md->maplock);
+	if (md->map == NULL || test_bit(DMF_BLOCK_IO, &md->flags))
+		r = bdi_bits;
+	else
+		r = dm_table_any_congested(md->map, bdi_bits);
+	read_unlock(&md->maplock);
 
 	return r;
 }
@@ -642,6 +646,7 @@
 
 	memset(md, 0, sizeof(*md));
 	init_rwsem(&md->lock);
+	rwlock_init(&md->maplock);
 	atomic_set(&md->holders, 1);
 
 	md->queue = blk_alloc_queue(GFP_KERNEL);
@@ -741,7 +746,9 @@
 	if (size == 0)
 		return 0;
 
+	write_lock(&md->maplock);
 	md->map = t;
+	write_unlock(&md->maplock);
 	dm_table_event_callback(md->map, event_callback, md);
 
 	dm_table_get(t);
@@ -751,12 +758,16 @@
 
 static void __unbind(struct mapped_device *md)
 {
-	if (!md->map)
+	struct dm_table *map = md->map;
+
+	if (!map)
 		return;
 
-	dm_table_event_callback(md->map, NULL, NULL);
-	dm_table_put(md->map);
+	dm_table_event_callback(map, NULL, NULL);
+	write_lock(&md->maplock);
 	md->map = NULL;
+	write_unlock(&md->maplock);
+	dm_table_put(map);
 }
 
 /*