| Message ID | 20230710130253.3484695-2-willy@infradead.org (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | Create large folios in iomap buffered write path | expand |
On Mon, Jul 10, 2023 at 02:02:45PM +0100, Matthew Wilcox (Oracle) wrote: > copy_page_from_iter_atomic() already handles !highmem compound > pages correctly, but if we are passed a highmem compound page, > each base page needs to be mapped & unmapped individually. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > lib/iov_iter.c | 43 ++++++++++++++++++++++++++++--------------- > 1 file changed, 28 insertions(+), 15 deletions(-) > > diff --git a/lib/iov_iter.c b/lib/iov_iter.c > index b667b1e2f688..c728b6e4fb18 100644 > --- a/lib/iov_iter.c > +++ b/lib/iov_iter.c > @@ -566,24 +566,37 @@ size_t iov_iter_zero(size_t bytes, struct iov_iter *i) > } > EXPORT_SYMBOL(iov_iter_zero); > > -size_t copy_page_from_iter_atomic(struct page *page, unsigned offset, size_t bytes, > - struct iov_iter *i) > +size_t copy_page_from_iter_atomic(struct page *page, unsigned offset, > + size_t bytes, struct iov_iter *i) > { > - char *kaddr = kmap_atomic(page), *p = kaddr + offset; > - if (!page_copy_sane(page, offset, bytes)) { > - kunmap_atomic(kaddr); > + size_t n, copied = 0; > + > + if (!page_copy_sane(page, offset, bytes)) > return 0; > - } > - if (WARN_ON_ONCE(!i->data_source)) { > - kunmap_atomic(kaddr); > + if (WARN_ON_ONCE(!i->data_source)) > return 0; To make it easier to review the split of the kmap_atomic() until later and the saving of the unwinding would be nice as separate patches. > - } > - iterate_and_advance(i, bytes, base, len, off, > - copyin(p + off, base, len), > - memcpy_from_iter(i, p + off, base, len) > - ) > - kunmap_atomic(kaddr); > - return bytes; > + > + do { > + char *p; > + > + n = bytes - copied; > + if (PageHighMem(page)) { > + page += offset / PAGE_SIZE; I don't quite follow here how before the page was not modified to get to the first kmap_atomic(page) and now immediately if we're on a PageHighMem(page) we're doing some arithmetic to the page address to get the first kmap_atomic(). The only thing I could think of is seems like an implicit assumption here that if its a compound highmem page then we always start off with offset with a value of 0, is that right? But that seems to not be correct either. Luis
On Mon, Jul 10, 2023 at 04:43:35PM -0700, Luis Chamberlain wrote: > > - } > > - iterate_and_advance(i, bytes, base, len, off, > > - copyin(p + off, base, len), > > - memcpy_from_iter(i, p + off, base, len) > > - ) > > - kunmap_atomic(kaddr); > > - return bytes; > > + > > + do { > > + char *p; > > + > > + n = bytes - copied; > > + if (PageHighMem(page)) { > > + page += offset / PAGE_SIZE; > > I don't quite follow here how before the page was not modified > to get to the first kmap_atomic(page) and now immediately if we're > on a PageHighMem(page) we're doing some arithmetic to the page > address to get the first kmap_atomic(). The only thing I could > think of is seems like an implicit assumption here that if its a compound > highmem page then we always start off with offset with a value of > 0, is that right? But that seems to not be correct either. The important thing to know is that it was buggy before. If you called copy_page_from_iter_atomic() with an offset larger than PAGE_SIZE, it corrupted random memory! I can only assume that nobody was doing that.
On Mon, Jul 10, 2023 at 02:02:45PM +0100, Matthew Wilcox (Oracle) wrote: > +size_t copy_page_from_iter_atomic(struct page *page, unsigned offset, > + size_t bytes, struct iov_iter *i) > { > - char *kaddr = kmap_atomic(page), *p = kaddr + offset; > - if (!page_copy_sane(page, offset, bytes)) { > - kunmap_atomic(kaddr); > + size_t n, copied = 0; > + > + if (!page_copy_sane(page, offset, bytes)) > return 0; > - } > - if (WARN_ON_ONCE(!i->data_source)) { > - kunmap_atomic(kaddr); > + if (WARN_ON_ONCE(!i->data_source)) > return 0; > - iterate_and_advance(i, bytes, base, len, off, > - copyin(p + off, base, len), > - memcpy_from_iter(i, p + off, base, len) > - ) > - kunmap_atomic(kaddr); I have to agree with Luis that just moving the odd early kmap in the existing code is probably worth it's own patch just to keep the thing readable. I'm not going to ask for a resend just because of that, but if we need a respin it would be nice to split it out. > + > + do { > + char *p; > + > + n = bytes - copied; > + if (PageHighMem(page)) { > + page += offset / PAGE_SIZE; > + offset %= PAGE_SIZE; > + n = min_t(size_t, n, PAGE_SIZE - offset); > + } > + > + p = kmap_atomic(page) + offset; > + iterate_and_advance(i, n, base, len, off, > + copyin(p + off, base, len), > + memcpy_from_iter(i, p + off, base, len) > + ) > + kunmap_atomic(p); > + copied += n; > + offset += n; > + } while (PageHighMem(page) && copied != bytes && n > 0); This first read a little odd to me, but doing arithmetics on the page should never move it from highmem to non-highmem so I think it's fine. Would be a lot more readable if it was passed a folio, but I guess we can do that later. Otherwise looks good to me: Reviewed-by: Christoph Hellwig <hch@lst.de>
On Mon, Jul 10, 2023 at 02:02:45PM +0100, Matthew Wilcox (Oracle) wrote: > copy_page_from_iter_atomic() already handles !highmem compound > pages correctly, but if we are passed a highmem compound page, > each base page needs to be mapped & unmapped individually. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> Reviewed-by: Kent Overstreet <kent.overstreet@linux.dev> I just pulled this into the bcachefs tree; in a week or two if you haven't heard anything you can add my Tested-by:
On Mon, Jul 10, 2023 at 02:02:45PM +0100, Matthew Wilcox (Oracle) wrote: > copy_page_from_iter_atomic() already handles !highmem compound > pages correctly, but if we are passed a highmem compound page, > each base page needs to be mapped & unmapped individually. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> Yikes. Does this want a fixes tag? Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > lib/iov_iter.c | 43 ++++++++++++++++++++++++++++--------------- > 1 file changed, 28 insertions(+), 15 deletions(-) > > diff --git a/lib/iov_iter.c b/lib/iov_iter.c > index b667b1e2f688..c728b6e4fb18 100644 > --- a/lib/iov_iter.c > +++ b/lib/iov_iter.c > @@ -566,24 +566,37 @@ size_t iov_iter_zero(size_t bytes, struct iov_iter *i) > } > EXPORT_SYMBOL(iov_iter_zero); > > -size_t copy_page_from_iter_atomic(struct page *page, unsigned offset, size_t bytes, > - struct iov_iter *i) > +size_t copy_page_from_iter_atomic(struct page *page, unsigned offset, > + size_t bytes, struct iov_iter *i) > { > - char *kaddr = kmap_atomic(page), *p = kaddr + offset; > - if (!page_copy_sane(page, offset, bytes)) { > - kunmap_atomic(kaddr); > + size_t n, copied = 0; > + > + if (!page_copy_sane(page, offset, bytes)) > return 0; > - } > - if (WARN_ON_ONCE(!i->data_source)) { > - kunmap_atomic(kaddr); > + if (WARN_ON_ONCE(!i->data_source)) > return 0; > - } > - iterate_and_advance(i, bytes, base, len, off, > - copyin(p + off, base, len), > - memcpy_from_iter(i, p + off, base, len) > - ) > - kunmap_atomic(kaddr); > - return bytes; > + > + do { > + char *p; > + > + n = bytes - copied; > + if (PageHighMem(page)) { > + page += offset / PAGE_SIZE; > + offset %= PAGE_SIZE; > + n = min_t(size_t, n, PAGE_SIZE - offset); > + } > + > + p = kmap_atomic(page) + offset; > + iterate_and_advance(i, n, base, len, off, > + copyin(p + off, base, len), > + memcpy_from_iter(i, p + off, base, len) > + ) > + kunmap_atomic(p); > + copied += n; > + offset += n; > + } while (PageHighMem(page) && copied != bytes && n > 0); > + > + return copied; > } > EXPORT_SYMBOL(copy_page_from_iter_atomic); > > -- > 2.39.2 >
On Wed, Jul 12, 2023 at 09:42:07PM -0700, Darrick J. Wong wrote: > On Mon, Jul 10, 2023 at 02:02:45PM +0100, Matthew Wilcox (Oracle) wrote: > > copy_page_from_iter_atomic() already handles !highmem compound > > pages correctly, but if we are passed a highmem compound page, > > each base page needs to be mapped & unmapped individually. > > > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > > Yikes. Does this want a fixes tag? I was wondering the same thing. I think the bug is merely latent, and all existing users limit themselves to being within the page that they're passing in.
diff --git a/lib/iov_iter.c b/lib/iov_iter.c index b667b1e2f688..c728b6e4fb18 100644 --- a/lib/iov_iter.c +++ b/lib/iov_iter.c @@ -566,24 +566,37 @@ size_t iov_iter_zero(size_t bytes, struct iov_iter *i) } EXPORT_SYMBOL(iov_iter_zero); -size_t copy_page_from_iter_atomic(struct page *page, unsigned offset, size_t bytes, - struct iov_iter *i) +size_t copy_page_from_iter_atomic(struct page *page, unsigned offset, + size_t bytes, struct iov_iter *i) { - char *kaddr = kmap_atomic(page), *p = kaddr + offset; - if (!page_copy_sane(page, offset, bytes)) { - kunmap_atomic(kaddr); + size_t n, copied = 0; + + if (!page_copy_sane(page, offset, bytes)) return 0; - } - if (WARN_ON_ONCE(!i->data_source)) { - kunmap_atomic(kaddr); + if (WARN_ON_ONCE(!i->data_source)) return 0; - } - iterate_and_advance(i, bytes, base, len, off, - copyin(p + off, base, len), - memcpy_from_iter(i, p + off, base, len) - ) - kunmap_atomic(kaddr); - return bytes; + + do { + char *p; + + n = bytes - copied; + if (PageHighMem(page)) { + page += offset / PAGE_SIZE; + offset %= PAGE_SIZE; + n = min_t(size_t, n, PAGE_SIZE - offset); + } + + p = kmap_atomic(page) + offset; + iterate_and_advance(i, n, base, len, off, + copyin(p + off, base, len), + memcpy_from_iter(i, p + off, base, len) + ) + kunmap_atomic(p); + copied += n; + offset += n; + } while (PageHighMem(page) && copied != bytes && n > 0); + + return copied; } EXPORT_SYMBOL(copy_page_from_iter_atomic);
copy_page_from_iter_atomic() already handles !highmem compound pages correctly, but if we are passed a highmem compound page, each base page needs to be mapped & unmapped individually. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- lib/iov_iter.c | 43 ++++++++++++++++++++++++++++--------------- 1 file changed, 28 insertions(+), 15 deletions(-)