On mercoledì 28 giugno 2023 19:15:04 CEST 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.
Interesting...
> 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 {
Please let me understand why you are calling vboxsf_read() in a loop, a
PAGE_SIZE at a time.
I have had only few minutes (whereas I'd need more time) to look at this code.
If I understand the current code it reads a single page at offset zero of a
folio and then memset() with zeros from &buf[nread] up to the end of the page.
Then it seems that this function currently assume that the folio doesn't need
to be read until "offset < folio_size(folio)" becomes false.
Does it imply that the folio is always one page sized? Doesn't it? I'm surely
missing some basics...
> + 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);
Shouldn't we call folio_lock() to lock the folio to be able to unlock with
folio_unlock()?
If so, I can't find any neither a folio_lock() or a page_lock() in this
filesystem.
Again sorry for not understanding, can you please explain it?
> return err;
> }
Thanks,
Fabio
