diff mbox series

[v15,01/17] block: Add bio_add_folio()

Message ID 20210719184001.1750630-2-willy@infradead.org (mailing list archive)
State New, archived
Headers show
Series Folio support in block + iomap layers | expand

Commit Message

Matthew Wilcox July 19, 2021, 6:39 p.m. UTC
This is a thin wrapper around bio_add_page().  The main advantage here
is the documentation that the submitter can expect to see folios in the
completion handler, and that stupidly large folios are not supported.
It's not currently possible to allocate stupidly large folios, but if
it ever becomes possible, this function will fail gracefully instead of
doing I/O to the wrong bytes.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 block/bio.c         | 21 +++++++++++++++++++++
 include/linux/bio.h |  3 ++-
 2 files changed, 23 insertions(+), 1 deletion(-)

Comments

Darrick J. Wong July 20, 2021, 1:28 a.m. UTC | #1
On Mon, Jul 19, 2021 at 07:39:45PM +0100, Matthew Wilcox (Oracle) wrote:
> This is a thin wrapper around bio_add_page().  The main advantage here
> is the documentation that the submitter can expect to see folios in the
> completion handler, and that stupidly large folios are not supported.
> It's not currently possible to allocate stupidly large folios, but if
> it ever becomes possible, this function will fail gracefully instead of
> doing I/O to the wrong bytes.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Looks fine to me,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  block/bio.c         | 21 +++++++++++++++++++++
>  include/linux/bio.h |  3 ++-
>  2 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/block/bio.c b/block/bio.c
> index 1fab762e079b..c64e35548fb2 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -933,6 +933,27 @@ int bio_add_page(struct bio *bio, struct page *page,
>  }
>  EXPORT_SYMBOL(bio_add_page);
>  
> +/**
> + * bio_add_folio - Attempt to add part of a folio to a bio.
> + * @bio: Bio to add to.
> + * @folio: Folio to add.
> + * @len: How many bytes from the folio to add.
> + * @off: First byte in this folio to add.
> + *
> + * Always uses the head page of the folio in the bio.  If a submitter only
> + * uses bio_add_folio(), it can count on never seeing tail pages in the
> + * completion routine.  BIOs do not support folios that are 4GiB or larger.
> + *
> + * Return: The number of bytes from this folio added to the bio.
> + */
> +size_t bio_add_folio(struct bio *bio, struct folio *folio, size_t len,
> +		size_t off)
> +{
> +	if (len > UINT_MAX || off > UINT_MAX)
> +		return 0;
> +	return bio_add_page(bio, &folio->page, len, off);
> +}
> +
>  void bio_release_pages(struct bio *bio, bool mark_dirty)
>  {
>  	struct bvec_iter_all iter_all;
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 2203b686e1f0..ade93e2de6a1 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -462,7 +462,8 @@ extern void bio_uninit(struct bio *);
>  extern void bio_reset(struct bio *);
>  void bio_chain(struct bio *, struct bio *);
>  
> -extern int bio_add_page(struct bio *, struct page *, unsigned int,unsigned int);
> +int bio_add_page(struct bio *, struct page *, unsigned len, unsigned off);
> +size_t bio_add_folio(struct bio *, struct folio *, size_t len, size_t off);
>  extern int bio_add_pc_page(struct request_queue *, struct bio *, struct page *,
>  			   unsigned int, unsigned int);
>  int bio_add_zone_append_page(struct bio *bio, struct page *page,
> -- 
> 2.30.2
>
Christoph Hellwig July 20, 2021, 6:42 a.m. UTC | #2
On Mon, Jul 19, 2021 at 07:39:45PM +0100, Matthew Wilcox (Oracle) wrote:
> +/**
> + * bio_add_folio - Attempt to add part of a folio to a bio.
> + * @bio: Bio to add to.
> + * @folio: Folio to add.
> + * @len: How many bytes from the folio to add.
> + * @off: First byte in this folio to add.
> + *
> + * Always uses the head page of the folio in the bio.  If a submitter only
> + * uses bio_add_folio(), it can count on never seeing tail pages in the
> + * completion routine.  BIOs do not support folios that are 4GiB or larger.
> + *
> + * Return: The number of bytes from this folio added to the bio.
> + */
> +size_t bio_add_folio(struct bio *bio, struct folio *folio, size_t len,
> +		size_t off)
> +{
> +	if (len > UINT_MAX || off > UINT_MAX)
> +		return 0;
> +	return bio_add_page(bio, &folio->page, len, off);
> +}

I'd use the opportunity to switch to a true/false return instead of
the length.  This has been on my todo list for bio_add_page for a while,
so it might make sense to start out the new API the right way.
Matthew Wilcox July 20, 2021, 11:16 a.m. UTC | #3
On Tue, Jul 20, 2021 at 07:42:00AM +0100, Christoph Hellwig wrote:
> On Mon, Jul 19, 2021 at 07:39:45PM +0100, Matthew Wilcox (Oracle) wrote:
> > +/**
> > + * bio_add_folio - Attempt to add part of a folio to a bio.
> > + * @bio: Bio to add to.
> > + * @folio: Folio to add.
> > + * @len: How many bytes from the folio to add.
> > + * @off: First byte in this folio to add.
> > + *
> > + * Always uses the head page of the folio in the bio.  If a submitter only
> > + * uses bio_add_folio(), it can count on never seeing tail pages in the
> > + * completion routine.  BIOs do not support folios that are 4GiB or larger.
> > + *
> > + * Return: The number of bytes from this folio added to the bio.
> > + */
> > +size_t bio_add_folio(struct bio *bio, struct folio *folio, size_t len,
> > +		size_t off)
> > +{
> > +	if (len > UINT_MAX || off > UINT_MAX)
> > +		return 0;
> > +	return bio_add_page(bio, &folio->page, len, off);
> > +}
> 
> I'd use the opportunity to switch to a true/false return instead of
> the length.  This has been on my todo list for bio_add_page for a while,
> so it might make sense to start out the new API the right way.

ok.
Matthew Wilcox July 22, 2021, 4:27 p.m. UTC | #4
On Tue, Jul 20, 2021 at 07:42:00AM +0100, Christoph Hellwig wrote:
> On Mon, Jul 19, 2021 at 07:39:45PM +0100, Matthew Wilcox (Oracle) wrote:
> > +/**
> > + * bio_add_folio - Attempt to add part of a folio to a bio.
> > + * @bio: Bio to add to.
> > + * @folio: Folio to add.
> > + * @len: How many bytes from the folio to add.
> > + * @off: First byte in this folio to add.
> > + *
> > + * Always uses the head page of the folio in the bio.  If a submitter only
> > + * uses bio_add_folio(), it can count on never seeing tail pages in the
> > + * completion routine.  BIOs do not support folios that are 4GiB or larger.
> > + *
> > + * Return: The number of bytes from this folio added to the bio.
> > + */
> > +size_t bio_add_folio(struct bio *bio, struct folio *folio, size_t len,
> > +		size_t off)
> > +{
> > +	if (len > UINT_MAX || off > UINT_MAX)
> > +		return 0;
> > +	return bio_add_page(bio, &folio->page, len, off);
> > +}
> 
> I'd use the opportunity to switch to a true/false return instead of
> the length.  This has been on my todo list for bio_add_page for a while,
> so it might make sense to start out the new API the right way.

Looking at it with fresh eyes, I decided to rewrite the docs too.
ie this:

 /**
  * bio_add_folio - Attempt to add part of a folio to a bio.
- * @bio: Bio to add to.
+ * @bio: BIO to add to.
  * @folio: Folio to add.
  * @len: How many bytes from the folio to add.
  * @off: First byte in this folio to add.
  *
- * Always uses the head page of the folio in the bio.  If a submitter only
- * uses bio_add_folio(), it can count on never seeing tail pages in the
- * completion routine.  BIOs do not support folios that are 4GiB or larger.
+ * Filesystems that use folios can call this function instead of calling
+ * bio_add_page() for each page in the folio.  If @off is bigger than
+ * PAGE_SIZE, this function can create a bio_vec that starts in a page
+ * after the bv_page.  BIOs do not support folios that are 4GiB or larger.
  *
- * Return: The number of bytes from this folio added to the bio.
+ * Return: Whether the addition was successful.
  */
-size_t bio_add_folio(struct bio *bio, struct folio *folio, size_t len,
+bool bio_add_folio(struct bio *bio, struct folio *folio, size_t len,
                size_t off)
 {
        if (len > UINT_MAX || off > UINT_MAX)
                return 0;
-       return bio_add_page(bio, &folio->page, len, off);
+       return bio_add_page(bio, &folio->page, len, off) > 0;
 }

(i decided to go with > 0 so it's impervious to when you change
bio_add_page())
Ming Lei July 30, 2021, 8:25 a.m. UTC | #5
On Mon, Jul 19, 2021 at 07:39:45PM +0100, Matthew Wilcox (Oracle) wrote:
> This is a thin wrapper around bio_add_page().  The main advantage here
> is the documentation that the submitter can expect to see folios in the
> completion handler, and that stupidly large folios are not supported.
> It's not currently possible to allocate stupidly large folios, but if
> it ever becomes possible, this function will fail gracefully instead of
> doing I/O to the wrong bytes.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  block/bio.c         | 21 +++++++++++++++++++++
>  include/linux/bio.h |  3 ++-
>  2 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/block/bio.c b/block/bio.c
> index 1fab762e079b..c64e35548fb2 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -933,6 +933,27 @@ int bio_add_page(struct bio *bio, struct page *page,
>  }
>  EXPORT_SYMBOL(bio_add_page);
>  
> +/**
> + * bio_add_folio - Attempt to add part of a folio to a bio.
> + * @bio: Bio to add to.
> + * @folio: Folio to add.
> + * @len: How many bytes from the folio to add.
> + * @off: First byte in this folio to add.
> + *
> + * Always uses the head page of the folio in the bio.  If a submitter only
> + * uses bio_add_folio(), it can count on never seeing tail pages in the
> + * completion routine.  BIOs do not support folios that are 4GiB or larger.
> + *
> + * Return: The number of bytes from this folio added to the bio.
> + */
> +size_t bio_add_folio(struct bio *bio, struct folio *folio, size_t len,
> +		size_t off)
> +{
> +	if (len > UINT_MAX || off > UINT_MAX)
> +		return 0;

The added page shouldn't cross 4G boundary, so just wondering why not
check 'if (len > UINT_MAX - off)'?


Thanks, 
Ming
Matthew Wilcox July 30, 2021, 11:30 a.m. UTC | #6
On Fri, Jul 30, 2021 at 04:25:17PM +0800, Ming Lei wrote:
> > +size_t bio_add_folio(struct bio *bio, struct folio *folio, size_t len,
> > +		size_t off)
> > +{
> > +	if (len > UINT_MAX || off > UINT_MAX)
> > +		return 0;
> 
> The added page shouldn't cross 4G boundary, so just wondering why not
> check 'if (len > UINT_MAX - off)'?

That check is going to be vulnerable to wrapping, eg
	off = 2^32, len = 512

It would be less vulnerable to wrapping if we cast both sides to
signed long.  But at that point, we're firmly into obscuring the
intent of the check.
diff mbox series

Patch

diff --git a/block/bio.c b/block/bio.c
index 1fab762e079b..c64e35548fb2 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -933,6 +933,27 @@  int bio_add_page(struct bio *bio, struct page *page,
 }
 EXPORT_SYMBOL(bio_add_page);
 
+/**
+ * bio_add_folio - Attempt to add part of a folio to a bio.
+ * @bio: Bio to add to.
+ * @folio: Folio to add.
+ * @len: How many bytes from the folio to add.
+ * @off: First byte in this folio to add.
+ *
+ * Always uses the head page of the folio in the bio.  If a submitter only
+ * uses bio_add_folio(), it can count on never seeing tail pages in the
+ * completion routine.  BIOs do not support folios that are 4GiB or larger.
+ *
+ * Return: The number of bytes from this folio added to the bio.
+ */
+size_t bio_add_folio(struct bio *bio, struct folio *folio, size_t len,
+		size_t off)
+{
+	if (len > UINT_MAX || off > UINT_MAX)
+		return 0;
+	return bio_add_page(bio, &folio->page, len, off);
+}
+
 void bio_release_pages(struct bio *bio, bool mark_dirty)
 {
 	struct bvec_iter_all iter_all;
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 2203b686e1f0..ade93e2de6a1 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -462,7 +462,8 @@  extern void bio_uninit(struct bio *);
 extern void bio_reset(struct bio *);
 void bio_chain(struct bio *, struct bio *);
 
-extern int bio_add_page(struct bio *, struct page *, unsigned int,unsigned int);
+int bio_add_page(struct bio *, struct page *, unsigned len, unsigned off);
+size_t bio_add_folio(struct bio *, struct folio *, size_t len, size_t off);
 extern int bio_add_pc_page(struct request_queue *, struct bio *, struct page *,
 			   unsigned int, unsigned int);
 int bio_add_zone_append_page(struct bio *bio, struct page *page,