diff mbox series

[PATCHv3,1/6] block/bio: remove duplicate append pages code

Message ID 20220523210119.2500150-2-kbusch@fb.com (mailing list archive)
State New, archived
Headers show
Series direct io dma alignment | expand

Commit Message

Keith Busch May 23, 2022, 9:01 p.m. UTC 
From: Keith Busch <kbusch@kernel.org>

The getting pages setup for zone append and normal IO are identical. Use
common code for each.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
v2->v3:

  Folded in improvement suggestions from Christoph

 block/bio.c | 105 +++++++++++++++++++++-------------------------------
 1 file changed, 42 insertions(+), 63 deletions(-)

Comments

Christoph Hellwig May 24, 2022, 6 a.m. UTC | #1 
Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Johannes Thumshirn May 24, 2022, 6:24 a.m. UTC | #2 
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Pankaj Raghav May 24, 2022, 2:17 p.m. UTC | #3 
On Mon, May 23, 2022 at 02:01:14PM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The getting pages setup for zone append and normal IO are identical. Use
> common code for each.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> +
> +static int bio_iov_add_zone_append_page(struct bio *bio, struct page *page,
> +		unsigned int len, unsigned int offset)
> +{
> +	struct request_queue *q = bdev_get_queue(bio->bi_bdev);
> +	bool same_page = false;
> +
> +	if (bio_add_hw_page(q, bio, page, len, offset,
> +			queue_max_zone_append_sectors(q), &same_page) != len)
> +		return -EINVAL;
> +	if (same_page)
> +		put_page(page);
> +	return 0;
> +}
> +
>  
> -static int __bio_iov_append_get_pages(struct bio *bio, struct iov_iter *iter)
> -{
> -	unsigned short nr_pages = bio->bi_max_vecs - bio->bi_vcnt;
> -	unsigned short entries_left = bio->bi_max_vecs - bio->bi_vcnt;
> -	struct request_queue *q = bdev_get_queue(bio->bi_bdev);
> -	unsigned int max_append_sectors = queue_max_zone_append_sectors(q);
> -	struct bio_vec *bv = bio->bi_io_vec + bio->bi_vcnt;
> -	struct page **pages = (struct page **)bv;
> -	ssize_t size, left;
> -	unsigned len, i;
> -	size_t offset;
> -	int ret = 0;
> -

> -	if (WARN_ON_ONCE(!max_append_sectors))
> -		return 0;
I don't see this check in the append path. Should it be added in
bio_iov_add_zone_append_page() function?
> -
> -	/*
> -	 * Move page array up in the allocated memory for the bio vecs as far as
> -	 * possible so that we can start filling biovecs from the beginning
> -	 * without overwriting the temporary page array.
> -	 */
> -	BUILD_BUG_ON(PAGE_PTRS_PER_BVEC < 2);
> -	pages += entries_left * (PAGE_PTRS_PER_BVEC - 1);
> -
> -	size = iov_iter_get_pages(iter, pages, LONG_MAX, nr_pages, &offset);
> -	if (unlikely(size <= 0))
Keith Busch May 24, 2022, 3:38 p.m. UTC | #4 
On Tue, May 24, 2022 at 04:17:54PM +0200, Pankaj Raghav wrote:
> On Mon, May 23, 2022 at 02:01:14PM -0700, Keith Busch wrote:
> > -	if (WARN_ON_ONCE(!max_append_sectors))
> > -		return 0;
> I don't see this check in the append path. Should it be added in
> bio_iov_add_zone_append_page() function?

I'm not sure this check makes a lot of sense. If it just returns 0 here, then
won't that get bio_iov_iter_get_pages() stuck in an infinite loop? The bio
isn't filling, the iov isn't advancing, and 0 indicates keep-going.
Pankaj Raghav May 25, 2022, 7:49 a.m. UTC | #5 
On Tue, May 24, 2022 at 09:38:32AM -0600, Keith Busch wrote:
> On Tue, May 24, 2022 at 04:17:54PM +0200, Pankaj Raghav wrote:
> > On Mon, May 23, 2022 at 02:01:14PM -0700, Keith Busch wrote:
> > > -	if (WARN_ON_ONCE(!max_append_sectors))
> > > -		return 0;
> > I don't see this check in the append path. Should it be added in
> > bio_iov_add_zone_append_page() function?
> 
> I'm not sure this check makes a lot of sense. If it just returns 0 here, then
> won't that get bio_iov_iter_get_pages() stuck in an infinite loop? The bio
> isn't filling, the iov isn't advancing, and 0 indicates keep-going.
Yeah but if max_append_sectors is zero, then bio_add_hw_page() also
returns 0 as follows:
....
	if (((bio->bi_iter.bi_size + len) >> 9) > max_sectors)
		return 0;
....
With WARN_ON_ONCE, we at least get a warning message if it gets stuck in an
infinite loop because of max_append_sectors being zero right?
Damien Le Moal May 25, 2022, 8:30 a.m. UTC | #6 
On 5/25/22 16:49, Pankaj Raghav wrote:
> On Tue, May 24, 2022 at 09:38:32AM -0600, Keith Busch wrote:
>> On Tue, May 24, 2022 at 04:17:54PM +0200, Pankaj Raghav wrote:
>>> On Mon, May 23, 2022 at 02:01:14PM -0700, Keith Busch wrote:
>>>> -	if (WARN_ON_ONCE(!max_append_sectors))
>>>> -		return 0;
>>> I don't see this check in the append path. Should it be added in
>>> bio_iov_add_zone_append_page() function?
>>
>> I'm not sure this check makes a lot of sense. If it just returns 0 here, then
>> won't that get bio_iov_iter_get_pages() stuck in an infinite loop? The bio
>> isn't filling, the iov isn't advancing, and 0 indicates keep-going.
> Yeah but if max_append_sectors is zero, then bio_add_hw_page() also
> returns 0 as follows:
> ....
> 	if (((bio->bi_iter.bi_size + len) >> 9) > max_sectors)
> 		return 0;
> ....
> With WARN_ON_ONCE, we at least get a warning message if it gets stuck in an
> infinite loop because of max_append_sectors being zero right?
> 

Warning about an infinite loop that can be recovered from only by
rebooting the machine is not very useful...
If max_append_sectors is zero and bio_iov_add_zone_append_page() is
called, this is an error (stupid user) and everything should be failed
with -ENOSUPP or -EIO.
Keith Busch May 25, 2022, 1:37 p.m. UTC | #7 
On Wed, May 25, 2022 at 09:49:41AM +0200, Pankaj Raghav wrote:
> On Tue, May 24, 2022 at 09:38:32AM -0600, Keith Busch wrote:
> > On Tue, May 24, 2022 at 04:17:54PM +0200, Pankaj Raghav wrote:
> > > On Mon, May 23, 2022 at 02:01:14PM -0700, Keith Busch wrote:
> > > > -	if (WARN_ON_ONCE(!max_append_sectors))
> > > > -		return 0;
> > > I don't see this check in the append path. Should it be added in
> > > bio_iov_add_zone_append_page() function?
> > 
> > I'm not sure this check makes a lot of sense. If it just returns 0 here, then
> > won't that get bio_iov_iter_get_pages() stuck in an infinite loop? The bio
> > isn't filling, the iov isn't advancing, and 0 indicates keep-going.
> Yeah but if max_append_sectors is zero, then bio_add_hw_page() also
> returns 0 as follows:
> ....
> 	if (((bio->bi_iter.bi_size + len) >> 9) > max_sectors)
> 		return 0;
> ....
> With WARN_ON_ONCE, we at least get a warning message if it gets stuck in an
> infinite loop because of max_append_sectors being zero right?

The return for this function is the added length, not an indicator of success.
And we already handle '0' as an error from bio_iov_add_zone_append_page():

	if (bio_add_hw_page(q, bio, page, len, offset,
			queue_max_zone_append_sectors(q), &same_page) != len)
		return -EINVAL;
Pankaj Raghav May 25, 2022, 2:25 p.m. UTC | #8 
On Wed, May 25, 2022 at 07:37:45AM -0600, Keith Busch wrote:
> On Wed, May 25, 2022 at 09:49:41AM +0200, Pankaj Raghav wrote:
> > On Tue, May 24, 2022 at 09:38:32AM -0600, Keith Busch wrote:
> > > On Tue, May 24, 2022 at 04:17:54PM +0200, Pankaj Raghav wrote:
> > > > On Mon, May 23, 2022 at 02:01:14PM -0700, Keith Busch wrote:
> > > > > -	if (WARN_ON_ONCE(!max_append_sectors))
> > > > > -		return 0;
> > > > I don't see this check in the append path. Should it be added in
> > > > bio_iov_add_zone_append_page() function?
> > > 
> > > I'm not sure this check makes a lot of sense. If it just returns 0 here, then
> > > won't that get bio_iov_iter_get_pages() stuck in an infinite loop? The bio
> > > isn't filling, the iov isn't advancing, and 0 indicates keep-going.
> > Yeah but if max_append_sectors is zero, then bio_add_hw_page() also
> > returns 0 as follows:
> > ....
> > 	if (((bio->bi_iter.bi_size + len) >> 9) > max_sectors)
> > 		return 0;
> > ....
> > With WARN_ON_ONCE, we at least get a warning message if it gets stuck in an
> > infinite loop because of max_append_sectors being zero right?
> 
> The return for this function is the added length, not an indicator of success.
> And we already handle '0' as an error from bio_iov_add_zone_append_page():
> 
> 	if (bio_add_hw_page(q, bio, page, len, offset,
> 			queue_max_zone_append_sectors(q), &same_page) != len)
Ah. I didn't see the `!=len` part. Sorry for the noise and ignore this
comment.
> 		return -EINVAL;
diff mbox series

Patch

diff --git a/block/bio.c b/block/bio.c
index a3893d80dccc..55d2a9c4e312 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1158,6 +1158,37 @@  static void bio_put_pages(struct page **pages, size_t size, size_t off)
 		put_page(pages[i]);
 }
 
+static int bio_iov_add_page(struct bio *bio, struct page *page,
+		unsigned int len, unsigned int offset)
+{
+	bool same_page = false;
+
+	if (!__bio_try_merge_page(bio, page, len, offset, &same_page)) {
+		if (WARN_ON_ONCE(bio_full(bio, len)))
+			return -EINVAL;
+		__bio_add_page(bio, page, len, offset);
+		return 0;
+	}
+
+	if (same_page)
+		put_page(page);
+	return 0;
+}
+
+static int bio_iov_add_zone_append_page(struct bio *bio, struct page *page,
+		unsigned int len, unsigned int offset)
+{
+	struct request_queue *q = bdev_get_queue(bio->bi_bdev);
+	bool same_page = false;
+
+	if (bio_add_hw_page(q, bio, page, len, offset,
+			queue_max_zone_append_sectors(q), &same_page) != len)
+		return -EINVAL;
+	if (same_page)
+		put_page(page);
+	return 0;
+}
+
 #define PAGE_PTRS_PER_BVEC     (sizeof(struct bio_vec) / sizeof(struct page *))
 
 /**
@@ -1176,7 +1207,6 @@  static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
 	unsigned short entries_left = bio->bi_max_vecs - bio->bi_vcnt;
 	struct bio_vec *bv = bio->bi_io_vec + bio->bi_vcnt;
 	struct page **pages = (struct page **)bv;
-	bool same_page = false;
 	ssize_t size, left;
 	unsigned len, i;
 	size_t offset;
@@ -1185,7 +1215,7 @@  static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
 	 * Move page array up in the allocated memory for the bio vecs as far as
 	 * possible so that we can start filling biovecs from the beginning
 	 * without overwriting the temporary page array.
-	*/
+	 */
 	BUILD_BUG_ON(PAGE_PTRS_PER_BVEC < 2);
 	pages += entries_left * (PAGE_PTRS_PER_BVEC - 1);
 
@@ -1195,18 +1225,18 @@  static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
 
 	for (left = size, i = 0; left > 0; left -= len, i++) {
 		struct page *page = pages[i];
+		int ret;
 
 		len = min_t(size_t, PAGE_SIZE - offset, left);
+		if (bio_op(bio) == REQ_OP_ZONE_APPEND)
+			ret = bio_iov_add_zone_append_page(bio, page, len,
+					offset);
+		else
+			ret = bio_iov_add_page(bio, page, len, offset);
 
-		if (__bio_try_merge_page(bio, page, len, offset, &same_page)) {
-			if (same_page)
-				put_page(page);
-		} else {
-			if (WARN_ON_ONCE(bio_full(bio, len))) {
-				bio_put_pages(pages + i, left, offset);
-				return -EINVAL;
-			}
-			__bio_add_page(bio, page, len, offset);
+		if (ret) {
+			bio_put_pages(pages + i, left, offset);
+			return ret;
 		}
 		offset = 0;
 	}
@@ -1215,54 +1245,6 @@  static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
 	return 0;
 }
 
-static int __bio_iov_append_get_pages(struct bio *bio, struct iov_iter *iter)
-{
-	unsigned short nr_pages = bio->bi_max_vecs - bio->bi_vcnt;
-	unsigned short entries_left = bio->bi_max_vecs - bio->bi_vcnt;
-	struct request_queue *q = bdev_get_queue(bio->bi_bdev);
-	unsigned int max_append_sectors = queue_max_zone_append_sectors(q);
-	struct bio_vec *bv = bio->bi_io_vec + bio->bi_vcnt;
-	struct page **pages = (struct page **)bv;
-	ssize_t size, left;
-	unsigned len, i;
-	size_t offset;
-	int ret = 0;
-
-	if (WARN_ON_ONCE(!max_append_sectors))
-		return 0;
-
-	/*
-	 * Move page array up in the allocated memory for the bio vecs as far as
-	 * possible so that we can start filling biovecs from the beginning
-	 * without overwriting the temporary page array.
-	 */
-	BUILD_BUG_ON(PAGE_PTRS_PER_BVEC < 2);
-	pages += entries_left * (PAGE_PTRS_PER_BVEC - 1);
-
-	size = iov_iter_get_pages(iter, pages, LONG_MAX, nr_pages, &offset);
-	if (unlikely(size <= 0))
-		return size ? size : -EFAULT;
-
-	for (left = size, i = 0; left > 0; left -= len, i++) {
-		struct page *page = pages[i];
-		bool same_page = false;
-
-		len = min_t(size_t, PAGE_SIZE - offset, left);
-		if (bio_add_hw_page(q, bio, page, len, offset,
-				max_append_sectors, &same_page) != len) {
-			bio_put_pages(pages + i, left, offset);
-			ret = -EINVAL;
-			break;
-		}
-		if (same_page)
-			put_page(page);
-		offset = 0;
-	}
-
-	iov_iter_advance(iter, size - left);
-	return ret;
-}
-
 /**
  * bio_iov_iter_get_pages - add user or kernel pages to a bio
  * @bio: bio to add pages to
@@ -1297,10 +1279,7 @@  int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
 	}
 
 	do {
-		if (bio_op(bio) == REQ_OP_ZONE_APPEND)
-			ret = __bio_iov_append_get_pages(bio, iter);
-		else
-			ret = __bio_iov_iter_get_pages(bio, iter);
+		ret = __bio_iov_iter_get_pages(bio, iter);
 	} while (!ret && iov_iter_count(iter) && !bio_full(bio, 0));
 
 	/* don't account direct I/O as memory stall */