[PATCH 000 of 4] Introduction

[PATCH 000 of 4] Introduction

[NeilBrown]

Following 4 patches address issues with lockdep, particularly around bd_mutex.

They are against 2.6.18-mm3 and do *not* apply against -linus as -mm already has
some changes to the handling of bd_mutex nesting.  2-4 probably apply on top of -linus plus
-mm/broken-out/remove-the-old-bd_mutex-lockdep-annotation.patch  

I believe they are probably ok for 2.6.19.

The core issue is that blkdev_get when called on a partition needs to
lock (bd_mutex) the partition and the whole device.  lockdep would
normally see this as a possible deadlock and needs to be told that
this particular nesting is known to be safe.

The code to do this is in -linus is rather messy, largely because the
locking itself is messy.  The bd_mutex for the whole is taken several
times while bd_mutex for the partition is held, and it is taken at
both levels of the recursion (blkdev_get calls blkdev_get - only to
one level).

As key observation to simplifying the locking is to observer that a
lot of the locking is there to protect the updating of bd_part_count.
If those updates are moved, the locking can become simpler.

The first patch removes the current approach in -mm to handling this
nesting and explains why it is not ideal.
This reverts new-bd_mutex-lockdep-annotation.patch

The second simplifies the locking as explained above.

The third adds the mutex_lock_nested annotations, which are now trivial.

The last fixes a tangentially related lockdep problem in md - there is
a false relationship between bd_mutex and md->reconfig_mutex which
needs to be clarified.

 [PATCH 000 of 4] Introduction
 [PATCH 001 of 4] Remove lock_key approach to managing nested bd_mutex locks.
 [PATCH 002 of 4] Simplify some aspects of bd_mutex nesting.
 [PATCH 003 of 4] Use mutex_lock_nested for bd_mutex to avoid  lockdep warning.
 [PATCH 004 of 4] Avoid lockdep warning in md.

[PATCH 001 of 4] Remove lock_key approach to managing nested bd_mutex locks.

[NeilBrown]

The extra call to get_gendisk is not good.  It causes a ->probe and possible
module load before it is really appropriate to do this.

Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./fs/block_dev.c |    9 ---------
 1 file changed, 9 deletions(-)

diff .prev/fs/block_dev.c ./fs/block_dev.c
--- .prev/fs/block_dev.c	2006-10-11 15:37:05.000000000 +1000
+++ ./fs/block_dev.c	2006-10-11 15:37:10.000000000 +1000
@@ -357,14 +357,10 @@ static int bdev_set(struct inode *inode,
 
 static LIST_HEAD(all_bdevs);
 
-static struct lock_class_key bdev_part_lock_key;
-
 struct block_device *bdget(dev_t dev)
 {
 	struct block_device *bdev;
 	struct inode *inode;
-	struct gendisk *disk;
-	int part = 0;
 
 	inode = iget5_locked(bd_mnt->mnt_sb, hash(dev),
 			bdev_test, bdev_set, &dev);
@@ -390,11 +386,6 @@ struct block_device *bdget(dev_t dev)
 		list_add(&bdev->bd_list, &all_bdevs);
 		spin_unlock(&bdev_lock);
 		unlock_new_inode(inode);
-		mutex_init(&bdev->bd_mutex);
-		disk = get_gendisk(dev, &part);
-		if (part)
-			lockdep_set_class(&bdev->bd_mutex, &bdev_part_lock_key);
-		put_disk(disk);
 	}
 	return bdev;
 }

[PATCH 002 of 4] Simplify some aspects of bd_mutex nesting.

[NeilBrown]

When we open (actually blkdev_get) a partition we need to also open
(get) the whole device that holds the partition.  The involves some
limited recursion.  This patch tries to simplify some aspects of this.

As well as opening the whole device, we need to increment
->bd_part_count when a partition is opened (this is used by
rescan_partitions to avoid a rescan if any partition is active, as
that would be confusing).

The main change this patch makes is to move the inc/dec of
bd_part_count into blkdev_{get,put} for the whole rather than doing it
in blkdev_{get,put} for the partition.

More specifically, we introduce __blkdev_get and __blkdev_put which do
exactly what blkdev_{get,put} did, only with an extra "for_part"
argument (blkget_{get,put} then call the __ version with a '0' for the
extra argument).

If for_part is 1, then the blkdev is being get(put) because a
partition is being opened(closed) for the first(last) time, and so
bd_part_count should be updated (on success).  The particular
advantage of pushing this function down is that the bd_mutex lock
(which is needed to update bd_part_count) is already held at the lower
level.

Note that this slightly changes the semantics of bd_part_count.
Instead of updating it whenever a partition is opened or released, it
is now only updated on the first open or last release.  This is an
adequate semantic as it is only ever tested for "== 0".

Having introduced these functions we remove the current bd_part_count
updates from do_open (which is really the body of blkdev_get) and call
__blkdev_get(... 1).
Similarly in blkget_put we remove the old bd_part_count updates and
call __blkget_put(..., 1).  This call is moved to the end of
__blkdev_put to avoid nested locks of bd_mutex.

Finally the mutex_lock on whole->bd_mutex in do_open can be removed.
It was only really needed to protect bd_part_count, and that is now
managed (and protected) within the recursive call.


The observation that bd_part_count is central to the locking issues,
and the modifications to create __blkdev_put are from Peter Zijlstra.

Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./fs/block_dev.c |   51 +++++++++++++++++++++++++++++----------------------
 1 file changed, 29 insertions(+), 22 deletions(-)

diff .prev/fs/block_dev.c ./fs/block_dev.c
--- .prev/fs/block_dev.c	2006-10-11 15:37:10.000000000 +1000
+++ ./fs/block_dev.c	2006-10-11 15:38:31.000000000 +1000
@@ -889,7 +889,10 @@ void bd_set_size(struct block_device *bd
 }
 EXPORT_SYMBOL(bd_set_size);
 
-static int do_open(struct block_device *bdev, struct file *file)
+static int __blkdev_get(struct block_device *bdev, mode_t mode, unsigned flags,
+			int for_part);
+
+static int do_open(struct block_device *bdev, struct file *file, int for_part)
 {
 	struct module *owner = NULL;
 	struct gendisk *disk;
@@ -933,25 +936,21 @@ static int do_open(struct block_device *
 			ret = -ENOMEM;
 			if (!whole)
 				goto out_first;
-			ret = blkdev_get(whole, file->f_mode, file->f_flags);
+			BUG_ON(for_part);
+			ret = __blkdev_get(whole, file->f_mode, file->f_flags, 1);
 			if (ret)
 				goto out_first;
 			bdev->bd_contains = whole;
-			mutex_lock(&whole->bd_mutex);
-			whole->bd_part_count++;
 			p = disk->part[part - 1];
 			bdev->bd_inode->i_data.backing_dev_info =
 			   whole->bd_inode->i_data.backing_dev_info;
 			if (!(disk->flags & GENHD_FL_UP) || !p || !p->nr_sects) {
-				whole->bd_part_count--;
-				mutex_unlock(&whole->bd_mutex);
 				ret = -ENXIO;
 				goto out_first;
 			}
 			kobject_get(&p->kobj);
 			bdev->bd_part = p;
 			bd_set_size(bdev, (loff_t) p->nr_sects << 9);
-			mutex_unlock(&whole->bd_mutex);
 		}
 	} else {
 		put_disk(disk);
@@ -964,13 +963,11 @@ static int do_open(struct block_device *
 			}
 			if (bdev->bd_invalidated)
 				rescan_partitions(bdev->bd_disk, bdev);
-		} else {
-			mutex_lock(&bdev->bd_contains->bd_mutex);
-			bdev->bd_contains->bd_part_count++;
-			mutex_unlock(&bdev->bd_contains->bd_mutex);
 		}
 	}
 	bdev->bd_openers++;
+	if (for_part)
+		bdev->bd_part_count++;
 	mutex_unlock(&bdev->bd_mutex);
 	unlock_kernel();
 	return 0;
@@ -991,7 +988,8 @@ out:
 	return ret;
 }
 
-int blkdev_get(struct block_device *bdev, mode_t mode, unsigned flags)
+static int __blkdev_get(struct block_device *bdev, mode_t mode, unsigned flags,
+			int for_part)
 {
 	/*
 	 * This crockload is due to bad choice of ->open() type.
@@ -1006,9 +1004,13 @@ int blkdev_get(struct block_device *bdev
 	fake_file.f_dentry = &fake_dentry;
 	fake_dentry.d_inode = bdev->bd_inode;
 
-	return do_open(bdev, &fake_file);
+	return do_open(bdev, &fake_file, for_part);
 }
 
+int blkdev_get(struct block_device *bdev, mode_t mode, unsigned flags)
+{
+	return __blkdev_get(bdev, mode, flags, 0);
+}
 EXPORT_SYMBOL(blkdev_get);
 
 static int blkdev_open(struct inode * inode, struct file * filp)
@@ -1026,7 +1028,7 @@ static int blkdev_open(struct inode * in
 
 	bdev = bd_acquire(inode);
 
-	res = do_open(bdev, filp);
+	res = do_open(bdev, filp, 0);
 	if (res)
 		return res;
 
@@ -1040,14 +1042,18 @@ static int blkdev_open(struct inode * in
 	return res;
 }
 
-int blkdev_put(struct block_device *bdev)
+static int __blkdev_put(struct block_device *bdev, int for_part)
 {
 	int ret = 0;
 	struct inode *bd_inode = bdev->bd_inode;
 	struct gendisk *disk = bdev->bd_disk;
+	struct block_device *victim = NULL;
 
 	mutex_lock(&bdev->bd_mutex);
 	lock_kernel();
+	if (for_part)
+		bdev->bd_part_count--;
+
 	if (!--bdev->bd_openers) {
 		sync_blockdev(bdev);
 		kill_bdev(bdev);
@@ -1055,10 +1061,6 @@ int blkdev_put(struct block_device *bdev
 	if (bdev->bd_contains == bdev) {
 		if (disk->fops->release)
 			ret = disk->fops->release(bd_inode, NULL);
-	} else {
-		mutex_lock(&bdev->bd_contains->bd_mutex);
-		bdev->bd_contains->bd_part_count--;
-		mutex_unlock(&bdev->bd_contains->bd_mutex);
 	}
 	if (!bdev->bd_openers) {
 		struct module *owner = disk->fops->owner;
@@ -1072,17 +1074,22 @@ int blkdev_put(struct block_device *bdev
 		}
 		bdev->bd_disk = NULL;
 		bdev->bd_inode->i_data.backing_dev_info = &default_backing_dev_info;
-		if (bdev != bdev->bd_contains) {
-			blkdev_put(bdev->bd_contains);
-		}
+		if (bdev != bdev->bd_contains)
+			victim = bdev->bd_contains;
 		bdev->bd_contains = NULL;
 	}
 	unlock_kernel();
 	mutex_unlock(&bdev->bd_mutex);
 	bdput(bdev);
+	if (victim)
+		__blkdev_put(victim, 1);
 	return ret;
 }
 
+int blkdev_put(struct block_device *bdev)
+{
+	return __blkdev_put(bdev, 0);
+}
 EXPORT_SYMBOL(blkdev_put);
 
 static int blkdev_close(struct inode * inode, struct file * filp)

[PATCH 003 of 4] Use mutex_lock_nested for bd_mutex to avoid lockdep warning.

[NeilBrown]

Now that the nesting in blkdev_{get,put} is simpler, adding
mutex_lock_nested is trivial.

Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./fs/block_dev.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff .prev/fs/block_dev.c ./fs/block_dev.c
--- .prev/fs/block_dev.c	2006-10-11 15:38:31.000000000 +1000
+++ ./fs/block_dev.c	2006-10-11 15:41:55.000000000 +1000
@@ -909,7 +909,7 @@ static int do_open(struct block_device *
 	}
 	owner = disk->fops->owner;
 
-	mutex_lock(&bdev->bd_mutex);
+	mutex_lock_nested(&bdev->bd_mutex, for_part);
 	if (!bdev->bd_openers) {
 		bdev->bd_disk = disk;
 		bdev->bd_contains = bdev;
@@ -1049,7 +1049,7 @@ static int __blkdev_put(struct block_dev
 	struct gendisk *disk = bdev->bd_disk;
 	struct block_device *victim = NULL;
 
-	mutex_lock(&bdev->bd_mutex);
+	mutex_lock_nested(&bdev->bd_mutex, for_part);
 	lock_kernel();
 	if (for_part)
 		bdev->bd_part_count--;

[PATCH 004 of 4] Avoid lockdep warning in md.

[NeilBrown]

md_open takes ->reconfig_mutex which causes lockdep to complain.
This (normally) doesn't have deadlock potential as the possible
conflict is with a reconfig_mutex in a different device.

I say "normally" because if a loop were created in the array->member
hierarchy a deadlock could happen.  However that causes bigger
problems than a deadlock and should be fixed independently.

So we flag the lock in md_open as a nested lock.  This requires
defining mutex_lock_interruptible_nested.

Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Acked-by: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./drivers/md/md.c       |    2 +-
 ./include/linux/mutex.h |    2 ++
 ./kernel/mutex.c        |    9 +++++++++
 3 files changed, 12 insertions(+), 1 deletion(-)

diff .prev/drivers/md/md.c ./drivers/md/md.c
--- .prev/drivers/md/md.c	2006-10-11 15:53:48.000000000 +1000
+++ ./drivers/md/md.c	2006-10-11 15:42:09.000000000 +1000
@@ -4422,7 +4422,7 @@ static int md_open(struct inode *inode, 
 	mddev_t *mddev = inode->i_bdev->bd_disk->private_data;
 	int err;
 
-	if ((err = mddev_lock(mddev)))
+	if ((err = mutex_lock_interruptible_nested(&mddev->reconfig_mutex, 1)))
 		goto out;
 
 	err = 0;

diff .prev/include/linux/mutex.h ./include/linux/mutex.h
--- .prev/include/linux/mutex.h	2006-10-11 15:53:48.000000000 +1000
+++ ./include/linux/mutex.h	2006-10-11 15:43:22.000000000 +1000
@@ -125,8 +125,10 @@ extern int fastcall mutex_lock_interrupt
 
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 extern void mutex_lock_nested(struct mutex *lock, unsigned int subclass);
+extern int mutex_lock_interruptible_nested(struct mutex *lock, unsigned int subclass);
 #else
 # define mutex_lock_nested(lock, subclass) mutex_lock(lock)
+# define mutex_lock_interruptible_nested(lock, subclass) mutex_lock_interruptible(lock)
 #endif
 
 /*

diff .prev/kernel/mutex.c ./kernel/mutex.c
--- .prev/kernel/mutex.c	2006-10-11 15:53:48.000000000 +1000
+++ ./kernel/mutex.c	2006-10-11 15:45:08.000000000 +1000
@@ -206,6 +206,15 @@ mutex_lock_nested(struct mutex *lock, un
 }
 
 EXPORT_SYMBOL_GPL(mutex_lock_nested);
+
+int __sched
+mutex_lock_interruptible_nested(struct mutex *lock, unsigned int subclass)
+{
+	might_sleep();
+	return __mutex_lock_common(lock, TASK_INTERRUPTIBLE, subclass);
+}
+
+EXPORT_SYMBOL_GPL(mutex_lock_interruptible_nested);
 #endif
 
 /*

Re: [PATCH 000 of 4] Introduction

[Peter Zijlstra]

On Wed, 2006-10-11 at 16:09 +1000, NeilBrown wrote:
> Following 4 patches address issues with lockdep, particularly around bd_mutex.
> 
> They are against 2.6.18-mm3 and do *not* apply against -linus as -mm already has
> some changes to the handling of bd_mutex nesting.  2-4 probably apply on top of -linus plus
> -mm/broken-out/remove-the-old-bd_mutex-lockdep-annotation.patch  
> 
> I believe they are probably ok for 2.6.19.
> 
> The core issue is that blkdev_get when called on a partition needs to
> lock (bd_mutex) the partition and the whole device.  lockdep would
> normally see this as a possible deadlock and needs to be told that
> this particular nesting is known to be safe.
> 
> The code to do this is in -linus is rather messy, largely because the
> locking itself is messy.  The bd_mutex for the whole is taken several
> times while bd_mutex for the partition is held, and it is taken at
> both levels of the recursion (blkdev_get calls blkdev_get - only to
> one level).
> 
> As key observation to simplifying the locking is to observer that a
> lot of the locking is there to protect the updating of bd_part_count.
> If those updates are moved, the locking can become simpler.
> 
> The first patch removes the current approach in -mm to handling this
> nesting and explains why it is not ideal.
> This reverts new-bd_mutex-lockdep-annotation.patch
> 
> The second simplifies the locking as explained above.
> 
> The third adds the mutex_lock_nested annotations, which are now trivial.
> 
> The last fixes a tangentially related lockdep problem in md - there is
> a false relationship between bd_mutex and md->reconfig_mutex which
> needs to be clarified.
> 
>  [PATCH 000 of 4] Introduction
>  [PATCH 001 of 4] Remove lock_key approach to managing nested bd_mutex locks.
>  [PATCH 002 of 4] Simplify some aspects of bd_mutex nesting.
>  [PATCH 003 of 4] Use mutex_lock_nested for bd_mutex to avoid  lockdep warning.
>  [PATCH 004 of 4] Avoid lockdep warning in md.

Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>

Re: [PATCH 000 of 4] Introduction

[Ingo Molnar]

* NeilBrown <neilb@suse.de> wrote:

> Following 4 patches address issues with lockdep, particularly around 
> bd_mutex.
> 
> They are against 2.6.18-mm3 and do *not* apply against -linus as -mm 
> already has some changes to the handling of bd_mutex nesting.  2-4 
> probably apply on top of -linus plus 
> -mm/broken-out/remove-the-old-bd_mutex-lockdep-annotation.patch
> 
> I believe they are probably ok for 2.6.19.

agreed.

they look quite nice in that they also simplify the underlying locking. 
(instead of just trying to clone whatever locking yuckiness there is, 
into the lockdep space.)

> The last fixes a tangentially related lockdep problem in md - there is 
> a false relationship between bd_mutex and md->reconfig_mutex which 
> needs to be clarified.

> md_open takes ->reconfig_mutex which causes lockdep to complain. This 
> (normally) doesn't have deadlock potential as the possible conflict is 
> with a reconfig_mutex in a different device.
>
> I say "normally" because if a loop were created in the array->member 
> hierarchy a deadlock could happen.  However that causes bigger 
> problems than a deadlock and should be fixed independently.

ok to me. Sidenote: shouldnt we algorithmically forbid that "loop" 
scenario from occuring, as that possibility is what causes lockdep to 
complain about the worst-case scenario?

	Ingo

Re: [PATCH 000 of 4] Introduction

[Neil Brown]

On Wednesday October 11, mingo@elte.hu wrote:
> >
> > I say "normally" because if a loop were created in the array->member 
> > hierarchy a deadlock could happen.  However that causes bigger 
> > problems than a deadlock and should be fixed independently.
> 
> ok to me. Sidenote: shouldnt we algorithmically forbid that "loop" 
> scenario from occuring, as that possibility is what causes lockdep to 
> complain about the worst-case scenario?

Yes we should.  Possibly we could use the linkage information set up
by bd_claim_by_kobject.  However I'm afraid that the locking required
to check that linkage safely will look very dead-lock prone to
lockdep.  I suspect that can be worked-around though.

NeilBrown