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: <546eb88d-85dc-1cd5-9a3f-b11f3eb144ea@oracle.com>
Date:   Sun, 21 May 2023 19:56:08 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.11.0
Subject: Re: [PATCH v4 3/4] NFSD: handle GETATTR conflict with write
 delegation
Content-Language: en-US
To:     Jeff Layton <jlayton@kernel.org>, chuck.lever@oracle.com
Cc:     linux-nfs@vger.kernel.org, linux-fsdevel@vger.kernel.org
References: <1684618595-4178-1-git-send-email-dai.ngo@oracle.com>
 <1684618595-4178-4-git-send-email-dai.ngo@oracle.com>
 <d3ae1575dcdc44d1822a5b6a4ffd09b12c600374.camel@kernel.org>
From:   dai.ngo@oracle.com
In-Reply-To: <d3ae1575dcdc44d1822a5b6a4ffd09b12c600374.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?SWJQc1hqZ0dEdkdqNERZS3dhekxST3UzQjZTdjVsc1NPMXBLUUFlUkt5dzhm?=
 =?utf-8?B?WVZQNnhtUWFqU0xSUWpVcTRPUEFUYllLcEYvRHdSZWYrUUxEUGhtTFpSRkJw?=
 =?utf-8?B?WFU5eGg4cWxtNmsyWVFETkcwQUlWZ25USExBUUNRcEg0WTV0UElOd0dMMnoy?=
 =?utf-8?B?VUdzV1RZY2dVanNRV1VvSEJoK2NNSzBuNUpOeDQ0MWQ4TXRadGpyZjlGU2ZE?=
 =?utf-8?B?K3FFVm5MMkFPREdVSU5ieWUwWm9YNWFCcnZ5d2JSVG5kOE83WnhqRGFjbGJD?=
 =?utf-8?B?OEszV1N1OUlKcXNHSGtiRGlVS1g4M1FXaG42R0ZaTGlNTlNUeCtTb3Z0Mzl6?=
 =?utf-8?B?KzArNFlReFAvRXlFVlM0SVBrRVc0blllSjRzUHVjWE1SY0djUzVzMTJDMEtD?=
 =?utf-8?B?V3dBc2JBREMwcWU5b0VBOTBVeFd6TjdseEFrRmdZclpYV2s4bnF2Ni9VYWZF?=
 =?utf-8?B?cW5XRkF2dW5JMS9FVzdGMWFuYlE4QzRyczlNMTRpN0JITzNHVElpbkNZNkFt?=
 =?utf-8?B?aDdiRHpPK1hmQ1RmQy9DOFZFUnIzTk1ZZzEwT3d1NS9aV3RLKzFDeWZnblE3?=
 =?utf-8?B?eFdsN2xWbnVhalFHME5rM0FzYnFRMit2cEt0Nk51cEY3RjVxeUJwclhxTU53?=
 =?utf-8?B?d2srT0JHYkhxbEpSLzZ3ODQ5b3dHTWE1c05jN2w2VlFlQzhTYldwN3p0eTlC?=
 =?utf-8?B?ZCsvZ3VXZUdYamZPTjg3TW1vTVJPV21FTXFEcG8vUFVhSEtqTEVXVWFHQmdL?=
 =?utf-8?B?UGFkZTJnZm5mZEdZSHYrWFJWTW1FYVZmRFVQR0ZqWlJ1ZlllWjlqNElNYlhn?=
 =?utf-8?B?NGhYQXFKS0k3YXh5VkhlSGpDM2h4aHdtSEIxbEk4aDRJZjM3NTRmUnVqTDhK?=
 =?utf-8?B?YmtNNk8xTnZxNUo2NVlBTzhPMjNOY09CWXhibGRsVHhCU2JIWE5HWnVNUDcy?=
 =?utf-8?B?NHdpb3ZzaHRlK2lZaWplUmEzMmpiRGNyR0tsT20xb2o0b1ZzTzR5N1UvZmhY?=
 =?utf-8?B?NDVEVGNuL25pTzZaU1BROVkzN2prMmNRZkQ0c1VORkJBRDl6TnFXajE4SzZU?=
 =?utf-8?B?a3kwT3RqcFl6SmV0OFNoL1NDeGg1TGJLc3NaakI0aURSUTdSWUZPc0ZXWnow?=
 =?utf-8?B?cHMzbjZsVDJOKy9uOFZKNURoL3VENmFKN0hLOERhaGJnaWw3a1F5ejF2OEM2?=
 =?utf-8?B?OEFKSzkxd3YwSHBFWUwwckNtYmJ5YUdvakhPdXUyUnEzcTYwQ3NOcldxMXRt?=
 =?utf-8?B?MWxTWjRCOVUwbnl3WWtoSG4weU5TeFJmRmFGdGxQcUJRNFFxSU5ZTDA5bElX?=
 =?utf-8?B?R05GSG1WUHZ2NFdrQ1UxOGhqRDlJOHJMYitLbFlsN2VveENZRS9jSGJHRmx0?=
 =?utf-8?B?TlpQVmhDbWVtelhiM1JzUlZkb2FkT1lqcnl4NDlsaSsvZm94YW5xeEU2RW8w?=
 =?utf-8?B?cy95Q2lxOWF0QnBlN1dGbUpzdVNMTkV5a2lxNUhCcTUrYTV3Y0MxL2llQUM1?=
 =?utf-8?B?ZWV5MU9CSzlZRDh6NysvM0tTamVKNlNLVGRHTzZwOGcvQ0hYUjlJYWM5NGZJ?=
 =?utf-8?B?VmE4TGRUOGFQWGFrTi80a1ZBOW5ML2JKYW5COG5xZW5ybHlyQWJSOTdvYlh3?=
 =?utf-8?B?MWdacnlzaGVOdW40UUNMR1JpMjZkT3IxVTh5RHhTbktLWVl4VThSbUtOdHQ1?=
 =?utf-8?B?ZDB1RStTWmpvT213cUJrcGpIeUhudXRvZ3lOd3FaaEVYRy9FVmNRM2x0Z3lN?=
 =?utf-8?B?Vy9hbUVMZXN4SlZOb3M4VCtTeUV2ZkFCRzk0UXd4VkEzQXBHNU5POWx0VC9O?=
 =?utf-8?B?Y29CcnAwNGpaSUFOUWtiVW9jZFlOcjBIOXJvWWdzM25WazY4OVRZSlc3QTRN?=
 =?utf-8?B?VVlHUkpobGY2NkF6WXFsZUVtTm90MFo1RzdGeGFzWXBxNlR2eG5rS1hTdzhl?=
 =?utf-8?B?RFlnY3Y3dnFJd1g1T2htcDlEdnovcmN6WEpFdTRjUTFMVmZ3b2NMU3NlSmJL?=
 =?utf-8?B?RkQvYktjZjNQTnd1bThRcHJocC9MY2NTYTdWeDJ0czhxQlptR3NYbTZsU3Vt?=
 =?utf-8?B?K1NibzRxekd5ZTR1SFU4RXFXd281dW0vcGdvWnJvdkN2Wm9QM1BIb2h4UnRi?=
 =?utf-8?Q?h/d7IF8nSv9hkTVdP3kP8yvYy?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: g+awspKrLS2He8tA+4qB1YmX/A45g6Uocj40htOd/Q8mHfEHxE5Cx/PotCZers43AxlMVgmY80UTAF/KGRmN3lHSD1uNkbpV8dS2nTiqGY56cqugvmbl+xKulIXMlM58mIaNAKmT3j+i6PSvjB4/Em+9B6rwK6bAuhVhU5IkPcY2dtaRVMN7GcqL13KoIDXxsyhRQvt6guhsLSk9djLo2+q0d0WI+mw6dHzW2kvfreg+opShHT181f+Rt94M61Akv+Gk9qN5RU7SgmVo6eGKqmRElFzPn9evO6ogOWauE1474aDG4mf6JGPsRl+/zf69+eXh8rX3I/I/Keu3Rgut2dJ3M74X7nh8KEoJOZpEVA3K0QdFX4DLUevt+g/dAcP3+bIbEAe9gBrGTb8rZ6TScqY/Ek6mUyY1Cy/tyWgEmXAa12CIZ8yETEeli1pfAf3NafBBprI+h2paRvBf56Y9HIiLhN1ApJxAHeVUXReg1wBuEgmA/iciB9+9jMPyzETd3JMaaYUY4iKoH5PynmClb1jQlzEXADMtO0EvuTcCEuuQ7NiN8kzAGRvEmanqLvSXoXvL1Fe7Rlctsum0yn8Zhb3EOguRnlSjee+EnplEOVVsijVALA1P2K+ZlU09u7HunIvBc1hEDXdbzOcL14xOjD2HLKSNVzIR7YvjEWxnnuL9Dj9LfRsJEC8kvfshaqnj4/8JG/rqi+WHussJYkTyer1SZFwfOJ2+VELrM8jEes+wAbkeLv26uwVwoIKCwiQMEmeGf72t9CGKmCB3+FoFP8CRvwPyae0j43ITZYv9Tws=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5b115b55-5578-4116-8dc0-08db5a7019bb
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4257.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 May 2023 02:56:12.0989
 (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: pz5EHEdFssrcEFyVvNcQDCuZ3A23lk+9MjNb2Qu47SS6xtCsTi7i6oI51XzXoqWWyP5ewEUbx8oIOWiVqTU4Yw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR10MB4706
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.573,FMLib:17.11.170.22
 definitions=2023-05-21_18,2023-05-17_02,2023-02-09_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 malwarescore=0 bulkscore=0
 mlxlogscore=999 mlxscore=0 adultscore=0 spamscore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2304280000
 definitions=main-2305220023
X-Proofpoint-GUID: fPfYMc7YHrGEAd3o5KXMJ3lwQyHZ7FiF
X-Proofpoint-ORIG-GUID: fPfYMc7YHrGEAd3o5KXMJ3lwQyHZ7FiF
X-Spam-Status: No, score=-2.9 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,T_SCC_BODY_TEXT_LINE
        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 5/21/23 4:08 PM, Jeff Layton wrote:
> On Sat, 2023-05-20 at 14:36 -0700, Dai Ngo wrote:
>> If the GETATTR request on a file that has write delegation in effect
>> and the request attributes include the change info and size attribute
>> then the write delegation is recalled and NFS4ERR_DELAY is returned
>> for the GETATTR.
>>
>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>> ---
>>   fs/nfsd/nfs4xdr.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 45 insertions(+)
>>
>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>> index 76db2fe29624..e069b970f136 100644
>> --- a/fs/nfsd/nfs4xdr.c
>> +++ b/fs/nfsd/nfs4xdr.c
>> @@ -2920,6 +2920,46 @@ nfsd4_encode_bitmap(struct xdr_stream *xdr, u32 bmval0, u32 bmval1, u32 bmval2)
>>   	return nfserr_resource;
>>   }
>>   
>> +static struct file_lock *
>> +nfs4_wrdeleg_filelock(struct svc_rqst *rqstp, struct inode *inode)
>> +{
>> +	struct file_lock_context *ctx;
>> +	struct file_lock *fl;
>> +
>> +	ctx = locks_inode_context(inode);
>> +	if (!ctx)
>> +		return NULL;
>> +	spin_lock(&ctx->flc_lock);
>> +	if (!list_empty(&ctx->flc_lease)) {
>> +		fl = list_first_entry(&ctx->flc_lease,
>> +					struct file_lock, fl_list);
>> +		if (fl->fl_type == F_WRLCK) {
>> +			spin_unlock(&ctx->flc_lock);
>> +			return fl;
>> +		}
>> +	}
>> +	spin_unlock(&ctx->flc_lock);
>> +	return NULL;
>> +}
>> +
>> +static __be32
>> +nfs4_handle_wrdeleg_conflict(struct svc_rqst *rqstp, struct inode *inode)
>> +{
>> +	__be32 status;
>> +	struct file_lock *fl;
>> +	struct nfs4_delegation *dp;
>> +
>> +	fl = nfs4_wrdeleg_filelock(rqstp, inode);
>> +	if (!fl)
>> +		return 0;
>> +	dp = fl->fl_owner;
>> +	if (dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker))
>> +		return 0;
>> +	refcount_inc(&dp->dl_stid.sc_count);
> Another question: Why are you taking a reference here at all?

This is same as in nfsd_break_one_deleg and revoke_delegation.
I think it is to prevent the delegation to be freed while delegation
is being recalled.

>   AFAICT,
> you don't even look at the delegation again after that point, so it's
> not clear to me who's responsible for putting that reference.

In v2, the sc_count is decrement by nfs4_put_stid. I forgot to do that
in V4. I'll add it back in v5.

Thanks,
-Dai

>
>> +	status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
>> +	return status;
>> +}
>> +
>>   /*
>>    * Note: @fhp can be NULL; in this case, we might have to compose the filehandle
>>    * ourselves.
>> @@ -2966,6 +3006,11 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
>>   		if (status)
>>   			goto out;
>>   	}
>> +	if (bmval0 & (FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE)) {
>> +		status = nfs4_handle_wrdeleg_conflict(rqstp, d_inode(dentry));
>> +		if (status)
>> +			goto out;
>> +	}
>>   
>>   	err = vfs_getattr(&path, &stat,
>>   			  STATX_BASIC_STATS | STATX_BTIME | STATX_CHANGE_COOKIE,