add more bio helper

add more bio helper

[Christoph Hellwig]

Hi all,

this series adds more block layer helpers to remove boilerplate code when
adding memory to a bio or to even do the entire synchronous I/O.

The main aim is to avoid having to convert to a struct page in the caller
when adding kernel direct mapping or vmalloc memory.

Diffstat:
 block/bio.c                   |   57 ++++++++++++++++++++++
 block/blk-map.c               |  108 ++++++++++++++++--------------------------
 drivers/block/pktcdvd.c       |    2 
 drivers/block/rnbd/rnbd-srv.c |    7 --
 drivers/block/ublk_drv.c      |    3 -
 drivers/block/virtio_blk.c    |    4 -
 drivers/md/bcache/super.c     |    3 -
 drivers/md/dm-bufio.c         |    2 
 drivers/md/dm-integrity.c     |   16 ++----
 drivers/nvme/host/core.c      |    2 
 drivers/scsi/scsi_ioctl.c     |    2 
 drivers/scsi/scsi_lib.c       |    3 -
 fs/btrfs/scrub.c              |   10 ---
 fs/gfs2/ops_fstype.c          |   24 +++------
 fs/hfsplus/wrapper.c          |   46 +++--------------
 fs/xfs/xfs_bio_io.c           |   30 ++++-------
 fs/xfs/xfs_buf.c              |   27 +++-------
 fs/zonefs/super.c             |   34 ++++---------
 include/linux/bio.h           |   39 ++++++++++++++-
 include/linux/blk-mq.h        |    4 -
 kernel/power/swap.c           |  103 +++++++++++++++++-----------------------
 21 files changed, 253 insertions(+), 273 deletions(-)

[PATCH 01/17] block: add a bio_add_virt_nofail helper

[Christoph Hellwig]

Add a helper to add a directly mapped kernel virtual address to a
bio so that callers don't have to convert to pages or folios.

For now only the _nofail variant is provided as that is what all the
obvious callers want.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/bio.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/include/linux/bio.h b/include/linux/bio.h
index cafc7c215de8..0678b67162ee 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -417,6 +417,23 @@ void __bio_add_page(struct bio *bio, struct page *page,
 		unsigned int len, unsigned int off);
 void bio_add_folio_nofail(struct bio *bio, struct folio *folio, size_t len,
 			  size_t off);
+
+/**
+ * bio_add_virt_nofail - add data in the diret kernel mapping to a bio
+ * @bio: destination bio
+ * @vaddr: data to add
+ * @len: length of the data to add, may cross pages
+ *
+ * Add the data at @vaddr to @bio.  The caller must have ensure a segment
+ * is available for the added data.  No merging into an existing segment
+ * will be performed.
+ */
+static inline void bio_add_virt_nofail(struct bio *bio, void *vaddr,
+		unsigned len)
+{
+	__bio_add_page(bio, virt_to_page(vaddr), len, offset_in_page(vaddr));
+}
+
 int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter);
 void bio_iov_bvec_set(struct bio *bio, const struct iov_iter *iter);
 void __bio_release_pages(struct bio *bio, bool mark_dirty);
-- 
2.47.2

Re: [PATCH 01/17] block: add a bio_add_virt_nofail helper

[Hannes Reinecke]

On 4/22/25 16:26, Christoph Hellwig wrote:
> Add a helper to add a directly mapped kernel virtual address to a
> bio so that callers don't have to convert to pages or folios.
> 
> For now only the _nofail variant is provided as that is what all the
> obvious callers want.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   include/linux/bio.h | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 01/17] block: add a bio_add_virt_nofail helper

[Damien Le Moal]

On 4/22/25 23:26, Christoph Hellwig wrote:
> Add a helper to add a directly mapped kernel virtual address to a
> bio so that callers don't have to convert to pages or folios.
> 
> For now only the _nofail variant is provided as that is what all the
> obvious callers want.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  include/linux/bio.h | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index cafc7c215de8..0678b67162ee 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -417,6 +417,23 @@ void __bio_add_page(struct bio *bio, struct page *page,
>  		unsigned int len, unsigned int off);
>  void bio_add_folio_nofail(struct bio *bio, struct folio *folio, size_t len,
>  			  size_t off);
> +
> +/**
> + * bio_add_virt_nofail - add data in the diret kernel mapping to a bio

s/diret/direct

With that, looks good to me.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

> + * @bio: destination bio
> + * @vaddr: data to add
> + * @len: length of the data to add, may cross pages
> + *
> + * Add the data at @vaddr to @bio.  The caller must have ensure a segment
> + * is available for the added data.  No merging into an existing segment
> + * will be performed.
> + */
> +static inline void bio_add_virt_nofail(struct bio *bio, void *vaddr,
> +		unsigned len)
> +{
> +	__bio_add_page(bio, virt_to_page(vaddr), len, offset_in_page(vaddr));
> +}
> +
>  int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter);
>  void bio_iov_bvec_set(struct bio *bio, const struct iov_iter *iter);
>  void __bio_release_pages(struct bio *bio, bool mark_dirty);


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 01/17] block: add a bio_add_virt_nofail helper

[Johannes Thumshirn]

With the typo Damien mentioned fixed:
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 02/17] block: add a bdev_rw_virt helper

[Christoph Hellwig]

Add a helper to perform synchronous I/O on a kernel direct map range.
Currently this is implemented in various places in usually not very
efficient ways, so provide a generic helper instead.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/bio.c         | 30 ++++++++++++++++++++++++++++++
 include/linux/bio.h |  5 ++++-
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/block/bio.c b/block/bio.c
index 4e6c85a33d74..a6a867a432cf 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1301,6 +1301,36 @@ int submit_bio_wait(struct bio *bio)
 }
 EXPORT_SYMBOL(submit_bio_wait);
 
+/**
+ * bdev_rw_virt - synchronously read into / write from kernel mapping
+ * @bdev:	block device to access
+ * @sector:	sector to accasse
+ * @data:	data to read/write
+ * @len:	length to read/write
+ * @op:		operation (e.g. REQ_OP_READ/REQ_OP_WRITE)
+ *
+ * Performs synchronous I/O to @bdev for @data/@len.  @data must be in
+ * the kernel direct mapping and not a vmalloc address.
+ */
+int bdev_rw_virt(struct block_device *bdev, sector_t sector, void *data,
+		size_t len, enum req_op op)
+{
+	struct bio_vec bv;
+	struct bio bio;
+	int error;
+
+	if (WARN_ON_ONCE(is_vmalloc_addr(data)))
+		return -EIO;
+
+	bio_init(&bio, bdev, &bv, 1, op);
+	bio.bi_iter.bi_sector = sector;
+	bio_add_virt_nofail(&bio, data, len);
+	error = submit_bio_wait(&bio);
+	bio_uninit(&bio);
+	return error;
+}
+EXPORT_SYMBOL_GPL(bdev_rw_virt);
+
 static void bio_wait_end_io(struct bio *bio)
 {
 	complete(bio->bi_private);
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 0678b67162ee..17a10220c57d 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -402,7 +402,6 @@ static inline int bio_iov_vecs_to_alloc(struct iov_iter *iter, int max_segs)
 
 struct request_queue;
 
-extern int submit_bio_wait(struct bio *bio);
 void bio_init(struct bio *bio, struct block_device *bdev, struct bio_vec *table,
 	      unsigned short max_vecs, blk_opf_t opf);
 extern void bio_uninit(struct bio *);
@@ -434,6 +433,10 @@ static inline void bio_add_virt_nofail(struct bio *bio, void *vaddr,
 	__bio_add_page(bio, virt_to_page(vaddr), len, offset_in_page(vaddr));
 }
 
+int submit_bio_wait(struct bio *bio);
+int bdev_rw_virt(struct block_device *bdev, sector_t sector, void *data,
+		size_t len, enum req_op op);
+
 int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter);
 void bio_iov_bvec_set(struct bio *bio, const struct iov_iter *iter);
 void __bio_release_pages(struct bio *bio, bool mark_dirty);
-- 
2.47.2

Re: [PATCH 02/17] block: add a bdev_rw_virt helper

[Hannes Reinecke]

On 4/22/25 16:26, Christoph Hellwig wrote:
> Add a helper to perform synchronous I/O on a kernel direct map range.
> Currently this is implemented in various places in usually not very
> efficient ways, so provide a generic helper instead.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   block/bio.c         | 30 ++++++++++++++++++++++++++++++
>   include/linux/bio.h |  5 ++++-
>   2 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/block/bio.c b/block/bio.c
> index 4e6c85a33d74..a6a867a432cf 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -1301,6 +1301,36 @@ int submit_bio_wait(struct bio *bio)
>   }
>   EXPORT_SYMBOL(submit_bio_wait);
>   
> +/**
> + * bdev_rw_virt - synchronously read into / write from kernel mapping
> + * @bdev:	block device to access
> + * @sector:	sector to accasse
> + * @data:	data to read/write
> + * @len:	length to read/write
> + * @op:		operation (e.g. REQ_OP_READ/REQ_OP_WRITE)
> + *
> + * Performs synchronous I/O to @bdev for @data/@len.  @data must be in
> + * the kernel direct mapping and not a vmalloc address.
> + */
> +int bdev_rw_virt(struct block_device *bdev, sector_t sector, void *data,
> +		size_t len, enum req_op op)
> +{
> +	struct bio_vec bv;
> +	struct bio bio;
> +	int error;
> +
> +	if (WARN_ON_ONCE(is_vmalloc_addr(data)))
> +		return -EIO;
> +
> +	bio_init(&bio, bdev, &bv, 1, op);
> +	bio.bi_iter.bi_sector = sector;
> +	bio_add_virt_nofail(&bio, data, len);
> +	error = submit_bio_wait(&bio);
> +	bio_uninit(&bio);
> +	return error;
> +}
> +EXPORT_SYMBOL_GPL(bdev_rw_virt);
> +
>   static void bio_wait_end_io(struct bio *bio)
>   {
>   	complete(bio->bi_private);
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 0678b67162ee..17a10220c57d 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -402,7 +402,6 @@ static inline int bio_iov_vecs_to_alloc(struct iov_iter *iter, int max_segs)
>   
>   struct request_queue;
>   
> -extern int submit_bio_wait(struct bio *bio);
>   void bio_init(struct bio *bio, struct block_device *bdev, struct bio_vec *table,
>   	      unsigned short max_vecs, blk_opf_t opf);
>   extern void bio_uninit(struct bio *);
> @@ -434,6 +433,10 @@ static inline void bio_add_virt_nofail(struct bio *bio, void *vaddr,
>   	__bio_add_page(bio, virt_to_page(vaddr), len, offset_in_page(vaddr));
>   }
>   
> +int submit_bio_wait(struct bio *bio);
> +int bdev_rw_virt(struct block_device *bdev, sector_t sector, void *data,
> +		size_t len, enum req_op op);
> +
>   int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter);
>   void bio_iov_bvec_set(struct bio *bio, const struct iov_iter *iter);
>   void __bio_release_pages(struct bio *bio, bool mark_dirty);

Any specific reason why the declaration of 'submit_bio_wait()' is moved?

Other than that:

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 02/17] block: add a bdev_rw_virt helper

[Christoph Hellwig]

On Wed, Apr 23, 2025 at 08:07:19AM +0200, Hannes Reinecke wrote:
>>   +int submit_bio_wait(struct bio *bio);
>> +int bdev_rw_virt(struct block_device *bdev, sector_t sector, void *data,
>> +		size_t len, enum req_op op);
>> +
>>   int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter);
>>   void bio_iov_bvec_set(struct bio *bio, const struct iov_iter *iter);
>>   void __bio_release_pages(struct bio *bio, bool mark_dirty);
>
> Any specific reason why the declaration of 'submit_bio_wait()' is moved?

To keep the related declarations together.

Re: [PATCH 02/17] block: add a bdev_rw_virt helper

[Damien Le Moal]

On 4/22/25 23:26, Christoph Hellwig wrote:
> Add a helper to perform synchronous I/O on a kernel direct map range.
> Currently this is implemented in various places in usually not very
> efficient ways, so provide a generic helper instead.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/bio.c         | 30 ++++++++++++++++++++++++++++++
>  include/linux/bio.h |  5 ++++-
>  2 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/block/bio.c b/block/bio.c
> index 4e6c85a33d74..a6a867a432cf 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -1301,6 +1301,36 @@ int submit_bio_wait(struct bio *bio)
>  }
>  EXPORT_SYMBOL(submit_bio_wait);
>  
> +/**
> + * bdev_rw_virt - synchronously read into / write from kernel mapping
> + * @bdev:	block device to access
> + * @sector:	sector to accasse

s/accasse/access

> + * @data:	data to read/write
> + * @len:	length to read/write

Maybe "length in bytes to read/write", to avoid any confusion with sector unit.

With that, feel free to add:

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 02/17] block: add a bdev_rw_virt helper

[John Garry]

On 22/04/2025 15:26, Christoph Hellwig wrote:
> + * @sector:	sector to accasse

nit: typo in accasse

> + * @data:	data to read/write
> + * @len:	length to read/write
> + * @op:		operation (e.g. REQ_OP_READ/REQ_OP_WRITE)
> + *
> + * Performs synchronous I/O to @bdev for @data/@len.  @data must be in
> + * the kernel direct mapping and not a vmalloc address.
> + */
> +int bdev_rw_virt(struct block_device *bdev, sector_t sector, void *data,
> +		size_t len, enum req_op op)

Re: [PATCH 02/17] block: add a bdev_rw_virt helper

[Johannes Thumshirn]

With the typo fixes addressed:
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 03/17] block: add a bio_add_vmalloc helper

[Christoph Hellwig]

Add a helper to add a vmalloc region to a bio, abstracting away the
vmalloc addresses from the underlying pages.  Also add a helper to
calculate how many segments need to be allocated for a vmalloc region.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/bio.c         | 27 +++++++++++++++++++++++++++
 include/linux/bio.h | 17 +++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/block/bio.c b/block/bio.c
index a6a867a432cf..3cc93bbdeeb9 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1058,6 +1058,33 @@ bool bio_add_folio(struct bio *bio, struct folio *folio, size_t len,
 }
 EXPORT_SYMBOL(bio_add_folio);
 
+/**
+ * bio_add_vmalloc - add a vmalloc region to a bio
+ * @bio: destination bio
+ * @vaddr: virtual address to add
+ * @len: total length of the data to add
+ *
+ * Add the data at @vaddr to @bio and return how much was added.  This can an
+ * usually is less than the amount originally asked.  Returns 0 if no data could
+ * be added to the bio.
+ *
+ * This helper calls flush_kernel_vmap_range() for the range added.  For reads,
+ * the caller still needs to manually call invalidate_kernel_vmap_range() in
+ * the completion handler.
+ */
+unsigned int bio_add_vmalloc(struct bio *bio, void *vaddr, unsigned len)
+{
+	unsigned int offset = offset_in_page(vaddr);
+
+	len = min(len, PAGE_SIZE - offset);
+	if (bio_add_page(bio, vmalloc_to_page(vaddr), len, offset) < len)
+		return 0;
+	if (op_is_write(bio_op(bio)))
+		flush_kernel_vmap_range(vaddr, len);
+	return len;
+}
+EXPORT_SYMBOL_GPL(bio_add_vmalloc);
+
 void __bio_release_pages(struct bio *bio, bool mark_dirty)
 {
 	struct folio_iter fi;
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 17a10220c57d..c4069422fd0a 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -433,6 +433,23 @@ static inline void bio_add_virt_nofail(struct bio *bio, void *vaddr,
 	__bio_add_page(bio, virt_to_page(vaddr), len, offset_in_page(vaddr));
 }
 
+/**
+ * bio_vmalloc_max_vecs - number of segments needed to map vmalloc data
+ * @vaddr: address to map
+ * @len: length to map
+ *
+ * Calculate how many bio segments need to be allocated to map the vmalloc/vmap
+ * range in [@addr:@len].  This could be an overestimation if the vmalloc area
+ * is backed by large folios.
+ */
+static inline unsigned int bio_vmalloc_max_vecs(void *vaddr, unsigned int len)
+{
+	return DIV_ROUND_UP(offset_in_page(vaddr) + len, PAGE_SIZE);
+}
+
+unsigned int __must_check bio_add_vmalloc(struct bio *bio, void *vaddr,
+		unsigned len);
+
 int submit_bio_wait(struct bio *bio);
 int bdev_rw_virt(struct block_device *bdev, sector_t sector, void *data,
 		size_t len, enum req_op op);
-- 
2.47.2

Re: [PATCH 03/17] block: add a bio_add_vmalloc helper

[Hannes Reinecke]

On 4/22/25 16:26, Christoph Hellwig wrote:
> Add a helper to add a vmalloc region to a bio, abstracting away the
> vmalloc addresses from the underlying pages.  Also add a helper to
> calculate how many segments need to be allocated for a vmalloc region.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   block/bio.c         | 27 +++++++++++++++++++++++++++
>   include/linux/bio.h | 17 +++++++++++++++++
>   2 files changed, 44 insertions(+)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 03/17] block: add a bio_add_vmalloc helper

[Damien Le Moal]

On 4/22/25 23:26, Christoph Hellwig wrote:
> Add a helper to add a vmalloc region to a bio, abstracting away the
> vmalloc addresses from the underlying pages.  Also add a helper to
> calculate how many segments need to be allocated for a vmalloc region.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/bio.c         | 27 +++++++++++++++++++++++++++
>  include/linux/bio.h | 17 +++++++++++++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/block/bio.c b/block/bio.c
> index a6a867a432cf..3cc93bbdeeb9 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -1058,6 +1058,33 @@ bool bio_add_folio(struct bio *bio, struct folio *folio, size_t len,
>  }
>  EXPORT_SYMBOL(bio_add_folio);
>  
> +/**
> + * bio_add_vmalloc - add a vmalloc region to a bio
> + * @bio: destination bio
> + * @vaddr: virtual address to add
> + * @len: total length of the data to add

Nit: to be consistent in the wording...

 * @vaddr: address of the vmalloc region to add
 * @len: total length of the vmalloc region to add

> + *
> + * Add the data at @vaddr to @bio and return how much was added.  This can an

s/an/and

or may be simply:

This may be less than the amount originally asked.

> + * usually is less than the amount originally asked.  Returns 0 if no data could
> + * be added to the bio.
> + *
> + * This helper calls flush_kernel_vmap_range() for the range added.  For reads,
> + * the caller still needs to manually call invalidate_kernel_vmap_range() in
> + * the completion handler.
> + */
> +unsigned int bio_add_vmalloc(struct bio *bio, void *vaddr, unsigned len)
> +{
> +	unsigned int offset = offset_in_page(vaddr);
> +
> +	len = min(len, PAGE_SIZE - offset);
> +	if (bio_add_page(bio, vmalloc_to_page(vaddr), len, offset) < len)
> +		return 0;
> +	if (op_is_write(bio_op(bio)))
> +		flush_kernel_vmap_range(vaddr, len);
> +	return len;
> +}
> +EXPORT_SYMBOL_GPL(bio_add_vmalloc);
> +
>  void __bio_release_pages(struct bio *bio, bool mark_dirty)
>  {
>  	struct folio_iter fi;
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 17a10220c57d..c4069422fd0a 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -433,6 +433,23 @@ static inline void bio_add_virt_nofail(struct bio *bio, void *vaddr,
>  	__bio_add_page(bio, virt_to_page(vaddr), len, offset_in_page(vaddr));
>  }
>  
> +/**
> + * bio_vmalloc_max_vecs - number of segments needed to map vmalloc data

Nit: number of BIO segments needed to add a vmalloc-ed region to a BIO ?

> + * @vaddr: address to map
> + * @len: length to map

Nit:

 * @vaddr: address of the vmalloc region to add
 * @len: total length of the vmalloc region to add

> + *
> + * Calculate how many bio segments need to be allocated to map the vmalloc/vmap

s/to map/to add ?

> + * range in [@addr:@len].  This could be an overestimation if the vmalloc area
> + * is backed by large folios.
> + */
> +static inline unsigned int bio_vmalloc_max_vecs(void *vaddr, unsigned int len)
> +{
> +	return DIV_ROUND_UP(offset_in_page(vaddr) + len, PAGE_SIZE);
> +}
> +
> +unsigned int __must_check bio_add_vmalloc(struct bio *bio, void *vaddr,
> +		unsigned len);
> +
>  int submit_bio_wait(struct bio *bio);
>  int bdev_rw_virt(struct block_device *bdev, sector_t sector, void *data,
>  		size_t len, enum req_op op);

Other than these wording nits, looks OK to me.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 03/17] block: add a bio_add_vmalloc helper

[Johannes Thumshirn]

With the Damien's comments addressed:
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 04/17] block: remove the q argument from blk_rq_map_kern

[Christoph Hellwig]

Remove the q argument from blk_rq_map_kern and the internal helpers
called by it as the queue can trivially be derived from the request.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-map.c            | 24 ++++++++++--------------
 drivers/block/pktcdvd.c    |  2 +-
 drivers/block/ublk_drv.c   |  3 +--
 drivers/block/virtio_blk.c |  4 ++--
 drivers/nvme/host/core.c   |  2 +-
 drivers/scsi/scsi_ioctl.c  |  2 +-
 drivers/scsi/scsi_lib.c    |  3 +--
 include/linux/blk-mq.h     |  4 ++--
 8 files changed, 19 insertions(+), 25 deletions(-)

diff --git a/block/blk-map.c b/block/blk-map.c
index d2f22744b3d1..0cbceb2671c9 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -319,7 +319,6 @@ static void bio_map_kern_endio(struct bio *bio)
 
 /**
  *	bio_map_kern	-	map kernel address into bio
- *	@q: the struct request_queue for the bio
  *	@data: pointer to buffer to map
  *	@len: length in bytes
  *	@gfp_mask: allocation flags for bio allocation
@@ -327,8 +326,7 @@ static void bio_map_kern_endio(struct bio *bio)
  *	Map the kernel address into a bio suitable for io to a block
  *	device. Returns an error pointer in case of error.
  */
-static struct bio *bio_map_kern(struct request_queue *q, void *data,
-		unsigned int len, gfp_t gfp_mask)
+static struct bio *bio_map_kern(void *data, unsigned int len, gfp_t gfp_mask)
 {
 	unsigned long kaddr = (unsigned long)data;
 	unsigned long end = (kaddr + len + PAGE_SIZE - 1) >> PAGE_SHIFT;
@@ -402,7 +400,6 @@ static void bio_copy_kern_endio_read(struct bio *bio)
 
 /**
  *	bio_copy_kern	-	copy kernel address into bio
- *	@q: the struct request_queue for the bio
  *	@data: pointer to buffer to copy
  *	@len: length in bytes
  *	@gfp_mask: allocation flags for bio and page allocation
@@ -411,8 +408,8 @@ static void bio_copy_kern_endio_read(struct bio *bio)
  *	copy the kernel address into a bio suitable for io to a block
  *	device. Returns an error pointer in case of error.
  */
-static struct bio *bio_copy_kern(struct request_queue *q, void *data,
-		unsigned int len, gfp_t gfp_mask, int reading)
+static struct bio *bio_copy_kern(void *data, unsigned int len, gfp_t gfp_mask,
+		int reading)
 {
 	unsigned long kaddr = (unsigned long)data;
 	unsigned long end = (kaddr + len + PAGE_SIZE - 1) >> PAGE_SHIFT;
@@ -689,7 +686,6 @@ EXPORT_SYMBOL(blk_rq_unmap_user);
 
 /**
  * blk_rq_map_kern - map kernel data to a request, for passthrough requests
- * @q:		request queue where request should be inserted
  * @rq:		request to fill
  * @kbuf:	the kernel buffer
  * @len:	length of user data
@@ -700,24 +696,24 @@ EXPORT_SYMBOL(blk_rq_unmap_user);
  *    buffer is used. Can be called multiple times to append multiple
  *    buffers.
  */
-int blk_rq_map_kern(struct request_queue *q, struct request *rq, void *kbuf,
-		    unsigned int len, gfp_t gfp_mask)
+int blk_rq_map_kern(struct request *rq, void *kbuf, unsigned int len,
+		gfp_t gfp_mask)
 {
 	int reading = rq_data_dir(rq) == READ;
 	unsigned long addr = (unsigned long) kbuf;
 	struct bio *bio;
 	int ret;
 
-	if (len > (queue_max_hw_sectors(q) << 9))
+	if (len > (queue_max_hw_sectors(rq->q) << SECTOR_SHIFT))
 		return -EINVAL;
 	if (!len || !kbuf)
 		return -EINVAL;
 
-	if (!blk_rq_aligned(q, addr, len) || object_is_on_stack(kbuf) ||
-	    blk_queue_may_bounce(q))
-		bio = bio_copy_kern(q, kbuf, len, gfp_mask, reading);
+	if (!blk_rq_aligned(rq->q, addr, len) || object_is_on_stack(kbuf) ||
+	    blk_queue_may_bounce(rq->q))
+		bio = bio_copy_kern(kbuf, len, gfp_mask, reading);
 	else
-		bio = bio_map_kern(q, kbuf, len, gfp_mask);
+		bio = bio_map_kern(kbuf, len, gfp_mask);
 
 	if (IS_ERR(bio))
 		return PTR_ERR(bio);
diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index 65b96c083b3c..d5cc7bd2875c 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -725,7 +725,7 @@ static int pkt_generic_packet(struct pktcdvd_device *pd, struct packet_command *
 	scmd = blk_mq_rq_to_pdu(rq);
 
 	if (cgc->buflen) {
-		ret = blk_rq_map_kern(q, rq, cgc->buffer, cgc->buflen,
+		ret = blk_rq_map_kern(rq, cgc->buffer, cgc->buflen,
 				      GFP_NOIO);
 		if (ret)
 			goto out;
diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 03653bd7a1df..0bc77d4634fd 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -363,8 +363,7 @@ static int ublk_report_zones(struct gendisk *disk, sector_t sector,
 		if (ret)
 			goto free_req;
 
-		ret = blk_rq_map_kern(disk->queue, req, buffer, buffer_length,
-					GFP_KERNEL);
+		ret = blk_rq_map_kern(req, buffer, buffer_length, GFP_KERNEL);
 		if (ret)
 			goto erase_desc;
 
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 7cffea01d868..30bca8cb7106 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -571,7 +571,7 @@ static int virtblk_submit_zone_report(struct virtio_blk *vblk,
 	vbr->out_hdr.type = cpu_to_virtio32(vblk->vdev, VIRTIO_BLK_T_ZONE_REPORT);
 	vbr->out_hdr.sector = cpu_to_virtio64(vblk->vdev, sector);
 
-	err = blk_rq_map_kern(q, req, report_buf, report_len, GFP_KERNEL);
+	err = blk_rq_map_kern(req, report_buf, report_len, GFP_KERNEL);
 	if (err)
 		goto out;
 
@@ -817,7 +817,7 @@ static int virtblk_get_id(struct gendisk *disk, char *id_str)
 	vbr->out_hdr.type = cpu_to_virtio32(vblk->vdev, VIRTIO_BLK_T_GET_ID);
 	vbr->out_hdr.sector = 0;
 
-	err = blk_rq_map_kern(q, req, id_str, VIRTIO_BLK_ID_BYTES, GFP_KERNEL);
+	err = blk_rq_map_kern(req, id_str, VIRTIO_BLK_ID_BYTES, GFP_KERNEL);
 	if (err)
 		goto out;
 
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index eb6ea8acb3cc..34d2abdb2f89 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1157,7 +1157,7 @@ int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
 		req->cmd_flags &= ~REQ_FAILFAST_DRIVER;
 
 	if (buffer && bufflen) {
-		ret = blk_rq_map_kern(q, req, buffer, bufflen, GFP_KERNEL);
+		ret = blk_rq_map_kern(req, buffer, bufflen, GFP_KERNEL);
 		if (ret)
 			goto out;
 	}
diff --git a/drivers/scsi/scsi_ioctl.c b/drivers/scsi/scsi_ioctl.c
index 2fa45556e1ea..0ddc95bafc71 100644
--- a/drivers/scsi/scsi_ioctl.c
+++ b/drivers/scsi/scsi_ioctl.c
@@ -601,7 +601,7 @@ static int sg_scsi_ioctl(struct request_queue *q, bool open_for_write,
 	}
 
 	if (bytes) {
-		err = blk_rq_map_kern(q, rq, buffer, bytes, GFP_NOIO);
+		err = blk_rq_map_kern(rq, buffer, bytes, GFP_NOIO);
 		if (err)
 			goto error;
 	}
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 0d29470e86b0..f313fcd30269 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -313,8 +313,7 @@ int scsi_execute_cmd(struct scsi_device *sdev, const unsigned char *cmd,
 		return PTR_ERR(req);
 
 	if (bufflen) {
-		ret = blk_rq_map_kern(sdev->request_queue, req,
-				      buffer, bufflen, GFP_NOIO);
+		ret = blk_rq_map_kern(req, buffer, bufflen, GFP_NOIO);
 		if (ret)
 			goto out;
 	}
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 8eb9b3310167..8901347c778b 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -1031,8 +1031,8 @@ int blk_rq_map_user_io(struct request *, struct rq_map_data *,
 int blk_rq_map_user_iov(struct request_queue *, struct request *,
 		struct rq_map_data *, const struct iov_iter *, gfp_t);
 int blk_rq_unmap_user(struct bio *);
-int blk_rq_map_kern(struct request_queue *, struct request *, void *,
-		unsigned int, gfp_t);
+int blk_rq_map_kern(struct request *rq, void *kbuf, unsigned int len,
+		gfp_t gfp);
 int blk_rq_append_bio(struct request *rq, struct bio *bio);
 void blk_execute_rq_nowait(struct request *rq, bool at_head);
 blk_status_t blk_execute_rq(struct request *rq, bool at_head);
-- 
2.47.2

Re: [PATCH 04/17] block: remove the q argument from blk_rq_map_kern

[Hannes Reinecke]

On 4/22/25 16:26, Christoph Hellwig wrote:
> Remove the q argument from blk_rq_map_kern and the internal helpers
> called by it as the queue can trivially be derived from the request.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   block/blk-map.c            | 24 ++++++++++--------------
>   drivers/block/pktcdvd.c    |  2 +-
>   drivers/block/ublk_drv.c   |  3 +--
>   drivers/block/virtio_blk.c |  4 ++--
>   drivers/nvme/host/core.c   |  2 +-
>   drivers/scsi/scsi_ioctl.c  |  2 +-
>   drivers/scsi/scsi_lib.c    |  3 +--
>   include/linux/blk-mq.h     |  4 ++--
>   8 files changed, 19 insertions(+), 25 deletions(-)
> 
Good cleanup. I always wondered why we need to have it.

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 04/17] block: remove the q argument from blk_rq_map_kern

[Johannes Thumshirn]

On 23.04.25 08:10, Hannes Reinecke wrote:
> On 4/22/25 16:26, Christoph Hellwig wrote:
>> Remove the q argument from blk_rq_map_kern and the internal helpers
>> called by it as the queue can trivially be derived from the request.
>>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>> ---
>>    block/blk-map.c            | 24 ++++++++++--------------
>>    drivers/block/pktcdvd.c    |  2 +-
>>    drivers/block/ublk_drv.c   |  3 +--
>>    drivers/block/virtio_blk.c |  4 ++--
>>    drivers/nvme/host/core.c   |  2 +-
>>    drivers/scsi/scsi_ioctl.c  |  2 +-
>>    drivers/scsi/scsi_lib.c    |  3 +--
>>    include/linux/blk-mq.h     |  4 ++--
>>    8 files changed, 19 insertions(+), 25 deletions(-)
>>
> Good cleanup. I always wondered why we need to have it.

Because we used to call 'bio_add_pc_page()' in e.g. bio_map_kern()' 
which took a request_queue. But that got changed in 6aeb4f8364806 
("block: remove bio_add_pc_page") to a simple 'bio_add_page()'.

So much for the archeology,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 04/17] block: remove the q argument from blk_rq_map_kern

[hch]

On Tue, Apr 29, 2025 at 11:24:55AM +0000, Johannes Thumshirn wrote:
> > Good cleanup. I always wondered why we need to have it.
> 
> Because we used to call 'bio_add_pc_page()' in e.g. bio_map_kern()' 
> which took a request_queue. But that got changed in 6aeb4f8364806 
> ("block: remove bio_add_pc_page") to a simple 'bio_add_page()'.

Even back then you could have easily derived it from the struct
request, through.

Re: [PATCH 04/17] block: remove the q argument from blk_rq_map_kern

[Damien Le Moal]

On 4/22/25 23:26, Christoph Hellwig wrote:
> Remove the q argument from blk_rq_map_kern and the internal helpers
> called by it as the queue can trivially be derived from the request.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

[PATCH 05/17] block: pass the operation to bio_{map,copy}_kern

[Christoph Hellwig]

That way the bio can be allocated with the right operation already
set and there is no need to pass the separated 'reading' argument.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-map.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/block/blk-map.c b/block/blk-map.c
index 0cbceb2671c9..ca6b55ac0da1 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -321,12 +321,14 @@ static void bio_map_kern_endio(struct bio *bio)
  *	bio_map_kern	-	map kernel address into bio
  *	@data: pointer to buffer to map
  *	@len: length in bytes
+ *	@op: bio/request operation
  *	@gfp_mask: allocation flags for bio allocation
  *
  *	Map the kernel address into a bio suitable for io to a block
  *	device. Returns an error pointer in case of error.
  */
-static struct bio *bio_map_kern(void *data, unsigned int len, gfp_t gfp_mask)
+static struct bio *bio_map_kern(void *data, unsigned int len,
+		enum req_op op, gfp_t gfp_mask)
 {
 	unsigned long kaddr = (unsigned long)data;
 	unsigned long end = (kaddr + len + PAGE_SIZE - 1) >> PAGE_SHIFT;
@@ -340,7 +342,7 @@ static struct bio *bio_map_kern(void *data, unsigned int len, gfp_t gfp_mask)
 	bio = bio_kmalloc(nr_pages, gfp_mask);
 	if (!bio)
 		return ERR_PTR(-ENOMEM);
-	bio_init(bio, NULL, bio->bi_inline_vecs, nr_pages, 0);
+	bio_init(bio, NULL, bio->bi_inline_vecs, nr_pages, op);
 
 	if (is_vmalloc) {
 		flush_kernel_vmap_range(data, len);
@@ -402,14 +404,14 @@ static void bio_copy_kern_endio_read(struct bio *bio)
  *	bio_copy_kern	-	copy kernel address into bio
  *	@data: pointer to buffer to copy
  *	@len: length in bytes
+ *	@op: bio/request operation
  *	@gfp_mask: allocation flags for bio and page allocation
- *	@reading: data direction is READ
  *
  *	copy the kernel address into a bio suitable for io to a block
  *	device. Returns an error pointer in case of error.
  */
-static struct bio *bio_copy_kern(void *data, unsigned int len, gfp_t gfp_mask,
-		int reading)
+static struct bio *bio_copy_kern(void *data, unsigned int len, enum req_op op,
+		gfp_t gfp_mask)
 {
 	unsigned long kaddr = (unsigned long)data;
 	unsigned long end = (kaddr + len + PAGE_SIZE - 1) >> PAGE_SHIFT;
@@ -428,7 +430,7 @@ static struct bio *bio_copy_kern(void *data, unsigned int len, gfp_t gfp_mask,
 	bio = bio_kmalloc(nr_pages, gfp_mask);
 	if (!bio)
 		return ERR_PTR(-ENOMEM);
-	bio_init(bio, NULL, bio->bi_inline_vecs, nr_pages, 0);
+	bio_init(bio, NULL, bio->bi_inline_vecs, nr_pages, op);
 
 	while (len) {
 		struct page *page;
@@ -441,7 +443,7 @@ static struct bio *bio_copy_kern(void *data, unsigned int len, gfp_t gfp_mask,
 		if (!page)
 			goto cleanup;
 
-		if (!reading)
+		if (op_is_write(op))
 			memcpy(page_address(page), p, bytes);
 
 		if (bio_add_page(bio, page, bytes, 0) < bytes)
@@ -451,11 +453,11 @@ static struct bio *bio_copy_kern(void *data, unsigned int len, gfp_t gfp_mask,
 		p += bytes;
 	}
 
-	if (reading) {
+	if (op_is_write(op)) {
+		bio->bi_end_io = bio_copy_kern_endio;
+	} else {
 		bio->bi_end_io = bio_copy_kern_endio_read;
 		bio->bi_private = data;
-	} else {
-		bio->bi_end_io = bio_copy_kern_endio;
 	}
 
 	return bio;
@@ -699,7 +701,6 @@ EXPORT_SYMBOL(blk_rq_unmap_user);
 int blk_rq_map_kern(struct request *rq, void *kbuf, unsigned int len,
 		gfp_t gfp_mask)
 {
-	int reading = rq_data_dir(rq) == READ;
 	unsigned long addr = (unsigned long) kbuf;
 	struct bio *bio;
 	int ret;
@@ -711,16 +712,13 @@ int blk_rq_map_kern(struct request *rq, void *kbuf, unsigned int len,
 
 	if (!blk_rq_aligned(rq->q, addr, len) || object_is_on_stack(kbuf) ||
 	    blk_queue_may_bounce(rq->q))
-		bio = bio_copy_kern(kbuf, len, gfp_mask, reading);
+		bio = bio_copy_kern(kbuf, len, req_op(rq), gfp_mask);
 	else
-		bio = bio_map_kern(kbuf, len, gfp_mask);
+		bio = bio_map_kern(kbuf, len, req_op(rq), gfp_mask);
 
 	if (IS_ERR(bio))
 		return PTR_ERR(bio);
 
-	bio->bi_opf &= ~REQ_OP_MASK;
-	bio->bi_opf |= req_op(rq);
-
 	ret = blk_rq_append_bio(rq, bio);
 	if (unlikely(ret)) {
 		bio_uninit(bio);
-- 
2.47.2

Re: [PATCH 05/17] block: pass the operation to bio_{map,copy}_kern

[Hannes Reinecke]

On 4/22/25 16:26, Christoph Hellwig wrote:
> That way the bio can be allocated with the right operation already
> set and there is no need to pass the separated 'reading' argument.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   block/blk-map.c | 30 ++++++++++++++----------------
>   1 file changed, 14 insertions(+), 16 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 05/17] block: pass the operation to bio_{map,copy}_kern

[Damien Le Moal]

On 4/22/25 23:26, Christoph Hellwig wrote:
> That way the bio can be allocated with the right operation already
> set and there is no need to pass the separated 'reading' argument.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 05/17] block: pass the operation to bio_{map,copy}_kern

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 06/17] block: simplify bio_map_kern

[Christoph Hellwig]

Split bio_map_kern into a simple version that can use bio_add_virt_nofail
for kernel direct mapping addresses and a more complex bio_map_vmalloc
with the logic to chunk up and map vmalloc ranges using the
bio_add_vmalloc helper.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-map.c | 74 +++++++++++++++++++------------------------------
 1 file changed, 29 insertions(+), 45 deletions(-)

diff --git a/block/blk-map.c b/block/blk-map.c
index ca6b55ac0da1..7742d3cb0499 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -317,65 +317,47 @@ static void bio_map_kern_endio(struct bio *bio)
 	kfree(bio);
 }
 
-/**
- *	bio_map_kern	-	map kernel address into bio
- *	@data: pointer to buffer to map
- *	@len: length in bytes
- *	@op: bio/request operation
- *	@gfp_mask: allocation flags for bio allocation
- *
- *	Map the kernel address into a bio suitable for io to a block
- *	device. Returns an error pointer in case of error.
- */
-static struct bio *bio_map_kern(void *data, unsigned int len,
-		enum req_op op, gfp_t gfp_mask)
+static struct bio *bio_map_virt(void *data, unsigned int len, enum req_op op,
+		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;
-	bool is_vmalloc = is_vmalloc_addr(data);
-	struct page *page;
-	int offset, i;
 	struct bio *bio;
 
-	bio = bio_kmalloc(nr_pages, gfp_mask);
+	bio = bio_kmalloc(1, gfp_mask);
 	if (!bio)
 		return ERR_PTR(-ENOMEM);
-	bio_init(bio, NULL, bio->bi_inline_vecs, nr_pages, op);
-
-	if (is_vmalloc) {
-		flush_kernel_vmap_range(data, len);
-		bio->bi_private = data;
-	}
-
-	offset = offset_in_page(kaddr);
-	for (i = 0; i < nr_pages; i++) {
-		unsigned int bytes = PAGE_SIZE - offset;
+	bio_init(bio, NULL, bio->bi_inline_vecs, 1, op);
+	bio_add_virt_nofail(bio, data, len);
+	bio->bi_end_io = bio_map_kern_endio;
+	return bio;
+}
 
-		if (len <= 0)
-			break;
+static struct bio *bio_map_vmalloc(void *data, unsigned int len, enum req_op op,
+		gfp_t gfp_mask)
+{
+	unsigned int nr_vecs = bio_vmalloc_max_vecs(data, len);
+	unsigned int added;
+	struct bio *bio;
 
-		if (bytes > len)
-			bytes = len;
+	bio = bio_kmalloc(nr_vecs, gfp_mask);
+	if (!bio)
+		return ERR_PTR(-ENOMEM);
+	bio_init(bio, NULL, bio->bi_inline_vecs, nr_vecs, op);
+	bio->bi_private = data;
+	bio->bi_end_io = bio_map_kern_endio;
 
-		if (!is_vmalloc)
-			page = virt_to_page(data);
-		else
-			page = vmalloc_to_page(data);
-		if (bio_add_page(bio, page, bytes, offset) < bytes) {
+	do {
+		added = bio_add_vmalloc(bio, data, len);
+		if (!added) {
 			/* we don't support partial mappings */
 			bio_uninit(bio);
 			kfree(bio);
 			return ERR_PTR(-EINVAL);
 		}
 
-		data += bytes;
-		len -= bytes;
-		offset = 0;
-	}
+		data += added;
+		len -= added;
+	} while (len);
 
-	bio->bi_end_io = bio_map_kern_endio;
 	return bio;
 }
 
@@ -713,8 +695,10 @@ int blk_rq_map_kern(struct request *rq, void *kbuf, unsigned int len,
 	if (!blk_rq_aligned(rq->q, addr, len) || object_is_on_stack(kbuf) ||
 	    blk_queue_may_bounce(rq->q))
 		bio = bio_copy_kern(kbuf, len, req_op(rq), gfp_mask);
+	else if (is_vmalloc_addr(kbuf))
+		bio = bio_map_vmalloc(kbuf, len, req_op(rq), gfp_mask);
 	else
-		bio = bio_map_kern(kbuf, len, req_op(rq), gfp_mask);
+		bio = bio_map_virt(kbuf, len, req_op(rq), gfp_mask);
 
 	if (IS_ERR(bio))
 		return PTR_ERR(bio);
-- 
2.47.2

Re: [PATCH 06/17] block: simplify bio_map_kern

[Hannes Reinecke]

On 4/22/25 16:26, Christoph Hellwig wrote:
> Split bio_map_kern into a simple version that can use bio_add_virt_nofail
> for kernel direct mapping addresses and a more complex bio_map_vmalloc
> with the logic to chunk up and map vmalloc ranges using the
> bio_add_vmalloc helper.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   block/blk-map.c | 74 +++++++++++++++++++------------------------------
>   1 file changed, 29 insertions(+), 45 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 06/17] block: simplify bio_map_kern

[Damien Le Moal]

On 4/22/25 23:26, Christoph Hellwig wrote:
> Split bio_map_kern into a simple version that can use bio_add_virt_nofail
> for kernel direct mapping addresses and a more complex bio_map_vmalloc
> with the logic to chunk up and map vmalloc ranges using the
> bio_add_vmalloc helper.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks OK to me.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 06/17] block: simplify bio_map_kern

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 07/17] bcache: use bio_add_virt_nofail

[Christoph Hellwig]

Convert the __bio_add_page(..., virt_to_page(), ...) pattern to the
bio_add_virt_nofail helper implementing it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/md/bcache/super.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 813b38aec3e4..c40db9c161c1 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -293,8 +293,7 @@ static void __write_super(struct cache_sb *sb, struct cache_sb_disk *out,
 
 	bio->bi_opf = REQ_OP_WRITE | REQ_SYNC | REQ_META;
 	bio->bi_iter.bi_sector	= SB_SECTOR;
-	__bio_add_page(bio, virt_to_page(out), SB_SIZE,
-			offset_in_page(out));
+	bio_add_virt_nofail(bio, out, SB_SIZE);
 
 	out->offset		= cpu_to_le64(sb->offset);
 
-- 
2.47.2

Re: [PATCH 07/17] bcache: use bio_add_virt_nofail

[Damien Le Moal]

On 4/22/25 23:26, Christoph Hellwig wrote:
> Convert the __bio_add_page(..., virt_to_page(), ...) pattern to the
> bio_add_virt_nofail helper implementing it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 07/17] bcache: use bio_add_virt_nofail

[Coly Li]

> 2025年4月24日 14:14，Damien Le Moal <dlemoal@kernel.org> 写道：
> 
> On 4/22/25 23:26, Christoph Hellwig wrote:
>> Convert the __bio_add_page(..., virt_to_page(), ...) pattern to the
>> bio_add_virt_nofail helper implementing it.
>> 
>> Signed-off-by: Christoph Hellwig <hch@lst.de>

For bcache part,

Acked-by: Coly Li <colyli@kernel.org>

Thanks.

Coly Li

Re: [PATCH 07/17] bcache: use bio_add_virt_nofail

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 08/17] dm-bufio: use bio_add_virt_nofail

[Christoph Hellwig]

Convert the __bio_add_page(..., virt_to_page(), ...) pattern to the
bio_add_virt_nofail helper implementing it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/md/dm-bufio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
index 9c8ed65cd87e..e82cd5dc83ce 100644
--- a/drivers/md/dm-bufio.c
+++ b/drivers/md/dm-bufio.c
@@ -1362,7 +1362,7 @@ static void use_bio(struct dm_buffer *b, enum req_op op, sector_t sector,
 	ptr = (char *)b->data + offset;
 	len = n_sectors << SECTOR_SHIFT;
 
-	__bio_add_page(bio, virt_to_page(ptr), len, offset_in_page(ptr));
+	bio_add_virt_nofail(bio, ptr, len);
 
 	submit_bio(bio);
 }
-- 
2.47.2

Re: [PATCH 08/17] dm-bufio: use bio_add_virt_nofail

[Damien Le Moal]

On 4/22/25 23:26, Christoph Hellwig wrote:
> Convert the __bio_add_page(..., virt_to_page(), ...) pattern to the
> bio_add_virt_nofail helper implementing it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 08/17] dm-bufio: use bio_add_virt_nofail

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 09/17] dm-integrity: use bio_add_virt_nofail

[Christoph Hellwig]

Convert the __bio_add_page(..., virt_to_page(), ...) pattern to the
bio_add_virt_nofail helper implementing it, and do the same for the
similar pattern using bio_add_page for adding the first segment after
a bio allocation as that can't fail either.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/md/dm-integrity.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
index 2a283feb3319..9dca9dbabfaa 100644
--- a/drivers/md/dm-integrity.c
+++ b/drivers/md/dm-integrity.c
@@ -2557,14 +2557,8 @@ static void dm_integrity_inline_recheck(struct work_struct *w)
 		char *mem;
 
 		outgoing_bio = bio_alloc_bioset(ic->dev->bdev, 1, REQ_OP_READ, GFP_NOIO, &ic->recheck_bios);
-
-		r = bio_add_page(outgoing_bio, virt_to_page(outgoing_data), ic->sectors_per_block << SECTOR_SHIFT, 0);
-		if (unlikely(r != (ic->sectors_per_block << SECTOR_SHIFT))) {
-			bio_put(outgoing_bio);
-			bio->bi_status = BLK_STS_RESOURCE;
-			bio_endio(bio);
-			return;
-		}
+		bio_add_virt_nofail(outgoing_bio, outgoing_data,
+				ic->sectors_per_block << SECTOR_SHIFT);
 
 		bip = bio_integrity_alloc(outgoing_bio, GFP_NOIO, 1);
 		if (IS_ERR(bip)) {
@@ -3211,7 +3205,8 @@ static void integrity_recalc_inline(struct work_struct *w)
 
 	bio = bio_alloc_bioset(ic->dev->bdev, 1, REQ_OP_READ, GFP_NOIO, &ic->recalc_bios);
 	bio->bi_iter.bi_sector = ic->start + SB_SECTORS + range.logical_sector;
-	__bio_add_page(bio, virt_to_page(recalc_buffer), range.n_sectors << SECTOR_SHIFT, offset_in_page(recalc_buffer));
+	bio_add_virt_nofail(bio, recalc_buffer,
+			range.n_sectors << SECTOR_SHIFT);
 	r = submit_bio_wait(bio);
 	bio_put(bio);
 	if (unlikely(r)) {
@@ -3228,7 +3223,8 @@ static void integrity_recalc_inline(struct work_struct *w)
 
 	bio = bio_alloc_bioset(ic->dev->bdev, 1, REQ_OP_WRITE, GFP_NOIO, &ic->recalc_bios);
 	bio->bi_iter.bi_sector = ic->start + SB_SECTORS + range.logical_sector;
-	__bio_add_page(bio, virt_to_page(recalc_buffer), range.n_sectors << SECTOR_SHIFT, offset_in_page(recalc_buffer));
+	bio_add_virt_nofail(bio, recalc_buffer,
+			range.n_sectors << SECTOR_SHIFT);
 
 	bip = bio_integrity_alloc(bio, GFP_NOIO, 1);
 	if (unlikely(IS_ERR(bip))) {
-- 
2.47.2

Re: [PATCH 09/17] dm-integrity: use bio_add_virt_nofail

[Damien Le Moal]

On 4/22/25 23:26, Christoph Hellwig wrote:
> Convert the __bio_add_page(..., virt_to_page(), ...) pattern to the
> bio_add_virt_nofail helper implementing it, and do the same for the
> similar pattern using bio_add_page for adding the first segment after
> a bio allocation as that can't fail either.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 09/17] dm-integrity: use bio_add_virt_nofail

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 10/17] rnbd-srv: use bio_add_virt_nofail

[Christoph Hellwig]

Use the bio_add_virt_nofail to add a single kernel virtual address
to a bio as that can't fail.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/rnbd/rnbd-srv.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c
index 2ee6e9bd4e28..2df8941a6b14 100644
--- a/drivers/block/rnbd/rnbd-srv.c
+++ b/drivers/block/rnbd/rnbd-srv.c
@@ -147,12 +147,7 @@ static int process_rdma(struct rnbd_srv_session *srv_sess,
 
 	bio = bio_alloc(file_bdev(sess_dev->bdev_file), 1,
 			rnbd_to_bio_flags(le32_to_cpu(msg->rw)), GFP_KERNEL);
-	if (bio_add_page(bio, virt_to_page(data), datalen,
-			offset_in_page(data)) != datalen) {
-		rnbd_srv_err_rl(sess_dev, "Failed to map data to bio\n");
-		err = -EINVAL;
-		goto bio_put;
-	}
+	bio_add_virt_nofail(bio, data, datalen);
 
 	bio->bi_opf = rnbd_to_bio_flags(le32_to_cpu(msg->rw));
 	if (bio_has_data(bio) &&
-- 
2.47.2

Re: [PATCH 10/17] rnbd-srv: use bio_add_virt_nofail

[Damien Le Moal]

On 4/22/25 23:26, Christoph Hellwig wrote:
> Use the bio_add_virt_nofail to add a single kernel virtual address
> to a bio as that can't fail.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 10/17] rnbd-srv: use bio_add_virt_nofail

[Damien Le Moal]

On 4/22/25 23:26, Christoph Hellwig wrote:
> Use the bio_add_virt_nofail to add a single kernel virtual address
> to a bio as that can't fail.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 10/17] rnbd-srv: use bio_add_virt_nofail

[Jinpu Wang]

On Tue, Apr 22, 2025 at 4:27 PM Christoph Hellwig <hch@lst.de> wrote:
>
> Use the bio_add_virt_nofail to add a single kernel virtual address
> to a bio as that can't fail.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Acked-by: Jack Wang <jinpu.wang@ionos.com>
> ---
>  drivers/block/rnbd/rnbd-srv.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c
> index 2ee6e9bd4e28..2df8941a6b14 100644
> --- a/drivers/block/rnbd/rnbd-srv.c
> +++ b/drivers/block/rnbd/rnbd-srv.c
> @@ -147,12 +147,7 @@ static int process_rdma(struct rnbd_srv_session *srv_sess,
>
>         bio = bio_alloc(file_bdev(sess_dev->bdev_file), 1,
>                         rnbd_to_bio_flags(le32_to_cpu(msg->rw)), GFP_KERNEL);
> -       if (bio_add_page(bio, virt_to_page(data), datalen,
> -                       offset_in_page(data)) != datalen) {
> -               rnbd_srv_err_rl(sess_dev, "Failed to map data to bio\n");
> -               err = -EINVAL;
> -               goto bio_put;
> -       }
> +       bio_add_virt_nofail(bio, data, datalen);
>
>         bio->bi_opf = rnbd_to_bio_flags(le32_to_cpu(msg->rw));
>         if (bio_has_data(bio) &&
> --
> 2.47.2
>

Re: [PATCH 10/17] rnbd-srv: use bio_add_virt_nofail

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 11/17] xfs: simplify xfs_buf_submit_bio

[Christoph Hellwig]

Convert the __bio_add_page(..., virt_to_page(), ...) pattern to the
bio_add_virt_nofail helper implementing it and use bio_add_vmalloc
to insulate xfs from the details of adding vmalloc memory to a bio.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_buf.c | 27 ++++++++-------------------
 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 1a2b3f06fa71..042a738b7fda 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1339,37 +1339,26 @@ xfs_buf_submit_bio(
 
 	if (is_vmalloc_addr(bp->b_addr)) {
 		unsigned int	size = BBTOB(bp->b_length);
-		unsigned int	alloc_size = roundup(size, PAGE_SIZE);
 		void		*data = bp->b_addr;
+		unsigned int	added;
 
-		bio = bio_alloc(bp->b_target->bt_bdev, alloc_size >> PAGE_SHIFT,
-				xfs_buf_bio_op(bp), GFP_NOIO);
+		bio = bio_alloc(bp->b_target->bt_bdev,
+				howmany(size, PAGE_SIZE), xfs_buf_bio_op(bp),
+				GFP_NOIO);
 
 		do {
-			unsigned int	len = min(size, PAGE_SIZE);
-
-			ASSERT(offset_in_page(data) == 0);
-			__bio_add_page(bio, vmalloc_to_page(data), len, 0);
-			data += len;
-			size -= len;
+			added = bio_add_vmalloc(bio, data, size);
+			data += added;
+			size -= added;
 		} while (size);
-
-		flush_kernel_vmap_range(bp->b_addr, alloc_size);
 	} else {
 		/*
 		 * Single folio or slab allocation.  Must be contiguous and thus
 		 * only a single bvec is needed.
-		 *
-		 * This uses the page based bio add helper for now as that is
-		 * the lowest common denominator between folios and slab
-		 * allocations.  To be replaced with a better block layer
-		 * helper soon (hopefully).
 		 */
 		bio = bio_alloc(bp->b_target->bt_bdev, 1, xfs_buf_bio_op(bp),
 				GFP_NOIO);
-		__bio_add_page(bio, virt_to_page(bp->b_addr),
-				BBTOB(bp->b_length),
-				offset_in_page(bp->b_addr));
+		bio_add_virt_nofail(bio, bp->b_addr, BBTOB(bp->b_length));
 	}
 
 	bio->bi_private = bp;
-- 
2.47.2

Re: [PATCH 11/17] xfs: simplify xfs_buf_submit_bio

[Damien Le Moal]

On 4/22/25 23:26, Christoph Hellwig wrote:
> Convert the __bio_add_page(..., virt_to_page(), ...) pattern to the
> bio_add_virt_nofail helper implementing it and use bio_add_vmalloc
> to insulate xfs from the details of adding vmalloc memory to a bio.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks OK to me.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 11/17] xfs: simplify xfs_buf_submit_bio

[Darrick J. Wong]

On Tue, Apr 22, 2025 at 04:26:12PM +0200, Christoph Hellwig wrote:
> Convert the __bio_add_page(..., virt_to_page(), ...) pattern to the
> bio_add_virt_nofail helper implementing it and use bio_add_vmalloc
> to insulate xfs from the details of adding vmalloc memory to a bio.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

That reads much more cleanly now :)
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_buf.c | 27 ++++++++-------------------
>  1 file changed, 8 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 1a2b3f06fa71..042a738b7fda 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1339,37 +1339,26 @@ xfs_buf_submit_bio(
>  
>  	if (is_vmalloc_addr(bp->b_addr)) {
>  		unsigned int	size = BBTOB(bp->b_length);
> -		unsigned int	alloc_size = roundup(size, PAGE_SIZE);
>  		void		*data = bp->b_addr;
> +		unsigned int	added;
>  
> -		bio = bio_alloc(bp->b_target->bt_bdev, alloc_size >> PAGE_SHIFT,
> -				xfs_buf_bio_op(bp), GFP_NOIO);
> +		bio = bio_alloc(bp->b_target->bt_bdev,
> +				howmany(size, PAGE_SIZE), xfs_buf_bio_op(bp),
> +				GFP_NOIO);
>  
>  		do {
> -			unsigned int	len = min(size, PAGE_SIZE);
> -
> -			ASSERT(offset_in_page(data) == 0);
> -			__bio_add_page(bio, vmalloc_to_page(data), len, 0);
> -			data += len;
> -			size -= len;
> +			added = bio_add_vmalloc(bio, data, size);
> +			data += added;
> +			size -= added;
>  		} while (size);
> -
> -		flush_kernel_vmap_range(bp->b_addr, alloc_size);
>  	} else {
>  		/*
>  		 * Single folio or slab allocation.  Must be contiguous and thus
>  		 * only a single bvec is needed.
> -		 *
> -		 * This uses the page based bio add helper for now as that is
> -		 * the lowest common denominator between folios and slab
> -		 * allocations.  To be replaced with a better block layer
> -		 * helper soon (hopefully).
>  		 */
>  		bio = bio_alloc(bp->b_target->bt_bdev, 1, xfs_buf_bio_op(bp),
>  				GFP_NOIO);
> -		__bio_add_page(bio, virt_to_page(bp->b_addr),
> -				BBTOB(bp->b_length),
> -				offset_in_page(bp->b_addr));
> +		bio_add_virt_nofail(bio, bp->b_addr, BBTOB(bp->b_length));
>  	}
>  
>  	bio->bi_private = bp;
> -- 
> 2.47.2
> 
> 

[PATCH 12/17] xfs: simplify xfs_rw_bdev

[Christoph Hellwig]

Delegate to bdev_rw_virt when operating on non-vmalloc memory and use
bio_add_vmalloc to insulate xfs from the details of adding vmalloc memory
to a bio.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_bio_io.c | 30 ++++++++++++------------------
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/fs/xfs/xfs_bio_io.c b/fs/xfs/xfs_bio_io.c
index fe21c76f75b8..98ad42b0271e 100644
--- a/fs/xfs/xfs_bio_io.c
+++ b/fs/xfs/xfs_bio_io.c
@@ -18,42 +18,36 @@ xfs_rw_bdev(
 	enum req_op		op)
 
 {
-	unsigned int		is_vmalloc = is_vmalloc_addr(data);
-	unsigned int		left = count;
+	unsigned int		done = 0, added;
 	int			error;
 	struct bio		*bio;
 
-	if (is_vmalloc && op == REQ_OP_WRITE)
-		flush_kernel_vmap_range(data, count);
+	op |= REQ_META | REQ_SYNC;
+	if (!is_vmalloc_addr(data))
+		return bdev_rw_virt(bdev, sector, data, count, op);
 
-	bio = bio_alloc(bdev, bio_max_vecs(left), op | REQ_META | REQ_SYNC,
-			GFP_KERNEL);
+	bio = bio_alloc(bdev, bio_max_vecs(count), op, GFP_KERNEL);
 	bio->bi_iter.bi_sector = sector;
 
 	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) {
+		added = bio_add_vmalloc(bio, data + done, count - done);
+		if (!added) {
 			struct bio	*prev = bio;
 
-			bio = bio_alloc(prev->bi_bdev, bio_max_vecs(left),
+			bio = bio_alloc(prev->bi_bdev,
+					bio_max_vecs(count - done),
 					prev->bi_opf, GFP_KERNEL);
 			bio->bi_iter.bi_sector = bio_end_sector(prev);
 			bio_chain(prev, bio);
-
 			submit_bio(prev);
 		}
-
-		data += len;
-		left -= len;
-	} while (left > 0);
+		done += added;
+	} while (done < count);
 
 	error = submit_bio_wait(bio);
 	bio_put(bio);
 
-	if (is_vmalloc && op == REQ_OP_READ)
+	if (op == REQ_OP_READ)
 		invalidate_kernel_vmap_range(data, count);
 	return error;
 }
-- 
2.47.2

Re: [PATCH 12/17] xfs: simplify xfs_rw_bdev

[Damien Le Moal]

On 4/22/25 23:26, Christoph Hellwig wrote:
> Delegate to bdev_rw_virt when operating on non-vmalloc memory and use
> bio_add_vmalloc to insulate xfs from the details of adding vmalloc memory
> to a bio.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks OK to me.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 12/17] xfs: simplify xfs_rw_bdev

[Darrick J. Wong]

On Tue, Apr 22, 2025 at 04:26:13PM +0200, Christoph Hellwig wrote:
> Delegate to bdev_rw_virt when operating on non-vmalloc memory and use
> bio_add_vmalloc to insulate xfs from the details of adding vmalloc memory
> to a bio.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_bio_io.c | 30 ++++++++++++------------------
>  1 file changed, 12 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/xfs/xfs_bio_io.c b/fs/xfs/xfs_bio_io.c
> index fe21c76f75b8..98ad42b0271e 100644
> --- a/fs/xfs/xfs_bio_io.c
> +++ b/fs/xfs/xfs_bio_io.c
> @@ -18,42 +18,36 @@ xfs_rw_bdev(
>  	enum req_op		op)
>  
>  {
> -	unsigned int		is_vmalloc = is_vmalloc_addr(data);
> -	unsigned int		left = count;
> +	unsigned int		done = 0, added;
>  	int			error;
>  	struct bio		*bio;
>  
> -	if (is_vmalloc && op == REQ_OP_WRITE)
> -		flush_kernel_vmap_range(data, count);
> +	op |= REQ_META | REQ_SYNC;
> +	if (!is_vmalloc_addr(data))
> +		return bdev_rw_virt(bdev, sector, data, count, op);
>  
> -	bio = bio_alloc(bdev, bio_max_vecs(left), op | REQ_META | REQ_SYNC,
> -			GFP_KERNEL);
> +	bio = bio_alloc(bdev, bio_max_vecs(count), op, GFP_KERNEL);
>  	bio->bi_iter.bi_sector = sector;
>  
>  	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) {
> +		added = bio_add_vmalloc(bio, data + done, count - done);
> +		if (!added) {
>  			struct bio	*prev = bio;
>  
> -			bio = bio_alloc(prev->bi_bdev, bio_max_vecs(left),
> +			bio = bio_alloc(prev->bi_bdev,
> +					bio_max_vecs(count - done),
>  					prev->bi_opf, GFP_KERNEL);
>  			bio->bi_iter.bi_sector = bio_end_sector(prev);
>  			bio_chain(prev, bio);
> -
>  			submit_bio(prev);
>  		}
> -
> -		data += len;
> -		left -= len;
> -	} while (left > 0);
> +		done += added;
> +	} while (done < count);
>  
>  	error = submit_bio_wait(bio);
>  	bio_put(bio);
>  
> -	if (is_vmalloc && op == REQ_OP_READ)
> +	if (op == REQ_OP_READ)
>  		invalidate_kernel_vmap_range(data, count);
>  	return error;
>  }
> -- 
> 2.47.2
> 
> 

[PATCH 13/17] btrfs: use bdev_rw_virt in scrub_one_super

[Christoph Hellwig]

Replace the code building a bio from a kernel direct map address and
submitting it synchronously with the bdev_rw_virt helper.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/scrub.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index 2c5edcee9450..7bdb2bc0a212 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -2770,17 +2770,11 @@ static int scrub_one_super(struct scrub_ctx *sctx, struct btrfs_device *dev,
 			   struct page *page, u64 physical, u64 generation)
 {
 	struct btrfs_fs_info *fs_info = sctx->fs_info;
-	struct bio_vec bvec;
-	struct bio bio;
 	struct btrfs_super_block *sb = page_address(page);
 	int ret;
 
-	bio_init(&bio, dev->bdev, &bvec, 1, REQ_OP_READ);
-	bio.bi_iter.bi_sector = physical >> SECTOR_SHIFT;
-	__bio_add_page(&bio, page, BTRFS_SUPER_INFO_SIZE, 0);
-	ret = submit_bio_wait(&bio);
-	bio_uninit(&bio);
-
+	ret = bdev_rw_virt(dev->bdev, physical >> SECTOR_SHIFT, sb,
+			BTRFS_SUPER_INFO_SIZE, REQ_OP_READ);
 	if (ret < 0)
 		return ret;
 	ret = btrfs_check_super_csum(fs_info, sb);
-- 
2.47.2

Re: [PATCH 13/17] btrfs: use bdev_rw_virt in scrub_one_super

[Damien Le Moal]

On 4/22/25 23:26, Christoph Hellwig wrote:
> Replace the code building a bio from a kernel direct map address and
> submitting it synchronously with the bdev_rw_virt helper.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks OK to me.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 13/17] btrfs: use bdev_rw_virt in scrub_one_super

[Johannes Thumshirn]

Looks good to me,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 13/17] btrfs: use bdev_rw_virt in scrub_one_super

[Qu Wenruo]

在 2025/4/22 23:56, Christoph Hellwig 写道:
> Replace the code building a bio from a kernel direct map address and
> submitting it synchronously with the bdev_rw_virt helper.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Qu Wenruo <wqu@suse.com>

In fact I'm waiting for this helper to refactor how we do super block 
read/write, to get rid of any page cache usage of the block device.

Thanks,
Qu

> ---
>   fs/btrfs/scrub.c | 10 ++--------
>   1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> index 2c5edcee9450..7bdb2bc0a212 100644
> --- a/fs/btrfs/scrub.c
> +++ b/fs/btrfs/scrub.c
> @@ -2770,17 +2770,11 @@ static int scrub_one_super(struct scrub_ctx *sctx, struct btrfs_device *dev,
>   			   struct page *page, u64 physical, u64 generation)
>   {
>   	struct btrfs_fs_info *fs_info = sctx->fs_info;
> -	struct bio_vec bvec;
> -	struct bio bio;
>   	struct btrfs_super_block *sb = page_address(page);
>   	int ret;
>   
> -	bio_init(&bio, dev->bdev, &bvec, 1, REQ_OP_READ);
> -	bio.bi_iter.bi_sector = physical >> SECTOR_SHIFT;
> -	__bio_add_page(&bio, page, BTRFS_SUPER_INFO_SIZE, 0);
> -	ret = submit_bio_wait(&bio);
> -	bio_uninit(&bio);
> -
> +	ret = bdev_rw_virt(dev->bdev, physical >> SECTOR_SHIFT, sb,
> +			BTRFS_SUPER_INFO_SIZE, REQ_OP_READ);
>   	if (ret < 0)
>   		return ret;
>   	ret = btrfs_check_super_csum(fs_info, sb);

[PATCH 14/17] hfsplus: use bdev_rw_virt in hfsplus_submit_bio

[Christoph Hellwig]

Replace the code building a bio from a kernel direct map address and
submitting it synchronously with the bdev_rw_virt helper.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/hfsplus/wrapper.c | 46 +++++++++-----------------------------------
 1 file changed, 9 insertions(+), 37 deletions(-)

diff --git a/fs/hfsplus/wrapper.c b/fs/hfsplus/wrapper.c
index 74801911bc1c..30cf4fe78b3d 100644
--- a/fs/hfsplus/wrapper.c
+++ b/fs/hfsplus/wrapper.c
@@ -48,47 +48,19 @@ struct hfsplus_wd {
 int hfsplus_submit_bio(struct super_block *sb, sector_t sector,
 		       void *buf, void **data, blk_opf_t opf)
 {
-	const enum req_op op = opf & REQ_OP_MASK;
-	struct bio *bio;
-	int ret = 0;
-	u64 io_size;
-	loff_t start;
-	int offset;
+	u64 io_size = hfsplus_min_io_size(sb);
+	loff_t start = (loff_t)sector << HFSPLUS_SECTOR_SHIFT;
+	int offset = start & (io_size - 1);
+
+	if ((opf & REQ_OP_MASK) != REQ_OP_WRITE && data)
+		*data = (u8 *)buf + offset;
 
 	/*
-	 * Align sector to hardware sector size and find offset. We
-	 * assume that io_size is a power of two, which _should_
-	 * be true.
+	 * Align sector to hardware sector size and find offset. We assume that
+	 * io_size is a power of two, which _should_ be true.
 	 */
-	io_size = hfsplus_min_io_size(sb);
-	start = (loff_t)sector << HFSPLUS_SECTOR_SHIFT;
-	offset = start & (io_size - 1);
 	sector &= ~((io_size >> HFSPLUS_SECTOR_SHIFT) - 1);
-
-	bio = bio_alloc(sb->s_bdev, 1, opf, GFP_NOIO);
-	bio->bi_iter.bi_sector = sector;
-
-	if (op != REQ_OP_WRITE && data)
-		*data = (u8 *)buf + offset;
-
-	while (io_size > 0) {
-		unsigned int page_offset = offset_in_page(buf);
-		unsigned int len = min_t(unsigned int, PAGE_SIZE - page_offset,
-					 io_size);
-
-		ret = bio_add_page(bio, virt_to_page(buf), len, page_offset);
-		if (ret != len) {
-			ret = -EIO;
-			goto out;
-		}
-		io_size -= len;
-		buf = (u8 *)buf + len;
-	}
-
-	ret = submit_bio_wait(bio);
-out:
-	bio_put(bio);
-	return ret < 0 ? ret : 0;
+	return bdev_rw_virt(sb->s_bdev, sector, buf, io_size, opf);
 }
 
 static int hfsplus_read_mdb(void *bufptr, struct hfsplus_wd *wd)
-- 
2.47.2

Re: [PATCH 14/17] hfsplus: use bdev_rw_virt in hfsplus_submit_bio

[Damien Le Moal]

On 4/22/25 23:26, Christoph Hellwig wrote:
> Replace the code building a bio from a kernel direct map address and
> submitting it synchronously with the bdev_rw_virt helper.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks OK to me.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 14/17] hfsplus: use bdev_rw_virt in hfsplus_submit_bio

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 15/17] gfs2: use bdev_rw_virt in gfs2_read_super

[Christoph Hellwig]

Switch gfs2_read_super to allocate the superblock buffer using kmalloc
which falls back to the page allocator for PAGE_SIZE allocation but
gives us a kernel virtual address and then use bdev_rw_virt to perform
the synchronous read into it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/gfs2/ops_fstype.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index e83d293c3614..7c1014ba7ac7 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -226,28 +226,22 @@ static void gfs2_sb_in(struct gfs2_sbd *sdp, const struct gfs2_sb *str)
 
 static int gfs2_read_super(struct gfs2_sbd *sdp, sector_t sector, int silent)
 {
-	struct super_block *sb = sdp->sd_vfs;
-	struct page *page;
-	struct bio_vec bvec;
-	struct bio bio;
+	struct gfs2_sb *sb;
 	int err;
 
-	page = alloc_page(GFP_KERNEL);
-	if (unlikely(!page))
+	sb = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	if (unlikely(!sb))
 		return -ENOMEM;
-
-	bio_init(&bio, sb->s_bdev, &bvec, 1, REQ_OP_READ | REQ_META);
-	bio.bi_iter.bi_sector = sector * (sb->s_blocksize >> 9);
-	__bio_add_page(&bio, page, PAGE_SIZE, 0);
-
-	err = submit_bio_wait(&bio);
+	err = bdev_rw_virt(sdp->sd_vfs->s_bdev,
+			sector * (sdp->sd_vfs->s_blocksize >> 9), sb, PAGE_SIZE,
+			REQ_OP_READ | REQ_META);
 	if (err) {
 		pr_warn("error %d reading superblock\n", err);
-		__free_page(page);
+		kfree(sb);
 		return err;
 	}
-	gfs2_sb_in(sdp, page_address(page));
-	__free_page(page);
+	gfs2_sb_in(sdp, sb);
+	kfree(sb);
 	return gfs2_check_sb(sdp, silent);
 }
 
-- 
2.47.2

Re: [PATCH 15/17] gfs2: use bdev_rw_virt in gfs2_read_super

[Damien Le Moal]

On 4/22/25 23:26, Christoph Hellwig wrote:
> Switch gfs2_read_super to allocate the superblock buffer using kmalloc
> which falls back to the page allocator for PAGE_SIZE allocation but
> gives us a kernel virtual address and then use bdev_rw_virt to perform
> the synchronous read into it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

One nit below.

> ---
>  fs/gfs2/ops_fstype.c | 24 +++++++++---------------
>  1 file changed, 9 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> index e83d293c3614..7c1014ba7ac7 100644
> --- a/fs/gfs2/ops_fstype.c
> +++ b/fs/gfs2/ops_fstype.c
> @@ -226,28 +226,22 @@ static void gfs2_sb_in(struct gfs2_sbd *sdp, const struct gfs2_sb *str)
>  
>  static int gfs2_read_super(struct gfs2_sbd *sdp, sector_t sector, int silent)
>  {
> -	struct super_block *sb = sdp->sd_vfs;
> -	struct page *page;
> -	struct bio_vec bvec;
> -	struct bio bio;
> +	struct gfs2_sb *sb;
>  	int err;
>  
> -	page = alloc_page(GFP_KERNEL);
> -	if (unlikely(!page))
> +	sb = kmalloc(PAGE_SIZE, GFP_KERNEL);
> +	if (unlikely(!sb))
>  		return -ENOMEM;
> -
> -	bio_init(&bio, sb->s_bdev, &bvec, 1, REQ_OP_READ | REQ_META);
> -	bio.bi_iter.bi_sector = sector * (sb->s_blocksize >> 9);
> -	__bio_add_page(&bio, page, PAGE_SIZE, 0);
> -
> -	err = submit_bio_wait(&bio);
> +	err = bdev_rw_virt(sdp->sd_vfs->s_bdev,
> +			sector * (sdp->sd_vfs->s_blocksize >> 9), sb, PAGE_SIZE,

While at it, use SECTOR_SHIFT here ?

> +			REQ_OP_READ | REQ_META);
>  	if (err) {
>  		pr_warn("error %d reading superblock\n", err);
> -		__free_page(page);
> +		kfree(sb);
>  		return err;
>  	}
> -	gfs2_sb_in(sdp, page_address(page));
> -	__free_page(page);
> +	gfs2_sb_in(sdp, sb);
> +	kfree(sb);
>  	return gfs2_check_sb(sdp, silent);
>  }
>  


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 15/17] gfs2: use bdev_rw_virt in gfs2_read_super

[Andreas Gruenbacher]

On Thu, Apr 24, 2025 at 8:23 AM Damien Le Moal <dlemoal@kernel.org> wrote:
> On 4/22/25 23:26, Christoph Hellwig wrote:
> > Switch gfs2_read_super to allocate the superblock buffer using kmalloc
> > which falls back to the page allocator for PAGE_SIZE allocation but
> > gives us a kernel virtual address and then use bdev_rw_virt to perform
> > the synchronous read into it.
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
>
> One nit below.
>
> > ---
> >  fs/gfs2/ops_fstype.c | 24 +++++++++---------------
> >  1 file changed, 9 insertions(+), 15 deletions(-)
> >
> > diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> > index e83d293c3614..7c1014ba7ac7 100644
> > --- a/fs/gfs2/ops_fstype.c
> > +++ b/fs/gfs2/ops_fstype.c
> > @@ -226,28 +226,22 @@ static void gfs2_sb_in(struct gfs2_sbd *sdp, const struct gfs2_sb *str)
> >
> >  static int gfs2_read_super(struct gfs2_sbd *sdp, sector_t sector, int silent)
> >  {
> > -     struct super_block *sb = sdp->sd_vfs;
> > -     struct page *page;
> > -     struct bio_vec bvec;
> > -     struct bio bio;
> > +     struct gfs2_sb *sb;
> >       int err;
> >
> > -     page = alloc_page(GFP_KERNEL);
> > -     if (unlikely(!page))
> > +     sb = kmalloc(PAGE_SIZE, GFP_KERNEL);
> > +     if (unlikely(!sb))
> >               return -ENOMEM;
> > -
> > -     bio_init(&bio, sb->s_bdev, &bvec, 1, REQ_OP_READ | REQ_META);
> > -     bio.bi_iter.bi_sector = sector * (sb->s_blocksize >> 9);
> > -     __bio_add_page(&bio, page, PAGE_SIZE, 0);
> > -
> > -     err = submit_bio_wait(&bio);
> > +     err = bdev_rw_virt(sdp->sd_vfs->s_bdev,
> > +                     sector * (sdp->sd_vfs->s_blocksize >> 9), sb, PAGE_SIZE,
>
> While at it, use SECTOR_SHIFT here ?

This is hardcoded in several places; I can clean it up separately.

> > +                     REQ_OP_READ | REQ_META);
> >       if (err) {
> >               pr_warn("error %d reading superblock\n", err);
> > -             __free_page(page);
> > +             kfree(sb);
> >               return err;
> >       }
> > -     gfs2_sb_in(sdp, page_address(page));
> > -     __free_page(page);
> > +     gfs2_sb_in(sdp, sb);
> > +     kfree(sb);
> >       return gfs2_check_sb(sdp, silent);
> >  }
> >

Reviewed-by: Andreas Gruenbacher <agruenba@redhat.com>

Thanks,
Andreas

[PATCH 16/17] zonefs: use bdev_rw_virt in zonefs_read_super

[Christoph Hellwig]

Switch zonefs_read_super to allocate the superblock buffer using kmalloc
which falls back to the page allocator for PAGE_SIZE allocation but
gives us a kernel virtual address and then use bdev_rw_virt to perform
the synchronous read into it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/zonefs/super.c | 34 ++++++++++++----------------------
 1 file changed, 12 insertions(+), 22 deletions(-)

diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c
index faf1eb87895d..d165eb979f21 100644
--- a/fs/zonefs/super.c
+++ b/fs/zonefs/super.c
@@ -1111,28 +1111,19 @@ static int zonefs_read_super(struct super_block *sb)
 	struct zonefs_sb_info *sbi = ZONEFS_SB(sb);
 	struct zonefs_super *super;
 	u32 crc, stored_crc;
-	struct page *page;
-	struct bio_vec bio_vec;
-	struct bio bio;
 	int ret;
 
-	page = alloc_page(GFP_KERNEL);
-	if (!page)
+	super = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!super)
 		return -ENOMEM;
 
-	bio_init(&bio, sb->s_bdev, &bio_vec, 1, REQ_OP_READ);
-	bio.bi_iter.bi_sector = 0;
-	__bio_add_page(&bio, page, PAGE_SIZE, 0);
-
-	ret = submit_bio_wait(&bio);
+	ret = bdev_rw_virt(sb->s_bdev, 0, super, PAGE_SIZE, REQ_OP_READ);
 	if (ret)
-		goto free_page;
-
-	super = page_address(page);
+		goto free_super;
 
 	ret = -EINVAL;
 	if (le32_to_cpu(super->s_magic) != ZONEFS_MAGIC)
-		goto free_page;
+		goto free_super;
 
 	stored_crc = le32_to_cpu(super->s_crc);
 	super->s_crc = 0;
@@ -1140,14 +1131,14 @@ static int zonefs_read_super(struct super_block *sb)
 	if (crc != stored_crc) {
 		zonefs_err(sb, "Invalid checksum (Expected 0x%08x, got 0x%08x)",
 			   crc, stored_crc);
-		goto free_page;
+		goto free_super;
 	}
 
 	sbi->s_features = le64_to_cpu(super->s_features);
 	if (sbi->s_features & ~ZONEFS_F_DEFINED_FEATURES) {
 		zonefs_err(sb, "Unknown features set 0x%llx\n",
 			   sbi->s_features);
-		goto free_page;
+		goto free_super;
 	}
 
 	if (sbi->s_features & ZONEFS_F_UID) {
@@ -1155,7 +1146,7 @@ static int zonefs_read_super(struct super_block *sb)
 				       le32_to_cpu(super->s_uid));
 		if (!uid_valid(sbi->s_uid)) {
 			zonefs_err(sb, "Invalid UID feature\n");
-			goto free_page;
+			goto free_super;
 		}
 	}
 
@@ -1164,7 +1155,7 @@ static int zonefs_read_super(struct super_block *sb)
 				       le32_to_cpu(super->s_gid));
 		if (!gid_valid(sbi->s_gid)) {
 			zonefs_err(sb, "Invalid GID feature\n");
-			goto free_page;
+			goto free_super;
 		}
 	}
 
@@ -1173,15 +1164,14 @@ static int zonefs_read_super(struct super_block *sb)
 
 	if (memchr_inv(super->s_reserved, 0, sizeof(super->s_reserved))) {
 		zonefs_err(sb, "Reserved area is being used\n");
-		goto free_page;
+		goto free_super;
 	}
 
 	import_uuid(&sbi->s_uuid, super->s_uuid);
 	ret = 0;
 
-free_page:
-	__free_page(page);
-
+free_super:
+	kfree(super);
 	return ret;
 }
 
-- 
2.47.2

Re: [PATCH 16/17] zonefs: use bdev_rw_virt in zonefs_read_super

[Damien Le Moal]

On 4/22/25 23:26, Christoph Hellwig wrote:
> Switch zonefs_read_super to allocate the superblock buffer using kmalloc
> which falls back to the page allocator for PAGE_SIZE allocation but
> gives us a kernel virtual address and then use bdev_rw_virt to perform
> the synchronous read into it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Acked-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 16/17] zonefs: use bdev_rw_virt in zonefs_read_super

[Johannes Thumshirn]

On 22.04.25 16:31, Christoph Hellwig wrote:
> +	super = kmalloc(PAGE_SIZE, GFP_KERNEL);
> +	if (!super)

[...]

> +	ret = bdev_rw_virt(sb->s_bdev, 0, super, PAGE_SIZE, REQ_OP_READ);

Can we change these two PAGE_SIZE into ZONEFS_SUPER_SIZE which is 
semantically more correct?

Other than that,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 16/17] zonefs: use bdev_rw_virt in zonefs_read_super

[hch]

On Tue, Apr 29, 2025 at 11:44:37AM +0000, Johannes Thumshirn wrote:
> On 22.04.25 16:31, Christoph Hellwig wrote:
> > +	super = kmalloc(PAGE_SIZE, GFP_KERNEL);
> > +	if (!super)
> 
> [...]
> 
> > +	ret = bdev_rw_virt(sb->s_bdev, 0, super, PAGE_SIZE, REQ_OP_READ);
> 
> Can we change these two PAGE_SIZE into ZONEFS_SUPER_SIZE which is 
> semantically more correct?

I'd like to keep this change mechanical, even if I'm in violent agreement
about using the right constants.

[PATCH 17/17] PM: hibernate: split and simplify hib_submit_io

[Christoph Hellwig]

Split hib_submit_io into a sync and async version.  The sync version is
a small wrapper around bdev_rw_virt which implements all the logic to
add a kernel direct mapping range to a bio and synchronously submits it,
while the async version is slightly simplified using the
bio_add_virt_nofail for adding the single range.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 kernel/power/swap.c | 103 +++++++++++++++++++-------------------------
 1 file changed, 45 insertions(+), 58 deletions(-)

diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index 80ff5f933a62..ad13c461b657 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -268,35 +268,26 @@ static void hib_end_io(struct bio *bio)
 	bio_put(bio);
 }
 
-static int hib_submit_io(blk_opf_t opf, pgoff_t page_off, void *addr,
+static int hib_submit_io_sync(blk_opf_t opf, pgoff_t page_off, void *addr)
+{
+	return bdev_rw_virt(file_bdev(hib_resume_bdev_file),
+			page_off * (PAGE_SIZE >> 9), addr, PAGE_SIZE, opf);
+}
+
+static int hib_submit_io_async(blk_opf_t opf, pgoff_t page_off, void *addr,
 			 struct hib_bio_batch *hb)
 {
-	struct page *page = virt_to_page(addr);
 	struct bio *bio;
-	int error = 0;
 
 	bio = bio_alloc(file_bdev(hib_resume_bdev_file), 1, opf,
 			GFP_NOIO | __GFP_HIGH);
 	bio->bi_iter.bi_sector = page_off * (PAGE_SIZE >> 9);
-
-	if (bio_add_page(bio, page, PAGE_SIZE, 0) < PAGE_SIZE) {
-		pr_err("Adding page to bio failed at %llu\n",
-		       (unsigned long long)bio->bi_iter.bi_sector);
-		bio_put(bio);
-		return -EFAULT;
-	}
-
-	if (hb) {
-		bio->bi_end_io = hib_end_io;
-		bio->bi_private = hb;
-		atomic_inc(&hb->count);
-		submit_bio(bio);
-	} else {
-		error = submit_bio_wait(bio);
-		bio_put(bio);
-	}
-
-	return error;
+	bio_add_virt_nofail(bio, addr, PAGE_SIZE);
+	bio->bi_end_io = hib_end_io;
+	bio->bi_private = hb;
+	atomic_inc(&hb->count);
+	submit_bio(bio);
+	return 0;
 }
 
 static int hib_wait_io(struct hib_bio_batch *hb)
@@ -316,7 +307,7 @@ static int mark_swapfiles(struct swap_map_handle *handle, unsigned int flags)
 {
 	int error;
 
-	hib_submit_io(REQ_OP_READ, swsusp_resume_block, swsusp_header, NULL);
+	hib_submit_io_sync(REQ_OP_READ, swsusp_resume_block, swsusp_header);
 	if (!memcmp("SWAP-SPACE",swsusp_header->sig, 10) ||
 	    !memcmp("SWAPSPACE2",swsusp_header->sig, 10)) {
 		memcpy(swsusp_header->orig_sig,swsusp_header->sig, 10);
@@ -329,8 +320,8 @@ static int mark_swapfiles(struct swap_map_handle *handle, unsigned int flags)
 		swsusp_header->flags = flags;
 		if (flags & SF_CRC32_MODE)
 			swsusp_header->crc32 = handle->crc32;
-		error = hib_submit_io(REQ_OP_WRITE | REQ_SYNC,
-				      swsusp_resume_block, swsusp_header, NULL);
+		error = hib_submit_io_sync(REQ_OP_WRITE | REQ_SYNC,
+				      swsusp_resume_block, swsusp_header);
 	} else {
 		pr_err("Swap header not found!\n");
 		error = -ENODEV;
@@ -380,36 +371,30 @@ static int swsusp_swap_check(void)
 
 static int write_page(void *buf, sector_t offset, struct hib_bio_batch *hb)
 {
+	gfp_t gfp = GFP_NOIO | __GFP_NOWARN | __GFP_NORETRY;
 	void *src;
 	int ret;
 
 	if (!offset)
 		return -ENOSPC;
 
-	if (hb) {
-		src = (void *)__get_free_page(GFP_NOIO | __GFP_NOWARN |
-		                              __GFP_NORETRY);
-		if (src) {
-			copy_page(src, buf);
-		} else {
-			ret = hib_wait_io(hb); /* Free pages */
-			if (ret)
-				return ret;
-			src = (void *)__get_free_page(GFP_NOIO |
-			                              __GFP_NOWARN |
-			                              __GFP_NORETRY);
-			if (src) {
-				copy_page(src, buf);
-			} else {
-				WARN_ON_ONCE(1);
-				hb = NULL;	/* Go synchronous */
-				src = buf;
-			}
-		}
-	} else {
-		src = buf;
+	if (!hb)
+		goto sync_io;
+
+	src = (void *)__get_free_page(gfp);
+	if (!src) {
+		ret = hib_wait_io(hb); /* Free pages */
+		if (ret)
+			return ret;
+		src = (void *)__get_free_page(gfp);
+		if (WARN_ON_ONCE(!src))
+			goto sync_io;
 	}
-	return hib_submit_io(REQ_OP_WRITE | REQ_SYNC, offset, src, hb);
+
+	copy_page(src, buf);
+	return hib_submit_io_async(REQ_OP_WRITE | REQ_SYNC, offset, src, hb);
+sync_io:
+	return hib_submit_io_sync(REQ_OP_WRITE | REQ_SYNC, offset, buf);
 }
 
 static void release_swap_writer(struct swap_map_handle *handle)
@@ -1041,7 +1026,7 @@ static int get_swap_reader(struct swap_map_handle *handle,
 			return -ENOMEM;
 		}
 
-		error = hib_submit_io(REQ_OP_READ, offset, tmp->map, NULL);
+		error = hib_submit_io_sync(REQ_OP_READ, offset, tmp->map);
 		if (error) {
 			release_swap_reader(handle);
 			return error;
@@ -1065,7 +1050,10 @@ static int swap_read_page(struct swap_map_handle *handle, void *buf,
 	offset = handle->cur->entries[handle->k];
 	if (!offset)
 		return -EFAULT;
-	error = hib_submit_io(REQ_OP_READ, offset, buf, hb);
+	if (hb)
+		error = hib_submit_io_async(REQ_OP_READ, offset, buf, hb);
+	else
+		error = hib_submit_io_sync(REQ_OP_READ, offset, buf);
 	if (error)
 		return error;
 	if (++handle->k >= MAP_PAGE_ENTRIES) {
@@ -1590,8 +1578,8 @@ int swsusp_check(bool exclusive)
 				BLK_OPEN_READ, holder, NULL);
 	if (!IS_ERR(hib_resume_bdev_file)) {
 		clear_page(swsusp_header);
-		error = hib_submit_io(REQ_OP_READ, swsusp_resume_block,
-					swsusp_header, NULL);
+		error = hib_submit_io_sync(REQ_OP_READ, swsusp_resume_block,
+					swsusp_header);
 		if (error)
 			goto put;
 
@@ -1599,9 +1587,9 @@ int swsusp_check(bool exclusive)
 			memcpy(swsusp_header->sig, swsusp_header->orig_sig, 10);
 			swsusp_header_flags = swsusp_header->flags;
 			/* Reset swap signature now */
-			error = hib_submit_io(REQ_OP_WRITE | REQ_SYNC,
+			error = hib_submit_io_sync(REQ_OP_WRITE | REQ_SYNC,
 						swsusp_resume_block,
-						swsusp_header, NULL);
+						swsusp_header);
 		} else {
 			error = -EINVAL;
 		}
@@ -1650,13 +1638,12 @@ int swsusp_unmark(void)
 {
 	int error;
 
-	hib_submit_io(REQ_OP_READ, swsusp_resume_block,
-			swsusp_header, NULL);
+	hib_submit_io_sync(REQ_OP_READ, swsusp_resume_block, swsusp_header);
 	if (!memcmp(HIBERNATE_SIG,swsusp_header->sig, 10)) {
 		memcpy(swsusp_header->sig,swsusp_header->orig_sig, 10);
-		error = hib_submit_io(REQ_OP_WRITE | REQ_SYNC,
+		error = hib_submit_io_sync(REQ_OP_WRITE | REQ_SYNC,
 					swsusp_resume_block,
-					swsusp_header, NULL);
+					swsusp_header);
 	} else {
 		pr_err("Cannot find swsusp signature!\n");
 		error = -ENODEV;
-- 
2.47.2

Re: [PATCH 17/17] PM: hibernate: split and simplify hib_submit_io

[Damien Le Moal]

On 4/22/25 23:26, Christoph Hellwig wrote:
> Split hib_submit_io into a sync and async version.  The sync version is
> a small wrapper around bdev_rw_virt which implements all the logic to
> add a kernel direct mapping range to a bio and synchronously submits it,
> while the async version is slightly simplified using the
> bio_add_virt_nofail for adding the single range.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks OK to me.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 17/17] PM: hibernate: split and simplify hib_submit_io

[Rafael J. Wysocki]

On Tue, Apr 22, 2025 at 4:27 PM Christoph Hellwig <hch@lst.de> wrote:
>
> Split hib_submit_io into a sync and async version.  The sync version is
> a small wrapper around bdev_rw_virt which implements all the logic to
> add a kernel direct mapping range to a bio and synchronously submits it,
> while the async version is slightly simplified using the
> bio_add_virt_nofail for adding the single range.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

LGTM, so

Acked-by: Rafael J. Wysocki <rafael@kernel.org>

and please route it as needed along with the rest of the series.

> ---
>  kernel/power/swap.c | 103 +++++++++++++++++++-------------------------
>  1 file changed, 45 insertions(+), 58 deletions(-)
>
> diff --git a/kernel/power/swap.c b/kernel/power/swap.c
> index 80ff5f933a62..ad13c461b657 100644
> --- a/kernel/power/swap.c
> +++ b/kernel/power/swap.c
> @@ -268,35 +268,26 @@ static void hib_end_io(struct bio *bio)
>         bio_put(bio);
>  }
>
> -static int hib_submit_io(blk_opf_t opf, pgoff_t page_off, void *addr,
> +static int hib_submit_io_sync(blk_opf_t opf, pgoff_t page_off, void *addr)
> +{
> +       return bdev_rw_virt(file_bdev(hib_resume_bdev_file),
> +                       page_off * (PAGE_SIZE >> 9), addr, PAGE_SIZE, opf);
> +}
> +
> +static int hib_submit_io_async(blk_opf_t opf, pgoff_t page_off, void *addr,
>                          struct hib_bio_batch *hb)
>  {
> -       struct page *page = virt_to_page(addr);
>         struct bio *bio;
> -       int error = 0;
>
>         bio = bio_alloc(file_bdev(hib_resume_bdev_file), 1, opf,
>                         GFP_NOIO | __GFP_HIGH);
>         bio->bi_iter.bi_sector = page_off * (PAGE_SIZE >> 9);
> -
> -       if (bio_add_page(bio, page, PAGE_SIZE, 0) < PAGE_SIZE) {
> -               pr_err("Adding page to bio failed at %llu\n",
> -                      (unsigned long long)bio->bi_iter.bi_sector);
> -               bio_put(bio);
> -               return -EFAULT;
> -       }
> -
> -       if (hb) {
> -               bio->bi_end_io = hib_end_io;
> -               bio->bi_private = hb;
> -               atomic_inc(&hb->count);
> -               submit_bio(bio);
> -       } else {
> -               error = submit_bio_wait(bio);
> -               bio_put(bio);
> -       }
> -
> -       return error;
> +       bio_add_virt_nofail(bio, addr, PAGE_SIZE);
> +       bio->bi_end_io = hib_end_io;
> +       bio->bi_private = hb;
> +       atomic_inc(&hb->count);
> +       submit_bio(bio);
> +       return 0;
>  }
>
>  static int hib_wait_io(struct hib_bio_batch *hb)
> @@ -316,7 +307,7 @@ static int mark_swapfiles(struct swap_map_handle *handle, unsigned int flags)
>  {
>         int error;
>
> -       hib_submit_io(REQ_OP_READ, swsusp_resume_block, swsusp_header, NULL);
> +       hib_submit_io_sync(REQ_OP_READ, swsusp_resume_block, swsusp_header);
>         if (!memcmp("SWAP-SPACE",swsusp_header->sig, 10) ||
>             !memcmp("SWAPSPACE2",swsusp_header->sig, 10)) {
>                 memcpy(swsusp_header->orig_sig,swsusp_header->sig, 10);
> @@ -329,8 +320,8 @@ static int mark_swapfiles(struct swap_map_handle *handle, unsigned int flags)
>                 swsusp_header->flags = flags;
>                 if (flags & SF_CRC32_MODE)
>                         swsusp_header->crc32 = handle->crc32;
> -               error = hib_submit_io(REQ_OP_WRITE | REQ_SYNC,
> -                                     swsusp_resume_block, swsusp_header, NULL);
> +               error = hib_submit_io_sync(REQ_OP_WRITE | REQ_SYNC,
> +                                     swsusp_resume_block, swsusp_header);
>         } else {
>                 pr_err("Swap header not found!\n");
>                 error = -ENODEV;
> @@ -380,36 +371,30 @@ static int swsusp_swap_check(void)
>
>  static int write_page(void *buf, sector_t offset, struct hib_bio_batch *hb)
>  {
> +       gfp_t gfp = GFP_NOIO | __GFP_NOWARN | __GFP_NORETRY;
>         void *src;
>         int ret;
>
>         if (!offset)
>                 return -ENOSPC;
>
> -       if (hb) {
> -               src = (void *)__get_free_page(GFP_NOIO | __GFP_NOWARN |
> -                                             __GFP_NORETRY);
> -               if (src) {
> -                       copy_page(src, buf);
> -               } else {
> -                       ret = hib_wait_io(hb); /* Free pages */
> -                       if (ret)
> -                               return ret;
> -                       src = (void *)__get_free_page(GFP_NOIO |
> -                                                     __GFP_NOWARN |
> -                                                     __GFP_NORETRY);
> -                       if (src) {
> -                               copy_page(src, buf);
> -                       } else {
> -                               WARN_ON_ONCE(1);
> -                               hb = NULL;      /* Go synchronous */
> -                               src = buf;
> -                       }
> -               }
> -       } else {
> -               src = buf;
> +       if (!hb)
> +               goto sync_io;
> +
> +       src = (void *)__get_free_page(gfp);
> +       if (!src) {
> +               ret = hib_wait_io(hb); /* Free pages */
> +               if (ret)
> +                       return ret;
> +               src = (void *)__get_free_page(gfp);
> +               if (WARN_ON_ONCE(!src))
> +                       goto sync_io;
>         }
> -       return hib_submit_io(REQ_OP_WRITE | REQ_SYNC, offset, src, hb);
> +
> +       copy_page(src, buf);
> +       return hib_submit_io_async(REQ_OP_WRITE | REQ_SYNC, offset, src, hb);
> +sync_io:
> +       return hib_submit_io_sync(REQ_OP_WRITE | REQ_SYNC, offset, buf);
>  }
>
>  static void release_swap_writer(struct swap_map_handle *handle)
> @@ -1041,7 +1026,7 @@ static int get_swap_reader(struct swap_map_handle *handle,
>                         return -ENOMEM;
>                 }
>
> -               error = hib_submit_io(REQ_OP_READ, offset, tmp->map, NULL);
> +               error = hib_submit_io_sync(REQ_OP_READ, offset, tmp->map);
>                 if (error) {
>                         release_swap_reader(handle);
>                         return error;
> @@ -1065,7 +1050,10 @@ static int swap_read_page(struct swap_map_handle *handle, void *buf,
>         offset = handle->cur->entries[handle->k];
>         if (!offset)
>                 return -EFAULT;
> -       error = hib_submit_io(REQ_OP_READ, offset, buf, hb);
> +       if (hb)
> +               error = hib_submit_io_async(REQ_OP_READ, offset, buf, hb);
> +       else
> +               error = hib_submit_io_sync(REQ_OP_READ, offset, buf);
>         if (error)
>                 return error;
>         if (++handle->k >= MAP_PAGE_ENTRIES) {
> @@ -1590,8 +1578,8 @@ int swsusp_check(bool exclusive)
>                                 BLK_OPEN_READ, holder, NULL);
>         if (!IS_ERR(hib_resume_bdev_file)) {
>                 clear_page(swsusp_header);
> -               error = hib_submit_io(REQ_OP_READ, swsusp_resume_block,
> -                                       swsusp_header, NULL);
> +               error = hib_submit_io_sync(REQ_OP_READ, swsusp_resume_block,
> +                                       swsusp_header);
>                 if (error)
>                         goto put;
>
> @@ -1599,9 +1587,9 @@ int swsusp_check(bool exclusive)
>                         memcpy(swsusp_header->sig, swsusp_header->orig_sig, 10);
>                         swsusp_header_flags = swsusp_header->flags;
>                         /* Reset swap signature now */
> -                       error = hib_submit_io(REQ_OP_WRITE | REQ_SYNC,
> +                       error = hib_submit_io_sync(REQ_OP_WRITE | REQ_SYNC,
>                                                 swsusp_resume_block,
> -                                               swsusp_header, NULL);
> +                                               swsusp_header);
>                 } else {
>                         error = -EINVAL;
>                 }
> @@ -1650,13 +1638,12 @@ int swsusp_unmark(void)
>  {
>         int error;
>
> -       hib_submit_io(REQ_OP_READ, swsusp_resume_block,
> -                       swsusp_header, NULL);
> +       hib_submit_io_sync(REQ_OP_READ, swsusp_resume_block, swsusp_header);
>         if (!memcmp(HIBERNATE_SIG,swsusp_header->sig, 10)) {
>                 memcpy(swsusp_header->sig,swsusp_header->orig_sig, 10);
> -               error = hib_submit_io(REQ_OP_WRITE | REQ_SYNC,
> +               error = hib_submit_io_sync(REQ_OP_WRITE | REQ_SYNC,
>                                         swsusp_resume_block,
> -                                       swsusp_header, NULL);
> +                                       swsusp_header);
>         } else {
>                 pr_err("Cannot find swsusp signature!\n");
>                 error = -ENODEV;
> --
> 2.47.2
>

Re: add more bio helper

[Kent Overstreet]

On Tue, Apr 22, 2025 at 04:26:01PM +0200, Christoph Hellwig wrote:
> Hi all,
> 
> this series adds more block layer helpers to remove boilerplate code when
> adding memory to a bio or to even do the entire synchronous I/O.
> 
> The main aim is to avoid having to convert to a struct page in the caller
> when adding kernel direct mapping or vmalloc memory.

have you seen (bch,bch2)_bio_map?

it's a nicer interface than your bio_add_vmalloc(), and also handles the
if (is_vmalloc_addr())

bio_vmalloc_max_vecs() is good, though

> 
> Diffstat:
>  block/bio.c                   |   57 ++++++++++++++++++++++
>  block/blk-map.c               |  108 ++++++++++++++++--------------------------
>  drivers/block/pktcdvd.c       |    2 
>  drivers/block/rnbd/rnbd-srv.c |    7 --
>  drivers/block/ublk_drv.c      |    3 -
>  drivers/block/virtio_blk.c    |    4 -
>  drivers/md/bcache/super.c     |    3 -
>  drivers/md/dm-bufio.c         |    2 
>  drivers/md/dm-integrity.c     |   16 ++----
>  drivers/nvme/host/core.c      |    2 
>  drivers/scsi/scsi_ioctl.c     |    2 
>  drivers/scsi/scsi_lib.c       |    3 -
>  fs/btrfs/scrub.c              |   10 ---
>  fs/gfs2/ops_fstype.c          |   24 +++------
>  fs/hfsplus/wrapper.c          |   46 +++--------------
>  fs/xfs/xfs_bio_io.c           |   30 ++++-------
>  fs/xfs/xfs_buf.c              |   27 +++-------
>  fs/zonefs/super.c             |   34 ++++---------
>  include/linux/bio.h           |   39 ++++++++++++++-
>  include/linux/blk-mq.h        |    4 -
>  kernel/power/swap.c           |  103 +++++++++++++++++-----------------------
>  21 files changed, 253 insertions(+), 273 deletions(-)

Re: add more bio helper

[Christoph Hellwig]

On Tue, Apr 22, 2025 at 10:47:03AM -0400, Kent Overstreet wrote:
> On Tue, Apr 22, 2025 at 04:26:01PM +0200, Christoph Hellwig wrote:
> > Hi all,
> > 
> > this series adds more block layer helpers to remove boilerplate code when
> > adding memory to a bio or to even do the entire synchronous I/O.
> > 
> > The main aim is to avoid having to convert to a struct page in the caller
> > when adding kernel direct mapping or vmalloc memory.
> 
> have you seen (bch,bch2)_bio_map?

Now I have.

> 
> it's a nicer interface than your bio_add_vmalloc(), and also handles the
> if (is_vmalloc_addr())

Can you explain how it's nicer?

For use with non-vmalloc memory it does a lot of extra work
and generates less optimal bios using more vecs than required, but
maybe that wasn't the point?

For vmalloc it might also build suboptimal bios when using large vmalloc
mappings due to the lack of merging, but I don't think anyone does I/O to
those yet.

It lacks a API description and it or the callers miss handling for VIVT
caches, maybe because of that.

Besides optimizing the direct map case that always needs just one vec
that is also one of the reasons why I want the callers to know about
vmalloc vs non-vmalloc memory.

It also don't support bio chaining or error handling and requires a
single bio that is guaranteed to fit the required number of vectors.

OTOH for callers where that applies it would be nice to have a
helper that loops over bio_add_vmalloc.  I actually had one initially,
but given that I only found two obvious users I dropped it for now.
If we get more we can add one.

Re: add more bio helper

[Kent Overstreet]

On Wed, Apr 23, 2025 at 11:36:21AM +0200, Christoph Hellwig wrote:
> On Tue, Apr 22, 2025 at 10:47:03AM -0400, Kent Overstreet wrote:
> > On Tue, Apr 22, 2025 at 04:26:01PM +0200, Christoph Hellwig wrote:
> > > Hi all,
> > > 
> > > this series adds more block layer helpers to remove boilerplate code when
> > > adding memory to a bio or to even do the entire synchronous I/O.
> > > 
> > > The main aim is to avoid having to convert to a struct page in the caller
> > > when adding kernel direct mapping or vmalloc memory.
> > 
> > have you seen (bch,bch2)_bio_map?
> 
> Now I have.
> 
> > 
> > it's a nicer interface than your bio_add_vmalloc(), and also handles the
> > if (is_vmalloc_addr())
> 
> Can you explain how it's nicer?
> 
> For use with non-vmalloc memory it does a lot of extra work
> and generates less optimal bios using more vecs than required, but
> maybe that wasn't the point?
> 
> For vmalloc it might also build suboptimal bios when using large vmalloc
> mappings due to the lack of merging, but I don't think anyone does I/O to
> those yet.
> 
> It lacks a API description and it or the callers miss handling for VIVT
> caches, maybe because of that.
> 
> Besides optimizing the direct map case that always needs just one vec
> that is also one of the reasons why I want the callers to know about
> vmalloc vs non-vmalloc memory.

Sure, that code predates multipage bvecs - the interface is what I was
referring to.

> It also don't support bio chaining or error handling and requires a
> single bio that is guaranteed to fit the required number of vectors.

Why would bio chaining ever be required? The caller allocates both the
buf and the bio, I've never seen an instance where you'd want that; just
allocate a bio with the correct number of vecs, which your
bio_vmalloc_max_vecs() helps with.

> OTOH for callers where that applies it would be nice to have a
> helper that loops over bio_add_vmalloc.  I actually had one initially,
> but given that I only found two obvious users I dropped it for now.
> If we get more we can add one.

The "abstract over vmalloc and normal physically contigious allocations"
bit that bch2_bio_map() does is the important part.

It's not uncommon to prefer physically contiguous allocations but have a
vmalloc fallback; bcachefs does, and  xfs does with a clever "try the
big allocation if it's cheap, fall back to vmalloc to avoid waiting on
compaction" that I might steal.

is_vmalloc_addr() is also cheap, it's just a pointer comparison (and it
really should be changed to a static inline).

Re: add more bio helper

[Christoph Hellwig]

On Wed, Apr 23, 2025 at 06:37:41AM -0400, Kent Overstreet wrote:
> > It also don't support bio chaining or error handling and requires a
> > single bio that is guaranteed to fit the required number of vectors.
> 
> Why would bio chaining ever be required? The caller allocates both the
> buf and the bio, I've never seen an instance where you'd want that; just
> allocate a bio with the correct number of vecs, which your
> bio_vmalloc_max_vecs() helps with.

If you go beyond 1MB I/O for vmalloc you need it because a single
bio can't hold enough page size chunks.  That is unless you want
to use your own allocation for it and call bio_init which has various
other downsides.

> The "abstract over vmalloc and normal physically contigious allocations"
> bit that bch2_bio_map() does is the important part.
> 
> It's not uncommon to prefer physically contiguous allocations but have a
> vmalloc fallback; bcachefs does, and  xfs does with a clever "try the
> big allocation if it's cheap, fall back to vmalloc to avoid waiting on
> compaction" that I might steal.
> 
> is_vmalloc_addr() is also cheap, it's just a pointer comparison (and it
> really should be changed to a static inline).

The problem with transparent vmalloc handling is that it's not possible.
The magic handling for virtually indexed caches can be hidden on the
submission side, but the completion side also needs to call
invalidate_kernel_vmap_range for reads.  Requiring the caller to know
they deal vmalloc is a way to at least keep that on the radar.

The other benefit is that by forcing different calls it is much
easier to pick the optimal number of bvecs (1) for the non-vmalloc
path, although that is of course also possible without it.

Not for a purely synchronous helper we could handle both, but so far
I've not seen anything but the xfs log recovery code that needs it,
and we'd probably get into needing to pass a bio_set to avoid
deadlock when used deeper in the stack, etc.  I can look into that
if we have more than a single user, but for now it doesn't seem
worth it.

Having a common helper for vmalloc and the kernel direct mapping
is actually how I started, but then I ran into all the issues with
it and with the extremely simple helpers for the direct mapping
which are used a lot, and the more complicated version for vmalloc
which just has a few users instead.

Re: add more bio helper

[Kent Overstreet]

On Wed, Apr 23, 2025 at 06:07:33PM +0200, Christoph Hellwig wrote:
> On Wed, Apr 23, 2025 at 06:37:41AM -0400, Kent Overstreet wrote:
> > > It also don't support bio chaining or error handling and requires a
> > > single bio that is guaranteed to fit the required number of vectors.
> > 
> > Why would bio chaining ever be required? The caller allocates both the
> > buf and the bio, I've never seen an instance where you'd want that; just
> > allocate a bio with the correct number of vecs, which your
> > bio_vmalloc_max_vecs() helps with.
> 
> If you go beyond 1MB I/O for vmalloc you need it because a single
> bio can't hold enough page size chunks.  That is unless you want
> to use your own allocation for it and call bio_init which has various
> other downsides.

Allocating your own bio doesn't allow you to safely exceed the
BIO_MAX_VECS limit - there's places in the io path that need to bounce,
and they all use biosets.

That may be an issue even for non vmalloc bios, unless everything that
bounces has been converted to bounce to a folio of the same order.

> > The "abstract over vmalloc and normal physically contigious allocations"
> > bit that bch2_bio_map() does is the important part.
> > 
> > It's not uncommon to prefer physically contiguous allocations but have a
> > vmalloc fallback; bcachefs does, and  xfs does with a clever "try the
> > big allocation if it's cheap, fall back to vmalloc to avoid waiting on
> > compaction" that I might steal.
> > 
> > is_vmalloc_addr() is also cheap, it's just a pointer comparison (and it
> > really should be changed to a static inline).
> 
> The problem with transparent vmalloc handling is that it's not possible.
> The magic handling for virtually indexed caches can be hidden on the
> submission side, but the completion side also needs to call
> invalidate_kernel_vmap_range for reads.  Requiring the caller to know
> they deal vmalloc is a way to at least keep that on the radar.

yeesh, that's a landmine.

having a separate bio_add_vmalloc as a hint is still a really bad
"solution", unfortunately. And since this is something we don't have
sanitizers or debug code for, and it only shows up on some archs -
that's nasty.

> The other benefit is that by forcing different calls it is much
> easier to pick the optimal number of bvecs (1) for the non-vmalloc
> path, although that is of course also possible without it.

Your bio_vmalloc_max_vecs() could trivially handle both vmalloc and non
vmalloc addresses.

> Not for a purely synchronous helper we could handle both, but so far
> I've not seen anything but the xfs log recovery code that needs it,
> and we'd probably get into needing to pass a bio_set to avoid
> deadlock when used deeper in the stack, etc.  I can look into that
> if we have more than a single user, but for now it doesn't seem
> worth it.

bcache and bcachefs btree buffers can also be vmalloc backed. Possibly
also the prio_set path in bcache, for reading/writing bucket gens, but
I'd have to check.

> Having a common helper for vmalloc and the kernel direct mapping
> is actually how I started, but then I ran into all the issues with
> it and with the extremely simple helpers for the direct mapping
> which are used a lot, and the more complicated version for vmalloc
> which just has a few users instead.

*nod*

What else did you run into? invalidate_kernel_vmap_range() seems like
the only problematic one, given that is_vmalloc_addr() is cheap.

Re: add more bio helper

[Christoph Hellwig]

On Wed, Apr 23, 2025 at 02:02:11PM -0400, Kent Overstreet wrote:
> Allocating your own bio doesn't allow you to safely exceed the
> BIO_MAX_VECS limit - there's places in the io path that need to bounce,
> and they all use biosets.

Yes.  Another reason not to do it, which I don't want to anyway.

But we do have a few places that do it like squashs which we need to
weed out.  And/or finally kill the bounce bufferingreal, which is long
overdue.

> That may be an issue even for non vmalloc bios, unless everything that
> bounces has been converted to bounce to a folio of the same order.

Anything that actually hits the bounce buffering is going to
cause problems because it hasn't kept up with the evolution of
the block layer, and is basically not used for anything relevant.

> > The problem with transparent vmalloc handling is that it's not possible.
> > The magic handling for virtually indexed caches can be hidden on the
> > submission side, but the completion side also needs to call
> > invalidate_kernel_vmap_range for reads.  Requiring the caller to know
> > they deal vmalloc is a way to at least keep that on the radar.
> 
> yeesh, that's a landmine.
> 
> having a separate bio_add_vmalloc as a hint is still a really bad
> "solution", unfortunately. And since this is something we don't have
> sanitizers or debug code for, and it only shows up on some archs -
> that's nasty.

Well, we can't do it in the block stack because that doesn't have the
vmalloc address available.  So the caller has to do it, and having a
very visible sign is the best we can do.  Yes, signs aren't the
best cure for landmines, but they are better than nothing.

> > Not for a purely synchronous helper we could handle both, but so far
> > I've not seen anything but the xfs log recovery code that needs it,
> > and we'd probably get into needing to pass a bio_set to avoid
> > deadlock when used deeper in the stack, etc.  I can look into that
> > if we have more than a single user, but for now it doesn't seem
> > worth it.
> 
> bcache and bcachefs btree buffers can also be vmalloc backed. Possibly
> also the prio_set path in bcache, for reading/writing bucket gens, but
> I'd have to check.

But do you do synchronous I/O, i.e. using sumit_bio_wait on them?

> 
> > Having a common helper for vmalloc and the kernel direct mapping
> > is actually how I started, but then I ran into all the issues with
> > it and with the extremely simple helpers for the direct mapping
> > which are used a lot, and the more complicated version for vmalloc
> > which just has a few users instead.
> 
> *nod*
> 
> What else did you run into? invalidate_kernel_vmap_range() seems like
> the only problematic one, given that is_vmalloc_addr() is cheap.

invalidate_kernel_vmap_range is the major issue that can't be
worked around.  Everything else was mentioned before and can be
summarized as minor inconveniences.

Re: add more bio helper

[Kent Overstreet]

On Thu, Apr 24, 2025 at 10:37:40AM +0200, Christoph Hellwig wrote:
> On Wed, Apr 23, 2025 at 02:02:11PM -0400, Kent Overstreet wrote:
> > Allocating your own bio doesn't allow you to safely exceed the
> > BIO_MAX_VECS limit - there's places in the io path that need to bounce,
> > and they all use biosets.
> 
> Yes.  Another reason not to do it, which I don't want to anyway.
> 
> But we do have a few places that do it like squashs which we need to
> weed out.  And/or finally kill the bounce bufferingreal, which is long
> overdue.
> 
> > That may be an issue even for non vmalloc bios, unless everything that
> > bounces has been converted to bounce to a folio of the same order.
> 
> Anything that actually hits the bounce buffering is going to
> cause problems because it hasn't kept up with the evolution of
> the block layer, and is basically not used for anything relevant.

It's not just block/bounce.c that does bouncing, though.

e.g. bcache has to bounce on a cache miss that will be written to the
cache - we don't want to wait for the write to the backing device to
complete before returning the read completion, and we can't write to the
backing device with the original buffer if it was mapped to userspace.

I'm pretty sure I've seen bouncing in dm and maybe md as well, but it's
been years.

> > > The problem with transparent vmalloc handling is that it's not possible.
> > > The magic handling for virtually indexed caches can be hidden on the
> > > submission side, but the completion side also needs to call
> > > invalidate_kernel_vmap_range for reads.  Requiring the caller to know
> > > they deal vmalloc is a way to at least keep that on the radar.
> > 
> > yeesh, that's a landmine.
> > 
> > having a separate bio_add_vmalloc as a hint is still a really bad
> > "solution", unfortunately. And since this is something we don't have
> > sanitizers or debug code for, and it only shows up on some archs -
> > that's nasty.
> 
> Well, we can't do it in the block stack because that doesn't have the
> vmalloc address available.  So the caller has to do it, and having a
> very visible sign is the best we can do.  Yes, signs aren't the
> best cure for landmines, but they are better than nothing.

Given that only a few architectures need it, maybe sticking the vmalloc
address in struct bio is something we should think about.

Obviously not worth it if only 2-3 codepaths need it, but if vmalloc
fallbacks become more common it's something to think about.

> > > Not for a purely synchronous helper we could handle both, but so far
> > > I've not seen anything but the xfs log recovery code that needs it,
> > > and we'd probably get into needing to pass a bio_set to avoid
> > > deadlock when used deeper in the stack, etc.  I can look into that
> > > if we have more than a single user, but for now it doesn't seem
> > > worth it.
> > 
> > bcache and bcachefs btree buffers can also be vmalloc backed. Possibly
> > also the prio_set path in bcache, for reading/writing bucket gens, but
> > I'd have to check.
> 
> But do you do synchronous I/O, i.e. using sumit_bio_wait on them?

Most btree node reads are synchronous, but not when we're prefetching.