Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
To:     Matthew Wilcox <willy@infradead.org>
Cc:     Sumitra Sharma <sumitraartsy@gmail.com>,
        Hans de Goede <hdegoede@redhat.com>,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        Ira Weiny <ira.weiny@intel.com>, Deepak R Varma <drv@mailo.com>
Subject: Re: [PATCH] fs/vboxsf: Replace kmap() with kmap_local_{page, folio}()
Date:   Thu, 29 Jun 2023 17:04:26 +0200
Message-ID: <1810516.8hzESeGDPO@suse>
In-Reply-To: <ZJz3dO10o9+xV65F@casper.infradead.org>
References: <20230627135115.GA452832@sumitra.com> <6924669.18pcnM708K@suse>
 <ZJz3dO10o9+xV65F@casper.infradead.org>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="iso-8859-1"
Precedence: bulk

On gioved=EC 29 giugno 2023 05:16:04 CEST Matthew Wilcox wrote:
> On Thu, Jun 29, 2023 at 12:23:54AM +0200, Fabio M. De Francesco wrote:
> > > -	buf =3D kmap(page);
> > > +	do {
> >=20
> > Please let me understand why you are calling vboxsf_read() in a loop, a
> > PAGE_SIZE at a time.
>=20
> Because kmap_local_folio() can only (guarantee to) map one page at a
> time.

Yes, one page at a time. This part served to introduce the _main_ question=
=20
that is the one you answered below (i.e., since the current code maps a pag=
e a=20
a time with no loops, do we need to manage folios spanning more than a sing=
le=20
page?)

> Also vboxsf_read() is only tested with a single page at a time.
>=20
> > If I understand the current code it reads a single page at offset zero =
of=20
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 fals=
e.
> >=20
> > Does it imply that the folio is always one page sized? Doesn't it? I'm
> > surely
> > missing some basics...
>=20
> vboxsf does not yet claim to support large folios, so every folio that
> it sees will be only a single page in size.
> Hopefully at some point
> that will change.  Again, somebody would need to test that.  In the
> meantime, if someone is going to the trouble of switching over to using
> the folio API, let's actually include support for large folios.

"[...] at some point that will change." wrt larger folios spanning multiple=
=20
pages is the answer that justifies the loop. I couldn't know about this pla=
n.=20
Thanks for explaining that there is a plan towards that goal.

I think that Sumitra can address the task to re-use your patch to=20
vboxsf_read_folio() and then properly test it with VirtualBox.

Instead the much larger effort to implement vboxsf_readahead() and actually=
 do=20
an async call with deferred setting of the uptodate flag will surely requir=
e a=20
considerable amount of time and whoever wanted to address it would need you=
r=20
guidance.

You said that you are ready to provide consult, but I'm not sure whether=20
Sumitra would be allowed to spend a large part of her time to do an out of=
=20
scope task (wrt her internship).

If yes, I have nothing against. If not, I'm pretty sure that someone else c=
an=20
set aside enough time to address this large task ;-)

>=20
> > > -	kunmap(page);
> > > -	unlock_page(page);
> > > +	if (!err) {
> > > +		flush_dcache_folio(folio);
> > > +		folio_mark_uptodate(folio);
> > > +	}
> > > +	folio_unlock(folio);
> >=20
> > Shouldn't we call folio_lock() to lock the folio to be able to unlock w=
ith
> > folio_unlock()?
> >=20
> > If so, I can't find any neither a folio_lock() or a page_lock() in this
> > filesystem.
> >=20
> > Again sorry for not understanding, can you please explain it?
>=20
> Ira gave the minimal explanation, but a slightly fuller explanation is
> that the folio is locked while it is being fetched from backing store.

This explains why I could not easily find the call to lock it.=20

> That prevents both a second thread from reading from it while another
> thread is bringing it uptodate, and two threads trying to bring it
> uptodate at the same time.
>=20
> Most filesystems have an asynchronous read_folio, so you don't see the
> folio_unlock() in the read_folio() function; instead it's in the I/O
> completion path.  vboxsf is synchronous.

And this explains different implementation between synchronous and=20
asynchronous reads=20

Again thanks,

=46abio