fs: add AT_REPLACE flag for linkat()

Patch 1 implements the VFS support for this flag. The implementation resembles sys_renameat2(), and I took care to preserve all of the original error cases and make the new error cases consistent with rename.

Quoted 4 lines

Patch 1 implements the VFS support for this flag. The implementation resembles sys_renameat2(), and I took care to preserve all of the original error cases and make the new error cases consistent with rename.

Shouldn't we try to reuse the rename code and the ->rename method instead of largely duplicating it?

In fact I wonder if a better interface would just use renameat(2) and accept the AT_EMPTY_PATH (for the source name only).

From: Omar Sandoval <[email protected]>

One of the most common uses of temporary files is the classic atomic replacement pattern, i.e.,

- write temporary file
- fsync temporary file
- rename temporary file over real file
- fsync parent directory

Now, we have O_TMPFILE, which gives us a much better way to create temporary files, but it's not possible to use it for this pattern.

This patch introduces an AT_REPLACE flag which allows linkat() to replace the target file. Now, the temporary file in the pattern above can be a proper O_TMPFILE. Even without O_TMPFILE, this is a new primitive which might be useful in other contexts.

The implementation on the VFS side mimics sys_renameat2().

Cc Xi Wang <[email protected]>

Signed-off-by Omar Sandoval <[email protected]>

---
 fs/ecryptfs/inode.c        |   2 +-
 fs/namei.c                 | 181 +++++++++++++++++++++++++++++--------
 fs/nfsd/vfs.c              |   2 +-
 fs/overlayfs/overlayfs.h   |   2 +-
 include/linux/fs.h         |   3 +-
 include/uapi/linux/fcntl.h |   1 +
 6 files changed, 150 insertions(+), 41 deletions(-)

fs/namei.c +4 -1

index 186bd2464fd5..2cc2b1deaa12 100644
@@ -4149,6 +4149,7 @@ SYSCALL_DEFINE2(symlink, const char __user *, oldname, const char __user *, newn
  * @dir:	new parent
  * @new_dentry:	where to create the new link
  * @delegated_inode: returns inode needing a delegation break
+ * @flags:      link flags
  *
  * The caller must hold dir->i_mutex
  *

@@ -4162,16 +4163,26 @@ SYSCALL_DEFINE2(symlink, const char __user *, oldname, const char __user *, newn
  * be appropriate for callers that expect the underlying filesystem not
  * to be NFS exported.
  */
-int vfs_link(struct dentry *old_dentry, struct inode *dir, struct dentry *new_dentry, struct inode **delegated_inode)
+int vfs_link(struct dentry *old_dentry, struct inode *dir,
+	     struct dentry *new_dentry, struct inode **delegated_inode,
+	     unsigned int flags)
 {
 	struct inode *inode = old_dentry->d_inode;

fs/namei.c +6 -0

+			error = may_delete(dir, new_dentry, d_is_dir(old_dentry));
+		else
+			error = -EEXIST;
+	} else {
+		error = may_create(dir, new_dentry);
+	}
 	if (error)
 		return error;
 

Quoted 7 lines · Christoph Hellwig

Quoted 4 lines

Patch 1 implements the VFS support for this flag. The implementation resembles sys_renameat2(), and I took care to preserve all of the original error cases and make the new error cases consistent with rename.

Shouldn't we try to reuse the rename code and the ->rename method instead of largely duplicating it?

As I mentioned in my cover letter, I did try, and it was uglier to reuse rename.

For O_TMPFILE, we can't pass a meaningful old_dir to ->rename(), so now every ->rename() supporting AT_EMPTY_PATH has to grow a bunch of special cases for ignoring old_dir (I looked at Btrfs, ext4, and XFS, and it'd be messy for all three). Conversely, ->link() just needs the extra unlink before link logic (I added ->link2() for this RFC, but for the actual patch I'll just add the flag to ->link()).

On the VFS side, in my opinion, there isn't that much duplication -- we have the may_delete()/is_local_mountpoint() a la unlink, delegation handling, and some specific error cases we need to handle similarly to rename, but that's not too suprising for a new primitive.

Quoted 2 lines

In fact I wonder if a better interface would just use renameat(2) and accept the AT_EMPTY_PATH (for the source name only).

renameat() makes sense for the O_TMPFILE case, but linkat() + AT_REPLACE also supports a normal, named source file, and renameat() + AT_EMPTY_PATH for that doesn't make sense at all (rename this file by file descriptor, but don't really rename it?).

From: Omar Sandoval <[email protected]>

Hi, Al,

This is a respin of my linkat() AT_REPLACE series, previously posted here:

https://patchwork.kernel.org/patch/9636735/
https://patchwork.kernel.org/patch/9636733/

There are no changes since v2, only a rebase onto v4.17-rc2 and some minor additions to the commit messages.

The goal is to allow for updating a file atomically in-place with an O_TMPFILE like so:

- open temporary file with O_TMPFILE
- write temporary file contents
- fsync temporary file
- atomically replace permanent location with the temporary file
- fsync parent directory

Patch 1 implements the VFS support for this flag. The implementation resembles sys_renameat2(), and I took care to preserve all of the original error cases and make the new error cases consistent with rename.

Previously, we discussed extending renameat2() instead of linkat() for this, but this makes a mess of rename and also forces us to special-case the parent directory for O_TMPFILEs in implementations of i_op->rename(), so I still think linkat() is a better fit.

Patch 2 adds support for AT_REPLACE to Btrfs. That's the codebase I'm most familiar with so that's where I started, but it should be straightforward to implement for other filesystems. v1 of this series had some incorrect dentry fiddling, so now I just unhash the replaced dentry for simplicity.

Please take a look.

Thanks!

Cc Xi Wang <[email protected]>

Omar Sandoval (2):
fs: add AT_REPLACE flag for linkat() which replaces the target
Btrfs: add support for linkat() AT_REPLACE

 fs/btrfs/inode.c           |  65 ++++++++++++-
 fs/ecryptfs/inode.c        |   2 +-
 fs/namei.c                 | 181 +++++++++++++++++++++++++++++--------
 fs/nfsd/vfs.c              |   2 +-
 fs/overlayfs/overlayfs.h   |   2 +-
 include/linux/fs.h         |   3 +-
 include/uapi/linux/fcntl.h |   1 +
 7 files changed, 211 insertions(+), 45 deletions(-)

Hello Omar,

Quoted 13 lines · Omar Sandoval

From: Omar Sandoval <[email protected]>

Hi, Al,

This is a respin of my linkat() AT_REPLACE series, previously posted here:

https://patchwork.kernel.org/patch/9636735/
https://patchwork.kernel.org/patch/9636733/

There are no changes since v2, only a rebase onto v4.17-rc2 and some minor additions to the commit messages.

Quoted 8 lines

The goal is to allow for updating a file atomically in-place with an O_TMPFILE like so:

- open temporary file with O_TMPFILE
- write temporary file contents
- fsync temporary file
- atomically replace permanent location with the temporary file
- fsync parent directory

Could we have an example program with the next revision please, perhaps as part of the commit message?

And, please also some text suitable for inclusion in the linkat(2) manual page.

Thanks,

Michael

From: Omar Sandoval <[email protected]>

Hi, Al,

This is a respin of my linkat() AT_REPLACE series, previously posted here:

https://patchwork.kernel.org/patch/9636735/
https://patchwork.kernel.org/patch/9636733/

There are no changes since v2, only a rebase onto v4.17-rc2 and some minor additions to the commit messages.

The goal is to allow for updating a file atomically in-place with an O_TMPFILE like so:

- open temporary file with O_TMPFILE
- write temporary file contents
- fsync temporary file
- atomically replace permanent location with the temporary file
- fsync parent directory

Patch 1 implements the VFS support for this flag. The implementation resembles sys_renameat2(), and I took care to preserve all of the original error cases and make the new error cases consistent with rename.

Previously, we discussed extending renameat2() instead of linkat() for this, but this makes a mess of rename and also forces us to special-case the parent directory for O_TMPFILEs in implementations of i_op->rename(), so I still think linkat() is a better fit.

Patch 2 adds support for AT_REPLACE to Btrfs. That's the codebase I'm most familiar with so that's where I started, but it should be straightforward to implement for other filesystems. v1 of this series had some incorrect dentry fiddling, so now I just unhash the replaced dentry for simplicity.

Please take a look.

Thanks!