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:   Mon, 28 Jun 2021 16:23:31 -0400
From:   "J. Bruce Fields" <bfields@fieldses.org>
To:     Dai Ngo <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: <20210628202331.GC6776@fieldses.org>
References: <20210603181438.109851-1-dai.ngo@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210603181438.109851-1-dai.ngo@oracle.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

On Thu, Jun 03, 2021 at 02:14:38PM -0400, Dai Ngo wrote:
> @@ -6875,7 +6947,12 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	case -EAGAIN:		/* conflock holds conflicting lock */
>  		status = nfserr_denied;
>  		dprintk("NFSD: nfsd4_lock: conflicting lock found!\n");
> -		nfs4_set_lock_denied(conflock, &lock->lk_denied);
> +
> +		/* try again if conflict with courtesy client  */
> +		if (nfs4_set_lock_denied(conflock, &lock->lk_denied) == -EAGAIN && !retried) {
> +			retried = true;
> +			goto again;
> +		}

Ugh, apologies, this was my idea, but I just noticed it only handles conflicts
from other NFSv4 clients.  The conflicting lock could just as well come from
NLM or a local process.  So we need cooperation from the common locks.c code.

I'm not sure what to suggest....

Maybe something like:

@@ -1159,6 +1159,7 @@ static int posix_lock_inode(struct inode *inode, struct file_lock *request,
        }
 
        percpu_down_read(&file_rwsem);
+retry:
        spin_lock(&ctx->flc_lock);
        /*
         * New lock request. Walk all POSIX locks and look for conflicts. If
@@ -1169,6 +1170,11 @@ static int posix_lock_inode(struct inode *inode, struct file_lock *request,
                list_for_each_entry(fl, &ctx->flc_posix, fl_list) {
                        if (!posix_locks_conflict(request, fl))
                                continue;
+                       if (fl->fl_lops->fl_expire_lock(fl, 1)) {
+                               spin_unlock(&ctx->flc_lock);
+                               fl->fl_lops->fl_expire_locks(fl, 0);
+                               goto retry;
+                       }
                        if (conflock)
                                locks_copy_conflock(conflock, fl);
                        error = -EAGAIN;


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
	check = 0 means: go ahead and free this lock if you can

--b.