[linux-lvm] [RFC] Reverting "bd_mount_mutex" to "bd_mount_sem"

[linux-lvm] [RFC] Reverting "bd_mount_mutex" to "bd_mount_sem"

[Srinivasa Ds]

[-- Attachment #1: Type: text/plain, Size: 2167 bytes --]

Hi all
  When I was executing "dmsetup resume <device-name>" command,I got the 
error shown below. Which basically tells that, "bd_mount_mutex"  in 
thaw_bdev() is not locked by "dmsetup resume" command and hence it is 
not allowing it to unlock also.
=========================================================
Badness in debug_mutex_unlock at kernel/mutex-debug.c:80

Call Trace:
[C0000000634DB260] [C000000000010948] .show_stack+0x68/0x1b0 (unreliable)
[C0000000634DB300] [C0000000003376C4] .program_check_exception+0x1cc/0x5b0
[C0000000634DB3D0] [C0000000000047EC] program_check_common+0xec/0x100
--- Exception: 700 at .debug_mutex_unlock+0x3c/0xc4
    LR = .debug_mutex_unlock+0x30/0xc4
[C0000000634DB6C0] [C0000000634DB750] 0xc0000000634db750 (unreliable)
[C0000000634DB740] [C000000000335950] .__mutex_unlock_slowpath+0xd8/0x144
[C0000000634DB7E0] [C0000000000E2370] .thaw_bdev+0x9c/0xb8
[C0000000634DB870] [D000000000480830] .unlock_fs+0x34/0x70 [dm_mod]
[C0000000634DB900] [D000000000481720] .dm_resume+0x110/0x1ac [dm_mod]
[C0000000634DB9A0] [D000000000485C54] .dev_suspend+0x1b0/0x204 [dm_mod]
[C0000000634DBA40] [D000000000486728] .ctl_ioctl+0x29c/0x318 [dm_mod]
[C0000000634DBC30] [C0000000000F8310] .do_ioctl+0xbc/0xf0
[C0000000634DBCD0] [C0000000000F879C] .vfs_ioctl+0x458/0x498
[C0000000634DBD80] [C0000000000F8874] .sys_ioctl+0x98/0xe0
[C0000000634DBE30] [C00000000000871C] syscall_exit+0x0/0x40
======================================================================

   On debugging I found out that,"dmsetup suspend <device name>" calls 
"freeze_bdev()",which locks "bd_mount_mutex" to make sure that no new 
mounts happen on bdev until thaw_bdev() is called.
   This "thaw_bdev()" is getting called when we resume the device through
"dmsetup resume <device-name>".
   Hence we have 2 processes,one of which locks "bd_mount_mutex"(dmsetup
suspend) and Another(dmsetup resume) unlocks it.

   Since this is not allowed in mutex,I reverted back to 
bd_mount_sem(semaphore),It worked for me.

  So  need your comments for changing "bd_mount_mutex" to "bd_mount_sem".
  
  This is the patch,which I have used.

Thanks
 Srinivasa Ds
 LTC-IBM
 Bangalore



[-- Attachment #2: mutex_to_sem.fix --]
[-- Type: text/plain, Size: 2388 bytes --]

diff -Naurp linux-2.6.18-rc6-orig/fs/block_dev.c linux-2.6.18-rc6-mod/fs/block_dev.c
--- linux-2.6.18-rc6-orig/fs/block_dev.c	2006-09-27 04:47:25.000000000 -0700
+++ linux-2.6.18-rc6-mod/fs/block_dev.c	2006-09-27 04:53:29.000000000 -0700
@@ -261,7 +261,7 @@ static void init_once(void * foo, kmem_c
 	{
 		memset(bdev, 0, sizeof(*bdev));
 		mutex_init(&bdev->bd_mutex);
-		mutex_init(&bdev->bd_mount_mutex);
+		sema_init(&bdev->bd_mount_sem, 1);
 		INIT_LIST_HEAD(&bdev->bd_inodes);
 		INIT_LIST_HEAD(&bdev->bd_list);
 #ifdef CONFIG_SYSFS
diff -Naurp linux-2.6.18-rc6-orig/fs/buffer.c linux-2.6.18-rc6-mod/fs/buffer.c
--- linux-2.6.18-rc6-orig/fs/buffer.c	2006-09-27 04:46:14.000000000 -0700
+++ linux-2.6.18-rc6-mod/fs/buffer.c	2006-09-27 04:49:55.000000000 -0700
@@ -213,7 +213,7 @@ struct super_block *freeze_bdev(struct b
 {
 	struct super_block *sb;
 
-	mutex_lock(&bdev->bd_mount_mutex);
+	down(&bdev->bd_mount_sem);
 	sb = get_super(bdev);
 	if (sb && !(sb->s_flags & MS_RDONLY)) {
 		sb->s_frozen = SB_FREEZE_WRITE;
@@ -255,7 +255,7 @@ void thaw_bdev(struct block_device *bdev
 		drop_super(sb);
 	}
 
-	mutex_unlock(&bdev->bd_mount_mutex);
+	up(&bdev->bd_mount_sem);
 }
 EXPORT_SYMBOL(thaw_bdev);
 
diff -Naurp linux-2.6.18-rc6-orig/fs/super.c linux-2.6.18-rc6-mod/fs/super.c
--- linux-2.6.18-rc6-orig/fs/super.c	2006-09-27 04:46:51.000000000 -0700
+++ linux-2.6.18-rc6-mod/fs/super.c	2006-09-27 04:50:56.000000000 -0700
@@ -700,9 +700,9 @@ int get_sb_bdev(struct file_system_type 
 	 * will protect the lockfs code from trying to start a snapshot
 	 * while we are mounting
 	 */
-	mutex_lock(&bdev->bd_mount_mutex);
+	down(&bdev->bd_mount_sem);
 	s = sget(fs_type, test_bdev_super, set_bdev_super, bdev);
-	mutex_unlock(&bdev->bd_mount_mutex);
+	up(&bdev->bd_mount_sem);
 	if (IS_ERR(s))
 		goto error_s;
 
diff -Naurp linux-2.6.18-rc6-orig/include/linux/fs.h linux-2.6.18-rc6-mod/include/linux/fs.h
--- linux-2.6.18-rc6-orig/include/linux/fs.h	2006-09-27 04:45:31.000000000 -0700
+++ linux-2.6.18-rc6-mod/include/linux/fs.h	2006-09-27 04:52:30.000000000 -0700
@@ -414,7 +414,7 @@ struct block_device {
 	struct inode *		bd_inode;	/* will die */
 	int			bd_openers;
 	struct mutex		bd_mutex;	/* open/close mutex */
-	struct mutex		bd_mount_mutex;	/* mount mutex */
+	struct semaphore        bd_mount_sem;
 	struct list_head	bd_inodes;
 	void *			bd_holder;
 	int			bd_holders;

[linux-lvm] Re: [RFC] Reverting "bd_mount_mutex" to "bd_mount_sem"

[Ingo Molnar]

* Srinivasa Ds <srinivasa@in.ibm.com> wrote:

>   On debugging I found out that,"dmsetup suspend <device name>" calls 
> "freeze_bdev()",which locks "bd_mount_mutex" to make sure that no new 
> mounts happen on bdev until thaw_bdev() is called.
>   This "thaw_bdev()" is getting called when we resume the device 
> through "dmsetup resume <device-name>".
>   Hence we have 2 processes,one of which locks 
> "bd_mount_mutex"(dmsetup suspend) and Another(dmsetup resume) unlocks 
> it.

hm, to me this seems quite a fragile construct - even if the 
mutex-debugging warning is worked around by reverting to a semaphore.

	Ingo

[linux-lvm] Re: [RFC] Reverting "bd_mount_mutex" to "bd_mount_sem"

[Eric Sandeen]

Ingo Molnar wrote:
> * Srinivasa Ds <srinivasa@in.ibm.com> wrote:
> 
>>   On debugging I found out that,"dmsetup suspend <device name>" calls 
>> "freeze_bdev()",which locks "bd_mount_mutex" to make sure that no new 
>> mounts happen on bdev until thaw_bdev() is called.
>>   This "thaw_bdev()" is getting called when we resume the device 
>> through "dmsetup resume <device-name>".
>>   Hence we have 2 processes,one of which locks 
>> "bd_mount_mutex"(dmsetup suspend) and Another(dmsetup resume) unlocks 
>> it.
> 
> hm, to me this seems quite a fragile construct - even if the 
> mutex-debugging warning is worked around by reverting to a semaphore.
> 
> 	Ingo

Ingo, what do you feel is fragile about this?  It seems like this is a
reasonable way to go, except that maybe a down_trylock would be good if
a 2nd process tries to freeze while it's already frozen...

Thanks,

-Eric

[linux-lvm] Re: [RFC] Reverting "bd_mount_mutex" to "bd_mount_sem"

[Srinivasa Ds]

[-- Attachment #1: Type: text/plain, Size: 1086 bytes --]

Eric Sandeen wrote:
> Ingo Molnar wrote:
>   
>> * Srinivasa Ds <srinivasa@in.ibm.com> wrote:
>>
>>     
>>>   On debugging I found out that,"dmsetup suspend <device name>" calls 
>>> "freeze_bdev()",which locks "bd_mount_mutex" to make sure that no new 
>>> mounts happen on bdev until thaw_bdev() is called.
>>>   This "thaw_bdev()" is getting called when we resume the device 
>>> through "dmsetup resume <device-name>".
>>>   Hence we have 2 processes,one of which locks 
>>> "bd_mount_mutex"(dmsetup suspend) and Another(dmsetup resume) unlocks 
>>> it.
>>>       
>> hm, to me this seems quite a fragile construct - even if the 
>> mutex-debugging warning is worked around by reverting to a semaphore.
>>
>> 	Ingo
>>     
>
> Ingo, what do you feel is fragile about this?  It seems like this is a
> reasonable way to go, except that maybe a down_trylock would be good if
> a 2nd process tries to freeze while it's already frozen...
>
> Thanks,
>
> -Eric
>   
Ingo, As per the discussion resending the patch with down_trylock.

Signed-off-by: Srinivasa DS <srinivasa@in.ibm.com>



[-- Attachment #2: dmsetup.fix --]
[-- Type: text/plain, Size: 2388 bytes --]

---
 fs/block_dev.c     |    2 +-
 fs/buffer.c        |    6 ++++--
 fs/super.c         |    4 ++--
 include/linux/fs.h |    2 +-
 4 files changed, 8 insertions(+), 6 deletions(-)

Index: linux-2.6.19-rc1/fs/block_dev.c
===================================================================
--- linux-2.6.19-rc1.orig/fs/block_dev.c
+++ linux-2.6.19-rc1/fs/block_dev.c
@@ -263,7 +263,7 @@ static void init_once(void * foo, kmem_c
 	{
 		memset(bdev, 0, sizeof(*bdev));
 		mutex_init(&bdev->bd_mutex);
-		mutex_init(&bdev->bd_mount_mutex);
+		sema_init(&bdev->bd_mount_sem, 1);
 		INIT_LIST_HEAD(&bdev->bd_inodes);
 		INIT_LIST_HEAD(&bdev->bd_list);
 #ifdef CONFIG_SYSFS
Index: linux-2.6.19-rc1/fs/buffer.c
===================================================================
--- linux-2.6.19-rc1.orig/fs/buffer.c
+++ linux-2.6.19-rc1/fs/buffer.c
@@ -188,7 +188,9 @@ struct super_block *freeze_bdev(struct b
 {
 	struct super_block *sb;
 
-	mutex_lock(&bdev->bd_mount_mutex);
+	if (down_trylock(&bdev->bd_mount_sem))
+		return -EBUSY;
+
 	sb = get_super(bdev);
 	if (sb && !(sb->s_flags & MS_RDONLY)) {
 		sb->s_frozen = SB_FREEZE_WRITE;
@@ -230,7 +232,7 @@ void thaw_bdev(struct block_device *bdev
 		drop_super(sb);
 	}
 
-	mutex_unlock(&bdev->bd_mount_mutex);
+	up(&bdev->bd_mount_sem);
 }
 EXPORT_SYMBOL(thaw_bdev);
 
Index: linux-2.6.19-rc1/fs/super.c
===================================================================
--- linux-2.6.19-rc1.orig/fs/super.c
+++ linux-2.6.19-rc1/fs/super.c
@@ -735,9 +735,9 @@ int get_sb_bdev(struct file_system_type 
 	 * will protect the lockfs code from trying to start a snapshot
 	 * while we are mounting
 	 */
-	mutex_lock(&bdev->bd_mount_mutex);
+	down(&bdev->bd_mount_sem);
 	s = sget(fs_type, test_bdev_super, set_bdev_super, bdev);
-	mutex_unlock(&bdev->bd_mount_mutex);
+	up(&bdev->bd_mount_sem);
 	if (IS_ERR(s))
 		goto error_s;
 
Index: linux-2.6.19-rc1/include/linux/fs.h
===================================================================
--- linux-2.6.19-rc1.orig/include/linux/fs.h
+++ linux-2.6.19-rc1/include/linux/fs.h
@@ -456,7 +456,7 @@ struct block_device {
 	struct inode *		bd_inode;	/* will die */
 	int			bd_openers;
 	struct mutex		bd_mutex;	/* open/close mutex */
-	struct mutex		bd_mount_mutex;	/* mount mutex */
+	struct semaphore        bd_mount_sem;
 	struct list_head	bd_inodes;
 	void *			bd_holder;
 	int			bd_holders;

[linux-lvm] Re: [RFC] Reverting "bd_mount_mutex" to "bd_mount_sem"

[Arjan van de Ven]

On Tue, 2006-10-10 at 20:34 +0530, Srinivasa Ds wrote:
> Eric Sandeen wrote:
> > Ingo Molnar wrote:
> >   
> >> * Srinivasa Ds <srinivasa@in.ibm.com> wrote:
> >>
> >>     
> >>>   On debugging I found out that,"dmsetup suspend <device name>" calls 
> >>> "freeze_bdev()",which locks "bd_mount_mutex" to make sure that no new 
> >>> mounts happen on bdev until thaw_bdev() is called.
> >>>   This "thaw_bdev()" is getting called when we resume the device 
> >>> through "dmsetup resume <device-name>".
> >>>   Hence we have 2 processes,one of which locks 
> >>> "bd_mount_mutex"(dmsetup suspend) and Another(dmsetup resume) unlocks 
> >>> it.
> >>>       
> >> hm, to me this seems quite a fragile construct - even if the 
> >> mutex-debugging warning is worked around by reverting to a semaphore.
> >>
> >> 	Ingo
> >>     
> >
> > Ingo, what do you feel is fragile about this?  It seems like this is a
> > reasonable way to go, except that maybe a down_trylock would be good if
> > a 2nd process tries to freeze while it's already frozen...
> >
> > Thanks,
> >
> > -Eric
> >   
> Ingo, As per the discussion resending the patch with down_trylock.

Hi,

I still think that effectively exporting this semaphore to userspace is
a big design mistake; but at least it can't be a mutex for this reason
so the patch is sane in that regard...

Greetings,
   Arjan van de Ven

RE: [linux-lvm] Re: [RFC] Reverting "bd_mount_mutex" to "bd_mount_sem"

[Roger Lucas]

Hi,

Checking through the code, I think that the semaphore -> mutex change was introduced in the original 2.6.17 kernel release.  If it
has problems, then it will affect all 2.6.17 and 2.6.18 kernels as the mutex code is still in the 2.6.18.1 release.

Can someone explain under what conditions this bug will occur and what its impact is?  We are currently using the 2.6.16.20 kernel
(which seems OK) but were keen to move to 2.6.18 as it has the new SATA hotplug ability.  We use LVM and dmapper heavily, however,
so don't want to switch if there are issues with it.

Alternatively, would locally updating the 2.6.18.1 kernel with Srinivasa's patch be sensible to restore the 2.6.16 style semaphore
code (which does seem to be stable)?

Thanks,

Roger

> -----Original Message-----
> From: linux-lvm-bounces@redhat.com [mailto:linux-lvm-bounces@redhat.com] On Behalf Of Arjan van de
> Ven
> Sent: 10 October 2006 16:19
> To: Srinivasa Ds
> Cc: Eric Sandeen; linux-kernel@vger.kernel.org; dm-devel@redhat.com; linux-lvm@redhat.com; Ingo
> Molnar; agk@redhat.com
> Subject: [linux-lvm] Re: [RFC] Reverting "bd_mount_mutex" to "bd_mount_sem"
> 
> On Tue, 2006-10-10 at 20:34 +0530, Srinivasa Ds wrote:
> > Eric Sandeen wrote:
> > > Ingo Molnar wrote:
> > >
> > >> * Srinivasa Ds <srinivasa@in.ibm.com> wrote:
> > >>
> > >>
> > >>>   On debugging I found out that,"dmsetup suspend <device name>" calls
> > >>> "freeze_bdev()",which locks "bd_mount_mutex" to make sure that no new
> > >>> mounts happen on bdev until thaw_bdev() is called.
> > >>>   This "thaw_bdev()" is getting called when we resume the device
> > >>> through "dmsetup resume <device-name>".
> > >>>   Hence we have 2 processes,one of which locks
> > >>> "bd_mount_mutex"(dmsetup suspend) and Another(dmsetup resume) unlocks
> > >>> it.
> > >>>
> > >> hm, to me this seems quite a fragile construct - even if the
> > >> mutex-debugging warning is worked around by reverting to a semaphore.
> > >>
> > >> 	Ingo
> > >>
> > >
> > > Ingo, what do you feel is fragile about this?  It seems like this is a
> > > reasonable way to go, except that maybe a down_trylock would be good if
> > > a 2nd process tries to freeze while it's already frozen...
> > >
> > > Thanks,
> > >
> > > -Eric
> > >
> > Ingo, As per the discussion resending the patch with down_trylock.
> 
> Hi,
> 
> I still think that effectively exporting this semaphore to userspace is
> a big design mistake; but at least it can't be a mutex for this reason
> so the patch is sane in that regard...
> 
> Greetings,
>    Arjan van de Ven
> 
> _______________________________________________
> linux-lvm mailing list
> linux-lvm@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-lvm
> read the LVM HOW-TO at http://tldp.org/HOWTO/LVM-HOWTO/

[linux-lvm] [RFC] Reverting "bd_mount_mutex" to "bd_mount_sem"

[Srinivasa Ds]

Hi all
  When I was executing "dmsetup resume <device-name>" command,I got the 
error shown below. Which basically tells that, "bd_mount_mutex"  in 
thaw_bdev() is not locked by "dmsetup resume" command and hence it is 
not allowing it to unlock also.
=========================================================
Badness in debug_mutex_unlock at kernel/mutex-debug.c:80

Call Trace:
[C0000000634DB260] [C000000000010948] .show_stack+0x68/0x1b0 (unreliable)
[C0000000634DB300] [C0000000003376C4] .program_check_exception+0x1cc/0x5b0
[C0000000634DB3D0] [C0000000000047EC] program_check_common+0xec/0x100
--- Exception: 700 at .debug_mutex_unlock+0x3c/0xc4
    LR = .debug_mutex_unlock+0x30/0xc4
[C0000000634DB6C0] [C0000000634DB750] 0xc0000000634db750 (unreliable)
[C0000000634DB740] [C000000000335950] .__mutex_unlock_slowpath+0xd8/0x144
[C0000000634DB7E0] [C0000000000E2370] .thaw_bdev+0x9c/0xb8
[C0000000634DB870] [D000000000480830] .unlock_fs+0x34/0x70 [dm_mod]
[C0000000634DB900] [D000000000481720] .dm_resume+0x110/0x1ac [dm_mod]
[C0000000634DB9A0] [D000000000485C54] .dev_suspend+0x1b0/0x204 [dm_mod]
[C0000000634DBA40] [D000000000486728] .ctl_ioctl+0x29c/0x318 [dm_mod]
[C0000000634DBC30] [C0000000000F8310] .do_ioctl+0xbc/0xf0
[C0000000634DBCD0] [C0000000000F879C] .vfs_ioctl+0x458/0x498
[C0000000634DBD80] [C0000000000F8874] .sys_ioctl+0x98/0xe0
[C0000000634DBE30] [C00000000000871C] syscall_exit+0x0/0x40
======================================================================

   On debugging I found out that,"dmsetup suspend <device name>" calls 
"freeze_bdev()",which locks "bd_mount_mutex" to make sure that no new 
mounts happen on bdev until thaw_bdev() is called.
   This "thaw_bdev()" is getting called when we resume the device through
"dmsetup resume <device-name>".
   Hence we have 2 processes,one of which locks "bd_mount_mutex"(dmsetup
suspend) and Another(dmsetup resume) unlocks it.

   Since this is not allowed in mutex,I reverted back to 
bd_mount_sem(semaphore),It worked for me.

I posted the below patch to lkml also. So Please let me know your 
comments on this.
===================================================================

diff -Naurp linux-2.6.18-rc6-orig/fs/block_dev.c 
linux-2.6.18-rc6-mod/fs/block_dev.c
--- linux-2.6.18-rc6-orig/fs/block_dev.c	2006-09-27 04:47:25.000000000 -0700
+++ linux-2.6.18-rc6-mod/fs/block_dev.c	2006-09-27 04:53:29.000000000 -0700
@@ -261,7 +261,7 @@ static void init_once(void * foo, kmem_c
  	{
  		memset(bdev, 0, sizeof(*bdev));
  		mutex_init(&bdev->bd_mutex);
-		mutex_init(&bdev->bd_mount_mutex);
+		sema_init(&bdev->bd_mount_sem, 1);
  		INIT_LIST_HEAD(&bdev->bd_inodes);
  		INIT_LIST_HEAD(&bdev->bd_list);
  #ifdef CONFIG_SYSFS
diff -Naurp linux-2.6.18-rc6-orig/fs/buffer.c 
linux-2.6.18-rc6-mod/fs/buffer.c
--- linux-2.6.18-rc6-orig/fs/buffer.c	2006-09-27 04:46:14.000000000 -0700
+++ linux-2.6.18-rc6-mod/fs/buffer.c	2006-09-27 04:49:55.000000000 -0700
@@ -213,7 +213,7 @@ struct super_block *freeze_bdev(struct b
  {
  	struct super_block *sb;

-	mutex_lock(&bdev->bd_mount_mutex);
+	down(&bdev->bd_mount_sem);
  	sb = get_super(bdev);
  	if (sb && !(sb->s_flags & MS_RDONLY)) {
  		sb->s_frozen = SB_FREEZE_WRITE;
@@ -255,7 +255,7 @@ void thaw_bdev(struct block_device *bdev
  		drop_super(sb);
  	}

-	mutex_unlock(&bdev->bd_mount_mutex);
+	up(&bdev->bd_mount_sem);
  }
  EXPORT_SYMBOL(thaw_bdev);

diff -Naurp linux-2.6.18-rc6-orig/fs/super.c linux-2.6.18-rc6-mod/fs/super.c
--- linux-2.6.18-rc6-orig/fs/super.c	2006-09-27 04:46:51.000000000 -0700
+++ linux-2.6.18-rc6-mod/fs/super.c	2006-09-27 04:50:56.000000000 -0700
@@ -700,9 +700,9 @@ int get_sb_bdev(struct file_system_type
  	 * will protect the lockfs code from trying to start a snapshot
  	 * while we are mounting
  	 */
-	mutex_lock(&bdev->bd_mount_mutex);
+	down(&bdev->bd_mount_sem);
  	s = sget(fs_type, test_bdev_super, set_bdev_super, bdev);
-	mutex_unlock(&bdev->bd_mount_mutex);
+	up(&bdev->bd_mount_sem);
  	if (IS_ERR(s))
  		goto error_s;

diff -Naurp linux-2.6.18-rc6-orig/include/linux/fs.h 
linux-2.6.18-rc6-mod/include/linux/fs.h
--- linux-2.6.18-rc6-orig/include/linux/fs.h	2006-09-27 
04:45:31.000000000 -0700
+++ linux-2.6.18-rc6-mod/include/linux/fs.h	2006-09-27 
04:52:30.000000000 -0700
@@ -414,7 +414,7 @@ struct block_device {
  	struct inode *		bd_inode;	/* will die */
  	int			bd_openers;
  	struct mutex		bd_mutex;	/* open/close mutex */
-	struct mutex		bd_mount_mutex;	/* mount mutex */
+	struct semaphore        bd_mount_sem;
  	struct list_head	bd_inodes;
  	void *			bd_holder;
  	int			bd_holders;
==============================================================================

Thanks
  Srinivasa DS
  Linux Technology Centre