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
