drop mutex in __set_size

Commit Message

Mikulas Patocka July 27, 2009, 11:44 p.m. UTC

On Thu, 23 Jul 2009, Jun'ichi Nomura wrote:

> Hi Alasdair, Mikulas,
> 
> I found 2.6.31-rc includes the following commit:
>   commit 32a926da5a16c01a8213331e5764472ce2f14a8d
>   Author: Mikulas Patocka <[e-mail]>
>   Date:   Mon Jun 22 10:12:17 2009 +0100
> 
> which will introduce a deadlock problem described here:
> https://www.redhat.com/archives/dm-devel/2009-May/msg00097.html
> (or see below)
> 
> Was the problem fixed/worked around somehow?

Hi.

I'd submit this. What do others think about it?

Mikulas

---

Drop the mutex.

It doesn't make sense to lock it for a single assignment, this code can't
be executed concurrently. The size should be read with i_size_read which
is automatically protected against concurrent i_size_write.

This locking can lead to a deadlock, if someone holds i_mutex while
the device is being suspended, described by Jun'ichi Nomura at
https://www.redhat.com/archives/dm-devel/2009-May/msg00097.html

Signed-off-by: Mikulas Patocka <[e-mail]>

---
 drivers/md/dm.c |    2 --
 1 file changed, 2 deletions(-)


--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Comments

Junichi Nomura July 29, 2009, 12:14 a.m. UTC | #1

Hi Mikulas,

Mikulas Patocka wrote:
> Drop the mutex.
> 
> It doesn't make sense to lock it for a single assignment, this code can't
> be executed concurrently. The size should be read with i_size_read which
> is automatically protected against concurrent i_size_write.

But it seems there are codes which depend on i_mutex for
protected access to i_size: e.g. block_write_begin().

> This locking can lead to a deadlock, if someone holds i_mutex while
> the device is being suspended, described by Jun'ichi Nomura at
> https://www.redhat.com/archives/dm-devel/2009-May/msg00097.html

And while my description only mentioned noflush suspend,
with 2.6.31-rc, the deadlock can occur under normal suspend
because the new barrier code may push bios to deferred queue.

> Signed-off-by: Mikulas Patocka <[e-mail]>
> 
> ---
>  drivers/md/dm.c |    2 --
>  1 file changed, 2 deletions(-)
> 
> Index: linux-2.6.31-rc3-devel/drivers/md/dm.c
> ===================================================================
> --- linux-2.6.31-rc3-devel.orig/drivers/md/dm.c	2009-07-28 01:20:17.000000000 +0200
> +++ linux-2.6.31-rc3-devel/drivers/md/dm.c	2009-07-28 01:20:27.000000000 +0200
> @@ -1901,9 +1901,7 @@ static void __set_size(struct mapped_dev
>  {
>  	set_capacity(md->disk, size);
>  
> -	mutex_lock(&md->bdev->bd_inode->i_mutex);
>  	i_size_write(md->bdev->bd_inode, (loff_t)size << SECTOR_SHIFT);
> -	mutex_unlock(&md->bdev->bd_inode->i_mutex);
>  }
>  
>  static int __bind(struct mapped_device *md, struct dm_table *t,

Mikulas Patocka July 30, 2009, 3:17 p.m. UTC | #2

On Wed, 29 Jul 2009, Jun'ichi Nomura wrote:

> Hi Mikulas,
> 
> Mikulas Patocka wrote:
> > Drop the mutex.
> > 
> > It doesn't make sense to lock it for a single assignment, this code can't
> > be executed concurrently. The size should be read with i_size_read which
> > is automatically protected against concurrent i_size_write.
> 
> But it seems there are codes which depend on i_mutex for
> protected access to i_size: e.g. block_write_begin().

You are right, but I don't know what to do with it. Catch such uses and 
convert them to i_size_read?

> And while my description only mentioned noflush suspend,
> with 2.6.31-rc, the deadlock can occur under normal suspend
> because the new barrier code may push bios to deferred queue.

It can deadlock with any code that takes i_mutex and submits or waits for 
i/os, regardless of barriers.

Mikulas

--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Junichi Nomura July 30, 2009, 11:49 p.m. UTC | #3

Hi Mikulas,

Mikulas Patocka wrote:
> On Wed, 29 Jul 2009, Jun'ichi Nomura wrote:
>> Mikulas Patocka wrote:
>>> Drop the mutex.
>>>
>>> It doesn't make sense to lock it for a single assignment, this code can't
>>> be executed concurrently. The size should be read with i_size_read which
>>> is automatically protected against concurrent i_size_write.
>> But it seems there are codes which depend on i_mutex for
>> protected access to i_size: e.g. block_write_begin().
> 
> You are right, but I don't know what to do with it. Catch such uses and 
> convert them to i_size_read?

Or move the i_size_write outside of dm suspended state?
Not deeply examined, but I don't think it doesn't have
to be in the suspended code path.

>> And while my description only mentioned noflush suspend,
>> with 2.6.31-rc, the deadlock can occur under normal suspend
>> because the new barrier code may push bios to deferred queue.
> 
> It can deadlock with any code that takes i_mutex and submits or waits for 
> i/os, regardless of barriers.

Yes, you are right.

I attached a reproducer script for easy testing.

Patch

Index: linux-2.6.31-rc3-devel/drivers/md/dm.c
===================================================================
--- linux-2.6.31-rc3-devel.orig/drivers/md/dm.c	2009-07-28 01:20:17.000000000 +0200
+++ linux-2.6.31-rc3-devel/drivers/md/dm.c	2009-07-28 01:20:27.000000000 +0200
@@ -1901,9 +1901,7 @@  static void __set_size(struct mapped_dev
 {
 	set_capacity(md->disk, size);
 
-	mutex_lock(&md->bdev->bd_inode->i_mutex);
 	i_size_write(md->bdev->bd_inode, (loff_t)size << SECTOR_SHIFT);
-	mutex_unlock(&md->bdev->bd_inode->i_mutex);
 }
 
 static int __bind(struct mapped_device *md, struct dm_table *t,