Re: [PATCH v4 2/6] nbd: convert to use blk_mq_find_and_get_req()
On Wed, Sep 08, 2021 at 04:29:57PM +0800, yukuai (C) wrote:
> On 2021/09/08 16:00, Ming Lei wrote:
> > On Wed, Sep 08, 2021 at 03:37:06PM +0800, yukuai (C) wrote:
> > > On 2021/09/08 15:30, Ming Lei wrote:
> > >
> > > > > +put_req:
> > > > > + if (req)
> > > > > + blk_mq_put_rq_ref(req);
> > > > > return ret ? ERR_PTR(ret) : cmd;
> > > >
> > > > After the request's refcnt is dropped, it can be freed immediately, then
> > > > one stale command is returned to caller.
> > >
> > > Hi, Ming
> > >
> > > It's right this patch leave this problem. Please take a look at patch 3
> > > and patch 4, the problem should be fixed with these patches.
> >
> > Not see it is addressed in patch 3 & 4, and it is one obvious fault in
> > patch 2, please fix it from beginning by moving the refcnt drop
> > into recv_work().
>
> Hi, Ming
>
> With patch 3 & 4:
>
> if nbd_read_stat() return a valid cmd, then the refcnt should not drop
> to 0 before blk_mq_complete_request() in recv_work().
The valid cmd won't be timed out just between nbd_read_stat() and
calling blk_mq_complete_request()?
Yeah, the issue is addressed by patch 4, then please put 2 after
3 & 4, and suggest to add comment why request ref won't drop to zero
in recv_work().
>
> if nbd_read_stat() failed, it won't be a problem if the request is freed
> immediately when refcnt is dropped in nbd_read_stat().
>
> That's why I said that the problem will be fixed.
>
> BTW, if we move the refcnt drop into recv_work, we can only do that if
> nbd_read_stat() return a valid cmd. If we get a valid rq and failed in
> the following checkings in nbd_read_stat(), it's better to drop the
> refcnt in nbd_read_stat().
The usual pattern is to drop the refcnt on when the object isn't used
any more, so it is perfectly fine to hold the ref until that time.
Thanks,
Ming
Reply to: