Message-ID: <3b403ef1-22e6-0220-6c9c-435e3444b4d3@kernel.org>
Date: Thu, 6 Jul 2023 08:19:00 +0900
From: Damien Le Moal <dlemoal@...nel.org>
To: Jeff Layton <jlayton@...nel.org>, jk@...abs.org, arnd@...db.de,
 mpe@...erman.id.au, npiggin@...il.com, christophe.leroy@...roup.eu,
 hca@...ux.ibm.com, gor@...ux.ibm.com, agordeev@...ux.ibm.com,
 borntraeger@...ux.ibm.com, svens@...ux.ibm.com, gregkh@...uxfoundation.org,
 arve@...roid.com, tkjos@...roid.com, maco@...roid.com,
 joel@...lfernandes.org, brauner@...nel.org, cmllamas@...gle.com,
 surenb@...gle.com, dennis.dalessandro@...nelisnetworks.com, jgg@...pe.ca,
 leon@...nel.org, bwarrum@...ux.ibm.com, rituagar@...ux.ibm.com,
 ericvh@...nel.org, lucho@...kov.net, asmadeus@...ewreck.org,
 linux_oss@...debyte.com, dsterba@...e.com, dhowells@...hat.com,
 marc.dionne@...istor.com, viro@...iv.linux.org.uk, raven@...maw.net,
 luisbg@...nel.org, salah.triki@...il.com, aivazian.tigran@...il.com,
 ebiederm@...ssion.com, keescook@...omium.org, clm@...com,
 josef@...icpanda.com, xiubli@...hat.com, idryomov@...il.com,
 jaharkes@...cmu.edu, coda@...cmu.edu, jlbec@...lplan.org, hch@....de,
 nico@...xnic.net, rafael@...nel.org, code@...icks.com, ardb@...nel.org,
 xiang@...nel.org, chao@...nel.org, huyue2@...lpad.com,
 jefflexu@...ux.alibaba.com, linkinjeon@...nel.org, sj1557.seo@...sung.com,
 jack@...e.com, tytso@....edu, adilger.kernel@...ger.ca, jaegeuk@...nel.org,
 hirofumi@...l.parknet.co.jp, miklos@...redi.hu, rpeterso@...hat.com,
 agruenba@...hat.com, richard@....at, anton.ivanov@...bridgegreys.com,
 johannes@...solutions.net, mikulas@...ax.karlin.mff.cuni.cz,
 mike.kravetz@...cle.com, muchun.song@...ux.dev, dwmw2@...radead.org,
 shaggy@...nel.org, tj@...nel.org, trond.myklebust@...merspace.com,
 anna@...nel.org, chuck.lever@...cle.com, neilb@...e.de, kolga@...app.com,
 Dai.Ngo@...cle.com, tom@...pey.com, konishi.ryusuke@...il.com,
 anton@...era.com, almaz.alexandrovich@...agon-software.com, mark@...heh.com,
 joseph.qi@...ux.alibaba.com, me@...copeland.com, hubcap@...ibond.com,
 martin@...ibond.com, amir73il@...il.com, mcgrof@...nel.org,
 yzaikin@...gle.com, tony.luck@...el.com, gpiccoli@...lia.com,
 al@...rsen.net, sfrench@...ba.org, pc@...guebit.com, lsahlber@...hat.com,
 sprasad@...rosoft.com, senozhatsky@...omium.org, phillip@...ashfs.org.uk,
 rostedt@...dmis.org, mhiramat@...nel.org, dushistov@...l.ru,
 hdegoede@...hat.com, djwong@...nel.org, naohiro.aota@....com,
 jth@...nel.org, ast@...nel.org, daniel@...earbox.net, andrii@...nel.org,
 martin.lau@...ux.dev, song@...nel.org, yhs@...com, john.fastabend@...il.com,
 kpsingh@...nel.org, sdf@...gle.com, haoluo@...gle.com, jolsa@...nel.org,
 hughd@...gle.com, akpm@...ux-foundation.org, davem@...emloft.net,
 edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
 john.johansen@...onical.com, paul@...l-moore.com, jmorris@...ei.org,
 serge@...lyn.com, stephen.smalley.work@...il.com, eparis@...isplace.org,
 jgross@...e.com, stern@...land.harvard.edu, lrh2000@....edu.cn,
 sebastian.reichel@...labora.com, wsa+renesas@...g-engineering.com,
 quic_ugoswami@...cinc.com, quic_linyyuan@...cinc.com, john@...ping.me.uk,
 error27@...il.com, quic_uaggarwa@...cinc.com, hayama@...eo.co.jp,
 jomajm@...il.com, axboe@...nel.dk, dhavale@...gle.com, dchinner@...hat.com,
 hannes@...xchg.org, zhangpeng362@...wei.com, slava@...eyko.com,
 gargaditya08@...e.com, penguin-kernel@...ove.SAKURA.ne.jp,
 yifeliu@...stonybrook.edu, madkar@...stonybrook.edu, ezk@...stonybrook.edu,
 yuzhe@...china.com, willy@...radead.org, okanatov@...il.com,
 jeffxu@...omium.org, linux@...blig.org, mirimmad17@...il.com,
 yijiangshan@...inos.cn, yang.yang29@....com.cn, xu.xin16@....com.cn,
 chengzhihao1@...wei.com, shr@...kernel.io, Liam.Howlett@...cle.com,
 adobriyan@...il.com, chi.minghao@....com.cn, roberto.sassu@...wei.com,
 linuszeng@...cent.com, bvanassche@....org, zohar@...ux.ibm.com,
 yi.zhang@...wei.com, trix@...hat.com, fmdefrancesco@...il.com,
 ebiggers@...gle.com, princekumarmaurya06@...il.com, chenzhongjin@...wei.com,
 riel@...riel.com, shaozhengchao@...wei.com, jingyuwang_vip@....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@...ts.linux.dev,
 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 v2 08/92] fs: new helper: simple_rename_timestamp

On 7/6/23 03:58, Jeff Layton wrote:
> A rename potentially involves updating 4 different inode timestamps. Add
> a function that handles the details sanely, and convert the libfs.c
> callers to use it.
> 
> Signed-off-by: Jeff Layton <jlayton@...nel.org>
> ---
>  fs/libfs.c         | 36 +++++++++++++++++++++++++++---------
>  include/linux/fs.h |  2 ++
>  2 files changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/libfs.c b/fs/libfs.c
> index a7e56baf8bbd..9ee79668c909 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -692,6 +692,31 @@ int simple_rmdir(struct inode *dir, struct dentry *dentry)
>  }
>  EXPORT_SYMBOL(simple_rmdir);
>  
> +/**
> + * simple_rename_timestamp - update the various inode timestamps for rename
> + * @old_dir: old parent directory
> + * @old_dentry: dentry that is being renamed
> + * @new_dir: new parent directory
> + * @new_dentry: target for rename
> + *
> + * POSIX mandates that the old and new parent directories have their ctime and
> + * mtime updated, and that inodes of @old_dentry and @new_dentry (if any), have
> + * their ctime updated.
> + */
> +void simple_rename_timestamp(struct inode *old_dir, struct dentry *old_dentry,
> +			     struct inode *new_dir, struct dentry *new_dentry)
> +{
> +	struct inode *newino = d_inode(new_dentry);
> +
> +	old_dir->i_mtime = inode_set_ctime_current(old_dir);
> +	if (new_dir != old_dir)
> +		new_dir->i_mtime = inode_set_ctime_current(new_dir);
> +	inode_set_ctime_current(d_inode(old_dentry));
> +	if (newino)
> +		inode_set_ctime_current(newino);
> +}
> +EXPORT_SYMBOL_GPL(simple_rename_timestamp);
> +
>  int simple_rename_exchange(struct inode *old_dir, struct dentry *old_dentry,
>  			   struct inode *new_dir, struct dentry *new_dentry)
>  {
> @@ -707,11 +732,7 @@ int simple_rename_exchange(struct inode *old_dir, struct dentry *old_dentry,
>  			inc_nlink(old_dir);
>  		}
>  	}
> -	old_dir->i_ctime = old_dir->i_mtime =
> -	new_dir->i_ctime = new_dir->i_mtime =
> -	d_inode(old_dentry)->i_ctime =
> -	d_inode(new_dentry)->i_ctime = current_time(old_dir);
> -
> +	simple_rename_timestamp(old_dir, old_dentry, new_dir, new_dentry);

This is somewhat changing the current behavior: before the patch, the mtime and
ctime of old_dir, new_dir and the inodes associated with the dentries are always
equal. But given that simple_rename_timestamp() calls inode_set_ctime_current()
4 times, the times could potentially be different.

I am not sure if that is an issue, but it seems that calling
inode_set_ctime_current() once, recording the "now" time it sets and using that
value to set all times may be more efficient and preserve the existing behavior.

>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(simple_rename_exchange);
> @@ -720,7 +741,6 @@ int simple_rename(struct mnt_idmap *idmap, struct inode *old_dir,
>  		  struct dentry *old_dentry, struct inode *new_dir,
>  		  struct dentry *new_dentry, unsigned int flags)
>  {
> -	struct inode *inode = d_inode(old_dentry);
>  	int they_are_dirs = d_is_dir(old_dentry);
>  
>  	if (flags & ~(RENAME_NOREPLACE | RENAME_EXCHANGE))
> @@ -743,9 +763,7 @@ int simple_rename(struct mnt_idmap *idmap, struct inode *old_dir,
>  		inc_nlink(new_dir);
>  	}
>  
> -	old_dir->i_ctime = old_dir->i_mtime = new_dir->i_ctime =
> -		new_dir->i_mtime = inode->i_ctime = current_time(old_dir);
> -
> +	simple_rename_timestamp(old_dir, old_dentry, new_dir, new_dentry);
>  	return 0;
>  }
>  EXPORT_SYMBOL(simple_rename);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index bdfbd11a5811..14e38bd900f1 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2979,6 +2979,8 @@ extern int simple_open(struct inode *inode, struct file *file);
>  extern int simple_link(struct dentry *, struct inode *, struct dentry *);
>  extern int simple_unlink(struct inode *, struct dentry *);
>  extern int simple_rmdir(struct inode *, struct dentry *);
> +void simple_rename_timestamp(struct inode *old_dir, struct dentry *old_dentry,
> +			     struct inode *new_dir, struct dentry *new_dentry);
>  extern int simple_rename_exchange(struct inode *old_dir, struct dentry *old_dentry,
>  				  struct inode *new_dir, struct dentry *new_dentry);
>  extern int simple_rename(struct mnt_idmap *, struct inode *,

-- 
Damien Le Moal
Western Digital Research

Powered by blists - more mailing lists