In later patches, we're going to change how the ctime.tv_nsec field is
utilized. Switch to using accessor functions instead of raw accesses of
inode->i_ctime.
Signed-off-by: Jeff Layton <[email protected]>
---
fs/bfs/dir.c | 16 ++++++++--------
fs/bfs/inode.c | 6 +++---
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/fs/bfs/dir.c b/fs/bfs/dir.c
index d2e8a2a56b05..5bcfc6e481bc 100644
--- a/fs/bfs/dir.c
+++ b/fs/bfs/dir.c
@@ -97,7 +97,7 @@ static int bfs_create(struct mnt_idmap *idmap, struct inode *dir,
set_bit(ino, info->si_imap);
info->si_freei--;
inode_init_owner(&nop_mnt_idmap, inode, dir, mode);
- inode->i_mtime = inode->i_atime = inode->i_ctime = current_time(inode);
+ inode->i_mtime = inode->i_atime = inode_ctime_set_current(inode);
inode->i_blocks = 0;
inode->i_op = &bfs_file_inops;
inode->i_fop = &bfs_file_operations;
@@ -158,7 +158,7 @@ static int bfs_link(struct dentry *old, struct inode *dir,
return err;
}
inc_nlink(inode);
- inode->i_ctime = current_time(inode);
+ inode_ctime_set_current(inode);
mark_inode_dirty(inode);
ihold(inode);
d_instantiate(new, inode);
@@ -187,9 +187,9 @@ static int bfs_unlink(struct inode *dir, struct dentry *dentry)
}
de->ino = 0;
mark_buffer_dirty_inode(bh, dir);
- dir->i_ctime = dir->i_mtime = current_time(dir);
+ dir->i_mtime = inode_ctime_set_current(dir);
mark_inode_dirty(dir);
- inode->i_ctime = dir->i_ctime;
+ inode_ctime_set(inode, inode_ctime_peek(dir));
inode_dec_link_count(inode);
error = 0;
@@ -240,10 +240,10 @@ static int bfs_rename(struct mnt_idmap *idmap, struct inode *old_dir,
goto end_rename;
}
old_de->ino = 0;
- old_dir->i_ctime = old_dir->i_mtime = current_time(old_dir);
+ old_dir->i_mtime = inode_ctime_set_current(old_dir);
mark_inode_dirty(old_dir);
if (new_inode) {
- new_inode->i_ctime = current_time(new_inode);
+ inode_ctime_set_current(new_inode);
inode_dec_link_count(new_inode);
}
mark_buffer_dirty_inode(old_bh, old_dir);
@@ -292,9 +292,9 @@ static int bfs_add_entry(struct inode *dir, const struct qstr *child, int ino)
pos = (block - sblock) * BFS_BSIZE + off;
if (pos >= dir->i_size) {
dir->i_size += BFS_DIRENT_SIZE;
- dir->i_ctime = current_time(dir);
+ inode_ctime_set_current(dir);
}
- dir->i_mtime = dir->i_ctime = current_time(dir);
+ dir->i_mtime = inode_ctime_set_current(dir);
mark_inode_dirty(dir);
de->ino = cpu_to_le16((u16)ino);
for (i = 0; i < BFS_NAMELEN; i++)
diff --git a/fs/bfs/inode.c b/fs/bfs/inode.c
index 1926bec2c850..c964316be32b 100644
--- a/fs/bfs/inode.c
+++ b/fs/bfs/inode.c
@@ -82,10 +82,10 @@ struct inode *bfs_iget(struct super_block *sb, unsigned long ino)
inode->i_blocks = BFS_FILEBLOCKS(di);
inode->i_atime.tv_sec = le32_to_cpu(di->i_atime);
inode->i_mtime.tv_sec = le32_to_cpu(di->i_mtime);
- inode->i_ctime.tv_sec = le32_to_cpu(di->i_ctime);
+ inode_ctime_set_sec(inode, le32_to_cpu(di->i_ctime));
inode->i_atime.tv_nsec = 0;
inode->i_mtime.tv_nsec = 0;
- inode->i_ctime.tv_nsec = 0;
+ inode_ctime_set_nsec(inode, 0);
brelse(bh);
unlock_new_inode(inode);
@@ -143,7 +143,7 @@ static int bfs_write_inode(struct inode *inode, struct writeback_control *wbc)
di->i_nlink = cpu_to_le32(inode->i_nlink);
di->i_atime = cpu_to_le32(inode->i_atime.tv_sec);
di->i_mtime = cpu_to_le32(inode->i_mtime.tv_sec);
- di->i_ctime = cpu_to_le32(inode->i_ctime.tv_sec);
+ di->i_ctime = cpu_to_le32(inode_ctime_peek(inode).tv_sec);
i_sblock = BFS_I(inode)->i_sblock;
di->i_sblock = cpu_to_le32(i_sblock);
di->i_eblock = cpu_to_le32(BFS_I(inode)->i_eblock);
--
2.41.0
On Wed 21-06-23 10:45:28, Jeff Layton wrote:
> In later patches, we're going to change how the ctime.tv_nsec field is
> utilized. Switch to using accessor functions instead of raw accesses of
> inode->i_ctime.
>
> Signed-off-by: Jeff Layton <[email protected]>
...
> diff --git a/fs/bfs/inode.c b/fs/bfs/inode.c
> index 1926bec2c850..c964316be32b 100644
> --- a/fs/bfs/inode.c
> +++ b/fs/bfs/inode.c
> @@ -82,10 +82,10 @@ struct inode *bfs_iget(struct super_block *sb, unsigned long ino)
> inode->i_blocks = BFS_FILEBLOCKS(di);
> inode->i_atime.tv_sec = le32_to_cpu(di->i_atime);
> inode->i_mtime.tv_sec = le32_to_cpu(di->i_mtime);
> - inode->i_ctime.tv_sec = le32_to_cpu(di->i_ctime);
> + inode_ctime_set_sec(inode, le32_to_cpu(di->i_ctime));
> inode->i_atime.tv_nsec = 0;
> inode->i_mtime.tv_nsec = 0;
> - inode->i_ctime.tv_nsec = 0;
> + inode_ctime_set_nsec(inode, 0);
So I'm somewhat wondering here - in other filesystem you construct
timespec64 and then use inode_ctime_set(). Here you use
inode_ctime_set_sec() + inode_ctime_set_nsec(). What's the benefit? It
seems these two functions are not used that much some maybe we could just
live with just inode_ctime_set() and constructing timespec64 when needed?
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Wed, 2023-06-21 at 18:48 +0200, Jan Kara wrote:
> On Wed 21-06-23 10:45:28, Jeff Layton wrote:
> > In later patches, we're going to change how the ctime.tv_nsec field is
> > utilized. Switch to using accessor functions instead of raw accesses of
> > inode->i_ctime.
> >
> > Signed-off-by: Jeff Layton <[email protected]>
>
> ...
>
> > diff --git a/fs/bfs/inode.c b/fs/bfs/inode.c
> > index 1926bec2c850..c964316be32b 100644
> > --- a/fs/bfs/inode.c
> > +++ b/fs/bfs/inode.c
> > @@ -82,10 +82,10 @@ struct inode *bfs_iget(struct super_block *sb, unsigned long ino)
> > inode->i_blocks = BFS_FILEBLOCKS(di);
> > inode->i_atime.tv_sec = le32_to_cpu(di->i_atime);
> > inode->i_mtime.tv_sec = le32_to_cpu(di->i_mtime);
> > - inode->i_ctime.tv_sec = le32_to_cpu(di->i_ctime);
> > + inode_ctime_set_sec(inode, le32_to_cpu(di->i_ctime));
> > inode->i_atime.tv_nsec = 0;
> > inode->i_mtime.tv_nsec = 0;
> > - inode->i_ctime.tv_nsec = 0;
> > + inode_ctime_set_nsec(inode, 0);
>
> So I'm somewhat wondering here - in other filesystem you construct
> timespec64 and then use inode_ctime_set(). Here you use
> inode_ctime_set_sec() + inode_ctime_set_nsec(). What's the benefit? It
> seems these two functions are not used that much some maybe we could just
> live with just inode_ctime_set() and constructing timespec64 when needed?
>
> Honza
The main advantage is that by using that, I didn't need to do quite so
much of this conversion by hand. My coccinelle skills are pretty
primitive. I went with whatever conversion was going to give minimal
changes, to the existing accesses for the most part.
We could certainly do it the way you suggest, it just means having to
re-touch a lot of this code by hand, or someone with better coccinelle
chops suggesting a way to declare a temporary variables in place.
--
Jeff Layton <[email protected]>
On Wed 21-06-23 12:57:19, Jeff Layton wrote:
> On Wed, 2023-06-21 at 18:48 +0200, Jan Kara wrote:
> > On Wed 21-06-23 10:45:28, Jeff Layton wrote:
> > > In later patches, we're going to change how the ctime.tv_nsec field is
> > > utilized. Switch to using accessor functions instead of raw accesses of
> > > inode->i_ctime.
> > >
> > > Signed-off-by: Jeff Layton <[email protected]>
> >
> > ...
> >
> > > diff --git a/fs/bfs/inode.c b/fs/bfs/inode.c
> > > index 1926bec2c850..c964316be32b 100644
> > > --- a/fs/bfs/inode.c
> > > +++ b/fs/bfs/inode.c
> > > @@ -82,10 +82,10 @@ struct inode *bfs_iget(struct super_block *sb, unsigned long ino)
> > > inode->i_blocks = BFS_FILEBLOCKS(di);
> > > inode->i_atime.tv_sec = le32_to_cpu(di->i_atime);
> > > inode->i_mtime.tv_sec = le32_to_cpu(di->i_mtime);
> > > - inode->i_ctime.tv_sec = le32_to_cpu(di->i_ctime);
> > > + inode_ctime_set_sec(inode, le32_to_cpu(di->i_ctime));
> > > inode->i_atime.tv_nsec = 0;
> > > inode->i_mtime.tv_nsec = 0;
> > > - inode->i_ctime.tv_nsec = 0;
> > > + inode_ctime_set_nsec(inode, 0);
> >
> > So I'm somewhat wondering here - in other filesystem you construct
> > timespec64 and then use inode_ctime_set(). Here you use
> > inode_ctime_set_sec() + inode_ctime_set_nsec(). What's the benefit? It
> > seems these two functions are not used that much some maybe we could just
> > live with just inode_ctime_set() and constructing timespec64 when needed?
> >
> > Honza
>
> The main advantage is that by using that, I didn't need to do quite so
> much of this conversion by hand. My coccinelle skills are pretty
> primitive. I went with whatever conversion was going to give minimal
> changes, to the existing accesses for the most part.
>
> We could certainly do it the way you suggest, it just means having to
> re-touch a lot of this code by hand, or someone with better coccinelle
> chops suggesting a way to declare a temporary variables in place.
Well, maybe temporary variables aren't that convenient but we could provide
function setting ctime from sec & nsec value without having to declare
temporary timespec64? Attached is a semantic patch that should deal with
that - at least it seems to handle all the cases I've found.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Thu, 2023-06-22 at 14:30 +0200, Jan Kara wrote:
> On Wed 21-06-23 12:57:19, Jeff Layton wrote:
> > On Wed, 2023-06-21 at 18:48 +0200, Jan Kara wrote:
> > > On Wed 21-06-23 10:45:28, Jeff Layton wrote:
> > > > In later patches, we're going to change how the ctime.tv_nsec field is
> > > > utilized. Switch to using accessor functions instead of raw accesses of
> > > > inode->i_ctime.
> > > >
> > > > Signed-off-by: Jeff Layton <[email protected]>
> > >
> > > ...
> > >
> > > > diff --git a/fs/bfs/inode.c b/fs/bfs/inode.c
> > > > index 1926bec2c850..c964316be32b 100644
> > > > --- a/fs/bfs/inode.c
> > > > +++ b/fs/bfs/inode.c
> > > > @@ -82,10 +82,10 @@ struct inode *bfs_iget(struct super_block *sb, unsigned long ino)
> > > > inode->i_blocks = BFS_FILEBLOCKS(di);
> > > > inode->i_atime.tv_sec = le32_to_cpu(di->i_atime);
> > > > inode->i_mtime.tv_sec = le32_to_cpu(di->i_mtime);
> > > > - inode->i_ctime.tv_sec = le32_to_cpu(di->i_ctime);
> > > > + inode_ctime_set_sec(inode, le32_to_cpu(di->i_ctime));
> > > > inode->i_atime.tv_nsec = 0;
> > > > inode->i_mtime.tv_nsec = 0;
> > > > - inode->i_ctime.tv_nsec = 0;
> > > > + inode_ctime_set_nsec(inode, 0);
> > >
> > > So I'm somewhat wondering here - in other filesystem you construct
> > > timespec64 and then use inode_ctime_set(). Here you use
> > > inode_ctime_set_sec() + inode_ctime_set_nsec(). What's the benefit? It
> > > seems these two functions are not used that much some maybe we could just
> > > live with just inode_ctime_set() and constructing timespec64 when needed?
> > >
> > > Honza
> >
> > The main advantage is that by using that, I didn't need to do quite so
> > much of this conversion by hand. My coccinelle skills are pretty
> > primitive. I went with whatever conversion was going to give minimal
> > changes, to the existing accesses for the most part.
> >
> > We could certainly do it the way you suggest, it just means having to
> > re-touch a lot of this code by hand, or someone with better coccinelle
> > chops suggesting a way to declare a temporary variables in place.
>
> Well, maybe temporary variables aren't that convenient but we could provide
> function setting ctime from sec & nsec value without having to declare
> temporary timespec64? Attached is a semantic patch that should deal with
> that - at least it seems to handle all the cases I've found.
>
Ok, let me try respinning this with your cocci script and see how it
looks.
Damien also suggested in a reply to the zonefs patch a preference for
the naming style you have above. Should I also rename these like?
inode_ctime_peek -> inode_get_ctime
inode_ctime_set -> inode_set_ctime
This would be the time to change it if that's preferred.
--
Jeff Layton <[email protected]>
On Thu 22-06-23 08:51:58, Jeff Layton wrote:
> On Thu, 2023-06-22 at 14:30 +0200, Jan Kara wrote:
> > On Wed 21-06-23 12:57:19, Jeff Layton wrote:
> > > On Wed, 2023-06-21 at 18:48 +0200, Jan Kara wrote:
> > > > On Wed 21-06-23 10:45:28, Jeff Layton wrote:
> > > > > In later patches, we're going to change how the ctime.tv_nsec field is
> > > > > utilized. Switch to using accessor functions instead of raw accesses of
> > > > > inode->i_ctime.
> > > > >
> > > > > Signed-off-by: Jeff Layton <[email protected]>
> > > >
> > > > ...
> > > >
> > > > > diff --git a/fs/bfs/inode.c b/fs/bfs/inode.c
> > > > > index 1926bec2c850..c964316be32b 100644
> > > > > --- a/fs/bfs/inode.c
> > > > > +++ b/fs/bfs/inode.c
> > > > > @@ -82,10 +82,10 @@ struct inode *bfs_iget(struct super_block *sb, unsigned long ino)
> > > > > inode->i_blocks = BFS_FILEBLOCKS(di);
> > > > > inode->i_atime.tv_sec = le32_to_cpu(di->i_atime);
> > > > > inode->i_mtime.tv_sec = le32_to_cpu(di->i_mtime);
> > > > > - inode->i_ctime.tv_sec = le32_to_cpu(di->i_ctime);
> > > > > + inode_ctime_set_sec(inode, le32_to_cpu(di->i_ctime));
> > > > > inode->i_atime.tv_nsec = 0;
> > > > > inode->i_mtime.tv_nsec = 0;
> > > > > - inode->i_ctime.tv_nsec = 0;
> > > > > + inode_ctime_set_nsec(inode, 0);
> > > >
> > > > So I'm somewhat wondering here - in other filesystem you construct
> > > > timespec64 and then use inode_ctime_set(). Here you use
> > > > inode_ctime_set_sec() + inode_ctime_set_nsec(). What's the benefit? It
> > > > seems these two functions are not used that much some maybe we could just
> > > > live with just inode_ctime_set() and constructing timespec64 when needed?
> > > >
> > > > Honza
> > >
> > > The main advantage is that by using that, I didn't need to do quite so
> > > much of this conversion by hand. My coccinelle skills are pretty
> > > primitive. I went with whatever conversion was going to give minimal
> > > changes, to the existing accesses for the most part.
> > >
> > > We could certainly do it the way you suggest, it just means having to
> > > re-touch a lot of this code by hand, or someone with better coccinelle
> > > chops suggesting a way to declare a temporary variables in place.
> >
> > Well, maybe temporary variables aren't that convenient but we could provide
> > function setting ctime from sec & nsec value without having to declare
> > temporary timespec64? Attached is a semantic patch that should deal with
> > that - at least it seems to handle all the cases I've found.
> >
>
> Ok, let me try respinning this with your cocci script and see how it
> looks.
>
> Damien also suggested in a reply to the zonefs patch a preference for
> the naming style you have above. Should I also rename these like?
>
> inode_ctime_peek -> inode_get_ctime
> inode_ctime_set -> inode_set_ctime
>
> This would be the time to change it if that's preferred.
I don't really care much so whatever you decide is better :)
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Thu, Jun 22, 2023 at 04:57:47PM +0200, Jan Kara wrote:
> On Thu 22-06-23 08:51:58, Jeff Layton wrote:
> > On Thu, 2023-06-22 at 14:30 +0200, Jan Kara wrote:
> > > On Wed 21-06-23 12:57:19, Jeff Layton wrote:
> > > > On Wed, 2023-06-21 at 18:48 +0200, Jan Kara wrote:
> > > > > On Wed 21-06-23 10:45:28, Jeff Layton wrote:
> > > > > > In later patches, we're going to change how the ctime.tv_nsec field is
> > > > > > utilized. Switch to using accessor functions instead of raw accesses of
> > > > > > inode->i_ctime.
> > > > > >
> > > > > > Signed-off-by: Jeff Layton <[email protected]>
> > > > >
> > > > > ...
> > > > >
> > > > > > diff --git a/fs/bfs/inode.c b/fs/bfs/inode.c
> > > > > > index 1926bec2c850..c964316be32b 100644
> > > > > > --- a/fs/bfs/inode.c
> > > > > > +++ b/fs/bfs/inode.c
> > > > > > @@ -82,10 +82,10 @@ struct inode *bfs_iget(struct super_block *sb, unsigned long ino)
> > > > > > inode->i_blocks = BFS_FILEBLOCKS(di);
> > > > > > inode->i_atime.tv_sec = le32_to_cpu(di->i_atime);
> > > > > > inode->i_mtime.tv_sec = le32_to_cpu(di->i_mtime);
> > > > > > - inode->i_ctime.tv_sec = le32_to_cpu(di->i_ctime);
> > > > > > + inode_ctime_set_sec(inode, le32_to_cpu(di->i_ctime));
> > > > > > inode->i_atime.tv_nsec = 0;
> > > > > > inode->i_mtime.tv_nsec = 0;
> > > > > > - inode->i_ctime.tv_nsec = 0;
> > > > > > + inode_ctime_set_nsec(inode, 0);
> > > > >
> > > > > So I'm somewhat wondering here - in other filesystem you construct
> > > > > timespec64 and then use inode_ctime_set(). Here you use
> > > > > inode_ctime_set_sec() + inode_ctime_set_nsec(). What's the benefit? It
> > > > > seems these two functions are not used that much some maybe we could just
> > > > > live with just inode_ctime_set() and constructing timespec64 when needed?
> > > > >
> > > > > Honza
> > > >
> > > > The main advantage is that by using that, I didn't need to do quite so
> > > > much of this conversion by hand. My coccinelle skills are pretty
> > > > primitive. I went with whatever conversion was going to give minimal
> > > > changes, to the existing accesses for the most part.
> > > >
> > > > We could certainly do it the way you suggest, it just means having to
> > > > re-touch a lot of this code by hand, or someone with better coccinelle
> > > > chops suggesting a way to declare a temporary variables in place.
> > >
> > > Well, maybe temporary variables aren't that convenient but we could provide
> > > function setting ctime from sec & nsec value without having to declare
> > > temporary timespec64? Attached is a semantic patch that should deal with
> > > that - at least it seems to handle all the cases I've found.
> > >
> >
> > Ok, let me try respinning this with your cocci script and see how it
> > looks.
> >
> > Damien also suggested in a reply to the zonefs patch a preference for
> > the naming style you have above. Should I also rename these like?
> >
> > inode_ctime_peek -> inode_get_ctime
> > inode_ctime_set -> inode_set_ctime
> >
> > This would be the time to change it if that's preferred.
>
> I don't really care much so whatever you decide is better :)
I have a mild preference for inode_{get,set}_ctime().
On Fri, Jun 23, 2023 at 02:33:26PM +0200, Christian Brauner wrote:
> On Thu, Jun 22, 2023 at 04:57:47PM +0200, Jan Kara wrote:
> > On Thu 22-06-23 08:51:58, Jeff Layton wrote:
> > > On Thu, 2023-06-22 at 14:30 +0200, Jan Kara wrote:
> > > > On Wed 21-06-23 12:57:19, Jeff Layton wrote:
> > > > > On Wed, 2023-06-21 at 18:48 +0200, Jan Kara wrote:
> > > > > > On Wed 21-06-23 10:45:28, Jeff Layton wrote:
> > > > > > > In later patches, we're going to change how the ctime.tv_nsec field is
> > > > > > > utilized. Switch to using accessor functions instead of raw accesses of
> > > > > > > inode->i_ctime.
> > > > > > >
> > > > > > > Signed-off-by: Jeff Layton <[email protected]>
> > > > > >
> > > > > > ...
> > > > > >
> > > > > > > diff --git a/fs/bfs/inode.c b/fs/bfs/inode.c
> > > > > > > index 1926bec2c850..c964316be32b 100644
> > > > > > > --- a/fs/bfs/inode.c
> > > > > > > +++ b/fs/bfs/inode.c
> > > > > > > @@ -82,10 +82,10 @@ struct inode *bfs_iget(struct super_block *sb, unsigned long ino)
> > > > > > > inode->i_blocks = BFS_FILEBLOCKS(di);
> > > > > > > inode->i_atime.tv_sec = le32_to_cpu(di->i_atime);
> > > > > > > inode->i_mtime.tv_sec = le32_to_cpu(di->i_mtime);
> > > > > > > - inode->i_ctime.tv_sec = le32_to_cpu(di->i_ctime);
> > > > > > > + inode_ctime_set_sec(inode, le32_to_cpu(di->i_ctime));
> > > > > > > inode->i_atime.tv_nsec = 0;
> > > > > > > inode->i_mtime.tv_nsec = 0;
> > > > > > > - inode->i_ctime.tv_nsec = 0;
> > > > > > > + inode_ctime_set_nsec(inode, 0);
> > > > > >
> > > > > > So I'm somewhat wondering here - in other filesystem you construct
> > > > > > timespec64 and then use inode_ctime_set(). Here you use
> > > > > > inode_ctime_set_sec() + inode_ctime_set_nsec(). What's the benefit? It
> > > > > > seems these two functions are not used that much some maybe we could just
> > > > > > live with just inode_ctime_set() and constructing timespec64 when needed?
> > > > > >
> > > > > > Honza
> > > > >
> > > > > The main advantage is that by using that, I didn't need to do quite so
> > > > > much of this conversion by hand. My coccinelle skills are pretty
> > > > > primitive. I went with whatever conversion was going to give minimal
> > > > > changes, to the existing accesses for the most part.
> > > > >
> > > > > We could certainly do it the way you suggest, it just means having to
> > > > > re-touch a lot of this code by hand, or someone with better coccinelle
> > > > > chops suggesting a way to declare a temporary variables in place.
> > > >
> > > > Well, maybe temporary variables aren't that convenient but we could provide
> > > > function setting ctime from sec & nsec value without having to declare
> > > > temporary timespec64? Attached is a semantic patch that should deal with
> > > > that - at least it seems to handle all the cases I've found.
> > > >
> > >
> > > Ok, let me try respinning this with your cocci script and see how it
> > > looks.
> > >
> > > Damien also suggested in a reply to the zonefs patch a preference for
> > > the naming style you have above. Should I also rename these like?
> > >
> > > inode_ctime_peek -> inode_get_ctime
> > > inode_ctime_set -> inode_set_ctime
> > >
> > > This would be the time to change it if that's preferred.
> >
> > I don't really care much so whatever you decide is better :)
>
> I have a mild preference for inode_{get,set}_ctime().
Jeff, did you plan on sending a v2 with this renamed or do you want me
to pick this up now?
On Mon, Jul 03, 2023 at 06:46:33AM -0400, Jeff Layton wrote:
> On Mon, 2023-07-03 at 12:12 +0200, Christian Brauner wrote:
> > On Fri, Jun 23, 2023 at 02:33:26PM +0200, Christian Brauner wrote:
> > > On Thu, Jun 22, 2023 at 04:57:47PM +0200, Jan Kara wrote:
> > > > On Thu 22-06-23 08:51:58, Jeff Layton wrote:
> > > > > On Thu, 2023-06-22 at 14:30 +0200, Jan Kara wrote:
> > > > > > On Wed 21-06-23 12:57:19, Jeff Layton wrote:
> > > > > > > On Wed, 2023-06-21 at 18:48 +0200, Jan Kara wrote:
> > > > > > > > On Wed 21-06-23 10:45:28, Jeff Layton wrote:
> > > > > > > > > In later patches, we're going to change how the ctime.tv_nsec field is
> > > > > > > > > utilized. Switch to using accessor functions instead of raw accesses of
> > > > > > > > > inode->i_ctime.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Jeff Layton <[email protected]>
> > > > > > > >
> > > > > > > > ...
> > > > > > > >
> > > > > > > > > diff --git a/fs/bfs/inode.c b/fs/bfs/inode.c
> > > > > > > > > index 1926bec2c850..c964316be32b 100644
> > > > > > > > > --- a/fs/bfs/inode.c
> > > > > > > > > +++ b/fs/bfs/inode.c
> > > > > > > > > @@ -82,10 +82,10 @@ struct inode *bfs_iget(struct super_block *sb, unsigned long ino)
> > > > > > > > > inode->i_blocks = BFS_FILEBLOCKS(di);
> > > > > > > > > inode->i_atime.tv_sec = le32_to_cpu(di->i_atime);
> > > > > > > > > inode->i_mtime.tv_sec = le32_to_cpu(di->i_mtime);
> > > > > > > > > - inode->i_ctime.tv_sec = le32_to_cpu(di->i_ctime);
> > > > > > > > > + inode_ctime_set_sec(inode, le32_to_cpu(di->i_ctime));
> > > > > > > > > inode->i_atime.tv_nsec = 0;
> > > > > > > > > inode->i_mtime.tv_nsec = 0;
> > > > > > > > > - inode->i_ctime.tv_nsec = 0;
> > > > > > > > > + inode_ctime_set_nsec(inode, 0);
> > > > > > > >
> > > > > > > > So I'm somewhat wondering here - in other filesystem you construct
> > > > > > > > timespec64 and then use inode_ctime_set(). Here you use
> > > > > > > > inode_ctime_set_sec() + inode_ctime_set_nsec(). What's the benefit? It
> > > > > > > > seems these two functions are not used that much some maybe we could just
> > > > > > > > live with just inode_ctime_set() and constructing timespec64 when needed?
> > > > > > > >
> > > > > > > > Honza
> > > > > > >
> > > > > > > The main advantage is that by using that, I didn't need to do quite so
> > > > > > > much of this conversion by hand. My coccinelle skills are pretty
> > > > > > > primitive. I went with whatever conversion was going to give minimal
> > > > > > > changes, to the existing accesses for the most part.
> > > > > > >
> > > > > > > We could certainly do it the way you suggest, it just means having to
> > > > > > > re-touch a lot of this code by hand, or someone with better coccinelle
> > > > > > > chops suggesting a way to declare a temporary variables in place.
> > > > > >
> > > > > > Well, maybe temporary variables aren't that convenient but we could provide
> > > > > > function setting ctime from sec & nsec value without having to declare
> > > > > > temporary timespec64? Attached is a semantic patch that should deal with
> > > > > > that - at least it seems to handle all the cases I've found.
> > > > > >
> > > > >
> > > > > Ok, let me try respinning this with your cocci script and see how it
> > > > > looks.
> > > > >
> > > > > Damien also suggested in a reply to the zonefs patch a preference for
> > > > > the naming style you have above. Should I also rename these like?
> > > > >
> > > > > inode_ctime_peek -> inode_get_ctime
> > > > > inode_ctime_set -> inode_set_ctime
> > > > >
> > > > > This would be the time to change it if that's preferred.
> > > >
> > > > I don't really care much so whatever you decide is better :)
> > >
> > > I have a mild preference for inode_{get,set}_ctime().
> >
> > Jeff, did you plan on sending a v2 with this renamed or do you want me
> > to pick this up now?
>
> I'm working on a new set that I'll send out in a few days. Sorry it has
> taken a while, I spent quite a bit of time trying to improve my
> coccinelle chops for this.
Absolutely no problem of course. I just wanted to check that I didn't
pointlessly delay you by not taking care of this.
Thanks!
On Mon, 2023-07-03 at 12:12 +0200, Christian Brauner wrote:
> On Fri, Jun 23, 2023 at 02:33:26PM +0200, Christian Brauner wrote:
> > On Thu, Jun 22, 2023 at 04:57:47PM +0200, Jan Kara wrote:
> > > On Thu 22-06-23 08:51:58, Jeff Layton wrote:
> > > > On Thu, 2023-06-22 at 14:30 +0200, Jan Kara wrote:
> > > > > On Wed 21-06-23 12:57:19, Jeff Layton wrote:
> > > > > > On Wed, 2023-06-21 at 18:48 +0200, Jan Kara wrote:
> > > > > > > On Wed 21-06-23 10:45:28, Jeff Layton wrote:
> > > > > > > > In later patches, we're going to change how the ctime.tv_nsec field is
> > > > > > > > utilized. Switch to using accessor functions instead of raw accesses of
> > > > > > > > inode->i_ctime.
> > > > > > > >
> > > > > > > > Signed-off-by: Jeff Layton <[email protected]>
> > > > > > >
> > > > > > > ...
> > > > > > >
> > > > > > > > diff --git a/fs/bfs/inode.c b/fs/bfs/inode.c
> > > > > > > > index 1926bec2c850..c964316be32b 100644
> > > > > > > > --- a/fs/bfs/inode.c
> > > > > > > > +++ b/fs/bfs/inode.c
> > > > > > > > @@ -82,10 +82,10 @@ struct inode *bfs_iget(struct super_block *sb, unsigned long ino)
> > > > > > > > inode->i_blocks = BFS_FILEBLOCKS(di);
> > > > > > > > inode->i_atime.tv_sec = le32_to_cpu(di->i_atime);
> > > > > > > > inode->i_mtime.tv_sec = le32_to_cpu(di->i_mtime);
> > > > > > > > - inode->i_ctime.tv_sec = le32_to_cpu(di->i_ctime);
> > > > > > > > + inode_ctime_set_sec(inode, le32_to_cpu(di->i_ctime));
> > > > > > > > inode->i_atime.tv_nsec = 0;
> > > > > > > > inode->i_mtime.tv_nsec = 0;
> > > > > > > > - inode->i_ctime.tv_nsec = 0;
> > > > > > > > + inode_ctime_set_nsec(inode, 0);
> > > > > > >
> > > > > > > So I'm somewhat wondering here - in other filesystem you construct
> > > > > > > timespec64 and then use inode_ctime_set(). Here you use
> > > > > > > inode_ctime_set_sec() + inode_ctime_set_nsec(). What's the benefit? It
> > > > > > > seems these two functions are not used that much some maybe we could just
> > > > > > > live with just inode_ctime_set() and constructing timespec64 when needed?
> > > > > > >
> > > > > > > Honza
> > > > > >
> > > > > > The main advantage is that by using that, I didn't need to do quite so
> > > > > > much of this conversion by hand. My coccinelle skills are pretty
> > > > > > primitive. I went with whatever conversion was going to give minimal
> > > > > > changes, to the existing accesses for the most part.
> > > > > >
> > > > > > We could certainly do it the way you suggest, it just means having to
> > > > > > re-touch a lot of this code by hand, or someone with better coccinelle
> > > > > > chops suggesting a way to declare a temporary variables in place.
> > > > >
> > > > > Well, maybe temporary variables aren't that convenient but we could provide
> > > > > function setting ctime from sec & nsec value without having to declare
> > > > > temporary timespec64? Attached is a semantic patch that should deal with
> > > > > that - at least it seems to handle all the cases I've found.
> > > > >
> > > >
> > > > Ok, let me try respinning this with your cocci script and see how it
> > > > looks.
> > > >
> > > > Damien also suggested in a reply to the zonefs patch a preference for
> > > > the naming style you have above. Should I also rename these like?
> > > >
> > > > inode_ctime_peek -> inode_get_ctime
> > > > inode_ctime_set -> inode_set_ctime
> > > >
> > > > This would be the time to change it if that's preferred.
> > >
> > > I don't really care much so whatever you decide is better :)
> >
> > I have a mild preference for inode_{get,set}_ctime().
>
> Jeff, did you plan on sending a v2 with this renamed or do you want me
> to pick this up now?
I'm working on a new set that I'll send out in a few days. Sorry it has
taken a while, I spent quite a bit of time trying to improve my
coccinelle chops for this.
Thanks,
--
Jeff Layton <[email protected]>