Mailing List Archive

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

On Thu, 19 May 2022 at 11:39, Miklos Szeredi <miklos@szeredi.hu> wrote:

> 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

Note to self: invalidating a valid positive dentry would break things.

Revalidation would need to be moved into ->atomic_open(), which is a
bigger surgery. Oh well...

Thanks,
Miklos

On 5/19/22 11:39, Miklos Szeredi wrote:
> 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.


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

On Thu, 19 May 2022 at 19:42, Bernd Schubert <bschubert@ddn.com> wrote:

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

Make it clear in the code flow if we are using the new request or the
old; e.g. rename current fuse_atomic_open() to fuse_open_nonatomic()
and do

static int fuse_open_atomic(...)
{
...
args.opcode = FUSE_OPEN_ATOMIC;
...
err = fuse_simple_request(...);
if (err == -ENOSYS)
goto fallback;
...
fallback:
return fuse_open_nonatomic();
}

static int fuse_atomic_open(...)
{
if (fc->no_open_atomic)
return fuse_open_nonatomic();
else
return fuse_open_atomic();
}

Also we can tweak fuse_dentry_revalidate() so it always invalidates
negative dentries if the new atomic open is available, and possibly
for positive dentries as well, if the rfc patch makes it.

Thanks,
Miklos

On Thu, May 19, 2022 at 11:39:01AM +0200, Miklos Szeredi wrote:
> 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):

Hi Miklos,

Thanks for providing this breakup. There are too many cases here and
this data helps a lot with that. I feel this should really be captured
in commit logs to show the current paths and how these have been
optimized with ATOMIC_OPEN/CREATE_EXT.

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

So fuse_dentry_revalidate() will return 0 even if timeout has not
expired (if server supports so called atomic_open()).
And that will lead to calling d_invalidate() on existing positive dentry
always. IOW, if I am calling open() on a dentry, dentry will always be
dropped and a new dentry will always be created from ->atomic_open() path,
is that right.

I am not sure what does it mean from VFS perspective to always call
d_invalidate() on a cached positive dentry when open() is called.

/**
* d_invalidate - detach submounts, prune dcache, and drop
* @dentry: dentry to invalidate (aka detach, prune and drop)
*/

Thanks
Vivek

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

On Thu, 1 Jun 2023 at 13:17, Bernd Schubert <bschubert@ddn.com> wrote:
>
> Hi Miklos,
>
> On 5/19/22 11:39, Miklos Szeredi wrote:
> > 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]
> > ->atomic_open()
> > OPEN_ATOMIC
>
> new patch version is eventually going through xfstests (and it finds
> some issues), but I have a question about wording here. Why
> "OPEN_ATOMIC" and not "ATOMIC_OPEN". Based on your comment @Dharmendra
> renamed all functions and this fuse op "open atomic" instead of "atomic
> open" - for my non native English this sounds rather weird. At best it
> should be "open atomically"?

FUSE_OPEN_ATOMIC is a specialization of FUSE_OPEN. Does that explain
my thinking?

Thanks,
Miklos

Hi Miklos,

On 5/19/22 11:39, Miklos Szeredi wrote:
> 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]
> ->atomic_open()
> OPEN_ATOMIC

new patch version is eventually going through xfstests (and it finds
some issues), but I have a question about wording here. Why
"OPEN_ATOMIC" and not "ATOMIC_OPEN". Based on your comment @Dharmendra
renamed all functions and this fuse op "open atomic" instead of "atomic
open" - for my non native English this sounds rather weird. At best it
should be "open atomically"?


Thanks,
Bernd

On 6/1/23 13:50, Miklos Szeredi wrote:
> On Thu, 1 Jun 2023 at 13:17, Bernd Schubert <bschubert@ddn.com> wrote:
>>
>> Hi Miklos,
>>
>> On 5/19/22 11:39, Miklos Szeredi wrote:
>>> 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]
>>> ->atomic_open()
>>> OPEN_ATOMIC
>>
>> new patch version is eventually going through xfstests (and it finds
>> some issues), but I have a question about wording here. Why
>> "OPEN_ATOMIC" and not "ATOMIC_OPEN". Based on your comment @Dharmendra
>> renamed all functions and this fuse op "open atomic" instead of "atomic
>> open" - for my non native English this sounds rather weird. At best it
>> should be "open atomically"?
>
> FUSE_OPEN_ATOMIC is a specialization of FUSE_OPEN. Does that explain
> my thinking?

Yeah, just the vfs function is also called atomic_open. We now have


static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
struct file *file, unsigned flags,
umode_t mode)
{
struct fuse_conn *fc = get_fuse_conn(dir);

if (fc->no_open_atomic)
return fuse_open_nonatomic(dir, entry, file, flags, mode);
else
return fuse_open_atomic(dir, entry, file, flags, mode);
}


Personally I would use something like _fuse_atomic_open() and
fuse_create_open() (instead of fuse_open_nonatomic). The order of "open
atomic" also made it into libfuse and comments - it just sounds a bit
weird ;) I have to live with it, if you prefer it like this.


Thanks,
Bernd

On Thu, 1 Jun 2023 at 14:01, Bernd Schubert <bschubert@ddn.com> wrote:
>
> On 6/1/23 13:50, Miklos Szeredi wrote:
> > On Thu, 1 Jun 2023 at 13:17, Bernd Schubert <bschubert@ddn.com> wrote:
> >>
> >> Hi Miklos,
> >>
> >> On 5/19/22 11:39, Miklos Szeredi wrote:
> >>> 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]
> >>> ->atomic_open()
> >>> OPEN_ATOMIC
> >>
> >> new patch version is eventually going through xfstests (and it finds
> >> some issues), but I have a question about wording here. Why
> >> "OPEN_ATOMIC" and not "ATOMIC_OPEN". Based on your comment @Dharmendra
> >> renamed all functions and this fuse op "open atomic" instead of "atomic
> >> open" - for my non native English this sounds rather weird. At best it
> >> should be "open atomically"?
> >
> > FUSE_OPEN_ATOMIC is a specialization of FUSE_OPEN. Does that explain
> > my thinking?
>
> Yeah, just the vfs function is also called atomic_open. We now have
>
>
> static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
> struct file *file, unsigned flags,
> umode_t mode)
> {
> struct fuse_conn *fc = get_fuse_conn(dir);
>
> if (fc->no_open_atomic)
> return fuse_open_nonatomic(dir, entry, file, flags, mode);
> else
> return fuse_open_atomic(dir, entry, file, flags, mode);
> }
>
>
> Personally I would use something like _fuse_atomic_open() and
> fuse_create_open() (instead of fuse_open_nonatomic). The order of "open
> atomic" also made it into libfuse and comments - it just sounds a bit
> weird ;) I have to live with it, if you prefer it like this.

I'd prefer FUSE_OPEN_ATOMIC for the API, but I don't care about
function names, as long as the purpose is clear.

Thanks,
Miklos