On Sat, 2014-04-19 at 07:50 -0700, Christoph Hellwig wrote:
> > +static void nfs4_put_stateid(struct nfs4_stid *s)
> > +{
> > + if (s == NULL)
> > + return;
> > + switch (s->sc_type) {
> > + case NFS4_OPEN_STID:
> > + case NFS4_LOCK_STID:
> > + case NFS4_CLOSED_STID:
> > + put_generic_stateid(openlockstateid(s));
> > + break;
> > + case NFS4_DELEG_STID:
> > + case NFS4_REVOKED_DELEG_STID:
> > + case NFS4_CLOSED_DELEG_STID:
> > + nfs4_put_delegation(delegstateid(s));
> > + }
> > +}
>
> I really don't like the way the inheritance for the stateids works,
> a pure put operation shouldn't need this. I think all this can be
> fixed by adding a ->free function pointer to struct nfs4_stid. At this
> point the braindamage of passing a kmem_cache pointer to various
> function can be removed (similar to how nfs4_alloc_stid should be
> replaced with a nfs4_init_stid that takes an already allocated stid),
> and nothing in the normal refcounting path should need these switches.
Ack. I've added a free() pointer to nfs4_stid.
> > @@ -3804,26 +3823,33 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
> > return nfserr_bad_stateid;
> > status = check_stateid_generation(stateid, &s->sc_stateid, 1);
> > if (status)
> > - return status;
> > + goto out_put_stid;
> > switch (s->sc_type) {
> > case NFS4_DELEG_STID:
> > - return nfs_ok;
> > + status = nfs_ok;
> > + break;
> > case NFS4_REVOKED_DELEG_STID:
> > - return nfserr_deleg_revoked;
> > + status = nfserr_deleg_revoked;
> > + break;
> > case NFS4_OPEN_STID:
> > case NFS4_LOCK_STID:
> > ols = openlockstateid(s);
> > if (ols->st_stateowner->so_is_open_owner
> > && !(openowner(ols->st_stateowner)->oo_flags
> > & NFS4_OO_CONFIRMED))
> > - return nfserr_bad_stateid;
> > - return nfs_ok;
> > + status = nfserr_bad_stateid;
> > + else
> > + status = nfs_ok;
> > + break;
>
> Not quite as urgent as for the refcounting, but I think moving more
> of these switches on the type into proper methods would improve the
> stateid code a lot.
Agreed, but I'm leaving that as an exercise for the reader (at least as
far as this patch series is concerned).
--
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@xxxxxxxxxxxxxxx
--
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
