Re: [PATCH v2 12/92] exfat: convert to simple_rename_timestamp

[Jeff Layton]

On Thu, 2023-07-06 at 12:39 +0200, Jan Kara wrote:
> On Wed 05-07-23 15:00:37, Jeff Layton wrote:
> > A rename potentially involves updating 4 different inode timestamps.
> > Convert to the new simple_rename_timestamp helper function.
> > 
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > ---
> >  fs/exfat/namei.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c
> > index d9b46fa36bff..e91022ff80ef 100644
> > --- a/fs/exfat/namei.c
> > +++ b/fs/exfat/namei.c
> > @@ -1312,8 +1312,8 @@ static int exfat_rename(struct mnt_idmap *idmap,
> >  		goto unlock;
> >  
> >  	inode_inc_iversion(new_dir);
> > -	new_dir->i_ctime = new_dir->i_mtime = new_dir->i_atime =
> > -		EXFAT_I(new_dir)->i_crtime = current_time(new_dir);
> > +	simple_rename_timestamp(old_dir, old_dentry, new_dir, new_dentry);
> > +	EXFAT_I(new_dir)->i_crtime = current_time(new_dir);
> 
> Hum, you loose atime update with this. Not that it would make sense to have
> it but it would probably deserve a comment in the changelog.
> 
> Also why you use current_time(new_dir) here instead of say inode->i_ctime?
> 

I think the atime update there is a mistake. A rename is not a "read"
operation. I'll note it in the changelog.

The i_crtime in exfat is the creation time (aka btime). I don't think it
matters much which source that ultimately comes from, but now I'm
wondering why it gets set here at all. Does exfat create a new inode
during a rename? If not, then the i_crtime updates here should probably
be removed.


> >  	exfat_truncate_atime(&new_dir->i_atime);
> >  	if (IS_DIRSYNC(new_dir))
> >  		exfat_sync_inode(new_dir);
> > @@ -1336,7 +1336,6 @@ static int exfat_rename(struct mnt_idmap *idmap,
> >  	}
> >  
> >  	inode_inc_iversion(old_dir);
> > -	old_dir->i_ctime = old_dir->i_mtime = current_time(old_dir);
> >  	if (IS_DIRSYNC(old_dir))
> >  		exfat_sync_inode(old_dir);
> >  	else
> 
> Also there is:
> 
>                 new_inode->i_ctime = EXFAT_I(new_inode)->i_crtime =
>                         current_time(new_inode);
> 
> in exfat_rename() from which you can remove the ctime update?
> 

Yeah, that should be removed too. I'll fix that in my tree. The i_crtime
update here looks pretty suspicious too, fwiw.

Thanks!
-- 
Jeff Layton <jlayton@xxxxxxxxxx>