Re: [PATCH] nbd: make starting request more reasonable

[Yufen Yu <yuyufen@huawei.com>]

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