| Message ID | 162306071065.69474.8064509709844383785.stgit@web.messagingengine.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kernfs: proposed locking and concurrency improvement | expand |
Ian Kent <raven@themaw.net> writes: > Add a revision counter to kernfs directory nodes so it can be used > to detect if a directory node has changed. > > 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 | 8 ++++++++ > fs/kernfs/kernfs-internal.h | 24 ++++++++++++++++++++++++ > include/linux/kernfs.h | 5 +++++ > 3 files changed, 37 insertions(+) > > diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c > index 33166ec90a112..b88432c48851f 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); > @@ -1105,6 +1107,12 @@ static struct dentry *kernfs_iop_lookup(struct inode *dir, > > /* instantiate and hash dentry */ > ret = d_splice_alias(inode, dentry); > + if (!IS_ERR(ret)) { > + if (unlikely(ret)) > + kernfs_set_rev(parent, ret); > + else > + kernfs_set_rev(parent, dentry); Do we care about d_time on non-NULL dentries? For d_splice_alias to return a different dentry implies that the dentry was non-NULL. I am wondering if having a guarantee that d_time never changes could help simplify the implementation. For never changing it would see to make sense to call kernfs_set_rev before d_splice_alias on dentry, and simply not worry about it after d_splice_alias. > + } > out_unlock: > mutex_unlock(&kernfs_mutex); > return ret; > diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h > index ccc3b44f6306f..1536002584fc4 100644 > --- a/fs/kernfs/kernfs-internal.h > +++ b/fs/kernfs/kernfs-internal.h > @@ -81,6 +81,30 @@ 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) { > + /* Not really a time bit it does what's needed */ > + if (time_after(kn->dir.rev, dentry->d_time)) > + return true; Why not simply make this: if (kn->dir.rev != dentry->d_time) return true; I don't see what is gained by not counting as changed something in the wrong half of the values. > + } > + 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..7947acb1163d7 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 revalidation. > + */ > + unsigned long rev; > }; > > struct kernfs_elem_symlink { Eric
On Mon, 2021-06-07 at 12:53 -0500, Eric W. Biederman wrote: > Ian Kent <raven@themaw.net> writes: > > > Add a revision counter to kernfs directory nodes so it can be used > > to detect if a directory node has changed. > > > > 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 | 8 ++++++++ > > fs/kernfs/kernfs-internal.h | 24 ++++++++++++++++++++++++ > > include/linux/kernfs.h | 5 +++++ > > 3 files changed, 37 insertions(+) > > > > diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c > > index 33166ec90a112..b88432c48851f 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); > > @@ -1105,6 +1107,12 @@ static struct dentry > > *kernfs_iop_lookup(struct inode *dir, > > > > /* instantiate and hash dentry */ > > ret = d_splice_alias(inode, dentry); > > + if (!IS_ERR(ret)) { > > + if (unlikely(ret)) > > + kernfs_set_rev(parent, ret); > > + else > > + kernfs_set_rev(parent, dentry); > > Do we care about d_time on non-NULL dentries? Would we ever need to use it avoid a search for any other cases? Probably not ... those export ops mean that some dentries might not have d_time set. Maybe it's best to put a comment in about only using it for negative dentries and set it unconditionally in ->lookup() as you describe. > > For d_splice_alias to return a different dentry implies > that the dentry was non-NULL. > > I am wondering if having a guarantee that d_time never changes could > help simplify the implementation. For never changing it would see to > make sense to call kernfs_set_rev before d_splice_alias on dentry, > and > simply not worry about it after d_splice_alias. Yes, I was tempted to do that. > > > + } > > out_unlock: > > mutex_unlock(&kernfs_mutex); > > return ret; > > diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs- > > internal.h > > index ccc3b44f6306f..1536002584fc4 100644 > > --- a/fs/kernfs/kernfs-internal.h > > +++ b/fs/kernfs/kernfs-internal.h > > @@ -81,6 +81,30 @@ 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) { > > + /* Not really a time bit it does what's needed */ > > + if (time_after(kn->dir.rev, dentry->d_time)) > > + return true; > > Why not simply make this: > if (kn->dir.rev != dentry->d_time) > return true; > > I don't see what is gained by not counting as changed something in > the > wrong half of the values. Yes, it was like that originally and really shouldn't make any difference. I'll change it back. Ian > > > + } > > + 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..7947acb1163d7 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 revalidation. > > + */ > > + unsigned long rev; > > }; > > > > struct kernfs_elem_symlink { > > Eric
diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c index 33166ec90a112..b88432c48851f 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); @@ -1105,6 +1107,12 @@ static struct dentry *kernfs_iop_lookup(struct inode *dir, /* instantiate and hash dentry */ ret = d_splice_alias(inode, dentry); + if (!IS_ERR(ret)) { + if (unlikely(ret)) + kernfs_set_rev(parent, ret); + else + kernfs_set_rev(parent, dentry); + } out_unlock: mutex_unlock(&kernfs_mutex); return ret; diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h index ccc3b44f6306f..1536002584fc4 100644 --- a/fs/kernfs/kernfs-internal.h +++ b/fs/kernfs/kernfs-internal.h @@ -81,6 +81,30 @@ 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) { + /* Not really a time bit it does what's needed */ + if (time_after(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..7947acb1163d7 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 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. 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 | 8 ++++++++ fs/kernfs/kernfs-internal.h | 24 ++++++++++++++++++++++++ include/linux/kernfs.h | 5 +++++ 3 files changed, 37 insertions(+)