diff for duplicates of <1544658892.185366.412.camel@acm.org> diff --git a/a/1.txt b/N1/1.txt index d035e58..26abb13 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,53 +1,53 @@ 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 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+AF8-create() is called with the queue lock held +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 +ADw-bvanassche+AEA-acm.org+AD4 +Reviewed-by: Bart Van Assche <bvanassche@acm.org> diff --git a/a/content_digest b/N1/content_digest index 7165557..21340ca 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -16,57 +16,57 @@ "\00:1\0" "b\0" "On Tue, 2018-12-11 at 23:06 -0500, Dennis Zhou wrote:\n" - "+AD4 Hi Bart,\n" - "+AD4 \n" - "+AD4 On Tue, Dec 11, 2018 at 03:16:13PM -0800, Bart Van Assche wrote:\n" - "+AD4 +AD4 On Tue, 2018-12-11 at 18:03 -0500, Dennis Zhou wrote:\n" - "+AD4 +AD4 +AD4 diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c\n" - "+AD4 +AD4 +AD4 index 6bd0619a7d6e..c30661ddc873 100644\n" - "+AD4 +AD4 +AD4 --- a/block/blk-cgroup.c\n" - "+AD4 +AD4 +AD4 +-+-+- b/block/blk-cgroup.c\n" - "+AD4 +AD4 +AD4 +AEAAQA -202,6 +-202,12 +AEAAQA static struct blkcg+AF8-gq +ACo-blkg+AF8-create(struct blkcg +ACo-blkcg,\n" - "+AD4 +AD4 +AD4 \tWARN+AF8-ON+AF8-ONCE(+ACE-rcu+AF8-read+AF8-lock+AF8-held())+ADs\n" - "+AD4 +AD4 +AD4 \tlockdep+AF8-assert+AF8-held(+ACY-q-+AD4-queue+AF8-lock)+ADs\n" - "+AD4 +AD4 +AD4 \n" - "+AD4 +AD4 +AD4 +-\t/+ACo request+AF8-queue is dying, do not create/recreate a blkg +ACo-/\n" - "+AD4 +AD4 +AD4 +-\tif (blk+AF8-queue+AF8-dying(q)) +AHs\n" - "+AD4 +AD4 +AD4 +-\t\tret +AD0 -ENODEV+ADs\n" - "+AD4 +AD4 +AD4 +-\t\tgoto err+AF8-free+AF8-blkg+ADs\n" - "+AD4 +AD4 +AD4 +-\t+AH0\n" - "+AD4 +AD4 +AD4 +-\n" - "+AD4 +AD4 +AD4 \t/+ACo blkg holds a reference to blkcg +ACo-/\n" - "+AD4 +AD4 +AD4 \tif (+ACE-css+AF8-tryget+AF8-online(+ACY-blkcg-+AD4-css)) +AHs\n" - "+AD4 +AD4 +AD4 \t\tret +AD0 -ENODEV+ADs\n" - "+AD4 +AD4 \n" - "+AD4 +AD4 What prevents that the queue state changes after blk+AF8-queue+AF8-dying() has returned\n" - "+AD4 +AD4 and before blkg+AF8-create() returns? Are you sure you don't need to protect this\n" - "+AD4 +AD4 code with a blk+AF8-queue+AF8-enter() / blk+AF8-queue+AF8-exit() pair?\n" - "+AD4 +AD4 \n" - "+AD4 \n" - "+AD4 Hmmm. So I think the idea is that we rely on normal shutdown as I don't\n" - "+AD4 think there is anything wrong with creating a blkg on a dying\n" - "+AD4 request+AF8-queue. When we are doing association, the request+AF8-queue should\n" - "+AD4 be pinned by the open call. What we are racing against is when the\n" - "+AD4 request+AF8-queue is shutting down, it goes around and destroys the blkgs.\n" - "+AD4 For clarity, QUEUE+AF8-FLAG+AF8-DYING is set in blk+AF8-cleanup+AF8-queue() before\n" - "+AD4 calling blk+AF8-exit+AF8-queue() which eventually calls blkcg+AF8-exit+AF8-queue().\n" - "+AD4 \n" - "+AD4 The use of blk+AF8-queue+AF8-dying() is to determine whether blkg shutdown has\n" - "+AD4 already started as if we create one after it has started, we may\n" - "+AD4 incorrectly orphan a blkg and leak it. Both blkg creation and\n" - "+AD4 destruction require holding the queue+AF8-lock, so if the QUEUE+AF8-FLAG+AF8-DYING\n" - "+AD4 flag is set after we've checked it, it means blkg destruction hasn't\n" - "+AD4 started because it has to wait on the queue+AF8-lock. If QUEUE+AF8-FLAG+AF8-DYING is\n" - "+AD4 set, then we have no guarantee of knowing what phase blkg destruction is\n" - "+AD4 in leading to a potential leak.\n" + "> Hi Bart,\n" + "> \n" + "> On Tue, Dec 11, 2018 at 03:16:13PM -0800, Bart Van Assche wrote:\n" + "> > On Tue, 2018-12-11 at 18:03 -0500, Dennis Zhou wrote:\n" + "> > > diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c\n" + "> > > index 6bd0619a7d6e..c30661ddc873 100644\n" + "> > > --- a/block/blk-cgroup.c\n" + "> > > +++ b/block/blk-cgroup.c\n" + "> > > @@ -202,6 +202,12 @@ static struct blkcg_gq *blkg_create(struct blkcg *blkcg,\n" + "> > > \tWARN_ON_ONCE(!rcu_read_lock_held());\n" + "> > > \tlockdep_assert_held(&q->queue_lock);\n" + "> > > \n" + "> > > +\t/* request_queue is dying, do not create/recreate a blkg */\n" + "> > > +\tif (blk_queue_dying(q)) {\n" + "> > > +\t\tret = -ENODEV;\n" + "> > > +\t\tgoto err_free_blkg;\n" + "> > > +\t}\n" + "> > > +\n" + "> > > \t/* blkg holds a reference to blkcg */\n" + "> > > \tif (!css_tryget_online(&blkcg->css)) {\n" + "> > > \t\tret = -ENODEV;\n" + "> > \n" + "> > What prevents that the queue state changes after blk_queue_dying() has returned\n" + "> > and before blkg_create() returns? Are you sure you don't need to protect this\n" + "> > code with a blk_queue_enter() / blk_queue_exit() pair?\n" + "> > \n" + "> \n" + "> Hmmm. So I think the idea is that we rely on normal shutdown as I don't\n" + "> think there is anything wrong with creating a blkg on a dying\n" + "> request_queue. When we are doing association, the request_queue should\n" + "> be pinned by the open call. What we are racing against is when the\n" + "> request_queue is shutting down, it goes around and destroys the blkgs.\n" + "> For clarity, QUEUE_FLAG_DYING is set in blk_cleanup_queue() before\n" + "> calling blk_exit_queue() which eventually calls blkcg_exit_queue().\n" + "> \n" + "> The use of blk_queue_dying() is to determine whether blkg shutdown has\n" + "> already started as if we create one after it has started, we may\n" + "> incorrectly orphan a blkg and leak it. Both blkg creation and\n" + "> destruction require holding the queue_lock, so if the QUEUE_FLAG_DYING\n" + "> flag is set after we've checked it, it means blkg destruction hasn't\n" + "> started because it has to wait on the queue_lock. If QUEUE_FLAG_DYING is\n" + "> set, then we have no guarantee of knowing what phase blkg destruction is\n" + "> in leading to a potential leak.\n" "\n" "Hi Dennis,\n" "\n" "To answer my own question: since all queue flag manipulations are protected\n" - "by the queue lock and since blkg+AF8-create() is called with the queue lock held\n" + "by the queue lock and since blkg_create() is called with the queue lock held\n" "the above code does not need any further protection. Hence feel free to add\n" "the following:\n" "\n" - Reviewed-by: Bart Van Assche +ADw-bvanassche+AEA-acm.org+AD4 + Reviewed-by: Bart Van Assche <bvanassche@acm.org> -9236c6b35b6221acad98aab29a8a851aac42e4862d966ba065b1c8e6796c63f2 +f8ccfe05e0e0d59ec37723f10524bef60745876f37a521d51f5b79ac16fb4fb3
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.