Re: [PATCH -next v10 3/4] block, bfq: refactor the counting of 'num_groups_with_pending_reqs'

[Yu Kuai]

在 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@xxxxxxxxxxxxxxx> ha scritto:
> > >
> > > Hi, Paolo!
> > >
> > > 在 2022/08/25 22:59, Paolo Valente 写道:
> > > > > Il giorno 11 ago 2022, alle ore 03:19, Yu Kuai 
> > > > > <yukuai1@xxxxxxxxxxxxxxx <mailto:yukuai1@xxxxxxxxxxxxxxx>> ha scritto:
> > > > >
> > > > > Hi, Paolo
> > > > >
> > > > > 在 2022/08/10 18:49, Paolo Valente 写道:
> > > > > > > Il giorno 27 lug 2022, alle ore 14:11, Yu Kuai 
> > > > > > > <yukuai1@xxxxxxxxxxxxxxx <mailto:yukuai1@xxxxxxxxxxxxxxx>> 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.

Thanks,
Kuai