Message-ID: <1bf774e2-8667-b222-7cbc-ccdd24bffdb0@oracle.com>
Date:   Thu, 2 Feb 2023 15:38:58 -0800
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.6.1
Subject: Re: [PATCH] NFSD: fix deny mode logic in nfs4_upgrade_open
Content-Language: en-US
To:     Jeff Layton <jlayton@kernel.org>,
        Chuck Lever III <chuck.lever@oracle.com>
Cc:     Pumpkin <cc85nod@gmail.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
References: <20230202073611.13106-1-cc85nod@gmail.com>
 <CBAFB3E5-5737-47A7-8234-3A771E908C4F@oracle.com>
 <7b7edc8bacb863915d00673f9e03d38798341a69.camel@kernel.org>
From:   dai.ngo@oracle.com
In-Reply-To: <7b7edc8bacb863915d00673f9e03d38798341a69.camel@kernel.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?OU1GMUxyL0c1NkM0WUJDZXJaMGY3c21UMEtuZ250d3J5R2d4ODNiWGo3SW8r?=
 =?utf-8?B?VFJsU2tNVG9HeG93Sk1JaW1udW0rcjdRZFprQjNSMVhMdGNwN2JSNWw1dVBU?=
 =?utf-8?B?dU9ucVhNV3ZLbEs1a2kyV29MSHhjbm94dFAvZ3JINCtKU2s2YzAzNmdXdW9M?=
 =?utf-8?B?OUFZdlFJNFZMZmtFTTdOT2Z4Z21MQUNkS042VHFwcHRTSWtvam9MaTJzZFNt?=
 =?utf-8?B?UTRvbXYxbW91eUx1cjRwbXlHbWpwZDVHc2E4aWJ3eGxGcWMraU5RMS9jQ1FV?=
 =?utf-8?B?SFZpN1FiWVA1amJJR0c5VW9MeStUYlJ4dHpZT3JBUkZHcHFoa2JoN2VJY0RM?=
 =?utf-8?B?VHdPbnp4SzlGRjc3VUF5OERINExuK3Z2MmpRQXRlK0VBRHFubTlHVFhXV28r?=
 =?utf-8?B?QWpRYWZrNFpHTEd0RVkyNTlPdzVrM1drU0wvVHZ0WElXaUhQUmhnQUdOeFFX?=
 =?utf-8?B?eElFYnE3MXhSOUdsWkdaUWY2aVBsN2NjL0pBSXJXc2hEeWxhNHhpaG1ySnNC?=
 =?utf-8?B?eVorNHNWdktRNUlqdzU0cmVEV25iNEFBTmx4N3pzWnlMVmUyaHlBY0RRWFBo?=
 =?utf-8?B?UFJkRTU0dExnTHhJTm5UNnBZRGdkcHBkRnlwY3lrY3NzSHZFN0FUVkJCdGsw?=
 =?utf-8?B?ZkVQZm54aDBXM2ZYRS8zYnhVZWNDc2ZEbzRYRnpJWVpOTVZyRHp0TGtQKzhQ?=
 =?utf-8?B?TnRqaitFRkpTREtvcDNSSDNCVzl2YUx2OGhWOEk5amhERXlkOTlpVU56YnVn?=
 =?utf-8?B?ZjFYNE41eU1GOGY5MVRmeWlkSTFkUUlrZ2dqVTMrbHRHM0crWjRlNm5NMURW?=
 =?utf-8?B?Y2gwSWU5V292eXd1ZkZ4L0lYL0ZQRWovQTJYdjRSbXBoNGdpZU0wVnlnMWxZ?=
 =?utf-8?B?WHpETFhqcTc1SWJjcFpoOGF2T2JrZHJlTDBxU2JNaGZPVnZnL3NvNDZ2bUJn?=
 =?utf-8?B?ZU81ejZiUnQ5Y2pLNU5yblBjQTVUVEFIY0tMejUzaWdaMyszalcycVRUUmN5?=
 =?utf-8?B?c3cxUk9jVG93MlNINnk3RFJtRDZMS2l5aDUzM2xNT293UzF5RzZwM09GL2l5?=
 =?utf-8?B?QXdsRXN5ZGZ1K3czekRiOEIxbm82Vlp4QStaN0oweXNPQTFEOExsbWJNakdl?=
 =?utf-8?B?Yy9mcS9QdFJZMWRrZEFYUjZERndhTTZvTUNlb09PSU03d2Z6cE45aDNUa3ZH?=
 =?utf-8?B?MGFDN05ZS0RqYWFlWk91SzdYWDA4WmF0RkhNaEcyaExoQnc4M2QyZnNVMG5p?=
 =?utf-8?B?dmZ3a2pVUkUyemVlbDdLcmEvRGNCVEEzMnRNT1pKUmJETXdDbFpLYzVYOFU5?=
 =?utf-8?B?UExOelZnYlBMZDQ4Wmc3RURqTDlvY000WGorSlltVHdKd2JQYlZqT3duOEps?=
 =?utf-8?B?eUcyQ3N1dWZiWUdMYXI4SkJGdmdzY1JDdG1kS2ZwbFlWamQwaFlqdkpnbTRL?=
 =?utf-8?B?R0wrN1FUMXk2VUNjckYyelBhR1RyeE9ibVBkSTJkZ1ZjRi8za3NyZG5wNXFP?=
 =?utf-8?B?NFJ2eE5XT1BSUk1kYkhHc0EwZVlhNGpxSDZOczVZYk56ZEorenc4ODFyODNT?=
 =?utf-8?B?Y2dYQUM3cy9DTGdGMjRlMGlzd2VKbVptTk83TmdhZ2ZEMnZYV3hxa0FXTVFn?=
 =?utf-8?B?L0R4Z3cvMmx6TmZGNnJ0djBiOXZxb01iN0wrNHc5aWZJZXp1Zis0UHBPcy84?=
 =?utf-8?B?bzRBd3dHN2hGWVB5Z2o2TXdJUk5hQlN4OFJFWnljcDJ3RjNCNFJBZGlWMk1l?=
 =?utf-8?B?aTRNMmdGdFc4RWR6NlJkYU5JUlA4aXpoa1NINEdXcnFOU2RySkJzL3JuT1RG?=
 =?utf-8?B?Mk5Jc0kxM00zT21oMzkvalh0OURoTGtqVmtMb2srK0Q5Y3ZXaElaYnE1ODlr?=
 =?utf-8?B?UGl1ajZIci9VQVEzNUY1WWR5QzkvZWRJVGxTR2JjQkkySGNoVlQrZC9VZnBh?=
 =?utf-8?B?MjhwVU1vVmFoS1J3aTVQOGNRUloxMllyRmp6RS9zQnpxakFEL0xEOEd1Znkv?=
 =?utf-8?B?bHdPeWhDTktNd0cyZXhDUlBUMzl4a1JGY2ZscUVtWGk5amZrTjhYN01ZRkVI?=
 =?utf-8?B?ZU43K0hsanliQURtSVgyN21oRTFQOE9DczJmeEhUV1hQT2JnRGxSS2ZWc1dQ?=
 =?utf-8?Q?44qVYFtZnO0ifDqM1e0nQM0pG?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 8psUGehnTz84WCSpMB7bYYa/d+Z1o0vE+EoZ2jXUmIkfo520jeulTBBirFNCDIi1Ysv3pl5f2KwKisg1BW5tc4EuPgmKKA20tdjxN/fe3w15NgAiJ+O6Npx6XutI5lD+DGveOW76d0jPjXcQ/LV2JXK1mVWdk2sEwYBu1fjbguPv204TE4RYZPFtNrunYLch9KQNN1aFtPjpZjFqAc2jmvKBXoyu20OPUKCFqwe+OIFfMnyI2zdh9HTcK7ji8lhm+sB2Bvwwp0SboHfK8MPeSFQaiQuh5r+PXCBWLRkOMfyhW3YH3iLNeCW1hYYBFKIX6unN9eml8UEHDWM0sERfliy4HkZ+TMNMtGPNWYZHcnndrCaCym/1JKDT+PiszKr4XzIiMV6bknHK/3tE0bhlg1wDrgR6w4Iuyy2HvEN3YPgYNXGU/U1MbPJw1HiTJpI54fsjFIGV6dZT9WvdoeTUX89oURNPDEuNMj8nvKmyCvWO051SKrzTDoVY689GKdj+0aTr60gepd7cEh0JYyB1c/UZKnWq9VW+TDX7yC9T3CeDE/P7AcCsLcu9lyRvB2nwK1jAf3+P6WwjSxKIiSecg0oVdl8gp2G+0xoC1mNwUQGulM3oyIs24SXEQQb5nTbNJvCVHA60DUW2SrhTgTBtwz1hg7RQCRl0YQ0v02zuetdE1hrL5ZpLExKr8tiN6HYt/0p7JgRg59nGEo4m98OOJgnkNsnF9KIPW5GtWOhVjXe1n8+WesQy/06PV9QokU0kv7MeNKAaKfR8GDSeFMaM9sCW7QS+GhPnbj6l9U6jQOvqnVQ5UkfD+YGArQ4Ydrk+
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 820efefd-02cf-442b-9341-08db0576a9a5
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4257.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Feb 2023 23:39:01.5880
 (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: JdixWiQONpIXPY1Th8f6gvqVYmjta7/C2/I7wj8BEk/dhf/f3sojFpdDjTtAtdmGk1q7K69YCrHf4X+5Aco9dw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR10MB6819
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.219,Aquarius:18.0.930,Hydra:6.0.562,FMLib:17.11.122.1
 definitions=2023-02-02_15,2023-02-02_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 malwarescore=0
 suspectscore=0 adultscore=0 mlxscore=0 phishscore=0 mlxlogscore=999
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2212070000 definitions=main-2302020210
X-Proofpoint-GUID: ZN7iDvxVefZcyzf1E29sPQguhLm_6uB2
X-Proofpoint-ORIG-GUID: ZN7iDvxVefZcyzf1E29sPQguhLm_6uB2
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


On 2/2/23 1:22 PM, Jeff Layton wrote:
> On Thu, 2023-02-02 at 19:41 +0000, Chuck Lever III wrote:
>>> On Feb 2, 2023, at 2:36 AM, Pumpkin <cc85nod@gmail.com> wrote:
>>>
>>> If the upgrading deny mode is invalid or conflicts with other client, we
>>> should try to resolve it, but the if-condition makes those error handling
>>> cannot be executed.
>>>
>>> Signed-off-by: Pumpkin <cc85nod@gmail.com>
>>> ---
>>> fs/nfsd/nfs4state.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>> index 4ef529379..ebdfaf0f9 100644
>>> --- a/fs/nfsd/nfs4state.c
>>> +++ b/fs/nfsd/nfs4state.c
>>> @@ -5298,7 +5298,7 @@ nfs4_upgrade_open(struct svc_rqst *rqstp, struct nfs4_file *fp,
>>> 	/* test and set deny mode */
>>> 	spin_lock(&fp->fi_lock);
>>> 	status = nfs4_file_check_deny(fp, open->op_share_deny);
>>> -	if (status == nfs_ok) {
>>> +	if (status != nfs_ok) {
>>> 		if (status != nfserr_share_denied) {
>> if status == nfs_ok then status will definitely not equal
>> share_denied. So this check is a bit nonsensical as it stands.
>>
>> Usually I prefer "switch (status)" in situations like this
>> because that avoids this kind of issue and I find it easier
>> to read quickly.
>>
>> Jeff, you are the original author of this function, and
>> Dai, your commit is the last one to touch this area. Can
>> you guys have a look? The one-liner looks correct, but I
>> might be missing something.
>>
> Yeah, that code is clearly broken and it looks like it was done in
> 3d69427151806 (NFSD: add support for share reservation conflict to
> courteous server).
>
> I don't believe that one-liner is correct though. If the result is
> nfs_ok, then we want to set the deny mode here and that won't happen.

not sure what I was thinking. The check was totally wrong.

>
> Something like this maybe? (completely untested):
>
> ---------------8<-------------------
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index c39e43742dd6..af22dfdc6fcc 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5282,16 +5282,17 @@ nfs4_upgrade_open(struct svc_rqst *rqstp, struct nfs4_file *fp,
>          /* test and set deny mode */
>          spin_lock(&fp->fi_lock);
>          status = nfs4_file_check_deny(fp, open->op_share_deny);
> -       if (status == nfs_ok) {
> -               if (status != nfserr_share_denied) {
> -                       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;
> -               }
> +       switch (status) {
> +       case nfs_ok:
> +               set_deny(open->op_share_deny, stp);
> +               fp->fi_share_deny |=
> +                       (open->op_share_deny & NFS4_SHARE_DENY_BOTH);
> +               break;
> +       case nfserr_share_denied:
> +               if (nfs4_resolve_deny_conflicts_locked(fp, false,
> +                               stp, open->op_share_deny, false))
> +                       status = nfserr_jukebox;
> +               break;
>          }
>          spin_unlock(&fp->fi_lock);

LGTM.

Thanks,
-Dai