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;
Date:   Thu, 29 Jun 2023 04:16:04 +0100
From:   Matthew Wilcox <willy@infradead.org>
To:     "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
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}()
Message-ID: <ZJz3dO10o9+xV65F@casper.infradead.org>
References: <20230627135115.GA452832@sumitra.com>
 <ZJxqmEVKoxxftfXM@casper.infradead.org>
 <6924669.18pcnM708K@suse>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <6924669.18pcnM708K@suse>
Precedence: bulk

On Thu, Jun 29, 2023 at 12:23:54AM +0200, Fabio M. De Francesco wrote:
> > -	buf = kmap(page);
> > +	do {
> 
> Please let me understand why you are calling vboxsf_read() in a loop, a 
> PAGE_SIZE at a time.

Because kmap_local_folio() can only (guarantee to) map one page at a
time.  Also vboxsf_read() is only tested with a single page at a time.

> 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...  

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.

> > -	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?

Ira gave the minimal explanation, but a slightly fuller explanation is
that the folio is locked while it is being fetched from backing store.
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.

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.