From: Dongsheng Yang <dongsheng.yang@easystack.cn>
To: Ilya Dryomov <idryomov@gmail.com>, ceph-devel@vger.kernel.org
Subject: Re: [PATCH 4/4] rbd: take header_rwsem in rbd_dev_refresh() only when updating
Date: Tue, 26 Sep 2023 16:23:58 +0800 [thread overview]
Message-ID: <99159846-2913-9ffe-fa36-776cefecb8d5@easystack.cn> (raw)
In-Reply-To: <20230925194036.197899-5-idryomov@gmail.com>
在 2023/9/26 星期二 上午 3:40, Ilya Dryomov 写道:
> rbd_dev_refresh() has been holding header_rwsem across header and
> parent info read-in unnecessarily for ages. With commit 870611e4877e
> ("rbd: get snapshot context after exclusive lock is ensured to be
> held"), the potential for deadlocks became much more real owning to
> a) header_rwsem now nesting inside lock_rwsem and b) rw_semaphores
> not allowing new readers after a writer is registered.
>
> For example, assuming that I/O request 1, I/O request 2 and header
> read-in request all target the same OSD:
>
> 1. I/O request 1 comes in and gets submitted
> 2. watch error occurs
> 3. rbd_watch_errcb() takes lock_rwsem for write, clears owner_cid and
> releases lock_rwsem
> 4. after reestablishing the watch, rbd_reregister_watch() calls
> rbd_dev_refresh() which takes header_rwsem for write and submits
> a header read-in request
> 5. I/O request 2 comes in: after taking lock_rwsem for read in
> __rbd_img_handle_request(), it blocks trying to take header_rwsem
> for read in rbd_img_object_requests()
> 6. another watch error occurs
> 7. rbd_watch_errcb() blocks trying to take lock_rwsem for write
> 8. I/O request 1 completion is received by the messenger but can't be
> processed because lock_rwsem won't be granted anymore
> 9. header read-in request completion can't be received, let alone
> processed, because the messenger is stranded
>
> Change rbd_dev_refresh() to take header_rwsem only for actually
> updating rbd_dev->header. Header and parent info read-in don't need
> any locking.
Nice catch
Reviewed-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
for the whole patchset.
>
> Cc: stable@vger.kernel.org # e3580eaee090: rbd: move rbd_dev_refresh() definition
> Cc: stable@vger.kernel.org # 641d828d82d0: rbd: decouple header read-in from updating rbd_dev->header
> Cc: stable@vger.kernel.org # 2147c0b31b95: rbd: decouple parent info read-in from updating rbd_dev
> Cc: stable@vger.kernel.org
> Fixes: 870611e4877e ("rbd: get snapshot context after exclusive lock is ensured to be held")
> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
> ---
> drivers/block/rbd.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index d62a0298c890..a999b698b131 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -6986,7 +6986,14 @@ static void rbd_dev_update_header(struct rbd_device *rbd_dev,
> rbd_assert(rbd_image_format_valid(rbd_dev->image_format));
> rbd_assert(rbd_dev->header.object_prefix); /* !first_time */
>
> - rbd_dev->header.image_size = header->image_size;
> + if (rbd_dev->header.image_size != header->image_size) {
> + rbd_dev->header.image_size = header->image_size;
> +
> + if (!rbd_is_snap(rbd_dev)) {
> + rbd_dev->mapping.size = header->image_size;
> + rbd_dev_update_size(rbd_dev);
> + }
> + }
>
> ceph_put_snap_context(rbd_dev->header.snapc);
> rbd_dev->header.snapc = header->snapc;
> @@ -7044,11 +7051,9 @@ static int rbd_dev_refresh(struct rbd_device *rbd_dev)
> {
> struct rbd_image_header header = { 0 };
> struct parent_image_info pii = { 0 };
> - u64 mapping_size;
> int ret;
>
> - down_write(&rbd_dev->header_rwsem);
> - mapping_size = rbd_dev->mapping.size;
> + dout("%s rbd_dev %p\n", __func__, rbd_dev);
>
> ret = rbd_dev_header_info(rbd_dev, &header, false);
> if (ret)
> @@ -7064,18 +7069,13 @@ static int rbd_dev_refresh(struct rbd_device *rbd_dev)
> goto out;
> }
>
> + down_write(&rbd_dev->header_rwsem);
> rbd_dev_update_header(rbd_dev, &header);
> if (rbd_dev->parent)
> rbd_dev_update_parent(rbd_dev, &pii);
> -
> - rbd_assert(!rbd_is_snap(rbd_dev));
> - rbd_dev->mapping.size = rbd_dev->header.image_size;
> -
> -out:
> up_write(&rbd_dev->header_rwsem);
> - if (!ret && mapping_size != rbd_dev->mapping.size)
> - rbd_dev_update_size(rbd_dev);
>
> +out:
> rbd_parent_info_cleanup(&pii);
> rbd_image_header_cleanup(&header);
> return ret;
>
prev parent reply other threads:[~2023-09-26 8:29 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-25 19:40 [PATCH 0/4] rbd: fix a deadlock around header_rwsem and lock_rwsem Ilya Dryomov
2023-09-25 19:40 ` [PATCH 1/4] rbd: move rbd_dev_refresh() definition Ilya Dryomov
2023-09-25 19:40 ` [PATCH 2/4] rbd: decouple header read-in from updating rbd_dev->header Ilya Dryomov
2023-09-25 19:40 ` [PATCH 3/4] rbd: decouple parent info read-in from updating rbd_dev Ilya Dryomov
2023-09-25 19:40 ` [PATCH 4/4] rbd: take header_rwsem in rbd_dev_refresh() only when updating Ilya Dryomov
2023-09-26 8:23 ` Dongsheng Yang [this message]
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=99159846-2913-9ffe-fa36-776cefecb8d5@easystack.cn \
--to=dongsheng.yang@easystack.cn \
--cc=ceph-devel@vger.kernel.org \
--cc=idryomov@gmail.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 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 read-only IMAP folder(s) and NNTP newsgroup(s).