On 5/19/22 11:39, Miklos Szeredi wrote:
On Tue, 17 May 2022 at 12:08, Dharmendra Singh <dharamhans87@xxxxxxxxx> 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.
Can you help me a bit to understand what we should change? I had also
already thought to merge CREATE_EXT and OPEN_ATOMIC - so agreed.
Shall we make the other cases more visible?
Also thanks a lot for you revalidate patch.
Thanks,
Bernd
Thanks,
Bernd
