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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 6/7/2023 10:21 AM, Yin Fengwei wrote:
> 
> 
> On 6/7/23 02:07, Matthew Wilcox wrote:
>> 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);
> This is smart.
> 
>> +	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).
> 
>> +		offset += n;
>> +		page += offset / PAGE_SIZE;
> Should be page += n / PAGE_SIZE? Thanks.
offset / PAGE_SIZE is correct. Sorry for the noise.

Regards
Yin, Fengwei

> 
> 
> Regards
> Yin, Fengwei
> 
>> +		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);
>>  



[Index of Archives]     [XFS Filesystem Development (older mail)]     [Linux Filesystem Development]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux RAID]     [Linux SCSI]


  Powered by Linux