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

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

[Benny Halevy]

From: Andy Adamson <andros@netapp.com>

The size of the nfsd4_op array in nfsd4_compoundargs determines the
supported maximum number of operations.

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

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

[J. Bruce Fields]

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

)

--b.

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

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

[J. Bruce Fields]

On Thu, Jun 18, 2009 at 06:14:43PM -0400, bfields 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."

(Went ahead and did that, and applied.)

--b.

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

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

[Mike Mackovitch]

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

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

[J. Bruce Fields]

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

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

[Mike Mackovitch]

On Thu, Jun 18, 2009 at 09:04:10PM -0400, J. Bruce Fields wrote:
> On Thu, Jun 18, 2009 at 05:49:36PM -0700, Mike Mackovitch wrote:
> 
> > 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.

Drat!  I was kinda hoping you wouldn't ask.  :-)

MacOSX's VFS currently has both an extended attribute interface and a
named stream (think "resource fork") interface.  I was investigating
using NFSv4(.0) named attributes to back both, and I was trying to devise
an efficient method of "getting" named attributes (and their typically-small
contents) and properly maintaining a cache of named attribute directory/file
attributes and contents.

I wanted to minimize the number of round trips to the server.  I also
wanted to minimize code duplication.  And I ended up with a common
function that sends a compound containing 5, 8, or 12 ops - depending
on what we had cached already and if we were trying to also READ data:

PUTFH       - of the file/directory we want the named attribute for
OPENATTR    - to get the named attribute directory
GETATTR(FH) - to be able to cache the attribute directory (name cache and readdir cache)
LOOKUP or OPEN(potentially with CREATE)
GETATTR(FH) - to get the named attribute file's file handle and attributes
SAVEFH      - to save it for reading the first chunk of data from it
PUTFH       - of the file/directory
OPENATTR    - to get back to its named attribute directory
GETATTR     - to get updated attributes for the named attribute directory
RESTOREFH   - get back to the (potentially new) named attribute file
NVERIFY (size==0) - don't bother trying to read if the named attribute file is empty
READ        - read the first chunk (and potentially all of) of the named attribute file

If we already have the attribute directory we don't need to do the OPENATTR's
or the first GETATTR (we just PUTFH the attribute directory's fh).  We'd
like to also fetch post-lookup/open attributes for the attribute directory.
And we want to leave the READ results for last (to simplify some of the coding).
If we're not READing we don't need to send the SAVEFH, RESTOREFH, NVERIFY, READ.

It's not necessarily pretty (some might argue it's ugly)... but it appears to
work and I don't think it's too much of an "abuse" of the protocol.  :-)

In any event, while we don't normally expect to use long compounds, we still
would like to be able to use them when we're trying to efficiently handle the
demands of an operating system that depends on certain file system features.

So, if a server is going to put a limit on the number of ops in a compound,
we'd prefer if it were higher rather than lower.  8 seems too low.  16 might
be good but I'm thinking 24 or 32 might be a safer, longer-term limit to
satisfy the needs of future "demanding" NFS clients.

Thanks
--macko