From: Bart Van Assche <bvanassche@acm.org>
To: Dennis Zhou <dennis@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>, Tejun Heo <tj@kernel.org>,
Josef Bacik <josef@toxicpanda.com>,
kernel-team@fb.com, linux-block@vger.kernel.org,
cgroups@vger.kernel.org, linux-kernel@vger.kernel.org,
Ming Lei <ming.lei@redhat.com>
Subject: Re: [PATCH] blkcg: handle dying request_queue when associating a blkg
Date: Wed, 12 Dec 2018 15:54:52 -0800 [thread overview]
Message-ID: <1544658892.185366.412.camel@acm.org> (raw)
In-Reply-To: <20181212040643.GA73727@dennisz-mbp.dhcp.thefacebook.com>
On Tue, 2018-12-11 at 23:06 -0500, Dennis Zhou wrote:
+AD4 Hi Bart,
+AD4
+AD4 On Tue, Dec 11, 2018 at 03:16:13PM -0800, Bart Van Assche wrote:
+AD4 +AD4 On Tue, 2018-12-11 at 18:03 -0500, Dennis Zhou wrote:
+AD4 +AD4 +AD4 diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
+AD4 +AD4 +AD4 index 6bd0619a7d6e..c30661ddc873 100644
+AD4 +AD4 +AD4 --- a/block/blk-cgroup.c
+AD4 +AD4 +AD4 +-+-+- b/block/blk-cgroup.c
+AD4 +AD4 +AD4 +AEAAQA -202,6 +-202,12 +AEAAQA static struct blkcg+AF8-gq +ACo-blkg+AF8-create(struct blkcg +ACo-blkcg,
+AD4 +AD4 +AD4 WARN+AF8-ON+AF8-ONCE(+ACE-rcu+AF8-read+AF8-lock+AF8-held())+ADs
+AD4 +AD4 +AD4 lockdep+AF8-assert+AF8-held(+ACY-q-+AD4-queue+AF8-lock)+ADs
+AD4 +AD4 +AD4
+AD4 +AD4 +AD4 +- /+ACo request+AF8-queue is dying, do not create/recreate a blkg +ACo-/
+AD4 +AD4 +AD4 +- if (blk+AF8-queue+AF8-dying(q)) +AHs
+AD4 +AD4 +AD4 +- ret +AD0 -ENODEV+ADs
+AD4 +AD4 +AD4 +- goto err+AF8-free+AF8-blkg+ADs
+AD4 +AD4 +AD4 +- +AH0
+AD4 +AD4 +AD4 +-
+AD4 +AD4 +AD4 /+ACo blkg holds a reference to blkcg +ACo-/
+AD4 +AD4 +AD4 if (+ACE-css+AF8-tryget+AF8-online(+ACY-blkcg-+AD4-css)) +AHs
+AD4 +AD4 +AD4 ret +AD0 -ENODEV+ADs
+AD4 +AD4
+AD4 +AD4 What prevents that the queue state changes after blk+AF8-queue+AF8-dying() has returned
+AD4 +AD4 and before blkg+AF8-create() returns? Are you sure you don't need to protect this
+AD4 +AD4 code with a blk+AF8-queue+AF8-enter() / blk+AF8-queue+AF8-exit() pair?
+AD4 +AD4
+AD4
+AD4 Hmmm. So I think the idea is that we rely on normal shutdown as I don't
+AD4 think there is anything wrong with creating a blkg on a dying
+AD4 request+AF8-queue. When we are doing association, the request+AF8-queue should
+AD4 be pinned by the open call. What we are racing against is when the
+AD4 request+AF8-queue is shutting down, it goes around and destroys the blkgs.
+AD4 For clarity, QUEUE+AF8-FLAG+AF8-DYING is set in blk+AF8-cleanup+AF8-queue() before
+AD4 calling blk+AF8-exit+AF8-queue() which eventually calls blkcg+AF8-exit+AF8-queue().
+AD4
+AD4 The use of blk+AF8-queue+AF8-dying() is to determine whether blkg shutdown has
+AD4 already started as if we create one after it has started, we may
+AD4 incorrectly orphan a blkg and leak it. Both blkg creation and
+AD4 destruction require holding the queue+AF8-lock, so if the QUEUE+AF8-FLAG+AF8-DYING
+AD4 flag is set after we've checked it, it means blkg destruction hasn't
+AD4 started because it has to wait on the queue+AF8-lock. If QUEUE+AF8-FLAG+AF8-DYING is
+AD4 set, then we have no guarantee of knowing what phase blkg destruction is
+AD4 in leading to a potential leak.
Hi Dennis,
To answer my own question: since all queue flag manipulations are protected
by the queue lock and since blkg+AF8-create() is called with the queue lock held
the above code does not need any further protection. Hence feel free to add
the following:
Reviewed-by: Bart Van Assche +ADw-bvanassche+AEA-acm.org+AD4
WARNING: multiple messages have this Message-ID (diff)
From: Bart Van Assche <bvanassche@acm.org>
To: Dennis Zhou <dennis@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>, Tejun Heo <tj@kernel.org>,
Josef Bacik <josef@toxicpanda.com>,
kernel-team@fb.com, linux-block@vger.kernel.org,
cgroups@vger.kernel.org, linux-kernel@vger.kernel.org,
Ming Lei <ming.lei@redhat.com>
Subject: Re: [PATCH] blkcg: handle dying request_queue when associating a blkg
Date: Wed, 12 Dec 2018 15:54:52 -0800 [thread overview]
Message-ID: <1544658892.185366.412.camel@acm.org> (raw)
In-Reply-To: <20181212040643.GA73727@dennisz-mbp.dhcp.thefacebook.com>
On Tue, 2018-12-11 at 23:06 -0500, Dennis Zhou wrote:
> Hi Bart,
>
> On Tue, Dec 11, 2018 at 03:16:13PM -0800, Bart Van Assche wrote:
> > On Tue, 2018-12-11 at 18:03 -0500, Dennis Zhou wrote:
> > > diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> > > index 6bd0619a7d6e..c30661ddc873 100644
> > > --- a/block/blk-cgroup.c
> > > +++ b/block/blk-cgroup.c
> > > @@ -202,6 +202,12 @@ static struct blkcg_gq *blkg_create(struct blkcg *blkcg,
> > > WARN_ON_ONCE(!rcu_read_lock_held());
> > > lockdep_assert_held(&q->queue_lock);
> > >
> > > + /* request_queue is dying, do not create/recreate a blkg */
> > > + if (blk_queue_dying(q)) {
> > > + ret = -ENODEV;
> > > + goto err_free_blkg;
> > > + }
> > > +
> > > /* blkg holds a reference to blkcg */
> > > if (!css_tryget_online(&blkcg->css)) {
> > > ret = -ENODEV;
> >
> > What prevents that the queue state changes after blk_queue_dying() has returned
> > and before blkg_create() returns? Are you sure you don't need to protect this
> > code with a blk_queue_enter() / blk_queue_exit() pair?
> >
>
> Hmmm. So I think the idea is that we rely on normal shutdown as I don't
> think there is anything wrong with creating a blkg on a dying
> request_queue. When we are doing association, the request_queue should
> be pinned by the open call. What we are racing against is when the
> request_queue is shutting down, it goes around and destroys the blkgs.
> For clarity, QUEUE_FLAG_DYING is set in blk_cleanup_queue() before
> calling blk_exit_queue() which eventually calls blkcg_exit_queue().
>
> The use of blk_queue_dying() is to determine whether blkg shutdown has
> already started as if we create one after it has started, we may
> incorrectly orphan a blkg and leak it. Both blkg creation and
> destruction require holding the queue_lock, so if the QUEUE_FLAG_DYING
> flag is set after we've checked it, it means blkg destruction hasn't
> started because it has to wait on the queue_lock. If QUEUE_FLAG_DYING is
> set, then we have no guarantee of knowing what phase blkg destruction is
> in leading to a potential leak.
Hi Dennis,
To answer my own question: since all queue flag manipulations are protected
by the queue lock and since blkg_create() is called with the queue lock held
the above code does not need any further protection. Hence feel free to add
the following:
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
next prev parent reply other threads:[~2018-12-12 23:54 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-11 23:03 [PATCH] blkcg: handle dying request_queue when associating a blkg Dennis Zhou
2018-12-11 23:16 ` Bart Van Assche
2018-12-11 23:16 ` Bart Van Assche
2018-12-12 4:06 ` Dennis Zhou
2018-12-12 23:54 ` Bart Van Assche [this message]
2018-12-12 23:54 ` Bart Van Assche
2018-12-13 15:47 ` Dennis Zhou
2018-12-13 0:43 ` 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=1544658892.185366.412.camel@acm.org \
--to=bvanassche@acm.org \
--cc=axboe@kernel.dk \
--cc=cgroups@vger.kernel.org \
--cc=dennis@kernel.org \
--cc=josef@toxicpanda.com \
--cc=kernel-team@fb.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=ming.lei@redhat.com \
--cc=tj@kernel.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 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.