Jeff Layton
2023-Jun-21 18:01 UTC
[Ocfs2-devel] [PATCH 01/79] fs: add ctime accessors infrastructure
On Wed, 2023-06-21 at 13:29 -0400, Tom Talpey wrote:> On 6/21/2023 10:45 AM, Jeff Layton wrote: > > struct timespec64 has unused bits in the tv_nsec field that can be used > > for other purposes. In future patches, we're going to change how the > > inode->i_ctime is accessed in certain inodes in order to make use of > > them. In order to do that safely though, we'll need to eradicate raw > > accesses of the inode->i_ctime field from the kernel. > > > > Add new accessor functions for the ctime that we can use to replace them. > > > > Signed-off-by: Jeff Layton <jlayton at kernel.org> > > --- > > fs/inode.c | 16 ++++++++++++++ > > include/linux/fs.h | 53 +++++++++++++++++++++++++++++++++++++++++++++- > > 2 files changed, 68 insertions(+), 1 deletion(-) > > > > diff --git a/fs/inode.c b/fs/inode.c > > index d37fad91c8da..c005e7328fbb 100644 > > --- a/fs/inode.c > > +++ b/fs/inode.c > > @@ -2499,6 +2499,22 @@ struct timespec64 current_time(struct inode *inode) > > } > > EXPORT_SYMBOL(current_time); > > > > +/** > > + * inode_ctime_set_current - set the ctime to current_time > > + * @inode: inode > > + * > > + * Set the inode->i_ctime to the current value for the inode. Returns > > + * the current value that was assigned to i_ctime. > > + */ > > +struct timespec64 inode_ctime_set_current(struct inode *inode) > > +{ > > + struct timespec64 now = current_time(inode); > > + > > + inode_set_ctime(inode, now); > > + return now; > > +} > > +EXPORT_SYMBOL(inode_ctime_set_current); > > + > > /** > > * in_group_or_capable - check whether caller is CAP_FSETID privileged > > * @idmap: idmap of the mount @inode was found from > > diff --git a/include/linux/fs.h b/include/linux/fs.h > > index 6867512907d6..9afb30606373 100644 > > --- a/include/linux/fs.h > > +++ b/include/linux/fs.h > > @@ -1474,7 +1474,58 @@ static inline bool fsuidgid_has_mapping(struct super_block *sb, > > kgid_has_mapping(fs_userns, kgid); > > } > > > > -extern struct timespec64 current_time(struct inode *inode); > > +struct timespec64 current_time(struct inode *inode); > > +struct timespec64 inode_ctime_set_current(struct inode *inode); > > + > > +/** > > + * inode_ctime_peek - fetch the current ctime from the inode > > + * @inode: inode from which to fetch ctime > > + * > > + * Grab the current ctime from the inode and return it. > > + */ > > +static inline struct timespec64 inode_ctime_peek(const struct inode *inode) > > +{ > > + return inode->i_ctime; > > +} > > + > > +/** > > + * inode_ctime_set - set the ctime in the inode to the given value > > + * @inode: inode in which to set the ctime > > + * @ts: timespec value to set the ctime > > + * > > + * Set the ctime in @inode to @ts. > > + */ > > +static inline struct timespec64 inode_ctime_set(struct inode *inode, struct timespec64 ts) > > +{ > > + inode->i_ctime = ts; > > + return ts; > > +} > > + > > +/** > > + * inode_ctime_set_sec - set only the tv_sec field in the inode ctime > > I'm curious about why you choose to split the tv_sec and tv_nsec > set_ functions. Do any callers not set them both? Wouldn't a > single call enable a more atomic behavior someday? > > inode_ctime_set_sec_nsec(struct inode *, time64_t, time64_t) > > (or simply initialize a timespec64 and use inode_ctime_spec() ) >Yes, quite a few places set the fields individually. For example, when loading a value from disk that doesn't have sufficient granularity to set the nsecs field to anything but 0. Could I have done it by declaring a local timespec64 variable and just use the inode_ctime_set function in these places? Absolutely. That's a bit more difficult to handle with coccinelle though. If someone wants to suggest a way to do that without having to change all of these call sites manually, then I'm open to redoing the set. That might be better left for a later cleanup though.> > + * @inode: inode in which to set the ctime > > + * @sec: value to set the tv_sec field > > + * > > + * Set the sec field in the ctime. Returns @sec. > > + */ > > +static inline time64_t inode_ctime_set_sec(struct inode *inode, time64_t sec) > > +{ > > + inode->i_ctime.tv_sec = sec; > > + return sec; > > +} > > + > > +/** > > + * inode_ctime_set_nsec - set only the tv_nsec field in the inode ctime > > + * @inode: inode in which to set the ctime > > + * @nsec: value to set the tv_nsec field > > + * > > + * Set the nsec field in the ctime. Returns @nsec. > > + */ > > +static inline long inode_ctime_set_nsec(struct inode *inode, long nsec) > > +{ > > + inode->i_ctime.tv_nsec = nsec; > > + return nsec; > > +} > > > > /* > > * Snapshotting support.-- Jeff Layton <jlayton at kernel.org>
Tom Talpey
2023-Jun-21 18:19 UTC
[Ocfs2-devel] [PATCH 01/79] fs: add ctime accessors infrastructure
On 6/21/2023 2:01 PM, Jeff Layton wrote:> On Wed, 2023-06-21 at 13:29 -0400, Tom Talpey wrote: >> On 6/21/2023 10:45 AM, Jeff Layton wrote: >>> struct timespec64 has unused bits in the tv_nsec field that can be used >>> for other purposes. In future patches, we're going to change how the >>> inode->i_ctime is accessed in certain inodes in order to make use of >>> them. In order to do that safely though, we'll need to eradicate raw >>> accesses of the inode->i_ctime field from the kernel. >>> >>> Add new accessor functions for the ctime that we can use to replace them. >>> >>> Signed-off-by: Jeff Layton <jlayton at kernel.org> >>> --- >>> fs/inode.c | 16 ++++++++++++++ >>> include/linux/fs.h | 53 +++++++++++++++++++++++++++++++++++++++++++++- >>> 2 files changed, 68 insertions(+), 1 deletion(-) >>> >>> diff --git a/fs/inode.c b/fs/inode.c >>> index d37fad91c8da..c005e7328fbb 100644 >>> --- a/fs/inode.c >>> +++ b/fs/inode.c >>> @@ -2499,6 +2499,22 @@ struct timespec64 current_time(struct inode *inode) >>> } >>> EXPORT_SYMBOL(current_time); >>> >>> +/** >>> + * inode_ctime_set_current - set the ctime to current_time >>> + * @inode: inode >>> + * >>> + * Set the inode->i_ctime to the current value for the inode. Returns >>> + * the current value that was assigned to i_ctime. >>> + */ >>> +struct timespec64 inode_ctime_set_current(struct inode *inode) >>> +{ >>> + struct timespec64 now = current_time(inode); >>> + >>> + inode_set_ctime(inode, now); >>> + return now; >>> +} >>> +EXPORT_SYMBOL(inode_ctime_set_current); >>> + >>> /** >>> * in_group_or_capable - check whether caller is CAP_FSETID privileged >>> * @idmap: idmap of the mount @inode was found from >>> diff --git a/include/linux/fs.h b/include/linux/fs.h >>> index 6867512907d6..9afb30606373 100644 >>> --- a/include/linux/fs.h >>> +++ b/include/linux/fs.h >>> @@ -1474,7 +1474,58 @@ static inline bool fsuidgid_has_mapping(struct super_block *sb, >>> kgid_has_mapping(fs_userns, kgid); >>> } >>> >>> -extern struct timespec64 current_time(struct inode *inode); >>> +struct timespec64 current_time(struct inode *inode); >>> +struct timespec64 inode_ctime_set_current(struct inode *inode); >>> + >>> +/** >>> + * inode_ctime_peek - fetch the current ctime from the inode >>> + * @inode: inode from which to fetch ctime >>> + * >>> + * Grab the current ctime from the inode and return it. >>> + */ >>> +static inline struct timespec64 inode_ctime_peek(const struct inode *inode) >>> +{ >>> + return inode->i_ctime; >>> +} >>> + >>> +/** >>> + * inode_ctime_set - set the ctime in the inode to the given value >>> + * @inode: inode in which to set the ctime >>> + * @ts: timespec value to set the ctime >>> + * >>> + * Set the ctime in @inode to @ts. >>> + */ >>> +static inline struct timespec64 inode_ctime_set(struct inode *inode, struct timespec64 ts) >>> +{ >>> + inode->i_ctime = ts; >>> + return ts; >>> +} >>> + >>> +/** >>> + * inode_ctime_set_sec - set only the tv_sec field in the inode ctime >> >> I'm curious about why you choose to split the tv_sec and tv_nsec >> set_ functions. Do any callers not set them both? Wouldn't a >> single call enable a more atomic behavior someday? >> >> inode_ctime_set_sec_nsec(struct inode *, time64_t, time64_t) >> >> (or simply initialize a timespec64 and use inode_ctime_spec() ) >> > > Yes, quite a few places set the fields individually. For example, when > loading a value from disk that doesn't have sufficient granularity to > set the nsecs field to anything but 0.Well, they still need to set the tv_nsec so they could just pass 0. But ok.> Could I have done it by declaring a local timespec64 variable and just > use the inode_ctime_set function in these places? Absolutely. > > That's a bit more difficult to handle with coccinelle though. If someone > wants to suggest a way to do that without having to change all of these > call sites manually, then I'm open to redoing the set. > > That might be better left for a later cleanup though.Acked-by: Tom Talpey <tom at talpey.com>>>> + * @inode: inode in which to set the ctime >>> + * @sec: value to set the tv_sec field >>> + * >>> + * Set the sec field in the ctime. Returns @sec. >>> + */ >>> +static inline time64_t inode_ctime_set_sec(struct inode *inode, time64_t sec) >>> +{ >>> + inode->i_ctime.tv_sec = sec; >>> + return sec; >>> +} >>> + >>> +/** >>> + * inode_ctime_set_nsec - set only the tv_nsec field in the inode ctime >>> + * @inode: inode in which to set the ctime >>> + * @nsec: value to set the tv_nsec field >>> + * >>> + * Set the nsec field in the ctime. Returns @nsec. >>> + */ >>> +static inline long inode_ctime_set_nsec(struct inode *inode, long nsec) >>> +{ >>> + inode->i_ctime.tv_nsec = nsec; >>> + return nsec; >>> +} >>> >>> /* >>> * Snapshotting support. >