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: <e2b3dfe6-d7c6-3bb1-eebb-0f8cd97c27e7@oracle.com>
Date:   Mon, 25 Apr 2022 14:35:27 -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 v21 1/7] NFSD: add courteous server support for thread
 with only delegation
Content-Language: en-US
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: <1650739455-26096-1-git-send-email-dai.ngo@oracle.com>
 <1650739455-26096-2-git-send-email-dai.ngo@oracle.com>
 <20220425185121.GE24825@fieldses.org>
 <90f5ec04-deff-38d0-2b6f-8b2f48b72d9d@oracle.com>
 <20220425204006.GI24825@fieldses.org>
From:   dai.ngo@oracle.com
In-Reply-To: <20220425204006.GI24825@fieldses.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?elhjOERaRTZ4WU1zNm1qZlZMemFPWWUreHNCZHVacUYrdHhYNm5NZ2xsQndD?=
 =?utf-8?B?U0paMTYySnpQTXBTTHJUWlhSVFpRdlJ0dnlHWFhLV3c3eVU5Wk5pYjlDOHJC?=
 =?utf-8?B?ZTNzcXVjcGVxcS9uc0ZvZEtDU1dIUTYvd2lsY2Rlb3EzenpMUUdsRkV5OUdh?=
 =?utf-8?B?V0NzZldjNmJzMVByREVlNzFaL0lSMDl6REhBRlJBWUJkMUxMaGlHR0tuVk5h?=
 =?utf-8?B?SmJPbHdCMjBoOVBUV1N1VDlnZUZHaGh5SXdwaVlUUk1WK3lrbFdhaWUxd0Rt?=
 =?utf-8?B?L1NzZkppUlZ3cGdCYWh2amtwbDhJYXREcWJ3VlRmV0puTlFmR1pUSCtqYUd6?=
 =?utf-8?B?bnluQ1pNNzRhWVpwNExoNXJ6V3l3RUFYWGxnK2RkZytid2tGNmZaUWNjWmFs?=
 =?utf-8?B?SGw2Zk1oQTVrcUtHeUxjUUNHNWNVQ09UQVNTVHR4RDk5aFloU2taUXVjWWNB?=
 =?utf-8?B?Y2wvV04yWUt4QkM0N0ZsQ2dhTWJiNXNSLzdCOG9ldEU2YVJYaHByOHUyVjZr?=
 =?utf-8?B?cGhPaVRtM2tjNmpOdzQ3K2FSV2s1dHRiZGYrSlFiMlA5Um1uYkRhRVgra0xU?=
 =?utf-8?B?eHdaWEdEVnViQ1lJZitCU2ZuVzdRSlFnaXJndlE3UTFZTllMS0tUQzdycVJV?=
 =?utf-8?B?Z0YvTHloSmtwZlJxWlpVV3IyV3lsQnhwMk15UW5JakhMVnh5UWNVeDF2UHMw?=
 =?utf-8?B?WjlqMUdORW1uTVlTd0NURFlrZXAxbnBwWW1SUDc2RWJRbjEwVjlxY2lkTmUw?=
 =?utf-8?B?OHI5TGxTRjhuS0ZwMEdKR21EMjZHUEZjbW1GRGhDZlpJTnl1YUtYbVo2NEZY?=
 =?utf-8?B?ZzBoSlpnTnc4QXFXKzNiajY1Z09RenZUYnl3WDZSZklpTUFLa0xTM0xLQ1FU?=
 =?utf-8?B?cWRlZmxaRE9JMzBqQVZ3ZGdLYkgwSVRxaEovMGtZTE14SkNvY1FMNVhWd1BJ?=
 =?utf-8?B?M0R2eXVzT0VPa3dkZjlXa3BQZHM4T0NQN3dnRDJiZ1djR1ovSGg3ZTJqWUxI?=
 =?utf-8?B?a2czSUNpOFV3ekcxS1Y4MmgyMW5Jclh3RHJBdXR1VStmV1REZHNDN3NMU0pj?=
 =?utf-8?B?NHZOeEtPZFhtN2o0SU1wWCtFbzYwVndTQnJMUG5GNGRGN1M4UjJRWkJwVy9i?=
 =?utf-8?B?QVdvRnNBS1hiV2diWFlORUlDVHZyQ0FNeUx4SkxvdjdnSzY1Mkh5M3NER3ly?=
 =?utf-8?B?bURxOHVnUHpJL3lyekpORGhCU0UvcnlicFc0ZVBqWkVZRzI3UDF2a2grQVNI?=
 =?utf-8?B?bUQvSmpTL0dNL2c1UElOTEZ1NURIQTBaNld6VE96VjgzQVJqUitxYnUySlUr?=
 =?utf-8?B?QnhhYjEwaU93dTVuSHpiM1ZnZDNPUnliQzRERnAyc3NrL1p0L1FkWDdNUkRD?=
 =?utf-8?B?eEhwbXlFSTEzWWxrakszOUw1QjhBeE0vQll2U1hGaFpNekhEUVIyMUx2MUxI?=
 =?utf-8?B?d1FkUzJIc2praERyQStsejNLWTAwOHAxNFJqdTdrdnR0cFV3anhrVUtpMkZJ?=
 =?utf-8?B?WG5Nb3JheHFQTGhLY1VZZnRjVVpUOFY1aFBWMHFLZXVHR3JzTTRkZmVNL3JI?=
 =?utf-8?B?eVF3WXI5aWpiT1VkKzdiejZjNFJmOE9hOGZKRTdHZFYyWnU2Y2EydDBnblNu?=
 =?utf-8?B?bGtQRUc0M3JFQTAzTzlZcVJzdEt0Q0JUc0x1N2lWYzRMcWwrc1g1L2dhSEhh?=
 =?utf-8?B?ek4xRDl0WndWRkVBMkRsM2QvTi9MY01MbVRmRkRtWHNpbUhzVCtEei9sSDBO?=
 =?utf-8?B?RW1hVkRwNkF5eXczbHlOSWlBL3lCakJTc0cxYlR1WTRWOEVnaFc4d3Q0ODlJ?=
 =?utf-8?B?TE5TMHY3cDV1V3RpaDdIY0Radm1xTjF2MlpzckY3SFhueW12eVJOaGs3Y0xK?=
 =?utf-8?B?N1N3bFh6V1l6NTNlMzBOaVlzYTBrdHN0SHZlYWlhNmU2emZqalJ4QVZIVHJj?=
 =?utf-8?B?Z2JrRjg2MEZTK2RsRHI1ekdVTWNkeTA4U2c5RzE0aWVSS0V0allMQlNYUkVn?=
 =?utf-8?B?dFArV0pBelozN2ZIMmhKMFEvMi92NjdoMGt6eUlJMFJWZmExc0pDV1RzSWUv?=
 =?utf-8?B?K3MrelRraE1IME52NHdjQllyTVNoSTVPUFgzZGl4VER6Tm1rb05RMWlXOWZZ?=
 =?utf-8?B?aVd1cG9id2xpUm41Q0EzOGZWK1RVbDgyMHpaeldsL0prc1F1cjAzVlpYSmlq?=
 =?utf-8?B?a0Y1bGJMWjZUVS8xelpLcmdnUnZVWEVPMjFmcUxHdEJDcVZQSS9ZSnRjWG1s?=
 =?utf-8?B?bGxkMzVxMjg5N2FiYVpkZWtmajBLcXVKdDFMWlBVWS9ETGx6Z3JvWHkweG1Q?=
 =?utf-8?B?a2x4RVRvNEtidFhSTUNwRS9RN2IzTjdFM0cvYTVqc1h0ODVRR1VlUT09?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6052b76e-8e61-4aba-df5a-08da2703850e
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4257.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Apr 2022 21:35:29.8325
 (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: 8TsV1dpyVQky/9hXcAYe0XDCh46iG8N+DxaoaudNwnFl4DwTW8hEPo67OEjXgTy9QIOxRrZLXVL9urrTk4N2fw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR10MB3517
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.486,18.0.858
 definitions=2022-04-25_08:2022-04-25,2022-04-25 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 mlxlogscore=999
 suspectscore=0 malwarescore=0 adultscore=0 spamscore=0 mlxscore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2202240000 definitions=main-2204250095
X-Proofpoint-GUID: 6sqLVuETyY1azGHMVKy4QAKqXuiN-EM7
X-Proofpoint-ORIG-GUID: 6sqLVuETyY1azGHMVKy4QAKqXuiN-EM7
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/25/22 1:40 PM, J. Bruce Fields wrote:
> On Mon, Apr 25, 2022 at 12:42:58PM -0700, dai.ngo@oracle.com wrote:
>> On 4/25/22 11:51 AM, J. Bruce Fields wrote:
>>> On Sat, Apr 23, 2022 at 11:44:09AM -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 client is resolved by setting it
>>>> to EXPIRABLE, queue work for the laundromat and return delay to
>>>> 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 state is allowed to reconnect and continues to
>>>> have access to its state while client in EXPIRABLE state is not.
>>>> To prevent 2 clients with the same name are on the conf_name_tree,
>>>> nfsd4_setclientid is modified to expire confirmed client in EXPIRABLE
>>>> state.
>>>>
>>>> The spinlock cl_cs_lock is used to handle race conditions between
>>>> conflict resolver, nfsd_break_deleg_cb, and the COURTESY client
>>>> doing reconnect.
>>>>
>>>> find_in_sessionid_hashtbl, find_confirmed_client_by_name and
>>>> find_confirmed_client are modified to activate COURTESY client by
>>>> setting it to ACTIVE state and skip client in EXPIRABLE state.
>>>>
>>>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>>>> ---
>>>>   fs/nfsd/nfs4state.c | 129 +++++++++++++++++++++++++++++++++++++++++++++-------
>>>>   fs/nfsd/nfsd.h      |   1 +
>>>>   fs/nfsd/state.h     |  63 +++++++++++++++++++++++++
>>>>   3 files changed, 177 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>> index 234e852fcdfa..fea5e24e7d94 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;
>>>> @@ -690,6 +692,14 @@ static unsigned int file_hashval(struct svc_fh *fh)
>>>>   static struct hlist_head file_hashtbl[FILE_HASH_SIZE];
>>>> +static inline void
>>>> +run_laundromat(struct nfsd_net *nn, bool wait)
>>>> +{
>>>> +	mod_delayed_work(laundry_wq, &nn->laundromat_work, 0);
>>>> +	if (wait)
>>>> +		flush_workqueue(laundry_wq);
>>>> +}
>>> Let's keep those two things separate.  The "wait" argument isn't
>>> self-documenting when reading the calling code.
>> we can use enum to spell out the intention.
>>
>>>    And the
>>> mod_delayed_work call isn't always needed before flush_workqueue.
>> ok. I'll keep them separate.
>>
>>>> +
>>>>   static void
>>>>   __nfs4_file_get_access(struct nfs4_file *fp, u32 access)
>>>>   {
>>>> @@ -1939,6 +1949,11 @@ find_in_sessionid_hashtbl(struct nfs4_sessionid *sessionid, struct net *net,
>>>>   	session = __find_in_sessionid_hashtbl(sessionid, net);
>>>>   	if (!session)
>>>>   		goto out;
>>>> +	if (!try_to_activate_client(session->se_client)) {
>>>> +		/* client is EXPIRABLE */
>>>> +		session = NULL;
>>>> +		goto out;
>>> No, we definitely don't want to do this.  As I said before, an
>>> "expirable client" should be treated in every way exactly like any
>>> regular active client.  Literally the only difference is that the
>>> laundromat can try to expire it.
>> Do you mean leave the state as EXPIRABLE but allow the callers
>> to use the client as an ACTIVE client:
> The only place we should even be checking whether a client is EXPIRABLE
> is in the laundromat thread, in the loop over the client_lru.  Even
> there, EXPIRABLE being set doesn't mean the client *has* to be expired,
> the real decision is left to mark_client_expired_locked(), as it
> currently is.
>
>> static inline bool try_to_expire_client(struct nfs4_client *clp)
>> {
>>          bool ret;
>>
>>          spin_lock(&clp->cl_cs_lock);
>>          if (clp->cl_state == NFSD4_EXPIRABLE) {
>>                  spin_unlock(&clp->cl_cs_lock);
>>                  return false;            <<<====== was true
>>          }
>>          ret = NFSD4_COURTESY ==
>>                  cmpxchg(&clp->cl_state, NFSD4_COURTESY, NFSD4_EXPIRABLE);
>>          spin_unlock(&clp->cl_cs_lock);
>>          return ret;
>> }
> So, try_to_expire_client(), as I said, should be just
>
>    static bool try_to_expire_client(struct nfs4_client *clp)
>    {
> 	return COURTESY == cmpxchg(clp->cl_state, COURTESY, EXPIRABLE);
>    }
>
> Except, OK, I did forget that somebody else could have already set
> EXPIRABLE, and we still want to tell the caller to retry in that case,
> so, oops, let's make it:
>
> 	return ACTIVE != cmpxchg(clp->cl_state, COURTESY, EXPIRABLE);

So functionally this is the same as what i have in the patch, except this
makes it simpler?

Do we need to make any change in try_to_activate_client to work with
this change in try_to_expire_client?

>
> In other words: set EXPIRABLE if and only if the state is COURTESY, and
> then tell the caller to retry the operation if and only if the state was
> previously COURTESY or EXPIRABLE.
>
> But we shouldn't need the cl_cs_lock,

The cl_cs_lock is to synchronize between COURTESY client trying to
reconnect (try_to_activate_client) and another thread is trying to
resolve a delegation/share/lock conflict (try_to_expire_client).
So you don't think this is necessary?

-Dai

>   and certainly shouldn't be failing
> lookups of EXPIRABLE clients.
>
>
>
> --b.
>
>
>> -Dai
>>
>>> And then all this code becomes unnecessary:
>>>
>>>> @@ -702,4 +727,42 @@ extern void nfsd4_client_record_remove(struct nfs4_client *clp);
>>>>   extern int nfsd4_client_record_check(struct nfs4_client *clp);
>>>>   extern void nfsd4_record_grace_done(struct nfsd_net *nn);
>>>> +static inline bool try_to_expire_client(struct nfs4_client *clp)
>>>> +{
>>>> +	bool ret;
>>>> +
>>>> +	spin_lock(&clp->cl_cs_lock);
>>>> +	if (clp->cl_state == NFSD4_EXPIRABLE) {
>>>> +		spin_unlock(&clp->cl_cs_lock);
>>>> +		return true;
>>>> +	}
>>>> +	ret = NFSD4_COURTESY ==
>>>> +		cmpxchg(&clp->cl_state, NFSD4_COURTESY, NFSD4_EXPIRABLE);
>>>> +	spin_unlock(&clp->cl_cs_lock);
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static inline bool try_to_activate_client(struct nfs4_client *clp)
>>>> +{
>>>> +	bool ret;
>>>> +	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
>>>> +
>>>> +	/* sync with laundromat */
>>>> +	lockdep_assert_held(&nn->client_lock);
>>>> +
>>>> +	/* sync with try_to_expire_client */
>>>> +	spin_lock(&clp->cl_cs_lock);
>>>> +	if (clp->cl_state == NFSD4_ACTIVE) {
>>>> +		spin_unlock(&clp->cl_cs_lock);
>>>> +		return true;
>>>> +	}
>>>> +	if (clp->cl_state == NFSD4_EXPIRABLE) {
>>>> +		spin_unlock(&clp->cl_cs_lock);
>>>> +		return false;
>>>> +	}
>>>> +	ret = NFSD4_COURTESY ==
>>>> +		cmpxchg(&clp->cl_state, NFSD4_COURTESY, NFSD4_ACTIVE);
>>>> +	spin_unlock(&clp->cl_cs_lock);
>>>> +	return ret;
>>>> +}
>>> --b.