Avoid taking md->bdev->bd_inode->i_mutex to update the DM block device's
size. Using md->bdev->bd_mutex eliminates the potential for deadlock if
an fsync is racing with a device resize.

revalidate_disk() was avoided because it would flush_disk() while the DM
device is suspended.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
Cc: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
---
drivers/md/dm.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

Jun'ichi, was the following your implict Acked-by? Care to make it
explicit?
"Anyway, I think your bd_mutex patch should be fine for now and is
better than the current code with i_mutex, which has a real deadlock
issue."

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 7cb1352..248794a 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1996,9 +1996,9 @@ static void __set_size(struct mapped_device *md, sector_t size)
{
set_capacity(md->disk, size);

- mutex_lock(&md->bdev->bd_inode->i_mutex);
+ mutex_lock(&md->bdev->bd_mutex);
i_size_write(md->bdev->bd_inode, (loff_t)size << SECTOR_SHIFT);
- mutex_unlock(&md->bdev->bd_inode->i_mutex);
+ mutex_unlock(&md->bdev->bd_mutex);
}

/*

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Hi Mike,

(10/30/10 06:50), Mike Snitzer wrote:
> Avoid taking md->bdev->bd_inode->i_mutex to update the DM block device's
> size. Using md->bdev->bd_mutex eliminates the potential for deadlock if
> an fsync is racing with a device resize.
>
> revalidate_disk() was avoided because it would flush_disk() while the DM
> device is suspended.
>
> Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> Cc: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
> ---
> drivers/md/dm.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> Jun'ichi, was the following your implict Acked-by? Care to make it
> explicit?
> "Anyway, I think your bd_mutex patch should be fine for now and is
> better than the current code with i_mutex, which has a real deadlock
> issue."

No, it was not an ACK.
(This is not multipath. So I think you don't need my ack.)

I'm reluctant to ack this because, as I wrote, it's prone to
cause deadlock in future.
But I couldn't find a real problem with the patch,
so I'm not NACK-ing either.

--
Jun'ichi Nomura, NEC Corporation

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

On Mon, Nov 01 2010 at 3:19am -0400,
Jun'ichi Nomura <j-nomura@ce.jp.nec.com> wrote:

> Hi Mike,
>
> (10/30/10 06:50), Mike Snitzer wrote:
> > Avoid taking md->bdev->bd_inode->i_mutex to update the DM block device's
> > size. Using md->bdev->bd_mutex eliminates the potential for deadlock if
> > an fsync is racing with a device resize.
> >
> > revalidate_disk() was avoided because it would flush_disk() while the DM
> > device is suspended.
> >
> > Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> > Cc: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
> > ---
> > drivers/md/dm.c | 4 ++--
> > 1 files changed, 2 insertions(+), 2 deletions(-)
> >
> > Jun'ichi, was the following your implict Acked-by? Care to make it
> > explicit?
> > "Anyway, I think your bd_mutex patch should be fine for now and is
> > better than the current code with i_mutex, which has a real deadlock
> > issue."
>
> No, it was not an ACK.
> (This is not multipath. So I think you don't need my ack.)

Your ack is always meaningful but that is fine too.

> I'm reluctant to ack this because, as I wrote, it's prone to
> cause deadlock in future.
> But I couldn't find a real problem with the patch,
> so I'm not NACK-ing either.

dm_swap_table's md->suspend_mutex already provides adequate protection
of __set_size's i_size_write. You didn't like this because it implied:
"dm_resume is the only code which calls i_size_write() for dm device".

I don't see that as a problem at all; its a more meaningful/enforcable
rule to have in DM than something tethered to the generic bd_mutex.

I'll discuss this further with Alasdair and/or others.

Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel