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

["NeilBrown" <neilb@suse.de>]

On Wed, 29 Jun 2022, Dave Chinner wrote:
> 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 would document this by saying "The rename will fail if either file is open
for write access as in that case the mtimes cannot be considered to be
stable".

The app would respond as it would to any other unexpected error.

The expectation, as I understand it, is that this is used in situations
when the target file is only changed by a rename.  Someone else opening
the file for O_WRITE would be just as bad as someone else unlinking the
source file so renameat2 gets ENOENT.  In both cases, the new
renameat2() cannot do anything useful, so it shouldn't try.  Equally in
both cases the application cannot do anything useful.

NeilBrown


> 
> 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
>