On 6/30/21 12:24 PM, J. Bruce Fields wrote:
On Wed, Jun 30, 2021 at 12:13:35PM -0700, dai.ngo@xxxxxxxxxx wrote:
On 6/30/21 11:55 AM, J. Bruce Fields wrote:
On Wed, Jun 30, 2021 at 11:49:18AM -0700, dai.ngo@xxxxxxxxxx wrote:
On 6/30/21 11:05 AM, J. Bruce Fields wrote:
On Wed, Jun 30, 2021 at 10:51:27AM -0700, dai.ngo@xxxxxxxxxx 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:
I doubt that works in the case there are multiple locks with
lm_expire_lock set.
If you really don't want another callback here, maybe you could set some
kind of flag on the lock.
At the time a client expires, you're going to have to walk all of its
locks to see if anyone's waiting for them. At the same time maybe you
could set an FL_EXPIRABLE flag on all those locks, and test for that
here.
If the network partition heals and the client comes back, you'd have to
remember to clear that flag again.
It's too much unnecessary work.
Would this be suffice:
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 &&
fl->fl_lmops->lm_expire_lock(fl, 1)) {
spin_unlock(&ctx->flc_lock);
fl->fl_lmops->lm_expire_lock(fl, 0);
spin_lock(&ctx->flc_lock);
goto retry;
}
if (conflock)
locks_copy_conflock(conflock, fl);
-Dai
--b.
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.
