Re: [PATCH 2/2] new bd_mutex lockdep annotation
From: Peter Zijlstra
Date: Thu Sep 14 2006 - 04:48:46 EST
On Thu, 2006-09-14 at 17:11 +1000, Neil Brown wrote:
> On Wednesday September 13, a.p.zijlstra@xxxxxxxxx wrote:
> > Use the gendisk partition number to set a lock class.
>
> Yes, this does look a lot nicer, thanks.
>
> Two observations.
> 1/ I was confused that you added a call to mutex_init. One would
> normally expect to only have one of these for any given mutex, so
> adding one was a surprise.
> I now realise that the purpose of this call is not exactly to init
> the mutex, but to init the lockdep class in case this inode was
> previously used for a partition but is now being used for a whole
> device. This makes sense, but renders the mutex_init in
> init_once pointless. Maybe that should be removed?
Yes, that would be quite redundant now, new patch attached.
> 2/ You are introducing a new call to get_gendisk.
> This bothers me for two reasons. Both relate to a comparison
> with the call to get_gendisk in block_dev.c:do_open.
> a/ That call is protected by lock_kernel. Your call is not.
> b/ That call is followed by a test for '!disk' implying that it
> can return NULL. Yours is not - at least not obviously
> (put_disk does have the check).
a) kobj_lookup() vs kobj_(un)map() use the domain lock.
Not all calls to blk_register_region() were under lock_kernel() afaicf.
So I don't think this is needed, but I'll gladly take advise otherwise,
I'm not well versed with the kobj stuff.
b) from quick inspection yesterday I reached two (false) conclusions
- &part would not be changed when !disk
- disk would have to exists at the time we call bdget()
Now I can't seem to validate either of them. Added disk to the if
statement just to be safe.
> I'm not sure if these are actually problems, but the do bother me.
>
> Thinking through the possibly reasons for the lock_kernel, I wonder
> it the current device number mapping scheme actually allows you
> to determine if something is partitioned or not in a static sense.
> Maybe that is only guaranteed to be stable while the device is
> open...
Hmm, yes I think I see what you mean...
> I wonder if Al Viro could put my mind at rest .... Al - do you have
> a moment to look at this? Thanks.
+1
---
Use the gendisk partition number to set a lock class.
Signed-off-by: Peter Zijlstra <a.p.zijlstra@xxxxxxxxx>
Acked-by: Arjan van de Ven <arjan@xxxxxxxxxxxxxxx>
Cc: Neil Brown <neilb@xxxxxxxxxxxxxxx>
Cc: Ingo Molnar <mingo@xxxxxxx>
Cc: Andrew Morton <akpm@xxxxxxxx>
Cc: Jason Baron <jbaron@xxxxxxxxxx>
---
fs/block_dev.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
Index: linux-2.6-mm/fs/block_dev.c
===================================================================
--- linux-2.6-mm.orig/fs/block_dev.c
+++ linux-2.6-mm/fs/block_dev.c
@@ -264,7 +264,6 @@ static void init_once(void * foo, kmem_c
SLAB_CTOR_CONSTRUCTOR)
{
memset(bdev, 0, sizeof(*bdev));
- mutex_init(&bdev->bd_mutex);
mutex_init(&bdev->bd_mount_mutex);
INIT_LIST_HEAD(&bdev->bd_inodes);
INIT_LIST_HEAD(&bdev->bd_list);
@@ -357,10 +356,14 @@ 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);
@@ -386,6 +389,11 @@ 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 (disk && part)
+ lockdep_set_class(&bdev->bd_mutex, &bdev_part_lock_key);
+ put_disk(disk);
}
return bdev;
}
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/