Message ID | 20230705185812.579118-3-jlayton@kernel.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:9f45:0:b0:3ea:f831:8777 with SMTP id v5csp2097652vqx; Wed, 5 Jul 2023 12:45:17 -0700 (PDT) X-Google-Smtp-Source: APBJJlFMMVgEetJhPckIDWTh8uxYtvib9tFA2yga/uyE5AxnB2B1xaUt0XWKQsYx7G3VjO5FxxWQ X-Received: by 2002:a17:90a:8b07:b0:263:1e6c:16f4 with SMTP id y7-20020a17090a8b0700b002631e6c16f4mr4587953pjn.20.1688586317222; Wed, 05 Jul 2023 12:45:17 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1688586317; cv=none; d=google.com; s=arc-20160816; b=YMpvRvs4wgzQGZyX0Flqw93rHNf+K6w8BG7EPDL9HNEtzn2r4lyr1wAHMLp5XYo4qQ zxSMh2WqUnKwAgVmx/SAb8NCcQYXbK6OeCmF/QEAAvsdzkLN2K+yWvTue1V1gDwJtG4/ aJnpjn5VtRcKwPSTfxQgErbXHMS+PVhzYcIe8Mrss0Lz1xZRaBG8iE+GuwqNZPbk9xVK 8sGs72hLvhMQGrNiNiVMIuvCXsgDaRAxkf83V+nVbnfx+VRDjlZt3kS++LJpYwyqcab2 uvhBUgPhRGYo38M4wBDuHquRgsHQdgroFwhQL7gBWt4IT315WMPbRq1bOJiALqmjHpUS cQeg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:to:from :dkim-signature; bh=ReqA6iULWNP01vdMNQO/9GaGuHnlFE8K/R95B3vy3dg=; fh=V5HIGG0FeiSUuroICa8j0SV2YaHNkm+sfptl9ssF/xM=; b=gTDp7rnlVL5fnxK5HxNiKcOnx6Sf45posFTFU8Q0v5SHvBStj3vxp4eWoePFEPT/h+ Hs+80GtuZbx7ueJpUTezhTQu8hEIyaPQxHPT5h4OnbXG4+Kl7rdd/LmVDEcQGFV0+DhI F3mbfOSTwvEe4bJD1rQduqgKAoLoYq3KHIssVIRAoEX25wq5iy6nkmdtYbGF4pjsdtQf kfQrGMNEhqoKURm7urQ1+sdJi20Qhyvb8MdhrrrNQDqS8IEF2C85ygUnSYDpMxxhBC7S Ve1c2OjFgPgBTWZLNjPvgyEdAvVp1yQmDnrHxzV1wIIXofgsSAVD25XDO1XgT3zj1ISz OBpw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b="SV/pROPi"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id mm8-20020a17090b358800b00259b2afc651si2175977pjb.62.2023.07.05.12.45.02; Wed, 05 Jul 2023 12:45:17 -0700 (PDT) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b="SV/pROPi"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233335AbjGES72 (ORCPT <rfc822;tebrre53rla2o@gmail.com> + 99 others); Wed, 5 Jul 2023 14:59:28 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:38990 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233179AbjGES7W (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 5 Jul 2023 14:59:22 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5080F199E; Wed, 5 Jul 2023 11:59:10 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 9BBA9616D8; Wed, 5 Jul 2023 18:59:09 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id EEEF1C4167D; Wed, 5 Jul 2023 18:58:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1688583549; bh=96h56beXLMAvkoYPxw1Hf+1T9dWrr7543/fVCrKiiH8=; h=From:To:Subject:Date:In-Reply-To:References:From; b=SV/pROPiikvWgbuJG9o5j/mdUiyRGcRaEJl1K0hVl0eCHK9Ob+YmgZ0cOVDNwxcd0 s3XXK3FiPaVML6VBfbeTny5HiSuKA0KEKAjFDlBtEkXvpck7V4KUAdjnzKCg5fiugt aiOuDDRaoB7EF/ycBMYU5USexpq/ceGXPIb8HpDBW4vIvyY547qmZyivJt5T6PXA4H rtd7d3FTcpVX0DGPx74bauINVbOI0AHI9Zym6FoqdyCXdEJPTgmmD/blFQIJoOkcln fC0uEV1EXwoGgwuuPieKxDXph4tQwZUYbTrLEd3zCpi9KdekgoBzBb9kt9un4ZFr7M 0DXS4Kyxlm/3g== From: Jeff Layton <jlayton@kernel.org> To: jk@ozlabs.org, arnd@arndb.de, mpe@ellerman.id.au, npiggin@gmail.com, christophe.leroy@csgroup.eu, hca@linux.ibm.com, gor@linux.ibm.com, agordeev@linux.ibm.com, borntraeger@linux.ibm.com, svens@linux.ibm.com, gregkh@linuxfoundation.org, arve@android.com, tkjos@android.com, maco@android.com, joel@joelfernandes.org, brauner@kernel.org, cmllamas@google.com, surenb@google.com, dennis.dalessandro@cornelisnetworks.com, jgg@ziepe.ca, leon@kernel.org, bwarrum@linux.ibm.com, rituagar@linux.ibm.com, ericvh@kernel.org, lucho@ionkov.net, asmadeus@codewreck.org, linux_oss@crudebyte.com, dsterba@suse.com, dhowells@redhat.com, marc.dionne@auristor.com, viro@zeniv.linux.org.uk, raven@themaw.net, luisbg@kernel.org, salah.triki@gmail.com, aivazian.tigran@gmail.com, ebiederm@xmission.com, keescook@chromium.org, clm@fb.com, josef@toxicpanda.com, xiubli@redhat.com, idryomov@gmail.com, jlayton@kernel.org, jaharkes@cs.cmu.edu, coda@cs.cmu.edu, jlbec@evilplan.org, hch@lst.de, nico@fluxnic.net, rafael@kernel.org, code@tyhicks.com, ardb@kernel.org, xiang@kernel.org, chao@kernel.org, huyue2@coolpad.com, jefflexu@linux.alibaba.com, linkinjeon@kernel.org, sj1557.seo@samsung.com, jack@suse.com, tytso@mit.edu, adilger.kernel@dilger.ca, jaegeuk@kernel.org, hirofumi@mail.parknet.co.jp, miklos@szeredi.hu, rpeterso@redhat.com, agruenba@redhat.com, richard@nod.at, anton.ivanov@cambridgegreys.com, johannes@sipsolutions.net, mikulas@artax.karlin.mff.cuni.cz, mike.kravetz@oracle.com, muchun.song@linux.dev, dwmw2@infradead.org, shaggy@kernel.org, tj@kernel.org, trond.myklebust@hammerspace.com, anna@kernel.org, chuck.lever@oracle.com, neilb@suse.de, kolga@netapp.com, Dai.Ngo@oracle.com, tom@talpey.com, konishi.ryusuke@gmail.com, anton@tuxera.com, almaz.alexandrovich@paragon-software.com, mark@fasheh.com, joseph.qi@linux.alibaba.com, me@bobcopeland.com, hubcap@omnibond.com, martin@omnibond.com, amir73il@gmail.com, mcgrof@kernel.org, yzaikin@google.com, tony.luck@intel.com, gpiccoli@igalia.com, al@alarsen.net, sfrench@samba.org, pc@manguebit.com, lsahlber@redhat.com, sprasad@microsoft.com, senozhatsky@chromium.org, phillip@squashfs.org.uk, rostedt@goodmis.org, mhiramat@kernel.org, dushistov@mail.ru, hdegoede@redhat.com, djwong@kernel.org, dlemoal@kernel.org, naohiro.aota@wdc.com, jth@kernel.org, ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org, martin.lau@linux.dev, song@kernel.org, yhs@fb.com, john.fastabend@gmail.com, kpsingh@kernel.org, sdf@google.com, haoluo@google.com, jolsa@kernel.org, hughd@google.com, akpm@linux-foundation.org, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, john.johansen@canonical.com, paul@paul-moore.com, jmorris@namei.org, serge@hallyn.com, stephen.smalley.work@gmail.com, eparis@parisplace.org, jgross@suse.com, stern@rowland.harvard.edu, lrh2000@pku.edu.cn, sebastian.reichel@collabora.com, wsa+renesas@sang-engineering.com, quic_ugoswami@quicinc.com, quic_linyyuan@quicinc.com, john@keeping.me.uk, error27@gmail.com, quic_uaggarwa@quicinc.com, hayama@lineo.co.jp, jomajm@gmail.com, axboe@kernel.dk, dhavale@google.com, dchinner@redhat.com, hannes@cmpxchg.org, zhangpeng362@huawei.com, slava@dubeyko.com, gargaditya08@live.com, penguin-kernel@I-love.SAKURA.ne.jp, yifeliu@cs.stonybrook.edu, madkar@cs.stonybrook.edu, ezk@cs.stonybrook.edu, yuzhe@nfschina.com, willy@infradead.org, okanatov@gmail.com, jeffxu@chromium.org, linux@treblig.org, mirimmad17@gmail.com, yijiangshan@kylinos.cn, yang.yang29@zte.com.cn, xu.xin16@zte.com.cn, chengzhihao1@huawei.com, shr@devkernel.io, Liam.Howlett@Oracle.com, adobriyan@gmail.com, chi.minghao@zte.com.cn, roberto.sassu@huawei.com, linuszeng@tencent.com, bvanassche@acm.org, zohar@linux.ibm.com, yi.zhang@huawei.com, trix@redhat.com, fmdefrancesco@gmail.com, ebiggers@google.com, princekumarmaurya06@gmail.com, chenzhongjin@huawei.com, riel@surriel.com, shaozhengchao@huawei.com, jingyuwang_vip@163.com, linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org, linux-s390@vger.kernel.org, linux-rdma@vger.kernel.org, linux-usb@vger.kernel.org, v9fs@lists.linux.dev, linux-fsdevel@vger.kernel.org, linux-afs@lists.infradead.org, autofs@vger.kernel.org, linux-mm@kvack.org, linux-btrfs@vger.kernel.org, ceph-devel@vger.kernel.org, codalist@coda.cs.cmu.edu, ecryptfs@vger.kernel.org, linux-efi@vger.kernel.org, linux-erofs@lists.ozlabs.org, linux-ext4@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, cluster-devel@redhat.com, linux-um@lists.infradead.org, linux-mtd@lists.infradead.org, jfs-discussion@lists.sourceforge.net, linux-nfs@vger.kernel.org, linux-nilfs@vger.kernel.org, linux-ntfs-dev@lists.sourceforge.net, ntfs3@lists.linux.dev, ocfs2-devel@lists.linux.dev, linux-karma-devel@lists.sourceforge.net, devel@lists.orangefs.org, linux-unionfs@vger.kernel.org, linux-hardening@vger.kernel.org, reiserfs-devel@vger.kernel.org, linux-cifs@vger.kernel.org, samba-technical@lists.samba.org, linux-trace-kernel@vger.kernel.org, linux-xfs@vger.kernel.org, bpf@vger.kernel.org, netdev@vger.kernel.org, apparmor@lists.ubuntu.com, linux-security-module@vger.kernel.org, selinux@vger.kernel.org Subject: [PATCH v2 08/92] fs: new helper: simple_rename_timestamp Date: Wed, 5 Jul 2023 14:58:11 -0400 Message-ID: <20230705185812.579118-3-jlayton@kernel.org> X-Mailer: git-send-email 2.41.0 In-Reply-To: <20230705185812.579118-1-jlayton@kernel.org> References: <20230705185812.579118-1-jlayton@kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1770611085815140284?= X-GMAIL-MSGID: =?utf-8?q?1770611085815140284?= |
Series |
fs: new accessors for inode->i_ctime
|
|
Commit Message
Jeff Layton
July 5, 2023, 6:58 p.m. UTC
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@kernel.org>
---
fs/libfs.c | 36 +++++++++++++++++++++++++++---------
include/linux/fs.h | 2 ++
2 files changed, 29 insertions(+), 9 deletions(-)
Comments
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@kernel.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 *,
On Thu, 2023-07-06 at 08:19 +0900, Damien Le Moal wrote: > 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@kernel.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. > I don't believe it's an issue. I've seen nothing in the POSIX spec that mandates that timestamp updates to different inodes involved in an operation be set to the _same_ value. It just says they must be updated. It's also hard to believe that any software would depend on this either, given that it's very inconsistent across filesystems today. AFAICT, this was mostly done in the past just as a matter of convenience. The other problem with doing it that way is that it assumes that current_time(inode) should always return the same value when given different inodes. Is it really correct to do this? inode_set_ctime(dir, inode_set_ctime_current(inode)); "dir" and "inode" are different inodes, after all, and you're setting dir's timestamp to "inode"'s value. It's not a big deal today since they're always on the same sb, but the ultimate goal of these changes is to implement multigrain timestamps. That will mean that fetching a fine- grained timestamp for an update when the existing mtime or ctime value has been queried via getattr. With that change, I think it's best that we treat updates to different inodes individually, as some of them may require updating with a fine- grained timestamp and some may not. > > 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 *, >
On Wed 05-07-23 14:58:11, 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@kernel.org> Looks good to me. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > 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); > 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 *, > -- > 2.41.0 >
On Wed, Jul 05, 2023 at 08:04:41PM -0400, Jeff Layton wrote: > > I don't believe it's an issue. I've seen nothing in the POSIX spec that > mandates that timestamp updates to different inodes involved in an > operation be set to the _same_ value. It just says they must be updated. > > It's also hard to believe that any software would depend on this either, > given that it's very inconsistent across filesystems today. AFAICT, this > was mostly done in the past just as a matter of convenience. I've seen this assumption in several programs: mutt buffy.c https://sources.debian.org/src/mutt/2.2.9-1/buffy.c/?hl=625#L625 if (mailbox->newly_created && (sb->st_ctime != sb->st_mtime || sb->st_ctime != sb->st_atime)) mailbox->newly_created = 0; neomutt mbox/mbox.c https://sources.debian.org/src/neomutt/20220429+dfsg1-4.1/mbox/mbox.c/?hl=1820#L1820 if (m->newly_created && ((st.st_ctime != st.st_mtime) || (st.st_ctime != st.st_atime))) m->newly_created = false; screen logfile.c https://sources.debian.org/src/screen/4.9.0-4/logfile.c/?hl=130#L130 if ((!s->st_dev && !s->st_ino) || /* stat failed, that's new! */ !s->st_nlink || /* red alert: file unlinked */ (s->st_size < o.st_size) || /* file truncated */ (s->st_mtime != o.st_mtime) || /* file modified */ ((s->st_ctime != o.st_ctime) && /* file changed (moved) */ !(s->st_mtime == s->st_ctime && /* and it was not a change */ o.st_ctime < s->st_ctime))) /* due to delayed nfs write */ { nemo libnemo-private/nemo-vfs-file.c https://sources.debian.org/src/nemo/5.6.5-1/libnemo-private/nemo-vfs-file.c/?hl=344#L344 /* mtime is when the contents changed; ctime is when the * contents or the permissions (inc. owner/group) changed. * So we can only know when the permissions changed if mtime * and ctime are different. */ if (file->details->mtime == file->details->ctime) { return FALSE; } While looking for more examples, I found a perl test that seems to suggest that at least Solaris, AFS, AmigaOS, DragonFly BSD do as you suggest: https://sources.debian.org/src/perl/5.36.0-7/t/op/stat.t/?hl=158#L140 Thanks
On Thu, 2023-07-06 at 21:02 +0000, Seth Arnold wrote: > On Wed, Jul 05, 2023 at 08:04:41PM -0400, Jeff Layton wrote: > > > > I don't believe it's an issue. I've seen nothing in the POSIX spec that > > mandates that timestamp updates to different inodes involved in an > > operation be set to the _same_ value. It just says they must be updated. > > > > It's also hard to believe that any software would depend on this either, > > given that it's very inconsistent across filesystems today. AFAICT, this > > was mostly done in the past just as a matter of convenience. > > I've seen this assumption in several programs: > Thanks for looking into this! To be clear, POSIX doesn't require that _different_ inodes ever be set to the same timestamp value. IOW, it certainly doesn't require that the source and target directories on a rename() end up with the exact same timestamp value. Granted, POSIX is rather vague on timestamps in general, but most of the examples below involve comparing different timestamps on the _same_ inode. > mutt buffy.c > https://sources.debian.org/src/mutt/2.2.9-1/buffy.c/?hl=625#L625 > > if (mailbox->newly_created && > (sb->st_ctime != sb->st_mtime || sb->st_ctime != sb->st_atime)) > mailbox->newly_created = 0; > This should be fine with this patchset. Note that this is comparing a/c/mtime on the same inode, and our usual pattern on inode instantiation is: inode->i_atime = inode->i_mtime = inode_set_ctime_current(inode); ...which should result in all of inode's timestamps being synchronized. > > neomutt mbox/mbox.c > https://sources.debian.org/src/neomutt/20220429+dfsg1-4.1/mbox/mbox.c/?hl=1820#L1820 > > if (m->newly_created && ((st.st_ctime != st.st_mtime) || (st.st_ctime != st.st_atime))) > m->newly_created = false; > Ditto here. > > screen logfile.c > https://sources.debian.org/src/screen/4.9.0-4/logfile.c/?hl=130#L130 > > if ((!s->st_dev && !s->st_ino) || /* stat failed, that's new! */ > !s->st_nlink || /* red alert: file unlinked */ > (s->st_size < o.st_size) || /* file truncated */ > (s->st_mtime != o.st_mtime) || /* file modified */ > ((s->st_ctime != o.st_ctime) && /* file changed (moved) */ > !(s->st_mtime == s->st_ctime && /* and it was not a change */ > o.st_ctime < s->st_ctime))) /* due to delayed nfs write */ > { > This one is really weird. You have two different struct stat's, "o" and "s". I assume though that these should be stat values from the same inode, because otherwise this comparison would make no sense: ((s->st_ctime != o.st_ctime) && /* file changed (moved) */ In general, we can never contrive to ensure that the ctime of two different inodes are the same, since that is always set by the kernel to the current time, and you'd have to ensure that they were created within the same jiffy (at least with today's code). > nemo libnemo-private/nemo-vfs-file.c > https://sources.debian.org/src/nemo/5.6.5-1/libnemo-private/nemo-vfs-file.c/?hl=344#L344 > > /* mtime is when the contents changed; ctime is when the > * contents or the permissions (inc. owner/group) changed. > * So we can only know when the permissions changed if mtime > * and ctime are different. > */ > if (file->details->mtime == file->details->ctime) { > return FALSE; > } > Ditto here with the first examples. This involves comparing timestamps on the same inode, which should be fine. > > While looking for more examples, I found a perl test that seems to suggest > that at least Solaris, AFS, AmigaOS, DragonFly BSD do as you suggest: > https://sources.debian.org/src/perl/5.36.0-7/t/op/stat.t/?hl=158#L140 > (I kinda miss Perl. I wrote a bunch of stuff in it in the 90's and early naughties) I think this test is supposed to be testing whether the mtime changes on link() ? -----------------8<---------------- my($nlink, $mtime, $ctime) = (stat($tmpfile))[$NLINK, $MTIME, $CTIME]; [...] skip "Solaris tmpfs has different mtime/ctime link semantics", 2 if $Is_Solaris and $cwd =~ m#^/tmp# and $mtime && $mtime == $ctime; -----------------8<---------------- ...again, I think this would be ok too since it's just comparing the mtime and ctime of the same inode. Granted this is a Solaris-specific test, but Linux would be fine here too. So in conclusion, I don't think this patchset will cause problems with any of the above code.
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); 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 *,