Re: [PATCH] nbd: make starting request more reasonable
Hi, Josef
On 2020/3/4 5:18, Josef Bacik wrote:
On 3/3/20 8:08 AM, Yufen Yu wrote:
Our test robot reported a warning for refcount_dec trying to decrease
value '0'. The reason is that blk_mq_dispatch_rq_list() try to complete
the failed request from nbd driver, while the request have finished in
nbd timeout handle function. The race as following:
CPU1 CPU2
//req->ref = 1
blk_mq_dispatch_rq_list
nbd_queue_rq
nbd_handle_cmd
blk_mq_start_request
blk_mq_check_expired
//req->ref = 2
blk_mq_rq_timed_out
nbd_xmit_timeout
blk_mq_complete_request
//req->ref = 1
refcount_dec_and_test(&req->ref)
refcount_dec_and_test(&req->ref)
//req->ref = 0
__blk_mq_free_request(req)
ret = BLK_STS_IOERR
blk_mq_end_request
// req->ref = 0, req have been free
refcount_dec_and_test(&rq->ref)
In fact, the bug also have been reported by syzbot:
https://lkml.org/lkml/2018/12/5/1308
Since the request have been freed by timeout handle, it can be reused
by others. Then, blk_mq_end_request() may get the re-initialized request
and free it, which is unexpected.
To fix the problem, we move blk_mq_start_request() down until the driver
will handle the request actully. If .queue_rq return something error in
preparation phase, timeout handle may don't need. Thus, moving start
request down may be more reasonable. Then, nbd_queue_rq() will not return
BLK_STS_IOERR after starting request.
This won't work, you have to have the request started if you return an error because of this in blk_mq_dispatch_rq_list >
if (unlikely(ret != BLK_STS_OK)) {
errors++;
blk_mq_end_request(rq, BLK_STS_IOERR);
continue;
}
The request has to be started before we return an error, pushing it down means we have all of these error cases where we haven't started the reques
IMO, the reason that we need to start request after issuing is for timeout
handle function could trace the request. Here, we should make sure the request
started before the driver process (e.g sock_xmit()). Right?
Before that, if something errors occur in nbd_handle_cmd(), like -EIO, -EINVAL,
that means the request have not actually been handled. So, we also don't need
timeout handler trace it. And the dispatcher function blk_mq_dispatch_rq_list()
or blk_mq_try_issue_directly() is responsible for ending the request.
BTW, other drivers, such as nvme_queue_rq(), scsi_queue_rq(), also start request
before processing it actually. If I get it wrong, please point it out.
Thanks,
Yufen
Reply to: