> On May 24, 2023, at 1:49 PM, Dai Ngo <dai.ngo@xxxxxxxxxx> wrote:
>
>
> On 5/24/23 8:07 AM, Jeff Layton wrote:
>> On Mon, 2023-05-22 at 16:52 -0700, Dai Ngo wrote:
>>> If the GETATTR request on a file that has write delegation in effect
>>> and the request attributes include the change info and size attribute
>>> then the write delegation is recalled and NFS4ERR_DELAY is returned
>>> for the GETATTR.
>>>
>>> Signed-off-by: Dai Ngo <dai.ngo@xxxxxxxxxx>
>>> ---
>>> fs/nfsd/nfs4state.c | 37 +++++++++++++++++++++++++++++++++++++
>>> fs/nfsd/nfs4xdr.c | 5 +++++
>>> fs/nfsd/state.h | 3 +++
>>> 3 files changed, 45 insertions(+)
>>>
>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>> index b90b74a5e66e..ea9cd781db5f 100644
>>> --- a/fs/nfsd/nfs4state.c
>>> +++ b/fs/nfsd/nfs4state.c
>>> @@ -8353,3 +8353,40 @@ nfsd4_get_writestateid(struct nfsd4_compound_state *cstate,
>>> {
>>> get_stateid(cstate, &u->write.wr_stateid);
>>> }
>>> +
>>> +__be32
>>> +nfs4_handle_wrdeleg_conflict(struct svc_rqst *rqstp, struct inode *inode)
>>> +{
>>> + struct file_lock_context *ctx;
>>> + struct file_lock *fl;
>>> + struct nfs4_delegation *dp;
>>> +
>>> + ctx = locks_inode_context(inode);
>>> + if (!ctx)
>>> + return 0;
>>> + spin_lock(&ctx->flc_lock);
>>> + list_for_each_entry(fl, &ctx->flc_lease, fl_list) {
>>> + if (fl->fl_flags == FL_LAYOUT ||
>>> + fl->fl_lmops != &nfsd_lease_mng_ops)
>>> + continue;
>>> + if (fl->fl_type == F_WRLCK) {
>>> + dp = fl->fl_owner;
>>> + /*
>>> + * increment the sc_count to prevent the delegation to
>>> + * be freed while sending the CB_RECALL. The refcount is
>>> + * decremented by nfs4_put_stid in nfsd4_cb_recall_release
>>> + * after the request was sent.
>>> + */
>>> + if (dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker) ||
>>> + !refcount_inc_not_zero(&dp->dl_stid.sc_count)) {
>> I still don't get why you're incrementing the refcount of this stateid.
>> At this point, you know that this stateid is owned by a different client
>> altogether, and breaking its lease doesn't require a reference to the
>> stateid.
>
> You're right, the intention was to make sure the delegation does not go
> away when the recall is being sent. However, this was already done in
> nfsd_break_one_deleg where the sc_count is incremented. Incrementing the
> sc_count refcount would be needed here if we do the CB_GETATTR. I'll remove
> this in next version.
>
> But should we drop the this patch altogether? since there is no value in
> recall the write delegation when there is an GETATTR from another client
> as I mentioned in the previous email.
a. Neither Solaris nor OnTAP do a CB_RECALL or a CB_GETATTR in this case
b. RFC 8881 Section 18.7.4 states that a server MUST NOT proceed with a
GETATTR response unless it has done one of those callbacks
So we have two examples of non-compliant server implementations that
don't seem to have consequences for not following that MUST.
It doesn't seem unreasonable to recall in this scenario, but it's
unknown what kind of performance impact that will have.
Can you send an updated version of this patch, rebased on nfsd-next
(which now has the other two applied), plus a patch to add a counter
for this type of conflict?
--
Chuck Lever
