Re: [PATCH v9 21/25] block/rnbd: server: functionality for IO submission to file or block dev

[Danil Kipnis <danil.kipnis@cloud.ionos.com>]

On Sun, Mar 1, 2020 at 4:09 AM Bart Van Assche <bvanassche@acm.org> 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.