[PATCH] Use conn->session_info->security_token in posix_acls.c to make sysvolreset faster (was: Re: [PATCH] improve performance for samba-tool ntacl sysvolreset)

[Uri Simchoni]

Hi,

The idea of using whatever information we already have instead of
re-calculating it is correct IMHO.

What I like less is the idea of an incomplete session_info that we have
to second-guess, as in 3/16 (checking for null unix_info). We may miss
checks now or on in the future, and null-case-handling may be
non-uniform. AFAICT we don't have checks like this in the code, which is
(hopefully) a testament for this field always being populated when the
session_info is built.

So I'm just asking - what would it take to construct a session info with
unix_info on the Python side.

Beside that I'm curious - it seems like the function we're optimizing
(uid_entry_in_group()) gets called in one of the following case:
1. If the SD somehow doesn't translate into a POSIX ACL with a USER_OBJ
2. To emulate deny ACE

Which one of the two gets called in the sysvolreset? (and if it's 1.,
why do we get an ACL without a USER_OBJ when we do a "reset" operation
which should bring things to the detault state)

Thanks,
Uri

On 07/10/2018 06:04 AM, Gary Lockyer via samba-technical wrote:
> Looks good to me RB+
> 
> I'll defer pushing this until tomorrow morning, to get wider feedback.
> 
> Gary.
> 
> On 10/07/18 12:53, Andrew Bartlett via samba-technical wrote:
>> On Fri, 2018-07-06 at 15:12 +1200, joeg--- via samba-technical wrote:
>>> Hi all:
>>>
>>> I created a patch to improve the performance for samba-tool ntacl
>>> sysvolreset cmd.
>>>
>>> The `sysvolreset` cmd was slow with large amount of files.
>>>
>>> With perf tool, we figure out the bottle neck is in
>>> `user_sid_in_group_sid` which is trying to create user token from sid
>>> all the time while we call `setntacl`.
>>>
>>> To avoid that, we pass a `session_info` parameter all the way down to
>>> the related functions, which has the `security_token` with it. And then
>>> in `smbd/posix_acls.c:uid_entry_in_group`, we check with the
>>> security_token if exist.
>>>
>>> In our testenv, with 2k files, time for sysvolreset cmd decrease from
>>> about 155s to 80s.
>>>
>>> A Merge Request is also created on GitLab:
>>>
>>> https://gitlab.com/samba-team/samba/merge_requests/17
>>
>> This looks great. 
>>
>> I've pushed it back to that merge request with my review and fixes to a
>> couple of comments/whitespace.  The previous CI (successful) is here:
>>
>> https://gitlab.com/catalyst-samba/samba/pipelines/25235335
>>
>> Reviewed-by: Andrew Bartlett <abartlet at samba.org>
>>
>> sysvolreset time is a major pain point for users of the AD DC, so
>> getting this in for 4.9 would be great.
>>
>> Can I get a second team reviewer?
>>
>> Thanks,
>>
>> Andrew Bartlett
>>
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20180710/f0759152/signature.sig>