Re: [PATCH v11 22/26] block/rnbd: server: functionality for IO submission to file or block dev

[Bart Van Assche <bvanassche@acm.org>]

On 2020-03-20 05:16, Jack Wang wrote:
> This provides helper functions for IO submission to file or block dev.

Regarding the title of this patch: is file I/O still supported? Wasn't
that support removed some time ago?

> +struct rnbd_dev *rnbd_dev_open(const char *path, fmode_t flags,
> +			       void (*io_cb)(void *priv, int error))
> +{
> +	struct rnbd_dev *dev;
> +	int ret;
> +
> +	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> +	if (!dev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	dev->blk_open_flags = flags;
> +	dev->bdev = blkdev_get_by_path(path, flags, THIS_MODULE);
> +	ret = PTR_ERR_OR_ZERO(dev->bdev);
> +	if (ret)
> +		goto err;
> +
> +	dev->blk_open_flags	= flags;
> +	dev->io_cb		= io_cb;
> +	bdevname(dev->bdev, dev->name);
> +
> +	return dev;
> +
> +err:
> +	kfree(dev);
> +	return ERR_PTR(ret);
> +}

This function only has one caller so io_cb is always equal to the
argument passed by that single caller, namely rnbd_endio. If that
argument and also dev->io_cb would be removed, would that make the hot
path faster?

> +int rnbd_dev_submit_io(struct rnbd_dev *dev, sector_t sector, void *data,
> +			size_t len, u32 bi_size, enum rnbd_io_flags flags,
> +			short prio, void *priv)
> +{
> +	struct request_queue *q = bdev_get_queue(dev->bdev);
> +	struct rnbd_dev_blk_io *io;
> +	struct bio *bio;
> +
> +	/* check if the buffer is suitable for bdev */
> +	if (WARN_ON(!blk_rq_aligned(q, (unsigned long)data, len)))
> +		return -EINVAL;

The blk_rq_aligned() check looks weird to me. bio_map_kern() can handle
data buffers that do not match the DMA alignment requirements, so why to
refuse data buffers that are not satisfy DMA alignment requirements?

> +	/* Generate bio with pages pointing to the rdma buffer */
> +	bio = bio_map_kern(q, data, len, GFP_KERNEL);
> +	if (IS_ERR(bio))
> +		return PTR_ERR(bio);
> +
> +	io = kmalloc(sizeof(*io), GFP_KERNEL);
> +	if (unlikely(!io)) {
> +		bio_put(bio);
> +		return -ENOMEM;
> +	}
> +
> +	io->dev		= dev;
> +	io->priv	= priv;
> +
> +	bio->bi_end_io		= rnbd_dev_bi_end_io;
> +	bio->bi_private		= io;
> +	bio->bi_opf		= rnbd_to_bio_flags(flags);
> +	bio->bi_iter.bi_sector	= sector;
> +	bio->bi_iter.bi_size	= bi_size;
> +	bio_set_prio(bio, prio);
> +	bio_set_dev(bio, dev->bdev);

I think Jason strongly prefers to have a single space at the left of the
assignment operator.

Thanks,

Bart.