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

Set systemd-udevd monitor buffer size to 128MB #11389

Merged
merged 1 commit into from Jan 11, 2019

Conversation

2 participants
@yuwata
Copy link
Member

yuwata commented Jan 11, 2019

For #11314.

Two reporters conclude e39b4d2 causes the issue.
I hope that this effectively revert some changes done by the commit.

@keszybz

This comment has been minimized.

Copy link
Member

keszybz commented Jan 11, 2019

@yuwata There are multiple confirmations that this fixes the issue. So maybe let's merge?

@yuwata

This comment has been minimized.

Copy link
Member

yuwata commented Jan 11, 2019

I will update the commit message, and drop the second commit.

@yuwata yuwata added this to the v241 milestone Jan 11, 2019

@yuwata yuwata force-pushed the yuwata:sd-device-monitor-vs-fork branch from 03cc80e to 08908ca Jan 11, 2019

@yuwata yuwata removed the dont-merge 💣 label Jan 11, 2019

@yuwata

This comment has been minimized.

Copy link
Member

yuwata commented Jan 11, 2019

Updated.

  • Add a short comment in the code, and update commit message.
  • The second commit yuwata@03cc80e is dropped.

@yuwata yuwata changed the title [WIP] sd-device,udev: revert several changes in v240 sd-device,udev: revert several changes in v240 Jan 11, 2019

@keszybz

This comment has been minimized.

Copy link
Member

keszybz commented Jan 11, 2019

Heh, that's a tricky one. The code paths for SO_RCVBUF and SO_RCVBUFFORCE are aaaaalmost the same, but not quite. SO_RCVBUF is written to never fail, just silently limit the request to sysctl_rmem_max. Since we are running as root, in the first case, before the patch, the buffer size is set to sysctl_rmem_max, and in the second case, after the patch, the buffer size is set to the requested value (128 MB).

So now we need to figure out why the buffer size makes such a difference.

@keszybz keszybz changed the title sd-device,udev: revert several changes in v240 Set systemd-udevd monitor buffer size to 128MB Jan 11, 2019

@yuwata

This comment has been minimized.

Copy link
Member

yuwata commented Jan 11, 2019

My laptop shows the following:

$ sysctl net.core.rmem_max
net.core.rmem_max = 212992

So, rmem_max seems to be too small for receiving uevents. I guess that the verbose option -v for udevadm trigger makes triggering uevents slower, and then udevd can handle all uevents with smaller buffer.

@keszybz

This comment has been minimized.

Copy link
Member

keszybz commented Jan 11, 2019

Yes.

This change affects a few components: systemd-udevd, udevadm, systemd. But, IIRC, some people mentioned that just copying systemd-udevd is enough to fix the issue. So it seems that the effect on systemd-udevd is what makes a difference.

@yuwata

This comment has been minimized.

Copy link
Member

yuwata commented Jan 11, 2019

This change affects a few components: systemd-udevd, udevadm, systemd. But, IIRC, some people mentioned that just copying systemd-udevd is enough to fix the issue. So it seems that the effect on systemd-udevd is what makes a difference.

Maybe -Dlink-udev-shared=false is related.
https://salsa.debian.org/systemd-team/systemd/blob/master/debian/rules#L68

@yuwata

This comment has been minimized.

Copy link
Member

yuwata commented Jan 11, 2019

Oh,,, I noticed that SO_RCVBUFFORCE should be first, then if it fails, SO_RCVBUF should be used. But, my code was in the inverse order... Ugh.

sd-device-monitor: fix ordering of setting buffer size
By b1c097a (#10239), the receive buffer
size for uevents was set by SO_RCVBUF at first, and fallback to
use SO_RCVBUFFORCE. So, as SO_RCVBUF limits to the buffer size
net.core.rmem_max, which is usually much smaller than 128MB udevd requests,
uevents buffer size was not sufficient.

This fixes the ordering of the request: SO_RCVBUFFORCE first, and
fallback to SO_RCVBUF. Then, udevd's uevent buffer size can be set to
128MB.

This also revert 9038932.

Fixes #11314 and #10754.

@yuwata yuwata force-pushed the yuwata:sd-device-monitor-vs-fork branch from 08908ca to 98aed1d Jan 11, 2019

@yuwata

This comment has been minimized.

Copy link
Member

yuwata commented Jan 11, 2019

Updated.

  • fix the ordering of SO_RCVBUF and SO_RCVBUFFORCE.
  • drop a workaround by 9038932.

So, it seems that the issue #11314 is a variant of #10754. The commit 9038932 fixes the issue when the socket is initialized by pid1, but not fixed when udevd is started by sysv init.

@keszybz

This comment has been minimized.

Copy link
Member

keszybz commented Jan 11, 2019

Right, I think we now understand the cause of failure. This patch should fix it.

That said, I think udevd works mostly by chance. We're setting an absurdly high buffer size because if we ever miss any events, we are completely screwed. But if we ever were on a system which had little RAM and and lots of devices (e.g. something like NAS), we'd be in trouble. One way to work around this would be to provide a feedback channel from udev to udevadm that is doing udevadm trigger so that triggering would be slowed down if udev has trouble keeping up with the events. But that's a bigger change, for later.

@keszybz

This comment has been minimized.

Copy link
Member

keszybz commented Jan 11, 2019

Ooops, I tried to push this by hand, and got

To github.com:systemd/systemd
 ! [remote rejected]       master -> master (cannot lock ref 'refs/heads/master': is at 28daa1d10d6dd30ea62a154e4b2adc93d955edda but expected 0192cbdb2c19ed2abbf6662090f45ab3bf4caf56)
error: failed to push some refs to 'git@github.com:systemd/systemd'

@keszybz keszybz merged commit ee0b9e7 into systemd:master Jan 11, 2019

3 of 8 checks passed

CentOS CI Build started for merge commit.
Details
LGTM analysis: C/C++ Running analyses for revisions
Details
bionic-amd64 autopkgtest running
Details
bionic-i386 autopkgtest running
Details
bionic-s390x autopkgtest running
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@yuwata yuwata deleted the yuwata:sd-device-monitor-vs-fork branch Jan 11, 2019

@yuwata

This comment has been minimized.

Copy link
Member

yuwata commented Jan 11, 2019

I merged #11400 at almost same time. So, github may be confused. The git tree seems fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment