On 3/30/22 4:51 PM, David Howells wrote:
> Jeffle Xu <jeffl...@linux.alibaba.com> 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.
OK.
>
>> @@ -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.
Right. Only cachefiles_open_file() will trigger "Inode already in use"
warning.
>
>> @@ -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.
OK.
--
Thanks,
Jeffle
--
Linux-cachefs mailing list
Linux-cachefs@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-cachefs
