From: Jan Kara <jack@suse.cz>
To: Yu Kuai <yukuai1@huaweicloud.com>
Cc: Paolo VALENTE <paolo.valente@unimore.it>, Jan Kara <jack@suse.cz>,
cgroups@vger.kernel.org,
linux-block <linux-block@vger.kernel.org>,
Tejun Heo <tj@kernel.org>, Jens Axboe <axboe@kernel.dk>,
LKML <linux-kernel@vger.kernel.org>,
yi.zhang@huawei.com, "yukuai (C)" <yukuai3@huawei.com>
Subject: Re: [PATCH -next v10 3/4] block, bfq: refactor the counting of 'num_groups_with_pending_reqs'
Date: Wed, 14 Sep 2022 11:00:36 +0200 [thread overview]
Message-ID: <20220914090036.46zsrj2l23ubvvk6@quack3> (raw)
In-Reply-To: <97534773-484f-5c2c-a371-446cc0680b73@huaweicloud.com>
Hi guys!
On Wed 14-09-22 16:15:26, Yu Kuai wrote:
> 在 2022/09/14 15:50, Paolo VALENTE 写道:
> >
> >
> > > Il giorno 14 set 2022, alle ore 03:55, Yu Kuai <yukuai1@huaweicloud.com> ha scritto:
> > >
> > >
> > >
> > > 在 2022/09/07 9:16, Yu Kuai 写道:
> > > > Hi, Paolo!
> > > > 在 2022/09/06 17:37, Paolo Valente 写道:
> > > > >
> > > > >
> > > > > > Il giorno 26 ago 2022, alle ore 04:34, Yu Kuai <yukuai1@huaweicloud.com> ha scritto:
> > > > > >
> > > > > > Hi, Paolo!
> > > > > >
> > > > > > 在 2022/08/25 22:59, Paolo Valente 写道:
> > > > > > > > Il giorno 11 ago 2022, alle ore 03:19, Yu Kuai <yukuai1@huaweicloud.com <mailto:yukuai1@huaweicloud.com>> ha scritto:
> > > > > > > >
> > > > > > > > Hi, Paolo
> > > > > > > >
> > > > > > > > 在 2022/08/10 18:49, Paolo Valente 写道:
> > > > > > > > > > Il giorno 27 lug 2022, alle ore 14:11, Yu Kuai <yukuai1@huaweicloud.com <mailto:yukuai1@huaweicloud.com>> ha scritto:
> > > > > > > > > >
> > > > > > > > > > Hi, Paolo
> > > > > > > > > >
> > > > > > > > > hi
> > > > > > > > > > Are you still interested in this patchset?
> > > > > > > > > >
> > > > > > > > > Yes. Sorry for replying very late again.
> > > > > > > > > Probably the last fix that you suggest is enough, but I'm a little bit
> > > > > > > > > concerned that it may be a little hasty. In fact, before this fix, we
> > > > > > > > > exchanged several messages, and I didn't seem to be very good at
> > > > > > > > > convincing you about the need to keep into account also in-service
> > > > > > > > > I/O. So, my question is: are you sure that now you have a
> > > > > > > >
> > > > > > > > I'm confused here, I'm pretty aware that in-service I/O(as said pending
> > > > > > > > requests is the patchset) should be counted, as you suggested in v7, are
> > > > > > > > you still thinking that the way in this patchset is problematic?
> > > > > > > >
> > > > > > > > I'll try to explain again that how to track is bfqq has pending pending
> > > > > > > > requests, please let me know if you still think there are some problems:
> > > > > > > >
> > > > > > > > patch 1 support to track if bfqq has pending requests, it's
> > > > > > > > done by setting the flag 'entity->in_groups_with_pending_reqs' when the
> > > > > > > > first request is inserted to bfqq, and it's cleared when the last
> > > > > > > > request is completed. specifically the flag is set in
> > > > > > > > bfq_add_bfqq_busy() when 'bfqq->dispatched' if false, and it's cleared
> > > > > > > > both in bfq_completed_request() and bfq_del_bfqq_busy() when
> > > > > > > > 'bfqq->diapatched' is false.
> > > > > > > >
> > > > > > > This general description seems correct to me. Have you already sent a new version of your patchset?
> > > > > >
> > > > > > It's glad that we finially on the same page here.
> > > > > >
> > > > >
> > > > > Yep. Sorry for my chronicle delay.
> > > > Better late than never 😁
> > > > >
> > > > > > Please take a look at patch 1, which already impelement the above
> > > > > > descriptions, it seems to me there is no need to send a new version
> > > > > > for now. If you think there are still some other problems, please let
> > > > > > me know.
> > > > > >
> > > > >
> > > > > Patch 1 seems ok to me. I seem to have only one pending comment on this patch (3/4) instead. Let me paste previous stuff here for your convenience:
> > > > That sounds good.
> > > > >
> > > > > > >
> > > > > > > - /*
> > > > > > > - * Next function is invoked last, because it causes bfqq to be
> > > > > > > - * freed if the following holds: bfqq is not in service and
> > > > > > > - * has no dispatched request. DO NOT use bfqq after the next
> > > > > > > - * function invocation.
> > > > > > > - */
> > > > > > I would really love it if you leave this comment. I added it after
> > > > > > suffering a lot for a nasty UAF. Of course the first sentence may
> > > > > > need to be adjusted if the code that precedes it is to be removed.
> > > > > > Same as above, if this patch is applied, this function will be gone.
> > >
> > > Hi, I'm curious while I'm trying to add the comment, before this
> > > patchset, can bfqq be freed when bfq_weights_tree_remove is called?
> > >
> > > bfq_completed_request
> > > bfqq->dispatched--
> > > if (!bfqq->dispatched && !bfq_bfqq_busy(bfqq))
> > > bfq_weights_tree_remove(bfqd, bfqq);
> > >
> > > // continue to use bfqq
> > >
> > > It seems to me this is problematic if so, because bfqq is used after
> > > bfq_weights_tree_remove() is called.
> > >
> >
> > It is. Yet, IIRC, I verified that bfqq was not used after that free,
> > and I added that comment as a heads-up. What is a scenario (before
> > your pending modifications) where this use-after-free happens?
> >
>
> No, it never happens, I just notice it because it'll be weird if I
> place the comment where bfq_weights_tree_remove() is called, since bfqq
> will still be accessed.
>
> If the suituation that the comment says is possible, perhaps we should
> move bfq_weights_tree_remove() to the last of bfq_completed_request().
> However, it seems that we haven't meet the problem for quite a long
> time...
I'm bit confused which comment you are speaking about but
bfq_completed_request() gets called only from bfq_finish_requeue_request()
and the request itself still holds a reference to bfqq. Only later in
bfq_finish_requeue_request() when we do:
bfqq_request_freed(bfqq);
bfq_put_queue(bfqq);
bfqq can get freed.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
next prev parent reply other threads:[~2022-09-14 9:01 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-10 2:16 [PATCH -next v10 0/4] support concurrent sync io for bfq on a specail occasion Yu Kuai
2022-06-10 2:16 ` [PATCH -next v10 1/4] block, bfq: support to track if bfqq has pending requests Yu Kuai
2022-06-10 2:16 ` [PATCH -next v10 2/4] block, bfq: record how many queues have " Yu Kuai
2022-06-10 2:17 ` [PATCH -next v10 3/4] block, bfq: refactor the counting of 'num_groups_with_pending_reqs' Yu Kuai
2022-06-23 15:32 ` Paolo Valente
2022-06-24 1:26 ` Yu Kuai
2022-06-25 8:10 ` Yu Kuai
2022-07-12 13:30 ` Yu Kuai
[not found] ` <C2CF100A-9A7C-4300-9A70-1295BC939C66@unimore.it>
2022-07-20 11:38 ` Yu Kuai
2022-07-27 12:11 ` Yu Kuai
2022-08-05 11:20 ` Yu Kuai
2022-08-10 10:49 ` Paolo Valente
2022-08-11 1:19 ` Yu Kuai
2022-08-25 12:14 ` Yu Kuai
[not found] ` <D89DCF20-27D8-4F8F-B8B0-FD193FC4F18D@unimore.it>
2022-08-26 2:34 ` Yu Kuai
2022-09-06 9:37 ` Paolo Valente
2022-09-07 1:16 ` Yu Kuai
2022-09-14 1:55 ` Yu Kuai
2022-09-14 7:50 ` Paolo VALENTE
2022-09-14 8:15 ` Yu Kuai
2022-09-14 9:00 ` Jan Kara [this message]
2022-09-15 1:18 ` Yu Kuai
2022-06-10 2:17 ` [PATCH -next v10 4/4] block, bfq: do not idle if only one group is activated Yu Kuai
2022-06-17 1:12 ` [PATCH -next v10 0/4] support concurrent sync io for bfq on a specail occasion Yu Kuai
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=20220914090036.46zsrj2l23ubvvk6@quack3 \
--to=jack@suse.cz \
--cc=axboe@kernel.dk \
--cc=cgroups@vger.kernel.org \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=paolo.valente@unimore.it \
--cc=tj@kernel.org \
--cc=yi.zhang@huawei.com \
--cc=yukuai1@huaweicloud.com \
--cc=yukuai3@huawei.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 NNTP newsgroup(s).