| Message ID | 20230621144507.55591-2-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs: new accessors for inode->i_ctime | expand |
On Wed 21-06-23 10:45:06, 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@kernel.org> Looks good to me. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > 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 > + * @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. > -- > 2.41.0 >
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@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() ) Tom. > + * @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.
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@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.
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@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@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. >
On Wed, 2023-06-21 at 14:19 -0400, Tom Talpey wrote: > 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@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. > Sure. The difficulty is in trying to do this in an automated way. For instance, look at the hfsplus patch; it has separate assignments in place already: - result->i_ctime.tv_sec = result->i_mtime.tv_sec = result->i_atime.tv_sec = local_to_gmt(dir->i_sb, le32_to_cpu(dee.creation_date)); - result->i_ctime.tv_nsec = 0; + inode_ctime_set_sec(result, + result->i_mtime.tv_sec = result->i_atime.tv_sec = local_to_gmt(dir->i_sb, le32_to_cpu(dee.creation_date))); + inode_ctime_set_nsec(result, 0); Granted the new code is pretty ugly, but it compiles! Transforming that into what you're suggesting is a tougher proposition to do with coccinelle. I didn't see a way to conditionally catch cases like this, declare a new variable in the appropriate spot and then transform two assignments (that may not be next to one another!) into a single one. Maybe it's possible, but my grasp of SMPL is not that great. The docs and examples (including Kees' vey helpful ones!) cover fairly simple changes well, but I didn't quite grasp how to do that complex an evolution. > > 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@talpey.com> > Many thanks! > > > > + * @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. > >
On 6/21/23 23:45, 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@kernel.org> [...] > +/** > + * 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) To be consistent with inode_ctime_set(), why not call this one inode_ctime_get() ? Also, inode_set_ctime() & inode_get_ctime() may be a little more natural. But no strong opinion about that though.
On Thu, 2023-06-22 at 09:46 +0900, Damien Le Moal wrote: > On 6/21/23 23:45, 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@kernel.org> > > [...] > > > +/** > > + * 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) > > To be consistent with inode_ctime_set(), why not call this one inode_ctime_get() In later patches fetching the ctime for presentation may have side effects on certain filesystems. Using "peek" here is a hint that we want to avoid those side effects in these calls. > ? Also, inode_set_ctime() & inode_get_ctime() may be a little more natural. But > no strong opinion about that though. > I like the consistency of the inode_ctime_* prefix. It makes it simpler to find these calls when grepping, etc. That said, my opinions on naming are pretty loosely-held, so if the consensus is that the names should as you suggest, I'll go along with it.
On Wed, Jun 21, 2023 at 10:45:06AM -0400, 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@kernel.org> Reviewed-by: Luis Chamberlain <mcgrof@kernel.org> Luis
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 + * @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.
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@kernel.org> --- fs/inode.c | 16 ++++++++++++++ include/linux/fs.h | 53 +++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 68 insertions(+), 1 deletion(-)