diff mbox series

[15/79] bfs: switch to new ctime accessors

Message ID 20230621144735.55953-14-jlayton@kernel.org
State New
Headers
Series None |

Commit Message

Jeff Layton June 21, 2023, 2:45 p.m. UTC 
  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 <jlayton@kernel.org>
---
 fs/bfs/dir.c   | 16 ++++++++--------
 fs/bfs/inode.c |  6 +++---
 2 files changed, 11 insertions(+), 11 deletions(-)

Comments

Jan Kara June 21, 2023, 4:48 p.m. UTC | #1 
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 <jlayton@kernel.org>

...

> 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
Jeff Layton June 21, 2023, 4:57 p.m. UTC | #2 
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 <jlayton@kernel.org>
> 
> ...
> 
> > 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.
Jan Kara June 22, 2023, 12:30 p.m. UTC | #3 
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 <jlayton@kernel.org>
> > 
> > ...
> > 
> > > 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
Jeff Layton June 22, 2023, 12:51 p.m. UTC | #4 
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 <jlayton@kernel.org>
> > > 
> > > ...
> > > 
> > > > 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.
Jan Kara June 22, 2023, 2:57 p.m. UTC | #5 
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 <jlayton@kernel.org>
> > > > 
> > > > ...
> > > > 
> > > > > 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
Christian Brauner June 23, 2023, 12:33 p.m. UTC | #6 
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 <jlayton@kernel.org>
> > > > > 
> > > > > ...
> > > > > 
> > > > > > 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().
Christian Brauner July 3, 2023, 10:12 a.m. UTC | #7 
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 <jlayton@kernel.org>
> > > > > > 
> > > > > > ...
> > > > > > 
> > > > > > > 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?
Jeff Layton July 3, 2023, 10:46 a.m. UTC | #8 
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 <jlayton@kernel.org>
> > > > > > > 
> > > > > > > ...
> > > > > > > 
> > > > > > > > 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,
Christian Brauner July 3, 2023, 10:57 a.m. UTC | #9 
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 <jlayton@kernel.org>
> > > > > > > > 
> > > > > > > > ...
> > > > > > > > 
> > > > > > > > > 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!
diff mbox series

Patch

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);