[PATCH 0/2] block: add bio based read-write helper

[PATCH 0/2] block: add bio based read-write helper

[Chaitanya Kulkarni]

Hi,

With reference to the discussion [1] on the linux-block mailing list,
this patch series adds a helper to map the buffer to bio and provides
two variants for synchronous and asynchronous I/O submission.

Right now only XFS is the only user, once gets accepted block/rnbd [2]
code can re-use this helper and we can avoid code duplication. 

Please note that I've not tested the XFS patch yet, once I get some
feedback, I'll add a test for the same in the xfstests. It will be
great if XFS developers can provide some insight into testing log
related code which used this helper.

Regards,
Chaitanya

[1] Linux-Block Mailing list discussion on the same topic :-

https://www.spinics.net/lists/linux-block/msg51040.html
https://www.spinics.net/lists/linux-block/msg51462.html
https://www.spinics.net/lists/linux-block/msg51465.html
https://www.spinics.net/lists/linux-block/msg51480.html

Chaitanya Kulkarni (2):
  block: add bio based rw helper for data buffer
  xfs: use block layer helper for rw

 block/blk-lib.c        | 105 +++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_bio_io.c    |  47 +-----------------
 include/linux/blkdev.h |   7 +++
 3 files changed, 114 insertions(+), 45 deletions(-)

-- 
2.22.1

[PATCH 1/2] block: add bio based rw helper for data buffer

[Chaitanya Kulkarni]

One of the common application for the file systems and drivers to map
the buffer to the bio and issue I/Os on the block device.

This is a prep patch which adds two helper functions for block layer
which allows file-systems and the drivers to map data buffer on to the
bios and issue bios synchronously using blkdev_issue_rw() or
asynchronously using __blkdev_issue_rw().

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 block/blk-lib.c        | 105 +++++++++++++++++++++++++++++++++++++++++
 include/linux/blkdev.h |   7 +++
 2 files changed, 112 insertions(+)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index 5f2c429d4378..451c367fc0d6 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -405,3 +405,108 @@ int blkdev_issue_zeroout(struct block_device *bdev, sector_t sector,
 	return ret;
 }
 EXPORT_SYMBOL(blkdev_issue_zeroout);
+
+/**
+ * __blkdev_ssue_rw - issue read/write bios from buffer asynchronously
+ * @bdev:	blockdev to read/write
+ * @buf:	data buffer
+ * @sector:	start sector
+ * @nr_sects:	number of sectors
+ * @op:	REQ_OP_READ or REQ_OP_WRITE
+ * @opf:	flags
+ * @gfp_mask:	memory allocation flags (for bio_alloc)
+ * @biop:	pointer to anchor bio
+ *
+ * Description:
+ *  Generic helper function to map data buffer into bios for read and write ops.
+ *  Returns pointer to the anchored last bio for caller to submit asynchronously
+ *  or synchronously.
+ */
+int __blkdev_issue_rw(struct block_device *bdev, char *buf, sector_t sector,
+		      sector_t nr_sects, unsigned op, unsigned opf,
+		      gfp_t gfp_mask, struct bio **biop)
+{
+	bool vm = is_vmalloc_addr(buf) ? true : false;
+	struct bio *bio = *biop;
+	unsigned int nr_pages;
+	struct page *page;
+	unsigned int off;
+	unsigned int len;
+	int bi_size;
+
+	if (!bdev_get_queue(bdev))
+		return -ENXIO;
+
+	if (bdev_read_only(bdev))
+		return -EPERM;
+
+	if (!(op == REQ_OP_READ || op == REQ_OP_WRITE))
+		return -EINVAL;
+
+	while (nr_sects != 0) {
+		nr_pages = __blkdev_sectors_to_bio_pages(nr_sects);
+
+		bio = blk_next_bio(bio, nr_pages, gfp_mask);
+		bio->bi_iter.bi_sector = sector;
+		bio_set_dev(bio, bdev);
+		bio_set_op_attrs(bio, op, 0);
+
+		while (nr_sects != 0) {
+			off = offset_in_page(buf);
+			page = vm ? vmalloc_to_page(buf) : virt_to_page(buf);
+			len = min((sector_t) PAGE_SIZE, nr_sects << 9);
+
+			bi_size = bio_add_page(bio, page, len, off);
+
+			nr_sects -= bi_size >> 9;
+			sector += bi_size >> 9;
+			buf += bi_size;
+
+			if (bi_size < len)
+				break;
+		}
+		cond_resched();
+	}
+	*biop = bio;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(__blkdev_issue_rw);
+
+/**
+ * blkdev_execute_rw_sync - issue read/write bios from buffer synchronously
+ * @bdev:	blockdev to read/write
+ * @buf:	data buffer
+ * @sector:	start sector
+ * @count:	number of bytes
+ * @op:	REQ_OP_READ or REQ_OP_WRITE
+ * @opf:	flags
+ * @gfp_mask:	memory allocation flags (for bio_alloc)
+ *
+ * Description:
+ *  Generic helper function to map data buffer buffer into bios for read and
+ *  write requests.
+ */
+int blkdev_issue_rw(struct block_device *b, char *buf, sector_t sector,
+		     unsigned count, unsigned op, unsigned opf, gfp_t mask)
+{
+	unsigned int is_vmalloc = is_vmalloc_addr(buf);
+	sector_t nr_sects = count >> 9;
+	struct bio *bio = NULL;
+	int error;
+
+	if (is_vmalloc && op == REQ_OP_WRITE)
+		flush_kernel_vmap_range(buf, count);
+
+	opf |= REQ_SYNC;
+	error = __blkdev_issue_rw(b, buf, sector, nr_sects, op, opf, mask, &bio);
+	if (!error && bio) {
+		error = submit_bio_wait(bio);
+		bio_put(bio);
+	}
+
+	if (is_vmalloc && op == REQ_OP_READ)
+		invalidate_kernel_vmap_range(buf, count);
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(blkdev_issue_rw);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 32868fbedc9e..cb315b301ad9 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1248,6 +1248,13 @@ static inline int sb_issue_zeroout(struct super_block *sb, sector_t block,
 				    gfp_mask, 0);
 }
 
+extern int __blkdev_issue_rw(struct block_device *bdev, char *buf,
+			     sector_t sector, sector_t nr_sects, unsigned op,
+			     unsigned opf, gfp_t gfp_mask, struct bio **biop);
+extern int blkdev_issue_rw(struct block_device *b, char *buf, sector_t sector,
+			   unsigned count, unsigned op, unsigned opf,
+			   gfp_t mask);
+
 extern int blk_verify_command(unsigned char *cmd, fmode_t mode);
 
 enum blk_default_limits {
-- 
2.22.1

[PATCH 2/2] xfs: use block layer helper for rw

[Chaitanya Kulkarni]

The existing routine xfs_rw_bdev has block layer bio-based code which
maps the data buffer allocated with kmalloc or vmalloc to the READ/WRITE
bios. Use a block layer helper from the previous patch to avoid code
duplication.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 fs/xfs/xfs_bio_io.c | 47 ++-------------------------------------------
 1 file changed, 2 insertions(+), 45 deletions(-)

diff --git a/fs/xfs/xfs_bio_io.c b/fs/xfs/xfs_bio_io.c
index e2148f2d5d6b..b04c398fb99c 100644
--- a/fs/xfs/xfs_bio_io.c
+++ b/fs/xfs/xfs_bio_io.c
@@ -4,11 +4,6 @@
  */
 #include "xfs.h"
 
-static inline unsigned int bio_max_vecs(unsigned int count)
-{
-	return min_t(unsigned, howmany(count, PAGE_SIZE), BIO_MAX_PAGES);
-}
-
 int
 xfs_rw_bdev(
 	struct block_device	*bdev,
@@ -18,44 +13,6 @@ xfs_rw_bdev(
 	unsigned int		op)
 
 {
-	unsigned int		is_vmalloc = is_vmalloc_addr(data);
-	unsigned int		left = count;
-	int			error;
-	struct bio		*bio;
-
-	if (is_vmalloc && op == REQ_OP_WRITE)
-		flush_kernel_vmap_range(data, count);
-
-	bio = bio_alloc(GFP_KERNEL, bio_max_vecs(left));
-	bio_set_dev(bio, bdev);
-	bio->bi_iter.bi_sector = sector;
-	bio->bi_opf = op | REQ_META | REQ_SYNC;
-
-	do {
-		struct page	*page = kmem_to_page(data);
-		unsigned int	off = offset_in_page(data);
-		unsigned int	len = min_t(unsigned, left, PAGE_SIZE - off);
-
-		while (bio_add_page(bio, page, len, off) != len) {
-			struct bio	*prev = bio;
-
-			bio = bio_alloc(GFP_KERNEL, bio_max_vecs(left));
-			bio_copy_dev(bio, prev);
-			bio->bi_iter.bi_sector = bio_end_sector(prev);
-			bio->bi_opf = prev->bi_opf;
-			bio_chain(prev, bio);
-
-			submit_bio(prev);
-		}
-
-		data += len;
-		left -= len;
-	} while (left > 0);
-
-	error = submit_bio_wait(bio);
-	bio_put(bio);
-
-	if (is_vmalloc && op == REQ_OP_READ)
-		invalidate_kernel_vmap_range(data, count);
-	return error;
+	return blkdev_issue_rw(bdev, data, sector, count, op, REQ_META,
+			       GFP_KERNEL);
 }
-- 
2.22.1

Re: [PATCH 1/2] block: add bio based rw helper for data buffer

[Damien Le Moal]

On 2020/04/07 8:24, Chaitanya Kulkarni wrote:
> One of the common application for the file systems and drivers to map
> the buffer to the bio and issue I/Os on the block device.

Drivers generally do not do this. At least not lower ones (scsi, nvme, etc). I
guess you are referring to DM drivers. If yes, better be clear about this.
Also, "the buffer" is a little unclear. Better qualify that.

Another thing: "to map the buffer to the bio" is a little strange. The reverse
seems more natural: a bio maps a buffer.

> 
> This is a prep patch which adds two helper functions for block layer
> which allows file-systems and the drivers to map data buffer on to the
> bios and issue bios synchronously using blkdev_issue_rw() or
> asynchronously using __blkdev_issue_rw().

The function names are basically the same but do completely different things.
Better rename that. May be blkdev_issue_rw_sync() and blkdev_issue_rw_async() ?

> 
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> ---
>  block/blk-lib.c        | 105 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/blkdev.h |   7 +++
>  2 files changed, 112 insertions(+)
> 
> diff --git a/block/blk-lib.c b/block/blk-lib.c
> index 5f2c429d4378..451c367fc0d6 100644
> --- a/block/blk-lib.c
> +++ b/block/blk-lib.c
> @@ -405,3 +405,108 @@ int blkdev_issue_zeroout(struct block_device *bdev, sector_t sector,
>  	return ret;
>  }
>  EXPORT_SYMBOL(blkdev_issue_zeroout);
> +
> +/**
> + * __blkdev_ssue_rw - issue read/write bios from buffer asynchronously

s/__blkdev_ssue_rw/__blkdev_issue_rw

> + * @bdev:	blockdev to read/write
> + * @buf:	data buffer
> + * @sector:	start sector
> + * @nr_sects:	number of sectors
> + * @op:	REQ_OP_READ or REQ_OP_WRITE
> + * @opf:	flags
> + * @gfp_mask:	memory allocation flags (for bio_alloc)
> + * @biop:	pointer to anchor bio
> + *
> + * Description:
> + *  Generic helper function to map data buffer into bios for read and write ops.
> + *  Returns pointer to the anchored last bio for caller to submit asynchronously
> + *  or synchronously.
> + */
> +int __blkdev_issue_rw(struct block_device *bdev, char *buf, sector_t sector,
> +		      sector_t nr_sects, unsigned op, unsigned opf,
> +		      gfp_t gfp_mask, struct bio **biop)
> +{
> +	bool vm = is_vmalloc_addr(buf) ? true : false;

You do not need the clunky "? true : false" here. is_vmalloc_addr() returns a
bool already.

> +	struct bio *bio = *biop;
> +	unsigned int nr_pages;
> +	struct page *page;
> +	unsigned int off;
> +	unsigned int len;
> +	int bi_size;
> +
> +	if (!bdev_get_queue(bdev))
> +		return -ENXIO;
> +
> +	if (bdev_read_only(bdev))
> +		return -EPERM;

One can read a read-only device. So the check here is not correct. You need a
"&& op == REQ_OP_WRITE" in the condition.

> +
> +	if (!(op == REQ_OP_READ || op == REQ_OP_WRITE))
> +		return -EINVAL;

This probably should be checked before read-only.

> +
> +	while (nr_sects != 0) {
> +		nr_pages = __blkdev_sectors_to_bio_pages(nr_sects);
> +
> +		bio = blk_next_bio(bio, nr_pages, gfp_mask);
> +		bio->bi_iter.bi_sector = sector;
> +		bio_set_dev(bio, bdev);
> +		bio_set_op_attrs(bio, op, 0);
> +
> +		while (nr_sects != 0) {
> +			off = offset_in_page(buf);
> +			page = vm ? vmalloc_to_page(buf) : virt_to_page(buf);
> +			len = min((sector_t) PAGE_SIZE, nr_sects << 9);
> +
> +			bi_size = bio_add_page(bio, page, len, off);

The variable naming is super confusing. bio_add_page() returns 0 if nothing is
added and len if the page was added. So bi_size as a var name is not the best of
name in my opinion.

> +
> +			nr_sects -= bi_size >> 9;
> +			sector += bi_size >> 9;
> +			buf += bi_size;
> +
> +			if (bi_size < len)
> +				break;

You will get either 0 or len from bio_add_page. So the check here is not ideal.
I think it is better to move it up under bio_add_page() and make it:

			len = bioa_add_page(bio, page, len, off);
			if (!len)
				break;

> +		}
> +		cond_resched();
> +	}
> +	*biop = bio;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(__blkdev_issue_rw);
> +
> +/**
> + * blkdev_execute_rw_sync - issue read/write bios from buffer synchronously
> + * @bdev:	blockdev to read/write
> + * @buf:	data buffer
> + * @sector:	start sector
> + * @count:	number of bytes
> + * @op:	REQ_OP_READ or REQ_OP_WRITE
> + * @opf:	flags
> + * @gfp_mask:	memory allocation flags (for bio_alloc)
> + *
> + * Description:
> + *  Generic helper function to map data buffer buffer into bios for read and
> + *  write requests.
> + */
> +int blkdev_issue_rw(struct block_device *b, char *buf, sector_t sector,
> +		     unsigned count, unsigned op, unsigned opf, gfp_t mask)

function name differs between description and declaration.
blkdev_execute_rw_sync() is better than blkdev_issue_rw() in my opinion.

> +{
> +	unsigned int is_vmalloc = is_vmalloc_addr(buf);
> +	sector_t nr_sects = count >> 9;
> +	struct bio *bio = NULL;
> +	int error;
> +
> +	if (is_vmalloc && op == REQ_OP_WRITE)
> +		flush_kernel_vmap_range(buf, count);
> +
> +	opf |= REQ_SYNC;

You can add this directly in the call below.

> +	error = __blkdev_issue_rw(b, buf, sector, nr_sects, op, opf, mask, &bio);
> +	if (!error && bio) {
> +		error = submit_bio_wait(bio);
> +		bio_put(bio);
> +	}
> +
> +	if (is_vmalloc && op == REQ_OP_READ)
> +		invalidate_kernel_vmap_range(buf, count);
> +
> +	return error;
> +}
> +EXPORT_SYMBOL_GPL(blkdev_issue_rw);
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 32868fbedc9e..cb315b301ad9 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -1248,6 +1248,13 @@ static inline int sb_issue_zeroout(struct super_block *sb, sector_t block,
>  				    gfp_mask, 0);
>  }
>  
> +extern int __blkdev_issue_rw(struct block_device *bdev, char *buf,
> +			     sector_t sector, sector_t nr_sects, unsigned op,
> +			     unsigned opf, gfp_t gfp_mask, struct bio **biop);
> +extern int blkdev_issue_rw(struct block_device *b, char *buf, sector_t sector,
> +			   unsigned count, unsigned op, unsigned opf,
> +			   gfp_t mask);

No need for the extern I think.

> +
>  extern int blk_verify_command(unsigned char *cmd, fmode_t mode);
>  
>  enum blk_default_limits {
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/2] xfs: use block layer helper for rw

[Damien Le Moal]

On 2020/04/07 8:25, Chaitanya Kulkarni wrote:
> The existing routine xfs_rw_bdev has block layer bio-based code which
> maps the data buffer allocated with kmalloc or vmalloc to the READ/WRITE
> bios. Use a block layer helper from the previous patch to avoid code
> duplication.
> 
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> ---
>  fs/xfs/xfs_bio_io.c | 47 ++-------------------------------------------
>  1 file changed, 2 insertions(+), 45 deletions(-)
> 
> diff --git a/fs/xfs/xfs_bio_io.c b/fs/xfs/xfs_bio_io.c
> index e2148f2d5d6b..b04c398fb99c 100644
> --- a/fs/xfs/xfs_bio_io.c
> +++ b/fs/xfs/xfs_bio_io.c
> @@ -4,11 +4,6 @@
>   */
>  #include "xfs.h"
>  
> -static inline unsigned int bio_max_vecs(unsigned int count)
> -{
> -	return min_t(unsigned, howmany(count, PAGE_SIZE), BIO_MAX_PAGES);
> -}
> -
>  int
>  xfs_rw_bdev(
>  	struct block_device	*bdev,
> @@ -18,44 +13,6 @@ xfs_rw_bdev(
>  	unsigned int		op)
>  
>  {
> -	unsigned int		is_vmalloc = is_vmalloc_addr(data);
> -	unsigned int		left = count;
> -	int			error;
> -	struct bio		*bio;
> -
> -	if (is_vmalloc && op == REQ_OP_WRITE)
> -		flush_kernel_vmap_range(data, count);
> -
> -	bio = bio_alloc(GFP_KERNEL, bio_max_vecs(left));
> -	bio_set_dev(bio, bdev);
> -	bio->bi_iter.bi_sector = sector;
> -	bio->bi_opf = op | REQ_META | REQ_SYNC;
> -
> -	do {
> -		struct page	*page = kmem_to_page(data);
> -		unsigned int	off = offset_in_page(data);
> -		unsigned int	len = min_t(unsigned, left, PAGE_SIZE - off);
> -
> -		while (bio_add_page(bio, page, len, off) != len) {
> -			struct bio	*prev = bio;
> -
> -			bio = bio_alloc(GFP_KERNEL, bio_max_vecs(left));
> -			bio_copy_dev(bio, prev);
> -			bio->bi_iter.bi_sector = bio_end_sector(prev);
> -			bio->bi_opf = prev->bi_opf;
> -			bio_chain(prev, bio);
> -
> -			submit_bio(prev);
> -		}
> -
> -		data += len;
> -		left -= len;
> -	} while (left > 0);

Your helper could use a similar loop structure. This is very easy to read.

> -
> -	error = submit_bio_wait(bio);
> -	bio_put(bio);
> -
> -	if (is_vmalloc && op == REQ_OP_READ)
> -		invalidate_kernel_vmap_range(data, count);
> -	return error;
> +	return blkdev_issue_rw(bdev, data, sector, count, op, REQ_META,
> +			       GFP_KERNEL);
>  }
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/2] block: add bio based rw helper for data buffer

[Danil Kipnis]

In __blkdev_issue_rw():
> +               bio = blk_next_bio(bio, nr_pages, gfp_mask);

Doesn't this already submits the bio even before the pages are added?

> +       error = __blkdev_issue_rw(b, buf, sector, nr_sects, op, opf, mask, &bio);
> +       if (!error && bio) {
> +               error = submit_bio_wait(bio);

And then the bio is submitted again in blkdev_issue_rw()...

Or do I understand it wrong?

Re: [PATCH 1/2] block: add bio based rw helper for data buffer

[Chaitanya Kulkarni]

On 04/06/2020 05:28 PM, Damien Le Moal wrote:
> On 2020/04/07 8:24, Chaitanya Kulkarni wrote:
>> One of the common application for the file systems and drivers to map
>> the buffer to the bio and issue I/Os on the block device.
>
> Drivers generally do not do this. At least not lower ones (scsi, nvme, etc). I
> guess you are referring to DM drivers. If yes, better be clear about this.
> Also, "the buffer" is a little unclear. Better qualify that.
>
Agree most drivers deals with sg_list. Right now there is only one such
an example I know which is rnbd as driver as it is mentioned in the
cover-letter references.
> Another thing: "to map the buffer to the bio" is a little strange. The reverse
> seems more natural: a bio maps a buffer.
>
Make sense, will do.
>>
>> This is a prep patch which adds two helper functions for block layer
>> which allows file-systems and the drivers to map data buffer on to the
>> bios and issue bios synchronously using blkdev_issue_rw() or
>> asynchronously using __blkdev_issue_rw().
>
> The function names are basically the same but do completely different things.
> Better rename that. May be blkdev_issue_rw_sync() and blkdev_issue_rw_async() ?
>
 >

This is exactly I named functions to start with (see the function
documentation which I missed to update), but the pattern in blk-lib.c
uses no such sync and async postfix, which is used is sync and async
context all over the kernel :-

# grep EXPORT block/blk-lib.c
EXPORT_SYMBOL(__blkdev_issue_discard);
EXPORT_SYMBOL(blkdev_issue_discard);, since as it is nr_sects and sector 
calculation doesn't matter after break.
EXPORT_SYMBOL(blkdev_issue_write_same);
EXPORT_SYMBOL(__blkdev_issue_zeroout);
EXPORT_SYMBOL(blkdev_issue_zeroout);
EXPORT_SYMBOL_GPL(__blkdev_issue_rw);
EXPORT_SYMBOL_GPL(blkdev_issue_rw);

In absence of documentation for naming how about we just follow
existing naming convention ?

Which matches the pattern for new API.

>>
>> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
>> ---
>>   block/blk-lib.c        | 105 +++++++++++++++++++++++++++++++++++++++++
>>   include/linux/blkdev.h |   7 +++
>>   2 files changed, 112 insertions(+)
>>
>> diff --git a/block/blk-lib.c b/block/blk-lib.c
>> index 5f2c429d4378..451c367fc0d6 100644
>> --- a/block/blk-lib.c
>> +++ b/block/blk-lib.c
>> @@ -405,3 +405,108 @@ int blkdev_issue_zeroout(struct block_device *bdev, sector_t sector,
>>   	return ret;
>>   }
>>   EXPORT_SYMBOL(blkdev_issue_zeroout);
>> +
>> +/**
>> + * __blkdev_ssue_rw - issue read/write bios from buffer asynchronously
>
> s/__blkdev_ssue_rw/__blkdev_issue_rw
Thanks, will fix.
>
>> + * @bdev:	blockdev to read/write
>> + * @buf:	data buffer
>> + * @sector:	start sector
>> + * @nr_sects:	number of sectors
>> + * @op:	REQ_OP_READ or REQ_OP_WRITE
>> + * @opf:	flags
>> + * @gfp_mask:	memory allocation flags (for bio_alloc)
>> + * @biop:	pointer to anchor bio
>> + *
>> + * Description:
>> + *  Generic helper function to map data buffer into bios for read and write ops.
>> + *  Returns pointer to the anchored last bio for caller to submit asynchronously
>> + *  or synchronously.
>> + */
>> +int __blkdev_issue_rw(struct block_device *bdev, char *buf, sector_t sector,
>> +		      sector_t nr_sects, unsigned op, unsigned opf,
>> +		      gfp_t gfp_mask, struct bio **biop)
>> +{
>> +	bool vm = is_vmalloc_addr(buf) ? true : false;
>
> You do not need the clunky "? true : false" here. is_vmalloc_addr() returns a
> bool already.
>
I will remove it.
>> +	struct bio *bio = *biop;
>> +	unsigned int nr_pages;
>> +	struct page *page;
>> +	unsigned int off;
>> +	unsigned int len;
>> +	int bi_size;
>> +
>> +	if (!bdev_get_queue(bdev))
>> +		return -ENXIO;
>> +
>> +	if (bdev_read_only(bdev))
>> +		return -EPERM;
>
> One can read a read-only device. So the check here is not correct. You need a
> "&& op == REQ_OP_WRITE" in the condition.
>
True, this shouldn't be here, also I'm thinking of lifting these checks 
to caller, we can add it later if needed.
>> +
>> +	if (!(op == REQ_OP_READ || op == REQ_OP_WRITE))
>> +		return -EINVAL;
>
> This probably should be checked before read-only.
>
Yes.
>> +
>> +	while (nr_sects != 0) {
>> +		nr_pages = __blkdev_sectors_to_bio_pages(nr_sects);
>> +
>> +		bio = blk_next_bio(bio, nr_pages, gfp_mask);
>> +		bio->bi_iter.bi_sector = sector;
>> +		bio_set_dev(bio, bdev);
>> +		bio_set_op_attrs(bio, op, 0);
>> +
>> +		while (nr_sects != 0) {
>> +			off = offset_in_page(buf);
>> +			page = vm ? vmalloc_to_page(buf) : virt_to_page(buf);
>> +			len = min((sector_t) PAGE_SIZE, nr_sects << 9);
>> +
>> +			bi_size = bio_add_page(bio, page, len, off);
>
> The variable naming is super confusing. bio_add_page() returns 0 if nothing is
> added and len if the page was added. So bi_size as a var name is not the best of
> name in my opinion.
>
Here bio, page, len, off variables are passed to the bio_add_page() 
function, which has the same names in the function parameter list.
(I'll fix the off to offset)

Regarding the bi_size, given that bio_add_page() returns 
bio->bi_iter.bi_size, isn't bi_size also maps to the what function is 
returning and explains what we are dealing with?

Also, bi_size is used in the blk-lib.c: __blkdev_issue_zero_pages(), if 
that is confusing then we need to change that, what is your preference?

I'll send a cleanup patch __blkdev_issue_zero_pages() also.

>> +
>> +			nr_sects -= bi_size >> 9;
>> +			sector += bi_size >> 9;
>> +			buf += bi_size;
>> +
>> +			if (bi_size < len)
>> +				break;
>
> You will get either 0 or len from bio_add_page. So the check here is not ideal.
> I think it is better to move it up under bio_add_page() and make it:
>
> 			len = bioa_add_page(bio, page, len, off);
> 			if (!len)
> 				break;
I'd still like to keep bi_size, I think I had received a comment about 
using same variable for function call and updating as a return value.

Regarding the check, will move it.
>
>> +		}
>> +		cond_resched();
>> +	}
>> +	*biop = bio;
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(__blkdev_issue_rw);
>> +
>> +/**
>> + * blkdev_execute_rw_sync - issue read/write bios from buffer synchronously
>> + * @bdev:	blockdev to read/write
>> + * @buf:	data buffer
>> + * @sector:	start sector
>> + * @count:	number of bytes
>> + * @op:	REQ_OP_READ or REQ_OP_WRITE
>> + * @opf:	flags
>> + * @gfp_mask:	memory allocation flags (for bio_alloc)
>> + *
>> + * Description:
>> + *  Generic helper function to map data buffer buffer into bios for read and
>> + *  write requests.
>> + */
>> +int blkdev_issue_rw(struct block_device *b, char *buf, sector_t sector,
>> +		     unsigned count, unsigned op, unsigned opf, gfp_t mask)
>
> function name differs between description and declaration.
> blkdev_execute_rw_sync() is better than blkdev_issue_rw() in my opinion.
>
That was remained from initial version, will fix it, thanks for
pointing it out.
>> +{
>> +	unsigned int is_vmalloc = is_vmalloc_addr(buf);
>> +	sector_t nr_sects = count >> 9;
>> +	struct bio *bio = NULL;
>> +	int error;
>> +
>> +	if (is_vmalloc && op == REQ_OP_WRITE)
>> +		flush_kernel_vmap_range(buf, count);
>> +
>> +	opf |= REQ_SYNC;
>
> You can add this directly in the call below.
>
Breaks calling of __blkdev_issue_rw() to new line. This also adds a new
line which is unavoidable but keeps the function call smooth in one
line.
>> +	error = __blkdev_issue_rw(b, buf, sector, nr_sects, op, opf, mask, &bio);
>> +	if (!error && bio) {
>> +		error = submit_bio_wait(bio);
>> +		bio_put(bio);
>> +	}
>> +
>> +	if (is_vmalloc && op == REQ_OP_READ)
>> +		invalidate_kernel_vmap_range(buf, count);
>> +
>> +	return error;
>> +}
>> +EXPORT_SYMBOL_GPL(blkdev_issue_rw);
>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>> index 32868fbedc9e..cb315b301ad9 100644
>> --- a/include/linux/blkdev.h
>> +++ b/include/linux/blkdev.h
>> @@ -1248,6 +1248,13 @@ static inline int sb_issue_zeroout(struct super_block *sb, sector_t block,
>>   				    gfp_mask, 0);
>>   }
>>
>> +extern int __blkdev_issue_rw(struct block_device *bdev, char *buf,
>> +			     sector_t sector, sector_t nr_sects, unsigned op,
>> +			     unsigned opf, gfp_t gfp_mask, struct bio **biop);
>> +extern int blkdev_issue_rw(struct block_device *b, char *buf, sector_t sector,
>> +			   unsigned count, unsigned op, unsigned opf,
>> +			   gfp_t mask);
>
> No need for the extern I think.
>
Again, following the exiting pattern in the file for the header:-

  # grep blkdev_issue include/linux/blkdev.h
extern int blkdev_issue_flush(struct block_device *, gfp_t, sector_t *);

extern int blkdev_issue_write_same(struct block_device *bdev, sector_t
sector,

extern int blkdev_issue_discard(struct block_device *bdev, sector_t
sector,

extern int __blkdev_issue_discard(struct block_device *bdev, sector_t
sector,

extern int __blkdev_issue_zeroout(struct block_device *bdev, sector_t
sector,

extern int blkdev_issue_zeroout(struct block_device *bdev, sector_t
sector,

extern int __blkdev_issue_rw(struct block_device *bdev, char *buf,

extern int blkdev_issue_rw(struct block_device *b, char *buf, sector_t
sector,
>> +
>>   extern int blk_verify_command(unsigned char *cmd, fmode_t mode);
>>
>>   enum blk_default_limits {
>>
>
>

Re: [PATCH 2/2] xfs: use block layer helper for rw

[Chaitanya Kulkarni]

On 04/06/2020 05:32 PM, Damien Le Moal wrote:
>> -
>> >-	do {
>> >-		struct page	*page = kmem_to_page(data);
>> >-		unsigned int	off = offset_in_page(data);
>> >-		unsigned int	len = min_t(unsigned, left, PAGE_SIZE - off);
>> >-
>> >-		while (bio_add_page(bio, page, len, off) != len) {
>> >-			struct bio	*prev = bio;
>> >-
>> >-			bio = bio_alloc(GFP_KERNEL, bio_max_vecs(left));
>> >-			bio_copy_dev(bio, prev);
>> >-			bio->bi_iter.bi_sector = bio_end_sector(prev);
>> >-			bio->bi_opf = prev->bi_opf;
>> >-			bio_chain(prev, bio);
>> >-
>> >-			submit_bio(prev);
>> >-		}
>> >-
>> >-		data += len;
>> >-		left -= len;
>> >-	} while (left > 0);
> Your helper could use a similar loop structure. This is very easy to read.
>
If I understand correctly this pattern is used since it is not a part of 
block layer.

The helpers in blk-lib.c are not accessible so this :-
1. Adds an extra helper bio_max_vecs().
2. Adds a new macro howmany which is XFS specific and doesn't
    follow usual block layer macros naming.
3. Open codes bio chaining.
4. Uses two bio variables for chaining.
5. Doesn't allow to anchor bio which is needed in async I/O scenario.

All above breaks the existing pattern and code reuse in blk-lib.c, since 
blk-lib.c:-
1. Already provides blk_next_bio() why repeat the bio allocation
    and bio chaining code ?
2. Instead of adding a new helper bio_max_vecs() why not use existing
     __blkdev_sectors_to_bio_pages() ?
3. Why use two bios when it can be done with one bio with the helpers
    in blk-lib.c ?
4. Allows async version.

Re: [PATCH 2/2] xfs: use block layer helper for rw

[Dave Chinner]

On Tue, Apr 07, 2020 at 08:06:35PM +0000, Chaitanya Kulkarni wrote:
> On 04/06/2020 05:32 PM, Damien Le Moal wrote:
> >> -
> >> >-	do {
> >> >-		struct page	*page = kmem_to_page(data);
> >> >-		unsigned int	off = offset_in_page(data);
> >> >-		unsigned int	len = min_t(unsigned, left, PAGE_SIZE - off);
> >> >-
> >> >-		while (bio_add_page(bio, page, len, off) != len) {
> >> >-			struct bio	*prev = bio;
> >> >-
> >> >-			bio = bio_alloc(GFP_KERNEL, bio_max_vecs(left));
> >> >-			bio_copy_dev(bio, prev);
> >> >-			bio->bi_iter.bi_sector = bio_end_sector(prev);
> >> >-			bio->bi_opf = prev->bi_opf;
> >> >-			bio_chain(prev, bio);
> >> >-
> >> >-			submit_bio(prev);
> >> >-		}
> >> >-
> >> >-		data += len;
> >> >-		left -= len;
> >> >-	} while (left > 0);
> > Your helper could use a similar loop structure. This is very easy to read.
> >
> If I understand correctly this pattern is used since it is not a part of 
> block layer.

It's because it was simple and easy to understandi, not because of
the fact it is outside the core block layer.

Us XFS folks are simple people who like simple things that are easy to
understand because there is so much of XFS that is so horrifically
complex that we want to implement simple stuff once and just not
have to care about it again.

> The helpers in blk-lib.c are not accessible so this :-

So export the helpers?

> All above breaks the existing pattern and code reuse in blk-lib.c, since 
> blk-lib.c:-
> 1. Already provides blk_next_bio() why repeat the bio allocation
>     and bio chaining code ?

So export the helper?

> 2. Instead of adding a new helper bio_max_vecs() why not use existing
>      __blkdev_sectors_to_bio_pages() ?

That's not an improvement. The XFS code is _much_ easier to read
and understand.

> 3. Why use two bios when it can be done with one bio with the helpers
>     in blk-lib.c ?

That helper is blk_next_bio(), which hides the second bio inside it.
So you aren't actually getting rid of the need for two bio pointers.

> 4. Allows async version.

Which is not used by XFS, so just adds complexity to this XFS path
for no good reason.

Seriously, this 20 lines of code in XFS turns into 50-60 lines
of code in the block layer to do the same thing. How is that an
improvement?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] xfs: use block layer helper for rw

[Chaitanya Kulkarni]

(+cc Christoph)

On 04/07/2020 04:28 PM, Dave Chinner wrote:
> On Tue, Apr 07, 2020 at 08:06:35PM +0000, Chaitanya Kulkarni wrote:
>> On 04/06/2020 05:32 PM, Damien Le Moal wrote:
>>>> -
>>>>> -	do {
>>>>> -		struct page	*page = kmem_to_page(data);
>>>>> -		unsigned int	off = offset_in_page(data);
>>>>> -		unsigned int	len = min_t(unsigned, left, PAGE_SIZE - off);
>>>>> -
>>>>> -		while (bio_add_page(bio, page, len, off) != len) {
>>>>> -			struct bio	*prev = bio;
>>>>> -
>>>>> -			bio = bio_alloc(GFP_KERNEL, bio_max_vecs(left));
>>>>> -			bio_copy_dev(bio, prev);
>>>>> -			bio->bi_iter.bi_sector = bio_end_sector(prev);
>>>>> -			bio->bi_opf = prev->bi_opf;
>>>>> -			bio_chain(prev, bio);
>>>>> -
>>>>> -			submit_bio(prev);
>>>>> -		}
>>>>> -
>>>>> -		data += len;
>>>>> -		left -= len;
>>>>> -	} while (left > 0);
>>> Your helper could use a similar loop structure. This is very easy to read.
>>>
>> If I understand correctly this pattern is used since it is not a part of
>> block layer.
>
> It's because it was simple and easy to understandi, not because of
> the fact it is outside the core block layer.
>
> Us XFS folks are simple people who like simple things that are easy to
> understand because there is so much of XFS that is so horrifically
> complex that we want to implement simple stuff once and just not
> have to care about it again.
>

Agree, new code should not make things complicated, in fact initial 
version had existing code pattern see [1].

Before answering the questions I think I should share few
considerations I went through based on the code and requirement :-

1. The new helper is needed since XFS and rnbd is doing the same thing,
    i.e. mapping bios to buffer. The helper should be a part of the
    block layer library function, as most of the library APIs are
    present there, which are used by the file systems/network drivers.
    e.g. blkdev_issue_zeroout().
    Summery :- Something like blkdev_issue_zeroout() is a good
               candidate since it is used in file-systems and network
               drivers.

2. Figure out the common code which can be moved to the library and
    identify what different APIs are needed for the functionality to
    save code duplication :-
    A. Synchronous version is needed for XFS. (without cond_resched() I
       need to fix this in next version).
    B. Asynchronous version is needed for network drivers rnbd as
       mentioned in the cover letter.
    Summery :- Add sync and async version.

3. Whenever a new function is added to the library we should not
    invent new patterns and follow the same style which is present in
    the library code i.e. in blk-lib.c, unless new API can't fit into
    current pattern. This is needed since block layer maintainers
    might complain about new design pattern.
    Summary :- See if new API can be designed identical to
               blkdev_issue_zeroout() and __blkdev_issue_zero_pages().

4. Look at the existing user(s) (XFS) of the library (blk-lib.c)
    API(zeroout) and how it is using existing API if any. For XFS,
    which uses existing library function :-
     "fs/xfs/xfs_bmap_util.c:65: blkdev_issue_zeroout(target->bt_bdev"
    Summery :- Since the  library(blk-lib.c) API(zeroout) is used in
               the user(XFS) that means it is acceptable to use similar
               code pattern.

5. Design a new API in the library based on the existing code. That :-
    A. Matches the code pattern which is present in the library, already
       used in the exiting user and reuse it.
    B. Make sure to have identical APIs parameter to keep the
       consistency whenever it is possible. (including APIs prototype
       parameters name etc).
    Summary :- Add blkdev_issue_rw() -> blkdev_issue_zeroout() and
                 __blkdev_issue_rw() -> __blkdev_issue_zero_pages().

Regarding the number of lines, this is not aimed at having less number
of lines in XFS only than existing xfs_rw_bdev() (although it does
that), but since it is a library API it will save overall
lines of the code (e.g. XFS 60 and rnbd ~50 and future uses) in the
users like fs and network drivers similar to existing zeroout API.

Please correct me if I'm wrong in any the above considerations, given
that I'm not a XFS developer I'd like to make sure new API pattern is
acceptable to both XFS developers and the block layer maintainers.

I'm not stuck on using this pattern and I'll be happy to re-use the
xfs_rw_bdev() pattern if block layer maintainers are okay with it.

I'll wait for them to reply.

>> The helpers in blk-lib.c are not accessible so this :-
>
> So export the helpers?
>
>> All above breaks the existing pattern and code reuse in blk-lib.c, since
>> blk-lib.c:-
>> 1. Already provides blk_next_bio() why repeat the bio allocation
>>      and bio chaining code ?
>
> So export the helper?
>
>> 2. Instead of adding a new helper bio_max_vecs() why not use existing
>>       __blkdev_sectors_to_bio_pages() ?
>
> That's not an improvement. The XFS code is _much_ easier to read
> and understand.
>
>> 3. Why use two bios when it can be done with one bio with the helpers
>>      in blk-lib.c ?
>
> That helper is blk_next_bio(), which hides the second bio inside it.
> So you aren't actually getting rid of the need for two bio pointers.
>
>> 4. Allows async version.
>
> Which is not used by XFS, so just adds complexity to this XFS path
> for no good reason.
>
> Seriously, this 20 lines of code in XFS turns into 50-60 lines
> of code in the block layer to do the same thing. How is that an
> improvement?
>
> Cheers,
>
> Dave.
>


[1] Initial draft (for reference only) :-
struct bio *__bio_execute_rw(struct block_device *bd, char *buf, 
sector_t sect,
                               unsigned count, unsigned op, unsigned opf,
                               gfp_t gfp_mask)
{
          bool vm = is_vmalloc_addr(buf) ? true : false;
          struct request_queue *q = bd->bd_queue;
          unsigned int left = count;
          struct page *page;
          struct bio *new_bio;
          blk_qc_t cookie;
          bool do_poll;

          new_bio = bio_alloc(gfp_mask, bio_max_vecs(left));
          bio_set_dev(new_bio, bd);
          new_bio->bi_iter.bi_sector = sect;
          new_bio->bi_opf = op_opf | REQ_SYNC;

          do {
                  unsigned int offset = offset_in_page(buf);
                  unsigned int len = min_t(unsigned, left, PAGE_SIZE - 
offset);

                  page =  vm ? vmalloc_to_page(buf) : virt_to_page(buf);
                  while (bio_add_page(new_bio, page, len, offset) != len) {
                          struct bio *curr_bio = new_bio;

                          new_bio = bio_alloc(gfp_mask, bio_max_vecs(left));
                          bio_copy_dev(new_bio, curr_bio);
                          new_bio->bi_iter.bi_sector = 
bio_end_sector(curr_bio);
                          new_bio->bi_opf = curr_bio->bi_opf;
                          bio_chain(curr_bio, new_bio);

                          cookie = submit_bio(curr_bio);
                  }

                  buf += len;
                  left -= len;
          } while (left > 0);

          return new_bio;
}
EXPORT_SYMBOL(__bio_execute_rw);

int bio_execute_rw_sync(struct block_device *bd, char *buf, sector_t sect,
                          unsigned count, unsigned op, unsigned opf,
                          gfp_t gfp_mask)
{
          unsigned int is_vmalloc = is_vmalloc_addr(buf);
          struct bio *bio;
          int error;

          if (is_vmalloc && op == REQ_OP_WRITE)
                  flush_kernel_vmap_range(buf, count);

          bio = __bio_execute_rw(bd, sect, count, buf, op, opf, mask);
          error = submit_bio_wait(bio);
          bio_put(bio);

          if (is_vmalloc && op == REQ_OP_READ)
                  invalidate_kernel_vmap_range(buf, count);

          return error;
}
EXPORT_SYMBOL_GPL(bio_execute_rw_sync);