From: "J. Bruce Fields" <bfields@fieldses.org>
To: dai.ngo@oracle.com
Cc: chuck.lever@oracle.com, linux-nfs@vger.kernel.org
Subject: Re: [PATCH RFC 1/1] nfsd: Initial implementation of NFSv4 Courteous Server
Date: Wed, 30 Jun 2021 21:16:36 -0400 [thread overview]
Message-ID: <20210701011636.GI20229@fieldses.org> (raw)
In-Reply-To: <a111226a-c6ac-0a39-190a-c2cc8b781213@oracle.com>
On Wed, Jun 30, 2021 at 04:48:57PM -0700, dai.ngo@oracle.com wrote:
>
> On 6/30/21 12:24 PM, J. Bruce Fields wrote:
> >On Wed, Jun 30, 2021 at 12:13:35PM -0700, dai.ngo@oracle.com wrote:
> >>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:
> >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);
Looks OK to me.--b.
>
> -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.
next prev parent reply other threads:[~2021-07-01 1:16 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-03 18:14 [PATCH RFC 1/1] nfsd: Initial implementation of NFSv4 Courteous Server Dai Ngo
2021-06-11 8:42 ` dai.ngo
2021-06-16 16:02 ` J. Bruce Fields
2021-06-16 16:32 ` Chuck Lever III
2021-06-16 19:25 ` dai.ngo
2021-06-16 19:29 ` Chuck Lever III
2021-06-16 20:30 ` Bruce Fields
2021-06-16 19:17 ` dai.ngo
2021-06-16 19:19 ` Calum Mackay
2021-06-16 19:27 ` dai.ngo
2021-06-24 14:02 ` J. Bruce Fields
2021-06-24 19:50 ` dai.ngo
2021-06-24 20:36 ` J. Bruce Fields
2021-06-28 20:23 ` J. Bruce Fields
2021-06-28 23:39 ` dai.ngo
2021-06-29 4:40 ` dai.ngo
2021-06-30 1:35 ` J. Bruce Fields
2021-06-30 8:41 ` dai.ngo
2021-06-30 14:52 ` J. Bruce Fields
2021-06-30 17:51 ` dai.ngo
2021-06-30 18:05 ` J. Bruce Fields
2021-06-30 18:49 ` dai.ngo
2021-06-30 18:55 ` J. Bruce Fields
2021-06-30 19:13 ` dai.ngo
2021-06-30 19:24 ` J. Bruce Fields
2021-06-30 23:48 ` dai.ngo
2021-07-01 1:16 ` J. Bruce Fields [this message]
2021-06-30 15:13 ` J. Bruce Fields
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210701011636.GI20229@fieldses.org \
--to=bfields@fieldses.org \
--cc=chuck.lever@oracle.com \
--cc=dai.ngo@oracle.com \
--cc=linux-nfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).