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: <fa1d32f4-f322-1894-2663-accb35363a2f@oracle.com>
Date:   Mon, 25 Apr 2022 15:24:49 -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>
 <e2b3dfe6-d7c6-3bb1-eebb-0f8cd97c27e7@oracle.com>
 <20220425214850.GK24825@fieldses.org>
From:   dai.ngo@oracle.com
In-Reply-To: <20220425214850.GK24825@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?dXhNanNIZlBoUzNiVXhDTWxIVlJzQnN1QWI5MElWRzY4S1BTcU9zNmlBRnV0?=
 =?utf-8?B?TVNqS01hUWkrb1BWQzNIZnpkMFYvRm1zTDNLMmIzSEd0b01IcitjZHlqSmtM?=
 =?utf-8?B?V3FGUXIramxWU0VDSDVnd2pWbUpPQzlUc0lmTGlVU1p3QTFKaG1RUjRpNWYz?=
 =?utf-8?B?S0Vyc3BMem1wWm9ic1h4cTl0VnFGdVRqSmhiRnYrdGp4YzF5dCtDbTlVMVRu?=
 =?utf-8?B?NUFTK0ZiUDd1dm4zczRUNVVpVXljUnFmdkJkNzdDeldSWkRBMzhpVU5XNjNj?=
 =?utf-8?B?ZFVqNlRxdlRRYW9OQWN6ekZpM2t4ZksvR3pvMFg4ZGdmU0VMMDJkRmVuZDhl?=
 =?utf-8?B?Q3IzSDV1RnBZMTZ2YWc5Z1orYWxiTG92VTk1Qk1iMktOUnh6N1NwWlFzd09E?=
 =?utf-8?B?cVY2NUVudzJwOTFGMHBJdzBtMGZ0KysvbGNyaXdDZTA0MDFtMEhlNmpoSlRV?=
 =?utf-8?B?U1Jrd1JOVXNDTnpUZFNsaWpzQThCVE9WMmUyNEI2R3RlMXhzZ3ZJMmFWS3NS?=
 =?utf-8?B?MEdEaGpFcUhFK2x3VU9mTmZHNkxDUnpxUzAyd01SZVBtT2RTKzZoaGZ6cmlU?=
 =?utf-8?B?ZlMyZkJaU25QRFloaXZvRlhEbEliM3hRWTV0dzRkMmN6UFoxQWZNSklWQU9t?=
 =?utf-8?B?NE95TWFtZzFnQUxJUUhXdWc1YmxsVVNKQzVHcHZ5elVVUFNVaFV2WXgrNUg4?=
 =?utf-8?B?N0pkcFhzYVNBcVAvQjhFMFp5VGNBclB4bitmb2o5ejRtalRuYzRCaVJaWW1n?=
 =?utf-8?B?ZjM3ZjBuSE9KWk83eERMVnFLQVAwZW1Dd0gzMnZRSXB4VFFPdGJtNmxyUEhO?=
 =?utf-8?B?YnFSZFpUWVRqTG1SYlJiUGlVVytQQ21lRWdoclE0WUg3RXp0ZUhVTm1zT2Ni?=
 =?utf-8?B?cVlvRnh5TXJSckFHUnRSWGVOWlJ5WTVPd3k2WE1uNUo5d3hPbXovMDlDOVU5?=
 =?utf-8?B?anhRaVZ4M0x4VGwrQ2ZrOEErVjJRV01HR1ZlT21RL1VXUnZKK1JYRDlCK3Ny?=
 =?utf-8?B?dEZhVDBYU3dTRktTcHlINVZsd2JobUJXUXk3Yy8vZmhZNVhUUFRZZHMvK0xr?=
 =?utf-8?B?ZzZJUjJMVXNUejJkNk44UjJKci9wYjRsRzJpYWF6alJnaSt0ZUd1bURzUEtY?=
 =?utf-8?B?L01Ib0ZwVTJZU3prVEtrS2dpbE5CYmVQZW1WVkxseVlCRWUwOWdQQlBNR0tY?=
 =?utf-8?B?YlB4NXRKTjJHdmI4b0lIOGt2Qm1TV284ekZxcXRocTNmWjJlSDlMc2tDYkxh?=
 =?utf-8?B?aFZlcXZ6bzNRaVVmdzJzRFFNRzJzdUxDcjRIV2FBeDQrUFdkSGRLWjNicnZH?=
 =?utf-8?B?bW5WSmhnWWc3dUtOdDRadzZSUGFUVkVrUnJRcGNqTG1qTThFbms2aEIwbDNW?=
 =?utf-8?B?bVhKb3hjdWMxbnVTWTdqM3ExVkZqUW15TlBtc2dCbVJkcEEya0ZwQUhLbTUy?=
 =?utf-8?B?cmk5Sm9vT0o4OUlBd3h0T1NGcmRRWGlkSXN5NVJPVHp5dUpkWVFUeEV5WGkv?=
 =?utf-8?B?RktoTmRCcTBSLytCNVFsVFlTNHNTSmUwK01BYkhNSkJNV0tKZ2xPN2NpRXRp?=
 =?utf-8?B?RVJ5UzJMQU05Q0pralNkMVlLblBEUDJwcHByOVVseXpZZEFmelM5MkovTk5t?=
 =?utf-8?B?L2Z3SUJsLzFjYXJidjBIWU1XR2duR0czUlZMZDZIMnNBd3FaVDNOcUlVU2ta?=
 =?utf-8?B?bmtTcGZUeEY3ekVWRkFKQnlMQVhFd3VkRWFsN0dqRVM5UUFxR3pJcVk1UnRB?=
 =?utf-8?B?Q2J3WHJpWFpaU3dMVXRKOHIraUZKbWhyOGFCU2p2Ny9jSFFsTGh5YjhLNlIv?=
 =?utf-8?B?OWdYZUJLSEZzd0Z4c0ZJcWM1SVJGMmhVc080KzR3Mi9pNTdwNjh3SmpENCts?=
 =?utf-8?B?SWhzSUVmUjgwc1FhYTFwUVJBRitmbWlEVE0rWjM4cGJMbGdMUWJGSjJEcUNt?=
 =?utf-8?B?T2lJeVdUNFNnU3p2VUpmRDRwOW1makJDc293YlozZVZxMWFoNHJqcGdEQ1g1?=
 =?utf-8?B?WTNvaFFXQTg3OWxyS3dNaUlCMEhQSlkwVUE2cXhZNTh0ZWVxNG13UHVlM0Vp?=
 =?utf-8?B?VlViVHc3bXlsUlNRZ0FGWGorRHoyQUcrUGh2OUtsSUxtWmtIS2gwVFJCN3N2?=
 =?utf-8?B?bmhLY1hzeHBrT0xPOWM2aDQyZ0diR3YzV3pLdUhUOW9CbE03NTNBSGRyQndD?=
 =?utf-8?B?R1dKaFBoQStoQittQzJtTlYxTUw3WWNJUDkrRlp1OE11ME5DUnlJak9lN3pB?=
 =?utf-8?B?RFpBampoNzNNSWp2NDVCZjFRRk95a2crZUdwYWhNb0lIa2lvUXlVdHNIa1Vy?=
 =?utf-8?B?MGtZZlBGb0U0SWhnTXFOOXZmSzBLcDI3aGliU3RsM1A4KzZFRVpqUT09?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d3028752-a81e-423f-6731-08da270a6a17
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4257.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Apr 2022 22:24:51.0670
 (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: iLalVGHggElJj8eoB7pINQhvipgl8q3DYOwk+YZr8cg4QrXrC3yOf92vn+VdgYENAouWMMqeNo13D4dIrN4RBg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR10MB1587
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-2204250099
X-Proofpoint-ORIG-GUID: H-t48i3FrSf9CnA6FA-OAdzvAEsqbQ88
X-Proofpoint-GUID: H-t48i3FrSf9CnA6FA-OAdzvAEsqbQ88
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 2:48 PM, J. Bruce Fields wrote:
> On Mon, Apr 25, 2022 at 02:35:27PM -0700, dai.ngo@oracle.com wrote:
>> 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:
>>>> 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?
> Right.
>
> And my main complaint is still about the code that fails lookups of
> EXPIRABLE clients.  We shouldn't need to modify find_in_sessionid_hastbl
> or other client lookups.
>
>> 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?
> Correct, it's not necessary.
>
> The only synchronization is provided by mark_client_expired_locked and
> get_client_locked.
>
> We don't need try_to_activate_client.  Just set cl_state to ACTIVE in
> get_client_locked.

ok, what you suggested seems to work. I will remove try_to_activate_client
and just set cl_state to ACTIVE and test it to see if there is any problems
that we haven't thought of with this scheme.

-Dai

>
> --b.