[Linux-cachefs] [PATCH] cachefiles: revert inode in use in error path

[JeffleXu]

On 3/30/22 4:51 PM, David Howells wrote:
> Jeffle Xu <jefflexu at 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