Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Date:   Fri, 29 Sep 2023 13:23:06 -0400
From:   Chuck Lever <chuck.lever@oracle.com>
To:     Jeff Layton <jlayton@poochiereds.net>
Cc:     Chuck Lever <cel@kernel.org>, linux-nfs@vger.kernel.org
Subject: Re: [PATCH v1 3/7] NFSD: Add nfsd4_encode_open_read_delegation4()
Message-ID: <ZRcH+lQ5RRxsT5x6@tissot.1015granger.net>
References: <169599581942.5622.15965175797823365235.stgit@manet.1015granger.net>
 <169599594587.5622.94747681619250190.stgit@manet.1015granger.net>
 <4c6c46419d34cbe0a408aea4705383edecffa00d.camel@poochiereds.net>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4c6c46419d34cbe0a408aea4705383edecffa00d.camel@poochiereds.net>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?8xGfFgU8+fDhJrEd2UFXwNhL8W1Y1wLJmC0PIoxZ0ufwjP05+sKdmbRTd3qw?=
 =?us-ascii?Q?F7Y4boWzIweGXx8P/BvHst8jJqZK+26Lx9Ao7tnVzNfKRctI3UobGXPdflZR?=
 =?us-ascii?Q?gS2GBc5MKCiaY25P+jCzXpsSco+KWsYHawbLeiX8bcxTar32JQ8+P5urHnu9?=
 =?us-ascii?Q?FAl25SvKblb9ZgGFwSTNri/BoDsNLaSSDdBAMycNwwyDsr15KfaQZbJImcbd?=
 =?us-ascii?Q?ap2xgEhNwfpm+0Z0gauK9dZSRJQzlXVeVO4DWOAVyg/JhzgP8xDzWcLyAdgm?=
 =?us-ascii?Q?txih9J+XvfKAAv5l/HrkHrMVo+Ct7gLq00+aEgP7P+VqqFV8/zGAvf1qe/lX?=
 =?us-ascii?Q?ridJsai+0gAKA4oIFqasGKGWW4MM9lhnRsD3BVDds1OnQcZoLqJ1unGC7ZtZ?=
 =?us-ascii?Q?o6ikA/G97ac8rkw7XPNMFDZyKti7qdMcWHsOlJRyth2BaWJNgx4xwuollYWF?=
 =?us-ascii?Q?oFb6bom2kjINC+BX0oS/BufYuWtLSxVquyWYLpsKm1gsRd210KZfsHgCd8C3?=
 =?us-ascii?Q?QMHaiABfSCt/gL4uUQCz0JvddEGW1FNCm4UNsbYLPfN7aNbqnV4y9blZlDcL?=
 =?us-ascii?Q?qbpdlgX/ZN1fQZmi1rwCCieMEJPFZF4RKBuEUmYCTDZ503n4MZr+qcswmBRv?=
 =?us-ascii?Q?OCSoZ0y8mCIDFVzaapR4Ym3QsXwwqR5aufxmOI5yI+XebYp8+sbnjykhuyi7?=
 =?us-ascii?Q?kAbkuBXzQsBDyqcvLFRJvMo24AUPhkTXHJYmiMb33Xnsdii5Jo4QdPAvCDoi?=
 =?us-ascii?Q?ZJ8CFl573UjeXpla/FTrcnnidoh8WsDSfAIqOnuxnmPwY2F4t6Sz6PsqGOcj?=
 =?us-ascii?Q?ZVjiXbfkYJhX/2lGA/YS+SzZj1Xiv+Im3eWKVBhoskfQsm3r4GUOhrMGKFHV?=
 =?us-ascii?Q?68cDENpVKgVRVIU/ucfScbuJIcVV6M5PIXHCTdPp25q6BvtLIYD7uE1Flk2D?=
 =?us-ascii?Q?trfysdXVRZ29Ycd+pS/QHI482+QwsLmmEVkcrYCoLpaGU1GibT65BTF1yUwz?=
 =?us-ascii?Q?kN+LIDjYov++WRvJzpDpq5Pw3KkRbG+InMWuzYe+tYzWXUNf3/b1hDCqI107?=
 =?us-ascii?Q?jaexe/pdnegq4+a/AJxqkPCKhq3ygHNlaDHQAxEtNT5WgA8hHt12NM+lxUST?=
 =?us-ascii?Q?ywdVviFCE0uk/35nlWkz7iHnVvk1pR9tc8GhpVsRk9VkSC5uvxmBXGNkoSi/?=
 =?us-ascii?Q?tKkeMjfcsR7b+T4Jwqvc6CliQodvpO+IN8cRz2rxpk+CMDm0SIuSkmagA244?=
 =?us-ascii?Q?KRbkzsvljcuoRjL1A9SDKb9TxSKnTjOqCBK21nsnjJ6qIyMNhy+pCG9mTCFQ?=
 =?us-ascii?Q?RH3EkC9qwzU+UzQxhAs0m8RrBhiesgHeLtPBmLmYxqnQ+T1dYGuMEXKlnDLZ?=
 =?us-ascii?Q?DAYfj7rVOBZVyeXjsW19Wbg5ax8bvBw6dznJeiwqbq+SJR4ic+pjTTH5DS1x?=
 =?us-ascii?Q?rW2DGFDoEh3tsSroCC+RboNgVwnnekoESjcz1G9D2JZv8VkGM7UcgV2XuM9M?=
 =?us-ascii?Q?X02T1W8U8tHgD6u+C2Bo85L3WZ8wgdpkcCN76vQaHB8cNV0GEOeFbru/jHEs?=
 =?us-ascii?Q?cgF8A8kcuqyFc+iBJCPOB9mBBzotfEfYPheE5SAcZcPg3t/XpHWnQE5KNh4r?=
 =?us-ascii?Q?tQ=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: cLGymCfOxMsyoxQHJg5xXn/376b0z0SzHKL1z/uYrguYGSYXMV4FdWPLZ5os8L2jGTwfqQDBTphlYexh79CnRhci3d1hWhQaFHKDeJpQTHsW2vSZrPnqd/1WdV3DVLnMEUVIUB7brGWbYTAU98dyaMZxsHomjA0V9iKXDLEz3zBlsAcD7GLy7FqYz0juAzL5b6hlBB+SegvG49o+x3OJxGLv595hphsqmmS0t0fbKPBKpBjj03Y65zQUg/comJVvNscVGboPTQABXJlnRX0/mHES625n3GAqwN3I0CPHIK8nzDxX1iKy069GcGLtY33vvFq2k1dzah8HRe0um+OJJQZ8hvZUqZjba8Dtuvr9dkaorq1QZJc2GH4sxDTPGxcWY3mDC46IAuiNejI1sVHGEkjIoUjlfddBLMTB6mu3l9CUVnSJcd/xjnjayFUzYz7O6r0WCoZjtx7RPgpzDxnRTi0lV1F3ZLLeZxP3naCRs8/Sb6L63YjANQppsmKlj1+g3ewDlr/dGAfVuyPfSYopRvphURRhhSAiFyKN+0jEccUn6DXTAO2EewFEHEYPOjFaOvPEQDhX5ySfs99APQcpHYU1M2JGxeNZCv5qba15b1r5cAtoB16AXBOHGwxV01a4m8wo5fACy4TUnSYCF4rD7Vq1lmWx9DJd6D0Q5wVuJS0nKgK7/BqfWcUp8kbRgBlQtFkDmPj6aPLV4QNzDWz4l0+JVwod6oJ9NYnl2eWK73y7k7Gn7lu1/0o8/nGG0zSmzS7Fl3w08K72vtLURnzuBU4V60y9AtPsHVS8jdC+izUhO2DEi5Ll6i8j0j7f3R15gH6kac7BMXPJ77Fjosv4dg==
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d198a6e1-c2fd-43bc-be08-08dbc110bfff
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Sep 2023 17:23:09.1352
 (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: hUoUOhkGRPGhtdxnzc0UBf7mu95IpU3unA/S0BeA33YxXM8wlAwcOYZ0DUxpeHf9SXEaqO8CH+0h3zbaqMWhMw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CYYPR10MB7627
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.267,Aquarius:18.0.980,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-09-29_16,2023-09-28_03,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 bulkscore=0 mlxscore=0
 adultscore=0 mlxlogscore=999 suspectscore=0 malwarescore=0 spamscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2309180000
 definitions=main-2309290149
X-Proofpoint-GUID: hiI1p9NO4KYowVg4EOSL4oFbhZdalW4j
X-Proofpoint-ORIG-GUID: hiI1p9NO4KYowVg4EOSL4oFbhZdalW4j
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Fri, 29 Sep 2023 10:23:24 -0700 (PDT)

On Fri, Sep 29, 2023 at 12:21:28PM -0400, Jeff Layton wrote:
> On Fri, 2023-09-29 at 09:59 -0400, Chuck Lever wrote:
> > From: Chuck Lever <chuck.lever@oracle.com>
> > 
> > Refactor nfsd4_encode_open() so the open_read_delegation4 type is
> > encoded in a separate function. This makes it more straightforward
> > to later add support for returning an nfsace4 in OPEN responses that
> > offer a delegation.
> > 
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > ---
> >  fs/nfsd/nfs4state.c |    6 +++--
> >  fs/nfsd/nfs4xdr.c   |   61 ++++++++++++++++++++++++++++++++++++++-------------
> >  fs/nfsd/xdr4.h      |    2 +-
> >  3 files changed, 49 insertions(+), 20 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 22e95b9ae82f..b1118050ff52 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -5688,11 +5688,11 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> >  	struct path path;
> >  
> >  	cb_up = nfsd4_cb_channel_good(oo->oo_owner.so_client);
> > -	open->op_recall = 0;
> > +	open->op_recall = false;
> >  	switch (open->op_claim_type) {
> >  		case NFS4_OPEN_CLAIM_PREVIOUS:
> >  			if (!cb_up)
> > -				open->op_recall = 1;
> > +				open->op_recall = true;
> >  			break;
> >  		case NFS4_OPEN_CLAIM_NULL:
> >  			parent = currentfh;
> > @@ -5746,7 +5746,7 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> >  	if (open->op_claim_type == NFS4_OPEN_CLAIM_PREVIOUS &&
> >  	    open->op_delegate_type != NFS4_OPEN_DELEGATE_NONE) {
> >  		dprintk("NFSD: WARNING: refusing delegation reclaim\n");
> > -		open->op_recall = 1;
> > +		open->op_recall = true;
> >  	}
> >  
> >  	/* 4.1 client asking for a delegation? */
> > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > index ee8a7989f54f..f411fcc435f6 100644
> > --- a/fs/nfsd/nfs4xdr.c
> > +++ b/fs/nfsd/nfs4xdr.c
> > @@ -4074,6 +4074,49 @@ nfsd4_encode_link(struct nfsd4_compoundres *resp, __be32 nfserr,
> >  	return nfsd4_encode_change_info4(xdr, &link->li_cinfo);
> >  }
> >  
> > +/*
> > + * This implementation does not yet support returning an ACE in an
> > + * OPEN that offers a delegation.
> > + */
> > +static __be32
> > +nfsd4_encode_open_nfsace4(struct xdr_stream *xdr)
> > +{
> > +	__be32 status;
> > +
> > +	/* type */
> > +	status = nfsd4_encode_acetype4(xdr, NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE);
> > +	if (status != nfs_ok)
> > +		return nfserr_resource;
> > +	/* flag */
> > +	status = nfsd4_encode_aceflag4(xdr, 0);
> > +	if (status != nfs_ok)
> > +		return nfserr_resource;
> > +	/* access mask */
> > +	status = nfsd4_encode_acemask4(xdr, 0);
> > +	if (status != nfs_ok)
> > +		return nfserr_resource;
> > +	/* who - empty for now */
> > +	if (xdr_stream_encode_u32(xdr, 0) != XDR_UNIT)
> > +		return nfserr_resource;
> > +	return nfs_ok;
> > +}
> > +
> > +static __be32
> > +nfsd4_encode_open_read_delegation4(struct xdr_stream *xdr, struct nfsd4_open *open)
> > +{
> > +	__be32 status;
> > +
> > +	/* stateid */
> > +	status = nfsd4_encode_stateid4(xdr, &open->op_delegate_stateid);
> > +	if (status != nfs_ok)
> > +		return status;
> > +	/* recall */
> > +	status = nfsd4_encode_bool(xdr, open->op_recall);
> > +	if (status != nfs_ok)
> > +		return status;
> > +	/* permissions */
> > +	return nfsd4_encode_open_nfsace4(xdr);
> > +}
> >  
> >  static __be32
> >  nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr,
> > @@ -4106,22 +4149,8 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr,
> >  	case NFS4_OPEN_DELEGATE_NONE:
> >  		break;
> >  	case NFS4_OPEN_DELEGATE_READ:
> > -		nfserr = nfsd4_encode_stateid4(xdr, &open->op_delegate_stateid);
> > -		if (nfserr)
> > -			return nfserr;
> > -		p = xdr_reserve_space(xdr, 20);
> > -		if (!p)
> > -			return nfserr_resource;
> > -		*p++ = cpu_to_be32(open->op_recall);
> > -
> > -		/*
> > -		 * TODO: ACE's in delegations
> > -		 */
> > -		*p++ = cpu_to_be32(NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE);
> > -		*p++ = cpu_to_be32(0);
> > -		*p++ = cpu_to_be32(0);
> > -		*p++ = cpu_to_be32(0);   /* XXX: is NULL principal ok? */
> > -		break;
> > +		/* read */
> > +		return nfsd4_encode_open_read_delegation4(xdr, open);
> >  	case NFS4_OPEN_DELEGATE_WRITE:
> >  		nfserr = nfsd4_encode_stateid4(xdr, &open->op_delegate_stateid);
> >  		if (nfserr)
> > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> > index aba07d5378fc..c142a9b5ab98 100644
> > --- a/fs/nfsd/xdr4.h
> > +++ b/fs/nfsd/xdr4.h
> > @@ -389,7 +389,7 @@ struct nfsd4_open {
> >  	u32		op_deleg_want;      /* request */
> >  	stateid_t	op_stateid;         /* response */
> >  	__be32		op_xdr_error;       /* see nfsd4_open_omfg() */
> > -	u32		op_recall;          /* recall */
> > +	bool		op_recall;          /* response */
> 
> nit: this would probably pack better if you moved op_recall down beside
> op_truncate.

Fixed. All 15 pushed to nfsd-next with your Reviewed-by. Thanks!


> >  	struct nfsd4_change_info  op_cinfo; /* response */
> >  	u32		op_rflags;          /* response */
> >  	bool		op_truncate;        /* used during processing */
> > 
> > 
> 
> -- 
> Jeff Layton <jlayton@poochiereds.net>

-- 
Chuck Lever