Received-SPF: pass (google.com: domain of linux-nfs+bounces-1924-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Wed, 14 Feb 2024 13:14:42 -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: <Zc0DEuJ+jYevKc3Y@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>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <7994e006-b2e7-4c9b-9644-52225e1d9594@oracle.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?ACY8S2zP8LcWzAlaz6LSr2cOTA50kgt8hMAbarfpzHI9MSkM8MSnIsrKZzHQ?=
 =?us-ascii?Q?j1kKBdf0NmjeJWx4ymOPIgOpOd2c/vV6/3ZOsxomT+kbtJ6AzKSV3IVTzLnT?=
 =?us-ascii?Q?pLvu+71PrgjGpTKzud2nc5PYIFT7JkdVp5dWP1XKx46S2MU+ehZQBwHnYPZO?=
 =?us-ascii?Q?mK4RxpPWugclMR59aBrc58lmK074pcYsnXcA2+RYpA3UdfVATPlJJarz63oc?=
 =?us-ascii?Q?Ae30zqNxeYcc1+dgiKBvoUYga95TURy6em1VfeZZWxEEWwlM6458gLgYhQzP?=
 =?us-ascii?Q?s/CczPBSCbIMbbLHGJrRwHH6ZAPB6M09f5IGryh2mYOXFVoPxNHMQgUQM+gk?=
 =?us-ascii?Q?XgDY4O3lNQpg3koxphaY9PddbnJSHQLepbWWJxYtAKJ+aOuk13BdRHfJeGq8?=
 =?us-ascii?Q?xxdYZqGb06fjh/LwPEnDdvSERofhW1LVEl8xih+xEj/h1c4BrIKoVWkPM83V?=
 =?us-ascii?Q?FI6R8w3jz2J6cGzSL5apF9q6ChDzvLkG19EVwVjOyKfiIlb9xFOmADey9big?=
 =?us-ascii?Q?bD501/PkxpMQB+jmutvT2osBORGdR4rr1bd6C/x6jGLJGfxjau39ni5BdOA0?=
 =?us-ascii?Q?T75qNS9bSomg9TArhv1MYh0DXGdFW1gsXlkFLwkuQ+LSPxo/Cdc89ldBw7ZM?=
 =?us-ascii?Q?tzDsR4pB1cOGGnnaPwQ5fuEqL6s7Yg5w5q++Js2liB5U6Dz6pvnZonSFWIv/?=
 =?us-ascii?Q?tUx81wItyqtuye00xqy/JO074DBH6ZMjk8NUU1MMqQiQHufM9yP4yt5qPPAH?=
 =?us-ascii?Q?e5xjZ4vPlnQAUY1sgFnDIBwuhfBDye76rX8W3bE3EDP4zt7htO3ZqwKP/CZG?=
 =?us-ascii?Q?Nzqir8FjMy0TJ4VoHl94x0T41jKm7h4ML2HSqawjnG2b98JwDr8vBN3xMsmT?=
 =?us-ascii?Q?3ZIF5QL8alaoB+nXVE7Ot5OLyvdlxKUE403jaIH/nuXVosk4ZnT6LJXb7LRb?=
 =?us-ascii?Q?fZN7mVIRDwOVIbrBO04u/uhqCagZFgNPylY0JrIbOcFpGkeHqpFnP47nOcSh?=
 =?us-ascii?Q?1tFtqOuPzhHraogvjYpDGhnzCOPZmyR+6mdW8HlNCb4Gl+rIvrbDVt/YjjSZ?=
 =?us-ascii?Q?2e/yWtIA/q5oC+XiDztZ5C8Nkosoqvxs+dhLNWwuFDQg5OLFDMLcfkLHEHKI?=
 =?us-ascii?Q?mVGRhzaPv0RJVIT6Do6bxuLaGepYdGOKmnqUh1jv1x3oFAzZUHnDRGGBIK/n?=
 =?us-ascii?Q?IQgi6dqZOm4XVVYxBI76O+3jgAGqBSkFVZ0/LDyu9y1PkQFzDxX70ABHm4El?=
 =?us-ascii?Q?V2koXlQHtKhXVTa0FlvbTTDSbxVLvQ9ynnTA2cF1uxTMrbWwXAnmZnD9+qeC?=
 =?us-ascii?Q?bPCZPIiP8cHHAQlZNmOYg6gj9KHEU/uuraDawQA29zPtV8LPnC5AA/SsYLnH?=
 =?us-ascii?Q?djZlXiGxahNkPohoKObXPC3KAueOPVL+JFaQQC06LFvp1y2tufzYtBrSuzqW?=
 =?us-ascii?Q?D+UbU9VlypBATG3rD09c9eoj5Fwbo3gYZAZ7UfYDHlA0EXK+fZ0GFY+jC5p2?=
 =?us-ascii?Q?zfw3snqlczLuZnkA0cO46pNNomnrJQcPhQ5r2muhpGbWzQlzWUHMSR6mSh00?=
 =?us-ascii?Q?0pfnsGzUwNPeCCNG92iao4/s1lolySEEv1wC2kCKedmg//mPTuTn5gpvCbBc?=
 =?us-ascii?Q?NA=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	K7yKYq83DSKJhWj5ojOcbJ2rkqnxpjOY21MWzDkMrELTADYUb7EXZC2eBQon8vOzhSDumBYrBO+buR+CyYGjvPxTOhEjRZnptK82sh9pFVnA5P75uuM4SGCiUFvcnt5uOw8GdGIGJqBJkYkU4W+t6egO1cox9rYAdQ9/MWfwKt4G9sazrne0qEoVOHLAQObh++KWLhvxmp6byRvC0LhSe7Rcw5OOEaoo8WEj19+jSFFvM4z3U/fTV0shSdgkrPWPkO7ey8/pDvwMzWMUmmahzCqBfLgjfBY8X5/L75nfYaQ1ZIzk9yqTQ51j4GZAUJ94EFGscGptvuDSnYx+a+AwiqoFdt2K1uu+sIplCIHLJHPan9knlsLIiR6XHWClsoZT4nvdQLknSs1kWLfD9qTNrlGV7bXIQ6+0BIy3jSBrOePZW3ks0k7VlUnZI+y/Tu6MnwYdPhbxstwQOVugoAwjVrNE+zPrw0XMOVUBTjp+yJvcBTrBMdGYVSUVHFVHRIvq59jwupKI84gUSKi7ug5Q5IX7HG8aywESxNSvYnYMXlMIUZT8DD7EeCdcNQIqfXkkn55ogI11MfAb71TG12Zftzd+SKz/qg+t4Z7zjAKCNL8=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 86cc9c71-5495-4143-6461-08dc2d88d26c
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Feb 2024 18:14:45.0547
 (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: SuD7JZuvncBmQbkeK3yp/0oahUA1tAygU1o2HWCQa2xb1DLz4nk7fXD4+/sneZtIzUNS45+ppEtOmb1y9+pRng==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR10MB7082
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-14_10,2024-02-14_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 bulkscore=0
 mlxlogscore=999 malwarescore=0 mlxscore=0 spamscore=0 suspectscore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2402140143
X-Proofpoint-GUID: ljFs_OEwhQZ65d9o8hsuck2bHQ2l3Vm_
X-Proofpoint-ORIG-GUID: ljFs_OEwhQZ65d9o8hsuck2bHQ2l3Vm_

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?


-- 
Chuck Lever