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

Yin, Fengwei
2023-06-05T08:25:40Z

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.

Regards
Yin, Fengwei