All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mike Snitzer <snitzer@redhat.com>
To: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Cc: device-mapper development <dm-devel@redhat.com>
Subject: Re: dm: use revalidate_disk to update device size after set_capacity
Date: Thu, 28 Oct 2010 18:18:02 -0400	[thread overview]
Message-ID: <20101028221802.GA1364@redhat.com> (raw)
In-Reply-To: <20101028195432.GA23063@redhat.com>

On Thu, Oct 28 2010 at  3:54pm -0400,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Thu, Oct 28 2010 at  8:15am -0400,
> Jun'ichi Nomura <j-nomura@ce.jp.nec.com> wrote:

> > Though I can't point out actual problem,
> > I think it's deadlock-prone to take bd_mutex in dm_swap_table.
> > 
> > There are already codes which do I/O while holding bd_mutex,
> > e.g. block/ioctl.c, though the code is not called for dm,
> > so we can' just set a general rule "Don't do I/O while holding bd_mutex".
> 
> Right, it would work provided we had that understanding within DM.

I think I misunderstood what you were saying as: no code does I/O to DM
while holding bd_mutex so we _can_ just set the general rule....

But re-reading your mail made me realize you were likely saying the
opposite?  e.g. your can' was meant to be can't.

So even though we currently can't see a real deadlock associated with
using ->bd_mutex directly you're contending there is potential for one
(may already be one in hiding or one could be introduced in the future).

That is fine, best to be conservative.

> I'll cleanup the patch from my previous mail to include some in-code
> comments and re-post it.  Please ack it if you agree that direct use of
> bd_mutex in __set_size() is a reasonable fix given the current code.

Taking a step back, don't we already have adequate locking for
__set_size's i_size_write() via dm_swap_table's md->suspend_lock?

So something like the following?

Mike
---
 drivers/md/dm.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 7cb1352..e71b8a1 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1996,9 +1996,8 @@ static void __set_size(struct mapped_device *md, sector_t size)
 {
 	set_capacity(md->disk, size);
 
-	mutex_lock(&md->bdev->bd_inode->i_mutex);
+	/* i_size_write() is protected by md->suspend_lock */
 	i_size_write(md->bdev->bd_inode, (loff_t)size << SECTOR_SHIFT);
-	mutex_unlock(&md->bdev->bd_inode->i_mutex);
 }
 
 /*

  reply	other threads:[~2010-10-28 22:18 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-10-19 22:07 [PATCH] dm: use revalidate_disk to update device size after set_capacity Mike Snitzer
2010-10-20  5:42 ` Jun'ichi Nomura
2010-10-28  1:16   ` Mike Snitzer
2010-10-28 12:15     ` Jun'ichi Nomura
2010-10-28 19:54       ` Mike Snitzer
2010-10-28 22:18         ` Mike Snitzer [this message]
2010-10-29  3:00         ` Jun'ichi Nomura
2010-10-29 21:50 ` dm: lock bd_mutex when setting device size Mike Snitzer
2010-11-01  7:19   ` Jun'ichi Nomura
2010-11-01 13:14     ` Mike Snitzer
2010-11-03 18:06       ` [PATCH] dm: remove extra locking when changing " Mike Snitzer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20101028221802.GA1364@redhat.com \
    --to=snitzer@redhat.com \
    --cc=dm-devel@redhat.com \
    --cc=j-nomura@ce.jp.nec.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.