Re: [block.git conflicts] Re: [PATCH 37/44] block: convert to advancing variants of iov_iter_get_pages{,_alloc}()

[Al Viro <viro@zeniv.linux.org.uk>]

On Fri, Jul 01, 2022 at 07:07:45PM +0100, Al Viro wrote:
> On Fri, Jul 01, 2022 at 11:53:44AM -0600, Keith Busch wrote:
> > On Fri, Jul 01, 2022 at 06:40:40PM +0100, Al Viro wrote:
> > > -static void bio_put_pages(struct page **pages, size_t size, size_t off)
> > > -{
> > > -	size_t i, nr = DIV_ROUND_UP(size + (off & ~PAGE_MASK), PAGE_SIZE);
> > > -
> > > -	for (i = 0; i < nr; i++)
> > > -		put_page(pages[i]);
> > > -}
> > > -
> > >  static int bio_iov_add_page(struct bio *bio, struct page *page,
> > >  		unsigned int len, unsigned int offset)
> > >  {
> > > @@ -1228,11 +1220,11 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
> > >  	 * the iov data will be picked up in the next bio iteration.
> > >  	 */
> > >  	size = iov_iter_get_pages(iter, pages, LONG_MAX, nr_pages, &offset);
> > > -	if (size > 0)
> > > -		size = ALIGN_DOWN(size, bdev_logical_block_size(bio->bi_bdev));
> > >  	if (unlikely(size <= 0))
> > >  		return size ? size : -EFAULT;
> > > +	nr_pages = DIV_ROUND_UP(offset + size, PAGE_SIZE);
> > >  
> > > +	size = ALIGN_DOWN(size, bdev_logical_block_size(bio->bi_bdev));
> > 
> > This isn't quite right. The result of the ALIGN_DOWN could be 0, so whatever
> > page we got before would be leaked since unused pages are only released on an
> > add_page error. I was about to reply with a patch that fixes this, but here's
> > the one that I'm currently testing:
> 
> AFAICS, result is broken; you might end up consuming some data and leaving
> iterator not advanced at all.  With no way for the caller to tell which way it
> went.

How about the following?

commit 5e3e9769404de54734c110b2040bdb93593e0f1b
Author: Al Viro <viro@zeniv.linux.org.uk>
Date:   Fri Jul 1 13:15:36 2022 -0400

    __bio_iov_iter_get_pages(): make sure we don't leak page refs on failure
    
    Calculate the number of pages we'd grabbed before trimming size down.
    And don't bother with bio_put_pages() - an explicit cleanup loop is
    easier to follow...
    
    Fixes: b1a000d3b8ec "block: relax direct io memory alignment"
    Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

diff --git a/block/bio.c b/block/bio.c
index 933ea3210954..a9fe20cb71fe 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1151,14 +1151,6 @@ void bio_iov_bvec_set(struct bio *bio, struct iov_iter *iter)
 	bio_set_flag(bio, BIO_CLONED);
 }
 
-static void bio_put_pages(struct page **pages, size_t size, size_t off)
-{
-	size_t i, nr = DIV_ROUND_UP(size + (off & ~PAGE_MASK), PAGE_SIZE);
-
-	for (i = 0; i < nr; i++)
-		put_page(pages[i]);
-}
-
 static int bio_iov_add_page(struct bio *bio, struct page *page,
 		unsigned int len, unsigned int offset)
 {
@@ -1211,6 +1203,7 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
 	ssize_t size, left;
 	unsigned len, i;
 	size_t offset;
+	int ret;
 
 	/*
 	 * Move page array up in the allocated memory for the bio vecs as far as
@@ -1228,14 +1221,13 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
 	 * the iov data will be picked up in the next bio iteration.
 	 */
 	size = iov_iter_get_pages(iter, pages, LONG_MAX, nr_pages, &offset);
-	if (size > 0)
-		size = ALIGN_DOWN(size, bdev_logical_block_size(bio->bi_bdev));
 	if (unlikely(size <= 0))
 		return size ? size : -EFAULT;
+	nr_pages = DIV_ROUND_UP(offset + size, PAGE_SIZE);
 
+	size = ALIGN_DOWN(size, bdev_logical_block_size(bio->bi_bdev));
 	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)
@@ -1244,15 +1236,15 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
 		else
 			ret = bio_iov_add_page(bio, page, len, offset);
 
-		if (ret) {
-			bio_put_pages(pages + i, left, offset);
-			return ret;
-		}
+		if (ret)
+			break;
 		offset = 0;
 	}
+	while (i < nr_pages)
+		put_page(pages[i++]);
 
-	iov_iter_advance(iter, size);
-	return 0;
+	iov_iter_advance(iter, size - left);
+	return ret;
 }
 
 /**