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.
