fs: add AT_REPLACE flag for linkat()
Subsystems (7)
v2 cover fs: add AT_REPLACE flag for linkat()
From: Omar Sandoval <[email protected]>
The goal of this series 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
This series implements atomic replace step with a new linkat(..., AT_REPLACE).
Al, I took a look at implementing this usecase with renameat2() after we talked. Like you said, on the dcache side, "replace with tmpfile" looks a lot like rename. However, on the filesystem side, trying to use rename for this is much uglier than using link. We can't give i_op->rename() a meaningful old_dir, and dealing with that new special case gets messy fast.
So, here is the approach of having AT_REPLACE just unhash the replaced dentry.
Quoted 1 line
From the original cover letter:
This is a proof-of-concept patch series implementing an AT_REPLACE flag for linkat(2) which allows us to replace the target. This is a nice primitive on its own, but it's most interesting when combined with O_TMPFILE, as it allows you to do an atomic update of a file with an O_TMPFILE.
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.
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.
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(-)
Signature · 1 line
2.12.0
RFC PATCH v2 1/2 fs: add AT_REPLACE flag for linkat() which replaces the target
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.
--- 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/ecryptfs/inode.c +1 -1
@@ -442,7 +442,7 @@ static int ecryptfs_link(struct dentry *old_dentry, struct inode *dir,dget (lower_new_dentry); lower_dir_dentry =lock_parent (lower_new_dentry); rc =vfs_link (lower_old_dentry,d_inode (lower_dir_dentry), - lower_new_dentry,NULL ); + lower_new_dentry,NULL ,0 );if (rc ||d_really_is_negative (lower_new_dentry)) goto out_lock; rc =ecryptfs_interpose (lower_new_dentry, new_dentry, dir->i_sb);
fs/namei.c +144 -37
255 lines not on this page · raw message
fs/nfsd/vfs.c +1 -1
@@ -1589,7 +1589,7 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp, err = nfserr_noent;if (d_really_is_negative (dold)) goto out_dput; - host_err =vfs_link (dold, dirp, dnew,NULL ); + host_err =vfs_link (dold, dirp, dnew,NULL ,0 );if (!host_err) { err =nfserrno (commit_metadata (ffhp));if (!err)
fs/overlayfs/overlayfs.h +1 -1
@@ -40,7 +40,7 @@ static inline int ovl_do_unlink(struct inode *dir, struct dentry *dentry)static inline int ovl_do_link (struct dentry *old_dentry,struct inode *dir,struct dentry *new_dentry,bool debug) { -int err =vfs_link (old_dentry, dir, new_dentry,NULL ); +int err =vfs_link (old_dentry, dir, new_dentry,NULL ,0 );if (debug) {pr_debug ("link(%pd2, %pd2) = %i\n" , old_dentry, new_dentry, err);
include/linux/fs.h +2 -1
@@ -1561,7 +1561,7 @@ extern int vfs_create(struct inode *, struct dentry *, umode_t, bool);extern int vfs_mkdir (struct inode *,struct dentry *,umode_t );extern int vfs_mknod (struct inode *,struct dentry *,umode_t ,dev_t );extern int vfs_symlink (struct inode *,struct dentry *,const char *); -extern int vfs_link (struct dentry *,struct inode *,struct dentry *,struct inode **); +extern int vfs_link (struct dentry *,struct inode *,struct dentry *,struct inode **,unsigned int );extern int vfs_rmdir (struct inode *,struct dentry *);extern int vfs_unlink (struct inode *,struct dentry *,struct inode **);extern int vfs_rename (struct inode *,struct dentry *,struct inode *,struct dentry *,struct inode **,unsigned int );
@@ -1701,6 +1701,7 @@ struct inode_operations {int (*create) (struct inode *,struct dentry *,umode_t ,bool );int (*link) (struct dentry *,struct inode *,struct dentry *); +int (*link2) (struct dentry *,struct inode *,struct dentry *,unsigned int );int (*unlink) (struct inode *,struct dentry *);int (*symlink) (struct inode *,struct dentry *,const char *);int (*mkdir) (struct inode *,struct dentry *,umode_t );
include/uapi/linux/fcntl.h +1 -0
@@ -62,6 +62,7 @@#define AT_SYMLINK_FOLLOW 0x400/* Follow symbolic links. */ #define AT_NO_AUTOMOUNT 0x800/* Suppress terminal automount traversal */ #define AT_EMPTY_PATH 0x1000/* Allow empty relative pathname */ +#define AT_REPLACE 0x2000/* Replace new path */ #define AT_STATX_SYNC_TYPE 0x6000/* Type of synchronisation required from statx() */ #define AT_STATX_SYNC_AS_STAT 0x0000/* - Do whatever stat() does */
Signature · 1 line
2.12.0
Re: [RFC PATCH v2 1/2] fs: add AT_REPLACE flag for linkat() which replaces the target
Quoted 13 lines · Omar Sandoval
*/
fs/namei.c +4
-1-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; +struct inode *target = new_dentry->d_inode;unsigned max_links = dir->i_sb->s_max_links;int error;if (!inode)return -ENOENT ;
fs/namei.c SYSCALL_DEFINE2(symlink, const char __user *, oldname, const char __user *, newn · line 4155
Quoted 10 lines
fs/namei.c +8
-1- error =may_create (dir, new_dentry); +if (target) { +if (flags &AT_REPLACE ) + error =may_delete (dir, new_dentry,d_is_dir (old_dentry)); +else + error = -EEXIST ; + }else { + error =may_create (dir, new_dentry); + }
This looks bogus.
In particular, that "may_delete()" cannot be right. It should still *also* have the right to create something in that directory.
But even if you replace it with checks for both deletion _and_ creation, it won't be right, since the test for d_is_negative() on the target in may_delete() looks wrong for this situation.
Normally, you cannot delete a negative entry (think of what that would do in a overlay situation), but it should still be ok to link a positive entry on top of a negative one.
Also, I think the above is incorrect for yet another case: moving somethign on top of a directory should be disallowed. We do later on check that the *source* isn't a directory (since you can't link directories), but I'm not seeing where you'd be checking that you don't move over a directory either.
So I don't think this patch is acceptable as such. I also suspect that it should be done in multiple phases.
Linus
Re: [RFC PATCH v2 1/2] fs: add AT_REPLACE flag for linkat() which replaces the target
Quoted 15 lines · Linus Torvalds
Quoted 14 lines · Omar Sandoval
*/
fs/namei.c +4
-1-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; +struct inode *target = new_dentry->d_inode;unsigned max_links = dir->i_sb->s_max_links;int error;if (!inode)return -ENOENT ;
Quoted 9 lines
fs/namei.c SYSCALL_DEFINE2(symlink, const char __user *, oldname, const char __user *, newn · line 4155
Quoted 9 lines
fs/namei.c +8
-1- error =may_create (dir, new_dentry); +if (target) { +if (flags &AT_REPLACE ) + error =may_delete (dir, new_dentry,d_is_dir (old_dentry)); +else + error = -EEXIST ; + }else { + error =may_create (dir, new_dentry); + }
Hi, Linus,
Thanks for taking a look.
Quoted 5 lines
This looks bogus.
In particular, that "
may_delete()" cannot be right. It should still *also* have the right to create something in that directory.
Quoted 7 lines
But even if you replace it with checks for both deletion _and_ creation, it won't be right, since the test for
d_is_negative()on the target inmay_delete()looks wrong for this situation.Normally, you cannot delete a negative entry (think of what that would do in a overlay situation), but it should still be ok to link a positive entry on top of a negative one.
This is actually the exact same check we have in vfs_rename():
----
if (!target) {
error = may_create(new_dir, new_dentry);
} else {
new_is_dir = d_is_dir(new_dentry);
if (!(flags & RENAME_EXCHANGE))
error = may_delete(new_dir, new_dentry, is_dir);
else
error = may_delete(new_dir, new_dentry, new_is_dir);
}----
I tried to keep the same semantics as rename().
Quoted 5 lines
Also, I think the above is incorrect for yet another case: moving somethign on top of a directory should be disallowed. We do later on check that the *source* isn't a directory (since you can't link directories), but I'm not seeing where you'd be checking that you don't move over a directory either.
That check is happening in may_delete():
----
if (isdir) {
if (!d_is_dir(victim))
return -ENOTDIR;
if (IS_ROOT(victim))
return -EBUSY;
} else if (d_is_dir(victim))
return -EISDIR;----
Quoted 4 lines
So I don't think this patch is acceptable as such. I also suspect that it should be done in multiple phases.
Linus
Re: [RFC PATCH v2 1/2] fs: add AT_REPLACE flag for linkat() which replaces the target
Quoted 2 lines · Omar Sandoval
This is actually the exact same check we have in
vfs_rename():
Hmm. That looks bogus too exactly due to the overlayfs issues. Oh well. I guess people don't actually use overlayfs and care.
Quoted 1 line
I tried to keep the same semantics as
rename().
Ok, it does seem to match the non-exchange rename(), which I guess is what we want. It does worry me a bit that we only check for "may_delete()" even though we end up replacing it with a new one, but apparently those semantics indeed aren't new.
Linus
Miklos Szeredi
maintainerRe: [RFC PATCH v2 1/2] fs: add AT_REPLACE flag for linkat() which replaces the target
Quoted 6 lines · Linus Torvalds
Quoted 2 lines · Omar Sandoval
This is actually the exact same check we have in
vfs_rename():Hmm. That looks bogus too exactly due to the overlayfs issues. Oh well. I guess people don't actually use overlayfs and care.
What overlayfs issues? Overlayfs behaves exactly like a normal filesystem wrt negative dentries.
If you are referring to whiteouts, they are not visible on the overlay layer (remember overlay duplicates the dentry tree of the merged dentry trees of the underlying layers).
Quoted 6 lines
Quoted 1 line
I tried to keep the same semantics as
rename().Ok, it does seem to match the non-exchange
rename(), which I guess is what we want. It does worry me a bit that we only check for "may_delete()" even though we end up replacing it with a new one, but apparently those semantics indeed aren't new.
I think this is historic. may_delete() was doing the same as may_create() and more. Doesn't look like that's still strictly true, so it might make sense to add a may_replace() operation that does both.
Thanks,
Miklos
Re: [RFC PATCH v2 1/2] fs: add AT_REPLACE flag for linkat() which replaces the target
Quoted 3 lines · Miklos Szeredi
What overlayfs issues? Overlayfs behaves exactly like a normal filesystem wrt negative dentries.
Ignore me. I did indeed just confuse whiteouts with the normal negative dentries.
Quoted 4 lines
I think this is historic.
may_delete()was doing the same asmay_create()and more. Doesn't look like that's still strictly true, so it might make sense to add amay_replace()operation that does both.
They are definitely very different these days, with may_create() requiring a valid UID mapping into the filesystem, for example.
That may make no sense for the move/link case, of course (since the uid comes from the source file, not the current process), so it is possible that may_delete() ends up being effectively a superset of the may_create() things apart from that one thing, of course.
The one noticeable difference would seem to be the audit difference. Maybe nobody cares.
Linus
RFC PATCH v2 2/2 Btrfs: add support for linkat() AT_REPLACE
In reply to Omar SandovalFrom: Omar Sandoval <[email protected]>
The implementation is fairly straightforward and looks a lot like btrfs_rename().
--- fs/btrfs/inode.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 61 insertions(+), 4 deletions(-)
fs/btrfs/inode.c +61 -4
@@ -6468,16 +6468,21 @@ static int btrfs_create(struct inode *dir, struct dentry *dentry, }static int btrfs_link (struct dentry *old_dentry,struct inode *dir, -struct dentry *dentry) +struct dentry *dentry,unsigned int flags) {struct btrfs_trans_handle *trans =NULL ; +unsigned int trans_num_items;struct btrfs_root *root =BTRFS_I (dir)->root;struct inode *inode =d_inode (old_dentry); +struct inode *new_inode =d_inode (dentry);struct btrfs_fs_info *fs_info =btrfs_sb (inode->i_sb);u64 index;int err;int drop_inode =0 ; +if (flags & ~AT_REPLACE ) +return -EINVAL ; +/* do not allow sys_link's with other subvols of the same device */ if (root->objectid !=BTRFS_I (inode)->root->objectid)return -EXDEV ;
@@ -6485,16 +6490,47 @@ static int btrfs_link(struct dentry *old_dentry, struct inode *dir,if (inode->i_nlink >=BTRFS_LINK_MAX )return -EMLINK ; +/* check for collisions, even if the name isn't there */ + err =btrfs_check_dir_item_collision (root, dir->i_ino, + dentry->d_name.name, + dentry->d_name.len); +if (err) { +if (err == -EEXIST ) { +if (WARN_ON (!new_inode)) +return err; + }else { +return err; + } + } + +/* +* we're using link to replace one file with another. Start IO on it now +* so we don't add too much work to the end of the transaction +*/ +if (new_inode &&S_ISREG (inode->i_mode) && new_inode->i_size) +filemap_flush (inode->i_mapping); + err =btrfs_set_inode_index (BTRFS_I (dir), &index);if (err) goto fail;/* +* For the source: * 2 items for inode and inode ref * 2 items for dir items * 1 item for parent inode +* +* For the target: +* 1 for the possible orphan item +* 1 for the dir item +* 1 for the dir index +* 1 for the inode ref +* 1 for the inode */ - trans =btrfs_start_transaction (root,5 ); + trans_num_items =5 ; +if (new_inode) + trans_num_items +=5 ; + trans =btrfs_start_transaction (root, trans_num_items);if (IS_ERR (trans)) { err =PTR_ERR (trans); trans =NULL ;
@@ -6506,6 +6542,22 @@ static int btrfs_link(struct dentry *old_dentry, struct inode *dir,inc_nlink (inode);inode_inc_iversion (inode); inode->i_ctime =current_time (inode); + +if (new_inode) { +inode_inc_iversion (new_inode); + new_inode->i_ctime =current_time (new_inode); + err =btrfs_unlink_inode (trans, root,BTRFS_I (dir), +BTRFS_I (new_inode), + dentry->d_name.name, + dentry->d_name.len); +if (!err && new_inode->i_nlink ==0 ) + err =btrfs_orphan_add (trans,BTRFS_I (new_inode)); +if (err) { +btrfs_abort_transaction (trans, err); + goto fail; + } + } +ihold (inode);set_bit (BTRFS_INODE_COPY_EVERYTHING , &BTRFS_I (inode)->runtime_flags);
@@ -6528,7 +6580,12 @@ static int btrfs_link(struct dentry *old_dentry, struct inode *dir,if (err) goto fail; } -d_instantiate (dentry, inode); +if (new_inode) { +d_drop (dentry); +iput (inode); + }else { +d_instantiate (dentry, inode); + }btrfs_log_new_name (trans,BTRFS_I (inode),NULL , parent); }
@@ -10519,7 +10576,7 @@ static const struct inode_operations btrfs_dir_inode_operations = { .lookup = btrfs_lookup, .create = btrfs_create, .unlink = btrfs_unlink, - .link = btrfs_link, + .link2 = btrfs_link, .mkdir = btrfs_mkdir, .rmdir = btrfs_rmdir, .rename = btrfs_rename2,
Signature · 1 line
2.12.0