| Message ID | 20230816143313.2591328-4-bschubert@ddn.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuse: full atomic open and atomic-open-revalidate | expand |
On Wed, 16 Aug 2023 at 16:34, Bernd Schubert <bschubert@ddn.com> wrote: > > From: Miklos Szeredi <miklos@szeredi.hu> > > atomic_open() will do an open-by-name or create-and-open > depending on the flags. > > If file was created, then the old positive dentry is obviously > stale, so it will be invalidated and a new one will be allocated. > > If not created, then check whether it's the same inode (same as in > ->d_revalidate()) and if not, invalidate & allocate new dentry. > > Changes (v7 global series) from Miklos initial patch (by Bernd): > - LOOKUP_ATOMIC_REVALIDATE was added and is set for revalidate > calls into the file system when revalidate by atomic open is > supported - this is to avoid that ->d_revalidate() would skip > revalidate and set DCACHE_ATOMIC_OPEN, although vfs > does not supported it in the given code path (for example > when LOOKUP_RCU is set)). > - Support atomic-open-revalidate in lookup_fast() - allow atomic > open for positive dentries without O_CREAT being set. > > Changes (v8 global series) > - Introduce enum for d_revalidate return values > - LOOKUP_ATOMIC_REVALIDATE is removed again > - DCACHE_ATOMIC_OPEN flag is replaced by D_REVALIDATE_ATOMIC > return value > > Co-developed-by: Bernd Schubert <bschubert@ddn.com> > Signed-off-by: Miklos Szeredi <miklos@szeredi.hu> > Signed-off-by: Bernd Schubert <bschubert@ddn.com> > Cc: Christian Brauner <brauner@kernel.org> > Cc: Al Viro <viro@zeniv.linux.org.uk> > Cc: Dharmendra Singh <dsingh@ddn.com> > Cc: linux-fsdevel@vger.kernel.org > --- > fs/namei.c | 25 +++++++++++++++++++------ > include/linux/namei.h | 6 ++++++ > 2 files changed, 25 insertions(+), 6 deletions(-) > > diff --git a/fs/namei.c b/fs/namei.c > index e4fe0879ae55..8381ec7645f5 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -858,7 +858,7 @@ static inline int d_revalidate(struct dentry *dentry, unsigned int flags) > if (unlikely(dentry->d_flags & DCACHE_OP_REVALIDATE)) > return dentry->d_op->d_revalidate(dentry, flags); > else > - return 1; > + return D_REVALIDATE_VALID; > } > > /** > @@ -1611,10 +1611,11 @@ struct dentry *lookup_one_qstr_excl(const struct qstr *name, > } > EXPORT_SYMBOL(lookup_one_qstr_excl); > > -static struct dentry *lookup_fast(struct nameidata *nd) > +static struct dentry *lookup_fast(struct nameidata *nd, int *atomic_revalidate) bool? > { > struct dentry *dentry, *parent = nd->path.dentry; > int status = 1; > + *atomic_revalidate = 0; > > /* > * Rename seqlock is not required here because in the off chance > @@ -1656,6 +1657,10 @@ static struct dentry *lookup_fast(struct nameidata *nd) > dput(dentry); > return ERR_PTR(status); > } > + > + if (status == D_REVALIDATE_ATOMIC) > + *atomic_revalidate = 1; > + > return dentry; > } > > @@ -1981,6 +1986,7 @@ static const char *handle_dots(struct nameidata *nd, int type) > static const char *walk_component(struct nameidata *nd, int flags) > { > struct dentry *dentry; > + int atomic_revalidate; > /* > * "." and ".." are special - ".." especially so because it has > * to be able to know about the current root directory and > @@ -1991,7 +1997,7 @@ static const char *walk_component(struct nameidata *nd, int flags) > put_link(nd); > return handle_dots(nd, nd->last_type); > } > - dentry = lookup_fast(nd); > + dentry = lookup_fast(nd, &atomic_revalidate); > if (IS_ERR(dentry)) > return ERR_CAST(dentry); > if (unlikely(!dentry)) { > @@ -1999,6 +2005,9 @@ static const char *walk_component(struct nameidata *nd, int flags) > if (IS_ERR(dentry)) > return ERR_CAST(dentry); > } > + > + WARN_ON(atomic_revalidate); > + > if (!(flags & WALK_MORE) && nd->depth) > put_link(nd); > return step_into(nd, flags, dentry); > @@ -3430,7 +3439,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file, > dput(dentry); > dentry = NULL; > } > - if (dentry->d_inode) { > + if (dentry->d_inode && error != D_REVALIDATE_ATOMIC) { > /* Cached positive dentry: will open in f_op->open */ > return dentry; > } > @@ -3523,15 +3532,19 @@ static const char *open_last_lookups(struct nameidata *nd, > } > > if (!(open_flag & O_CREAT)) { > + int atomic_revalidate; > if (nd->last.name[nd->last.len]) > nd->flags |= LOOKUP_FOLLOW | LOOKUP_DIRECTORY; > /* we _can_ be in RCU mode here */ > - dentry = lookup_fast(nd); > + dentry = lookup_fast(nd, &atomic_revalidate); > if (IS_ERR(dentry)) > return ERR_CAST(dentry); > + if (dentry && unlikely(atomic_revalidate)) { Need to assert !LOOKUP_RCU > + dput(dentry); > + dentry = NULL; > + } Feels a shame to throw away the dentry. May be worth adding a helper for the plain atomic open, most of the complexity of lookup_open() is because of O_CREAT, so this should be much simplified. > if (likely(dentry)) > goto finish_lookup; > - Adding/removing empty lines is just a distraction, so it shouldn't be done unless it serves a real purpose. > BUG_ON(nd->flags & LOOKUP_RCU); > } else { > /* create side of things */ > diff --git a/include/linux/namei.h b/include/linux/namei.h > index 1463cbda4888..675fd6c88201 100644 > --- a/include/linux/namei.h > +++ b/include/linux/namei.h > @@ -47,6 +47,12 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT}; > /* LOOKUP_* flags which do scope-related checks based on the dirfd. */ > #define LOOKUP_IS_SCOPED (LOOKUP_BENEATH | LOOKUP_IN_ROOT) > > +enum { > + D_REVALIDATE_INVALID = 0, > + D_REVALIDATE_VALID = 1, > + D_REVALIDATE_ATOMIC = 2, /* Not allowed with LOOKUP_RCU */ > +}; > + > extern int path_pts(struct path *path); > > extern int user_path_at_empty(int, const char __user *, unsigned, struct path *, int *empty); > -- > 2.37.2 >
On 8/16/23 17:25, Miklos Szeredi wrote: > On Wed, 16 Aug 2023 at 16:34, Bernd Schubert <bschubert@ddn.com> wrote: >> >> From: Miklos Szeredi <miklos@szeredi.hu> >> >> atomic_open() will do an open-by-name or create-and-open >> depending on the flags. >> >> If file was created, then the old positive dentry is obviously >> stale, so it will be invalidated and a new one will be allocated. >> >> If not created, then check whether it's the same inode (same as in >> ->d_revalidate()) and if not, invalidate & allocate new dentry. >> >> Changes (v7 global series) from Miklos initial patch (by Bernd): >> - LOOKUP_ATOMIC_REVALIDATE was added and is set for revalidate >> calls into the file system when revalidate by atomic open is >> supported - this is to avoid that ->d_revalidate() would skip >> revalidate and set DCACHE_ATOMIC_OPEN, although vfs >> does not supported it in the given code path (for example >> when LOOKUP_RCU is set)). >> - Support atomic-open-revalidate in lookup_fast() - allow atomic >> open for positive dentries without O_CREAT being set. >> >> Changes (v8 global series) >> - Introduce enum for d_revalidate return values >> - LOOKUP_ATOMIC_REVALIDATE is removed again >> - DCACHE_ATOMIC_OPEN flag is replaced by D_REVALIDATE_ATOMIC >> return value >> >> Co-developed-by: Bernd Schubert <bschubert@ddn.com> >> Signed-off-by: Miklos Szeredi <miklos@szeredi.hu> >> Signed-off-by: Bernd Schubert <bschubert@ddn.com> >> Cc: Christian Brauner <brauner@kernel.org> >> Cc: Al Viro <viro@zeniv.linux.org.uk> >> Cc: Dharmendra Singh <dsingh@ddn.com> >> Cc: linux-fsdevel@vger.kernel.org >> --- >> fs/namei.c | 25 +++++++++++++++++++------ >> include/linux/namei.h | 6 ++++++ >> 2 files changed, 25 insertions(+), 6 deletions(-) >> >> diff --git a/fs/namei.c b/fs/namei.c >> index e4fe0879ae55..8381ec7645f5 100644 >> --- a/fs/namei.c >> +++ b/fs/namei.c >> @@ -858,7 +858,7 @@ static inline int d_revalidate(struct dentry *dentry, unsigned int flags) >> if (unlikely(dentry->d_flags & DCACHE_OP_REVALIDATE)) >> return dentry->d_op->d_revalidate(dentry, flags); >> else >> - return 1; >> + return D_REVALIDATE_VALID; >> } >> >> /** >> @@ -1611,10 +1611,11 @@ struct dentry *lookup_one_qstr_excl(const struct qstr *name, >> } >> EXPORT_SYMBOL(lookup_one_qstr_excl); >> >> -static struct dentry *lookup_fast(struct nameidata *nd) >> +static struct dentry *lookup_fast(struct nameidata *nd, int *atomic_revalidate) > > bool? > >> { >> struct dentry *dentry, *parent = nd->path.dentry; >> int status = 1; >> + *atomic_revalidate = 0; >> >> /* >> * Rename seqlock is not required here because in the off chance >> @@ -1656,6 +1657,10 @@ static struct dentry *lookup_fast(struct nameidata *nd) >> dput(dentry); >> return ERR_PTR(status); >> } >> + >> + if (status == D_REVALIDATE_ATOMIC) >> + *atomic_revalidate = 1; >> + >> return dentry; >> } >> >> @@ -1981,6 +1986,7 @@ static const char *handle_dots(struct nameidata *nd, int type) >> static const char *walk_component(struct nameidata *nd, int flags) >> { >> struct dentry *dentry; >> + int atomic_revalidate; >> /* >> * "." and ".." are special - ".." especially so because it has >> * to be able to know about the current root directory and >> @@ -1991,7 +1997,7 @@ static const char *walk_component(struct nameidata *nd, int flags) >> put_link(nd); >> return handle_dots(nd, nd->last_type); >> } >> - dentry = lookup_fast(nd); >> + dentry = lookup_fast(nd, &atomic_revalidate); >> if (IS_ERR(dentry)) >> return ERR_CAST(dentry); >> if (unlikely(!dentry)) { >> @@ -1999,6 +2005,9 @@ static const char *walk_component(struct nameidata *nd, int flags) >> if (IS_ERR(dentry)) >> return ERR_CAST(dentry); >> } >> + >> + WARN_ON(atomic_revalidate); >> + >> if (!(flags & WALK_MORE) && nd->depth) >> put_link(nd); >> return step_into(nd, flags, dentry); >> @@ -3430,7 +3439,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file, >> dput(dentry); >> dentry = NULL; >> } >> - if (dentry->d_inode) { >> + if (dentry->d_inode && error != D_REVALIDATE_ATOMIC) { >> /* Cached positive dentry: will open in f_op->open */ >> return dentry; >> } >> @@ -3523,15 +3532,19 @@ static const char *open_last_lookups(struct nameidata *nd, >> } >> >> if (!(open_flag & O_CREAT)) { >> + int atomic_revalidate; >> if (nd->last.name[nd->last.len]) >> nd->flags |= LOOKUP_FOLLOW | LOOKUP_DIRECTORY; >> /* we _can_ be in RCU mode here */ >> - dentry = lookup_fast(nd); >> + dentry = lookup_fast(nd, &atomic_revalidate); >> if (IS_ERR(dentry)) >> return ERR_CAST(dentry); >> + if (dentry && unlikely(atomic_revalidate)) { > > Need to assert !LOOKUP_RCU Are you sure? There is the BUG_ON(nd->flags & LOOKUP_RCU) directly after - should be enough? > >> + dput(dentry); >> + dentry = NULL; >> + } > > Feels a shame to throw away the dentry. May be worth adding a helper > for the plain atomic open, most of the complexity of lookup_open() is > because of O_CREAT, so this should be much simplified. Thanks, I'm going to look into it. > >> if (likely(dentry)) >> goto finish_lookup; >> - > > Adding/removing empty lines is just a distraction, so it shouldn't be > done unless it serves a real purpose. Ah sorry, accidentally. I'm going to travel the next two days, going to update it on Monday (or at best over the weekend). Thanks, Bernd
diff --git a/fs/namei.c b/fs/namei.c index e4fe0879ae55..8381ec7645f5 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -858,7 +858,7 @@ static inline int d_revalidate(struct dentry *dentry, unsigned int flags) if (unlikely(dentry->d_flags & DCACHE_OP_REVALIDATE)) return dentry->d_op->d_revalidate(dentry, flags); else - return 1; + return D_REVALIDATE_VALID; } /** @@ -1611,10 +1611,11 @@ struct dentry *lookup_one_qstr_excl(const struct qstr *name, } EXPORT_SYMBOL(lookup_one_qstr_excl); -static struct dentry *lookup_fast(struct nameidata *nd) +static struct dentry *lookup_fast(struct nameidata *nd, int *atomic_revalidate) { struct dentry *dentry, *parent = nd->path.dentry; int status = 1; + *atomic_revalidate = 0; /* * Rename seqlock is not required here because in the off chance @@ -1656,6 +1657,10 @@ static struct dentry *lookup_fast(struct nameidata *nd) dput(dentry); return ERR_PTR(status); } + + if (status == D_REVALIDATE_ATOMIC) + *atomic_revalidate = 1; + return dentry; } @@ -1981,6 +1986,7 @@ static const char *handle_dots(struct nameidata *nd, int type) static const char *walk_component(struct nameidata *nd, int flags) { struct dentry *dentry; + int atomic_revalidate; /* * "." and ".." are special - ".." especially so because it has * to be able to know about the current root directory and @@ -1991,7 +1997,7 @@ static const char *walk_component(struct nameidata *nd, int flags) put_link(nd); return handle_dots(nd, nd->last_type); } - dentry = lookup_fast(nd); + dentry = lookup_fast(nd, &atomic_revalidate); if (IS_ERR(dentry)) return ERR_CAST(dentry); if (unlikely(!dentry)) { @@ -1999,6 +2005,9 @@ static const char *walk_component(struct nameidata *nd, int flags) if (IS_ERR(dentry)) return ERR_CAST(dentry); } + + WARN_ON(atomic_revalidate); + if (!(flags & WALK_MORE) && nd->depth) put_link(nd); return step_into(nd, flags, dentry); @@ -3430,7 +3439,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file, dput(dentry); dentry = NULL; } - if (dentry->d_inode) { + if (dentry->d_inode && error != D_REVALIDATE_ATOMIC) { /* Cached positive dentry: will open in f_op->open */ return dentry; } @@ -3523,15 +3532,19 @@ static const char *open_last_lookups(struct nameidata *nd, } if (!(open_flag & O_CREAT)) { + int atomic_revalidate; if (nd->last.name[nd->last.len]) nd->flags |= LOOKUP_FOLLOW | LOOKUP_DIRECTORY; /* we _can_ be in RCU mode here */ - dentry = lookup_fast(nd); + dentry = lookup_fast(nd, &atomic_revalidate); if (IS_ERR(dentry)) return ERR_CAST(dentry); + if (dentry && unlikely(atomic_revalidate)) { + dput(dentry); + dentry = NULL; + } if (likely(dentry)) goto finish_lookup; - BUG_ON(nd->flags & LOOKUP_RCU); } else { /* create side of things */ diff --git a/include/linux/namei.h b/include/linux/namei.h index 1463cbda4888..675fd6c88201 100644 --- a/include/linux/namei.h +++ b/include/linux/namei.h @@ -47,6 +47,12 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT}; /* LOOKUP_* flags which do scope-related checks based on the dirfd. */ #define LOOKUP_IS_SCOPED (LOOKUP_BENEATH | LOOKUP_IN_ROOT) +enum { + D_REVALIDATE_INVALID = 0, + D_REVALIDATE_VALID = 1, + D_REVALIDATE_ATOMIC = 2, /* Not allowed with LOOKUP_RCU */ +}; + extern int path_pts(struct path *path); extern int user_path_at_empty(int, const char __user *, unsigned, struct path *, int *empty);