Re: [PATCH 6/6] nbd: reduce the nbd_index_mutex scope
From: Josef Bacik <josef@toxicpanda.com>
Date: 2021-08-13 14:47:11
On 8/11/21 8:44 AM, Christoph Hellwig wrote:
quoted hunk ↗ jump to hunk
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