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

From: Bart Van Assche <bvanassche@acm.org>
To: Jack Wang <jinpuwang@gmail.com>,
	linux-block@vger.kernel.org, linux-rdma@vger.kernel.org
Cc: axboe@kernel.dk, hch@infradead.org, sagi@grimberg.me,
	leon@kernel.org, dledford@redhat.com, jgg@ziepe.ca,
	danil.kipnis@cloud.ionos.com, jinpu.wang@cloud.ionos.com,
	rpenyaev@suse.de, pankaj.gupta@cloud.ionos.com
Subject: Re: [PATCH v9 21/25] block/rnbd: server: functionality for IO submission to file or block dev
Date: Sat, 29 Feb 2020 19:09:32 -0800	[thread overview]
Message-ID: <92721a50-158f-3018-39d4-40fce7b0f4d8@acm.org> (raw)
In-Reply-To: <20200221104721.350-22-jinpuwang@gmail.com>

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.

> +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.

Thanks,

Bart.