On Mon, Jun 05, 2023 at 04:25:22PM +0800, Yin, Fengwei wrote:
> On 6/5/2023 6:11 AM, Matthew Wilcox wrote:
> > On Sun, Jun 04, 2023 at 11:29:52AM -0700, Darrick J. Wong wrote:
> >> On Fri, Jun 02, 2023 at 11:24:44PM +0100, Matthew Wilcox (Oracle) wrote:
> >>> - copied = copy_page_from_iter_atomic(page, offset, bytes, i);
> >>> + copied = copy_page_from_iter_atomic(&folio->page, offset, bytes, i);
> >>
> >> I think I've gotten lost in the weeds. Does copy_page_from_iter_atomic
> >> actually know how to deal with a multipage folio? AFAICT it takes a
> >> page, kmaps it, and copies @bytes starting at @offset in the page. If
> >> a caller feeds it a multipage folio, does that all work correctly? Or
> >> will the pagecache split multipage folios as needed to make it work
> >> right?
> >
> > It's a smidgen inefficient, but it does work. First, it calls
> > page_copy_sane() to check that offset & n fit within the compound page
> > (ie this all predates folios).
> >
> > ... Oh. copy_page_from_iter() handles this correctly.
> > copy_page_from_iter_atomic() doesn't. I'll have to fix this
> > first. Looks like Al fixed copy_page_from_iter() in c03f05f183cd
> > and didn't fix copy_page_from_iter_atomic().
> >
> >> If we create a 64k folio at pos 0 and then want to write a byte at pos
> >> 40k, does __filemap_get_folio break up the 64k folio so that the folio
> >> returned by iomap_get_folio starts at 40k? Or can the iter code handle
> >> jumping ten pages into a 16-page folio and I just can't see it?
> >
> > Well ... it handles it fine unless it's highmem. p is kaddr + offset,
> > so if offset is 40k, it works correctly on !highmem.
> So is it better to have implementations for !highmem and highmem? And for
> !highmem, we don't need the kmap_local_page()/kunmap_local() and chunk
> size per copy is not limited to PAGE_SIZE. Thanks.
No, that's not needed; we can handle that just fine. Maybe this can
use kmap_local_page() instead of kmap_atomic(). Al, what do you think?
I haven't tested this yet; need to figure out a qemu config with highmem ...
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 960223ed9199..d3d6a0789625 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -857,24 +857,36 @@ 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 = bytes, 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;
+
+ page += offset / PAGE_SIZE;
+ offset %= PAGE_SIZE;
+ if (PageHighMem(page))
+ n = min_t(size_t, bytes, PAGE_SIZE);
+ while (1) {
+ char *kaddr = kmap_atomic(page) + offset;
+ iterate_and_advance(i, n, base, len, off,
+ copyin(kaddr + off, base, len),
+ memcpy_from_iter(i, kaddr + off, base, len)
+ )
+ kunmap_atomic(kaddr);
+ copied += n;
+ if (!PageHighMem(page) || copied == bytes || n == 0)
+ break;
+ offset += n;
+ page += offset / PAGE_SIZE;
+ offset %= PAGE_SIZE;
+ n = min_t(size_t, bytes - copied, PAGE_SIZE);
}
- 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;
+ return copied;
}
EXPORT_SYMBOL(copy_page_from_iter_atomic);
