Re: [PATCH v5 0/3] FUSE: Implement atomic lookup + open/create

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux