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 12:38:36PM -0600, Keith Busch wrote:
> On Fri, Jul 01, 2022 at 07:12:17PM +0100, Al Viro wrote:
> > On Fri, Jul 01, 2022 at 07:07:45PM +0100, Al Viro wrote:
> > > > 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.
> 
> I think I see what you mean, though the issue with a non-advancing iterator on
> a partially filled bio was happening prior to this patch.
> 
> > How about the following?
> 
> This looks close to what I was about to respond with. Just a couple issues
> below:
> 
> > -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;
> 
> 'ret' might never be initialized if 'size' aligns down to 0.

Point.

> >  	/*
> >  	 * 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));
> 
> We still need to return EFAULT if size becomes 0 because that's the only way
> bio_iov_iter_get_pages()'s loop will break out in this condition.

I really don't like these calling conventions ;-/

What do you want to happen if we have that align-down to reduce size?
IOW, what should be the state after that loop in the caller?