linux-kernel - Re: [PATCH V3] NVMe: basic conversion to blk-mq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 29 May 2014 08:25:56 -0600
From:	Jens Axboe <axboe@...nel.dk>
To:	Keith Busch <keith.busch@...el.com>,
	Matias Bjørling <m@...rling.me>
CC:	willy@...ux.intel.com, sbradshaw@...ron.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH V3] NVMe: basic conversion to blk-mq

On 2014-05-28 21:07, Keith Busch wrote:
> On Wed, 28 May 2014, Matias Bjørling wrote:
>> This converts the current NVMe driver to utilize the blk-mq layer.
>
> I am concerned about device hot removal since the h/w queues can be
> freed at any time. I *think* blk-mq helps with this in that the driver
> will not see a new request after calling blk_cleanup_queue. If you can
> confirm that's true and that blk-mq waits for all requests active in the
> driver to return to the block layer, then we're probably okay in this
> path. That wasn't true as a bio based driver which is why we are cautious
> with all the locking and barriers. But what about the IOCTL paths?

Barring any bugs in the code, then yes, this should work. On the scsi-mq 
side, extensive error injection and pulling has been done, and it seems 
to hold up fine now. The ioctl path would need to be audited.

>
> It also doesn't look like we're handling the case where the SGL can't
> map to a PRP.
>
>> +static void req_completion(struct nvme_queue *nvmeq, void *ctx,
>>                         struct nvme_completion *cqe)
>> {
>>     struct nvme_iod *iod = ctx;
>> -    struct bio *bio = iod->private;
>> +    struct request *req = iod->private;
>> +
>>     u16 status = le16_to_cpup(&cqe->status) >> 1;
>>
>> -    if (unlikely(status)) {
>> -        if (!(status & NVME_SC_DNR ||
>> -                bio->bi_rw & REQ_FAILFAST_MASK) &&
>> -                (jiffies - iod->start_time) < IOD_TIMEOUT) {
>> -            if (!waitqueue_active(&nvmeq->sq_full))
>> -                add_wait_queue(&nvmeq->sq_full,
>> -                            &nvmeq->sq_cong_wait);
>> -            list_add_tail(&iod->node, &nvmeq->iod_bio);
>> -            wake_up(&nvmeq->sq_full);
>> -            return;
>> -        }
>> -    }
>>     if (iod->nents) {
>> -        dma_unmap_sg(nvmeq->q_dmadev, iod->sg, iod->nents,
>> -            bio_data_dir(bio) ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
>> -        nvme_end_io_acct(bio, iod->start_time);
>> +        dma_unmap_sg(&nvmeq->dev->pci_dev->dev, iod->sg, iod->nents,
>> +            rq_data_dir(req) ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
>>     }
>>     nvme_free_iod(nvmeq->dev, iod);
>> -    if (status)
>> -        bio_endio(bio, -EIO);
>> +
>> +    if (unlikely(status))
>> +        req->errors = -EIO;
>>     else
>> -        bio_endio(bio, 0);
>> +        req->errors = 0;
>> +
>> +    blk_mq_complete_request(req);
>> }
>
> Is blk-mq going to retry intermittently failed commands for me? It
> doesn't look like it will.

Not sure what kind of behavior you are looking for here. If you can 
expand on the above a bit, I'll gladly help sort it out. Only the driver 
really knows if a particular request should be failed hard or retried. 
So you'd probably have to track retry counts in the request and 
reinsert/end as appropriate.

>
>> +static int nvme_submit_flush_sync(struct nvme_queue *nvmeq, struct
>> nvme_ns *ns)
>> +{
>> +    struct request *req;
>> +    struct nvme_command cmnd;
>> +
>> +    req = blk_mq_alloc_request(ns->queue, WRITE, GFP_KERNEL, false);
>> +    if (!req)
>> +        return -ENOMEM;
>> +
>> +    nvme_setup_flush(&cmnd, ns, req->tag);
>> +    nvme_submit_sync_cmd(req, &cmnd, NULL, NVME_IO_TIMEOUT);
>>
>>     return 0;
>> }
>
> It looks like this function above is being called from an interrupt
> context where we are already holding a spinlock. The sync command will
> try to take that same lock.

Yes, that code still looks very buggy. The initial alloc for 
flush_cmd_info should also retry, not fail hard, if that alloc fails. 
For the reinsert part, Matias, you want to look at the flush code in 
blk-mq and how that handles it.

>> +    if ((req->cmd_flags & REQ_FLUSH) && psegs) {
>> +        struct flush_cmd_info *flush_cmd = kmalloc(
>> +                sizeof(struct flush_cmd_info), GFP_KERNEL);
>
> The comment above says "may not sleep", but using GFP_KERNEL here. I
> actually think it is safe to sleep, though since you're not taking a
> lock until later, so maybe you can change all the allocs to GFP_KERNEL?

It might be safe, and it might not be. It depends on the CPU mapping to 
the queue. preemption _could_ be off here (if needed), so GFP_KERNEL 
cannot be used. Additionally, see above comment on what to do on alloc 
failure.

-- 
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/