Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <646b7283ede4945b335ad16aea5ff60e1361241e.camel@kernel.org>
Subject: Re: [PATCH 15/79] bfs: switch to new ctime accessors
From:   Jeff Layton <jlayton@kernel.org>
To:     Jan Kara <jack@suse.cz>
Cc:     Christian Brauner <brauner@kernel.org>,
        "Tigran A. Aivazian" <aivazian.tigran@gmail.com>,
        Al Viro <viro@zeniv.linux.org.uk>, linux-kernel@vger.kernel.org
Date:   Wed, 21 Jun 2023 12:57:19 -0400
In-Reply-To: <20230621164808.5lhujni7qb36hhtk@quack3>
References: <20230621144507.55591-1-jlayton@kernel.org>
         <20230621144735.55953-1-jlayton@kernel.org>
         <20230621144735.55953-14-jlayton@kernel.org>
         <20230621164808.5lhujni7qb36hhtk@quack3>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.3 (3.48.3-1.fc38) 
MIME-Version: 1.0
Precedence: bulk

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.
> >=20
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
>=20
> ...
>=20
> > 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, unsi=
gned long ino)
> >  	inode->i_blocks =3D BFS_FILEBLOCKS(di);
> >  	inode->i_atime.tv_sec =3D  le32_to_cpu(di->i_atime);
> >  	inode->i_mtime.tv_sec =3D  le32_to_cpu(di->i_mtime);
> > -	inode->i_ctime.tv_sec =3D  le32_to_cpu(di->i_ctime);
> > +	inode_ctime_set_sec(inode, le32_to_cpu(di->i_ctime));
> >  	inode->i_atime.tv_nsec =3D 0;
> >  	inode->i_mtime.tv_nsec =3D 0;
> > -	inode->i_ctime.tv_nsec =3D 0;
> > +	inode_ctime_set_nsec(inode, 0);
>=20
> 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?
>=20
> 								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.
--=20
Jeff Layton <jlayton@kernel.org>