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;
MIME-Version: 1.0
References: <20220517100744.26849-1-dharamhans87@gmail.com>
In-Reply-To: <20220517100744.26849-1-dharamhans87@gmail.com>
From:   Miklos Szeredi <miklos@szeredi.hu>
Date:   Thu, 19 May 2022 11:39:01 +0200
Message-ID: <CAJfpegsDxsMsyfP4a_5H1q91xFtwcEdu9-WBnzWKwjUSrPNdmw@mail.gmail.com>
Subject: Re: [PATCH v5 0/3] FUSE: Implement atomic lookup + open/create
To:     Dharmendra Singh <dharamhans87@gmail.com>
Cc:     Vivek Goyal <vgoyal@redhat.com>, linux-fsdevel@vger.kernel.org,
        fuse-devel <fuse-devel@lists.sourceforge.net>,
        linux-kernel@vger.kernel.org, Bernd Schubert <bschubert@ddn.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, 17 May 2022 at 12:08, Dharmendra Singh <dharamhans87@gmail.com> wrote:
>
> In FUSE, as of now, uncached lookups are expensive over the wire.
> E.g additional latencies and stressing (meta data) servers from
> thousands of clients. These lookup calls possibly can be avoided
> in some cases. Incoming three patches address this issue.
>
>
> Fist patch handles the case where we are creating a file with O_CREAT.
> Before we go for file creation, we do a lookup on the file which is most
> likely non-existent. After this lookup is done, we again go into libfuse
> to create file. Such lookups where file is most likely non-existent, can
> be avoided.

I'd really like to see a bit wider picture...

We have several cases, first of all let's look at plain O_CREAT
without O_EXCL (assume that there were no changes since the last
lookup for simplicity):

[not cached, negative]
   ->atomic_open()
      LOOKUP
      CREATE

[not cached, positive]
   ->atomic_open()
      LOOKUP
   ->open()
      OPEN

[cached, negative, validity timeout not expired]
   ->d_revalidate()
      return 1
   ->atomic_open()
      CREATE

[cached, negative, validity timeout expired]
   ->d_revalidate()
      return 0
   ->atomic_open()
      LOOKUP
      CREATE

[cached, positive, validity timeout not expired]
   ->d_revalidate()
      return 1
   ->open()
      OPEN

[cached, positive, validity timeout expired]
   ->d_revalidate()
      LOOKUP
      return 1
   ->open()
      OPEN

(Caveat emptor: I'm just looking at the code and haven't actually
tested what happens.)

Apparently in all of these cases we are doing at least one request, so
it would make sense to make them uniform:

[not cached]
   ->atomic_open()
      CREATE_EXT

[cached]
   ->d_revalidate()
      return 0
   ->atomic_open()
      CREATE_EXT

Similarly we can look at the current O_CREAT | O_EXCL cases:

[not cached, negative]
   ->atomic_open()
      LOOKUP
      CREATE

[not cached, positive]
   ->atomic_open()
      LOOKUP
   return -EEXIST

[cached, negative]
   ->d_revalidate()
      return 0 (see LOOKUP_EXCL check)
   ->atomic_open()
      LOOKUP
      CREATE

[cached, positive]
   ->d_revalidate()
      LOOKUP
      return 1
   return -EEXIST

Again we are doing at least one request, so we can unconditionally
replace them with CREATE_EXT like the non-O_EXCL case.


>
> Second patch handles the case where we open first time a file/dir
> but do a lookup first on it. After lookup is performed we make another
> call into libfuse to open the file. Now these two separate calls into
> libfuse can be combined and performed as a single call into libfuse.

And here's my analysis:

[not cached, negative]
   ->lookup()
      LOOKUP
   return -ENOENT

[not cached, positive]
   ->lookup()
      LOOKUP
   ->open()
      OPEN

[cached, negative, validity timeout not expired]
    ->d_revalidate()
       return 1
    return -ENOENT

[cached, negative, validity timeout expired]
   ->d_revalidate()
      return 0
   ->atomic_open()
      LOOKUP
   return -ENOENT

[cached, positive, validity timeout not expired]
   ->d_revalidate()
      return 1
   ->open()
      OPEN

[cached, positive, validity timeout expired]
   ->d_revalidate()
      LOOKUP
      return 1
   ->open()
      OPEN

There's one case were no request is sent:  a valid cached negative
dentry.   Possibly we can also make this uniform, e.g.:

[not cached]
   ->atomic_open()
       OPEN_ATOMIC

[cached, negative, validity timeout not expired]
    ->d_revalidate()
       return 1
    return -ENOENT

[cached, negative, validity timeout expired]
   ->d_revalidate()
      return 0
   ->atomic_open()
      OPEN_ATOMIC

[cached, positive]
   ->d_revalidate()
      return 0
   ->atomic_open()
      OPEN_ATOMIC

It may even make the code simpler to clearly separate the cases where
the atomic variants are supported and when not.  I'd also consider
merging CREATE_EXT into OPEN_ATOMIC, since a filesystem implementing
one will highly likely want to implement the other as well.

Thanks,
Miklos