Re: [PATCH] fs/vboxsf: Replace kmap() with kmap_local_{page, folio}()

[Hans de Goede]

Hi,

On 6/29/23 11:28, Sumitra Sharma wrote:
> On Wed, Jun 28, 2023 at 06:15:04PM +0100, Matthew Wilcox wrote:
>> Here's a more comprehensive read_folio patch.  It's not at all
>> efficient, but then if we wanted an efficient vboxsf, we'd implement
>> vboxsf_readahead() and actually do an async call with deferred setting
>> of the uptodate flag.  I can consult with anyone who wants to do all
>> this work.
>>
>> I haven't even compiled this, just trying to show the direction this
>> should take.
>>
>> diff --git a/fs/vboxsf/file.c b/fs/vboxsf/file.c
>> index 2307f8037efc..f1af9a7bd3d8 100644
>> --- a/fs/vboxsf/file.c
>> +++ b/fs/vboxsf/file.c
>> @@ -227,26 +227,31 @@ const struct inode_operations vboxsf_reg_iops = {
>>  
>>  static int vboxsf_read_folio(struct file *file, struct folio *folio)
>>  {
>> -	struct page *page = &folio->page;
>>  	struct vboxsf_handle *sf_handle = file->private_data;
>> -	loff_t off = page_offset(page);
>> -	u32 nread = PAGE_SIZE;
>> -	u8 *buf;
>> +	loff_t pos = folio_pos(folio);
>> +	size_t offset = 0;
>>  	int err;
>>  
>> -	buf = kmap(page);
>> +	do {
>> +		u8 *buf = kmap_local_folio(folio, offset);
>> +		u32 nread = PAGE_SIZE;
>>  
>> -	err = vboxsf_read(sf_handle->root, sf_handle->handle, off, &nread, buf);
>> -	if (err == 0) {
>> -		memset(&buf[nread], 0, PAGE_SIZE - nread);
>> -		flush_dcache_page(page);
>> -		SetPageUptodate(page);
>> -	} else {
>> -		SetPageError(page);
>> -	}
>> +		err = vboxsf_read(sf_handle->root, sf_handle->handle, pos,
>> +				&nread, buf);
>> +		if (nread < PAGE_SIZE)
>> +			memset(&buf[nread], 0, PAGE_SIZE - nread);
>> +		kunmap_local(buf);
>> +		if (err)
>> +			break;
>> +		offset += PAGE_SIZE;
>> +		pos += PAGE_SIZE;
>> +	} while (offset < folio_size(folio);
>>  
>> -	kunmap(page);
>> -	unlock_page(page);
>> +	if (!err) {
>> +		flush_dcache_folio(folio);
>> +		folio_mark_uptodate(folio);
>> +	}
>> +	folio_unlock(folio);
>>  	return err;
>>  }
>>
> 
> Hi 
> 
> So, after reading the comments, I understood that the problem presented 
> by Hans and Matthew is as follows:
> 
> 1) In the current code, the buffers used by vboxsf_write()/vboxsf_read() are 
> translated to PAGELIST-s before passing to the hypervisor, 
> but inefficiently— it first maps a page in vboxsf_read_folio() and then 
> calls page_to_phys(virt_to_page()) in the function hgcm_call_init_linaddr(). 
> 
> The inefficiency in the current implementation arises due to the unnecessary 
> mapping of a page in vboxsf_read_folio() because the mapping output, i.e. the 
> linear address, is used deep down in file 'drivers/virt/vboxguest/vboxguest_utils.c'. 
> Hence, the mapping must be done in this file; to do so, the folio must be passed 
> until this point. It can be done by adding a new member, 'struct folio *folio', 
> in the 'struct vmmdev_hgcm_function_parameter64'. 

struct vmmdev_hgcm_function_parameter64 is defined in

include/uapi/linux/vbox_vmmdev_types.h

This is part of the userspace API of vboxguest which allows userspace
to make (some) VirtualBox hypervisor request through a chardev.

You can not just go and change this struct.

Note there already is a VMMDEV_HGCM_PARM_TYPE_PAGELIST type. So you
could do the conversion from folio to pagelist (see the vboxguest code
for how to do this) in the vboxsf_read() code (making it take a folio
pointer as arg) and then directly pass the pagelist to the vbg_hgcm_call().

> The unused member 'phys_addr' in this struct can also be removed.

Again no you can NOT just go and make changes to an uapi header.

Regards,

Hans