fs: add AT_REPLACE flag for linkat()
Subsystems (7)
cover fs: add AT_REPLACE flag for linkat()
From: Omar Sandoval <[email protected]>
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 a dcache helper for filesystem implementations of AT_REPLACE. I'm not entirely convinced that it's 100% correct.
Patch 3 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.
Any comments are welcome.
Omar Sandoval (3):
fs: add AT_REPLACE flag for linkat() which replaces the target
vfs: add d_replace()
Btrfs: add support for linkat() AT_REPLACE
fs/btrfs/inode.c | 59 ++++++++++++++- fs/dcache.c | 68 +++++++++++++++-- fs/ecryptfs/inode.c | 2 +- fs/namei.c | 180 +++++++++++++++++++++++++++++++++++---------- fs/nfsd/vfs.c | 2 +- fs/overlayfs/overlayfs.h | 2 +- include/linux/dcache.h | 1 + include/linux/fs.h | 3 +- include/uapi/linux/fcntl.h | 1 + 9 files changed, 267 insertions(+), 51 deletions(-)
Signature · 1 line
2.10.2
RFC PATCH 1/3 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 | 180 +++++++++++++++++++++++++++++++++++---------- fs/nfsd/vfs.c | 2 +- fs/overlayfs/overlayfs.h | 2 +- include/linux/fs.h | 3 +- include/uapi/linux/fcntl.h | 1 + 6 files changed, 149 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 +143 -37
248 lines not on this page · raw message
fs/nfsd/vfs.c +1 -1
@@ -1594,7 +1594,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
@@ -46,7 +46,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
@@ -1606,7 +1606,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 );
@@ -1743,6 +1743,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 */ #endif /* _UAPI_LINUX_FCNTL_H */
Signature · 1 line
2.10.2
Re: [RFC PATCH 1/3] fs: add AT_REPLACE flag for linkat() which replaces the target
Quoted 5 lines · Omar Sandoval
From: Omar Sandoval <[email protected]>
One of the most common uses of temporary files is the classic atomic replacement pattern, i.e.,
Quoted 7 lines
- write temporary file
- fsync temporary file
- rename temporary file over real file
- fsync parent directoryNow, 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.
FWIW, I have some userspace code that indeed wants exactly this:
https://github.com/GNOME/libglnx/blob/36396b49ad6636c9959f3dfac5e04d41584b1a92/glnx-fdio.c#L232
So if we add this, I'd presumably change the code to try it and fall back on EINVAL as usual?
RFC PATCH 2/3 vfs: add d_replace()
In reply to Omar SandovalFrom: Omar Sandoval <[email protected]>
Changes the inode associated with a dentry. This'll be useful for implementations of linkat() AT_REPLACE.
--- fs/dcache.c | 68 +++++++++++++++++++++++++++++++++++++++++++++----- include/linux/dcache.h | 1 + 2 files changed, 63 insertions(+), 6 deletions(-)
fs/dcache.c +62 -6
@@ -316,6 +316,16 @@ static void dentry_free(struct dentry *dentry)call_rcu (&dentry->d_u.d_rcu, __d_free); } +static void dentry_iput (struct dentry *dentry,struct inode *inode) +{ +if (!inode->i_nlink) +fsnotify_inoderemove (inode); +if (dentry->d_op && dentry->d_op->d_iput) + dentry->d_op->d_iput (dentry, inode); +else +iput (inode); +} +/* * Release the dentry's inode, using the filesystem * d_iput() operation if defined.
@@ -335,12 +345,7 @@ static void dentry_unlink_inode(struct dentry * dentry)raw_write_seqcount_end (&dentry->d_seq);spin_unlock (&dentry->d_lock);spin_unlock (&inode->i_lock); -if (!inode->i_nlink) -fsnotify_inoderemove (inode); -if (dentry->d_op && dentry->d_op->d_iput) - dentry->d_op->d_iput (dentry, inode); -else -iput (inode); +dentry_iput (dentry, inode); }/*
@@ -1816,6 +1821,24 @@ void d_instantiate(struct dentry *entry, struct inode * inode) }EXPORT_SYMBOL (d_instantiate); +static void lock_two_inodes (struct inode *inode1,struct inode *inode2) +{ +if (inode1 > inode2) +swap (inode1, inode2); +if (inode1) +spin_lock (&inode1->i_lock); +if (inode2) +spin_lock (&inode2->i_lock); +} + +static void unlock_two_inodes (struct inode *inode1,struct inode *inode2) +{ +if (inode1) +spin_unlock (&inode1->i_lock); +if (inode2) +spin_unlock (&inode2->i_lock); +} +/** * d_instantiate_no_diralias - instantiate a non-aliased dentry * @entry: dentry to complete
@@ -2339,6 +2362,39 @@ void d_delete(struct dentry * dentry) }EXPORT_SYMBOL (d_delete); +/** +* d_replace - change the inode a dentry is associated with +* @dentry: dentry to modify +* @inode: inode to attach to this dentry +* +* Fill in new inode information in a dentry that may have previously been +* instantiated. This handles both negative and positive dentries. +*/ +void d_replace (struct dentry *dentry,struct inode *inode) +{ +struct inode *old_inode = dentry->d_inode; +unsigned int add_flags; + +lock_two_inodes (old_inode, inode); +spin_lock (&dentry->d_lock); + add_flags =d_flags_for_inode (inode); + +if (old_inode) +hlist_del (&dentry->d_u.d_alias); +hlist_add_head (&dentry->d_u.d_alias, &inode->i_dentry); + +raw_write_seqcount_begin (&dentry->d_seq); +__d_set_inode_and_type (dentry, inode, add_flags); +raw_write_seqcount_end (&dentry->d_seq); +fsnotify_update_flags (dentry); + +spin_unlock (&dentry->d_lock); +unlock_two_inodes (old_inode, inode); +if (old_inode) +dentry_iput (dentry, old_inode); +} +EXPORT_SYMBOL (d_replace); +static void __d_rehash (struct dentry *entry) {struct hlist_bl_head *b =d_hash (entry->d_name.hash);
include/linux/dcache.h +1 -0
@@ -224,6 +224,7 @@ extern int d_instantiate_no_diralias(struct dentry *, struct inode *);extern void __d_drop (struct dentry *dentry);extern void d_drop (struct dentry *dentry);extern void d_delete (struct dentry *); +extern void d_replace (struct dentry *,struct inode *);extern void d_set_d_op (struct dentry *dentry,const struct dentry_operations *op);/* allocate/de-allocate */
Signature · 1 line
2.10.2
Al Viro
maintainerRe: [RFC PATCH 2/3] vfs: add d_replace()
Quoted 4 lines · Omar Sandoval
From: Omar Sandoval <[email protected]>
Changes the inode associated with a dentry. This'll be useful for implementations of
linkat()AT_REPLACE.
Hard NAK. That violates all kinds of assumptions made by VFS and filesystems alike; never, ever do that. If you have a reference to a positive dentry, inode should *NEVER* change.
If it unhashed the old dentry, created a new one and attached inode to it, it _might_ have a chance. I'm less than sure it's a good idea, but it this form it's a non-starter.
Again,
and don't bring it back in that form.
Re: [RFC PATCH 2/3] vfs: add d_replace()
Quoted 9 lines · Al Viro
Quoted 4 lines · Omar Sandoval
From: Omar Sandoval <[email protected]>
Changes the inode associated with a dentry. This'll be useful for implementations of
linkat()AT_REPLACE.Hard NAK. That violates all kinds of assumptions made by VFS and filesystems alike; never, ever do that. If you have a reference to a positive dentry, inode should *NEVER* change.
Okay, thanks for the sanity check.
Quoted 3 lines
If it unhashed the old dentry, created a new one and attached inode to it, it _might_ have a chance. I'm less than sure it's a good idea, but it this form it's a non-starter.
One thing I considered was having the filesystem unhash the dentry and just letting the next lookup that comes along instantiate the new one. Is that better or worse than doing something like your suggestion?
Quoted 5 lines
Again,
and don't bring it back in that form.
Signature · 1 line
Omar
Re: [RFC PATCH 2/3] vfs: add d_replace()
Quoted 9 lines · Omar Sandoval
Quoted 3 lines · Al Viro
If it unhashed the old dentry, created a new one and attached inode to it, it _might_ have a chance. I'm less than sure it's a good idea, but it this form it's a non-starter.
One thing I considered was having the filesystem unhash the dentry and just letting the next lookup that comes along instantiate the new one. Is that better or worse than doing something like your suggestion?
I don't have the background for why you want this, but the two approaches should be equivalent.
However, it's not a safe operation in the general case, since the low-level filesystem may depend on the single unique dentry meaning that operations on one particular filename are serialized and that the dentry is unique, and your "unhash and create new" would leave old users with a stale dentry that is no longer "unique" in that filename. So you certainly cannot do even that kind of "d_replace()" in some general situation.
An example of that kind of situation is the whole "d_in_lookup()" where we use the dentry itself to guarantee uniqueness while possibly looking up multiple entries in parallell in the same directory.
So for some particular filesystem, under some very particular situations, such a d_replace() may be valid. But without seeing the background, it's hard to tell. Apparently this was discussed on the fsdevel list that google doesn't even index, and looking at the fsdevel archives is a pain. Looks like it's AT_REPLACE for linkat().
In that context it superficially looks ok to me (ie it's filesystem-controlled and done only when we've serialized the directory for the link() operation anyway). But I didn't think about it _that_ much.
Linus
Re: [RFC PATCH 2/3] vfs: add d_replace()
Quoted 22 lines · Linus Torvalds
Quoted 9 lines · Omar Sandoval
Quoted 3 lines · Al Viro
If it unhashed the old dentry, created a new one and attached inode to it, it _might_ have a chance. I'm less than sure it's a good idea, but it this form it's a non-starter.
One thing I considered was having the filesystem unhash the dentry and just letting the next lookup that comes along instantiate the new one. Is that better or worse than doing something like your suggestion?
I don't have the background for why you want this, but the two approaches should be equivalent.
However, it's not a safe operation in the general case, since the low-level filesystem may depend on the single unique dentry meaning that operations on one particular filename are serialized and that the dentry is unique, and your "unhash and create new" would leave old users with a stale dentry that is no longer "unique" in that filename. So you certainly cannot do even that kind of "
d_replace()" in some general situation.
Quoted 3 lines
An example of that kind of situation is the whole "
d_in_lookup()" where we use the dentry itself to guarantee uniqueness while possibly looking up multiple entries in parallell in the same directory.
Thanks, this was helpful. As you mentioned below, since this is for linkat(), we're serialized on i_rwsem, so this particular case should be fine. But if there's something that tries to serialize on the dentry itself without holding i_rwsem at least shared, then this would definitely be wrong. Does such a thing exist?
Quoted 5 lines
So for some particular filesystem, under some very particular situations, such a
d_replace()may be valid. But without seeing the background, it's hard to tell. Apparently this was discussed on the fsdevel list that google doesn't even index, and looking at the fsdevel archives is a pain. Looks like it'sAT_REPLACEforlinkat().
That's right, here's the thread:
[RFC PATCH 0/3] http://marc.info/?t=147980325700004&r=1&w=2
[RFC PATCH 1/3] http://marc.info/?t=147980325700006&r=1&w=2
[RFC PATCH 2/3] http://marc.info/?t=147980325700005&r=1&w=2
[RFC PATCH 3/3] http://marc.info/?t=147980325700007&r=1&w=2
(Man, I miss gmane.) I'll cc lkml on the next submission.
Quoted 6 lines
In that context it superficially looks ok to me (ie it's filesystem-controlled and done only when we've serialized the directory for the
link()operation anyway). But I didn't think about it _that_ much.Linus
RFC PATCH 3/3 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 | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 55 insertions(+), 4 deletions(-)
fs/btrfs/inode.c +55 -4
@@ -6566,15 +6566,20 @@ 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);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 ;
@@ -6582,16 +6587,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 (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 ;
@@ -6603,6 +6639,21 @@ 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, dir, new_inode, + dentry->d_name.name, + dentry->d_name.len); +if (!err && new_inode->i_nlink ==0 ) + err =btrfs_orphan_add (trans, new_inode); +if (err) { +btrfs_abort_transaction (trans, err); + goto fail; + } + } +ihold (inode);set_bit (BTRFS_INODE_COPY_EVERYTHING , &BTRFS_I (inode)->runtime_flags);
@@ -6624,7 +6675,7 @@ static int btrfs_link(struct dentry *old_dentry, struct inode *dir,if (err) goto fail; } -d_instantiate (dentry, inode); +d_replace (dentry, inode);btrfs_log_new_name (trans, inode,NULL , parent); }
@@ -10570,7 +10621,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.10.2