Re: [PATCH 28/44] nfsd41: use the maximum operations per compound in nfsd4_compoundargs

[Mike Mackovitch <macko-2kanFRK1NckAvxtiuMwx3w@public.gmane.org>]

On Thu, Jun 18, 2009 at 06:14:43PM -0400, J. Bruce Fields wrote:
> On Tue, Jun 16, 2009 at 04:20:33AM +0300, Benny Halevy wrote:
> > From: Andy Adamson <andros@netapp.com>
> > 
> > The size of the nfsd4_op array in nfsd4_compoundargs determines the
> > supported maximum number of operations.
> 
> This is another one that is a clear straightfoward bugfix to existing
> code, so please put it right up at the front of the patch series.
> 
> (ALso a comment that more clearly explains the problem would help, say,
> like:
> 
> 	"We're returning NFSD_MAX_OPS_PER_COMPOUND = 16 to 4.1 clients,
> 	but the limit the server actually enforces (in
> 	nfsd4_decode_compound) is 8.  Fix the value of
> 	NFSD_MAX_OPS_PER_COMPOUND to return the correct value to 4.1
> 	clients."
> 
> )

Doesn't nfsd4_decode_compound() handle more ops by allocating
a separate, larger array to hold them?:

>From http://lxr.linux.no/linux+v2.6.30/fs/nfsd/nfs4xdr.c :

1424        if (argp->opcnt > 100)
1425                goto xdr_error;
1426
1427        if (argp->opcnt > ARRAY_SIZE(argp->iops)) {
1428                argp->ops = kmalloc(argp->opcnt * sizeof(*argp->ops), GFP_KERNEL);
1429                if (!argp->ops) {
1430                        argp->ops = argp->iops;
1431                        dprintk("nfsd: couldn't allocate room for COMPOUND\n");
1432                        goto xdr_error;
1433                }
1434        }

That looks to me like up to 100 ops are allowed.

As an NFS client implementer I wouldn't expect to generate a compound
with anything near 100 ops.  However, I have recently composed a
compound consisting of up to 12 ops (attempting to be efficient in
some NFSv4.0 named attribute code).  And 12 is larger than 8.... so
I'll probably be sad if Linux isn't going to be supporting that many
ops in a compound.

Thanks
--macko

> > Signed-off-by: Andy Adamson <andros@netapp.com>
> > Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> > ---
> >  include/linux/nfsd/state.h |    3 +--
> >  include/linux/nfsd/xdr4.h  |    2 +-
> >  2 files changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/linux/nfsd/state.h b/include/linux/nfsd/state.h
> > index aea8137..093f165 100644
> > --- a/include/linux/nfsd/state.h
> > +++ b/include/linux/nfsd/state.h
> > @@ -94,8 +94,7 @@ struct nfs4_cb_conn {
> >  
> >  #define NFSD_MAX_SLOTS_PER_SESSION	16
> >  #define NFSD_SLOT_CACHE_SIZE		512
> > -/* Maximum number of operations per session compound */
> > -#define NFSD_MAX_OPS_PER_COMPOUND	16
> > +#define NFSD_MAX_OPS_PER_COMPOUND	8
> >  
> >  struct nfsd4_slot {
> >  	bool	sl_inuse;
> > diff --git a/include/linux/nfsd/xdr4.h b/include/linux/nfsd/xdr4.h
> > index 84ac4bb..d99c8fe 100644
> > --- a/include/linux/nfsd/xdr4.h
> > +++ b/include/linux/nfsd/xdr4.h
> > @@ -446,7 +446,7 @@ struct nfsd4_compoundargs {
> >  	u32				minorversion;
> >  	u32				opcnt;
> >  	struct nfsd4_op			*ops;
> > -	struct nfsd4_op			iops[8];
> > +	struct nfsd4_op			iops[NFSD_MAX_OPS_PER_COMPOUND];
> >  };
> >  
> >  struct nfsd4_compoundres {
> > -- 
> > 1.6.3
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html