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;
Subject: Re: [PATCH v3 2/3] nfsd: Initial implementation of NFSv4 Courteous
 Server
To:     "J. Bruce Fields" <bfields@fieldses.org>
Cc:     chuck.lever@oracle.com, linux-nfs@vger.kernel.org,
        linux-fsdevel@vger.kernel.org
References: <20210916182212.81608-1-dai.ngo@oracle.com>
 <20210916182212.81608-3-dai.ngo@oracle.com>
 <20210922211444.GC22937@fieldses.org>
 <f38c8587-d5db-d07e-6e28-351221be4a69@oracle.com>
 <20210923011824.GD22937@fieldses.org>
From:   dai.ngo@oracle.com
Message-ID: <1ad8214a-636d-6e62-13fe-0ad0a6cf97c3@oracle.com>
Date:   Thu, 23 Sep 2021 10:09:07 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0)
 Gecko/20100101 Thunderbird/78.14.0
In-Reply-To: <20210923011824.GD22937@fieldses.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?bXFQK0lqSjdyWkFBRVE2dXdQcUFSS0RaVnorRGQzZEt2QlgxcWdKaWNPTUsv?=
 =?utf-8?B?TnhnK2pVWHFQcHlVRlNmMXE5TC9JOHh0K2Nqakt6RVFOUmlEVnR4M2dDRGRm?=
 =?utf-8?B?RWNzbzV3eENDMGcvUW5WNUFXNk5HT2FVYWVkZDBMbDVYS1JMc1EvVTJ5Y0lt?=
 =?utf-8?B?QytHTDNCNkkvTXNOYXJxRTkwQnd3ZTJHUzRCY0RMbDZ4M3h3TXJ6dGVaYTA3?=
 =?utf-8?B?MWRVVFdpV0hNYnVPTWw3Wnh6STM4azZxTytLMWttdHRRQmZ4QTZFR0JHMnBN?=
 =?utf-8?B?c0MrRXE1QVpTVFFJK1FtcXdwWUpFbUVoMEUvT2RrM2ZhU3drMGR2ZkwwWWJN?=
 =?utf-8?B?ekxLbHpwY1dyazhZU0ljcWpQZmFFTGpmYWxSNDJNTG5PcnBaaHcwK2ttYTRn?=
 =?utf-8?B?dmUzdFJZM2oyVTgwWWFWbkt3VEc0MjIwRmt4aEh5enNmS2Z3L083dlZTb3BP?=
 =?utf-8?B?VWZubXlzajZyNUE3OURydGVwQXZPWGNjZ1hyWFIyTVRwQ3lxVHYrTzdETnY2?=
 =?utf-8?B?eWprQndrdDF0UU1ydzJYMUk5ZkRDUmMyWGNBckNkbStwblBDcVRBTVFpQWx0?=
 =?utf-8?B?djJEVEpvd2R3bUJCZU11dVVBZGdGaXRDenpDaWUveUpGdkZ6dkR4WTl2Q0Zk?=
 =?utf-8?B?Z1JKUDlPM0FpK3NhTzdBUWYzcXAyTFdmN243Z1VueXUyQVlJUkhIenJTMHlh?=
 =?utf-8?B?bE1OOTdaeGdPRzVabzdyUU1JNnkwZGdkYi90c01wZU9nMUptY21tb01MRzVx?=
 =?utf-8?B?Y3dLYVdPZkhZbmhMZlUrbC92dG8xbElKNG80SUpDaUpYQlRoZnp3SHJuVWF0?=
 =?utf-8?B?THVDOXdzVGZ5bVY5NGpMbDhHQUVQN1BDOVp6SXR1dDFUUitwbXNSWEQ1UFo4?=
 =?utf-8?B?Qnd1UE9IRlI3MHRTcWN4Ulc5NFBoM09CU2hsVkMwYWc2MzFqNmx4UUpTSzll?=
 =?utf-8?B?bElnUUlsd3ltdVV2WWRoSmU2UmJ5ZXlQYzA3a0JsQktuM2p5ckR1b25pNmFU?=
 =?utf-8?B?NnBFMXhpaVRreUdyMWVLQWl5WjNNNEltalJUclFtaGs1YTU1QitmaTBMbStL?=
 =?utf-8?B?RHNsSjVnV3JPOHc0UnhmNmhYUWlET1RKWWllM29LY3RDcEp2ZEVSTk5Ed0gz?=
 =?utf-8?B?NjRxNzF3MW5BZXViUWZ0OVpQSlpiZ1czcDA1VnM4SWg2cDNYVVZ0OEd5SjY1?=
 =?utf-8?B?aEtpVFdUZmFUUmplZ2xoaXNRM3NPaFdRNmlBbURNSGtONWVyczdtWHRZbUVi?=
 =?utf-8?B?bFhzSFJmeTVMeUc5a01RbWg0U0dXUlloYmJzdSsxMTB0MmVvWTFUK2VlS2Nj?=
 =?utf-8?B?anNBYmxkQWhydzltZ0VQcWFFUkpIbDF3b1FlWkhFUkFLSnRlWmpBZU1KVEpT?=
 =?utf-8?B?UGdBV2VuUXJoOUtjRHpBcTlJRFhiVFpyZlhicmpTVXZSUi93QlI5dWFBUFcx?=
 =?utf-8?B?elJHcEpncUZOYldwMVp1dHhvazA2cXRjQVozbS8wNVdydzg2bGFZMy81ejlq?=
 =?utf-8?B?UGgwZzljSmF2bzVIb2xzdmF1YWdxSFMvNjJyclpoSzBPMzQ1bElqekZYMW0z?=
 =?utf-8?B?V0drTGYvaSs2L1crS3liZWJoWmZnTnNxVUJ5Z2ZERDE5eTBpTWlRV3RDZ1JM?=
 =?utf-8?B?bG1mSitsZ0hJL0Jjbm9jby84WGxsUGlyQnN3ekRhcmZ1WlNzaVJ1YkNpeThn?=
 =?utf-8?B?R2EwWStTZWZzSTFBb2tTVHRWVkFRVkhRMUZKTHpqVWhGTjhIbjZ4QUlhWU5p?=
 =?utf-8?Q?iqdXItj1rGz8NONCMlaGXeQ/zgelZnwQg6yLBcX?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: fb55cfd4-16a0-4b9c-b51e-08d97eb4dc20
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4257.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Sep 2021 17:09:10.4100
 (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: 42I7sWbezAxGL8BGWgguLbbaASllM2fd1TFp7jzfxg0mhjtbEYGz1yQ54z6af/rQ30ho+0BtD1TwZVraDaSe6Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB3717
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10116 signatures=668682
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 phishscore=0
 malwarescore=0 mlxlogscore=999 suspectscore=0 bulkscore=0 mlxscore=0
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2109200000 definitions=main-2109230104
X-Proofpoint-ORIG-GUID: qo_FInUVRbI0SUZ2dB1Zih7MNRmSN5OH
X-Proofpoint-GUID: qo_FInUVRbI0SUZ2dB1Zih7MNRmSN5OH
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


On 9/22/21 6:18 PM, J. Bruce Fields wrote:
> On Wed, Sep 22, 2021 at 03:16:34PM -0700, dai.ngo@oracle.com wrote:
>> On 9/22/21 2:14 PM, J. Bruce Fields wrote:
>>> On Thu, Sep 16, 2021 at 02:22:11PM -0400, Dai Ngo wrote:
>>>> +/*
>>>> + * If the conflict happens due to a NFSv4 request then check for
>>>> + * courtesy client and set rq_conflict_client so that upper layer
>>>> + * can destroy the conflict client and retry the call.
>>>> + */
>>>> +static bool
>>>> +nfsd_check_courtesy_client(struct nfs4_delegation *dp)
>>>> +{
>>>> +	struct svc_rqst *rqst;
>>>> +	struct nfs4_client *clp = dp->dl_recall.cb_clp;
>>>> +	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
>>>> +	bool ret = false;
>>>> +
>>>> +	if (!i_am_nfsd()) {
>>>> +		if (test_bit(NFSD4_COURTESY_CLIENT, &clp->cl_flags)) {
>>>> +			set_bit(NFSD4_DESTROY_COURTESY_CLIENT, &clp->cl_flags);
>>>> +			mod_delayed_work(laundry_wq, &nn->laundromat_work, 0);
>>>> +			return true;
>>>> +		}
>>>> +		return false;
>>>> +	}
>>>> +	rqst = kthread_data(current);
>>>> +	if (rqst->rq_prog != NFS_PROGRAM || rqst->rq_vers < 4)
>>>> +		return false;
>>>> +	rqst->rq_conflict_client = NULL;
>>>> +
>>>> +	spin_lock(&nn->client_lock);
>>>> +	if (test_bit(NFSD4_COURTESY_CLIENT, &clp->cl_flags) &&
>>>> +				!mark_client_expired_locked(clp)) {
>>>> +		rqst->rq_conflict_client = clp;
>>>> +		ret = true;
>>>> +	}
>>>> +	spin_unlock(&nn->client_lock);
>>> Check whether this is safe; I think the flc_lock may be taken inside of
>>> this lock elsewhere, resulting in a potential deadlock?
>>>
>>> rqst doesn't need any locking as it's only being used by this thread, so
>>> it's the client expiration stuff that's the problem, I guess.
>> mark_client_expired_locked needs to acquire cl_lock. I think the lock
>> ordering is ok, client_lock -> cl_lock. nfsd4_exchange_id uses this
>> lock ordering.
> It's flc_lock (see locks.c) that I'm worried about.  I've got a lockdep
> warning here, taking a closer look....
>
> nfsd4_release_lockowner takes clp->cl_lock and then fcl_lock.
>
> Here we're taking fcl_lock and then client_lock.
>
> As you say, elsewhere client_lock is taken and then cl_lock.
>
> So that's the loop, I think.

Thanks Bruce, I see the deadlock. We will need a new approach for this.

-Dai

>
> --b.