Date: Wed, 21 Jun 2023 14:48:17 -0400
From: Jeff Layton <jlayton@...nel.org>
To: Tom Talpey <tom@...pey.com>, Jeremy Kerr <jk@...abs.org>, Arnd Bergmann
 <arnd@...db.de>, Michael Ellerman <mpe@...erman.id.au>, Nicholas Piggin
 <npiggin@...il.com>, Christophe Leroy <christophe.leroy@...roup.eu>, Heiko
 Carstens <hca@...ux.ibm.com>, Vasily Gorbik <gor@...ux.ibm.com>, Alexander
 Gordeev <agordeev@...ux.ibm.com>, Christian Borntraeger
 <borntraeger@...ux.ibm.com>,  Sven Schnelle <svens@...ux.ibm.com>, Greg
 Kroah-Hartman <gregkh@...uxfoundation.org>, Arve
 Hjønnevåg <arve@...roid.com>, Todd Kjos
 <tkjos@...roid.com>, Martijn Coenen <maco@...roid.com>, Joel Fernandes
 <joel@...lfernandes.org>, Christian Brauner <brauner@...nel.org>, Carlos
 Llamas <cmllamas@...gle.com>, Suren Baghdasaryan <surenb@...gle.com>,
 Dennis Dalessandro <dennis.dalessandro@...nelisnetworks.com>, Jason
 Gunthorpe <jgg@...pe.ca>,  Leon Romanovsky <leon@...nel.org>, Brad Warrum
 <bwarrum@...ux.ibm.com>, Ritu Agarwal <rituagar@...ux.ibm.com>, Eric Van
 Hensbergen <ericvh@...nel.org>, Latchesar Ionkov <lucho@...kov.net>,
 Dominique Martinet <asmadeus@...ewreck.org>, Christian Schoenebeck
 <linux_oss@...debyte.com>, David Sterba <dsterba@...e.com>, David Howells
 <dhowells@...hat.com>, Marc Dionne <marc.dionne@...istor.com>, Alexander
 Viro <viro@...iv.linux.org.uk>, Ian Kent <raven@...maw.net>, Luis de
 Bethencourt <luisbg@...nel.org>, Salah Triki <salah.triki@...il.com>,
 "Tigran A. Aivazian" <aivazian.tigran@...il.com>, Eric Biederman
 <ebiederm@...ssion.com>, Kees Cook <keescook@...omium.org>, Chris Mason
 <clm@...com>, Josef Bacik <josef@...icpanda.com>, Xiubo Li
 <xiubli@...hat.com>, Ilya Dryomov <idryomov@...il.com>, Jan Harkes
 <jaharkes@...cmu.edu>, coda@...cmu.edu, Joel Becker <jlbec@...lplan.org>,
 Christoph Hellwig <hch@....de>, Nicolas Pitre <nico@...xnic.net>,  "Rafael
 J. Wysocki" <rafael@...nel.org>, Tyler Hicks <code@...icks.com>, Ard
 Biesheuvel <ardb@...nel.org>, Gao Xiang <xiang@...nel.org>, Chao Yu
 <chao@...nel.org>,  Yue Hu <huyue2@...lpad.com>, Jeffle Xu
 <jefflexu@...ux.alibaba.com>, Namjae Jeon <linkinjeon@...nel.org>, Sungjong
 Seo <sj1557.seo@...sung.com>, Jan Kara <jack@...e.com>, Theodore Ts'o
 <tytso@....edu>, Andreas Dilger <adilger.kernel@...ger.ca>, Jaegeuk Kim
 <jaegeuk@...nel.org>, OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>, Miklos
 Szeredi <miklos@...redi.hu>, Bob Peterson <rpeterso@...hat.com>, Andreas
 Gruenbacher <agruenba@...hat.com>, Richard Weinberger <richard@....at>,
 Anton Ivanov <anton.ivanov@...bridgegreys.com>, Johannes Berg
 <johannes@...solutions.net>, Mikulas Patocka
 <mikulas@...ax.karlin.mff.cuni.cz>,  Mike Kravetz
 <mike.kravetz@...cle.com>, Muchun Song <muchun.song@...ux.dev>, David
 Woodhouse <dwmw2@...radead.org>, Dave Kleikamp <shaggy@...nel.org>, Tejun
 Heo <tj@...nel.org>, Trond Myklebust <trond.myklebust@...merspace.com>,
 Anna Schumaker <anna@...nel.org>, Chuck Lever <chuck.lever@...cle.com>,
 Ryusuke Konishi <konishi.ryusuke@...il.com>, Anton Altaparmakov
 <anton@...era.com>,  Konstantin Komarov
 <almaz.alexandrovich@...agon-software.com>, Mark Fasheh <mark@...heh.com>,
 Joseph Qi <joseph.qi@...ux.alibaba.com>, Bob Copeland <me@...copeland.com>,
 Mike Marshall <hubcap@...ibond.com>, Martin Brandenburg
 <martin@...ibond.com>, Luis Chamberlain <mcgrof@...nel.org>, Iurii Zaikin
 <yzaikin@...gle.com>, Tony Luck <tony.luck@...el.com>,  "Guilherme G.
 Piccoli" <gpiccoli@...lia.com>, Anders Larsen <al@...rsen.net>, Steve
 French <sfrench@...ba.org>, Paulo Alcantara <pc@...guebit.com>, Ronnie
 Sahlberg <lsahlber@...hat.com>, Shyam Prasad N <sprasad@...rosoft.com>,
 Sergey Senozhatsky <senozhatsky@...omium.org>, Phillip Lougher
 <phillip@...ashfs.org.uk>, Steven Rostedt <rostedt@...dmis.org>, Masami
 Hiramatsu <mhiramat@...nel.org>, Evgeniy Dushistov <dushistov@...l.ru>,
 Hans de Goede <hdegoede@...hat.com>, "Darrick J. Wong" <djwong@...nel.org>,
 Damien Le Moal <dlemoal@...nel.org>, Naohiro Aota <naohiro.aota@....com>,
 Johannes Thumshirn <jth@...nel.org>, Alexei Starovoitov <ast@...nel.org>,
 Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko
 <andrii@...nel.org>, Martin KaFai Lau <martin.lau@...ux.dev>, Song Liu
 <song@...nel.org>, Yonghong Song <yhs@...com>, John Fastabend
 <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>, Stanislav
 Fomichev <sdf@...gle.com>, Hao Luo <haoluo@...gle.com>, Jiri Olsa
 <jolsa@...nel.org>,  Hugh Dickins <hughd@...gle.com>, Andrew Morton
 <akpm@...ux-foundation.org>, "David S. Miller" <davem@...emloft.net>, Eric
 Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo
 Abeni <pabeni@...hat.com>, John Johansen <john.johansen@...onical.com>,
 Paul Moore <paul@...l-moore.com>, James Morris <jmorris@...ei.org>, "Serge
 E. Hallyn" <serge@...lyn.com>, Stephen Smalley
 <stephen.smalley.work@...il.com>, Eric Paris <eparis@...isplace.org>, 
 Juergen Gross <jgross@...e.com>, Ruihan Li <lrh2000@....edu.cn>, Laurent
 Pinchart <laurent.pinchart+renesas@...asonboard.com>, Wolfram Sang
 <wsa+renesas@...g-engineering.com>, Udipto Goswami
 <quic_ugoswami@...cinc.com>,  Linyu Yuan <quic_linyyuan@...cinc.com>, John
 Keeping <john@...ping.me.uk>, Andrzej Pietrasiewicz
 <andrzej.p@...labora.com>, Dan Carpenter <error27@...il.com>, Yuta Hayama
 <hayama@...eo.co.jp>, Jozef Martiniak <jomajm@...il.com>, Jens Axboe
 <axboe@...nel.dk>, Alan Stern <stern@...land.harvard.edu>, Sandeep Dhavale
 <dhavale@...gle.com>, Dave Chinner <dchinner@...hat.com>, Johannes Weiner
 <hannes@...xchg.org>, ZhangPeng <zhangpeng362@...wei.com>, Viacheslav
 Dubeyko <slava@...eyko.com>, Tetsuo Handa
 <penguin-kernel@...ove.SAKURA.ne.jp>,  Aditya Garg <gargaditya08@...e.com>,
 Erez Zadok <ezk@...stonybrook.edu>, Yifei Liu <yifeliu@...stonybrook.edu>,
 Yu Zhe <yuzhe@...china.com>, "Matthew Wilcox (Oracle)"
 <willy@...radead.org>, Oleg Kanatov <okanatov@...il.com>, "Dr. David Alan
 Gilbert" <linux@...blig.org>, Jiangshan Yi <yijiangshan@...inos.cn>, xu xin
 <cgel.zte@...il.com>, Stefan Roesch <shr@...kernel.io>, Zhihao Cheng
 <chengzhihao1@...wei.com>, "Liam R. Howlett" <Liam.Howlett@...cle.com>, 
 Alexey Dobriyan <adobriyan@...il.com>, Minghao Chi
 <chi.minghao@....com.cn>, Seth Forshee <sforshee@...italocean.com>, Zeng
 Jingxiang <linuszeng@...cent.com>, Bart Van Assche <bvanassche@....org>,
 Mimi Zohar <zohar@...ux.ibm.com>, Roberto Sassu <roberto.sassu@...wei.com>,
 Zhang Yi <yi.zhang@...wei.com>, Tom Rix <trix@...hat.com>, "Fabio M. De
 Francesco" <fmdefrancesco@...il.com>, Chen Zhongjin
 <chenzhongjin@...wei.com>, Zhengchao Shao <shaozhengchao@...wei.com>, Rik
 van Riel <riel@...riel.com>, Jingyu Wang <jingyuwang_vip@....com>, Hangyu
 Hua <hbh25y@...il.com>, linuxppc-dev@...ts.ozlabs.org,
 linux-kernel@...r.kernel.org,  linux-s390@...r.kernel.org,
 linux-rdma@...r.kernel.org,  linux-usb@...r.kernel.org,
 v9fs@...ts.linux.dev, linux-fsdevel@...r.kernel.org, 
 linux-afs@...ts.infradead.org, autofs@...r.kernel.org, linux-mm@...ck.org, 
 linux-btrfs@...r.kernel.org, ceph-devel@...r.kernel.org, 
 codalist@...a.cs.cmu.edu, ecryptfs@...r.kernel.org,
 linux-efi@...r.kernel.org,  linux-erofs@...ts.ozlabs.org,
 linux-ext4@...r.kernel.org,  linux-f2fs-devel@...ts.sourceforge.net,
 cluster-devel@...hat.com,  linux-um@...ts.infradead.org,
 linux-mtd@...ts.infradead.org,  jfs-discussion@...ts.sourceforge.net,
 linux-nfs@...r.kernel.org,  linux-nilfs@...r.kernel.org,
 linux-ntfs-dev@...ts.sourceforge.net,  ntfs3@...ts.linux.dev,
 ocfs2-devel@....oracle.com,  linux-karma-devel@...ts.sourceforge.net,
 devel@...ts.orangefs.org,  linux-unionfs@...r.kernel.org,
 linux-hardening@...r.kernel.org,  reiserfs-devel@...r.kernel.org,
 linux-cifs@...r.kernel.org,  samba-technical@...ts.samba.org,
 linux-trace-kernel@...r.kernel.org,  linux-xfs@...r.kernel.org,
 bpf@...r.kernel.org, netdev@...r.kernel.org,  apparmor@...ts.ubuntu.com,
 linux-security-module@...r.kernel.org,  selinux@...r.kernel.org
Subject: Re: [PATCH 01/79] fs: add ctime accessors infrastructure

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@...nel.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@...pey.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.
> > 

-- 
Jeff Layton <jlayton@...nel.org>

Powered by blists - more mailing lists