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

[James Yonan <james@openvpn.net>]

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 the basic guarantees of rename still hold, i.e. that 
readers see an atomic transition from old to new files, or no transition 
(where mtime comparison results in -EEXIST return).  I understand that 
it doesn't guarantee i_mtime stability, but the application layer may 
not need that guarantee. In our case, mtime is immutable after local 
file creation and before do_renameat2() is used to move the file into place.

Re: RENAME_EXCHANGE_WITH_NEWER, that's an interesting idea.  You could 
actually implement it with minor changes in the patch, by simply 
combining RENAME_EXCHANGE|RENAME_NEWER.  Because fundamentally, all 
RENAME_NEWER does is compare mtimes and possibly return early with 
-EEXIST.  If the early return is not taken, then it becomes a plain 
rename or RENAME_EXCHANGE if that flag is also specified.

James