Jeffle Xu <jefflexu@xxxxxxxxxxxxxxxxx> wrote:
> Subject: [PATCH] cachefiles: revert inode in use in error path
Can you say "Unmark" rather than "revert"? I would tend to reserve the word
"revert" for when I'm reverting a commit.
> @@ -494,15 +502,20 @@ struct file *cachefiles_create_tmpfile(struct cachefiles_object *object)
> ...
> +out_unuse:
> + cachefiles_do_unmark_inode_in_use(object, path.dentry);
It shouldn't matter here. We're dealing with a tmpfile, so we shouldn't ever
see it again. If we do, it's a bug in the backing filesystem. OTOH, I
suppose it makes sense to clear it anyway.
> @@ -574,8 +587,16 @@ static bool cachefiles_open_file(struct cachefiles_object *object,
> _debug("file -> %pd positive", dentry);
>
> ret = cachefiles_check_auxdata(object, file);
> - if (ret < 0)
> - goto check_failed;
> + if (ret < 0) {
> + fscache_cookie_lookup_negative(object->cookie);
> + cachefiles_unmark_inode_in_use(object, file);
> + fput(file);
> + dput(dentry);
> +
> + if (ret == -ESTALE)
> + return cachefiles_create_file(object);
> + return false;
> + }
>
> object->file = file;
>
> @@ -587,17 +608,10 @@ static bool cachefiles_open_file(struct cachefiles_object *object,
> dput(dentry);
> return true;
>
> -check_failed:
> - fscache_cookie_lookup_negative(object->cookie);
> - cachefiles_unmark_inode_in_use(object, file);
> - if (ret == -ESTALE) {
> - fput(file);
> - dput(dentry);
> - return cachefiles_create_file(object);
> - }
> error_fput:
> fput(file);
> error:
> + cachefiles_do_unmark_inode_in_use(object, dentry);
> dput(dentry);
> return false;
> }
Okay, you are correct here, but could you leave the "check_failed:" label
where it is and make your rearrangements there? That way the error handling
is outside the main path through the function.
David
--
Linux-cachefs mailing list
Linux-cachefs@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/linux-cachefs
