Re: [Xen-devel] [PATCH RFC 1/4] xen, blkfront: add support for the multi-queue block layer API

[Christoph Hellwig <hch@xxxxxxxxxxxxx>]

Hi Arianna,

thanks for doing this work!

keeping both the legacy and blk-mq is fine for testing, but before you
submit the code for submission please make sure the blk-mq path
unconditionally better and remove the legacy one, similar to most
drivers we converted (virtio, mtip, soon nvme)

> +static int blkfront_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req)
> +{
> +     struct blkfront_info *info = req->rq_disk->private_data;
> +
> +     pr_debug("Entered blkfront_queue_rq\n");
> +
> +     spin_lock_irq(&info->io_lock);
> +     if (RING_FULL(&info->ring))
> +             goto wait;
> +
> +     if ((req->cmd_type != REQ_TYPE_FS) ||
> +                     ((req->cmd_flags & (REQ_FLUSH | REQ_FUA)) &&
> +                      !info->flush_op)) {
> +             req->errors = -EIO;
> +             blk_mq_complete_request(req);
> +             spin_unlock_irq(&info->io_lock);
> +             return BLK_MQ_RQ_QUEUE_ERROR;


> +     if (blkif_queue_request(req)) {
> +wait:

Just a small style nipick: goto labels inside conditionals are not
very easy to undertand.  Just add another goto here and move the wait
label and its code to the very end of the function.

> +static int blkfront_init_hctx(struct blk_mq_hw_ctx *hctx, void *data,
> +                       unsigned int index)
> +{
> +     return 0;
> +}

There is no need to have an empty implementation of this function,
the blk-mq code is fine with not having one.

> +static void blkfront_complete(struct request *req)
> +{
> +     blk_mq_end_io(req, req->errors);
> +}

No need to have this one either, blk_mq_end_io is the default I/O
completion implementation if no other one is provided.

> +             memset(&info->tag_set, 0, sizeof(info->tag_set));
> +             info->tag_set.ops = &blkfront_mq_ops;
> +             info->tag_set.nr_hw_queues = hardware_queues;
> +             info->tag_set.queue_depth = BLK_RING_SIZE;
> +             info->tag_set.numa_node = NUMA_NO_NODE;
> +             info->tag_set.flags = BLK_MQ_F_SHOULD_MERGE;

You probably also want the recently added BLK_MQ_F_SG_MERGE flag,
and maybe BLK_MQ_F_SHOULD_SORT depending on the speed of the device.

Does Xenstore expose something like a rotational flag to key off wether
we want to do guest side merging/scheduling?

> +             info->tag_set.cmd_size = 0;
> +             info->tag_set.driver_data = info;
> +
> +             if (blk_mq_alloc_tag_set(&info->tag_set))
> +                     return -1;
> +             rq = blk_mq_init_queue(&info->tag_set);
> +             if (!rq) {
> +                     blk_mq_free_tag_set(&info->tag_set);
> +                     return -1;

It seems like returning -1 is the existing style in this driver, but
it's generaly preferable to return a real errno.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel