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

From: Jinpu Wang <jinpu.wang@cloud.ionos.com>
To: Bart Van Assche <bvanassche@acm.org>
Cc: linux-block@vger.kernel.org, linux-rdma@vger.kernel.org,
	Jens Axboe <axboe@kernel.dk>,
	Christoph Hellwig <hch@infradead.org>,
	Sagi Grimberg <sagi@grimberg.me>,
	Leon Romanovsky <leon@kernel.org>,
	Doug Ledford <dledford@redhat.com>,
	Jason Gunthorpe <jgg@ziepe.ca>,
	Danil Kipnis <danil.kipnis@cloud.ionos.com>,
	Roman Penyaev <rpenyaev@suse.de>,
	Pankaj Gupta <pankaj.gupta@cloud.ionos.com>
Subject: Re: [PATCH v11 22/26] block/rnbd: server: functionality for IO submission to file or block dev
Date: Tue, 31 Mar 2020 12:06:05 +0200	[thread overview]
Message-ID: <CAMGffEkOM9k2A60OHPBbPqRPp09+D5DeThB3iMLFqnLjLc=QAg@mail.gmail.com> (raw)
In-Reply-To: <cfe1dba2-04f7-f74d-af90-c70dce4d85cf@acm.org>

On Sat, Mar 28, 2020 at 7:39 PM Bart Van Assche <bvanassche@acm.org> wrote:
>
> 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?
Sorry, we forget to update it, will fix.
>
> > +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?
Sounds good, will do it.

>
> > +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?
We add the check since 2014 to make sure the data buffer is aligned.
AFAIR we nerver see the WARN triggered.
so will remove it.
>
> > +     /* 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.
ok.
>
> Thanks,
>
> Bart.
Thanks!