On Sun, Mar 1, 2020 at 4:09 AM Bart Van Assche <bvanassche@xxxxxxx> wrote:
>
> On 2020-02-21 02:47, Jack Wang wrote:
> > +static struct bio *rnbd_bio_map_kern(struct request_queue *q, void *data,
> > + struct bio_set *bs,
> > + unsigned int len, gfp_t gfp_mask)
> > +{
> > + unsigned long kaddr = (unsigned long)data;
> > + unsigned long end = (kaddr + len + PAGE_SIZE - 1) >> PAGE_SHIFT;
> > + unsigned long start = kaddr >> PAGE_SHIFT;
> > + const int nr_pages = end - start;
> > + int offset, i;
> > + struct bio *bio;
> > +
> > + bio = bio_alloc_bioset(gfp_mask, nr_pages, bs);
> > + if (!bio)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + offset = offset_in_page(kaddr);
> > + for (i = 0; i < nr_pages; i++) {
> > + unsigned int bytes = PAGE_SIZE - offset;
> > +
> > + if (len <= 0)
> > + break;
> > +
> > + if (bytes > len)
> > + bytes = len;
> > +
> > + if (bio_add_pc_page(q, bio, virt_to_page(data), bytes,
> > + offset) < bytes) {
> > + /* we don't support partial mappings */
> > + bio_put(bio);
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > + data += bytes;
> > + len -= bytes;
> > + offset = 0;
> > + }
> > +
> > + bio->bi_end_io = bio_put;
> > + return bio;
> > +}
>
> The above function is almost identical to bio_map_kern(). Please find a
> way to prevent such code duplication.
Hi Bart,
We prealloc bio_set in order to avoid allocation in io path done by
bio_map_kern() (call to bio_kmalloc). Instead we use
bio_alloc_bioset() with a preallocated bio_set. Will test whether
performance advantage is measurable and if not will switch to
bio_map_kern.
>
> > +static inline int rnbd_dev_get_logical_bsize(const struct rnbd_dev *dev)
> > +{
> > + return bdev_logical_block_size(dev->bdev);
> > +}
> > +
> > +static inline int rnbd_dev_get_phys_bsize(const struct rnbd_dev *dev)
> > +{
> > + return bdev_physical_block_size(dev->bdev);
> > +}
> > +
> > +static inline int
> > +rnbd_dev_get_max_write_same_sects(const struct rnbd_dev *dev)
> > +{
> > + return bdev_write_same(dev->bdev);
> > +}
>
> Are you sure that the above functions are useful? Please do not
> introduce inline functions for well known functions, especially if their
> function name is longer than their implementation.
This was initially introduced to abstract fileio/blockio devices, will
drop those since we only support block io now.
Thank you!
> Thanks,
>
> Bart.
