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: <07c572c3-bf4b-fcca-9a15-a8b4b627af46@oracle.com>
Date:   Sun, 30 Oct 2022 09:32:50 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.4.1
Subject: Re: [PATCH] NFSD: Fix the share reservation conflict to courteous
 server logic in nfs4_upgrade_open()
Content-Language: en-US
To:     Chuck Lever III <chuck.lever@oracle.com>,
        Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Cc:     Jeff Layton <jlayton@kernel.org>,
        Bruce Fields <bfields@fieldses.org>,
        LKML <linux-kernel@vger.kernel.org>,
        "kernel-janitors@vger.kernel.org" <kernel-janitors@vger.kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
References: <7ed2d8f1ee8c441a13b450c5e5c50f13fae3a2b9.1667114760.git.christophe.jaillet@wanadoo.fr>
 <FB543B1D-A3BF-4403-9BFD-CE3AA40A89B5@oracle.com>
From:   dai.ngo@oracle.com
In-Reply-To: <FB543B1D-A3BF-4403-9BFD-CE3AA40A89B5@oracle.com>
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?QURkWGJMWDdacGx4aGdsVzVWaTFMc2NDR2I0U2JKWlA5SFlEZmxJSEZ5cEQ1?=
 =?utf-8?B?bFlERHVWTTJxNXRkcHh3dS9WaVJJc09PVFJJczhLRGJYRkdQaEhhRml5Q09m?=
 =?utf-8?B?NW94KzloVWNzbUpMVmUzMGMwRWxYY0dTcVVkNmtITVIzcXIyaXY5RytNMUtC?=
 =?utf-8?B?M3hBSElwWDBUQ21pMkxRY1BzTWdWSGJ5L215ajZzV0JFRncvL0RWK2V3dmgx?=
 =?utf-8?B?TlVEUUxDUFdwRUZvY2pwZ1ZDUWRKVk5Wd3RzMklTY0pINVBVTDlQMCtNYXEy?=
 =?utf-8?B?Kzk5Y2RtS2NndUZnNkprL3VPNitBNjFFV1g5K0M0SC8yOXkwS29TY1VpeE9z?=
 =?utf-8?B?WlZ6blBkWFNab2swakFuTUxuaWlsays1REE3YWIvSkZLMG9FWnRFamlrbzJu?=
 =?utf-8?B?NmtZdGlZN1R4STRWRXFpbU1mdmRuZGhiRldGdEhNUzV1UW1GNldoczBSUGVm?=
 =?utf-8?B?RngwWHR3ek1PS2xzREV1SEVhcjFkREZHb2UyU09xNnY0NFljem93TExmSGVI?=
 =?utf-8?B?R0FqQXBOQjdLMTBBSTJsZ04rU2E3bjJyd1hrOGN4NzhlNEpCY2ZuakljcWl4?=
 =?utf-8?B?cVcxNzJjNUg2Yk9wdzRPMCtRYjU2d2xBUkpNSUVKOUdtajNSRWlLbUFHaktw?=
 =?utf-8?B?NnEvb3diYStBeFB1YStWOUZnNDVCS3AwT0JIMU5XbmpNSWk0bjhaaVlaOWxw?=
 =?utf-8?B?WHR4alM3RlVVNFRYMUQvdHNuMmNzMnlYZ0wyMGNEWm1vN3ViZnBTN0Y4Nzdz?=
 =?utf-8?B?UGEzZzBBaDNGSEdrbVYxYzF3Y3o5NHRndWhxMy93dGRwenRoQmpxOW1od0Rz?=
 =?utf-8?B?UVZTbEYyYkZTc294Rnd1blJHQzZ2dFpTc2w0a28yeklGV3lEQTQ4MFlRYVJy?=
 =?utf-8?B?MlVmSGlibGJmdC9aYkJZdzlrMytCdGt3UllCVFFOVThwYW5qTHFERXlPUDZG?=
 =?utf-8?B?NWc4VGpKY3I1akNaTHZlNWpxM0JoQVd5SWVtaE15VEVYTm5BUmsxcmRXOURV?=
 =?utf-8?B?QXBid1VXeDBxSS9ReVN2cU5aM1RyOU1zTTNzU1dMSzdGU1RBeWdRZi9VY1Vr?=
 =?utf-8?B?dE11OTZJSUJLUnRkczBIblNjelp5VWhmNDZnTTgvdzhlZzJIK0ZzQkgzcjJP?=
 =?utf-8?B?b3dtRTNtK0hINnRzeGFJYVR1S0VFdjBnd291R1FiS3RvWGtTN2RCTENjRmpC?=
 =?utf-8?B?RWF5WC85SlpuOS84SlBHMExMQlR3cit6azQvU251SElyYkprZGwxQU1oQ2lO?=
 =?utf-8?B?bFZERG1iSVBZc1BkZzd6YmlGa1RxaWlKMmJnT0ZIbmYzMjJNZytIemNMREF2?=
 =?utf-8?B?SGliK2JhVS8ySUxxMlJqU0hubXlxVm5Xd2RtMktBUElLU2t0QWU1UHIrTTNV?=
 =?utf-8?B?ejBnczFaMzNEa2pUQmhwdTRRUUdrU2N5NWFMd3lRRldXTWZLM1VsK3Q2SmxB?=
 =?utf-8?B?UkhiZURXV01FUTV3Q2Zxb2t2MTUzSFJUZ3ZRaTM1dFFpNVVsU1hFK1F2L1hv?=
 =?utf-8?B?ZmFlaEZUd0ZSYjZiVVFKNTNUUnpwdjVnYkxZUXJHVUxJOWxrMjBoUEhmSmJF?=
 =?utf-8?B?OFhKa1ZuV01zRkZ6WldFKzFvZEp5N2pKM2JCL3EybktrMXBBTFhMZnkvR3do?=
 =?utf-8?B?cENHZ0prcWUzZlhpYkJ0TG0ycnU3U1gwbTA3SHQzdU1QUndjMXpJb014Y3pq?=
 =?utf-8?B?ZngyOWhGdjdDZHpYR1FjclpXYk1DK0U2WmlYaXhpUm02c0UyaDUyTk1DZ2cy?=
 =?utf-8?B?UlJjNTRoUjUxS1BlenlRT0N3NCttTFJsVGtINTBGZHhPVitVWXJDTGxCUHBt?=
 =?utf-8?B?WE84QWtRcUhrNXdheHlVa0JLVlJTWHRFcG1PR3NSeittT1RlZGU3U3hUUnVQ?=
 =?utf-8?B?RHEwejJsSlFzM1NnZ1M5c2U0RkRNUTN3RG9BQkNSWmttdzZuK2hwc2JqZER4?=
 =?utf-8?B?VzhUZC9nNW1kWVIxL0c2VEc4UU45NCt1cEluazV5bWpBeElUa3VPZkxSU1FL?=
 =?utf-8?B?ZnZXSkRRZUxBaGdlMVgzbmxGb01LTXdRTm9BMERhb2J6c0FuL0YzcW1wbkcw?=
 =?utf-8?B?WkRXOG5QejFwZUhwV1Zqb1dRV2xaMjY1K1JqT3doVXZ2WGRnWEFlSGVZUW1I?=
 =?utf-8?Q?k6woPXqIbgldXJ2GMfzY3BgmU?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 93fad9dc-1d12-423b-2329-08daba94655b
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4257.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Oct 2022 16:32:54.6463
 (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: tiYnIhez419ksgUu6gLm68+0KZ5tKepRUSGqiY4+NfeUO6RuTguXrOIcB3MNUPFWO+SlrkbNxUV5oMGTzZ7Udw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR10MB4238
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-10-30_10,2022-10-27_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 bulkscore=0 suspectscore=0
 mlxlogscore=999 mlxscore=0 adultscore=0 phishscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2210170000
 definitions=main-2210300109
X-Proofpoint-ORIG-GUID: 96cNPz6YmxrEwJy90EK5Kyg7p-V-D557
X-Proofpoint-GUID: 96cNPz6YmxrEwJy90EK5Kyg7p-V-D557
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,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 10/30/22 8:26 AM, Chuck Lever III wrote:
>
>> On Oct 30, 2022, at 3:26 AM, Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
>>
>> 'status != nfserr_share_denied' is known to be true because we test
>> 'status == nfs_ok' the line just above.
>>
>> So nfs4_resolve_deny_conflicts_locked() can never be called.
>>
>> Fix the logic and avoid the dead code.
>>
>> Fixes: 3d6942715180 ("NFSD: add support for share reservation conflict to courteous server")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> This patch is speculative.
>> It is compile tested only.
>>
>> REVIEW WITH CARE.
>> ---
>> fs/nfsd/nfs4state.c | 14 ++++++--------
>> 1 file changed, 6 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 1ded89235111..de0565e9485c 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -5260,15 +5260,13 @@ nfs4_upgrade_open(struct svc_rqst *rqstp, struct nfs4_file *fp,
>> 	spin_lock(&fp->fi_lock);
>> 	status = nfs4_file_check_deny(fp, open->op_share_deny);
> I agree there's dead code here. I believe the bug is the first check is
> supposed to be "if (status != nfs_ok)". I will let Dai have a look at
> this to confirm.

Yes, it's actually a bug when nfs4_file_check_deny returns
nfserr_share_denied we won't try to resolve the conflict at all.

Thanks for catching this!
-Dai

>
> But, in the fix, let's replace this logic with "switch (status) { }".
>
>
>> 	if (status == nfs_ok) {
>> -		if (status != nfserr_share_denied) {
>> -			set_deny(open->op_share_deny, stp);
>> -			fp->fi_share_deny |=
>> +		set_deny(open->op_share_deny, stp);
>> +		fp->fi_share_deny |=
>> 				(open->op_share_deny & NFS4_SHARE_DENY_BOTH);
>> -		} else {
>> -			if (nfs4_resolve_deny_conflicts_locked(fp, false,
>> -					stp, open->op_share_deny, false))
>> -				status = nfserr_jukebox;
>> -		}
>> +	} else if (status == nfserr_share_denied) {
>> +		if (nfs4_resolve_deny_conflicts_locked(fp, false, stp,
>> +				open->op_share_deny, false))
>> +			status = nfserr_jukebox;
>> 	}
>> 	spin_unlock(&fp->fi_lock);
>>
>> -- 
>> 2.34.1
>>
> --
> Chuck Lever
>
>
>