| Message ID | 162322859985.361452.14110524195807923374.stgit@web.messagingengine.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kernfs: proposed locking and concurrency improvement | expand |
On Wed, 9 Jun 2021 at 10:50, Ian Kent <raven@themaw.net> wrote: > > Add a revision counter to kernfs directory nodes so it can be used > to detect if a directory node has changed during negative dentry > revalidation. > > There's an assumption that sizeof(unsigned long) <= sizeof(pointer) > on all architectures and as far as I know that assumption holds. > > So adding a revision counter to the struct kernfs_elem_dir variant of > the kernfs_node type union won't increase the size of the kernfs_node > struct. This is because struct kernfs_elem_dir is at least > sizeof(pointer) smaller than the largest union variant. It's tempting > to make the revision counter a u64 but that would increase the size of > kernfs_node on archs where sizeof(pointer) is smaller than the revision > counter. > > Signed-off-by: Ian Kent <raven@themaw.net> > --- > fs/kernfs/dir.c | 2 ++ > fs/kernfs/kernfs-internal.h | 23 +++++++++++++++++++++++ > include/linux/kernfs.h | 5 +++++ > 3 files changed, 30 insertions(+) > > diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c > index 33166ec90a112..b3d1bc0f317d0 100644 > --- a/fs/kernfs/dir.c > +++ b/fs/kernfs/dir.c > @@ -372,6 +372,7 @@ static int kernfs_link_sibling(struct kernfs_node *kn) > /* successfully added, account subdir number */ > if (kernfs_type(kn) == KERNFS_DIR) > kn->parent->dir.subdirs++; > + kernfs_inc_rev(kn->parent); > > return 0; > } > @@ -394,6 +395,7 @@ static bool kernfs_unlink_sibling(struct kernfs_node *kn) > > if (kernfs_type(kn) == KERNFS_DIR) > kn->parent->dir.subdirs--; > + kernfs_inc_rev(kn->parent); > > rb_erase(&kn->rb, &kn->parent->dir.children); > RB_CLEAR_NODE(&kn->rb); > diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h > index ccc3b44f6306f..b4e7579e04799 100644 > --- a/fs/kernfs/kernfs-internal.h > +++ b/fs/kernfs/kernfs-internal.h > @@ -81,6 +81,29 @@ static inline struct kernfs_node *kernfs_dentry_node(struct dentry *dentry) > return d_inode(dentry)->i_private; > } > > +static inline void kernfs_set_rev(struct kernfs_node *kn, > + struct dentry *dentry) > +{ > + if (kernfs_type(kn) == KERNFS_DIR) > + dentry->d_time = kn->dir.rev; > +} > + > +static inline void kernfs_inc_rev(struct kernfs_node *kn) > +{ > + if (kernfs_type(kn) == KERNFS_DIR) > + kn->dir.rev++; > +} > + > +static inline bool kernfs_dir_changed(struct kernfs_node *kn, > + struct dentry *dentry) > +{ > + if (kernfs_type(kn) == KERNFS_DIR) { Aren't these always be called on a KERNFS_DIR node? You could just reduce that to a WARN_ON, or remove the conditions altogether then. Thanks, Miklos
On Fri, 2021-06-11 at 14:49 +0200, Miklos Szeredi wrote: > On Wed, 9 Jun 2021 at 10:50, Ian Kent <raven@themaw.net> wrote: > > > > Add a revision counter to kernfs directory nodes so it can be used > > to detect if a directory node has changed during negative dentry > > revalidation. > > > > There's an assumption that sizeof(unsigned long) <= sizeof(pointer) > > on all architectures and as far as I know that assumption holds. > > > > So adding a revision counter to the struct kernfs_elem_dir variant > > of > > the kernfs_node type union won't increase the size of the > > kernfs_node > > struct. This is because struct kernfs_elem_dir is at least > > sizeof(pointer) smaller than the largest union variant. It's > > tempting > > to make the revision counter a u64 but that would increase the size > > of > > kernfs_node on archs where sizeof(pointer) is smaller than the > > revision > > counter. > > > > Signed-off-by: Ian Kent <raven@themaw.net> > > --- > > fs/kernfs/dir.c | 2 ++ > > fs/kernfs/kernfs-internal.h | 23 +++++++++++++++++++++++ > > include/linux/kernfs.h | 5 +++++ > > 3 files changed, 30 insertions(+) > > > > diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c > > index 33166ec90a112..b3d1bc0f317d0 100644 > > --- a/fs/kernfs/dir.c > > +++ b/fs/kernfs/dir.c > > @@ -372,6 +372,7 @@ static int kernfs_link_sibling(struct > > kernfs_node *kn) > > /* successfully added, account subdir number */ > > if (kernfs_type(kn) == KERNFS_DIR) > > kn->parent->dir.subdirs++; > > + kernfs_inc_rev(kn->parent); > > > > return 0; > > } > > @@ -394,6 +395,7 @@ static bool kernfs_unlink_sibling(struct > > kernfs_node *kn) > > > > if (kernfs_type(kn) == KERNFS_DIR) > > kn->parent->dir.subdirs--; > > + kernfs_inc_rev(kn->parent); > > > > rb_erase(&kn->rb, &kn->parent->dir.children); > > RB_CLEAR_NODE(&kn->rb); > > diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs- > > internal.h > > index ccc3b44f6306f..b4e7579e04799 100644 > > --- a/fs/kernfs/kernfs-internal.h > > +++ b/fs/kernfs/kernfs-internal.h > > @@ -81,6 +81,29 @@ static inline struct kernfs_node > > *kernfs_dentry_node(struct dentry *dentry) > > return d_inode(dentry)->i_private; > > } > > > > +static inline void kernfs_set_rev(struct kernfs_node *kn, > > + struct dentry *dentry) > > +{ > > + if (kernfs_type(kn) == KERNFS_DIR) > > + dentry->d_time = kn->dir.rev; > > +} > > + > > +static inline void kernfs_inc_rev(struct kernfs_node *kn) > > +{ > > + if (kernfs_type(kn) == KERNFS_DIR) > > + kn->dir.rev++; > > +} > > + > > +static inline bool kernfs_dir_changed(struct kernfs_node *kn, > > + struct dentry *dentry) > > +{ > > + if (kernfs_type(kn) == KERNFS_DIR) { > > Aren't these always be called on a KERNFS_DIR node? Yes they are. > > You could just reduce that to a WARN_ON, or remove the conditions > altogether then. I was tempted to not use the check, a WARN_ON sounds better than removing the check, I'll do that in a v7. Thanks Ian
On Fri, Jun 11, 2021 at 08:56:18PM +0800, Ian Kent wrote: > On Fri, 2021-06-11 at 14:49 +0200, Miklos Szeredi wrote: > > On Wed, 9 Jun 2021 at 10:50, Ian Kent <raven@themaw.net> wrote: > > > > > > Add a revision counter to kernfs directory nodes so it can be used > > > to detect if a directory node has changed during negative dentry > > > revalidation. > > > > > > There's an assumption that sizeof(unsigned long) <= sizeof(pointer) > > > on all architectures and as far as I know that assumption holds. > > > > > > So adding a revision counter to the struct kernfs_elem_dir variant > > > of > > > the kernfs_node type union won't increase the size of the > > > kernfs_node > > > struct. This is because struct kernfs_elem_dir is at least > > > sizeof(pointer) smaller than the largest union variant. It's > > > tempting > > > to make the revision counter a u64 but that would increase the size > > > of > > > kernfs_node on archs where sizeof(pointer) is smaller than the > > > revision > > > counter. > > > > > > Signed-off-by: Ian Kent <raven@themaw.net> > > > --- > > > fs/kernfs/dir.c | 2 ++ > > > fs/kernfs/kernfs-internal.h | 23 +++++++++++++++++++++++ > > > include/linux/kernfs.h | 5 +++++ > > > 3 files changed, 30 insertions(+) > > > > > > diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c > > > index 33166ec90a112..b3d1bc0f317d0 100644 > > > --- a/fs/kernfs/dir.c > > > +++ b/fs/kernfs/dir.c > > > @@ -372,6 +372,7 @@ static int kernfs_link_sibling(struct > > > kernfs_node *kn) > > > /* successfully added, account subdir number */ > > > if (kernfs_type(kn) == KERNFS_DIR) > > > kn->parent->dir.subdirs++; > > > + kernfs_inc_rev(kn->parent); > > > > > > return 0; > > > } > > > @@ -394,6 +395,7 @@ static bool kernfs_unlink_sibling(struct > > > kernfs_node *kn) > > > > > > if (kernfs_type(kn) == KERNFS_DIR) > > > kn->parent->dir.subdirs--; > > > + kernfs_inc_rev(kn->parent); > > > > > > rb_erase(&kn->rb, &kn->parent->dir.children); > > > RB_CLEAR_NODE(&kn->rb); > > > diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs- > > > internal.h > > > index ccc3b44f6306f..b4e7579e04799 100644 > > > --- a/fs/kernfs/kernfs-internal.h > > > +++ b/fs/kernfs/kernfs-internal.h > > > @@ -81,6 +81,29 @@ static inline struct kernfs_node > > > *kernfs_dentry_node(struct dentry *dentry) > > > return d_inode(dentry)->i_private; > > > } > > > > > > +static inline void kernfs_set_rev(struct kernfs_node *kn, > > > + struct dentry *dentry) > > > +{ > > > + if (kernfs_type(kn) == KERNFS_DIR) > > > + dentry->d_time = kn->dir.rev; > > > +} > > > + > > > +static inline void kernfs_inc_rev(struct kernfs_node *kn) > > > +{ > > > + if (kernfs_type(kn) == KERNFS_DIR) > > > + kn->dir.rev++; > > > +} > > > + > > > +static inline bool kernfs_dir_changed(struct kernfs_node *kn, > > > + struct dentry *dentry) > > > +{ > > > + if (kernfs_type(kn) == KERNFS_DIR) { > > > > Aren't these always be called on a KERNFS_DIR node? > > Yes they are. > > > > > You could just reduce that to a WARN_ON, or remove the conditions > > altogether then. > > I was tempted to not use the check, a WARN_ON sounds better than > removing the check, I'll do that in a v7. No, WARN_ON is not ok, as systems will crash if panic-on-warn is set. If these are impossible to hit, great, let's not check this and we can just drop the code. If they can be hit, then the above code is correct and it should stay. thanks, greg k-h
On Fri, 2021-06-11 at 15:11 +0200, Greg Kroah-Hartman wrote: > On Fri, Jun 11, 2021 at 08:56:18PM +0800, Ian Kent wrote: > > On Fri, 2021-06-11 at 14:49 +0200, Miklos Szeredi wrote: > > > On Wed, 9 Jun 2021 at 10:50, Ian Kent <raven@themaw.net> wrote: > > > > > > > > Add a revision counter to kernfs directory nodes so it can be > > > > used > > > > to detect if a directory node has changed during negative > > > > dentry > > > > revalidation. > > > > > > > > There's an assumption that sizeof(unsigned long) <= > > > > sizeof(pointer) > > > > on all architectures and as far as I know that assumption > > > > holds. > > > > > > > > So adding a revision counter to the struct kernfs_elem_dir > > > > variant > > > > of > > > > the kernfs_node type union won't increase the size of the > > > > kernfs_node > > > > struct. This is because struct kernfs_elem_dir is at least > > > > sizeof(pointer) smaller than the largest union variant. It's > > > > tempting > > > > to make the revision counter a u64 but that would increase the > > > > size > > > > of > > > > kernfs_node on archs where sizeof(pointer) is smaller than the > > > > revision > > > > counter. > > > > > > > > Signed-off-by: Ian Kent <raven@themaw.net> > > > > --- > > > > fs/kernfs/dir.c | 2 ++ > > > > fs/kernfs/kernfs-internal.h | 23 +++++++++++++++++++++++ > > > > include/linux/kernfs.h | 5 +++++ > > > > 3 files changed, 30 insertions(+) > > > > > > > > diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c > > > > index 33166ec90a112..b3d1bc0f317d0 100644 > > > > --- a/fs/kernfs/dir.c > > > > +++ b/fs/kernfs/dir.c > > > > @@ -372,6 +372,7 @@ static int kernfs_link_sibling(struct > > > > kernfs_node *kn) > > > > /* successfully added, account subdir number */ > > > > if (kernfs_type(kn) == KERNFS_DIR) > > > > kn->parent->dir.subdirs++; > > > > + kernfs_inc_rev(kn->parent); > > > > > > > > return 0; > > > > } > > > > @@ -394,6 +395,7 @@ static bool kernfs_unlink_sibling(struct > > > > kernfs_node *kn) > > > > > > > > if (kernfs_type(kn) == KERNFS_DIR) > > > > kn->parent->dir.subdirs--; > > > > + kernfs_inc_rev(kn->parent); > > > > > > > > rb_erase(&kn->rb, &kn->parent->dir.children); > > > > RB_CLEAR_NODE(&kn->rb); > > > > diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs- > > > > internal.h > > > > index ccc3b44f6306f..b4e7579e04799 100644 > > > > --- a/fs/kernfs/kernfs-internal.h > > > > +++ b/fs/kernfs/kernfs-internal.h > > > > @@ -81,6 +81,29 @@ static inline struct kernfs_node > > > > *kernfs_dentry_node(struct dentry *dentry) > > > > return d_inode(dentry)->i_private; > > > > } > > > > > > > > +static inline void kernfs_set_rev(struct kernfs_node *kn, > > > > + struct dentry *dentry) > > > > +{ > > > > + if (kernfs_type(kn) == KERNFS_DIR) > > > > + dentry->d_time = kn->dir.rev; > > > > +} > > > > + > > > > +static inline void kernfs_inc_rev(struct kernfs_node *kn) > > > > +{ > > > > + if (kernfs_type(kn) == KERNFS_DIR) > > > > + kn->dir.rev++; > > > > +} > > > > + > > > > +static inline bool kernfs_dir_changed(struct kernfs_node *kn, > > > > + struct dentry *dentry) > > > > +{ > > > > + if (kernfs_type(kn) == KERNFS_DIR) { > > > > > > Aren't these always be called on a KERNFS_DIR node? > > > > Yes they are. > > > > > > > > You could just reduce that to a WARN_ON, or remove the conditions > > > altogether then. > > > > I was tempted to not use the check, a WARN_ON sounds better than > > removing the check, I'll do that in a v7. > > No, WARN_ON is not ok, as systems will crash if panic-on-warn is set. Thanks Greg, understood. > > If these are impossible to hit, great, let's not check this and we > can > just drop the code. If they can be hit, then the above code is > correct > and it should stay. It's a programming mistake to call these on a non-directory node. I can remove the check but do you think there's any value in passing the node and updating it's parent to avoid possible misuse? Ian
On Fri, Jun 11, 2021 at 09:31:36PM +0800, Ian Kent wrote: > On Fri, 2021-06-11 at 15:11 +0200, Greg Kroah-Hartman wrote: > > On Fri, Jun 11, 2021 at 08:56:18PM +0800, Ian Kent wrote: > > > On Fri, 2021-06-11 at 14:49 +0200, Miklos Szeredi wrote: > > > > On Wed, 9 Jun 2021 at 10:50, Ian Kent <raven@themaw.net> wrote: > > > > > > > > > > Add a revision counter to kernfs directory nodes so it can be > > > > > used > > > > > to detect if a directory node has changed during negative > > > > > dentry > > > > > revalidation. > > > > > > > > > > There's an assumption that sizeof(unsigned long) <= > > > > > sizeof(pointer) > > > > > on all architectures and as far as I know that assumption > > > > > holds. > > > > > > > > > > So adding a revision counter to the struct kernfs_elem_dir > > > > > variant > > > > > of > > > > > the kernfs_node type union won't increase the size of the > > > > > kernfs_node > > > > > struct. This is because struct kernfs_elem_dir is at least > > > > > sizeof(pointer) smaller than the largest union variant. It's > > > > > tempting > > > > > to make the revision counter a u64 but that would increase the > > > > > size > > > > > of > > > > > kernfs_node on archs where sizeof(pointer) is smaller than the > > > > > revision > > > > > counter. > > > > > > > > > > Signed-off-by: Ian Kent <raven@themaw.net> > > > > > --- > > > > > fs/kernfs/dir.c | 2 ++ > > > > > fs/kernfs/kernfs-internal.h | 23 +++++++++++++++++++++++ > > > > > include/linux/kernfs.h | 5 +++++ > > > > > 3 files changed, 30 insertions(+) > > > > > > > > > > diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c > > > > > index 33166ec90a112..b3d1bc0f317d0 100644 > > > > > --- a/fs/kernfs/dir.c > > > > > +++ b/fs/kernfs/dir.c > > > > > @@ -372,6 +372,7 @@ static int kernfs_link_sibling(struct > > > > > kernfs_node *kn) > > > > > /* successfully added, account subdir number */ > > > > > if (kernfs_type(kn) == KERNFS_DIR) > > > > > kn->parent->dir.subdirs++; > > > > > + kernfs_inc_rev(kn->parent); > > > > > > > > > > return 0; > > > > > } > > > > > @@ -394,6 +395,7 @@ static bool kernfs_unlink_sibling(struct > > > > > kernfs_node *kn) > > > > > > > > > > if (kernfs_type(kn) == KERNFS_DIR) > > > > > kn->parent->dir.subdirs--; > > > > > + kernfs_inc_rev(kn->parent); > > > > > > > > > > rb_erase(&kn->rb, &kn->parent->dir.children); > > > > > RB_CLEAR_NODE(&kn->rb); > > > > > diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs- > > > > > internal.h > > > > > index ccc3b44f6306f..b4e7579e04799 100644 > > > > > --- a/fs/kernfs/kernfs-internal.h > > > > > +++ b/fs/kernfs/kernfs-internal.h > > > > > @@ -81,6 +81,29 @@ static inline struct kernfs_node > > > > > *kernfs_dentry_node(struct dentry *dentry) > > > > > return d_inode(dentry)->i_private; > > > > > } > > > > > > > > > > +static inline void kernfs_set_rev(struct kernfs_node *kn, > > > > > + struct dentry *dentry) > > > > > +{ > > > > > + if (kernfs_type(kn) == KERNFS_DIR) > > > > > + dentry->d_time = kn->dir.rev; > > > > > +} > > > > > + > > > > > +static inline void kernfs_inc_rev(struct kernfs_node *kn) > > > > > +{ > > > > > + if (kernfs_type(kn) == KERNFS_DIR) > > > > > + kn->dir.rev++; > > > > > +} > > > > > + > > > > > +static inline bool kernfs_dir_changed(struct kernfs_node *kn, > > > > > + struct dentry *dentry) > > > > > +{ > > > > > + if (kernfs_type(kn) == KERNFS_DIR) { > > > > > > > > Aren't these always be called on a KERNFS_DIR node? > > > > > > Yes they are. > > > > > > > > > > > You could just reduce that to a WARN_ON, or remove the conditions > > > > altogether then. > > > > > > I was tempted to not use the check, a WARN_ON sounds better than > > > removing the check, I'll do that in a v7. > > > > No, WARN_ON is not ok, as systems will crash if panic-on-warn is set. > > Thanks Greg, understood. > > > > > If these are impossible to hit, great, let's not check this and we > > can > > just drop the code. If they can be hit, then the above code is > > correct > > and it should stay. > > It's a programming mistake to call these on a non-directory node. > > I can remove the check but do you think there's any value in passing > the node and updating it's parent to avoid possible misuse? I do not understand the question here, sorry. It's a static function, you control the callers, who can "misuse" it? thanks, greg k-h
On Fri, 2021-06-11 at 16:05 +0200, Greg Kroah-Hartman wrote: > On Fri, Jun 11, 2021 at 09:31:36PM +0800, Ian Kent wrote: > > On Fri, 2021-06-11 at 15:11 +0200, Greg Kroah-Hartman wrote: > > > On Fri, Jun 11, 2021 at 08:56:18PM +0800, Ian Kent wrote: > > > > On Fri, 2021-06-11 at 14:49 +0200, Miklos Szeredi wrote: > > > > > On Wed, 9 Jun 2021 at 10:50, Ian Kent <raven@themaw.net> > > > > > wrote: > > > > > > > > > > > > Add a revision counter to kernfs directory nodes so it can > > > > > > be > > > > > > used > > > > > > to detect if a directory node has changed during negative > > > > > > dentry > > > > > > revalidation. > > > > > > > > > > > > There's an assumption that sizeof(unsigned long) <= > > > > > > sizeof(pointer) > > > > > > on all architectures and as far as I know that assumption > > > > > > holds. > > > > > > > > > > > > So adding a revision counter to the struct kernfs_elem_dir > > > > > > variant > > > > > > of > > > > > > the kernfs_node type union won't increase the size of the > > > > > > kernfs_node > > > > > > struct. This is because struct kernfs_elem_dir is at least > > > > > > sizeof(pointer) smaller than the largest union variant. > > > > > > It's > > > > > > tempting > > > > > > to make the revision counter a u64 but that would increase > > > > > > the > > > > > > size > > > > > > of > > > > > > kernfs_node on archs where sizeof(pointer) is smaller than > > > > > > the > > > > > > revision > > > > > > counter. > > > > > > > > > > > > Signed-off-by: Ian Kent <raven@themaw.net> > > > > > > --- > > > > > > fs/kernfs/dir.c | 2 ++ > > > > > > fs/kernfs/kernfs-internal.h | 23 +++++++++++++++++++++++ > > > > > > include/linux/kernfs.h | 5 +++++ > > > > > > 3 files changed, 30 insertions(+) > > > > > > > > > > > > diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c > > > > > > index 33166ec90a112..b3d1bc0f317d0 100644 > > > > > > --- a/fs/kernfs/dir.c > > > > > > +++ b/fs/kernfs/dir.c > > > > > > @@ -372,6 +372,7 @@ static int kernfs_link_sibling(struct > > > > > > kernfs_node *kn) > > > > > > /* successfully added, account subdir number */ > > > > > > if (kernfs_type(kn) == KERNFS_DIR) > > > > > > kn->parent->dir.subdirs++; > > > > > > + kernfs_inc_rev(kn->parent); > > > > > > > > > > > > return 0; > > > > > > } > > > > > > @@ -394,6 +395,7 @@ static bool > > > > > > kernfs_unlink_sibling(struct > > > > > > kernfs_node *kn) > > > > > > > > > > > > if (kernfs_type(kn) == KERNFS_DIR) > > > > > > kn->parent->dir.subdirs--; > > > > > > + kernfs_inc_rev(kn->parent); > > > > > > > > > > > > rb_erase(&kn->rb, &kn->parent->dir.children); > > > > > > RB_CLEAR_NODE(&kn->rb); > > > > > > diff --git a/fs/kernfs/kernfs-internal.h > > > > > > b/fs/kernfs/kernfs- > > > > > > internal.h > > > > > > index ccc3b44f6306f..b4e7579e04799 100644 > > > > > > --- a/fs/kernfs/kernfs-internal.h > > > > > > +++ b/fs/kernfs/kernfs-internal.h > > > > > > @@ -81,6 +81,29 @@ static inline struct kernfs_node > > > > > > *kernfs_dentry_node(struct dentry *dentry) > > > > > > return d_inode(dentry)->i_private; > > > > > > } > > > > > > > > > > > > +static inline void kernfs_set_rev(struct kernfs_node *kn, > > > > > > + struct dentry *dentry) > > > > > > +{ > > > > > > + if (kernfs_type(kn) == KERNFS_DIR) > > > > > > + dentry->d_time = kn->dir.rev; > > > > > > +} > > > > > > + > > > > > > +static inline void kernfs_inc_rev(struct kernfs_node *kn) > > > > > > +{ > > > > > > + if (kernfs_type(kn) == KERNFS_DIR) > > > > > > + kn->dir.rev++; > > > > > > +} > > > > > > + > > > > > > +static inline bool kernfs_dir_changed(struct kernfs_node > > > > > > *kn, > > > > > > + struct dentry > > > > > > *dentry) > > > > > > +{ > > > > > > + if (kernfs_type(kn) == KERNFS_DIR) { > > > > > > > > > > Aren't these always be called on a KERNFS_DIR node? > > > > > > > > Yes they are. > > > > > > > > > > > > > > You could just reduce that to a WARN_ON, or remove the > > > > > conditions > > > > > altogether then. > > > > > > > > I was tempted to not use the check, a WARN_ON sounds better > > > > than > > > > removing the check, I'll do that in a v7. > > > > > > No, WARN_ON is not ok, as systems will crash if panic-on-warn is > > > set. > > > > Thanks Greg, understood. > > > > > > > > If these are impossible to hit, great, let's not check this and > > > we > > > can > > > just drop the code. If they can be hit, then the above code is > > > correct > > > and it should stay. > > > > It's a programming mistake to call these on a non-directory node. > > > > I can remove the check but do you think there's any value in > > passing > > the node and updating it's parent to avoid possible misuse? > > I do not understand the question here, sorry. It's a static > function, > you control the callers, who can "misuse" it? Yes, I'll drop the test and name the argument parent to make it clear for readers. Ian
diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c index 33166ec90a112..b3d1bc0f317d0 100644 --- a/fs/kernfs/dir.c +++ b/fs/kernfs/dir.c @@ -372,6 +372,7 @@ static int kernfs_link_sibling(struct kernfs_node *kn) /* successfully added, account subdir number */ if (kernfs_type(kn) == KERNFS_DIR) kn->parent->dir.subdirs++; + kernfs_inc_rev(kn->parent); return 0; } @@ -394,6 +395,7 @@ static bool kernfs_unlink_sibling(struct kernfs_node *kn) if (kernfs_type(kn) == KERNFS_DIR) kn->parent->dir.subdirs--; + kernfs_inc_rev(kn->parent); rb_erase(&kn->rb, &kn->parent->dir.children); RB_CLEAR_NODE(&kn->rb); diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h index ccc3b44f6306f..b4e7579e04799 100644 --- a/fs/kernfs/kernfs-internal.h +++ b/fs/kernfs/kernfs-internal.h @@ -81,6 +81,29 @@ static inline struct kernfs_node *kernfs_dentry_node(struct dentry *dentry) return d_inode(dentry)->i_private; } +static inline void kernfs_set_rev(struct kernfs_node *kn, + struct dentry *dentry) +{ + if (kernfs_type(kn) == KERNFS_DIR) + dentry->d_time = kn->dir.rev; +} + +static inline void kernfs_inc_rev(struct kernfs_node *kn) +{ + if (kernfs_type(kn) == KERNFS_DIR) + kn->dir.rev++; +} + +static inline bool kernfs_dir_changed(struct kernfs_node *kn, + struct dentry *dentry) +{ + if (kernfs_type(kn) == KERNFS_DIR) { + if (kn->dir.rev != dentry->d_time) + return true; + } + return false; +} + extern const struct super_operations kernfs_sops; extern struct kmem_cache *kernfs_node_cache, *kernfs_iattrs_cache; diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h index 9e8ca8743c268..d7e0160fce6df 100644 --- a/include/linux/kernfs.h +++ b/include/linux/kernfs.h @@ -98,6 +98,11 @@ struct kernfs_elem_dir { * better directly in kernfs_node but is here to save space. */ struct kernfs_root *root; + /* + * Monotonic revision counter, used to identify if a directory + * node has changed during negative dentry revalidation. + */ + unsigned long rev; }; struct kernfs_elem_symlink {
Add a revision counter to kernfs directory nodes so it can be used to detect if a directory node has changed during negative dentry revalidation. There's an assumption that sizeof(unsigned long) <= sizeof(pointer) on all architectures and as far as I know that assumption holds. So adding a revision counter to the struct kernfs_elem_dir variant of the kernfs_node type union won't increase the size of the kernfs_node struct. This is because struct kernfs_elem_dir is at least sizeof(pointer) smaller than the largest union variant. It's tempting to make the revision counter a u64 but that would increase the size of kernfs_node on archs where sizeof(pointer) is smaller than the revision counter. Signed-off-by: Ian Kent <raven@themaw.net> --- fs/kernfs/dir.c | 2 ++ fs/kernfs/kernfs-internal.h | 23 +++++++++++++++++++++++ include/linux/kernfs.h | 5 +++++ 3 files changed, 30 insertions(+)