Re: [PATCH RFC 1/1] nfsd: Initial implementation of NFSv4 Courteous Server

[dai.ngo@oracle.com]

On 6/30/21 11:55 AM, J. Bruce Fields wrote:
> On Wed, Jun 30, 2021 at 11:49:18AM -0700, dai.ngo@oracle.com wrote:
>> On 6/30/21 11:05 AM, J. Bruce Fields wrote:
>>> On Wed, Jun 30, 2021 at 10:51:27AM -0700, dai.ngo@oracle.com wrote:
>>>>> On 6/28/21 1:23 PM, J. Bruce Fields wrote:
>>>>>> where ->fl_expire_lock is a new lock callback with second
>>>>>> argument "check"
>>>>>> where:
>>>>>>
>>>>>>      check = 1 means: just check whether this lock could be freed
>>>> Why do we need this, is there a use case for it? can we just always try
>>>> to expire the lock and return success/fail?
>>> We can't expire the client while holding the flc_lock.  And once we drop
>>> that lock we need to restart the loop.  Clearly we can't do that every
>>> time.
>>>
>>> (So, my code was wrong, it should have been:
>>>
>>>
>>> 	if (fl->fl_lops->fl_expire_lock(fl, 1)) {
>>> 		spin_unlock(&ct->flc_lock);
>>> 		fl->fl_lops->fl_expire_locks(fl, 0);
>>> 		goto retry;
>>> 	}
>>>
>>> )
>> This is what I currently have:
>>
>> retry:
>>                  list_for_each_entry(fl, &ctx->flc_posix, fl_list) {
>>                          if (!posix_locks_conflict(request, fl))
>>                                  continue;
>>
>>                          if (fl->fl_lmops && fl->fl_lmops->lm_expire_lock) {
>>                                  spin_unlock(&ctx->flc_lock);
>>                                  ret = fl->fl_lmops->lm_expire_lock(fl, 0);
>>                                  spin_lock(&ctx->flc_lock);
>>                                  if (ret)
>>                                          goto retry;
> We have to retry regardless of the return value.  Once we've dropped
> flc_lock, it's not safe to continue trying to iterate through the list.

Yes, thanks!

>
>>                          }
>>
>>                          if (conflock)
>>                                  locks_copy_conflock(conflock, fl);
>>
>>> But the 1 and 0 cases are starting to look pretty different; maybe they
>>> should be two different callbacks.
>> why the case of 1 (test only) is needed,  who would use this call?
> We need to avoid dropping the spinlock in the case there are no clients
> to expire, otherwise we'll make no forward progress.

I think we can remember the last checked file_lock and skip it:

retry:
                 list_for_each_entry(fl, &ctx->flc_posix, fl_list) {
                         if (!posix_locks_conflict(request, fl))
                                 continue;

                         if (checked_fl != fl && fl->fl_lmops &&
                                         fl->fl_lmops->lm_expire_lock) {
                                 checked_fl = fl;
                                 spin_unlock(&ctx->flc_lock);
                                 fl->fl_lmops->lm_expire_lock(fl);
                                 spin_lock(&ctx->flc_lock);
                                 goto retry;
                         }

                         if (conflock)
                                 locks_copy_conflock(conflock, fl);

-Dai

>
> --b.