Re: [PATCH v2 7/7] iomap: Copy larger chunks from userspace

[Matthew Wilcox <willy@infradead.org>]

On Wed, Jun 07, 2023 at 10:21:41AM +0800, Yin Fengwei wrote:
> On 6/7/23 02:07, Matthew Wilcox wrote:
> > +++ 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);
> This is smart.

Thanks ;-)

> > +	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;
> My understanding is copied == bytes could cover !PageHighMem(page).

It could!  But the PageHighMem test serves two purposes.  One is that
it tells the human reader that this is all because of HighMem.  The
other is that on !HIGHMEM systems it compiles to false:

PAGEFLAG_FALSE(HighMem, highmem)

static inline int Page##uname(const struct page *page) { return 0; }

So it tells the _compiler_ that all of this code is ignorable and
it can optimise it out.  Now, you and I know that it will always
be true, but it lets the compiler remove the test.  Hopefully the
compiler can also see that:

	while (1) {
		...
		if (true)
			break;
	}

means it can optimise away the entire loop structure and just produce
the same code that it always did.