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

["Yin, Fengwei" <fengwei.yin@intel.com>]

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