diff mbox series

[RFC] vfs: allow ->atomic_open() on positive

Message ID YoZXro9PoYAPUeh5@miu.piliscsaba.redhat.com (mailing list archive)
State New, archived
Headers show
Series [RFC] vfs: allow ->atomic_open() on positive | expand

Commit Message

Miklos Szeredi May 19, 2022, 2:43 p.m. UTC 
Hi Al,

Do you see anything bad with allowing ->atomic_open() to take a positive dentry
and possibly invalidate it after it does the atomic LOOKUP/CREATE+OPEN?

It looks wrong not to allow optimizing away the roundtrip associated with
revalidation when we do allow optimizing away the roundtrip for the initial
lookup in the same situation.

Thanks,
Miklos

Comments

Vivek Goyal May 19, 2022, 8:09 p.m. UTC | #1 
On Thu, May 19, 2022 at 04:43:58PM +0200, Miklos Szeredi wrote:
> Hi Al,
> 
> Do you see anything bad with allowing ->atomic_open() to take a positive dentry
> and possibly invalidate it after it does the atomic LOOKUP/CREATE+OPEN?
> 
> It looks wrong not to allow optimizing away the roundtrip associated with
> revalidation when we do allow optimizing away the roundtrip for the initial
> lookup in the same situation.
> 
> Thanks,
> Miklos
> 
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 509657fdf4f5..d35b5cbf7f64 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3267,7 +3267,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
>  		dput(dentry);
>  		dentry = NULL;
>  	}
> -	if (dentry->d_inode) {
> +	if (dentry->d_inode && !d_atomic_open(dentry)) {
>  		/* Cached positive dentry: will open in f_op->open */
>  		return dentry;

Hi Miklos,

I see that lookup_open() calls d_revalidate() first. So basically
idea is that fuse ->.d_revalidate will skip LOOKUP needed to make sure
dentry is still valid (Only if atomic lookup+open is implemented) and
return 1 claiming dentry is valid.

And later in ->atomic_open(), it will either open the file or 
get an error and invalidate dentry. Hence will save one LOOKUP in
success case. Do I understand the intent right?

Thanks
Vivek

>  	}
> diff --git a/include/linux/dcache.h b/include/linux/dcache.h
> index f5bba51480b2..da681bdbc34e 100644
> --- a/include/linux/dcache.h
> +++ b/include/linux/dcache.h
> @@ -208,6 +208,7 @@ struct dentry_operations {
>  #define DCACHE_NOKEY_NAME		0x02000000 /* Encrypted name encoded without key */
>  #define DCACHE_OP_REAL			0x04000000
>  
> +#define DCACHE_ATOMIC_OPEN		0x08000000 /* Always use ->atomic_open() to open this file */
>  #define DCACHE_PAR_LOOKUP		0x10000000 /* being looked up (with parent locked shared) */
>  #define DCACHE_DENTRY_CURSOR		0x20000000
>  #define DCACHE_NORCU			0x40000000 /* No RCU delay for freeing */
> @@ -446,6 +447,11 @@ static inline bool d_is_positive(const struct dentry *dentry)
>  	return !d_is_negative(dentry);
>  }
>  
> +static inline bool d_atomic_open(const struct dentry *dentry)
> +{
> +	return dentry->d_flags & DCACHE_ATOMIC_OPEN;
> +}
> +
>  /**
>   * d_really_is_negative - Determine if a dentry is really negative (ignoring fallthroughs)
>   * @dentry: The dentry in question
>
Bernd Schubert May 19, 2022, 8:43 p.m. UTC | #2 
On 5/19/22 22:09, Vivek Goyal wrote:
> On Thu, May 19, 2022 at 04:43:58PM +0200, Miklos Szeredi wrote:
>> Hi Al,
>>
>> Do you see anything bad with allowing ->atomic_open() to take a positive dentry
>> and possibly invalidate it after it does the atomic LOOKUP/CREATE+OPEN?
>>
>> It looks wrong not to allow optimizing away the roundtrip associated with
>> revalidation when we do allow optimizing away the roundtrip for the initial
>> lookup in the same situation.
>>
>> Thanks,
>> Miklos
>>
>>
>> diff --git a/fs/namei.c b/fs/namei.c
>> index 509657fdf4f5..d35b5cbf7f64 100644
>> --- a/fs/namei.c
>> +++ b/fs/namei.c
>> @@ -3267,7 +3267,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
>>   		dput(dentry);
>>   		dentry = NULL;
>>   	}
>> -	if (dentry->d_inode) {
>> +	if (dentry->d_inode && !d_atomic_open(dentry)) {
>>   		/* Cached positive dentry: will open in f_op->open */
>>   		return dentry;
> 
> Hi Miklos,
> 
> I see that lookup_open() calls d_revalidate() first. So basically
> idea is that fuse ->.d_revalidate will skip LOOKUP needed to make sure
> dentry is still valid (Only if atomic lookup+open is implemented) and
> return 1 claiming dentry is valid.
> 
> And later in ->atomic_open(), it will either open the file or
> get an error and invalidate dentry. Hence will save one LOOKUP in
> success case. Do I understand the intent right?

Yeah, I think Dharmendra and I had internally already debated over this. 
In order to reduce complexity for the patches we preferred to go without 
vfs modifications.

I assume the patch is a follow up to this comment
https://lore.kernel.org/all/20220517100744.26849-1-dharamhans87@gmail.com/T/#m8bd440ddea4c135688c829f34e93371e861ba9fa


Thanks,
Bernd
Vivek Goyal May 19, 2022, 8:50 p.m. UTC | #3 
On Thu, May 19, 2022 at 10:43:54PM +0200, Bernd Schubert wrote:
> 
> 
> On 5/19/22 22:09, Vivek Goyal wrote:
> > On Thu, May 19, 2022 at 04:43:58PM +0200, Miklos Szeredi wrote:
> > > Hi Al,
> > > 
> > > Do you see anything bad with allowing ->atomic_open() to take a positive dentry
> > > and possibly invalidate it after it does the atomic LOOKUP/CREATE+OPEN?
> > > 
> > > It looks wrong not to allow optimizing away the roundtrip associated with
> > > revalidation when we do allow optimizing away the roundtrip for the initial
> > > lookup in the same situation.
> > > 
> > > Thanks,
> > > Miklos
> > > 
> > > 
> > > diff --git a/fs/namei.c b/fs/namei.c
> > > index 509657fdf4f5..d35b5cbf7f64 100644
> > > --- a/fs/namei.c
> > > +++ b/fs/namei.c
> > > @@ -3267,7 +3267,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
> > >   		dput(dentry);
> > >   		dentry = NULL;
> > >   	}
> > > -	if (dentry->d_inode) {
> > > +	if (dentry->d_inode && !d_atomic_open(dentry)) {
> > >   		/* Cached positive dentry: will open in f_op->open */
> > >   		return dentry;
> > 
> > Hi Miklos,
> > 
> > I see that lookup_open() calls d_revalidate() first. So basically
> > idea is that fuse ->.d_revalidate will skip LOOKUP needed to make sure
> > dentry is still valid (Only if atomic lookup+open is implemented) and
> > return 1 claiming dentry is valid.
> > 
> > And later in ->atomic_open(), it will either open the file or
> > get an error and invalidate dentry. Hence will save one LOOKUP in
> > success case. Do I understand the intent right?
> 
> Yeah, I think Dharmendra and I had internally already debated over this. In
> order to reduce complexity for the patches we preferred to go without vfs
> modifications.

Fair enough. It is not trivial to be able to see all the paths and make
sure none of these paths is broken.

> 
> I assume the patch is a follow up to this comment
> https://lore.kernel.org/all/20220517100744.26849-1-dharamhans87@gmail.com/T/#m8bd440ddea4c135688c829f34e93371e861ba9fa
> 

Yes looks like. There are too many paths here and being able to wrap
one's head around all the paths is not trivial. Thankfully miklos
has summarized it here.

https://lore.kernel.org/all/20220517100744.26849-1-dharamhans87@gmail.com/T/#m90f64cd8c8fff70e2fba2b551ae01d0d47b3337e

Thanks
Vivek
Al Viro May 20, 2022, 4:07 a.m. UTC | #4 
On Thu, May 19, 2022 at 04:43:58PM +0200, Miklos Szeredi wrote:
> Hi Al,
> 
> Do you see anything bad with allowing ->atomic_open() to take a positive dentry
> and possibly invalidate it after it does the atomic LOOKUP/CREATE+OPEN?
> 
> It looks wrong not to allow optimizing away the roundtrip associated with
> revalidation when we do allow optimizing away the roundtrip for the initial
> lookup in the same situation.

Details, please - what will your ->atomic_open() do in that case?
Miklos Szeredi May 20, 2022, 6:44 a.m. UTC | #5 
On Fri, 20 May 2022 at 06:07, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Thu, May 19, 2022 at 04:43:58PM +0200, Miklos Szeredi wrote:
> > Hi Al,
> >
> > Do you see anything bad with allowing ->atomic_open() to take a positive dentry
> > and possibly invalidate it after it does the atomic LOOKUP/CREATE+OPEN?
> >
> > It looks wrong not to allow optimizing away the roundtrip associated with
> > revalidation when we do allow optimizing away the roundtrip for the initial
> > lookup in the same situation.
>
> Details, please - what will your ->atomic_open() do in that case?

It will do an open-by-name or create-and-open depending on the flags.
It will return whether the file was created or not.

If created, then the old positive is obviously stale, so it will be
invalidated and a new one allocated.

If not created, then check whether it's the same inode (same as in
->d_revalidate()) and if not, invalidate & allocate new dentry.

Thanks,
Miklos
Miklos Szeredi May 20, 2022, 6:48 a.m. UTC | #6 
On Thu, 19 May 2022 at 22:09, Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Thu, May 19, 2022 at 04:43:58PM +0200, Miklos Szeredi wrote:
> > Hi Al,
> >
> > Do you see anything bad with allowing ->atomic_open() to take a positive dentry
> > and possibly invalidate it after it does the atomic LOOKUP/CREATE+OPEN?
> >
> > It looks wrong not to allow optimizing away the roundtrip associated with
> > revalidation when we do allow optimizing away the roundtrip for the initial
> > lookup in the same situation.
> >
> > Thanks,
> > Miklos
> >
> >
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 509657fdf4f5..d35b5cbf7f64 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -3267,7 +3267,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
> >               dput(dentry);
> >               dentry = NULL;
> >       }
> > -     if (dentry->d_inode) {
> > +     if (dentry->d_inode && !d_atomic_open(dentry)) {
> >               /* Cached positive dentry: will open in f_op->open */
> >               return dentry;
>
> Hi Miklos,
>
> I see that lookup_open() calls d_revalidate() first. So basically
> idea is that fuse ->.d_revalidate will skip LOOKUP needed to make sure
> dentry is still valid (Only if atomic lookup+open is implemented) and
> return 1 claiming dentry is valid.

Yes.

> And later in ->atomic_open(), it will either open the file or
> get an error and invalidate dentry. Hence will save one LOOKUP in
> success case. Do I understand the intent right?

It should not fail in the stale dentry case either, just merge the
revalidation into ->atomic_open().

Thanks,
Miklos
diff mbox series

Patch

diff --git a/fs/namei.c b/fs/namei.c
index 509657fdf4f5..d35b5cbf7f64 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3267,7 +3267,7 @@  static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 		dput(dentry);
 		dentry = NULL;
 	}
-	if (dentry->d_inode) {
+	if (dentry->d_inode && !d_atomic_open(dentry)) {
 		/* Cached positive dentry: will open in f_op->open */
 		return dentry;
 	}
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index f5bba51480b2..da681bdbc34e 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -208,6 +208,7 @@  struct dentry_operations {
 #define DCACHE_NOKEY_NAME		0x02000000 /* Encrypted name encoded without key */
 #define DCACHE_OP_REAL			0x04000000
 
+#define DCACHE_ATOMIC_OPEN		0x08000000 /* Always use ->atomic_open() to open this file */
 #define DCACHE_PAR_LOOKUP		0x10000000 /* being looked up (with parent locked shared) */
 #define DCACHE_DENTRY_CURSOR		0x20000000
 #define DCACHE_NORCU			0x40000000 /* No RCU delay for freeing */
@@ -446,6 +447,11 @@  static inline bool d_is_positive(const struct dentry *dentry)
 	return !d_is_negative(dentry);
 }
 
+static inline bool d_atomic_open(const struct dentry *dentry)
+{
+	return dentry->d_flags & DCACHE_ATOMIC_OPEN;
+}
+
 /**
  * d_really_is_negative - Determine if a dentry is really negative (ignoring fallthroughs)
  * @dentry: The dentry in question