Re: [PATCH v3 10/10] virtiofsd: Add lazy lo_do_find()

[Hanna Reitz]

On 10.08.21 16:12, Vivek Goyal wrote:
> On Tue, Aug 10, 2021 at 10:38:32AM +0200, Hanna Reitz wrote:
> > On 09.08.21 21:08, Vivek Goyal wrote:
> > > On Fri, Jul 30, 2021 at 05:01:34PM +0200, Max Reitz wrote:
> > > > lo_find() right now takes two lookup keys for two maps, namely the file
> > > > handle for inodes_by_handle and the statx information for inodes_by_ids.
> > > > However, we only need the statx information if looking up the inode by
> > > > the file handle failed.
> > > >
> > > > There are two callers of lo_find(): The first one, lo_do_lookup(), has
> > > > both keys anyway, so passing them does not incur any additional cost.
> > > > The second one, lookup_name(), though, needs to explicitly invoke
> > > > name_to_handle_at() (through get_file_handle()) and statx() (through
> > > > do_statx()).  We need to try to get a file handle as the primary key, so
> > > > we cannot get rid of get_file_handle(), but we only need the statx
> > > > information if looking up an inode by handle failed; so we can defer
> > > > that until the lookup has indeed failed.
> > > So IIUC, this patch seems to be all about avoiding do_statx()
> > > call in lookup_name() if file handle could be successfully
> > > generated.
> > >
> > > So can't we just not modify lookup_name() to not call statx()
> > > if file handle could be generated. And also modfiy lo_find()
> > > to use st/mnt_id only if fhandle==NULL.
> > >
> > > That probably is much simpler change as compared to passing function
> > > pointers around.
> > Definitely, but I don’t know whether it’s correct.
> What problem do you see from correctness point of view.

Again assuming that file handle generation can randomly fail (this time 
assuming it failed the first time, and later may succeed), it’s possible 
we have an lo_inode that we want to look up that does not have a file 
handle, but for the lookup we were able to generate a file handle for 
it.  In such a case, we need to call statx() to get st_ino/st_dev/mnt_id.

> > Or, we can just drop this patch and say that we don’t need to over-optimize
> > C virtiofsd.
> Rust version is used by very few people, while C version is in production.
> So I will definitely optimize C version. Once rust version is widely
> available and available in product, then we can start paying less
> attention to C version, IMHO.

OK, it was just an offer.  I mean, I myself wrote this patch as an 
optimization after all. :)

Hanna