From: Josef Bacik <josef@toxicpanda.com>
To: Christoph Hellwig <hch@lst.de>, Jens Axboe <axboe@kernel.dk>
Cc: linux-block@vger.kernel.org, nbd@other.debian.org,
Hou Tao <houtao1@huawei.com>
Subject: Re: [PATCH 6/6] nbd: reduce the nbd_index_mutex scope
Date: Fri, 13 Aug 2021 10:47:04 -0400 [thread overview]
Message-ID: <be56a424-7599-77f5-3f57-1f4609b3a0e6@toxicpanda.com> (raw)
In-Reply-To: <20210811124428.2368491-7-hch@lst.de>
On 8/11/21 8:44 AM, Christoph Hellwig wrote:
> nbd_index_mutex is currently held over add_disk and inside ->open, which
> leads to lock order reversals. Refactor the device creation code path
> so that nbd_dev_add is called without nbd_index_mutex lock held and
> only takes it for the IDR insertation.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/block/nbd.c | 55 +++++++++++++++++++++++----------------------
> 1 file changed, 28 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 0b46a608f879..4054cc33fc1e 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -1681,7 +1681,7 @@ static const struct blk_mq_ops nbd_mq_ops = {
> .timeout = nbd_xmit_timeout,
> };
>
> -static struct nbd_device *nbd_dev_add(int index)
> +static struct nbd_device *nbd_dev_add(int index, unsigned int refs)
> {
> struct nbd_device *nbd;
> struct gendisk *disk;
> @@ -1707,6 +1707,7 @@ static struct nbd_device *nbd_dev_add(int index)
> if (err)
> goto out_free_nbd;
>
> + mutex_lock(&nbd_index_mutex);
> if (index >= 0) {
> err = idr_alloc(&nbd_index_idr, nbd, index, index + 1,
> GFP_KERNEL);
> @@ -1717,6 +1718,7 @@ static struct nbd_device *nbd_dev_add(int index)
> if (err >= 0)
> index = err;
> }
> + mutex_unlock(&nbd_index_mutex);
> if (err < 0)
> goto out_free_tags;
> nbd->index = index;
> @@ -1743,7 +1745,7 @@ static struct nbd_device *nbd_dev_add(int index)
>
> mutex_init(&nbd->config_lock);
> refcount_set(&nbd->config_refs, 0);
> - refcount_set(&nbd->refs, 1);
> + refcount_set(&nbd->refs, refs);
> INIT_LIST_HEAD(&nbd->list);
> disk->major = NBD_MAJOR;
> disk->first_minor = index << part_shift;
> @@ -1847,34 +1849,35 @@ static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info)
> nbd = idr_find(&nbd_index_idr, index);
> }
>
> - if (!nbd) {
> - nbd = nbd_dev_add(index);
> - if (IS_ERR(nbd)) {
> + if (nbd) {
> + if (test_bit(NBD_DESTROY_ON_DISCONNECT, &nbd->flags) &&
> + test_bit(NBD_DISCONNECT_REQUESTED, &nbd->flags)) {
> + nbd->destroy_complete = &destroy_complete;
> mutex_unlock(&nbd_index_mutex);
> - pr_err("nbd: failed to add new device\n");
> - return PTR_ERR(nbd);
> +
> + /* wait until the nbd device is completely destroyed */
> + wait_for_completion(&destroy_complete);
> + goto again;
> }
> - }
>
> - if (test_bit(NBD_DESTROY_ON_DISCONNECT, &nbd->flags) &&
> - test_bit(NBD_DISCONNECT_REQUESTED, &nbd->flags)) {
> - nbd->destroy_complete = &destroy_complete;
> + if (!refcount_inc_not_zero(&nbd->refs)) {
> + mutex_unlock(&nbd_index_mutex);
> + if (index == -1)
> + goto again;
> + pr_err("nbd: device at index %d is going down\n",
> + index);
Errant whitespace here. Thanks,
Josef
next prev parent reply other threads:[~2021-08-13 14:47 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-11 12:44 nbd locking fixups Christoph Hellwig
2021-08-11 12:44 ` [PATCH 1/6] nbd: do del_gendisk() asynchronously for NBD_DESTROY_ON_DISCONNECT Christoph Hellwig
2021-08-11 12:44 ` [PATCH 2/6] nbd: refactor device removal Christoph Hellwig
2021-08-11 12:44 ` [PATCH 3/6] nbd: remove nbd_del_disk Christoph Hellwig
2021-08-11 12:44 ` [PATCH 4/6] nbd: return the allocated nbd_device from nbd_dev_add Christoph Hellwig
2021-08-11 12:44 ` [PATCH 5/6] nbd: refactor device search and allocation in nbd_genl_connect Christoph Hellwig
2021-08-11 12:44 ` [PATCH 6/6] nbd: reduce the nbd_index_mutex scope Christoph Hellwig
2021-08-11 15:57 ` Eric Blake
2021-08-13 14:47 ` Josef Bacik [this message]
2021-08-13 14:47 ` nbd locking fixups Josef Bacik
2021-08-13 19:32 ` Jens Axboe
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=be56a424-7599-77f5-3f57-1f4609b3a0e6@toxicpanda.com \
--to=josef@toxicpanda.com \
--cc=axboe@kernel.dk \
--cc=hch@lst.de \
--cc=houtao1@huawei.com \
--cc=linux-block@vger.kernel.org \
--cc=nbd@other.debian.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).