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@xxxxxx>
