| Message ID | 20220922084442.2401223-5-mszeredi@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuse tmpfile | expand |
On Thu, Sep 22, 2022 at 10:44:36AM +0200, Miklos Szeredi wrote: > The only reason to pass dentry was because of a pr_notice() text. Move > that to the two callers where it makes sense and add a WARN_ON() to the > third. > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > --- Looks good to me, Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>
On Thu, Sep 22, 2022 at 10:44:36AM +0200, Miklos Szeredi wrote: > @@ -78,7 +70,7 @@ void cachefiles_unmark_inode_in_use(struct cachefiles_object *object, > struct inode *inode = file_inode(file); ^^^^^^^^^^^^^^^^^^^^^^^^ > > if (inode) { ^^^^^ > - cachefiles_do_unmark_inode_in_use(object, file->f_path.dentry); > + cachefiles_do_unmark_inode_in_use(object, file_inode(file)); ^^^^^^^^^^^^^^^^ > > if (!test_bit(CACHEFILES_OBJECT_USING_TMPFILE, &object->flags)) { > atomic_long_add(inode->i_blocks, &cache->b_released); > @@ -225,7 +220,7 @@ void cachefiles_put_directory(struct dentry *dir) > inode_lock(dir->d_inode); > - __cachefiles_unmark_inode_in_use(NULL, dir); > + __cachefiles_unmark_inode_in_use(NULL, d_inode(dir)); > inode_unlock(dir->d_inode); Sequence seems identical to cachefiles_do_unmark_inode_in_use(NULL, dir->d_inode)... Incidentally, this void cachefiles_unmark_inode_in_use(struct cachefiles_object *object, struct file *file) { struct cachefiles_cache *cache = object->volume->cache; struct inode *inode = file_inode(file); if (inode) { is, er, excessively defensive prog^W^W^Wobfuscation for no reason - file_inode(file) is never NULL for any opened file. While we are at it, one of the callers of that puppy also looks interesting: cachefiles_unmark_inode_in_use(object, object->file); if (object->file) { fput(object->file); object->file = NULL; } file_inode(NULL) is not NULL, it's an oops... Fortunately, the only caller of that one is if (object->file) { cachefiles_begin_secure(cache, &saved_cred); cachefiles_clean_up_object(object, cache); I would rather leave unobfuscating that to a separate patch, if not a separate series, but since you are touching cachefiles_unmark_inode_in_use() anyway, might as well get rid of if (inode) in there - it's equivalent to if (true).
On Thu, 22 Sept 2022 at 18:22, Al Viro <viro@zeniv.linux.org.uk> wrote: > I would rather leave unobfuscating that to a separate patch, > if not a separate series, but since you are touching > cachefiles_unmark_inode_in_use() anyway, might as well > get rid of if (inode) in there - it's equivalent to if (true). Okay, pushed updated version (also with cachefiles_do_unmark_inode_in_use() un-open-coding) to: git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#fuse-tmpfile-v5 Thanks, Miklos
On Fri, Sep 23, 2022 at 05:42:01PM +0200, Miklos Szeredi wrote: > On Thu, 22 Sept 2022 at 18:22, Al Viro <viro@zeniv.linux.org.uk> wrote: > > > I would rather leave unobfuscating that to a separate patch, > > if not a separate series, but since you are touching > > cachefiles_unmark_inode_in_use() anyway, might as well > > get rid of if (inode) in there - it's equivalent to if (true). > > Okay, pushed updated version (also with > cachefiles_do_unmark_inode_in_use() un-open-coding) to: > > git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#fuse-tmpfile-v5 OK... I can live with that. Could you replace cachefiles_do_unmark_inode_in_use(object, file_inode(file)); with cachefiles_do_unmark_inode_in_use(object, inode); in there and repush? Or I could do cherry-pick and fix it up...
On Sat, 24 Sept 2022 at 06:56, Al Viro <viro@zeniv.linux.org.uk> wrote: > > On Fri, Sep 23, 2022 at 05:42:01PM +0200, Miklos Szeredi wrote: > > On Thu, 22 Sept 2022 at 18:22, Al Viro <viro@zeniv.linux.org.uk> wrote: > > > > > I would rather leave unobfuscating that to a separate patch, > > > if not a separate series, but since you are touching > > > cachefiles_unmark_inode_in_use() anyway, might as well > > > get rid of if (inode) in there - it's equivalent to if (true). > > > > Okay, pushed updated version (also with > > cachefiles_do_unmark_inode_in_use() un-open-coding) to: > > > > git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#fuse-tmpfile-v5 > > OK... I can live with that. Could you > replace > cachefiles_do_unmark_inode_in_use(object, file_inode(file)); > with > cachefiles_do_unmark_inode_in_use(object, inode); > in there and repush? Or I could do cherry-pick and fix it up... Done. Head is now: 7d37539037c2 ("fuse: implement ->tmpfile()") Thanks, Miklos
On Sat, Sep 24, 2022 at 07:02:19AM +0200, Miklos Szeredi wrote: > On Sat, 24 Sept 2022 at 06:56, Al Viro <viro@zeniv.linux.org.uk> wrote: > > > > On Fri, Sep 23, 2022 at 05:42:01PM +0200, Miklos Szeredi wrote: > > > On Thu, 22 Sept 2022 at 18:22, Al Viro <viro@zeniv.linux.org.uk> wrote: > > > > > > > I would rather leave unobfuscating that to a separate patch, > > > > if not a separate series, but since you are touching > > > > cachefiles_unmark_inode_in_use() anyway, might as well > > > > get rid of if (inode) in there - it's equivalent to if (true). > > > > > > Okay, pushed updated version (also with > > > cachefiles_do_unmark_inode_in_use() un-open-coding) to: > > > > > > git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#fuse-tmpfile-v5 > > > > OK... I can live with that. Could you > > replace > > cachefiles_do_unmark_inode_in_use(object, file_inode(file)); > > with > > cachefiles_do_unmark_inode_in_use(object, inode); > > in there and repush? Or I could do cherry-pick and fix it up... > > Done. Head is now: > > 7d37539037c2 ("fuse: implement ->tmpfile()") Merged and pushed out.
diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c index d3a5884fe5c9..4d04f5fb49f3 100644 --- a/fs/cachefiles/namei.c +++ b/fs/cachefiles/namei.c @@ -15,9 +15,8 @@ * file or directory. The caller must hold the inode lock. */ static bool __cachefiles_mark_inode_in_use(struct cachefiles_object *object, - struct dentry *dentry) + struct inode *inode) { - struct inode *inode = d_backing_inode(dentry); bool can_use = false; if (!(inode->i_flags & S_KERNEL_FILE)) { @@ -26,21 +25,18 @@ static bool __cachefiles_mark_inode_in_use(struct cachefiles_object *object, can_use = true; } else { trace_cachefiles_mark_failed(object, inode); - pr_notice("cachefiles: Inode already in use: %pd (B=%lx)\n", - dentry, inode->i_ino); } return can_use; } static bool cachefiles_mark_inode_in_use(struct cachefiles_object *object, - struct dentry *dentry) + struct inode *inode) { - struct inode *inode = d_backing_inode(dentry); bool can_use; inode_lock(inode); - can_use = __cachefiles_mark_inode_in_use(object, dentry); + can_use = __cachefiles_mark_inode_in_use(object, inode); inode_unlock(inode); return can_use; } @@ -49,21 +45,17 @@ static bool cachefiles_mark_inode_in_use(struct cachefiles_object *object, * Unmark a backing inode. The caller must hold the inode lock. */ static void __cachefiles_unmark_inode_in_use(struct cachefiles_object *object, - struct dentry *dentry) + struct inode *inode) { - struct inode *inode = d_backing_inode(dentry); - inode->i_flags &= ~S_KERNEL_FILE; trace_cachefiles_mark_inactive(object, inode); } static void cachefiles_do_unmark_inode_in_use(struct cachefiles_object *object, - struct dentry *dentry) + struct inode *inode) { - struct inode *inode = d_backing_inode(dentry); - inode_lock(inode); - __cachefiles_unmark_inode_in_use(object, dentry); + __cachefiles_unmark_inode_in_use(object, inode); inode_unlock(inode); } @@ -78,7 +70,7 @@ void cachefiles_unmark_inode_in_use(struct cachefiles_object *object, struct inode *inode = file_inode(file); if (inode) { - cachefiles_do_unmark_inode_in_use(object, file->f_path.dentry); + cachefiles_do_unmark_inode_in_use(object, file_inode(file)); if (!test_bit(CACHEFILES_OBJECT_USING_TMPFILE, &object->flags)) { atomic_long_add(inode->i_blocks, &cache->b_released); @@ -164,8 +156,11 @@ struct dentry *cachefiles_get_directory(struct cachefiles_cache *cache, inode_lock(d_inode(subdir)); inode_unlock(d_inode(dir)); - if (!__cachefiles_mark_inode_in_use(NULL, subdir)) + if (!__cachefiles_mark_inode_in_use(NULL, d_inode(subdir))) { + pr_notice("cachefiles: Inode already in use: %pd (B=%lx)\n", + subdir, d_inode(subdir)->i_ino); goto mark_error; + } inode_unlock(d_inode(subdir)); @@ -225,7 +220,7 @@ void cachefiles_put_directory(struct dentry *dir) { if (dir) { inode_lock(dir->d_inode); - __cachefiles_unmark_inode_in_use(NULL, dir); + __cachefiles_unmark_inode_in_use(NULL, d_inode(dir)); inode_unlock(dir->d_inode); dput(dir); } @@ -410,7 +405,7 @@ int cachefiles_bury_object(struct cachefiles_cache *cache, "Rename failed with error %d", ret); } - __cachefiles_unmark_inode_in_use(object, rep); + __cachefiles_unmark_inode_in_use(object, d_inode(rep)); unlock_rename(cache->graveyard, dir); dput(grave); _leave(" = 0"); @@ -474,9 +469,9 @@ struct file *cachefiles_create_tmpfile(struct cachefiles_object *object) trace_cachefiles_tmpfile(object, d_backing_inode(path.dentry)); - ret = -EBUSY; - if (!cachefiles_mark_inode_in_use(object, path.dentry)) - goto err_dput; + /* This is a newly created file with no other possible user */ + if (!cachefiles_mark_inode_in_use(object, d_inode(path.dentry))) + WARN_ON(1); ret = cachefiles_ondemand_init_object(object); if (ret < 0) @@ -520,8 +515,7 @@ struct file *cachefiles_create_tmpfile(struct cachefiles_object *object) return file; err_unuse: - cachefiles_do_unmark_inode_in_use(object, path.dentry); -err_dput: + cachefiles_do_unmark_inode_in_use(object, d_inode(path.dentry)); dput(path.dentry); err: file = ERR_PTR(ret); @@ -566,8 +560,11 @@ static bool cachefiles_open_file(struct cachefiles_object *object, _enter("%pd", dentry); - if (!cachefiles_mark_inode_in_use(object, dentry)) + if (!cachefiles_mark_inode_in_use(object, d_inode(dentry))) { + pr_notice("cachefiles: Inode already in use: %pd (B=%lx)\n", + dentry, d_inode(dentry)->i_ino); return false; + } /* We need to open a file interface onto a data file now as we can't do * it on demand because writeback called from do_exit() sees @@ -621,7 +618,7 @@ static bool cachefiles_open_file(struct cachefiles_object *object, error_fput: fput(file); error: - cachefiles_do_unmark_inode_in_use(object, dentry); + cachefiles_do_unmark_inode_in_use(object, d_inode(dentry)); dput(dentry); return false; }
The only reason to pass dentry was because of a pr_notice() text. Move that to the two callers where it makes sense and add a WARN_ON() to the third. Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> --- fs/cachefiles/namei.c | 47 ++++++++++++++++++++----------------------- 1 file changed, 22 insertions(+), 25 deletions(-)