Re: [PATCH v4 04/10] cachefiles: only pass inode to *mark_inode_inuse() helpers

[Al Viro <viro@zeniv.linux.org.uk>]

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).