Bug 48177 - performance of accounts-daemon is very poor
Summary: performance of accounts-daemon is very poor
Status: RESOLVED MOVED
Alias: None
Product: accountsservice
Classification: Unclassified
Component: general (show other bugs)
Version: unspecified
Hardware: All Linux (All)
: medium normal
Assignee: Matthias Clasen
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-02 02:29 UTC by Vadim Rutkovsky
Modified: 2018-08-07 09:33 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments
proposed patch (7.37 KB, patch)
2012-08-22 09:12 UTC, Ritesh Khadgaray
Details | Splinter Review
proposed patch (8.57 KB, patch)
2012-08-27 10:43 UTC, Ritesh Khadgaray
Details | Splinter Review
patch (20.60 KB, patch)
2013-01-28 10:01 UTC, Ritesh Khadgaray
Details | Splinter Review
patch (12.59 KB, patch)
2013-02-04 14:40 UTC, Ritesh Khadgaray
Details | Splinter Review
user: check if user is in wheel more efficiently (3.35 KB, patch)
2016-06-30 14:18 UTC, Ray Strode [halfline]
Details | Splinter Review
daemon: get local users from /etc/shadow not /etc/passwd (6.65 KB, patch)
2016-06-30 14:18 UTC, Ray Strode [halfline]
Details | Splinter Review
daemon: don't call getspnam for local users (32.19 KB, patch)
2016-06-30 14:18 UTC, Ray Strode [halfline]
Details | Splinter Review
daemon: constrain max local users to 50 (5.21 KB, patch)
2016-06-30 14:18 UTC, Ray Strode [halfline]
Details | Splinter Review
daemon: don't source user list from wtmp (13.43 KB, patch)
2016-06-30 14:18 UTC, Ray Strode [halfline]
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Vadim Rutkovsky 2012-04-02 02:29:06 UTC
Original report: https://bugs.launchpad.net/bugs/941673

The performance of accounts-daemon is unacceptable with large password and/or group files. We have appoximately 40000 users in the passwords file and 16000 lines in the group file. Having looked at the source, it appears to me the problem is that for each user is pulls from the password database (via getpwXXX)  its then calls getgrouplist to return the users list of groups.

Getgroups must make a complete pass through the group file to determine the groups for the user. When initializing this results in
reading the groups file 40000 times.

I think a better solution is to build a inverted group file map then first time getgroups is called by reading the entire group database and building a data structure indexed by username where a users group list can by found in a single lookup.
Of couse, an even better solution would be for getgroups to do that itself, but that would require much larger code changes.

I tried installed nscd to see if this would make a difference but no
dice.
Comment 1 Ray Strode [halfline] 2012-04-02 11:43:37 UTC
AccountsService sort of assumes users in /etc/passwd are "local users"  and that the 40000 user network login cases are covered by nis/ldap/etc

I guess we should do some sort of sanity check and treat /etc/passwd users as remote in the case the check fails.  This way we sidestep all the costly work entirely.
Comment 2 Ray Strode [halfline] 2012-04-02 11:44:29 UTC
maybe we could just bail if there are greater than N found users.  The problem then is finding a suitable value of N.
Comment 3 Ritesh Khadgaray 2012-08-22 09:12:28 UTC
Created attachment 65941 [details] [review]
proposed patch

limit the no of user listed by a-s to UID_MAX. Also, sets user-limit to true when this condition is meet, allowing apps like gdm/lightdm to enable manual login option.
Comment 4 Ray Strode [halfline] 2012-08-22 14:33:48 UTC
Talked with Ritesh about this bug today on IRC.  He's going to look into using fgetgrent to create the hash table of local groups and use that.
Comment 5 Ritesh Khadgaray 2012-08-27 10:43:46 UTC
Created attachment 66171 [details] [review]
proposed patch
Comment 6 Ray Strode [halfline] 2012-09-25 00:50:30 UTC
ritz, any chance you could get this applying against master (preferably as a git-format-patch formatted patch) ?
Comment 7 Ray Strode [halfline] 2013-01-04 16:20:15 UTC
any updates on this?
Comment 8 Ritesh Khadgaray 2013-01-28 10:01:51 UTC
Created attachment 73763 [details] [review]
patch

Hi halfline

  I have rewritten the code to use hashtable instead of list, leading to lot faster startup time on my test system ( <1s for ~40k users). I am currently testing the code ( add/remove users option ). This is for an older version of a-s (0.6.15) , will update the code against head and post. 

  Anything which does need the user list ( gnome-control-center user manager/ indicator-session/..) still breaks . 

  Personally I believe, the best solution would be to allow user to select an upper limit ( and list only cached users from login history ), and broadcast "user-limit-reached" option. The broadcast could be used by applications to display warning or change their behaviour ( such as disable user list in gdm ).
Comment 9 Ritesh Khadgaray 2013-02-04 14:40:14 UTC
Created attachment 74177 [details] [review]
patch

initial patch, against latest release.

still need to work quirk against entry_generator_wtmp. It calls getpwnam for every entry ( even if it is a duplicate entry ).
Comment 10 Ray Strode [halfline] 2013-10-15 20:24:56 UTC
any chance you could update this again for git master?
Comment 11 Ray Strode [halfline] 2016-06-30 14:18:05 UTC
Created attachment 124799 [details] [review]
user: check if user is in wheel more efficiently

We currently get all the groups a user belongs to in one pass,
then check each one to see if it's wheel.

It's much more efficient to just get the wheel group and check if
any of its members are the user.
Comment 12 Ray Strode [halfline] 2016-06-30 14:18:08 UTC
Created attachment 124800 [details] [review]
daemon: get local users from /etc/shadow not /etc/passwd

For some sites, it's common practice to rsync around large
/etc/passwd files containing the password entries for remote
users. That means accountsservices' "assume /etc/passwd is local
users" heuristic falls over.

This commit changes it to only treat users in /etc/shadow as local.
Comment 13 Ray Strode [halfline] 2016-06-30 14:18:11 UTC
Created attachment 124801 [details] [review]
daemon: don't call getspnam for local users

We're already iterating over the whole shadow file, so
just cache the entries instead of calling getspname a
few lines later.
Comment 14 Ray Strode [halfline] 2016-06-30 14:18:13 UTC
Created attachment 124802 [details] [review]
daemon: constrain max local users to 50

Systems with tens of thousands of users don't want all those users
showing up in the user list.

Set a cap at an even 50, which should cover the lion's share of use
cases well. Of course, if a user not in the list explicitly
logs in (from Not Listed? or whatever) they get added to the list.
Comment 15 Ray Strode [halfline] 2016-06-30 14:18:16 UTC
Created attachment 124803 [details] [review]
daemon: don't source user list from wtmp

wtmp can get rather large on some systems from ssh logins.
Furthermore it's pretty much completely redundant given the user
cache in /var/lib/AccountService

This commit changes the wtmp code to only get used for maintaining
login frequency and accounting, not for generating new users.
Comment 16 Ray Strode [halfline] 2016-06-30 14:23:53 UTC
i'm going to push these patches to master.  They're loosely based on ritz work in this bug, but they diverge in some significant ways:

1) i set a limit on the number of local users returned (ala comment 2)
2) i redid the wheel check to avoid needing an admin user cache
3) i cache shadow information a little differently and for a shorter period of time
4) i get "local user list" from /etc/shadow now instead of /etc/passwd (since /etc/passwd gets rsynced around)
Comment 17 Ray Strode [halfline] 2016-06-30 14:24:28 UTC
Attachment 124799 [details] pushed as c85b41d - user: check if user is in wheel more efficiently
Attachment 124800 [details] pushed as 98f4287 - daemon: get local users from /etc/shadow not /etc/passwd
Attachment 124801 [details] pushed as 14ca424 - daemon: don't call getspnam for local users
Attachment 124802 [details] pushed as 20c0315 - daemon: constrain max local users to 50
Attachment 124803 [details] pushed as a2f448b - daemon: don't source user list from wtmp
Comment 18 Ting-Wei Lan 2016-07-13 18:38:57 UTC
Unconditioned #include <shadow.h> breaks the build on FreeBSD.

FreeBSD doesn't have shadow.h and /etc/shadow. Hashed passwords are stored in /etc/master.passwd, which is a /etc/passwd with additional fields. There is no API specifically made to read /etc/master.passwd because getpwent on FreeBSD automatically switches to use /etc/spwd.db (the binary form of /etc/master.passwd) instead of /etc/pwd.db (the binary form of /etc/passwd) when the user has permission to read the file.
Comment 19 Antoine Jacoutot 2016-11-03 10:32:43 UTC
Same on OpenBSD. Since that change, accountsservice is broken here as well.
Comment 20 Ray Strode [halfline] 2016-11-03 13:47:13 UTC
is it common practice on BSD to rsync /etc/passwd around? Maybe bsd could go back to using /etc/passwd?
Comment 21 GitLab Migration User 2018-08-07 09:33:28 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/accountsservice/accountsservice/issues/39.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.