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

device: make sure we don't generate bogus PropertiesChanged signals #15233

Merged
merged 2 commits into from Mar 29, 2020

Conversation

msekletar
Copy link
Contributor

No description provided.

@rmetrich
Copy link
Contributor

Example of test scenario

  1. Set up a system with 200 mount points on a "data" volume group
# vgcreate data /dev/sdb
# for i in $(seq 1 200); do lvcreate -n lv$i -L 10M data; mkfs.ext4 /dev/data/lv$i; echo "/dev/mapper/data-lv$i /data/lv$i ext4 defaults 0 0" >> /etc/fstab; mkdir -p /data/lv$i; done
# systemctl daemon-reload
# mount -a
  1. Monitor systemd's "sendmsg" while performing 100 remounts of /data/lv1

In a terminal:

# timeout 1m strace -ttTvyy -o /tmp/systemd.strace -s 1024 -e sendmsg -p 1

In another terminal:

# for i in $(seq 1 100); do mount -o remount /data/lv1; sleep 0.1; done
  1. Count the number of PropertiesChanged messages sent
# grep -c PropertiesChanged /tmp/systemd.strace

Actual results:

40600

With the patch:

200

Copy link
Member

@keszybz keszybz left a comment

Choose a reason for hiding this comment

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

Looks OK, but the first patch seems unnecessary. Could you check if you still get the same improvement without the first patch?

src/core/unit.c Outdated Show resolved Hide resolved
@keszybz keszybz added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Mar 27, 2020
@keszybz
Copy link
Member

keszybz commented Mar 27, 2020

(I set the flag because I'm 80% sure that the first patch is unnecessary.)

Functions called from device_setup_unit() already make sure that unit is
enqueued in case it is a new unit or properties exported on the bus have
changed.

This should prevent unnecessary DBus wakeups and associated DBus traffic
when device_setup_unit() was called while reparsing /proc/self/mountinfo
due to the mountinfo notifications. Note that we parse
/proc/self/mountinfo quite often on the busy systems (e.g. k8s container
hosts) but majority of the time mounts didn't change, only some mount
got added. Thus we don't need to generate PropertiesChanged for devices
associated with the mounts that didn't change.

Thanks to Renaud Métrich <rmetrich@redhat.com> for debugging the
problem and providing draft version of the patch.
@msekletar
Copy link
Contributor Author

@keszybz I've dropped the unnecessary patch. PTAL.

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

Successfully merging this pull request may close these issues.

None yet

3 participants