On Aug 21, 2012, at 2:24 PM, J. Bruce Fields wrote:
> On Fri, Aug 10, 2012 at 05:22:17PM -0400, Chuck Lever wrote:
>> Can you explain why you think its a better idea for the client to
>> remove its lease? For 4.0, it's never had to do that before. The
>> case you've hit worked before by establishing an additional lease
>> rather than by zapping the old one, for example.
>>
>> Moreover, it shouldn't matter much that a client doesn't tell a server
>> when it's done, since the client's leases will quickly expire.
>> Generally the client cleans up everything but the lease and clientid4
>> when it unmounts, unless the client crashes, which is hopefully rare
>> (and couldn't result in an "I'm done" notification anyway). So, I
>> don't see how leaving a lease could be a significant server-side
>> resource issue, or how it might interfere with the operation of other
>> clients.
>
> Well, I've only seen the problem in this case. I'm not seeing why the
> client couldn't clean up its clientid as well, but there's also a server
> problem here: I don't believe it should be returning INUSE when the
> preexisting lease actually has no state associated with it any more. So
> my server's detection of when a clientid has no state is broken. I'll
> work on that.
Have you tried this optimization with a client that has concurrent mounts with different GSS flavors? The below patch may make the optimization moot.
> For now I'm also dropping the flavor comparison, if this look OK.
Very interesting, looks plausible. Commit d5497fc6 is in only 3.5?
With the below change and the current 3.6-rc2 NFS client, does the second open succeed now?
> --b.
>
> commit d81a6e088c8c884f9be58dfe3647cbff22e1f112
> Author: J. Bruce Fields <bfields@xxxxxxxxxx>
> Date: Tue Aug 21 12:48:30 2012 -0400
>
> nfsd4: don't pin clientids to pseudoflavors
>
> I added cr_flavor to the data compared in same_creds without any
> justification, in d5497fc693a446ce9100fcf4117c3f795ddfd0d2 "nfsd4: move
> rq_flavor into svc_cred".
>
> Recent client changes then started making
>
> mount -osec=krb5 server:/export /mnt/
> echo "hello" >/mnt/TMP
> umount /mnt/
> mount -osec=krb5i server:/export /mnt/
> echo "hello" >/mnt/TMP
>
> to fail due to a clid_inuse on the second open.
>
> Mounting sequentially like this with different flavors probably isn't
> that common outside artificial tests. Also, the real bug here may be
> that the server isn't just destroying the former clientid in this case
> (because it isn't good enough at recognizing when the old state is
> gone). But it prompted some discussion and a look back at the spec, and
> I think the check was probably wrong. Fix and document.
>
> Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxx>
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 6872852..b5058c2 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1223,10 +1223,26 @@ static bool groups_equal(struct group_info *g1, struct group_info *g2)
> return true;
> }
>
> +/*
> + * RFC 3530 language requires clid_inuse be returned when the
> + * "principal" associated with a requests differs from that previously
> + * used. We use uid, gid's, and gss principal string as our best
> + * approximation. We also don't want to allow non-gss use of a client
> + * established using gss: in theory cr_principal should catch that
> + * change, but in practice cr_principal can be null even in the gss case
> + * since gssd doesn't always pass down a principal string.
> + */
> +static bool is_gss_cred(struct svc_cred *cr)
> +{
> + /* Is cr_flavor one of the gss "pseudoflavors"?: */
> + return (cr->cr_flavor > RPC_AUTH_MAXFLAVOR);
> +}
> +
> +
> static bool
> same_creds(struct svc_cred *cr1, struct svc_cred *cr2)
> {
> - if ((cr1->cr_flavor != cr2->cr_flavor)
> + if ((is_gss_cred(cr1) != is_gss_cred(cr2))
> || (cr1->cr_uid != cr2->cr_uid)
> || (cr1->cr_gid != cr2->cr_gid)
> || !groups_equal(cr1->cr_group_info, cr2->cr_group_info))
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
