| Message ID | 20201109100343.3958378-3-chirantan@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support for O_TMPFILE in fuse | expand |
On Mon, Nov 9, 2020 at 11:04 AM Chirantan Ekbote <chirantan@chromium.org> wrote: > > Implement support for O_TMPFILE by re-using the existing infrastructure > for mkdir, symlink, mknod, etc. The server should reply to the tmpfile > request by sending a fuse_entry_out describing the newly created > tmpfile. > > Signed-off-by: Chirantan Ekbote <chirantan@chromium.org> > --- > fs/fuse/dir.c | 21 +++++++++++++++++++++ > fs/fuse/file.c | 3 ++- > 2 files changed, 23 insertions(+), 1 deletion(-) > > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c > index ff7dbeb16f88d..1ab52e7ec1625 100644 > --- a/fs/fuse/dir.c > +++ b/fs/fuse/dir.c > @@ -751,6 +751,26 @@ static int fuse_mkdir(struct inode *dir, struct dentry *entry, umode_t mode) > return create_new_entry(fm, &args, dir, entry, S_IFDIR); > } > > +static int fuse_tmpfile(struct inode *dir, struct dentry *entry, umode_t mode) > +{ > + struct fuse_tmpfile_in inarg; > + struct fuse_mount *fm = get_fuse_mount(dir); > + FUSE_ARGS(args); > + > + if (!fm->fc->dont_mask) > + mode &= ~current_umask(); > + > + memset(&inarg, 0, sizeof(inarg)); > + inarg.mode = mode; > + inarg.umask = current_umask(); > + args.opcode = FUSE_TMPFILE; > + args.in_numargs = 1; > + args.in_args[0].size = sizeof(inarg); > + args.in_args[0].value = &inarg; > + > + return create_new_entry(fm, &args, dir, entry, S_IFREG); > +} > + > static int fuse_symlink(struct inode *dir, struct dentry *entry, > const char *link) > { > @@ -1818,6 +1838,7 @@ static const struct inode_operations fuse_dir_inode_operations = { > .listxattr = fuse_listxattr, > .get_acl = fuse_get_acl, > .set_acl = fuse_set_acl, > + .tmpfile = fuse_tmpfile, > }; > > static const struct file_operations fuse_dir_operations = { > diff --git a/fs/fuse/file.c b/fs/fuse/file.c > index c03034e8c1529..8ecf85699a014 100644 > --- a/fs/fuse/file.c > +++ b/fs/fuse/file.c > @@ -39,7 +39,8 @@ static int fuse_send_open(struct fuse_mount *fm, u64 nodeid, struct file *file, > FUSE_ARGS(args); > > memset(&inarg, 0, sizeof(inarg)); > - inarg.flags = file->f_flags & ~(O_CREAT | O_EXCL | O_NOCTTY); > + inarg.flags = > + file->f_flags & ~(O_CREAT | O_EXCL | O_NOCTTY | O_TMPFILE); Why did you add this? At this stage O_TMPFILE should not be in the flags, since that case was handled via the ->tmpfile() path. Thanks, Miklos
On Mon, Nov 9, 2020 at 8:37 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > On Mon, Nov 9, 2020 at 11:04 AM Chirantan Ekbote <chirantan@chromium.org> wrote: > > > > Implement support for O_TMPFILE by re-using the existing infrastructure > > for mkdir, symlink, mknod, etc. The server should reply to the tmpfile > > request by sending a fuse_entry_out describing the newly created > > tmpfile. > > > > Signed-off-by: Chirantan Ekbote <chirantan@chromium.org> > > --- > > fs/fuse/dir.c | 21 +++++++++++++++++++++ > > fs/fuse/file.c | 3 ++- > > 2 files changed, 23 insertions(+), 1 deletion(-) > > > > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c > > index ff7dbeb16f88d..1ab52e7ec1625 100644 > > --- a/fs/fuse/dir.c > > +++ b/fs/fuse/dir.c > > @@ -751,6 +751,26 @@ static int fuse_mkdir(struct inode *dir, struct dentry *entry, umode_t mode) > > return create_new_entry(fm, &args, dir, entry, S_IFDIR); > > } > > > > +static int fuse_tmpfile(struct inode *dir, struct dentry *entry, umode_t mode) > > +{ > > + struct fuse_tmpfile_in inarg; > > + struct fuse_mount *fm = get_fuse_mount(dir); > > + FUSE_ARGS(args); > > + > > + if (!fm->fc->dont_mask) > > + mode &= ~current_umask(); > > + > > + memset(&inarg, 0, sizeof(inarg)); > > + inarg.mode = mode; > > + inarg.umask = current_umask(); > > + args.opcode = FUSE_TMPFILE; > > + args.in_numargs = 1; > > + args.in_args[0].size = sizeof(inarg); > > + args.in_args[0].value = &inarg; > > + > > + return create_new_entry(fm, &args, dir, entry, S_IFREG); > > +} > > + > > static int fuse_symlink(struct inode *dir, struct dentry *entry, > > const char *link) > > { > > @@ -1818,6 +1838,7 @@ static const struct inode_operations fuse_dir_inode_operations = { > > .listxattr = fuse_listxattr, > > .get_acl = fuse_get_acl, > > .set_acl = fuse_set_acl, > > + .tmpfile = fuse_tmpfile, > > }; > > > > static const struct file_operations fuse_dir_operations = { > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c > > index c03034e8c1529..8ecf85699a014 100644 > > --- a/fs/fuse/file.c > > +++ b/fs/fuse/file.c > > @@ -39,7 +39,8 @@ static int fuse_send_open(struct fuse_mount *fm, u64 nodeid, struct file *file, > > FUSE_ARGS(args); > > > > memset(&inarg, 0, sizeof(inarg)); > > - inarg.flags = file->f_flags & ~(O_CREAT | O_EXCL | O_NOCTTY); > > + inarg.flags = > > + file->f_flags & ~(O_CREAT | O_EXCL | O_NOCTTY | O_TMPFILE); > > Why did you add this? At this stage O_TMPFILE should not be in the > flags, since that case was handled via the ->tmpfile() path. > That's not the behavior I observed. Without this, the O_TMPFILE flag gets passed through to the server. The call stack is: - do_filp_open - path_openat - do_tmpfile - vfs_tmpfile - dir->i_op->tmpfile - finish_open - do_dentry_open - f->f_op->open and I didn't see O_TMPFILE being removed anywhere in there. Chirantan
On Tue, Nov 10, 2020 at 4:33 AM Chirantan Ekbote <chirantan@chromium.org> wrote: > That's not the behavior I observed. Without this, the O_TMPFILE flag > gets passed through to the server. The call stack is: > > - do_filp_open > - path_openat > - do_tmpfile > - vfs_tmpfile > - dir->i_op->tmpfile > - finish_open > - do_dentry_open > - f->f_op->open > > and I didn't see O_TMPFILE being removed anywhere in there. Ah, indeed. The reason I missed this is because IMO the way it *should* work is that FUSE_TMPFILE creates and opens the file in one go. We shouldn't need two separate request. Not sure how we should go about this... The ->atomic_open() API is sufficient, but maybe we want a new ->atomic_tmpfile(). Al? Thanks, Miklos
On Tue, Nov 10, 2020 at 4:52 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > On Tue, Nov 10, 2020 at 4:33 AM Chirantan Ekbote <chirantan@chromium.org> wrote: > > > That's not the behavior I observed. Without this, the O_TMPFILE flag > > gets passed through to the server. The call stack is: > > > > - do_filp_open > > - path_openat > > - do_tmpfile > > - vfs_tmpfile > > - dir->i_op->tmpfile > > - finish_open > > - do_dentry_open > > - f->f_op->open > > > > and I didn't see O_TMPFILE being removed anywhere in there. > > Ah, indeed. > > The reason I missed this is because IMO the way it *should* work is > that FUSE_TMPFILE creates and opens the file in one go. We shouldn't > need two separate request. > > Not sure how we should go about this... The ->atomic_open() API is > sufficient, but maybe we want a new ->atomic_tmpfile(). > I think I agree with you that it should probably be a single request but at this point is it worth adding an ->atomic_tmpfile() that's only used by fuse? Unlike regular file creation, it's not like the tmpfile entry is accessible via any other mechanism so other than latency I don't think there's any real harm with having it be 2 separate requests.
On Fri, Nov 13, 2020 at 6:19 AM Chirantan Ekbote <chirantan@chromium.org> wrote: > > On Tue, Nov 10, 2020 at 4:52 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > > > On Tue, Nov 10, 2020 at 4:33 AM Chirantan Ekbote <chirantan@chromium.org> wrote: > > > > > That's not the behavior I observed. Without this, the O_TMPFILE flag > > > gets passed through to the server. The call stack is: > > > > > > - do_filp_open > > > - path_openat > > > - do_tmpfile > > > - vfs_tmpfile > > > - dir->i_op->tmpfile > > > - finish_open > > > - do_dentry_open > > > - f->f_op->open > > > > > > and I didn't see O_TMPFILE being removed anywhere in there. > > > > Ah, indeed. > > > > The reason I missed this is because IMO the way it *should* work is > > that FUSE_TMPFILE creates and opens the file in one go. We shouldn't > > need two separate request. > > > > Not sure how we should go about this... The ->atomic_open() API is > > sufficient, but maybe we want a new ->atomic_tmpfile(). > > > > I think I agree with you that it should probably be a single request > but at this point is it worth adding an ->atomic_tmpfile() that's only > used by fuse? Unlike regular file creation, it's not like the tmpfile > entry is accessible via any other mechanism so other than latency I > don't think there's any real harm with having it be 2 separate > requests. It's the wrong interface, and we'll have to live with it forever if we go this route. Better get the interface right and *then* think about the implementation. I don't think adding ->atomic_tmpfile() would be that of a big deal, and likely other distributed fs would start using it in the future. Thanks, Miklos
On Fri, Nov 13, 2020 at 11:52:09AM +0100, Miklos Szeredi wrote: > It's the wrong interface, and we'll have to live with it forever if we > go this route. > > Better get the interface right and *then* think about the > implementation. I don't think adding ->atomic_tmpfile() would be that > of a big deal, and likely other distributed fs would start using it in > the future. Let me think about it; I'm very unhappy with the amount of surgery it has taken to somewhat sanitize the results of ->atomic_open() introduction, so I'd prefer to do it reasonably clean or not at all.
On Fri, Nov 13, 2020 at 1:28 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > On Fri, Nov 13, 2020 at 11:52:09AM +0100, Miklos Szeredi wrote: > > > It's the wrong interface, and we'll have to live with it forever if we > > go this route. > > > > Better get the interface right and *then* think about the > > implementation. I don't think adding ->atomic_tmpfile() would be that > > of a big deal, and likely other distributed fs would start using it in > > the future. > > Let me think about it; I'm very unhappy with the amount of surgery it has > taken to somewhat sanitize the results of ->atomic_open() introduction, so > I'd prefer to do it reasonably clean or not at all. The minimal VFS change for fuse to be able to do tmpfile with one request would be to pass open_flags to ->tmpfile(). That way the private data for the open file would need to be temporarily stored in the inode and ->open() would just pick it up from there without sending another request. Not the cleanest, but I really don't care as long as the public interface is the right one. Thanks, Miklos
On Fri, Nov 13, 2020 at 2:54:46PM +0100, Miklos Szeredi wrote: > > On Fri, Nov 13, 2020 at 1:28 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > > > On Fri, Nov 13, 2020 at 11:52:09AM +0100, Miklos Szeredi wrote: > > > > > It's the wrong interface, and we'll have to live with it forever if we > > > go this route. > > > > > > Better get the interface right and *then* think about the > > > implementation. I don't think adding ->atomic_tmpfile() would be that > > > of a big deal, and likely other distributed fs would start using it in > > > the future. > > > > Let me think about it; I'm very unhappy with the amount of surgery it has > > taken to somewhat sanitize the results of ->atomic_open() introduction, so > > I'd prefer to do it reasonably clean or not at all. > > The minimal VFS change for fuse to be able to do tmpfile with one > request would be to pass open_flags to ->tmpfile(). That way the > private data for the open file would need to be temporarily stored in > the inode and ->open() would just pick it up from there without > sending another request. Not the cleanest, but I really don't care as > long as the public interface is the right one. > > Thanks, > Miklos Resurrecting this old thread. Is there a conclusion on the addition of atomic_tmpfil() or vfs changes? Thanks, Yu-Li Lin
Ref: https://lwn.net/Articles/896153/ On Wed, 31 Aug 2022 at 04:57, Yu-Li Lin <yulilin@google.com> wrote: > > On Fri, Nov 13, 2020 at 2:54:46PM +0100, Miklos Szeredi wrote: > > > > On Fri, Nov 13, 2020 at 1:28 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > > > > > On Fri, Nov 13, 2020 at 11:52:09AM +0100, Miklos Szeredi wrote: > > > > > > > It's the wrong interface, and we'll have to live with it forever if we > > > > go this route. > > > > > > > > Better get the interface right and *then* think about the > > > > implementation. I don't think adding ->atomic_tmpfile() would be that > > > > of a big deal, and likely other distributed fs would start using it in > > > > the future. > > > > > > Let me think about it; I'm very unhappy with the amount of surgery it has > > > taken to somewhat sanitize the results of ->atomic_open() introduction, so > > > I'd prefer to do it reasonably clean or not at all. > > > > The minimal VFS change for fuse to be able to do tmpfile with one > > request would be to pass open_flags to ->tmpfile(). That way the > > private data for the open file would need to be temporarily stored in > > the inode and ->open() would just pick it up from there without > > sending another request. Not the cleanest, but I really don't care as > > long as the public interface is the right one. > > > > Thanks, > > Miklos > > Resurrecting this old thread. Is there a conclusion on the addition of atomic_tmpfil() or vfs changes? > > Thanks, > Yu-Li Lin
On Wed, Aug 31, 2022 at 5:20 AM Miklos Szeredi <miklos@szeredi.hu> wrote: > > Ref: https://lwn.net/Articles/896153/ > > On Wed, 31 Aug 2022 at 04:57, Yu-Li Lin <yulilin@google.com> wrote: > > > > On Fri, Nov 13, 2020 at 2:54:46PM +0100, Miklos Szeredi wrote: > > > > > > On Fri, Nov 13, 2020 at 1:28 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > > > > > > > On Fri, Nov 13, 2020 at 11:52:09AM +0100, Miklos Szeredi wrote: > > > > > > > > > It's the wrong interface, and we'll have to live with it forever if we > > > > > go this route. > > > > > > > > > > Better get the interface right and *then* think about the > > > > > implementation. I don't think adding ->atomic_tmpfile() would be that > > > > > of a big deal, and likely other distributed fs would start using it in > > > > > the future. > > > > > > > > Let me think about it; I'm very unhappy with the amount of surgery it has > > > > taken to somewhat sanitize the results of ->atomic_open() introduction, so > > > > I'd prefer to do it reasonably clean or not at all. > > > > > > The minimal VFS change for fuse to be able to do tmpfile with one > > > request would be to pass open_flags to ->tmpfile(). That way the > > > private data for the open file would need to be temporarily stored in > > > the inode and ->open() would just pick it up from there without > > > sending another request. Not the cleanest, but I really don't care as > > > long as the public interface is the right one. > > > > > > Thanks, > > > Miklos > > > > Resurrecting this old thread. Is there a conclusion on the addition of atomic_tmpfil() or vfs changes? > > > > Thanks, > > Yu-Li Lin Thanks for the reference. IIUC, the consensus is to make it atomic, although there's no agreement on how it should be done. Does that mean we should hold off on this patch until atomic temp files are figured out higher in the stack or do you have thoughts on how the fuse uapi should look like prior to the vfs/refactoring decision?
On Wed, Aug 31, 2022 at 02:30:40PM -0700, Yu-li Lin wrote: > Thanks for the reference. IIUC, the consensus is to make it atomic, > although there's no agreement on how it should be done. Does that mean > we should hold off on > this patch until atomic temp files are figured out higher in the stack > or do you have thoughts on how the fuse uapi should look like prior to > the vfs/refactoring decision? Here's a patch refactoring the tmpfile kapi to return an open file instead of a dentry. Comments? Thanks, Miklos --- fs/bad_inode.c | 2 fs/btrfs/inode.c | 12 +++-- fs/cachefiles/namei.c | 3 - fs/ext2/namei.c | 6 +- fs/ext4/namei.c | 15 ++++-- fs/f2fs/namei.c | 9 +++- fs/hugetlbfs/inode.c | 9 +++- fs/minix/namei.c | 9 ++-- fs/namei.c | 101 ++++++++++++++++++++++++++--------------------- fs/open.c | 7 +++ fs/overlayfs/overlayfs.h | 3 - fs/ramfs/inode.c | 6 +- fs/ubifs/dir.c | 5 +- fs/udf/namei.c | 6 +- fs/xfs/xfs_iops.c | 9 +++- include/linux/fs.h | 6 +- mm/shmem.c | 12 +++-- 17 files changed, 138 insertions(+), 82 deletions(-) --- a/fs/bad_inode.c +++ b/fs/bad_inode.c @@ -147,7 +147,7 @@ static int bad_inode_atomic_open(struct } static int bad_inode_tmpfile(struct user_namespace *mnt_userns, - struct inode *inode, struct dentry *dentry, + struct inode *inode, struct file *file, umode_t mode) { return -EIO; --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -10169,7 +10169,7 @@ static int btrfs_permission(struct user_ } static int btrfs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir, - struct dentry *dentry, umode_t mode) + struct file *file, umode_t mode) { struct btrfs_fs_info *fs_info = btrfs_sb(dir->i_sb); struct btrfs_trans_handle *trans; @@ -10177,7 +10177,7 @@ static int btrfs_tmpfile(struct user_nam struct inode *inode; struct btrfs_new_inode_args new_inode_args = { .dir = dir, - .dentry = dentry, + .dentry = file->f_path.dentry, .orphan = true, }; unsigned int trans_num_items; @@ -10214,7 +10214,7 @@ static int btrfs_tmpfile(struct user_nam set_nlink(inode, 1); if (!ret) { - d_tmpfile(dentry, inode); + d_tmpfile(file->f_path.dentry, inode); unlock_new_inode(inode); mark_inode_dirty(inode); } @@ -10224,9 +10224,11 @@ static int btrfs_tmpfile(struct user_nam out_new_inode_args: btrfs_new_inode_args_destroy(&new_inode_args); out_inode: - if (ret) + if (ret) { iput(inode); - return ret; + return ret; + } + return finish_tmpfile(file); } void btrfs_set_range_writeback(struct btrfs_inode *inode, u64 start, u64 end) --- a/fs/cachefiles/namei.c +++ b/fs/cachefiles/namei.c @@ -459,9 +459,10 @@ struct file *cachefiles_create_tmpfile(s cachefiles_begin_secure(cache, &saved_cred); path.mnt = cache->mnt; + path.dentry = fan; ret = cachefiles_inject_write_error(); if (ret == 0) - path.dentry = vfs_tmpfile(&init_user_ns, fan, S_IFREG, O_RDWR); + path.dentry = vfs_tmpfile(&init_user_ns, &path, S_IFREG, O_RDWR); else path.dentry = ERR_PTR(ret); if (IS_ERR(path.dentry)) { --- a/fs/ext2/namei.c +++ b/fs/ext2/namei.c @@ -120,7 +120,7 @@ static int ext2_create (struct user_name } static int ext2_tmpfile(struct user_namespace *mnt_userns, struct inode *dir, - struct dentry *dentry, umode_t mode) + struct file *file, umode_t mode) { struct inode *inode = ext2_new_inode(dir, mode, NULL); if (IS_ERR(inode)) @@ -128,9 +128,9 @@ static int ext2_tmpfile(struct user_name ext2_set_file_ops(inode); mark_inode_dirty(inode); - d_tmpfile(dentry, inode); + d_tmpfile(file->f_path.dentry, inode); unlock_new_inode(inode); - return 0; + return finish_tmpfile(file); } static int ext2_mknod (struct user_namespace * mnt_userns, struct inode * dir, --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -2849,7 +2849,7 @@ static int ext4_mknod(struct user_namesp } static int ext4_tmpfile(struct user_namespace *mnt_userns, struct inode *dir, - struct dentry *dentry, umode_t mode) + struct file *file, umode_t mode) { handle_t *handle; struct inode *inode; @@ -2857,7 +2857,7 @@ static int ext4_tmpfile(struct user_name err = dquot_initialize(dir); if (err) - return err; + goto out; retry: inode = ext4_new_inode_start_handle(mnt_userns, dir, mode, @@ -2871,7 +2871,7 @@ static int ext4_tmpfile(struct user_name inode->i_op = &ext4_file_inode_operations; inode->i_fop = &ext4_file_operations; ext4_set_aops(inode); - d_tmpfile(dentry, inode); + d_tmpfile(file->f_path.dentry, inode); err = ext4_orphan_add(handle, inode); if (err) goto err_unlock_inode; @@ -2882,11 +2882,16 @@ static int ext4_tmpfile(struct user_name ext4_journal_stop(handle); if (err == -ENOSPC && ext4_should_retry_alloc(dir->i_sb, &retries)) goto retry; - return err; +out: + if (err) + return err; + + return finish_tmpfile(file); + err_unlock_inode: ext4_journal_stop(handle); unlock_new_inode(inode); - return err; + goto out; } struct ext4_dir_entry_2 *ext4_init_dot_dotdot(struct inode *inode, --- a/fs/f2fs/namei.c +++ b/fs/f2fs/namei.c @@ -915,16 +915,21 @@ static int __f2fs_tmpfile(struct user_na } static int f2fs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir, - struct dentry *dentry, umode_t mode) + struct file *file, umode_t mode) { struct f2fs_sb_info *sbi = F2FS_I_SB(dir); + int err; if (unlikely(f2fs_cp_error(sbi))) return -EIO; if (!f2fs_is_checkpoint_ready(sbi)) return -ENOSPC; - return __f2fs_tmpfile(mnt_userns, dir, dentry, mode, false, NULL); + err = __f2fs_tmpfile(mnt_userns, dir, file->f_path.dentry, mode, false, NULL); + if (err) + return err; + + return finish_tmpfile(file); } static int f2fs_create_whiteout(struct user_namespace *mnt_userns, --- a/fs/hugetlbfs/inode.c +++ b/fs/hugetlbfs/inode.c @@ -932,10 +932,15 @@ static int hugetlbfs_create(struct user_ } static int hugetlbfs_tmpfile(struct user_namespace *mnt_userns, - struct inode *dir, struct dentry *dentry, + struct inode *dir, struct file *file, umode_t mode) { - return do_hugetlbfs_mknod(dir, dentry, mode | S_IFREG, 0, true); + int err = do_hugetlbfs_mknod(dir, file->f_path.dentry, mode | S_IFREG, 0, true); + + if (err) + return err; + + return finish_tmpfile(file); } static int hugetlbfs_symlink(struct user_namespace *mnt_userns, --- a/fs/minix/namei.c +++ b/fs/minix/namei.c @@ -53,16 +53,19 @@ static int minix_mknod(struct user_names } static int minix_tmpfile(struct user_namespace *mnt_userns, struct inode *dir, - struct dentry *dentry, umode_t mode) + struct file *file, umode_t mode) { int error; struct inode *inode = minix_new_inode(dir, mode, &error); if (inode) { minix_set_inode(inode, 0); mark_inode_dirty(inode); - d_tmpfile(dentry, inode); + d_tmpfile(file->f_path.dentry, inode); } - return error; + if (error) + return error; + + return finish_tmpfile(file); } static int minix_create(struct user_namespace *mnt_userns, struct inode *dir, --- a/fs/namei.c +++ b/fs/namei.c @@ -3568,59 +3568,78 @@ static int do_open(struct nameidata *nd, return error; } -/** - * vfs_tmpfile - create tmpfile - * @mnt_userns: user namespace of the mount the inode was found from - * @dentry: pointer to dentry of the base directory - * @mode: mode of the new tmpfile - * @open_flag: flags - * - * Create a temporary file. - * - * If the inode has been found through an idmapped mount the user namespace of - * the vfsmount must be passed through @mnt_userns. This function will then take - * care to map the inode according to @mnt_userns before checking permissions. - * On non-idmapped mounts or if permission checking is to be performed on the - * raw inode simply passs init_user_ns. - */ -struct dentry *vfs_tmpfile(struct user_namespace *mnt_userns, - struct dentry *dentry, umode_t mode, int open_flag) +static int vfs_tmpfile_new(struct user_namespace *mnt_userns, + const struct path *parent_path, + struct file *file, umode_t mode) { - struct dentry *child = NULL; - struct inode *dir = dentry->d_inode; - struct inode *inode; + struct inode *inode, *dir = d_inode(parent_path->dentry); + struct dentry *child; int error; /* we want directory to be writable */ error = inode_permission(mnt_userns, dir, MAY_WRITE | MAY_EXEC); if (error) - goto out_err; + goto out; error = -EOPNOTSUPP; if (!dir->i_op->tmpfile) - goto out_err; + goto out; error = -ENOMEM; - child = d_alloc(dentry, &slash_name); + child = d_alloc(parent_path->dentry, &slash_name); if (unlikely(!child)) - goto out_err; + goto out; + file->f_path.mnt = parent_path->mnt; + file->f_path.dentry = child; mode = vfs_prepare_mode(mnt_userns, dir, mode, mode, mode); - error = dir->i_op->tmpfile(mnt_userns, dir, child, mode); + error = dir->i_op->tmpfile(mnt_userns, dir, file, mode); if (error) - goto out_err; + goto out_dput; error = -ENOENT; inode = child->d_inode; if (unlikely(!inode)) - goto out_err; - if (!(open_flag & O_EXCL)) { + goto out_dput; + if (!(file->f_flags & O_EXCL)) { spin_lock(&inode->i_lock); inode->i_state |= I_LINKABLE; spin_unlock(&inode->i_lock); } ima_post_create_tmpfile(mnt_userns, inode); - return child; - -out_err: + error = 0; +out_dput: dput(child); - return ERR_PTR(error); +out: + return error; +} + +/** + * vfs_tmpfile - create tmpfile + * @mnt_userns: user namespace of the mount the inode was found from + * @dentry: pointer to dentry of the base directory + * @mode: mode of the new tmpfile + * @open_flag: flags + * + * Create a temporary file. + * + * If the inode has been found through an idmapped mount the user namespace of + * the vfsmount must be passed through @mnt_userns. This function will then take + * care to map the inode according to @mnt_userns before checking permissions. + * On non-idmapped mounts or if permission checking is to be performed on the + * raw inode simply passs init_user_ns. + */ +struct dentry *vfs_tmpfile(struct user_namespace *mnt_userns, + const struct path *path, umode_t mode, int open_flag) +{ + struct dentry *child; + struct file *file; + int error; + + file = alloc_empty_file(open_flag, current_cred()); + child = ERR_CAST(file); + if (!IS_ERR(file)) { + error = vfs_tmpfile_new(mnt_userns, path, file, mode); + child = error ? ERR_PTR(error) : dget(file->f_path.dentry); + fput(file); + } + return child; } EXPORT_SYMBOL(vfs_tmpfile); @@ -3629,26 +3648,22 @@ static int do_tmpfile(struct nameidata * struct file *file) { struct user_namespace *mnt_userns; - struct dentry *child; struct path path; - int error = path_lookupat(nd, flags | LOOKUP_DIRECTORY, &path); + int error; + + error = path_lookupat(nd, flags | LOOKUP_DIRECTORY, &path); if (unlikely(error)) return error; error = mnt_want_write(path.mnt); if (unlikely(error)) goto out; mnt_userns = mnt_user_ns(path.mnt); - child = vfs_tmpfile(mnt_userns, path.dentry, op->mode, op->open_flag); - error = PTR_ERR(child); - if (IS_ERR(child)) + error = vfs_tmpfile_new(mnt_userns, &path, file, op->mode); + if (error) goto out2; - dput(path.dentry); - path.dentry = child; - audit_inode(nd->name, child, 0); + audit_inode(nd->name, file->f_path.dentry, 0); /* Don't check for other permissions, the inode was just created */ - error = may_open(mnt_userns, &path, 0, op->open_flag); - if (!error) - error = vfs_open(&path, file); + error = may_open(mnt_userns, &file->f_path, 0, op->open_flag); out2: mnt_drop_write(path.mnt); out: --- a/fs/open.c +++ b/fs/open.c @@ -975,6 +975,13 @@ int finish_open(struct file *file, struc } EXPORT_SYMBOL(finish_open); +int finish_tmpfile(struct file *file) +{ + BUG_ON(file->f_mode & FMODE_OPENED); + return do_dentry_open(file, d_inode(file->f_path.dentry), NULL); +} + + /** * finish_no_open - finish ->atomic_open() without opening the file * --- a/fs/overlayfs/overlayfs.h +++ b/fs/overlayfs/overlayfs.h @@ -313,7 +313,8 @@ static inline int ovl_do_whiteout(struct static inline struct dentry *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); + struct path path = { .dentry = dentry, .mnt = ovl_upper_mnt(ofs) }; + struct dentry *ret = vfs_tmpfile(ovl_upper_mnt_userns(ofs), &path, mode, 0); int err = PTR_ERR_OR_ZERO(ret); pr_debug("tmpfile(%pd2, 0%o) = %i\n", dentry, mode, err); --- a/fs/ramfs/inode.c +++ b/fs/ramfs/inode.c @@ -146,15 +146,15 @@ static int ramfs_symlink(struct user_nam } static int ramfs_tmpfile(struct user_namespace *mnt_userns, - struct inode *dir, struct dentry *dentry, umode_t mode) + struct inode *dir, struct file *file, umode_t mode) { struct inode *inode; inode = ramfs_get_inode(dir->i_sb, dir, mode, 0); if (!inode) return -ENOSPC; - d_tmpfile(dentry, inode); - return 0; + d_tmpfile(file->f_path.dentry, inode); + return finish_tmpfile(file); } static const struct inode_operations ramfs_dir_inode_operations = { --- a/fs/ubifs/dir.c +++ b/fs/ubifs/dir.c @@ -424,8 +424,9 @@ static void unlock_2_inodes(struct inode } static int ubifs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir, - struct dentry *dentry, umode_t mode) + struct file *file, umode_t mode) { + struct dentry *dentry = file->f_path.dentry; struct inode *inode; struct ubifs_info *c = dir->i_sb->s_fs_info; struct ubifs_budget_req req = { .new_ino = 1, .new_dent = 1, @@ -489,7 +490,7 @@ static int ubifs_tmpfile(struct user_nam ubifs_release_budget(c, &req); - return 0; + return finish_tmpfile(file); out_cancel: unlock_2_inodes(dir, inode); --- a/fs/udf/namei.c +++ b/fs/udf/namei.c @@ -626,7 +626,7 @@ static int udf_create(struct user_namesp } static int udf_tmpfile(struct user_namespace *mnt_userns, struct inode *dir, - struct dentry *dentry, umode_t mode) + struct file *file, umode_t mode) { struct inode *inode = udf_new_inode(dir, mode); @@ -640,9 +640,9 @@ static int udf_tmpfile(struct user_names inode->i_op = &udf_file_inode_operations; inode->i_fop = &udf_file_operations; mark_inode_dirty(inode); - d_tmpfile(dentry, inode); + d_tmpfile(file->f_path.dentry, inode); unlock_new_inode(inode); - return 0; + return finish_tmpfile(file); } static int udf_mknod(struct user_namespace *mnt_userns, struct inode *dir, --- a/fs/xfs/xfs_iops.c +++ b/fs/xfs/xfs_iops.c @@ -1080,10 +1080,15 @@ STATIC int xfs_vn_tmpfile( struct user_namespace *mnt_userns, struct inode *dir, - struct dentry *dentry, + struct file *file, umode_t mode) { - return xfs_generic_create(mnt_userns, dir, dentry, mode, 0, true); + int err = xfs_generic_create(mnt_userns, dir, file->f_path.dentry, mode, 0, true); + + if (err) + return err; + + return finish_tmpfile(file); } static const struct inode_operations xfs_inode_operations = { --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2005,7 +2005,8 @@ static inline int vfs_whiteout(struct us } struct dentry *vfs_tmpfile(struct user_namespace *mnt_userns, - struct dentry *dentry, umode_t mode, int open_flag); + const struct path *path, + umode_t mode, int open_flag); int vfs_mkobj(struct dentry *, umode_t, int (*f)(struct dentry *, umode_t, void *), @@ -2167,7 +2168,7 @@ struct inode_operations { struct file *, unsigned open_flag, umode_t create_mode); int (*tmpfile) (struct user_namespace *, struct inode *, - struct dentry *, umode_t); + struct file *, umode_t); int (*set_acl)(struct user_namespace *, struct inode *, struct posix_acl *, int); int (*fileattr_set)(struct user_namespace *mnt_userns, @@ -2777,6 +2778,7 @@ extern void putname(struct filename *nam extern int finish_open(struct file *file, struct dentry *dentry, int (*open)(struct inode *, struct file *)); +extern int finish_tmpfile(struct file *file); extern int finish_no_open(struct file *file, struct dentry *dentry); /* fs/dcache.c */ --- a/mm/shmem.c +++ b/mm/shmem.c @@ -2912,7 +2912,7 @@ shmem_mknod(struct user_namespace *mnt_u static int shmem_tmpfile(struct user_namespace *mnt_userns, struct inode *dir, - struct dentry *dentry, umode_t mode) + struct file *file, umode_t mode) { struct inode *inode; int error = -ENOSPC; @@ -2927,12 +2927,16 @@ shmem_tmpfile(struct user_namespace *mnt error = simple_acl_create(dir, inode); if (error) goto out_iput; - d_tmpfile(dentry, inode); + d_tmpfile(file->f_path.dentry, inode); } - return error; +out: + if (error) + return error; + + return finish_tmpfile(file); out_iput: iput(inode); - return error; + goto out; } static int shmem_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
On Mon, Sep 5, 2022 at 7:25 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > On Wed, Aug 31, 2022 at 02:30:40PM -0700, Yu-li Lin wrote: > > Thanks for the reference. IIUC, the consensus is to make it atomic, > > although there's no agreement on how it should be done. Does that mean > > we should hold off on > > this patch until atomic temp files are figured out higher in the stack > > or do you have thoughts on how the fuse uapi should look like prior to > > the vfs/refactoring decision? > > Here's a patch refactoring the tmpfile kapi to return an open file instead of a > dentry. > > Comments? IDGI. Why did you need to place do_dentry_open() in all the implementations and not inside vfs_tmpfile_new()? Am I missing something? Thanks, Amir. > > Thanks, > Miklos > > --- > fs/bad_inode.c | 2 > fs/btrfs/inode.c | 12 +++-- > fs/cachefiles/namei.c | 3 - > fs/ext2/namei.c | 6 +- > fs/ext4/namei.c | 15 ++++-- > fs/f2fs/namei.c | 9 +++- > fs/hugetlbfs/inode.c | 9 +++- > fs/minix/namei.c | 9 ++-- > fs/namei.c | 101 ++++++++++++++++++++++++++--------------------- > fs/open.c | 7 +++ > fs/overlayfs/overlayfs.h | 3 - > fs/ramfs/inode.c | 6 +- > fs/ubifs/dir.c | 5 +- > fs/udf/namei.c | 6 +- > fs/xfs/xfs_iops.c | 9 +++- > include/linux/fs.h | 6 +- > mm/shmem.c | 12 +++-- > 17 files changed, 138 insertions(+), 82 deletions(-) > > --- a/fs/bad_inode.c > +++ b/fs/bad_inode.c > @@ -147,7 +147,7 @@ static int bad_inode_atomic_open(struct > } > > static int bad_inode_tmpfile(struct user_namespace *mnt_userns, > - struct inode *inode, struct dentry *dentry, > + struct inode *inode, struct file *file, > umode_t mode) > { > return -EIO; > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -10169,7 +10169,7 @@ static int btrfs_permission(struct user_ > } > > static int btrfs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir, > - struct dentry *dentry, umode_t mode) > + struct file *file, umode_t mode) > { > struct btrfs_fs_info *fs_info = btrfs_sb(dir->i_sb); > struct btrfs_trans_handle *trans; > @@ -10177,7 +10177,7 @@ static int btrfs_tmpfile(struct user_nam > struct inode *inode; > struct btrfs_new_inode_args new_inode_args = { > .dir = dir, > - .dentry = dentry, > + .dentry = file->f_path.dentry, > .orphan = true, > }; > unsigned int trans_num_items; > @@ -10214,7 +10214,7 @@ static int btrfs_tmpfile(struct user_nam > set_nlink(inode, 1); > > if (!ret) { > - d_tmpfile(dentry, inode); > + d_tmpfile(file->f_path.dentry, inode); > unlock_new_inode(inode); > mark_inode_dirty(inode); > } > @@ -10224,9 +10224,11 @@ static int btrfs_tmpfile(struct user_nam > out_new_inode_args: > btrfs_new_inode_args_destroy(&new_inode_args); > out_inode: > - if (ret) > + if (ret) { > iput(inode); > - return ret; > + return ret; > + } > + return finish_tmpfile(file); > } > > void btrfs_set_range_writeback(struct btrfs_inode *inode, u64 start, u64 end) > --- a/fs/cachefiles/namei.c > +++ b/fs/cachefiles/namei.c > @@ -459,9 +459,10 @@ struct file *cachefiles_create_tmpfile(s > cachefiles_begin_secure(cache, &saved_cred); > > path.mnt = cache->mnt; > + path.dentry = fan; > ret = cachefiles_inject_write_error(); > if (ret == 0) > - path.dentry = vfs_tmpfile(&init_user_ns, fan, S_IFREG, O_RDWR); > + path.dentry = vfs_tmpfile(&init_user_ns, &path, S_IFREG, O_RDWR); > else > path.dentry = ERR_PTR(ret); > if (IS_ERR(path.dentry)) { > --- a/fs/ext2/namei.c > +++ b/fs/ext2/namei.c > @@ -120,7 +120,7 @@ static int ext2_create (struct user_name > } > > static int ext2_tmpfile(struct user_namespace *mnt_userns, struct inode *dir, > - struct dentry *dentry, umode_t mode) > + struct file *file, umode_t mode) > { > struct inode *inode = ext2_new_inode(dir, mode, NULL); > if (IS_ERR(inode)) > @@ -128,9 +128,9 @@ static int ext2_tmpfile(struct user_name > > ext2_set_file_ops(inode); > mark_inode_dirty(inode); > - d_tmpfile(dentry, inode); > + d_tmpfile(file->f_path.dentry, inode); > unlock_new_inode(inode); > - return 0; > + return finish_tmpfile(file); > } > > static int ext2_mknod (struct user_namespace * mnt_userns, struct inode * dir, > --- a/fs/ext4/namei.c > +++ b/fs/ext4/namei.c > @@ -2849,7 +2849,7 @@ static int ext4_mknod(struct user_namesp > } > > static int ext4_tmpfile(struct user_namespace *mnt_userns, struct inode *dir, > - struct dentry *dentry, umode_t mode) > + struct file *file, umode_t mode) > { > handle_t *handle; > struct inode *inode; > @@ -2857,7 +2857,7 @@ static int ext4_tmpfile(struct user_name > > err = dquot_initialize(dir); > if (err) > - return err; > + goto out; > > retry: > inode = ext4_new_inode_start_handle(mnt_userns, dir, mode, > @@ -2871,7 +2871,7 @@ static int ext4_tmpfile(struct user_name > inode->i_op = &ext4_file_inode_operations; > inode->i_fop = &ext4_file_operations; > ext4_set_aops(inode); > - d_tmpfile(dentry, inode); > + d_tmpfile(file->f_path.dentry, inode); > err = ext4_orphan_add(handle, inode); > if (err) > goto err_unlock_inode; > @@ -2882,11 +2882,16 @@ static int ext4_tmpfile(struct user_name > ext4_journal_stop(handle); > if (err == -ENOSPC && ext4_should_retry_alloc(dir->i_sb, &retries)) > goto retry; > - return err; > +out: > + if (err) > + return err; > + > + return finish_tmpfile(file); > + > err_unlock_inode: > ext4_journal_stop(handle); > unlock_new_inode(inode); > - return err; > + goto out; > } > > struct ext4_dir_entry_2 *ext4_init_dot_dotdot(struct inode *inode, > --- a/fs/f2fs/namei.c > +++ b/fs/f2fs/namei.c > @@ -915,16 +915,21 @@ static int __f2fs_tmpfile(struct user_na > } > > static int f2fs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir, > - struct dentry *dentry, umode_t mode) > + struct file *file, umode_t mode) > { > struct f2fs_sb_info *sbi = F2FS_I_SB(dir); > + int err; > > if (unlikely(f2fs_cp_error(sbi))) > return -EIO; > if (!f2fs_is_checkpoint_ready(sbi)) > return -ENOSPC; > > - return __f2fs_tmpfile(mnt_userns, dir, dentry, mode, false, NULL); > + err = __f2fs_tmpfile(mnt_userns, dir, file->f_path.dentry, mode, false, NULL); > + if (err) > + return err; > + > + return finish_tmpfile(file); > } > > static int f2fs_create_whiteout(struct user_namespace *mnt_userns, > --- a/fs/hugetlbfs/inode.c > +++ b/fs/hugetlbfs/inode.c > @@ -932,10 +932,15 @@ static int hugetlbfs_create(struct user_ > } > > static int hugetlbfs_tmpfile(struct user_namespace *mnt_userns, > - struct inode *dir, struct dentry *dentry, > + struct inode *dir, struct file *file, > umode_t mode) > { > - return do_hugetlbfs_mknod(dir, dentry, mode | S_IFREG, 0, true); > + int err = do_hugetlbfs_mknod(dir, file->f_path.dentry, mode | S_IFREG, 0, true); > + > + if (err) > + return err; > + > + return finish_tmpfile(file); > } > > static int hugetlbfs_symlink(struct user_namespace *mnt_userns, > --- a/fs/minix/namei.c > +++ b/fs/minix/namei.c > @@ -53,16 +53,19 @@ static int minix_mknod(struct user_names > } > > static int minix_tmpfile(struct user_namespace *mnt_userns, struct inode *dir, > - struct dentry *dentry, umode_t mode) > + struct file *file, umode_t mode) > { > int error; > struct inode *inode = minix_new_inode(dir, mode, &error); > if (inode) { > minix_set_inode(inode, 0); > mark_inode_dirty(inode); > - d_tmpfile(dentry, inode); > + d_tmpfile(file->f_path.dentry, inode); > } > - return error; > + if (error) > + return error; > + > + return finish_tmpfile(file); > } > > static int minix_create(struct user_namespace *mnt_userns, struct inode *dir, > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -3568,59 +3568,78 @@ static int do_open(struct nameidata *nd, > return error; > } > > -/** > - * vfs_tmpfile - create tmpfile > - * @mnt_userns: user namespace of the mount the inode was found from > - * @dentry: pointer to dentry of the base directory > - * @mode: mode of the new tmpfile > - * @open_flag: flags > - * > - * Create a temporary file. > - * > - * If the inode has been found through an idmapped mount the user namespace of > - * the vfsmount must be passed through @mnt_userns. This function will then take > - * care to map the inode according to @mnt_userns before checking permissions. > - * On non-idmapped mounts or if permission checking is to be performed on the > - * raw inode simply passs init_user_ns. > - */ > -struct dentry *vfs_tmpfile(struct user_namespace *mnt_userns, > - struct dentry *dentry, umode_t mode, int open_flag) > +static int vfs_tmpfile_new(struct user_namespace *mnt_userns, > + const struct path *parent_path, > + struct file *file, umode_t mode) > { > - struct dentry *child = NULL; > - struct inode *dir = dentry->d_inode; > - struct inode *inode; > + struct inode *inode, *dir = d_inode(parent_path->dentry); > + struct dentry *child; > int error; > > /* we want directory to be writable */ > error = inode_permission(mnt_userns, dir, MAY_WRITE | MAY_EXEC); > if (error) > - goto out_err; > + goto out; > error = -EOPNOTSUPP; > if (!dir->i_op->tmpfile) > - goto out_err; > + goto out; > error = -ENOMEM; > - child = d_alloc(dentry, &slash_name); > + child = d_alloc(parent_path->dentry, &slash_name); > if (unlikely(!child)) > - goto out_err; > + goto out; > + file->f_path.mnt = parent_path->mnt; > + file->f_path.dentry = child; > mode = vfs_prepare_mode(mnt_userns, dir, mode, mode, mode); > - error = dir->i_op->tmpfile(mnt_userns, dir, child, mode); > + error = dir->i_op->tmpfile(mnt_userns, dir, file, mode); > if (error) > - goto out_err; > + goto out_dput; > error = -ENOENT; > inode = child->d_inode; > if (unlikely(!inode)) > - goto out_err; > - if (!(open_flag & O_EXCL)) { > + goto out_dput; > + if (!(file->f_flags & O_EXCL)) { > spin_lock(&inode->i_lock); > inode->i_state |= I_LINKABLE; > spin_unlock(&inode->i_lock); > } > ima_post_create_tmpfile(mnt_userns, inode); > - return child; > - > -out_err: > + error = 0; > +out_dput: > dput(child); > - return ERR_PTR(error); > +out: > + return error; > +} > + > +/** > + * vfs_tmpfile - create tmpfile > + * @mnt_userns: user namespace of the mount the inode was found from > + * @dentry: pointer to dentry of the base directory > + * @mode: mode of the new tmpfile > + * @open_flag: flags > + * > + * Create a temporary file. > + * > + * If the inode has been found through an idmapped mount the user namespace of > + * the vfsmount must be passed through @mnt_userns. This function will then take > + * care to map the inode according to @mnt_userns before checking permissions. > + * On non-idmapped mounts or if permission checking is to be performed on the > + * raw inode simply passs init_user_ns. > + */ > +struct dentry *vfs_tmpfile(struct user_namespace *mnt_userns, > + const struct path *path, umode_t mode, int open_flag) > +{ > + struct dentry *child; > + struct file *file; > + int error; > + > + file = alloc_empty_file(open_flag, current_cred()); > + child = ERR_CAST(file); > + if (!IS_ERR(file)) { > + error = vfs_tmpfile_new(mnt_userns, path, file, mode); > + child = error ? ERR_PTR(error) : dget(file->f_path.dentry); > + fput(file); > + } > + return child; > } > EXPORT_SYMBOL(vfs_tmpfile); > > @@ -3629,26 +3648,22 @@ static int do_tmpfile(struct nameidata * > struct file *file) > { > struct user_namespace *mnt_userns; > - struct dentry *child; > struct path path; > - int error = path_lookupat(nd, flags | LOOKUP_DIRECTORY, &path); > + int error; > + > + error = path_lookupat(nd, flags | LOOKUP_DIRECTORY, &path); > if (unlikely(error)) > return error; > error = mnt_want_write(path.mnt); > if (unlikely(error)) > goto out; > mnt_userns = mnt_user_ns(path.mnt); > - child = vfs_tmpfile(mnt_userns, path.dentry, op->mode, op->open_flag); > - error = PTR_ERR(child); > - if (IS_ERR(child)) > + error = vfs_tmpfile_new(mnt_userns, &path, file, op->mode); > + if (error) > goto out2; > - dput(path.dentry); > - path.dentry = child; > - audit_inode(nd->name, child, 0); > + audit_inode(nd->name, file->f_path.dentry, 0); > /* Don't check for other permissions, the inode was just created */ > - error = may_open(mnt_userns, &path, 0, op->open_flag); > - if (!error) > - error = vfs_open(&path, file); > + error = may_open(mnt_userns, &file->f_path, 0, op->open_flag); > out2: > mnt_drop_write(path.mnt); > out: > --- a/fs/open.c > +++ b/fs/open.c > @@ -975,6 +975,13 @@ int finish_open(struct file *file, struc > } > EXPORT_SYMBOL(finish_open); > > +int finish_tmpfile(struct file *file) > +{ > + BUG_ON(file->f_mode & FMODE_OPENED); > + return do_dentry_open(file, d_inode(file->f_path.dentry), NULL); > +} > + > + > /** > * finish_no_open - finish ->atomic_open() without opening the file > * > --- a/fs/overlayfs/overlayfs.h > +++ b/fs/overlayfs/overlayfs.h > @@ -313,7 +313,8 @@ static inline int ovl_do_whiteout(struct > static inline struct dentry *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); > + struct path path = { .dentry = dentry, .mnt = ovl_upper_mnt(ofs) }; > + struct dentry *ret = vfs_tmpfile(ovl_upper_mnt_userns(ofs), &path, mode, 0); > int err = PTR_ERR_OR_ZERO(ret); > > pr_debug("tmpfile(%pd2, 0%o) = %i\n", dentry, mode, err); > --- a/fs/ramfs/inode.c > +++ b/fs/ramfs/inode.c > @@ -146,15 +146,15 @@ static int ramfs_symlink(struct user_nam > } > > static int ramfs_tmpfile(struct user_namespace *mnt_userns, > - struct inode *dir, struct dentry *dentry, umode_t mode) > + struct inode *dir, struct file *file, umode_t mode) > { > struct inode *inode; > > inode = ramfs_get_inode(dir->i_sb, dir, mode, 0); > if (!inode) > return -ENOSPC; > - d_tmpfile(dentry, inode); > - return 0; > + d_tmpfile(file->f_path.dentry, inode); > + return finish_tmpfile(file); > } > > static const struct inode_operations ramfs_dir_inode_operations = { > --- a/fs/ubifs/dir.c > +++ b/fs/ubifs/dir.c > @@ -424,8 +424,9 @@ static void unlock_2_inodes(struct inode > } > > static int ubifs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir, > - struct dentry *dentry, umode_t mode) > + struct file *file, umode_t mode) > { > + struct dentry *dentry = file->f_path.dentry; > struct inode *inode; > struct ubifs_info *c = dir->i_sb->s_fs_info; > struct ubifs_budget_req req = { .new_ino = 1, .new_dent = 1, > @@ -489,7 +490,7 @@ static int ubifs_tmpfile(struct user_nam > > ubifs_release_budget(c, &req); > > - return 0; > + return finish_tmpfile(file); > > out_cancel: > unlock_2_inodes(dir, inode); > --- a/fs/udf/namei.c > +++ b/fs/udf/namei.c > @@ -626,7 +626,7 @@ static int udf_create(struct user_namesp > } > > static int udf_tmpfile(struct user_namespace *mnt_userns, struct inode *dir, > - struct dentry *dentry, umode_t mode) > + struct file *file, umode_t mode) > { > struct inode *inode = udf_new_inode(dir, mode); > > @@ -640,9 +640,9 @@ static int udf_tmpfile(struct user_names > inode->i_op = &udf_file_inode_operations; > inode->i_fop = &udf_file_operations; > mark_inode_dirty(inode); > - d_tmpfile(dentry, inode); > + d_tmpfile(file->f_path.dentry, inode); > unlock_new_inode(inode); > - return 0; > + return finish_tmpfile(file); > } > > static int udf_mknod(struct user_namespace *mnt_userns, struct inode *dir, > --- a/fs/xfs/xfs_iops.c > +++ b/fs/xfs/xfs_iops.c > @@ -1080,10 +1080,15 @@ STATIC int > xfs_vn_tmpfile( > struct user_namespace *mnt_userns, > struct inode *dir, > - struct dentry *dentry, > + struct file *file, > umode_t mode) > { > - return xfs_generic_create(mnt_userns, dir, dentry, mode, 0, true); > + int err = xfs_generic_create(mnt_userns, dir, file->f_path.dentry, mode, 0, true); > + > + if (err) > + return err; > + > + return finish_tmpfile(file); > } > > static const struct inode_operations xfs_inode_operations = { > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -2005,7 +2005,8 @@ static inline int vfs_whiteout(struct us > } > > struct dentry *vfs_tmpfile(struct user_namespace *mnt_userns, > - struct dentry *dentry, umode_t mode, int open_flag); > + const struct path *path, > + umode_t mode, int open_flag); > > int vfs_mkobj(struct dentry *, umode_t, > int (*f)(struct dentry *, umode_t, void *), > @@ -2167,7 +2168,7 @@ struct inode_operations { > struct file *, unsigned open_flag, > umode_t create_mode); > int (*tmpfile) (struct user_namespace *, struct inode *, > - struct dentry *, umode_t); > + struct file *, umode_t); > int (*set_acl)(struct user_namespace *, struct inode *, > struct posix_acl *, int); > int (*fileattr_set)(struct user_namespace *mnt_userns, > @@ -2777,6 +2778,7 @@ extern void putname(struct filename *nam > > extern int finish_open(struct file *file, struct dentry *dentry, > int (*open)(struct inode *, struct file *)); > +extern int finish_tmpfile(struct file *file); > extern int finish_no_open(struct file *file, struct dentry *dentry); > > /* fs/dcache.c */ > --- a/mm/shmem.c > +++ b/mm/shmem.c > @@ -2912,7 +2912,7 @@ shmem_mknod(struct user_namespace *mnt_u > > static int > shmem_tmpfile(struct user_namespace *mnt_userns, struct inode *dir, > - struct dentry *dentry, umode_t mode) > + struct file *file, umode_t mode) > { > struct inode *inode; > int error = -ENOSPC; > @@ -2927,12 +2927,16 @@ shmem_tmpfile(struct user_namespace *mnt > error = simple_acl_create(dir, inode); > if (error) > goto out_iput; > - d_tmpfile(dentry, inode); > + d_tmpfile(file->f_path.dentry, inode); > } > - return error; > +out: > + if (error) > + return error; > + > + return finish_tmpfile(file); > out_iput: > iput(inode); > - return error; > + goto out; > } > > static int shmem_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
On Tue, Sep 06, 2022 at 07:58:50AM +0300, Amir Goldstein wrote: > On Mon, Sep 5, 2022 at 7:25 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > > > On Wed, Aug 31, 2022 at 02:30:40PM -0700, Yu-li Lin wrote: > > > Thanks for the reference. IIUC, the consensus is to make it atomic, > > > although there's no agreement on how it should be done. Does that mean > > > we should hold off on > > > this patch until atomic temp files are figured out higher in the stack > > > or do you have thoughts on how the fuse uapi should look like prior to > > > the vfs/refactoring decision? > > > > Here's a patch refactoring the tmpfile kapi to return an open file instead of a > > dentry. > > > > Comments? > > IDGI. Why did you need to place do_dentry_open() in all the implementations > and not inside vfs_tmpfile_new()? > Am I missing something? The whole point of that horror is to have open done inside ->tmpfile() instances... Al, very unhappy with proposed interface ;-/
On Tue, 6 Sept 2022 at 07:29, Al Viro <viro@zeniv.linux.org.uk> wrote: > > On Tue, Sep 06, 2022 at 07:58:50AM +0300, Amir Goldstein wrote: > > On Mon, Sep 5, 2022 at 7:25 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > > > > > On Wed, Aug 31, 2022 at 02:30:40PM -0700, Yu-li Lin wrote: > > > > Thanks for the reference. IIUC, the consensus is to make it atomic, > > > > although there's no agreement on how it should be done. Does that mean > > > > we should hold off on > > > > this patch until atomic temp files are figured out higher in the stack > > > > or do you have thoughts on how the fuse uapi should look like prior to > > > > the vfs/refactoring decision? > > > > > > Here's a patch refactoring the tmpfile kapi to return an open file instead of a > > > dentry. > > > > > > Comments? > > > > IDGI. Why did you need to place do_dentry_open() in all the implementations > > and not inside vfs_tmpfile_new()? > > Am I missing something? > > The whole point of that horror is to have open done inside ->tmpfile() > instances... > > Al, very unhappy with proposed interface ;-/ So what does Al propose instead? Thanks, Miklos
On Mon, Sep 05, 2022 at 05:51:38PM +0200, Miklos Szeredi wrote: > On Wed, Aug 31, 2022 at 02:30:40PM -0700, Yu-li Lin wrote: > > Thanks for the reference. IIUC, the consensus is to make it atomic, > > although there's no agreement on how it should be done. Does that mean > > we should hold off on > > this patch until atomic temp files are figured out higher in the stack > > or do you have thoughts on how the fuse uapi should look like prior to > > the vfs/refactoring decision? > > Here's a patch refactoring the tmpfile kapi to return an open file instead of a > dentry. > > Comments? First and foremost: how many operations the interested filesystems (cifs, for one) are capable of for such beasts? I can believe that FUSE can handle that, but can something like cifs do it? Their protocol is pathname-based, IIRC; can they really handle that kind of files without something like sillyrename? Because if sillyrename and its analogues are in the game, we have seriously changed rules re directory locking, and we'd better figure that out first. IOW, I would really like to see proposed instances for FUSE and CIFS - the shape of the series seriously depends upon that. Before we commit to calling conventions changes. That aside, some notes on the patch: * file->f_path.dentry all over the place is ugly and wrong. If nothing else, d_tmpfile() could be converted to taking struct file * - nothing outside of ->tmpfile() instances is using it. * finish_tmpfile() parts would be less noisy if it was finish_tmpfile(file, errror), starting with if (error) return error; * a bit of weirdness in ext4: > err = dquot_initialize(dir); > if (err) > - return err; > + goto out; Huh? Your out: starts with if (err) return err; Sure, compiler will figure it out, but what have human readers done to you? * similar in shmem: > +out: > + if (error) > + return error; > + > + return finish_tmpfile(file); > out_iput: > iput(inode); > - return error; > + goto out; How the hell would it ever get to out_iput: with error == 0? * in your vfs_tmpfile() wrapper > + child = ERR_CAST(file); > + if (!IS_ERR(file)) { > + error = vfs_tmpfile_new(mnt_userns, path, file, mode); > + child = error ? ERR_PTR(error) : dget(file->f_path.dentry); > + fput(file); > + } > + return child; This really ought to be if (IS_ERR(file)) return ERR_CAST(file); ... IS_ERR() comes with implicit unlikely()... Next, there are users outside of do_o_tmpfile(). The one in cachefiles is a prime candidate for combining with open - the stuff done between vfs_tmpfile() and opening the sucker consists of * cachefiles_ondemand_init_object() - takes no cleanup on later failure exits, doesn't know anything about dentry created. Might as well be done before vfs_tmpfile(). * cachefiles_mark_inode_in_use() - which really can't fail there, and the only thing being done is marking the sucker with S_KERNEL_FILE. Could be done after opening just as well. * vfs_truncate() used to expand to required size. Trivially done after opening, and we probably want struct file there - there's a reason why ftruncate(2) sets ATTR_FILE/ia_file... We are unlikely to use anything like FUSE for cachefiles, but leaving traps like that is a bad idea. IOW, cachefiles probably wants a preliminary patch series that would massage it to the "tmpfile right next to open, the use struct file" form. Another user is overlayfs, and that's going to get painful. It checks for ->tmpfile() working and if it does work, we use it for copyup. The trouble is, will e.g. FUSE be ready to handle things like setxattr on such dentries? It's not just a matter of copying the data - there we would be quite fine with opened file; it's somewhat clumsy since copyup is done for FIFOs, etc., but that could be dealt with. But things like setting metadata are done by dentry there. And with your patch the file will have been closed by the time we get to that part. Can e.g. FUSE deal with that?
On Tue, 13 Sept 2022 at 03:51, Al Viro <viro@zeniv.linux.org.uk> wrote: > > On Mon, Sep 05, 2022 at 05:51:38PM +0200, Miklos Szeredi wrote: > > On Wed, Aug 31, 2022 at 02:30:40PM -0700, Yu-li Lin wrote: > > > Thanks for the reference. IIUC, the consensus is to make it atomic, > > > although there's no agreement on how it should be done. Does that mean > > > we should hold off on > > > this patch until atomic temp files are figured out higher in the stack > > > or do you have thoughts on how the fuse uapi should look like prior to > > > the vfs/refactoring decision? > > > > Here's a patch refactoring the tmpfile kapi to return an open file instead of a > > dentry. > > > > Comments? > > First and foremost: how many operations the interested filesystems (cifs, > for one) are capable of for such beasts? I can believe that FUSE can handle > that, but can something like cifs do it? Their protocol is pathname-based, IIRC; > can they really handle that kind of files without something like sillyrename? > Because if sillyrename and its analogues are in the game, we have seriously > changed rules re directory locking, and we'd better figure that out first. > > IOW, I would really like to see proposed instances for FUSE and CIFS - the shape > of the series seriously depends upon that. Before we commit to calling conventions > changes. Was there a requirement for CIFS supporting tmpfile? Is so where? > > That aside, some notes on the patch: > > * file->f_path.dentry all over the place is ugly and wrong. If nothing else, > d_tmpfile() could be converted to taking struct file * - nothing outside of > ->tmpfile() instances is using it. > * finish_tmpfile() parts would be less noisy if it was finish_tmpfile(file, errror), > starting with if (error) return error; > * a bit of weirdness in ext4: > > err = dquot_initialize(dir); > > if (err) > > - return err; > > + goto out; > Huh? Your out: starts with if (err) return err; Sure, compiler > will figure it out, but what have human readers done to you? > * similar in shmem: > > +out: > > + if (error) > > + return error; > > + > > + return finish_tmpfile(file); > > out_iput: > > iput(inode); > > - return error; > > + goto out; > How the hell would it ever get to out_iput: with error == 0? > * in your vfs_tmpfile() wrapper > > + child = ERR_CAST(file); > > + if (!IS_ERR(file)) { > > + error = vfs_tmpfile_new(mnt_userns, path, file, mode); > > + child = error ? ERR_PTR(error) : dget(file->f_path.dentry); > > + fput(file); > > + } > > + return child; > This really ought to be > if (IS_ERR(file)) > return ERR_CAST(file); > ... > IS_ERR() comes with implicit unlikely()... > Agreed with all of the above. > > Next, there are users outside of do_o_tmpfile(). The one in cachefiles > is a prime candidate for combining with open - the stuff done between > vfs_tmpfile() and opening the sucker consists of > * cachefiles_ondemand_init_object() - takes no cleanup on > later failure exits, doesn't know anything about dentry created. Might > as well be done before vfs_tmpfile(). > * cachefiles_mark_inode_in_use() - which really can't fail there, > and the only thing being done is marking the sucker with S_KERNEL_FILE. > Could be done after opening just as well. > * vfs_truncate() used to expand to required size. Trivially done > after opening, and we probably want struct file there - there's a reason > why ftruncate(2) sets ATTR_FILE/ia_file... We are unlikely to use > anything like FUSE for cachefiles, but leaving traps like that is a bad > idea. > IOW, cachefiles probably wants a preliminary patch series that would > massage it to the "tmpfile right next to open, the use struct file" > form. Sure, that would have been the next step after the API is settled. > Another user is overlayfs, and that's going to get painful. It checks for > ->tmpfile() working and if it does work, we use it for copyup. The trouble > is, will e.g. FUSE be ready to handle things like setxattr on such dentries? > It's not just a matter of copying the data - there we would be quite fine > with opened file; it's somewhat clumsy since copyup is done for FIFOs, etc., > but that could be dealt with. But things like setting metadata are done > by dentry there. And with your patch the file will have been closed by > the time we get to that part. Can e.g. FUSE deal with that? At the moment FUSE is not supported as upper layer, so it's a non-issue. But I don't see why copy-up can't be fixed to keep the tmp file open. In fact that's something I also want to do once the interface is agreed upon. Thanks, Miklos
Hi, for your information, as developer of a FUSE filesystem using the SFTP protocol (over SSH) there is an extension : https://datatracker.ietf.org/doc/html/draft-ietf-secsh-filexfer-extensions-00#section-8 to create and get the temporary folder on the server. Maybe it's useful. Stef Bon https://github.com/stefbon/OSNS https://osns.net/
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c index ff7dbeb16f88d..1ab52e7ec1625 100644 --- a/fs/fuse/dir.c +++ b/fs/fuse/dir.c @@ -751,6 +751,26 @@ static int fuse_mkdir(struct inode *dir, struct dentry *entry, umode_t mode) return create_new_entry(fm, &args, dir, entry, S_IFDIR); } +static int fuse_tmpfile(struct inode *dir, struct dentry *entry, umode_t mode) +{ + struct fuse_tmpfile_in inarg; + struct fuse_mount *fm = get_fuse_mount(dir); + FUSE_ARGS(args); + + if (!fm->fc->dont_mask) + mode &= ~current_umask(); + + memset(&inarg, 0, sizeof(inarg)); + inarg.mode = mode; + inarg.umask = current_umask(); + args.opcode = FUSE_TMPFILE; + args.in_numargs = 1; + args.in_args[0].size = sizeof(inarg); + args.in_args[0].value = &inarg; + + return create_new_entry(fm, &args, dir, entry, S_IFREG); +} + static int fuse_symlink(struct inode *dir, struct dentry *entry, const char *link) { @@ -1818,6 +1838,7 @@ static const struct inode_operations fuse_dir_inode_operations = { .listxattr = fuse_listxattr, .get_acl = fuse_get_acl, .set_acl = fuse_set_acl, + .tmpfile = fuse_tmpfile, }; static const struct file_operations fuse_dir_operations = { diff --git a/fs/fuse/file.c b/fs/fuse/file.c index c03034e8c1529..8ecf85699a014 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -39,7 +39,8 @@ static int fuse_send_open(struct fuse_mount *fm, u64 nodeid, struct file *file, FUSE_ARGS(args); memset(&inarg, 0, sizeof(inarg)); - inarg.flags = file->f_flags & ~(O_CREAT | O_EXCL | O_NOCTTY); + inarg.flags = + file->f_flags & ~(O_CREAT | O_EXCL | O_NOCTTY | O_TMPFILE); if (!fm->fc->atomic_o_trunc) inarg.flags &= ~O_TRUNC; args.opcode = opcode;
Implement support for O_TMPFILE by re-using the existing infrastructure for mkdir, symlink, mknod, etc. The server should reply to the tmpfile request by sending a fuse_entry_out describing the newly created tmpfile. Signed-off-by: Chirantan Ekbote <chirantan@chromium.org> --- fs/fuse/dir.c | 21 +++++++++++++++++++++ fs/fuse/file.c | 3 ++- 2 files changed, 23 insertions(+), 1 deletion(-)