Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 30 Jun 2021 14:55:19 -0400
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
Message-ID: <20210630185519.GG20229@fieldses.org>
References: <20210603181438.109851-1-dai.ngo@oracle.com>
 <20210628202331.GC6776@fieldses.org>
 <dc71d572-d108-bcfc-e264-d96ef0de1b36@oracle.com>
 <9628be9d-2bfd-d036-2308-847cb4f1a14d@oracle.com>
 <20210630180527.GE20229@fieldses.org>
 <08caefcd-5271-8d44-326d-395399ff465c@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <08caefcd-5271-8d44-326d-395399ff465c@oracle.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

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.

>                         }
> 
>                         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.

--b.