Re: [PATCH] namei: implemented RENAME_NEWER flag for renameat2() conditional replace

[Dave Chinner <david@fromorbit.com>]

On Wed, Jun 29, 2022 at 12:07:04PM +1000, NeilBrown wrote:
> On Wed, 29 Jun 2022, Dave Chinner wrote:
> > 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@zeniv.linux.org.uk> 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...
> 
> I don't think that is really important for the proposed use case.

Sure, but that's not the point. How do you explain it the API
semantics to an app developer that might want to use this
functionality? RENAME_EXCHANGE_WITH_NEWER would be atomic in the
sense you either get the old or new file at the destination, but
it's not atomic in the sense that it is serialised against all other
potential modification operations against either the source or
destination. Hence the "if newer" comparison is not part of the
"atomic rename" operation that is supposedly being performed...

I'm also sceptical of the use of mtime - we can't rely on mtime to
determine the newer file accurately on all filesystems. e.g. Some
fileystems only have second granularity in their timestamps, so
there's a big window where "newer" cannot actually be determined by
timestamp comparisons.

/me is having flashbacks to the bad old days of NFS using inode
timestamps for change ordering and cache consistency....

> In any case where you might be using this new rename flag, the target
> file wouldn't be open for write, so the mtime wouldn't change.
> The atomicity is really wanted to make sure the file at the destination
> name is still the one that was expected (I think).

How would you document this, and how would the application be
expected to handle such a "someone else has this open for write"
error? There's nothing the app can do about the cause of the
failure, so how is it expected to handle such an error?

I'm not opposed to adding functionality like this, I'm just pointing
out problems that I see arising from the insufficiently
constrained/specified behaviour of the proposed functionality.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com