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

[Christoph Hellwig <hch@infradead.org>]

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.