On Tue, Jun 28, 2022 at 05:19:12PM -0600, James Yonan wrote: > On 6/28/22 12:34, Amir Goldstein wrote: > > On Tue, Jun 28, 2022 at 8:44 PM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote: > > > On Mon, Jun 27, 2022 at 04:11:07PM -0600, James Yonan wrote: > > > > > > > && d_is_positive(new_dentry) > > > > && timespec64_compare(&d_backing_inode(old_dentry)->i_mtime, > > > > &d_backing_inode(new_dentry)->i_mtime) <= 0) > > > > goto exit5; > > > > > > > > It's pretty cool in a way that a new atomic file operation can even be > > > > implemented in just 5 lines of code, and it's thanks to the existing > > > > locking infrastructure around file rename/move that these operations > > > > become almost trivial. Unfortunately, every fs must approve a new > > > > renameat2() flag, so it bloats the patch a bit. > > > How is it atomic and what's to stabilize ->i_mtime in that test? > > > Confused... > > Good point. > > RENAME_EXCHANGE_WITH_NEWER would have been better > > in that regard. > > > > And you'd have to check in vfs_rename() after lock_two_nondirectories() > > So I mean atomic in the sense that you are comparing the old and new mtimes > inside the lock_rename/unlock_rename critical section in do_renameat2(), so mtime is not stable during rename, even with the inode locked. e.g. a write page fault occurring concurrently with rename will change mtime, and so which inode is "newer" can change during the rename syscall... Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
