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

["J. Bruce Fields" <bfields@fieldses.org>]

On Thu, Jun 18, 2009 at 05:49:36PM -0700, Mike Mackovitch wrote:
> 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.

Oops--I glanced at that code, saw the opcnt check, then turned my brain
off.  Thanks!  That patch wasn't in my published branch yet, so I've
removed it....

> 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.

OK.  I wanna know how you got up to 12, though! That's quite a compound.

--b.

> 
> 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