[pnfs] [PATCH 5/8] nfsd: tabulate nfs4 xdr encoding functions

J. Bruce Fields bfields at fieldses.org
Thu Jul 3 16:38:52 EDT 2008


On Thu, Jul 03, 2008 at 08:24:12PM +0300, Benny Halevy wrote:
> On Jul. 03, 2008, 17:29 +0300, "J. Bruce Fields" <bfields at fieldses.org> wrote:
> > On Thu, Jul 03, 2008 at 10:40:16AM +0300, Benny Halevy wrote:
> >>> #warnings:
> >>> CID1
> >>> LOCK8c
> >>> LOCK18
> >>> LOCK21
> >>> OPDG9a
> >>> OPDG9b
> >>> OPDG9c
> >>> OPDG9d
> >>> OPDG9f
> >>> OPDG9s
> >>> OPEN16
> >>> RD4
> >>> RM7
> >>> RNM10
> >> Wow :-(
> >> I'll look into these asap.
> > 
> > Just to make sure we understand each other: I'm *not* advising going
> > through that list and trying to fix each one.  If you really want to,
> > I've got a version of the list that's partially digested with some ideas
> > about which are worth working on, but that's probably not the best use
> > of your time.
> > 
> > I just use that list just to get a clean newpynfs run on recent kernels.
> > So I run
> > 
> > 	newpynfs $server:/$serverpath/pynfstest
> > 		--rundeps --hidepass --maketree all notimed noutf8
> > 		(list starts here:) noWRT5 noRNM10 noRM7 noRD4...
> > 
> > and it prints only the failures, so I immediately see any regressions.
> > 
> > You could accomplish the same by doing a diff against a known run,
> > except I think some of the known failures may be intermittent.
> > 
> > --b.
> 
> I ran ./testserver.py -v --out=/tmp/newpynfs-upstream.out buml:/pynfstest --rundeps --hidepass --maketree all
> on both v2.6.26-rc8 and my upstream branch. diff of the results showed
> no regressions with the following patch.
> 
> This patch adds a __be32 status return to all encoding functions.
> It also populates noop encoders for OP_DELEGPURGE and OP_PUTPUBFH
> (that also go through the common encoding routine w/
> nfserr_opntosupp error)
> 
> I'm going to squash it into "nfsd: tabulate nfs4 xdr encoding functions"
> and send you an updated version if that's ok.

That sounds reasonable, thanks!

--b.

> 
> Benny
> 
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index facd247..9b6a9ba 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -1950,7 +1950,7 @@ fail:
>  	return -EINVAL;
>  }
>  
> -static void
> +static __be32
>  nfsd4_encode_access(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_access *access)
>  {
>  	ENCODE_HEAD;
> @@ -1961,9 +1961,10 @@ nfsd4_encode_access(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_
>  		WRITE32(access->ac_resp_access);
>  		ADJUST_ARGS();
>  	}
> +	return nfserr;
>  }
>  
> -static void
> +static __be32
>  nfsd4_encode_close(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_close *close)
>  {
>  	ENCODE_SEQID_OP_HEAD;
> @@ -1975,10 +1976,11 @@ nfsd4_encode_close(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_c
>  		ADJUST_ARGS();
>  	}
>  	ENCODE_SEQID_OP_TAIL(close->cl_stateowner);
> +	return nfserr;
>  }
>  
>  
> -static void
> +static __be32
>  nfsd4_encode_commit(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_commit *commit)
>  {
>  	ENCODE_HEAD;
> @@ -1988,9 +1990,10 @@ nfsd4_encode_commit(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_
>  		WRITEMEM(commit->co_verf.data, 8);
>  		ADJUST_ARGS();
>  	}
> +	return nfserr;
>  }
>  
> -static void
> +static __be32
>  nfsd4_encode_create(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_create *create)
>  {
>  	ENCODE_HEAD;
> @@ -2003,6 +2006,7 @@ nfsd4_encode_create(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_
>  		WRITE32(create->cr_bmval[1]);
>  		ADJUST_ARGS();
>  	}
> +	return nfserr;
>  }
>  
>  static __be32
> @@ -2023,7 +2027,7 @@ nfsd4_encode_getattr(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4
>  	return nfserr;
>  }
>  
> -static void
> +static __be32
>  nfsd4_encode_getfh(struct nfsd4_compoundres *resp, __be32 nfserr, struct svc_fh **fhpp)
>  {
>  	struct svc_fh *fhp = *fhpp;
> @@ -2037,6 +2041,7 @@ nfsd4_encode_getfh(struct nfsd4_compoundres *resp, __be32 nfserr, struct svc_fh
>  		WRITEMEM(&fhp->fh_handle.fh_base, len);
>  		ADJUST_ARGS();
>  	}
> +	return nfserr;
>  }
>  
>  /*
> @@ -2064,7 +2069,7 @@ nfsd4_encode_lock_denied(struct nfsd4_compoundres *resp, struct nfsd4_lock_denie
>  	ADJUST_ARGS();
>  }
>  
> -static void
> +static __be32
>  nfsd4_encode_lock(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_lock *lock)
>  {
>  	ENCODE_SEQID_OP_HEAD;
> @@ -2078,16 +2083,18 @@ nfsd4_encode_lock(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_lo
>  		nfsd4_encode_lock_denied(resp, &lock->lk_denied);
>  
>  	ENCODE_SEQID_OP_TAIL(lock->lk_replay_owner);
> +	return nfserr;
>  }
>  
> -static void
> +static __be32
>  nfsd4_encode_lockt(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_lockt *lockt)
>  {
>  	if (nfserr == nfserr_denied)
>  		nfsd4_encode_lock_denied(resp, &lockt->lt_denied);
> +	return nfserr;
>  }
>  
> -static void
> +static __be32
>  nfsd4_encode_locku(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_locku *locku)
>  {
>  	ENCODE_SEQID_OP_HEAD;
> @@ -2100,10 +2107,11 @@ nfsd4_encode_locku(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_l
>  	}
>  				        
>  	ENCODE_SEQID_OP_TAIL(locku->lu_stateowner);
> +	return nfserr;
>  }
>  
>  
> -static void
> +static __be32
>  nfsd4_encode_link(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_link *link)
>  {
>  	ENCODE_HEAD;
> @@ -2113,10 +2121,11 @@ nfsd4_encode_link(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_li
>  		WRITECINFO(link->li_cinfo);
>  		ADJUST_ARGS();
>  	}
> +	return nfserr;
>  }
>  
>  
> -static void
> +static __be32
>  nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_open *open)
>  {
>  	ENCODE_SEQID_OP_HEAD;
> @@ -2179,9 +2188,10 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_op
>  	/* XXX save filehandle here */
>  out:
>  	ENCODE_SEQID_OP_TAIL(open->op_stateowner);
> +	return nfserr;
>  }
>  
> -static void
> +static __be32
>  nfsd4_encode_open_confirm(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_open_confirm *oc)
>  {
>  	ENCODE_SEQID_OP_HEAD;
> @@ -2194,9 +2204,10 @@ nfsd4_encode_open_confirm(struct nfsd4_compoundres *resp, __be32 nfserr, struct
>  	}
>  
>  	ENCODE_SEQID_OP_TAIL(oc->oc_stateowner);
> +	return nfserr;
>  }
>  
> -static void
> +static __be32
>  nfsd4_encode_open_downgrade(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_open_downgrade *od)
>  {
>  	ENCODE_SEQID_OP_HEAD;
> @@ -2209,6 +2220,7 @@ nfsd4_encode_open_downgrade(struct nfsd4_compoundres *resp, __be32 nfserr, struc
>  	}
>  
>  	ENCODE_SEQID_OP_TAIL(od->od_stateowner);
> +	return nfserr;
>  }
>  
>  static __be32
> @@ -2403,7 +2415,7 @@ err_no_verf:
>  	return nfserr;
>  }
>  
> -static void
> +static __be32
>  nfsd4_encode_remove(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_remove *remove)
>  {
>  	ENCODE_HEAD;
> @@ -2413,9 +2425,10 @@ nfsd4_encode_remove(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_
>  		WRITECINFO(remove->rm_cinfo);
>  		ADJUST_ARGS();
>  	}
> +	return nfserr;
>  }
>  
> -static void
> +static __be32
>  nfsd4_encode_rename(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_rename *rename)
>  {
>  	ENCODE_HEAD;
> @@ -2426,9 +2439,10 @@ nfsd4_encode_rename(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_
>  		WRITECINFO(rename->rn_tinfo);
>  		ADJUST_ARGS();
>  	}
> +	return nfserr;
>  }
>  
> -static void
> +static __be32
>  nfsd4_encode_secinfo(struct nfsd4_compoundres *resp, __be32 nfserr,
>  		     struct nfsd4_secinfo *secinfo)
>  {
> @@ -2492,13 +2506,14 @@ nfsd4_encode_secinfo(struct nfsd4_compoundres *resp, __be32 nfserr,
>  out:
>  	if (exp)
>  		exp_put(exp);
> +	return nfserr;
>  }
>  
>  /*
>   * The SETATTR encode routine is special -- it always encodes a bitmap,
>   * regardless of the error status.
>   */
> -static void
> +static __be32
>  nfsd4_encode_setattr(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_setattr *setattr)
>  {
>  	ENCODE_HEAD;
> @@ -2515,9 +2530,10 @@ nfsd4_encode_setattr(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4
>  		WRITE32(setattr->sa_bmval[1]);
>  	}
>  	ADJUST_ARGS();
> +	return nfserr;
>  }
>  
> -static void
> +static __be32
>  nfsd4_encode_setclientid(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_setclientid *scd)
>  {
>  	ENCODE_HEAD;
> @@ -2534,9 +2550,10 @@ nfsd4_encode_setclientid(struct nfsd4_compoundres *resp, __be32 nfserr, struct n
>  		WRITE32(0);
>  		ADJUST_ARGS();
>  	}
> +	return nfserr;
>  }
>  
> -static void
> +static __be32
>  nfsd4_encode_write(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_write *write)
>  {
>  	ENCODE_HEAD;
> @@ -2548,11 +2565,13 @@ nfsd4_encode_write(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_w
>  		WRITEMEM(write->wr_verifier.data, 8);
>  		ADJUST_ARGS();
>  	}
> +	return nfserr;
>  }
>  
> -static void
> +static __be32
>  nfsd4_encode_noop(struct nfsd4_compoundres *resp, __be32 nfserr, void *p)
>  {
> +	return nfserr;
>  }
>  
>  typedef __be32(* nfsd4_enc)(struct nfsd4_compoundres *, __be32, void *);
> @@ -2562,6 +2581,7 @@ static nfsd4_enc nfsd4_enc_ops[] = {
>  	[OP_CLOSE]		(nfsd4_enc)nfsd4_encode_close,
>  	[OP_COMMIT]		(nfsd4_enc)nfsd4_encode_commit,
>  	[OP_CREATE]		(nfsd4_enc)nfsd4_encode_create,
> +	[OP_DELEGPURGE]		(nfsd4_enc)nfsd4_encode_noop,
>  	[OP_DELEGRETURN]	(nfsd4_enc)nfsd4_encode_noop,
>  	[OP_GETATTR]		(nfsd4_enc)nfsd4_encode_getattr,
>  	[OP_GETFH]		(nfsd4_enc)nfsd4_encode_getfh,
> @@ -2576,6 +2596,7 @@ static nfsd4_enc nfsd4_enc_ops[] = {
>  	[OP_OPEN_CONFIRM]	(nfsd4_enc)nfsd4_encode_open_confirm,
>  	[OP_OPEN_DOWNGRADE]	(nfsd4_enc)nfsd4_encode_open_downgrade,
>  	[OP_PUTFH]		(nfsd4_enc)nfsd4_encode_noop,
> +	[OP_PUTPUBFH]		(nfsd4_enc)nfsd4_encode_noop,
>  	[OP_PUTROOTFH]		(nfsd4_enc)nfsd4_encode_noop,
>  	[OP_READ]		(nfsd4_enc)nfsd4_encode_read,
>  	[OP_READDIR]		(nfsd4_enc)nfsd4_encode_readdir,
> @@ -2605,10 +2626,12 @@ nfsd4_encode_operation(struct nfsd4_compoundres *resp, struct nfsd4_op *op)
>  	statp = p++;	/* to be backfilled at the end */
>  	ADJUST_ARGS();
>  
> +	if (op->opnum == OP_ILLEGAL)
> +		goto status;
>  	BUG_ON(op->opnum < 0 || op->opnum >= ARRAY_SIZE(nfsd4_enc_ops) ||
>  	       !nfsd4_enc_ops[op->opnum]);
> -	nfsd4_enc_ops[op->opnum](resp, op->status, &op->u);
> -
> +	op->status = nfsd4_enc_ops[op->opnum](resp, op->status, &op->u);
> +status:
>  	/*
>  	 * Note: We write the status directly, instead of using WRITE32(),
>  	 * since it is already in network byte order.


More information about the pNFS mailing list