Re: [PATCH 25/30] vboxsf: Convert vboxsf_read_folio() to use a folio

[Hans de Goede <hdegoede@redhat.com>]

Hi Matthew,

On 4/20/24 4:50 AM, Matthew Wilcox (Oracle) wrote:
> Remove conversion to a page and use folio APIs throughout.  This includes
> a removal of setting the error flag as nobody checks the error flag on
> vboxsf folios.  This does not include large folio support as we would
> have to map each page individually.
> 
> Cc: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  fs/vboxsf/file.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/vboxsf/file.c b/fs/vboxsf/file.c
> index 118dedef8ebe..e149158b105d 100644
> --- a/fs/vboxsf/file.c
> +++ b/fs/vboxsf/file.c
> @@ -228,26 +228,19 @@ 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);
> +	loff_t off = folio_pos(folio);
>  	u32 nread = PAGE_SIZE;
>  	u8 *buf;
>  	int err;
>  
> -	buf = kmap(page);
> +	buf = kmap_local_folio(folio, 0);
>  
>  	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);
> -	}
> +	buf = folio_zero_tail(folio, nread, buf);
>  
> -	kunmap(page);
> -	unlock_page(page);
> +	kunmap_local(buf);
> +	folio_end_read(folio, err == 0);
>  	return err;
>  }
>  

Thanks you for the patch.

I have this a test spin, but I got all 0 content for the kernel's README when trying
to read that from a directory shared through vboxsf.

I came up with the following fix for this, which I assume is the correct fix,
but please check:

--- a/fs/vboxsf/file.c
+++ b/fs/vboxsf/file.c
@@ -237,7 +237,7 @@ static int vboxsf_read_folio(struct file *file, struct folio *folio)
 	buf = kmap_local_folio(folio, 0);
 
 	err = vboxsf_read(sf_handle->root, sf_handle->handle, off, &nread, buf);
-	buf = folio_zero_tail(folio, nread, buf);
+	buf = folio_zero_tail(folio, nread, buf + nread);
 
 	kunmap_local(buf);
 	folio_end_read(folio, err == 0);


With this fix squashed in, this looks good to me and you can add:

Tested-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Feel free to merge this together with the other folio patches from this series.

Regards,

Hans






> @@ -295,7 +288,6 @@ static int vboxsf_writepage(struct page *page, struct writeback_control *wbc)
>  	kref_put(&sf_handle->refcount, vboxsf_handle_release);
>  
>  	if (err == 0) {
> -		ClearPageError(page);
>  		/* mtime changed */
>  		sf_i->force_restat = 1;
>  	} else {