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

[Vivek Goyal <vgoyal@redhat.com>]

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
>