Re: [PATCH v2 1/2] NFSD: added FREE_STATEID operation

[Bryan Schumaker <bjschuma@netapp.com>]

On 05/19/2011 12:35 PM, Bryan Schumaker wrote:
> On 05/19/2011 12:30 PM, J. Bruce Fields wrote:
>> On Thu, May 19, 2011 at 11:33:56AM -0400, Bryan Schumaker wrote:
>>> On 05/18/2011 06:56 PM, J. Bruce Fields wrote:
>>>> On Mon, May 16, 2011 at 03:43:40PM -0400, bjschuma@netapp.com wrote:
>>>>> +static __be32
>>>>> +nfsd4_free_file_stateid(stateid_t *stateid)
>>>>> +{
>>>>> +	struct nfs4_stateid *stp = search_for_stateid(stateid);
>>>>> +	if (!stp)
>>>>> +		return nfserr_bad_stateid;
>>>>> +	if (stateid->si_generation != 0) {
>>>>> +		if (stateid->si_generation < stp->st_stateid.si_generation)
>>>>> +			return nfserr_old_stateid;
>>>>> +		if (stateid->si_generation > stp->st_stateid.si_generation)
>>>>> +			return nfserr_bad_stateid;
>>>>> +	}
>>>>> +
>>>>> +	if (check_for_locks(stp->st_file, stp->st_stateowner))
>>>>> +		return nfserr_locks_held;
>>>>
>>>> I think this catches a lock stateid, but not an open stateid that has
>>>> associated lock stateid's that in turn hold locks.
>>>>
>>>> Hm, also:
>>>>
>>>> 	"The FREE_STATEID operation is used to free a stateid that no
>>>> 	longer has any associated locks (including opens, byte-range
>>>> 	locks, delegations, and layouts)"
>>>>
>>>> So an open stateid also shouldn't be freeable as long as there are opens
>>>> associated with it.
>>>
>>> So having an open stateid doesn't necessarily mean that the file is open?
>>
>> Looking back at it.... Sorry, you're right, open stateid's are destroyed
>> on close, so like delegation stateid's they should just never be
>> freeable.
>>
>>> and having a lock stateid doesn't mean that the file is locked?
>>
>> Here we don't know whether the file's locked or not, so we do have to
>> check.
>>
>>> I'll look at making a "check_for_opens()" function to help with this check.
>>
>> So actually I think it's really simple: always fail opens and
>> delegations, but check for locks.  (Except I'm not sure if
> 
> That is much simpler.  I'm glad I asked!
> 
>> check_for_locks() does the right things, as it operates on a stateowner
>> not a stateid--I'm forgetting how those work....  If there's a unique
>> lock stateid per (stateowner,file) pair then check_for_locks() should do
>> what you want.)
> 
> I'm not sure how they work either.  I'll browse through the code to see what I can find.

It looks like a stateid only applies to a single (stateowner, file), but I don't know if multiple stateids can point to the same (stateowner, file).

It looks like lock set / test / unlock is all done through the vfs, so I'm not sure how to check if a specific stateid is locked without using check_for_locks().
> 
> Thanks!
> 
>>
>> --b.
>>
>>>
>>>>
>>>> Also I guess a client shouldn't be permitted to free a delegation that
>>>> it still holds.  That means we'll always just return nfserr_locks for
>>>> delegation stateid's.  I assume free_stateid is only useful in this case
>>>
>>> Sounds simple enough.
>>>
>>>> for the case where a server forcibly revokes part of the client's state
>>>> and leaves some "stub" stateid's around in place of the revoked ones.
>>>>
>>>> --b.
>>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html