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