Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC manager: Add deferred parsing of mountinfo #29821

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/core/manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,7 @@ struct Manager {
/* Data specific to the mount subsystem */
struct libmnt_monitor *mount_monitor;
sd_event_source *mount_event_source;
sd_event_source *mount_enumerate_source;
yuwata marked this conversation as resolved.
Show resolved Hide resolved

/* Data specific to the swap filesystem */
FILE *proc_swaps;
Expand Down
67 changes: 62 additions & 5 deletions src/core/mount.c
Original file line number Diff line number Diff line change
Expand Up @@ -1957,9 +1957,27 @@ static int mount_load_proc_self_mountinfo(Manager *m, bool set_flags) {
return 0;
}

static int mount_enumerate_late(sd_event_source *s, void *userdata) {
Manager *m = ASSERT_PTR(userdata);
int r;

/* Do not delay manager instance start with long mountinfo parsing */
if (!m->ready_sent)
return 0;

r = mount_load_proc_self_mountinfo(m, /* set_flags = */ false);
if (r < 0)
return log_error_errno(r, "Failed to load mountinfo: %m");
r = sd_event_source_set_enabled(s, SD_EVENT_OFF);
if (r < 0)
return log_error_errno(r, "Failed to disable late event source: %m");
return r;
}

static void mount_shutdown(Manager *m) {
assert(m);

m->mount_enumerate_source = sd_event_source_disable_unref(m->mount_enumerate_source);
m->mount_event_source = sd_event_source_disable_unref(m->mount_event_source);

mnt_unref_monitor(m->mount_monitor);
Expand Down Expand Up @@ -2045,7 +2063,9 @@ static void mount_enumerate(Manager *m) {
mnt_init_debug(0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this 2nd commit should really be a separate PR. it should be fine to merge independently.


if (!m->mount_monitor) {
usec_t mount_rate_limit_interval = 1 * USEC_PER_SEC;
unsigned mount_rate_limit_burst = 5;
const char *e;
int fd;

m->mount_monitor = mnt_new_monitor();
Expand Down Expand Up @@ -2086,14 +2106,22 @@ static void mount_enumerate(Manager *m) {
}

/* Let users override the default (5 in 1s), as it stalls the boot sequence on busy systems. */
const char *e = secure_getenv("SYSTEMD_DEFAULT_MOUNT_RATE_LIMIT_BURST");
e = secure_getenv("SYSTEMD_DEFAULT_MOUNT_RATE_LIMIT_BURST");
if (e) {
r = safe_atou(e, &mount_rate_limit_burst);
if (r < 0)
log_debug("Invalid value in $SYSTEMD_DEFAULT_MOUNT_RATE_LIMIT_BURST, ignoring: %s", e);
}

r = sd_event_source_set_ratelimit(m->mount_event_source, 1 * USEC_PER_SEC, mount_rate_limit_burst);
e = secure_getenv("SYSTEMD_DEFAULT_MOUNT_RATE_LIMIT_INTERVAL");
if (e) {
r = parse_sec(e, &mount_rate_limit_interval);
if (r < 0)
log_debug("Invalid value in $SYSTEMD_DEFAULT_MOUNT_RATE_LIMIT_INTERVAL, ignoring: %s", e);
}


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spurious double empty line

r = sd_event_source_set_ratelimit(m->mount_event_source, mount_rate_limit_interval, mount_rate_limit_burst);
if (r < 0) {
log_error_errno(r, "Failed to enable rate limit for mount events: %m");
goto fail;
Expand All @@ -2106,11 +2134,40 @@ static void mount_enumerate(Manager *m) {
}

(void) sd_event_source_set_description(m->mount_event_source, "mount-monitor-dispatch");

if (!MANAGER_IS_SYSTEM(m)) {
/* Keep parsing of large mountinfos out of the way of user managers. We postpone
* mounts enumeration after the manager is ready (see user_manager_send_ready()).
* An arrival of a mountinfo event during start could trigger full mountinfo parsing
* too. As a secondary measure, apply ratelimit preemptively. If we manage to
* initialize manager under mount_rate_limit_interval, we're gold. If we are not, the
* ratelimit yields and that resolves events that would block the initialization.
* (Additionally, if the initialization naturally takes longer than
* mount_rate_limit_interval, we can spend yet more time on full parsing.) */
r = sd_event_source_enter_ratelimit(m->mount_event_source);
if (r < 0) {
log_debug_errno(r, "Failed to enter rate limit for mount events: %m");
}

r = sd_event_add_post(m->event, &m->mount_enumerate_source, mount_enumerate_late, m);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there should be an explanation here that explains the why

if (r < 0) {
log_error_errno(r, "Failed to add late mount enumerate source: %m");
goto fail;
}

r = sd_event_source_set_priority(m->mount_enumerate_source, SD_EVENT_PRIORITY_IDLE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idle seems very low for this. also, please use the EVENT_PRIORITY_XYZ enum that now is add the end of manager.h

if (r < 0) {
log_error_errno(r, "Failed to adjust mount enumerate priority: %m");
goto fail;
}
}
}

r = mount_load_proc_self_mountinfo(m, false);
if (r < 0)
goto fail;
if (MANAGER_IS_SYSTEM(m)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this conditionalization sounds really weird to me. if this is safe to do, why wouldn't this be safe for the system too?

r = mount_load_proc_self_mountinfo(m, /* set_flags = */ false);
if (r < 0)
goto fail;
}

return;

Expand Down
5 changes: 5 additions & 0 deletions src/libsystemd/libsystemd.sym
Original file line number Diff line number Diff line change
Expand Up @@ -834,3 +834,8 @@ global:
sd_id128_get_app_specific;
sd_device_enumerator_add_match_property_required;
} LIBSYSTEMD_254;

LIBSYSTEMD_256 {
global:
sd_event_source_enter_ratelimit;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also needs man page addition

} LIBSYSTEMD_255;
32 changes: 29 additions & 3 deletions src/libsystemd/sd-event/sd-event.c
Original file line number Diff line number Diff line change
Expand Up @@ -3343,7 +3343,7 @@ _public_ void *sd_event_source_set_userdata(sd_event_source *s, void *userdata)
return ret;
}

static int event_source_enter_ratelimited(sd_event_source *s) {
static int event_source_enter_ratelimited(sd_event_source *s, bool exceeded) {
int r;

assert(s);
Expand All @@ -3354,6 +3354,10 @@ static int event_source_enter_ratelimited(sd_event_source *s) {
if (s->ratelimited)
return 0; /* Already ratelimited, this is a NOP hence */

if (!exceeded) {
s->rate_limit.begin = now(CLOCK_MONOTONIC);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

coding style, no {} arund single line if block

also, why not just do this in sd_event_source_enter_ratelimit(), i.e. in the caller?


/* Make sure we can install a CLOCK_MONOTONIC event further down. */
r = setup_clock_data(s->event, &s->event->monotonic, CLOCK_MONOTONIC);
if (r < 0)
Expand All @@ -3379,7 +3383,8 @@ static int event_source_enter_ratelimited(sd_event_source *s) {

event_source_pp_prioq_reshuffle(s);

log_debug("Event source %p (%s) entered rate limit state.", s, strna(s->description));
if (exceeded)
log_debug("Event source %p (%s) entered rate limit state.", s, strna(s->description));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why conditionalize this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's to distinguish source limiting based on real even rate vs artificial via explicit call.

return 0;

fail:
Expand Down Expand Up @@ -4135,7 +4140,7 @@ static int source_dispatch(sd_event_source *s) {
/* Check if we hit the ratelimit for this event source, and if so, let's disable it. */
assert(!s->ratelimited);
if (!ratelimit_below(&s->rate_limit)) {
r = event_source_enter_ratelimited(s);
r = event_source_enter_ratelimited(s, /* exceeded = */ true);
if (r < 0)
return r;

Expand Down Expand Up @@ -5214,6 +5219,27 @@ _public_ int sd_event_source_leave_ratelimit(sd_event_source *s) {
return 1; /* tell caller that we indeed just left the ratelimit state */
}

_public_ int sd_event_source_enter_ratelimit(sd_event_source *s) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe call this "force_ratelimit"?

int r;

assert_return(s, -EINVAL);

if (!EVENT_SOURCE_CAN_RATE_LIMIT(s->type))
return -EINVAL;

if (!ratelimit_configured(&s->rate_limit))
return 0;

if (s->ratelimited)
return 0;

r = event_source_enter_ratelimited(s, /* exceeded = */ false);
if (r < 0)
return r;

return 1; /* tell caller that we indeed just entered the ratelimit state */
}

_public_ int sd_event_set_signal_exit(sd_event *e, int b) {
bool change = false;
int r;
Expand Down
1 change: 1 addition & 0 deletions src/systemd/sd-event.h
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ int sd_event_source_get_ratelimit(sd_event_source *s, uint64_t *ret_interval_use
int sd_event_source_is_ratelimited(sd_event_source *s);
int sd_event_source_set_ratelimit_expire_callback(sd_event_source *s, sd_event_handler_t callback);
int sd_event_source_leave_ratelimit(sd_event_source *s);
int sd_event_source_enter_ratelimit(sd_event_source *s);

int sd_event_trim_memory(void);

Expand Down