Received-SPF: pass (google.com: domain of linux-nfs+bounces-1960-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 15 Feb 2024 08:55:44 -0500
From: Chuck Lever <chuck.lever@oracle.com>
To: dai.ngo@oracle.com
Cc: jlayton@kernel.org, linux-nfs@vger.kernel.org
Subject: Re: [PATCH 2/2] NFSD: handle GETATTR conflict with write delegation
Message-ID: <Zc4X4HsBZiXS7lbX@tissot.1015granger.net>
References: <1707846447-21042-1-git-send-email-dai.ngo@oracle.com>
 <1707846447-21042-3-git-send-email-dai.ngo@oracle.com>
 <ZczTPSSCmKSmdSnB@tissot.1015granger.net>
 <7994e006-b2e7-4c9b-9644-52225e1d9594@oracle.com>
 <Zc0DEuJ+jYevKc3Y@tissot.1015granger.net>
 <3c26b22d-5b9f-4a8d-bb03-ddfe802e87c1@oracle.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <3c26b22d-5b9f-4a8d-bb03-ddfe802e87c1@oracle.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?f442aKdlQGT8xbyWZhT7m0tX0lAZvBMAaGRaIqovJuOIbmSXBSW8VRn/CmmU?=
 =?us-ascii?Q?hsu+jwJjyc2dtq8G2EDoAq0Hs8t6SEIqjijU9X1KWJzPMqhob9Su/cgPXtzb?=
 =?us-ascii?Q?I5wdufUMOxesrusT9ydNArwfdzdnBk1u5zx1wOM9prd7yBgp+McU6toTYBkJ?=
 =?us-ascii?Q?dlfw6zVYQRIo3yEK9mckDrRXGem9lUVIdjRiPCfWczWn3llOhZ3Ldl+lxgOT?=
 =?us-ascii?Q?omb/Oh27LJHUtqJ+uQSIMjok/sSVLw+Adlva/MIzYjRJ/EmELpydIvziLL69?=
 =?us-ascii?Q?gAwnGND2DFlPQvvS+W5UExgtDw0KtmYSoL0VhwzD9cnnchZKEa4fRQIYYYxH?=
 =?us-ascii?Q?DM6oisJR6WEVXfFJvpJ97T/1qT//ogAFMyWa7hVPt4WFKJZQfGLhVHLYKQlo?=
 =?us-ascii?Q?gl4xDZSm4qKefIobq3Au0qjyrfp8mPT6jhzoIHGAnuKmtvGSAsvI0dxwH6yR?=
 =?us-ascii?Q?ZtjklqfI87+Q9JJwnvf9apdQSkfjQEqPczsfdATyoNKfqhRNo4cF5fKTrQmL?=
 =?us-ascii?Q?DSIOdjM7pfxzALurZbdarLxbvu44FIqnXCyJjhvHYwb8iEekFuPEAE2CT4qI?=
 =?us-ascii?Q?i9nQ2j9hh7I12gSscMBmE6DJB9T1a8Q71UHnGOYrnQmKxIqTSsnWaRkLePAK?=
 =?us-ascii?Q?DKw9VGr3WLsJlY33KKDuyWZu2m8NgwBiWWDCgaT+LjTq8cdd66wy7zk9UYVa?=
 =?us-ascii?Q?26SwGs4HmT9KfTnBl1FsbcVqkgcjCxJKCn3Ily+1fwhML+CallffCDMzFivs?=
 =?us-ascii?Q?JIY8jqViR2CMKQuy1zsLw/xqv9p29ZjGmVYTKvb+VBAYzl0wm7AXnI2/xeLW?=
 =?us-ascii?Q?7RcnmSzcMnqmts3LsFvsMshaOA5Oszl23RTXLRJLoWBVZL/2G/VsX85e9VlK?=
 =?us-ascii?Q?UdV2aCPH5d4/SUnx/H0/3TUIgf2TYjEHJBIYx9nYeju54cZLygWQEtwRNruA?=
 =?us-ascii?Q?Dqh45OXotk+G89nPRHGi1aiX8aCe8aN9n7yyhnO0YQLjwOzpkhx1qWhGDyGR?=
 =?us-ascii?Q?afbIagdszUwmM2VgBK/eghp4Wze7cefoE1Uldgur/b5fsEj0NLROv2h45FIv?=
 =?us-ascii?Q?oW4CV5chCcGFLVFJtl/0kX+uOIxUKVvWXqF7R1n7vAmshzjcYQjtHSiU0yLt?=
 =?us-ascii?Q?rcyDI2y/be8EPOO6ktZsNUwYStUSbR8Ak5YX0qm5AvanpDR9HPGyLV+ZVDGL?=
 =?us-ascii?Q?hirQgbpZQyAMX8a4Y2Ll3+aKs2zvA8E88FRDb1OkLtZAynXQgVygRMV0lyuA?=
 =?us-ascii?Q?CnJTpjVVZO2dpumU7SNbffXHkT2ptcPrep/6tbDmfor36RK/R8IfL+Twql3u?=
 =?us-ascii?Q?+b+/MZHPzVfAMr1Uj/B7uLgq/GG6Zrl55P/5dFEqvqLLE6SfWB6jU+RTEoux?=
 =?us-ascii?Q?DRMQjubjKgfanpTmNhcTzQ/y8yUzCb2p71g5jjPvafhPjuNlE1WB/641nQKC?=
 =?us-ascii?Q?zMkdvubauZTfrVRSME+Ew87pe2q42XTKrlLF2fYwZU+8k4t+7/Hhg72+4BJA?=
 =?us-ascii?Q?IRMSGD/Vg788e76RL7TbQQo3KhjnatX/u2PM10AmAEm+fklltUlxrI1kjG4Q?=
 =?us-ascii?Q?JwCTo4QlQW0NeDD/r6srBgcyCKo32ydj8ASlrGqLcZ7daEiM518KkSGZ9boi?=
 =?us-ascii?Q?YQ=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	8WzPydOkYOKqItTCkwRSawW/UYvjGyuGx3pwo88N1oDDzct4Y38ryIdjq52tzXwOTjIK/0PbiUvTwqUFWrTh0E/Abd26binlWCldaNbkFdHEA9CGtH6cpenmoTzhnvuco2EKvnC4o1AMWPL47CII0nCkjrsuSZDDA3/izo30hEEXnn/NYZzhkBpq+oXqL6G0S1LBjKb84DP6WuTm0px0Xa/Z8AtIpvpicHXFRVHU4Gqpq5piwfkagERQgIb8INhX9zklRN7GHYOOm6uK68oR1ekJhdEubxqz4tBPWfw6rH0UHswnAxcrJkVfRDBvtsCQPRJw/MfzeNqHU+BRLOv+nG5ozNICAZ1pLj1kWTdn7InJKzxYfzVYlvbSeUpW/bhlflMo3j5HdeO/qHY4uxRUUxrDHdft2EdVSS/t+eCmFHGmv76HEwWrlAX1zzH6nkLeQcc25FPOJEjaWyKUkQugAmnga6FvKeR0PnXc+OTyOaWWDd8vVa3PtqFzjo+gc/OOX0sJ/NZT/n04EmQfTU+aB4Xoge6q4loFmJSZdM+XubeE81bKpPf7nAqt+ELi315YEh7ZWcg0pxyphdASarI/xjmxQOvI+Jnt5CHWub9ya+U=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c0ab36ec-61e7-42ed-3ef0-08dc2e2dcf72
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Feb 2024 13:55:47.1417
 (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: URbvH/pk5/OntLBoxMxn4Lwgfg1kuR2kR8X+CCetBXdGsUdkzmqxicIHKEhBl1m4lSv8ttYjXIFngkEgrdeBMQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA3PR10MB6950
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-02-15_12,2024-02-14_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 bulkscore=0 suspectscore=0
 phishscore=0 adultscore=0 spamscore=0 mlxlogscore=999 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2402150112
X-Proofpoint-ORIG-GUID: rkucsyZ_yCZIO7cJPynKhPVxnLoDsDwL
X-Proofpoint-GUID: rkucsyZ_yCZIO7cJPynKhPVxnLoDsDwL

On Wed, Feb 14, 2024 at 10:22:05AM -0800, dai.ngo@oracle.com wrote:
> 
> On 2/14/24 10:14 AM, Chuck Lever wrote:
> > On Wed, Feb 14, 2024 at 10:10:50AM -0800, dai.ngo@oracle.com wrote:
> > > On 2/14/24 6:50 AM, Chuck Lever wrote:
> > > > On Tue, Feb 13, 2024 at 09:47:27AM -0800, 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 request is handled as below:
> > > > > 
> > > > > Server sends CB_GETATTR to client to get the latest change info and file
> > > > > size. If these values are the same as the server's cached values then
> > > > > the GETATTR proceeds as normal.
> > > > > 
> > > > > If either the change info or file size is different from the server's
> > > > > cached values, or the file was already marked as modified, then:
> > > > > 
> > > > >       . update time_modify and time_metadata into file's metadata
> > > > >         with current time
> > > > > 
> > > > >       . encode GETATTR as normal except the file size is encoded with
> > > > >         the value returned from CB_GETATTR
> > > > > 
> > > > >       . mark the file as modified
> > > > > 
> > > > > The CB_GETATTR is given 30ms to complte. If the CB_GETATTR fails for
> > > > > any reasons, the delegation is recalled and NFS4ERR_DELAY is returned
> > > > > for the GETATTR from the second client.
> > > > > 
> > > > > Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> > > > > ---
> > > > >    fs/nfsd/nfs4state.c | 119 ++++++++++++++++++++++++++++++++++++++++----
> > > > >    fs/nfsd/nfs4xdr.c   |  10 +++-
> > > > >    fs/nfsd/nfsd.h      |   1 +
> > > > >    fs/nfsd/state.h     |  10 +++-
> > > > >    4 files changed, 127 insertions(+), 13 deletions(-)
> > > > > 
> > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > > > index d9260e77ef2d..87987515e03d 100644
> > > > > --- a/fs/nfsd/nfs4state.c
> > > > > +++ b/fs/nfsd/nfs4state.c
> > > > > @@ -127,6 +127,7 @@ static void free_session(struct nfsd4_session *);
> > > > >    static const struct nfsd4_callback_ops nfsd4_cb_recall_ops;
> > > > >    static const struct nfsd4_callback_ops nfsd4_cb_notify_lock_ops;
> > > > > +static const struct nfsd4_callback_ops nfsd4_cb_getattr_ops;
> > > > >    static struct workqueue_struct *laundry_wq;
> > > > > @@ -1189,6 +1190,10 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
> > > > >    	dp->dl_recalled = false;
> > > > >    	nfsd4_init_cb(&dp->dl_recall, dp->dl_stid.sc_client,
> > > > >    		      &nfsd4_cb_recall_ops, NFSPROC4_CLNT_CB_RECALL);
> > > > > +	nfsd4_init_cb(&dp->dl_cb_fattr.ncf_getattr, dp->dl_stid.sc_client,
> > > > > +			&nfsd4_cb_getattr_ops, NFSPROC4_CLNT_CB_GETATTR);
> > > > > +	dp->dl_cb_fattr.ncf_file_modified = false;
> > > > > +	dp->dl_cb_fattr.ncf_cb_bmap[0] = FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE;
> > > > >    	get_nfs4_file(fp);
> > > > >    	dp->dl_stid.sc_file = fp;
> > > > >    	return dp;
> > > > > @@ -3044,11 +3049,59 @@ nfsd4_cb_recall_any_release(struct nfsd4_callback *cb)
> > > > >    	spin_unlock(&nn->client_lock);
> > > > >    }
> > > > > +static int
> > > > > +nfsd4_cb_getattr_done(struct nfsd4_callback *cb, struct rpc_task *task)
> > > > > +{
> > > > > +	struct nfs4_cb_fattr *ncf =
> > > > > +			container_of(cb, struct nfs4_cb_fattr, ncf_getattr);
> > > > > +
> > > > > +	ncf->ncf_cb_status = task->tk_status;
> > > > > +	switch (task->tk_status) {
> > > > > +	case -NFS4ERR_DELAY:
> > > > > +		rpc_delay(task, 2 * HZ);
> > > > > +		return 0;
> > > > > +	default:
> > > > > +		return 1;
> > > > > +	}
> > > > > +}
> > > > > +
> > > > > +static void
> > > > > +nfsd4_cb_getattr_release(struct nfsd4_callback *cb)
> > > > > +{
> > > > > +	struct nfs4_cb_fattr *ncf =
> > > > > +			container_of(cb, struct nfs4_cb_fattr, ncf_getattr);
> > > > > +	struct nfs4_delegation *dp =
> > > > > +			container_of(ncf, struct nfs4_delegation, dl_cb_fattr);
> > > > > +
> > > > > +	nfs4_put_stid(&dp->dl_stid);
> > > > > +	clear_bit(CB_GETATTR_BUSY, &ncf->ncf_cb_flags);
> > > > > +	wake_up_bit(&ncf->ncf_cb_flags, CB_GETATTR_BUSY);
> > > > > +}
> > > > > +
> > > > What happens if the client responds after the GETATTR timer elapses?
> > > > Can nfsd4_cb_getattr_release over-write memory that is now being
> > > > used for something else?
> > > The refcount added in nfs4_cb_getattr keeps the delegation state valid
> > > until it's released here by nfs4_put_stid.
> > If the client never replies, does that pin the stateid?
> 
> Won't RPC timeout on waiting for reply?
> This is the same behavior as when recalling a delegation,
> nfsd_break_deleg_cb and nfsd4_cb_recall_release.

Fair enough. Looking forward to v2.


-- 
Chuck Lever