diff mbox series

[v2,4/8] ovl: use tmpfile_open() helper

Message ID 20220919141031.1834447-5-mszeredi@redhat.com (mailing list archive)
State New, archived
Headers show
Series fuse tmpfile | expand

Commit Message

Miklos Szeredi Sept. 19, 2022, 2:10 p.m. UTC 
If tmpfile is used for copy up, then use this helper to create the tmpfile
and open it at the same time.  This will later allow filesystems such as
fuse to do this operation atomically.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/overlayfs/copy_up.c   | 49 ++++++++++++++++++++++------------------
 fs/overlayfs/overlayfs.h | 12 ++++++----
 fs/overlayfs/super.c     | 10 ++++----
 3 files changed, 40 insertions(+), 31 deletions(-)

Comments

Al Viro Sept. 20, 2022, 1:33 a.m. UTC | #1 
On Mon, Sep 19, 2022 at 04:10:27PM +0200, Miklos Szeredi wrote:
> If tmpfile is used for copy up, then use this helper to create the tmpfile
> and open it at the same time.  This will later allow filesystems such as
> fuse to do this operation atomically.
> 
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>  fs/overlayfs/copy_up.c   | 49 ++++++++++++++++++++++------------------
>  fs/overlayfs/overlayfs.h | 12 ++++++----
>  fs/overlayfs/super.c     | 10 ++++----
>  3 files changed, 40 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index fdde6c56cc3d..ac087b48b5da 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -194,16 +194,16 @@ static int ovl_copy_fileattr(struct inode *inode, struct path *old,
>  }
>  
>  static int ovl_copy_up_data(struct ovl_fs *ofs, struct path *old,
> -			    struct path *new, loff_t len)
> +			    struct path *new, struct file *new_file, loff_t len)
>  {

Ugh...  Lift opening into both callers and get rid of struct path *new,
please.  Would be much easier to follow that way...

> -	err = ovl_copy_up_inode(c, temp);
> +	err = ovl_copy_up_inode(c, temp, NULL);

FWIW, I would consider passing a struct file * in all cases, with O_PATH
for non-regular ones...

> -	temp = ovl_do_tmpfile(ofs, ofs->workdir, S_IFREG | 0);
> -	ofs->tmpfile = !IS_ERR(temp);
> +	tmpfile = ovl_do_tmpfile(ofs, ofs->workdir, S_IFREG | 0);
> +	ofs->tmpfile = !IS_ERR(tmpfile);
>  	if (ofs->tmpfile)
> -		dput(temp);
> +		fput(tmpfile);

	Careful - that part essentially checks if we have a working
->tmpfile(), but we rely upon more than just having it - we want
dentry-based setxattr() and friends to work after O_TMPFILE.
Are we making that a requirement for ->tmpfile()?  I.e. that
after O_TMPFILE open notify_change() et.al. on its dentry
will work *without* corresponding struct file.  In particular,
fuse seems to check for ATTR_FILE...
Miklos Szeredi Sept. 20, 2022, 8:02 a.m. UTC | #2 
On Tue, 20 Sept 2022 at 03:33, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Mon, Sep 19, 2022 at 04:10:27PM +0200, Miklos Szeredi wrote:
> > If tmpfile is used for copy up, then use this helper to create the tmpfile
> > and open it at the same time.  This will later allow filesystems such as
> > fuse to do this operation atomically.
> >
> > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> > ---
> >  fs/overlayfs/copy_up.c   | 49 ++++++++++++++++++++++------------------
> >  fs/overlayfs/overlayfs.h | 12 ++++++----
> >  fs/overlayfs/super.c     | 10 ++++----
> >  3 files changed, 40 insertions(+), 31 deletions(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index fdde6c56cc3d..ac087b48b5da 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -194,16 +194,16 @@ static int ovl_copy_fileattr(struct inode *inode, struct path *old,
> >  }
> >
> >  static int ovl_copy_up_data(struct ovl_fs *ofs, struct path *old,
> > -                         struct path *new, loff_t len)
> > +                         struct path *new, struct file *new_file, loff_t len)
> >  {
>
> Ugh...  Lift opening into both callers and get rid of struct path *new,
> please.  Would be much easier to follow that way...
>
> > -     err = ovl_copy_up_inode(c, temp);
> > +     err = ovl_copy_up_inode(c, temp, NULL);
>
> FWIW, I would consider passing a struct file * in all cases, with O_PATH
> for non-regular ones...

OK.

>
> > -     temp = ovl_do_tmpfile(ofs, ofs->workdir, S_IFREG | 0);
> > -     ofs->tmpfile = !IS_ERR(temp);
> > +     tmpfile = ovl_do_tmpfile(ofs, ofs->workdir, S_IFREG | 0);
> > +     ofs->tmpfile = !IS_ERR(tmpfile);
> >       if (ofs->tmpfile)
> > -             dput(temp);
> > +             fput(tmpfile);
>
>         Careful - that part essentially checks if we have a working
> ->tmpfile(), but we rely upon more than just having it - we want
> dentry-based setxattr() and friends to work after O_TMPFILE.
> Are we making that a requirement for ->tmpfile()?

Yes, I think that should be expected of all sane filesystems.   Adding
tmpfile support to a fuse filesystem will require explicit code, so no
regression risk there.

Thanks,
Miklos
diff mbox series

Patch

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index fdde6c56cc3d..ac087b48b5da 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -194,16 +194,16 @@  static int ovl_copy_fileattr(struct inode *inode, struct path *old,
 }
 
 static int ovl_copy_up_data(struct ovl_fs *ofs, struct path *old,
-			    struct path *new, loff_t len)
+			    struct path *new, struct file *new_file, loff_t len)
 {
 	struct file *old_file;
-	struct file *new_file;
 	loff_t old_pos = 0;
 	loff_t new_pos = 0;
 	loff_t cloned;
 	loff_t data_pos = -1;
 	loff_t hole_len;
 	bool skip_hole = false;
+	bool fput_new = false;
 	int error = 0;
 
 	if (len == 0)
@@ -213,10 +213,13 @@  static int ovl_copy_up_data(struct ovl_fs *ofs, struct path *old,
 	if (IS_ERR(old_file))
 		return PTR_ERR(old_file);
 
-	new_file = ovl_path_open(new, O_LARGEFILE | O_WRONLY);
-	if (IS_ERR(new_file)) {
-		error = PTR_ERR(new_file);
-		goto out_fput;
+	if (!new_file) {
+		new_file = ovl_path_open(new, O_LARGEFILE | O_WRONLY);
+		if (IS_ERR(new_file)) {
+			error = PTR_ERR(new_file);
+			goto out_fput;
+		}
+		fput_new = true;
 	}
 
 	/* Try to use clone_file_range to clone up within the same fs */
@@ -285,7 +288,8 @@  static int ovl_copy_up_data(struct ovl_fs *ofs, struct path *old,
 out:
 	if (!error && ovl_should_sync(ofs))
 		error = vfs_fsync(new_file, 0);
-	fput(new_file);
+	if (fput_new)
+		fput(new_file);
 out_fput:
 	fput(old_file);
 	return error;
@@ -556,7 +560,8 @@  static int ovl_link_up(struct ovl_copy_up_ctx *c)
 	return err;
 }
 
-static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
+static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp,
+			     struct file *tmpfile)
 {
 	struct ovl_fs *ofs = OVL_FS(c->dentry->d_sb);
 	struct inode *inode = d_inode(c->dentry);
@@ -575,7 +580,7 @@  static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 	 */
 	if (S_ISREG(c->stat.mode) && !c->metacopy) {
 		ovl_path_lowerdata(c->dentry, &datapath);
-		err = ovl_copy_up_data(ofs, &datapath, &upperpath,
+		err = ovl_copy_up_data(ofs, &datapath, &upperpath, tmpfile,
 				       c->stat.size);
 		if (err)
 			return err;
@@ -688,7 +693,7 @@  static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
 	if (IS_ERR(temp))
 		goto unlock;
 
-	err = ovl_copy_up_inode(c, temp);
+	err = ovl_copy_up_inode(c, temp, NULL);
 	if (err)
 		goto cleanup;
 
@@ -732,6 +737,7 @@  static int ovl_copy_up_tmpfile(struct ovl_copy_up_ctx *c)
 	struct ovl_fs *ofs = OVL_FS(c->dentry->d_sb);
 	struct inode *udir = d_inode(c->destdir);
 	struct dentry *temp, *upper;
+	struct file *tmpfile;
 	struct ovl_cu_creds cc;
 	int err;
 
@@ -739,15 +745,16 @@  static int ovl_copy_up_tmpfile(struct ovl_copy_up_ctx *c)
 	if (err)
 		return err;
 
-	temp = ovl_do_tmpfile(ofs, c->workdir, c->stat.mode);
+	tmpfile = ovl_do_tmpfile(ofs, c->workdir, c->stat.mode);
 	ovl_revert_cu_creds(&cc);
 
-	if (IS_ERR(temp))
-		return PTR_ERR(temp);
+	if (IS_ERR(tmpfile))
+		return PTR_ERR(tmpfile);
 
-	err = ovl_copy_up_inode(c, temp);
+	temp = tmpfile->f_path.dentry;
+	err = ovl_copy_up_inode(c, temp, tmpfile);
 	if (err)
-		goto out_dput;
+		goto out_fput;
 
 	inode_lock_nested(udir, I_MUTEX_PARENT);
 
@@ -761,16 +768,14 @@  static int ovl_copy_up_tmpfile(struct ovl_copy_up_ctx *c)
 	inode_unlock(udir);
 
 	if (err)
-		goto out_dput;
+		goto out_fput;
 
 	if (!c->metacopy)
 		ovl_set_upperdata(d_inode(c->dentry));
-	ovl_inode_update(d_inode(c->dentry), temp);
+	ovl_inode_update(d_inode(c->dentry), dget(temp));
 
-	return 0;
-
-out_dput:
-	dput(temp);
+out_fput:
+	fput(tmpfile);
 	return err;
 }
 
@@ -919,7 +924,7 @@  static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
 			goto out;
 	}
 
-	err = ovl_copy_up_data(ofs, &datapath, &upperpath, c->stat.size);
+	err = ovl_copy_up_data(ofs, &datapath, &upperpath, NULL, c->stat.size);
 	if (err)
 		goto out_free;
 
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 87759165d32b..259a6e73d0c4 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -310,14 +310,16 @@  static inline int ovl_do_whiteout(struct ovl_fs *ofs,
 	return err;
 }
 
-static inline struct dentry *ovl_do_tmpfile(struct ovl_fs *ofs,
-					    struct dentry *dentry, umode_t mode)
+static inline struct file *ovl_do_tmpfile(struct ovl_fs *ofs,
+					  struct dentry *dentry, umode_t mode)
 {
-	struct dentry *ret = vfs_tmpfile(ovl_upper_mnt_userns(ofs), dentry, mode, 0);
-	int err = PTR_ERR_OR_ZERO(ret);
+	struct path path = { .mnt = ovl_upper_mnt(ofs), .dentry = dentry };
+	struct file *file = tmpfile_open(ovl_upper_mnt_userns(ofs), &path, mode,
+					O_LARGEFILE | O_WRONLY, current_cred());
+	int err = PTR_ERR_OR_ZERO(file);
 
 	pr_debug("tmpfile(%pd2, 0%o) = %i\n", dentry, mode, err);
-	return ret;
+	return file;
 }
 
 static inline struct dentry *ovl_lookup_upper(struct ovl_fs *ofs,
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index ec746d447f1b..7837223689c1 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -15,6 +15,7 @@ 
 #include <linux/seq_file.h>
 #include <linux/posix_acl_xattr.h>
 #include <linux/exportfs.h>
+#include <linux/file.h>
 #include "overlayfs.h"
 
 MODULE_AUTHOR("Miklos Szeredi <miklos@szeredi.hu>");
@@ -1356,7 +1357,8 @@  static int ovl_make_workdir(struct super_block *sb, struct ovl_fs *ofs,
 			    struct path *workpath)
 {
 	struct vfsmount *mnt = ovl_upper_mnt(ofs);
-	struct dentry *temp, *workdir;
+	struct dentry *workdir;
+	struct file *tmpfile;
 	bool rename_whiteout;
 	bool d_type;
 	int fh_type;
@@ -1392,10 +1394,10 @@  static int ovl_make_workdir(struct super_block *sb, struct ovl_fs *ofs,
 		pr_warn("upper fs needs to support d_type.\n");
 
 	/* Check if upper/work fs supports O_TMPFILE */
-	temp = ovl_do_tmpfile(ofs, ofs->workdir, S_IFREG | 0);
-	ofs->tmpfile = !IS_ERR(temp);
+	tmpfile = ovl_do_tmpfile(ofs, ofs->workdir, S_IFREG | 0);
+	ofs->tmpfile = !IS_ERR(tmpfile);
 	if (ofs->tmpfile)
-		dput(temp);
+		fput(tmpfile);
 	else
 		pr_warn("upper fs does not support tmpfile.\n");