Page MenuHomeFreeBSD
Differential D33326

bhyve: reset event after blockif resize
ClosedPublic
Actions

Authored by rew on Dec 7 2021, 8:14 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 23, 9:19 PM
Unknown Object (File)
Wed, Nov 20, 10:48 AM
Unknown Object (File)
Wed, Nov 20, 10:10 AM
Unknown Object (File)
Oct 3 2024, 11:26 AM
Unknown Object (File)
Oct 3 2024, 11:15 AM
Unknown Object (File)
Oct 3 2024, 9:13 AM
Unknown Object (File)
Sep 24 2024, 3:35 AM
Unknown Object (File)
Sep 23 2024, 6:49 PM
View All 64 Files
Subscribers
afedorov
bcran
imp
rgrimes

Details

Reviewers
jhb
Group Reviewers
bhyve
Commits
rGd3cc82c9b7aa: bhyve: set EV_CLEAR for EVFILT_VNODE mevents
rG0b29683b32df: bhyve: set EV_CLEAR for EVFILT_VNODE mevents
Summary

After a resize event is triggered, reset it; this prevents the resize
(NOTE_ATTRIB) event from being triggered more than once.

Currently, the blockif resize functionality is the only user of
mevent_add_flags() - so this change won't affect other mevent users.

I noticed this when bhyve was consuming a 100% of the cpu after a resize
event was triggered.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 43220
Build 40108: arc lint + arc unit

Event Timeline

rew created this revision.Dec 7 2021, 8:14 PM
Herald added a reviewer: bhyve. · View Herald TranscriptDec 7 2021, 8:14 PM
Herald added subscribers: bcran, rgrimes, imp. · View Herald Transcript
rew requested review of this revision.Dec 7 2021, 8:14 PM
Harbormaster completed remote builds in B43220: Diff 99674.Dec 7 2021, 8:14 PM
rew added a reviewer: jhb.Dec 7 2021, 8:14 PM
jhb added a comment.Dec 7 2021, 11:29 PM

I don't know that mevent_add_flags is really the right layer to fix this vs providing some way to explicitly mark an event as EV_CLEAR. It could be that all EVFILT_VNODE should be EV_CLEAR? That is, perhaps have mevent_kq_flags set EV_CLEAR in the flags it returns if me_type == EVF_VNODE. (I looked at the kevent(2) manpage and I do think we would want EV_CLEAR for any EVFILT_VNODE events.)

rew added a comment.Dec 8 2021, 12:57 AM
In D33326#753476, @jhb wrote:

I don't know that mevent_add_flags is really the right layer to fix this

Agreed - blockif_register_resize_callback() could call mevent_add_state(), but then sys/event.h would have to be included in block_if.c

It could be that all EVFILT_VNODE should be EV_CLEAR?

maybe, most uses of EVFILT_VNODE in base would agree with that.

The man page stated that some filters (e.g., EVFILT_SIGNAL and EVFILT_TIMER) automatically set EV_CLEAR, which led me to believe that event filters that do not set EV_CLEAR must have use-cases where the event filter doesn't want to clear the event.

rew added a comment.Dec 8 2021, 6:05 PM

Do you think it would be preferable to set EV_CLEAR in mevent_kq_flags as opposed to using mevent_add_state() directly from blockif_register_resize_callback()?

jhb added a comment.Dec 8 2021, 7:53 PM
In D33326#753714, @rew wrote:

Do you think it would be preferable to set EV_CLEAR in mevent_kq_flags as opposed to using mevent_add_state() directly from blockif_register_resize_callback()?

So in theory mevent is a somewhat portable layer that isn't kevent specific. bhyve on illumos doesn't use kevent but is able to just replace mevent.c. Exposing mevent_add_state would beak that I think, so I would prefer changing mevent_kq_fflags and avoiding changing block_if.c in this case (so that the EV_CLEAR is a property of EVF_VNODE that is provided by mevent)

rew updated this revision to Diff 99774.Dec 9 2021, 7:42 PM

set EV_CLEAR flag when EVFILT_VNODE filter is used

Harbormaster completed remote builds in B43263: Diff 99774.Dec 9 2021, 7:42 PM
jhb added inline comments.Dec 9 2021, 9:03 PM
usr.sbin/bhyve/mevent.c
178

I would perhaps make this stateless (not modifying mevp) and instead do something like:

int state;

state = mevp->me_state;
if (mevp->me_type == EVF_VNODE)
    state |= EV_CLEAR;
return (state);
rew updated this revision to Diff 99797.Dec 9 2021, 9:37 PM

don't modify mevp, just return the translated flag value

use the name retval and format this function to be consistent with the
function above and below it.

Harbormaster completed remote builds in B43267: Diff 99797.Dec 9 2021, 9:37 PM
jhb accepted this revision.Dec 9 2021, 9:47 PM

Thanks for fixing this!

This revision is now accepted and ready to land.Dec 9 2021, 9:47 PM
Closed by commit rG0b29683b32df: bhyve: set EV_CLEAR for EVFILT_VNODE mevents (authored by rew). · Explain WhyDec 12 2021, 9:50 PM
This revision was automatically updated to reflect the committed changes.
rew added a commit: rG0b29683b32df: bhyve: set EV_CLEAR for EVFILT_VNODE mevents.
afedorov added a subscriber: afedorov.Feb 17 2022, 9:23 AM

I have this problem in STABLE-13 with disk image located on NFS share.

This patch fixes the problem.

@rew , are you planning on making an MFC to 13-STABLE?

rew added a commit: rGd3cc82c9b7aa: bhyve: set EV_CLEAR for EVFILT_VNODE mevents.Feb 17 2022, 10:02 PM
rew added a comment.Feb 17 2022, 10:05 PM
In D33326#776604, @afedorov wrote:

I have this problem in STABLE-13 with disk image located on NFS share.

This patch fixes the problem.

@rew , are you planning on making an MFC to 13-STABLE?

done, thanks for the reminder

Revision Contents
Changeset List

PathSize
usr.sbin/
bhyve/
2 lines

Diff 99674

View Options

usr.sbin/bhyve/mevent.c

Loading...