On Thu, Jun 30, 2022 at 11:39:36PM +0100, Al Viro wrote:
> On Thu, Jun 30, 2022 at 11:11:27PM +0100, Al Viro wrote:
>
> > ... and the first half of that thing conflicts with "block: relax direct
> > io memory alignment" in -next...
>
> BTW, looking at that commit - are you sure that bio_put_pages() on failure
> exit will do the right thing? We have grabbed a bunch of page references;
> the amount if DIV_ROUND_UP(offset + size, PAGE_SIZE). And that's before
> your
> size = ALIGN_DOWN(size, bdev_logical_block_size(bio->bi_bdev));
Thanks for the catch, it does look like a page reference could get leaked here.
> in there. IMO the following would be more obviously correct:
> size = iov_iter_get_pages(iter, pages, LONG_MAX, nr_pages, &offset);
> if (unlikely(size <= 0))
> return size ? size : -EFAULT;
>
> nr_pages = DIV_ROUND_UP(size + offset, PAGE_SIZE);
> size = ALIGN_DOWN(size, bdev_logical_block_size(bio->bi_bdev));
>
> for (left = size, i = 0; left > 0; left -= len, i++) {
> ...
> if (ret) {
> while (i < nr_pages)
> put_page(pages[i++]);
> return ret;
> }
> ...
>
> and get rid of bio_put_pages() entirely. Objections?
I think that makes sense. I'll give your idea a test run tomorrow.
