[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 at samba.org
Tue Jul 10 04:49:15 UTC 2018
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)
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.
> 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:
>> 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:
>> 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?
>> Andrew Bartlett
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 488 bytes
Desc: OpenPGP digital signature
More information about the samba-technical