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

From: Jens Axboe <axboe@kernel.dk>
To: Al Viro <viro@zeniv.linux.org.uk>, Keith Busch <kbusch@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Christoph Hellwig <hch@lst.de>,
	Matthew Wilcox <willy@infradead.org>,
	David Howells <dhowells@redhat.com>,
	Dominique Martinet <asmadeus@codewreck.org>,
	Christian Brauner <brauner@kernel.org>,
	linux-fsdevel@vger.kernel.org
Subject: Re: [block.git conflicts] Re: [PATCH 37/44] block: convert to advancing variants of iov_iter_get_pages{,_alloc}()
Date: Fri, 1 Jul 2022 15:30:56 -0600	[thread overview]
Message-ID: <0197eded-b93f-6aff-aefe-b6a85dd8d953@kernel.dk> (raw)
In-Reply-To: <Yr8xmNMEOJke6NOx@ZenIV>

On 7/1/22 11:40 AM, Al Viro wrote:
> On Thu, Jun 30, 2022 at 08:07:24PM -0600, Keith Busch wrote:
>> 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.
> 
> See vfs.git#block-fixes, along with #work.iov_iter_get_pages-3 in there.
> Seems to work here...
> 
> If you are OK with #block-fixes (it's one commit on top of
> bf8d08532bc1 "iomap: add support for dma aligned direct-io" in
> block.git), the easiest way to deal with the conflicts would be
> to have that branch pulled into block.git.  Jens, would you be
> OK with that in terms of tree topology?  Provided that patch
> itself looks sane to you, of course...

I'm fine with that approach. Don't have too much time to look into this
stuff just yet, but looks like you and Keith are getting it sorted out.
I'll check in on emails later this weekend and we can get it pulled in
at that point when you guys deem it ready to check/pull.

-- 
Jens Axboe