Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a651f61e-f5e4-a96a-2d8d-96b5ca156ea5@oracle.com>
Date:   Wed, 27 Apr 2022 23:36:36 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.8.1
Subject: Re: [PATCH RFC v22 1/7] NFSD: add courteous server support for thread
 with only delegation
Content-Language: en-US
From:   dai.ngo@oracle.com
To:     "J. Bruce Fields" <bfields@fieldses.org>
Cc:     chuck.lever@oracle.com, jlayton@redhat.com,
        viro@zeniv.linux.org.uk, linux-nfs@vger.kernel.org,
        linux-fsdevel@vger.kernel.org
References: <1651049573-29552-1-git-send-email-dai.ngo@oracle.com>
 <1651049573-29552-2-git-send-email-dai.ngo@oracle.com>
 <20220427215614.GH13471@fieldses.org>
 <24607d8d-9a69-b139-ce1a-c0f70814de05@oracle.com>
 <20220428015234.GJ13471@fieldses.org>
 <3755605a-40ce-546f-b287-aa1cee10cd86@oracle.com>
In-Reply-To: <3755605a-40ce-546f-b287-aa1cee10cd86@oracle.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?VmFsTzZ6VVdOTFRQRnc0OFlZK1BtMGJ4RVU5UE1JUmZaZmI3dG9UVXVTa1Vm?=
 =?utf-8?B?cmM3VVY1WTV5Q1FVbFNiV25uTFovWGF5Q2JBS2hOUDc3VzAwMTZJYzBYWklo?=
 =?utf-8?B?dlRqcHhXNGJDQVVJYXNyWFpkemYrUW9tYS85OWJLZnFndkVGMmorSjBYZ2gv?=
 =?utf-8?B?UlNYekg4VWNmMTFrZ25MbG4zT2cwODZNS05MdmNRVTBqbjdkOWVUcy9sYjRp?=
 =?utf-8?B?YzgrR0FIdGdTdzRkbVVjUHVHWDROY2x3c0NxNnhIQXFENW4wREhwYlR6ZnZw?=
 =?utf-8?B?Q2lySEMzaHhzSW42VGhGWUx0dFd3SlRHVWdWMXduK3l4bkpUcE1XbmNKZGx6?=
 =?utf-8?B?S1VvZ0JvRkl1NEdGbmJGOGE1TXJGSlNiZE5WQkg4WGZVZjZhU0xJSzRyM0ZU?=
 =?utf-8?B?WXhhYUtOcTVUS3VxRHZXY0IreVF2YVFjR3RLdnJxbHpPL1paZzhCa0hXNlhu?=
 =?utf-8?B?emhQUG5YQTRORVlDOThQWUtjY1VJZHZUalN3aE80Yks4Mkg0UDc4VFRZaXNZ?=
 =?utf-8?B?MU9vMnRlR0hxM1dYYWZQeURCc2UwZnFLTHIvdjVaRDkvV00zaW1XYWcwTi9P?=
 =?utf-8?B?dGwzclBMZE16Z2NLWHEzcnZoL0VEOElLanNrLzd3TUVWZk9uaHlNbkdkVEJC?=
 =?utf-8?B?SXV6YzBZTVlHb3JDSy9SYkpMd0p0UFc4Q2kxdTJ0VTY1a2IzeStZSUc1dm9Q?=
 =?utf-8?B?WXdsRWJGNVFNOXBvSjhobzRWVnk0NGJvR0VDRFNLOFQwRW9yZHhFbUo3RlJG?=
 =?utf-8?B?UmZLaDFwekpRMXAwZXRZME9YN0xKblMyYy9nNGpDaFZMSldkazFLUWhwWDNB?=
 =?utf-8?B?enRoclZXNVJKZmxFS2VqTW1za3BOdWswU0xTaFJRdmpLUmtYakRxVWZTb1FC?=
 =?utf-8?B?Sk52QVFXYTR0bkdhdU80c2k4TklVZURWS2JMTUNSbUNwMEpBOGQ2WFc1REQ4?=
 =?utf-8?B?TEVSR2xEKzNFcjVVd1ROVnZDNlZzajdVditjaldiSHFzV0s4ek5nclNRS3cx?=
 =?utf-8?B?RnplQ3NpaXg5MkVTUjIxaFBkU1lFcUJWU3VNK3krSHBwTG5hQ3I2aWlMMldn?=
 =?utf-8?B?QVNnT2dCdThQVWdXaGROeTU1QUdVVFZaQXFweWdxdkpXV1ptVkVuODhuL2Zs?=
 =?utf-8?B?aC93K1dlV25CU05PSFM5b3h5MDZUNDV5MnNpaUNNQmFPaUdmZmxvZUZFMXp5?=
 =?utf-8?B?SkdTeU94VzJBMmNVREVDL1RHU0kvV1JwbFhjWlM2UmxPQTZkeGE2YnluekNG?=
 =?utf-8?B?ZWtNbU9wckMzOXl6UGYzSEZjM0lrYXVVd1FlSlpMbHhOUkVwOWhrcGtVdkNI?=
 =?utf-8?B?REFWTDFOS2QwbGNUL3pTMUJMT2E2d3J2aGFrUTJLNldOMFQvZDhBbE1WLzFR?=
 =?utf-8?B?eXB0S3BvazdOMWRDeVlSQXY1a3UwNjNpWHMvcGVXaXNnMnF2eFhuVHFyYUYz?=
 =?utf-8?B?bm1wc1NSZmNuYndOWHorZlljeHQ4bytFWkJsMWppbG4vVHEzQXQ4b0Y5Nzlv?=
 =?utf-8?B?UmtpekQ5S0dYc2tabnh5WGtrVXJCQUw4R2NwQzRyclZpd2ZrZjNmV3huN0Rz?=
 =?utf-8?B?L29LM005NFBwclo0SHpucXBpVVZKaUE4cXZHazZPSkJyRGQ3UG1wNnhNdEYy?=
 =?utf-8?B?S2ZaajYzaStuSnMrcXBwRmdQNnRFdUJDdXdDUGszeVIwVEpQYld1czBtR3lw?=
 =?utf-8?B?VmdNOVlXMVdiNWxoYkUwbnJZc0hOcWM4UEg0YVIxZ1lHbUJvNjFxaTMvZzJ1?=
 =?utf-8?B?Wm00ajg5cVRNN2huWkhLVzNidzQwbFA2dmNhY3NjVlVDQW9JTkZKcUpVRlF1?=
 =?utf-8?B?M2pGbjFwQjY0MFFYSVhVc1lJQWJxVzJqS3NWdURBRnM0cnhXL2sreDdZcjBz?=
 =?utf-8?B?SC9melAydFBJSEZRdW9MOEY3NXpsOUNNTEJ3QjlqY3ZFSlB6RDBrUjZNMzk0?=
 =?utf-8?B?bjhmSjBOSEYzNmk1Sk1xOGZxZWFld1NYcFoxdUJVRUVEUTBxOXVrTkFYaGN3?=
 =?utf-8?B?ZWczU1V0UVFHK2QvQVlRQXBXdzQvaEUzczAyTGF5WnhzNTUwODFPT0x0cHQv?=
 =?utf-8?B?eG1xektFbmZaeE01Lzc4LytrWCtXUlpIazl1RzNvVGUwcWVNTm5OUnlCeUhH?=
 =?utf-8?B?TDlkLzU3RWRxNWdWbUJ2VHBGdkN0L0xLbUFjR0xodExTb0dRRlE4V3c4UmVY?=
 =?utf-8?B?dnVHb0ptR1dVT2hUeURtbURiNHNJRG9pbmsvUWRRcUFLTEZXMUZLc3ErcWtY?=
 =?utf-8?B?aDBCNVpQUG1odGVveVZmb2VwQWhHYXEzQ0FrWlBEOHQ1S0tDa3pUb2Z1cVUr?=
 =?utf-8?B?aU5QdHBXOUIvNUhSQk1mYWIrQzREd2xYVTJuZW9rOE01QVFCbVZtdz09?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 58a85358-12d4-4640-6939-08da28e17407
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4257.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Apr 2022 06:36:40.6679
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: gY9YIuO/ylQM85Yhw/kwfCDF5YQSN0oIAwFmJiEkSe0DPNhWZ9cv5rxDvoTpEIURjuyYjL2j47OchoD51CovwQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR10MB5179
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.486,18.0.858
 definitions=2022-04-27_04:2022-04-27,2022-04-27 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 mlxlogscore=999
 malwarescore=0 mlxscore=0 phishscore=0 bulkscore=0 adultscore=0
 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2202240000 definitions=main-2204280038
X-Proofpoint-ORIG-GUID: bcf161Fv8532Mz9Vny-fopeNz01PCBMn
X-Proofpoint-GUID: bcf161Fv8532Mz9Vny-fopeNz01PCBMn
X-Spam-Status: No, score=-4.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


On 4/27/22 8:44 PM, dai.ngo@oracle.com wrote:
>
> On 4/27/22 6:52 PM, J. Bruce Fields wrote:
>> On Wed, Apr 27, 2022 at 03:52:37PM -0700, dai.ngo@oracle.com wrote:
>>> On 4/27/22 2:56 PM, J. Bruce Fields wrote:
>>>> On Wed, Apr 27, 2022 at 01:52:47AM -0700, Dai Ngo wrote:
>>>>> This patch provides courteous server support for delegation only.
>>>>> Only expired client with delegation but no conflict and no open
>>>>> or lock state is allowed to be in COURTESY state.
>>>>>
>>>>> Delegation conflict with COURTESY/EXPIRABLE client is resolved by
>>>>> setting it to EXPIRABLE, queue work for the laundromat and return
>>>>> delay to the caller. Conflict is resolved when the laudromat runs
>>>>> and expires the EXIRABLE client while the NFS client retries the
>>>>> OPEN request. Local thread request that gets conflict is doing the
>>>>> retry in _break_lease.
>>>>>
>>>>> Client in COURTESY or EXPIRABLE state is allowed to reconnect and
>>>>> continues to have access to its state. Access to the nfs4_client by
>>>>> the reconnecting thread and the laundromat is serialized via the
>>>>> client_lock.
>>>>>
>>>>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>>>>> ---
>>>>>   fs/nfsd/nfs4state.c | 86 
>>>>> +++++++++++++++++++++++++++++++++++++++++++++--------
>>>>>   fs/nfsd/nfsd.h      |  1 +
>>>>>   fs/nfsd/state.h     | 32 ++++++++++++++++++++
>>>>>   3 files changed, 106 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>>> index 234e852fcdfa..216bd77a8764 100644
>>>>> --- a/fs/nfsd/nfs4state.c
>>>>> +++ b/fs/nfsd/nfs4state.c
>>>>> @@ -125,6 +125,8 @@ static void free_session(struct nfsd4_session *);
>>>>>   static const struct nfsd4_callback_ops nfsd4_cb_recall_ops;
>>>>>   static const struct nfsd4_callback_ops nfsd4_cb_notify_lock_ops;
>>>>> +static struct workqueue_struct *laundry_wq;
>>>>> +
>>>>>   static bool is_session_dead(struct nfsd4_session *ses)
>>>>>   {
>>>>>       return ses->se_flags & NFS4_SESSION_DEAD;
>>>>> @@ -152,6 +154,7 @@ static __be32 get_client_locked(struct 
>>>>> nfs4_client *clp)
>>>>>       if (is_client_expired(clp))
>>>>>           return nfserr_expired;
>>>>>       atomic_inc(&clp->cl_rpc_users);
>>>>> +    clp->cl_state = NFSD4_ACTIVE;
>>>>>       return nfs_ok;
>>>>>   }
>>>>> @@ -172,6 +175,7 @@ renew_client_locked(struct nfs4_client *clp)
>>>>>       list_move_tail(&clp->cl_lru, &nn->client_lru);
>>>>>       clp->cl_time = ktime_get_boottime_seconds();
>>>>> +    clp->cl_state = NFSD4_ACTIVE;
>>>>>   }
>>>>>   static void put_client_renew_locked(struct nfs4_client *clp)
>>>>> @@ -2004,6 +2008,7 @@ static struct nfs4_client 
>>>>> *alloc_client(struct xdr_netobj name)
>>>>>       idr_init(&clp->cl_stateids);
>>>>>       atomic_set(&clp->cl_rpc_users, 0);
>>>>>       clp->cl_cb_state = NFSD4_CB_UNKNOWN;
>>>>> +    clp->cl_state = NFSD4_ACTIVE;
>>>>>       INIT_LIST_HEAD(&clp->cl_idhash);
>>>>>       INIT_LIST_HEAD(&clp->cl_openowners);
>>>>>       INIT_LIST_HEAD(&clp->cl_delegations);
>>>>> @@ -4694,9 +4699,16 @@ nfsd_break_deleg_cb(struct file_lock *fl)
>>>>>       bool ret = false;
>>>>>       struct nfs4_delegation *dp = (struct nfs4_delegation 
>>>>> *)fl->fl_owner;
>>>>>       struct nfs4_file *fp = dp->dl_stid.sc_file;
>>>>> +    struct nfs4_client *clp = dp->dl_stid.sc_client;
>>>>> +    struct nfsd_net *nn;
>>>>>       trace_nfsd_cb_recall(&dp->dl_stid);
>>>>> +    if (!try_to_expire_client(clp)) {
>>>>> +        nn = net_generic(clp->net, nfsd_net_id);
>>>>> +        mod_delayed_work(laundry_wq, &nn->laundromat_work, 0);
>>>>> +    }
>>>>> +
>>>>>       /*
>>>>>        * We don't want the locks code to timeout the lease for us;
>>>>>        * we'll remove it ourself if a delegation isn't returned
>>>>> @@ -5605,6 +5617,65 @@ static void nfsd4_ssc_expire_umount(struct 
>>>>> nfsd_net *nn)
>>>>>   }
>>>>>   #endif
>>>>> +/*
>>>>> + * place holder for now, no check for lock blockers yet
>>>>> + */
>>>>> +static bool
>>>>> +nfs4_anylock_blockers(struct nfs4_client *clp)
>>>>> +{
>>>>> +    /*
>>>>> +     * don't want to check for delegation conflict here since
>>>>> +     * we need the state_lock for it. The laundromat willexpire
>>>>> +     * COURTESY later when checking for delegation recall timeout.
>>>>> +     */
>>>>> +    return false;
>>>>> +}
>>>>> +
>>>>> +static bool client_has_state_tmp(struct nfs4_client *clp)
>>>>> +{
>>>>> +    if (!list_empty(&clp->cl_delegations) &&
>>>>> +            !client_has_openowners(clp) &&
>>>>> +            list_empty(&clp->async_copies))
>>>>> +        return true;
>>>>> +    return false;
>>>>> +}
>>>>> +
>>>>> +static void
>>>>> +nfs4_get_client_reaplist(struct nfsd_net *nn, struct list_head 
>>>>> *reaplist,
>>>>> +                struct laundry_time *lt)
>>>>> +{
>>>>> +    struct list_head *pos, *next;
>>>>> +    struct nfs4_client *clp;
>>>>> +    bool cour;
>>>>> +
>>>>> +    INIT_LIST_HEAD(reaplist);
>>>>> +    spin_lock(&nn->client_lock);
>>>>> +    list_for_each_safe(pos, next, &nn->client_lru) {
>>>>> +        clp = list_entry(pos, struct nfs4_client, cl_lru);
>>>>> +        if (clp->cl_state == NFSD4_EXPIRABLE)
>>>>> +            goto exp_client;
>>>>> +        if (!state_expired(lt, clp->cl_time))
>>>>> +            break;
>>>>> +        if (!client_has_state_tmp(clp))
>>>>> +            goto exp_client;
>>>>> +        cour = (clp->cl_state == NFSD4_COURTESY);
>>>>> +        if (cour && ktime_get_boottime_seconds() >=
>>>>> +                (clp->cl_time + NFSD_COURTESY_CLIENT_TIMEOUT)) {
>>>>> +            goto exp_client;
>>>>> +        }
>>>>> +        if (nfs4_anylock_blockers(clp)) {
>>>>> +exp_client:
>>>>> +            if (mark_client_expired_locked(clp))
>>>>> +                continue;
>>>>> +            list_add(&clp->cl_lru, reaplist);
>>>>> +            continue;
>>>>> +        }
>>>>> +        if (!cour)
>>>>> +            cmpxchg(&clp->cl_state, NFSD4_ACTIVE, NFSD4_COURTESY);
>>>> I just noticed there's a small race here: a lock conflict (for 
>>>> example)
>>>> could intervene between checking nfs4_anylock_blockers and setting
>>>> COURTESY.
>>> If there is lock conflict intervenes before setting COURTESY then that
>>> lock request is denied since the client is ACTIVE. Does NFSv4, NLM
>>> client retry the lock request? if it does then on next retry the
>>> COURTESY client will be expired.
>> I'm thinking of a local request for a blocking lock.  Yes, the request
>> will be denied, but then the process will block on the lock forever
>> (well, for 24 hours anyway).

Actually if the local request is blocked, it will be blocked for max of
a lease period. When the laundromat runs again, it will detect there is
blockers and expire the COURTESY client.

I will move the setting of COURTESY to earlier as you suggested since
it also makes the code more compact.

-Dai