2009-09-18 21:12:50

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [pnfs] [PATCH 2/2] nfsd41: add RPC header size to fore channel negotiation

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/[email protected]=
g> wrote:
> > On Fri, Sep 11, 2009 at 06:52:55PM -0400, [email protected] wrote:
> >> From: Andy Adamson <[email protected]>
> >>
> >> 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.


2009-09-21 12:50:10

by Andy Adamson

[permalink] [raw]
Subject: Re: [pnfs] [PATCH 2/2] nfsd41: add RPC header size to fore channel negotiation

On Fri, Sep 18, 2009 at 5:12 PM, J. Bruce Fields <[email protected]>=
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 <[email protected]=
rg> wrote:
>> > On Fri, Sep 11, 2009 at 06:52:55PM -0400, [email protected] wrote:
>> >> From: Andy Adamson <[email protected]>
>> >>
>> >> 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.
>