All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] nfsd41: fix fore channel attribute initialization
@ 2009-09-11 22:52 andros
  2009-09-11 22:52 ` [PATCH 1/2] nfsd41: fix NFSD_MIN_HDR_SEQ_SZ andros
  2009-09-14 18:54 ` [PATCH 0/2] nfsd41: fix fore channel attribute initialization J. Bruce Fields
  0 siblings, 2 replies; 11+ messages in thread
From: andros @ 2009-09-11 22:52 UTC (permalink / raw)
  To: bfields; +Cc: pnfs, linux-nfs

Fix forechannel attribute initialization

These two patches fix ca_maxresponsesize_cached, ca_maxresponsesize, and
ca_maxrequestsize calculations for the forechannel.

Tested:

Kerberos NFSv4.1 mount:
Passes Connectathon tests. Passes the expected pynfs tests when tests are run
individually.
When the pynfs tests are run all at once, there are too many sessions created,
and the server code (init_forechannel_attrs) works as planned and fails the
CREATE SESSION with NFS4ERR_SERVERFAULT.

The pynfs 4.1 tests need to be fixed to clean up after themselves! 

-->Andy

0001-nfsd41-fix-NFSD_MIN_HDR_SEQ_SZ.patch
0002-nfsd41-add-RPC-header-size-to-fore-channel-negotiat.patch


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/2] nfsd41: fix NFSD_MIN_HDR_SEQ_SZ
  2009-09-11 22:52 [PATCH 0/2] nfsd41: fix fore channel attribute initialization andros
@ 2009-09-11 22:52 ` andros
  2009-09-11 22:52   ` [PATCH 2/2] nfsd41: add RPC header size to fore channel negotiation andros
  2009-09-14 18:51   ` [PATCH 1/2] nfsd41: fix NFSD_MIN_HDR_SEQ_SZ J. Bruce Fields
  2009-09-14 18:54 ` [PATCH 0/2] nfsd41: fix fore channel attribute initialization J. Bruce Fields
  1 sibling, 2 replies; 11+ messages in thread
From: andros @ 2009-09-11 22:52 UTC (permalink / raw)
  To: bfields; +Cc: pnfs, linux-nfs, Andy Adamson

From: Andy Adamson <andros@netapp.com>

The reply RPC header is 12 bytes, the NULL verifier is 8 bytes giving 20
not 24 bytes in the NFSD_MIN_HDR_SEQ_SZ calculation,

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfsd/nfs4state.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 46e9ac5..5ecb42c 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -420,12 +420,12 @@ gen_sessionid(struct nfsd4_session *ses)
  * each time.  Therefore we can advertise a ca_maxresponssize_cached
  * value that is the number of bytes in our cache plus a few additional
  * bytes.  In order to stay on the safe side, and not promise more than
- * we can cache, those additional bytes must be the minimum possible: 24
+ * we can cache, those additional bytes must be the minimum possible: 20
  * bytes of rpc header (xid through accept state, with AUTH_NULL
  * verifier), 12 for the compound header (with zero-length tag), and 44
  * for the SEQUENCE op response:
  */
-#define NFSD_MIN_HDR_SEQ_SZ  (24 + 12 + 44)
+#define NFSD_MIN_HDR_SEQ_SZ  (20 + 12 + 44)
 
 /*
  * Give the client the number of ca_maxresponsesize_cached slots it
-- 
1.6.2.5


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] nfsd41: add RPC header size to fore channel negotiation
  2009-09-11 22:52 ` [PATCH 1/2] nfsd41: fix NFSD_MIN_HDR_SEQ_SZ andros
@ 2009-09-11 22:52   ` andros
  2009-09-14 19:03     ` J. Bruce Fields
  2009-09-14 18:51   ` [PATCH 1/2] nfsd41: fix NFSD_MIN_HDR_SEQ_SZ J. Bruce Fields
  1 sibling, 1 reply; 11+ messages in thread
From: andros @ 2009-09-11 22:52 UTC (permalink / raw)
  To: bfields; +Cc: pnfs, linux-nfs, Andy Adamson

From: Andy Adamson <andros@netapp.com>

Both the max request and the max response size include the RPC header with
credential (request only)  and verifier as well as the payload.

The RPCSEC_GSS credential and verifier are the largest. Kerberos is the only
supported GSS security mechansim, so the Kerberos GSS credential and verifier
sizes are used.

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfsd/nfs4state.c |   29 +++++++++++++++++++++++++----
 1 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 5ecb42c..e4d8f94 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -428,6 +428,27 @@ gen_sessionid(struct nfsd4_session *ses)
 #define NFSD_MIN_HDR_SEQ_SZ  (20 + 12 + 44)
 
 /*
+ * The protocol defines ca_maxrequestsize as the XDR encoded size of the
+ * request including the RPC headers (including security flavor credentials
+ * and verifiers) but excludes any RPC transport framing headers.
+ * So we need to advertize a ca_maxrequestsize value that is the number of
+ * bytes of the maximum payload we support, plus some additional bytes to cover
+ * the maximum RPC header size. The RPCSEC_GSS security flavor has the
+ * largest credential and verifier, so we add 24 bytes for the RPC call header
+ * (xid through proceedure), 32 bytes of GSS credential, and 48 bytes of
+ * Kerberos GSS verifier.
+ */
+#define NFSD_MAX_CALL_HDR_SZ (24 + 32 + 48)
+
+/* The protocol defines ca_maxresponsesize as also including the RPC headers
+ * just as in ca_maxrequestsize. Once again, we use the maximum supported
+ * payload plus the largest RPC reply header which uses the RPCSEC_GSS
+ * security flavor. We add 12 bytes of RPC reply header (xid through 
+ * reply state) and 48 bytes of GSS Kerberos verifier.
+ */
+#define NFSD_MAX_REPLY_HDR_SZ (12 + 48)
+
+/*
  * Give the client the number of ca_maxresponsesize_cached slots it
  * requests, of size bounded by NFSD_SLOT_CACHE_SIZE,
  * NFSD_MAX_MEM_PER_SESSION, and nfsd_drc_max_mem. Do not allow more
@@ -488,12 +509,12 @@ static int init_forechannel_attrs(struct svc_rqst *rqstp,
 	/* headerpadsz set to zero in encode routine */
 
 	/* Use the client's max request and max response size if possible */
-	if (fchan->maxreq_sz > maxcount)
-		fchan->maxreq_sz = maxcount;
+	if (fchan->maxreq_sz > maxcount + NFSD_MAX_CALL_HDR_SZ)
+		fchan->maxreq_sz = maxcount + NFSD_MAX_CALL_HDR_SZ;
 	session_fchan->maxreq_sz = fchan->maxreq_sz;
 
-	if (fchan->maxresp_sz > maxcount)
-		fchan->maxresp_sz = maxcount;
+	if (fchan->maxresp_sz > maxcount + NFSD_MAX_REPLY_HDR_SZ)
+		fchan->maxresp_sz = maxcount + NFSD_MAX_REPLY_HDR_SZ;
 	session_fchan->maxresp_sz = fchan->maxresp_sz;
 
 	/* Use the client's maxops if possible */
-- 
1.6.2.5


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] nfsd41: fix NFSD_MIN_HDR_SEQ_SZ
  2009-09-11 22:52 ` [PATCH 1/2] nfsd41: fix NFSD_MIN_HDR_SEQ_SZ andros
  2009-09-11 22:52   ` [PATCH 2/2] nfsd41: add RPC header size to fore channel negotiation andros
@ 2009-09-14 18:51   ` J. Bruce Fields
  2009-09-14 20:11     ` [pnfs] " William A. (Andy) Adamson
  1 sibling, 1 reply; 11+ messages in thread
From: J. Bruce Fields @ 2009-09-14 18:51 UTC (permalink / raw)
  To: andros; +Cc: pnfs, linux-nfs

On Fri, Sep 11, 2009 at 06:52:54PM -0400, andros@netapp.com wrote:
> From: Andy Adamson <andros@netapp.com>
> 
> The reply RPC header is 12 bytes, the NULL verifier is 8 bytes giving 20
> not 24 bytes in the NFSD_MIN_HDR_SEQ_SZ calculation,

No, 24 is right.  Maybe you forgot the accept stat at the end?:

	xid
	msg type
	reply stat
	verf flavor
	verf length
	accept stat

--b.

> 
> Signed-off-by: Andy Adamson <andros@netapp.com>
> ---
>  fs/nfsd/nfs4state.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 46e9ac5..5ecb42c 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -420,12 +420,12 @@ gen_sessionid(struct nfsd4_session *ses)
>   * each time.  Therefore we can advertise a ca_maxresponssize_cached
>   * value that is the number of bytes in our cache plus a few additional
>   * bytes.  In order to stay on the safe side, and not promise more than
> - * we can cache, those additional bytes must be the minimum possible: 24
> + * we can cache, those additional bytes must be the minimum possible: 20
>   * bytes of rpc header (xid through accept state, with AUTH_NULL
>   * verifier), 12 for the compound header (with zero-length tag), and 44
>   * for the SEQUENCE op response:
>   */
> -#define NFSD_MIN_HDR_SEQ_SZ  (24 + 12 + 44)
> +#define NFSD_MIN_HDR_SEQ_SZ  (20 + 12 + 44)
>  
>  /*
>   * Give the client the number of ca_maxresponsesize_cached slots it
> -- 
> 1.6.2.5
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] nfsd41: fix fore channel attribute initialization
  2009-09-11 22:52 [PATCH 0/2] nfsd41: fix fore channel attribute initialization andros
  2009-09-11 22:52 ` [PATCH 1/2] nfsd41: fix NFSD_MIN_HDR_SEQ_SZ andros
@ 2009-09-14 18:54 ` J. Bruce Fields
  1 sibling, 0 replies; 11+ messages in thread
From: J. Bruce Fields @ 2009-09-14 18:54 UTC (permalink / raw)
  To: andros; +Cc: pnfs, linux-nfs

On Fri, Sep 11, 2009 at 06:52:53PM -0400, andros@netapp.com wrote:
> Fix forechannel attribute initialization
> 
> These two patches fix ca_maxresponsesize_cached, ca_maxresponsesize, and
> ca_maxrequestsize calculations for the forechannel.
> 
> Tested:

Actually the think I was interested to test was just to make sure that
the client does writes of the size it should.  (It should be able to do
1MB reads and writes, but with the previous code, I believe it would
have been a bug for the client to do so, as we weren't advertising
maxresponse/requestsizes large enough.)

--b.

> 
> Kerberos NFSv4.1 mount:
> Passes Connectathon tests. Passes the expected pynfs tests when tests are run
> individually.
> When the pynfs tests are run all at once, there are too many sessions created,
> and the server code (init_forechannel_attrs) works as planned and fails the
> CREATE SESSION with NFS4ERR_SERVERFAULT.
> 
> The pynfs 4.1 tests need to be fixed to clean up after themselves! 
> 
> -->Andy
> 
> 0001-nfsd41-fix-NFSD_MIN_HDR_SEQ_SZ.patch
> 0002-nfsd41-add-RPC-header-size-to-fore-channel-negotiat.patch
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] nfsd41: add RPC header size to fore channel negotiation
  2009-09-11 22:52   ` [PATCH 2/2] nfsd41: add RPC header size to fore channel negotiation andros
@ 2009-09-14 19:03     ` J. Bruce Fields
  2009-09-18 16:47       ` [pnfs] " William A. (Andy) Adamson
  0 siblings, 1 reply; 11+ messages in thread
From: J. Bruce Fields @ 2009-09-14 19:03 UTC (permalink / raw)
  To: andros; +Cc: pnfs, linux-nfs

On Fri, Sep 11, 2009 at 06:52:55PM -0400, andros@netapp.com wrote:
> From: Andy Adamson <andros@netapp.com>
> 
> Both the max request and the max response size include the RPC header with
> credential (request only)  and verifier as well as the payload.
> 
> The RPCSEC_GSS credential and verifier are the largest. Kerberos is the only
> supported GSS security mechansim, so the Kerberos GSS credential and verifier
> sizes are used.

Rather than trying to estimate this is might be simplest just to use
what the server's using to allocate memory: RPCSVC_MAXPAGES.  No, that
also takes into account space for the reply.  You could do

	PAGE_SIZE * (1 + (RPCSVC_MAXPAYLOAD+PAGE_SIZE-1)/PAGE_SIZE)

Actually, by design the server's real limit is actually on the sum of
the request and the reply sizes.

What happens if we get a request such that both the request and reply
are under our advertised limits, but the sum is too much?  Can we just
declare that no client will be that weird and that we shouldn't have to
worry about it?

--b.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [pnfs] [PATCH 1/2] nfsd41: fix NFSD_MIN_HDR_SEQ_SZ
  2009-09-14 18:51   ` [PATCH 1/2] nfsd41: fix NFSD_MIN_HDR_SEQ_SZ J. Bruce Fields
@ 2009-09-14 20:11     ` William A. (Andy) Adamson
  0 siblings, 0 replies; 11+ messages in thread
From: William A. (Andy) Adamson @ 2009-09-14 20:11 UTC (permalink / raw)
  To: J. Bruce Fields; +Cc: linux-nfs, pnfs

On Mon, Sep 14, 2009 at 2:51 PM, J. Bruce Fields <bfields@fieldses.org>=
 wrote:
> On Fri, Sep 11, 2009 at 06:52:54PM -0400, andros@netapp.com wrote:
>> From: Andy Adamson <andros@netapp.com>
>>
>> The reply RPC header is 12 bytes, the NULL verifier is 8 bytes givin=
g 20
>> not 24 bytes in the NFSD_MIN_HDR_SEQ_SZ calculation,
>
> No, 24 is right. =A0Maybe you forgot the accept stat at the end?:

yes - missed the accept state.

-->Andy
>
> =A0 =A0 =A0 =A0xid
> =A0 =A0 =A0 =A0msg type
> =A0 =A0 =A0 =A0reply stat
> =A0 =A0 =A0 =A0verf flavor
> =A0 =A0 =A0 =A0verf length
> =A0 =A0 =A0 =A0accept stat
>
> --b.
>
>>
>> Signed-off-by: Andy Adamson <andros@netapp.com>
>> ---
>> =A0fs/nfsd/nfs4state.c | =A0 =A04 ++--
>> =A01 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 46e9ac5..5ecb42c 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -420,12 +420,12 @@ gen_sessionid(struct nfsd4_session *ses)
>> =A0 * each time. =A0Therefore we can advertise a ca_maxresponssize_c=
ached
>> =A0 * value that is the number of bytes in our cache plus a few addi=
tional
>> =A0 * bytes. =A0In order to stay on the safe side, and not promise m=
ore than
>> - * we can cache, those additional bytes must be the minimum possibl=
e: 24
>> + * we can cache, those additional bytes must be the minimum possibl=
e: 20
>> =A0 * bytes of rpc header (xid through accept state, with AUTH_NULL
>> =A0 * verifier), 12 for the compound header (with zero-length tag), =
and 44
>> =A0 * for the SEQUENCE op response:
>> =A0 */
>> -#define NFSD_MIN_HDR_SEQ_SZ =A0(24 + 12 + 44)
>> +#define NFSD_MIN_HDR_SEQ_SZ =A0(20 + 12 + 44)
>>
>> =A0/*
>> =A0 * Give the client the number of ca_maxresponsesize_cached slots =
it
>> --
>> 1.6.2.5
>>
> _______________________________________________
> pNFS mailing list
> pNFS@linux-nfs.org
> http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [pnfs] [PATCH 2/2] nfsd41: add RPC header size to fore channel negotiation
  2009-09-14 19:03     ` J. Bruce Fields
@ 2009-09-18 16:47       ` William A. (Andy) Adamson
       [not found]         ` <89c397150909180947w69b0139dx732100109e2793b0-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 11+ messages in thread
From: William A. (Andy) Adamson @ 2009-09-18 16:47 UTC (permalink / raw)
  To: J. Bruce Fields; +Cc: linux-nfs, pnfs

On Mon, Sep 14, 2009 at 3:03 PM, J. Bruce Fields <bfields@fieldses.org>=
 wrote:
> On Fri, Sep 11, 2009 at 06:52:55PM -0400, andros@netapp.com wrote:
>> From: Andy Adamson <andros@netapp.com>
>>
>> Both the max request and the max response size include the RPC heade=
r with
>> credential (request only) =A0and verifier as well as the payload.
>>
>> The RPCSEC_GSS credential and verifier are the largest. Kerberos is =
the only
>> supported GSS security mechansim, so the Kerberos GSS credential and=
 verifier
>> sizes are used.
>
> Rather than trying to estimate this is might be simplest just to use
> what the server's using to allocate memory: RPCSVC_MAXPAGES. =A0No, t=
hat
> also takes into account space for the reply. =A0You could do
>
> =A0 =A0 =A0 =A0PAGE_SIZE * (1 + (RPCSVC_MAXPAYLOAD+PAGE_SIZE-1)/PAGE_=
SIZE)
>
> Actually, by design the server's real limit is actually on the sum of
> the request and the reply sizes.

I think the actual limit is svc_max_payload rounded up to a multiple
of PAGE_SIZE plus PAGE_SIZE. which is a lot smaller than the sum of
the request and reply sizes. See below.

Note that svc_max_payload is what is returned in nfs4_encode_fattr for
MAXREAD and for MAXWRITE. These attributes use svc_max_payload in the
same way this patch does - the maximum data size not including rpc
headers.

I don't think the server wants is to advertise a MAXREAD/WRITE that it
can't supply because the fore channel maxrequest/maxresponse is too
small, so some additional space needs to be added to svc_max_payload
for the fore channel.

>
> What happens if we get a request such that both the request and reply
> are under our advertised limits, but the sum is too much? =A0Can we j=
ust
> declare that no client will be that weird and that we shouldn't have =
to
> worry about it?

I think the server already has this problem. In svc_init_buffer which
sets up the pages for a server thread request/response handling, it
uses sv_max_mesg / PAGE_SIZE + 1 with the comment

"extra page as we hold both request and reply. We assume one is at
most one page"

where
sv_max_mesg =3D roundup(serv->sv_max_payload + PAGE_SIZE, PAGE_SIZE).

-->Andy

>
> --b.
> _______________________________________________
> pNFS mailing list
> pNFS@linux-nfs.org
> http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [pnfs] [PATCH 2/2] nfsd41: add RPC header size to fore channel negotiation
       [not found]         ` <89c397150909180947w69b0139dx732100109e2793b0-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2009-09-18 21:12           ` J. Bruce Fields
  2009-09-21 12:29             ` William A. (Andy) Adamson
  0 siblings, 1 reply; 11+ messages in thread
From: J. Bruce Fields @ 2009-09-18 21:12 UTC (permalink / raw)
  To: William A. (Andy) Adamson; +Cc: linux-nfs, pnfs

On Fri, Sep 18, 2009 at 12:47:57PM -0400, William A. (Andy) Adamson wro=
te:
> On Mon, Sep 14, 2009 at 3:03 PM, J. Bruce Fields <bfields-uC3wQj2KruMpug/h7KTFAQ@public.gmane.org=
g> wrote:
> > On Fri, Sep 11, 2009 at 06:52:55PM -0400, andros@netapp.com wrote:
> >> From: Andy Adamson <andros@netapp.com>
> >>
> >> Both the max request and the max response size include the RPC hea=
der with
> >> credential (request only) =C2=A0and verifier as well as the payloa=
d.
> >>
> >> The RPCSEC_GSS credential and verifier are the largest. Kerberos i=
s the only
> >> supported GSS security mechansim, so the Kerberos GSS credential a=
nd verifier
> >> sizes are used.
> >
> > Rather than trying to estimate this is might be simplest just to us=
e
> > what the server's using to allocate memory: RPCSVC_MAXPAGES. =C2=A0=
No, that
> > also takes into account space for the reply. =C2=A0You could do
> >
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0PAGE_SIZE * (1 + (RPCSVC_MAXPAYLOAD+PAGE=
_SIZE-1)/PAGE_SIZE)
> >
> > Actually, by design the server's real limit is actually on the sum =
of
> > the request and the reply sizes.
>=20
> I think the actual limit is svc_max_payload rounded up to a multiple
> of PAGE_SIZE plus PAGE_SIZE. which is a lot smaller than the sum of
> the request and reply sizes. See below.

Right.   I think you're agreeing with me?

> Note that svc_max_payload is what is returned in nfs4_encode_fattr fo=
r
> MAXREAD and for MAXWRITE. These attributes use svc_max_payload in the
> same way this patch does - the maximum data size not including rpc
> headers.
>=20
> I don't think the server wants is to advertise a MAXREAD/WRITE that i=
t
> can't supply because the fore channel maxrequest/maxresponse is too
> small, so some additional space needs to be added to svc_max_payload
> for the fore channel.

Yes.

> > What happens if we get a request such that both the request and rep=
ly
> > are under our advertised limits, but the sum is too much? =C2=A0Can=
 we just
> > declare that no client will be that weird and that we shouldn't hav=
e to
> > worry about it?
>=20
> I think the server already has this problem. In svc_init_buffer which
> sets up the pages for a server thread request/response handling, it
> uses sv_max_mesg / PAGE_SIZE + 1 with the comment
>=20
> "extra page as we hold both request and reply. We assume one is at
> most one page"
>=20
> where
> sv_max_mesg =3D roundup(serv->sv_max_payload + PAGE_SIZE, PAGE_SIZE).

Right.  The difference is that now it looks to me like we're actually
going to start promising that we support the large request + large
response case, when actually we don't.

I guess the problem's unlikely to arise, so maybe it's not worth fixing=
=2E
But it's annoying to have yet another undocumented restriction on the
compounds we'll accept.

--b.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [pnfs] [PATCH 2/2] nfsd41: add RPC header size to fore channel negotiation
  2009-09-18 21:12           ` J. Bruce Fields
@ 2009-09-21 12:29             ` William A. (Andy) Adamson
       [not found]               ` <89c397150909210529q5b119d9dmff958dfe2d859635-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 11+ messages in thread
From: William A. (Andy) Adamson @ 2009-09-21 12:29 UTC (permalink / raw)
  To: J. Bruce Fields; +Cc: linux-nfs, pnfs

On Fri, Sep 18, 2009 at 5:12 PM, J. Bruce Fields <bfields@fieldses.org>=
 wrote:
> On Fri, Sep 18, 2009 at 12:47:57PM -0400, William A. (Andy) Adamson w=
rote:
>> On Mon, Sep 14, 2009 at 3:03 PM, J. Bruce Fields <bfields@fieldses.o=
rg> wrote:
>> > On Fri, Sep 11, 2009 at 06:52:55PM -0400, andros@netapp.com wrote:
>> >> From: Andy Adamson <andros@netapp.com>
>> >>
>> >> Both the max request and the max response size include the RPC he=
ader with
>> >> credential (request only) =A0and verifier as well as the payload.
>> >>
>> >> The RPCSEC_GSS credential and verifier are the largest. Kerberos =
is the only
>> >> supported GSS security mechansim, so the Kerberos GSS credential =
and verifier
>> >> sizes are used.
>> >
>> > Rather than trying to estimate this is might be simplest just to u=
se
>> > what the server's using to allocate memory: RPCSVC_MAXPAGES. =A0No=
, that
>> > also takes into account space for the reply. =A0You could do
>> >
>> > =A0 =A0 =A0 =A0PAGE_SIZE * (1 + (RPCSVC_MAXPAYLOAD+PAGE_SIZE-1)/PA=
GE_SIZE)
>> >
>> > Actually, by design the server's real limit is actually on the sum=
 of
>> > the request and the reply sizes.
>>
>> I think the actual limit is svc_max_payload rounded up to a multiple
>> of PAGE_SIZE plus PAGE_SIZE. which is a lot smaller than the sum of
>> the request and reply sizes. See below.
>
> Right. =A0 I think you're agreeing with me?

yes!

>
>> Note that svc_max_payload is what is returned in nfs4_encode_fattr f=
or
>> MAXREAD and for MAXWRITE. These attributes use svc_max_payload in th=
e
>> same way this patch does - the maximum data size not including rpc
>> headers.
>>
>> I don't think the server wants is to advertise a MAXREAD/WRITE that =
it
>> can't supply because the fore channel maxrequest/maxresponse is too
>> small, so some additional space needs to be added to svc_max_payload
>> for the fore channel.
>
> Yes.

=46or the additional space, shall we use what this patch calculates or
some other metric?

>
>> > What happens if we get a request such that both the request and re=
ply
>> > are under our advertised limits, but the sum is too much? =A0Can w=
e just
>> > declare that no client will be that weird and that we shouldn't ha=
ve to
>> > worry about it?
>>
>> I think the server already has this problem. In svc_init_buffer whic=
h
>> sets up the pages for a server thread request/response handling, it
>> uses sv_max_mesg / PAGE_SIZE + 1 with the comment
>>
>> "extra page as we hold both request and reply. We assume one is at
>> most one page"
>>
>> where
>> sv_max_mesg =3D roundup(serv->sv_max_payload + PAGE_SIZE, PAGE_SIZE)=
=2E
>
> Right. =A0The difference is that now it looks to me like we're actual=
ly
> going to start promising that we support the large request + large
> response case, when actually we don't.

OK - I see your point. With MAXREAD or MAXWRITE we only promise either
a large request or a large response per compound, not a large request
and a large response in a single compound, e.g. a read and a write in
the same compound.

>
> I guess the problem's unlikely to arise, so maybe it's not worth fixi=
ng.
> But it's annoying to have yet another undocumented restriction on the
> compounds we'll accept.

I wonder what other servers are doing.

-->Andy

>
> --b.
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [pnfs] [PATCH 2/2] nfsd41: add RPC header size to fore channel negotiation
       [not found]               ` <89c397150909210529q5b119d9dmff958dfe2d859635-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2009-09-21 21:20                 ` J. Bruce Fields
  0 siblings, 0 replies; 11+ messages in thread
From: J. Bruce Fields @ 2009-09-21 21:20 UTC (permalink / raw)
  To: William A. (Andy) Adamson; +Cc: linux-nfs, pnfs

On Mon, Sep 21, 2009 at 08:29:30AM -0400, William A. (Andy) Adamson wro=
te:
> On Fri, Sep 18, 2009 at 5:12 PM, J. Bruce Fields <bfields-uC3wQj2KruMpug/h7KTFAQ@public.gmane.org=
g> wrote:
> > On Fri, Sep 18, 2009 at 12:47:57PM -0400, William A. (Andy) Adamson=
 wrote:
> >> On Mon, Sep 14, 2009 at 3:03 PM, J. Bruce Fields <bfields@fieldses=
=2Eorg> wrote:
> >> > On Fri, Sep 11, 2009 at 06:52:55PM -0400, andros@netapp.com wrot=
e:
> >> >> From: Andy Adamson <andros@netapp.com>
> >> >>
> >> >> Both the max request and the max response size include the RPC =
header with
> >> >> credential (request only) =C2=A0and verifier as well as the pay=
load.
> >> >>
> >> >> The RPCSEC_GSS credential and verifier are the largest. Kerbero=
s is the only
> >> >> supported GSS security mechansim, so the Kerberos GSS credentia=
l and verifier
> >> >> sizes are used.
> >> >
> >> > Rather than trying to estimate this is might be simplest just to=
 use
> >> > what the server's using to allocate memory: RPCSVC_MAXPAGES. =C2=
=A0No, that
> >> > also takes into account space for the reply. =C2=A0You could do
> >> >
> >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0PAGE_SIZE * (1 + (RPCSVC_MAXPAYLOAD+P=
AGE_SIZE-1)/PAGE_SIZE)
> >> >
> >> > Actually, by design the server's real limit is actually on the s=
um of
> >> > the request and the reply sizes.
> >>
> >> I think the actual limit is svc_max_payload rounded up to a multip=
le
> >> of PAGE_SIZE plus PAGE_SIZE. which is a lot smaller than the sum o=
f
> >> the request and reply sizes. See below.
> >
> > Right. =C2=A0 I think you're agreeing with me?
>=20
> yes!
>=20
> >
> >> Note that svc_max_payload is what is returned in nfs4_encode_fattr=
 for
> >> MAXREAD and for MAXWRITE. These attributes use svc_max_payload in =
the
> >> same way this patch does - the maximum data size not including rpc
> >> headers.
> >>
> >> I don't think the server wants is to advertise a MAXREAD/WRITE tha=
t it
> >> can't supply because the fore channel maxrequest/maxresponse is to=
o
> >> small, so some additional space needs to be added to svc_max_paylo=
ad
> >> for the fore channel.
> >
> > Yes.
>=20
> For the additional space, shall we use what this patch calculates or
> some other metric?

I guess something like roundup(serv->sv_max_payload + PAGE_SIZE,
PAGE_SIZE) is the best we can do for now.

> >> > What happens if we get a request such that both the request and =
reply
> >> > are under our advertised limits, but the sum is too much? =C2=A0=
Can we just
> >> > declare that no client will be that weird and that we shouldn't =
have to
> >> > worry about it?
> >>
> >> I think the server already has this problem. In svc_init_buffer wh=
ich
> >> sets up the pages for a server thread request/response handling, i=
t
> >> uses sv_max_mesg / PAGE_SIZE + 1 with the comment
> >>
> >> "extra page as we hold both request and reply. We assume one is at
> >> most one page"
> >>
> >> where
> >> sv_max_mesg =3D roundup(serv->sv_max_payload + PAGE_SIZE, PAGE_SIZ=
E).
> >
> > Right. =C2=A0The difference is that now it looks to me like we're a=
ctually
> > going to start promising that we support the large request + large
> > response case, when actually we don't.
>=20
> OK - I see your point. With MAXREAD or MAXWRITE we only promise eithe=
r
> a large request or a large response per compound, not a large request
> and a large response in a single compound, e.g. a read and a write in
> the same compound.
>=20
> >
> > I guess the problem's unlikely to arise, so maybe it's not worth fi=
xing.
> > But it's annoying to have yet another undocumented restriction on t=
he
> > compounds we'll accept.
>=20
> I wonder what other servers are doing.

Yeah.  For now I guess we should ignore this.  (If you wanted to, in th=
e
same patch, add a note about this problem to the end of
Documentation/filesystems/nfs41-server.txt, that would be good.)

--b.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2009-09-21 21:20 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-09-11 22:52 [PATCH 0/2] nfsd41: fix fore channel attribute initialization andros
2009-09-11 22:52 ` [PATCH 1/2] nfsd41: fix NFSD_MIN_HDR_SEQ_SZ andros
2009-09-11 22:52   ` [PATCH 2/2] nfsd41: add RPC header size to fore channel negotiation andros
2009-09-14 19:03     ` J. Bruce Fields
2009-09-18 16:47       ` [pnfs] " William A. (Andy) Adamson
     [not found]         ` <89c397150909180947w69b0139dx732100109e2793b0-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2009-09-18 21:12           ` J. Bruce Fields
2009-09-21 12:29             ` William A. (Andy) Adamson
     [not found]               ` <89c397150909210529q5b119d9dmff958dfe2d859635-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2009-09-21 21:20                 ` J. Bruce Fields
2009-09-14 18:51   ` [PATCH 1/2] nfsd41: fix NFSD_MIN_HDR_SEQ_SZ J. Bruce Fields
2009-09-14 20:11     ` [pnfs] " William A. (Andy) Adamson
2009-09-14 18:54 ` [PATCH 0/2] nfsd41: fix fore channel attribute initialization J. Bruce Fields

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.