loadparm in Samba4

[simo]

On Sat, 2006-09-16 at 10:17 -0500, tridge at samba.org wrote:
> Simo,
> 
> I hit a snag with the new parameter handling code in Samba4. A lot of
> hosts in the build farm are currently failing the test_simple.sh test
> (which tests BASE-RW1 against the simple ntvfs backend), because the
> CHECK_READ_ONLY() check in each function in that backend is returning
> 'true', even though we have 'read only = no' in smb.conf.
> 
> It turned out that the problem was the change to a real bool type, and
> the call to set_boolean() in loadparm.c. On big-endian hosts where
> sizeof(int) != sizeof(BOOL) it broke. It should be fixed now.
> 
> Anyway, this prompted me to take a closer look at the new share
> parameter code, and I'm concerned about the speed.
> 
> In the old code, doing something like lp_readonly() involved maybe a
> couple of dozen machine instructions. It was just a function call and
> a pointer defer to return an integer. Very fast!
> 
> In the new code, the CHECK_READ_ONLY() test does the following:
> 
>  - deref two pointers in getting ntvfs->ctx->config
>  - deref 3 more pointers in getting to scfg->ctx->ops->bool_option()
>  - 1 strchr() call to look for a ':'
>  - 3 strcmp calls to find the lp_readonly() function
>  - finally the dereference is done
> 
> this is way too slow! 

Indeed!

> If we convert all our code to preload parameters the way the posix
> backend does (filling in pvfs_setup_options()) then the speed is OK,
> but I think we still have a lot of places where we do simple loadparm
> checks at runtime. Innocuous looking macros like CHECK_READ_ONLY() in
> the simple backend now expand to some very slow code.
> 
> I think we need to either make these common calls much faster, or make
> a concerted effort to get rid of any frequent calls to loadparm
> functions. 

I think we should get rid of any frequent calls to loadparm, and have
all the most needed and speed sensitive option cached on a per share
structure. About the readonly parm, I'd really like to get rid of it
completely or confine it into a generic ACL check. When you use Windows
mgmt tools to create or manipulate share there is no way to manipulate
it, so I'd really like to move to a model where we use only Share ACLs.

With the share_ldb module this is easy, we can simply save the share ACL
together with other data. If we want to keep stuff like readonly, valid
user, etc... in the classic module for backward compatibility purposes I
suggest we create a way to read this data and create an equivalent ACL
for access control purposes. This will make the code only care about
ACLs, while the share configuration backends will take care of handling
per backend specific representation of said ACLs.

This means that real share ACLs will not be available if the
share_classic backend is used, but in my experience this is only a good
thing, because mixing readonly/valid user etc... with share ACLs is
always a disaster in the end. Only a handful of careful admins would be
able to mix them and still understand whats going on.

Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer
email: idra at samba.org
http://samba.org