| Message ID | 20220920193632.2215598-5-mszeredi@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuse tmpfile | expand |
On Tue, Sep 20, 2022 at 09:36:27PM +0200, Miklos Szeredi wrote: > Use the tmpfile_open() helper instead of doing tmpfile creation and opening > separately. > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > --- > fs/cachefiles/namei.c | 26 ++++++++++---------------- > 1 file changed, 10 insertions(+), 16 deletions(-) > > diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c > index d3a5884fe5c9..44f575328af4 100644 > --- a/fs/cachefiles/namei.c > +++ b/fs/cachefiles/namei.c > @@ -451,18 +451,19 @@ struct file *cachefiles_create_tmpfile(struct cachefiles_object *object) > const struct cred *saved_cred; > struct dentry *fan = volume->fanout[(u8)object->cookie->key_hash]; > struct file *file; > - struct path path; > + struct path path = { .mnt = cache->mnt, .dentry = fan }; > uint64_t ni_size; > long ret; Maybe we shouldn't use struct path to first refer to the parent path and then to the tmp path to avoid any potential confusion and instead rely on a compount initializer for the tmpfile_open() call (__not tested__)?: diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c index 44f575328af4..979b2f173ac3 100644 --- a/fs/cachefiles/namei.c +++ b/fs/cachefiles/namei.c @@ -451,7 +451,7 @@ struct file *cachefiles_create_tmpfile(struct cachefiles_object *object) const struct cred *saved_cred; struct dentry *fan = volume->fanout[(u8)object->cookie->key_hash]; struct file *file; - struct path path = { .mnt = cache->mnt, .dentry = fan }; + struct path path; uint64_t ni_size; long ret; @@ -460,8 +460,10 @@ struct file *cachefiles_create_tmpfile(struct cachefiles_object *object) ret = cachefiles_inject_write_error(); if (ret == 0) { - file = tmpfile_open(&init_user_ns, &path, S_IFREG, - O_RDWR | O_LARGEFILE | O_DIRECT, + file = tmpfile_open(&init_user_ns, + &{const struct path} {.mnt = cache->mnt, + .dentry = fan}, + S_IFREG, O_RDWR | O_LARGEFILE | O_DIRECT, cache->cache_cred); ret = PTR_ERR_OR_ZERO(file); } @@ -472,7 +474,9 @@ struct file *cachefiles_create_tmpfile(struct cachefiles_object *object) cachefiles_io_error_obj(object, "Failed to create tmpfile"); goto err; } - /* From now path refers to the tmpfile */ + + /* prepare tmp path */ + path.mnt = cache->mnt; path.dentry = file->f_path.dentry; trace_cachefiles_tmpfile(object, d_backing_inode(path.dentry));
On Wed, 21 Sept 2022 at 10:27, Christian Brauner <brauner@kernel.org> wrote: > > On Tue, Sep 20, 2022 at 09:36:27PM +0200, Miklos Szeredi wrote: > > Use the tmpfile_open() helper instead of doing tmpfile creation and opening > > separately. > > > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > > --- > > fs/cachefiles/namei.c | 26 ++++++++++---------------- > > 1 file changed, 10 insertions(+), 16 deletions(-) > > > > diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c > > index d3a5884fe5c9..44f575328af4 100644 > > --- a/fs/cachefiles/namei.c > > +++ b/fs/cachefiles/namei.c > > @@ -451,18 +451,19 @@ struct file *cachefiles_create_tmpfile(struct cachefiles_object *object) > > const struct cred *saved_cred; > > struct dentry *fan = volume->fanout[(u8)object->cookie->key_hash]; > > struct file *file; > > - struct path path; > > + struct path path = { .mnt = cache->mnt, .dentry = fan }; > > uint64_t ni_size; > > long ret; > > Maybe we shouldn't use struct path to first refer to the parent path and > then to the tmp path to avoid any potential confusion and instead rely > on a compount initializer for the tmpfile_open() call (__not tested__)?: > > diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c > index 44f575328af4..979b2f173ac3 100644 > --- a/fs/cachefiles/namei.c > +++ b/fs/cachefiles/namei.c > @@ -451,7 +451,7 @@ struct file *cachefiles_create_tmpfile(struct cachefiles_object *object) > const struct cred *saved_cred; > struct dentry *fan = volume->fanout[(u8)object->cookie->key_hash]; > struct file *file; > - struct path path = { .mnt = cache->mnt, .dentry = fan }; > + struct path path; > uint64_t ni_size; > long ret; > > @@ -460,8 +460,10 @@ struct file *cachefiles_create_tmpfile(struct cachefiles_object *object) > > ret = cachefiles_inject_write_error(); > if (ret == 0) { > - file = tmpfile_open(&init_user_ns, &path, S_IFREG, > - O_RDWR | O_LARGEFILE | O_DIRECT, > + file = tmpfile_open(&init_user_ns, > + &{const struct path} {.mnt = cache->mnt, > + .dentry = fan}, This doesn't look nice. I fixed it with a separate "parentpath" variable. Thanks, Miklos
On Wed, Sep 21, 2022 at 10:26:12AM +0200, Christian Brauner wrote: > - /* From now path refers to the tmpfile */ > + > + /* prepare tmp path */ > + path.mnt = cache->mnt; > path.dentry = file->f_path.dentry; Do we even want that struct path from that point on? Look: d_backing_inode(path.dentry) is a weird way to spell file_inode(file). cachefiles_mark_inode_in_use() is an overkill here - it *can't* fail here, so all we want is inode_lock(inode); inode->i_flags |= S_KERNEL_FILE; trace_cachefiles_mark_active(object, inode); inode_unlock(inode); where inode is, again, file_inode(file). cachefiles_do_unmark_inode_in_use() uses only inode. vfs_truncate() could use &file->f_path, but there's a potentially nastier problem - theoretically, there are filesystems where we might want struct file available for operation, especially for opened-and-unlinked equivalents. In any case, &file->f_path would do just as well as its copy.
On Wed, Sep 21, 2022 at 08:46:20PM +0100, Al Viro wrote: > On Wed, Sep 21, 2022 at 10:26:12AM +0200, Christian Brauner wrote: > > - /* From now path refers to the tmpfile */ > > + > > + /* prepare tmp path */ > > + path.mnt = cache->mnt; > > path.dentry = file->f_path.dentry; > > Do we even want that struct path from that point on? Look: > > d_backing_inode(path.dentry) is a weird way to spell file_inode(file). > > cachefiles_mark_inode_in_use() is an overkill here - it *can't* fail > here, so all we want is > inode_lock(inode); > inode->i_flags |= S_KERNEL_FILE; > trace_cachefiles_mark_active(object, inode); > inode_unlock(inode); > where inode is, again, file_inode(file). > > cachefiles_do_unmark_inode_in_use() uses only inode. > > vfs_truncate() could use &file->f_path, but there's a potentially > nastier problem - theoretically, there are filesystems where we might > want struct file available for operation, especially for opened-and-unlinked > equivalents. In any case, &file->f_path would do just as well as its copy. Yeah, sounds reasonable.
diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c index d3a5884fe5c9..44f575328af4 100644 --- a/fs/cachefiles/namei.c +++ b/fs/cachefiles/namei.c @@ -451,18 +451,19 @@ struct file *cachefiles_create_tmpfile(struct cachefiles_object *object) const struct cred *saved_cred; struct dentry *fan = volume->fanout[(u8)object->cookie->key_hash]; struct file *file; - struct path path; + struct path path = { .mnt = cache->mnt, .dentry = fan }; uint64_t ni_size; long ret; cachefiles_begin_secure(cache, &saved_cred); - path.mnt = cache->mnt; ret = cachefiles_inject_write_error(); if (ret == 0) { - path.dentry = vfs_tmpfile(&init_user_ns, fan, S_IFREG, O_RDWR); - ret = PTR_ERR_OR_ZERO(path.dentry); + file = tmpfile_open(&init_user_ns, &path, S_IFREG, + O_RDWR | O_LARGEFILE | O_DIRECT, + cache->cache_cred); + ret = PTR_ERR_OR_ZERO(file); } if (ret) { trace_cachefiles_vfs_error(object, d_inode(fan), ret, @@ -471,12 +472,14 @@ struct file *cachefiles_create_tmpfile(struct cachefiles_object *object) cachefiles_io_error_obj(object, "Failed to create tmpfile"); goto err; } + /* From now path refers to the tmpfile */ + path.dentry = file->f_path.dentry; trace_cachefiles_tmpfile(object, d_backing_inode(path.dentry)); ret = -EBUSY; if (!cachefiles_mark_inode_in_use(object, path.dentry)) - goto err_dput; + goto err_fput; ret = cachefiles_ondemand_init_object(object); if (ret < 0) @@ -499,14 +502,6 @@ struct file *cachefiles_create_tmpfile(struct cachefiles_object *object) } } - file = open_with_fake_path(&path, O_RDWR | O_LARGEFILE | O_DIRECT, - d_backing_inode(path.dentry), cache->cache_cred); - ret = PTR_ERR(file); - if (IS_ERR(file)) { - trace_cachefiles_vfs_error(object, d_backing_inode(path.dentry), - ret, cachefiles_trace_open_error); - goto err_unuse; - } ret = -EINVAL; if (unlikely(!file->f_op->read_iter) || unlikely(!file->f_op->write_iter)) { @@ -514,15 +509,14 @@ struct file *cachefiles_create_tmpfile(struct cachefiles_object *object) pr_notice("Cache does not support read_iter and write_iter\n"); goto err_unuse; } - dput(path.dentry); out: cachefiles_end_secure(cache, saved_cred); return file; err_unuse: cachefiles_do_unmark_inode_in_use(object, path.dentry); -err_dput: - dput(path.dentry); +err_fput: + fput(file); err: file = ERR_PTR(ret); goto out;
Use the tmpfile_open() helper instead of doing tmpfile creation and opening separately. Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> --- fs/cachefiles/namei.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-)