Page MenuHomeFreeBSD

Events for mount, remount and umount in devdAdd remount vfs event, publish mount, remount and unmount events.
AbandonedPublic

Authored by imp on Aug 5 2020, 10:29 PM.

Details

Reviewers
chs
mckusick
kib
Summary

Some function had the blank lines, others didn't. Most of the ones that didn't were newer, so remove this now-optional blank line everywhere.

All the other printf() calls cast to (void) here, do the two newer ones for consistency.

Add VFS FS events for mount and unmount to devctl/devd

Report when a filesystem is mounted or unmounted via devd, along with details
about the mount point and mount options.

Sponsored by: Netflix

Add a remounted vfs event.

Hook up remounted vfs event to devd

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 32788
Build 30219: arc lint + arc unit

Event Timeline

imp requested review of this revision.Aug 5 2020, 10:29 PM
imp created this revision.
imp retitled this revision from Events for mount, remount and umount in devd Add remount vfs event, publish mount, remount and unmount events. to Events for mount, remount and umount in devdAdd remount vfs event, publish mount, remount and unmount events..Aug 5 2020, 10:31 PM
imp added reviewers: chs, mckusick, kib.
sys/kern/subr_bus.c
5935

Are spaces problematic for devd notifications consumers, typically shell scripts ?

5947

Does it make sense to decode 'reload' flag ?

5987

Both mount and unmount events are invoked while some vnodes are locked. sbuf_new_auto() seems to mean M_WAITOK allocations for the sbuf data by default. Waitable memory allocations while a vnode is locked can cause deadlock if the only way for pagedaemon to make the progress is to flush the locked vnode.

6000

!= 0

6011

I suggest to move all code to calculate sbuf strings from struct mount into kern/vfs_mount.c.
Even if functions like devctl_safe_quite_sb() would need to be exported, or better moved to kern/subr_sbuf.c under more generic name.

sys/kern/subr_bus.c
5987

Gotcha. I went back and forth. I'll do a NOWAIT malloc for a buffer too big for the stack and just omit the message if that fails (which should be quite rare). That will avoid the deadlock.

6011

Like the whole thing? Or just the stuff that does flags and options?

sys/kern/subr_bus.c
5987

/dev/devctl uses nowait malloc anyway, AFAIR ?

6011

I would say, move the whole thing. Then it is questionable why do you want to register eventhandlers instead of doing direct calls to the functions.

sys/kern/subr_bus.c
5987

Yes. It does.

6011

OK. I've moved the whole thing, but need to tweak one or two more things before reposting the review. I'll do that tomorrow.

Move everything to vfs_mount.c and simplify, per suggestions from kib@. Also do
a nowait alloc of the buffer, even though it might not strictly be needed after
the shuffle.

I didn't rename / make more general devctl_safe_quote_sb.
I didn't take the suggestion to export the bit after the MNT_ for the mount bits, but used mount's names instead.

sbin/mount/mount.c
95

This needs a smidge more work... I missed it focusing on the kernel.

sys/kern/subr_bus.c
45

Hmm, stray now and can be removed.

5947

I was unsure. What do you think?

sys/kern/vfs_mount.c
104

Not sure this is the right name to use, so would be interested in feedback.

Tweak mount to compile :)

imp marked 7 inline comments as done.Aug 7 2020, 6:51 PM
sbin/mount/mount.c
124

static const

sys/kern/vfs_mount.c
44

Perhaps devd events stuff could be split in dedicated header, as a followup.

1840

coveredvp is still locked there.

That said, I am not sure that it is reasonable to report mount options for unmount event.

2456

I suggest mount_devd_event()

This change will provide the interface that we will need to notify daemon processes for our forcible unmount and forcible downgrade to read-only as well as re-establishment of read-write after a successful background fsck.

This revision is now accepted and ready to land.Aug 8 2020, 4:19 AM

Update devd.conf.5 perhaps?

sys/kern/vfs_mount.c
1840

Won't they communicate how the system was last mounted?

2456

elsewhere we use 'devctl' rather than 'devd' so I'll use mount_devctl_event

Fold in feedback from reviewers

This revision now requires review to proceed.Aug 18 2020, 10:31 PM

All commits this time, eh?

cddl/compat/opensolaris/misc/zmount.c
50 ↗(On Diff #75981)

this is holding until after we land OpenZFS and won' tbe in this commit.

sys/amd64/conf/GENERIC
74 ↗(On Diff #75981)

this won't be in the commit.

sys/kern/vfs_mount.c
1840

Is it useful ? Also the operation of unmounting could mangle options in the process. From very quick view, for instance UFS clears MNT_LOCAL. Why ? I do not know.

2465

buf cannot be null.

sys/kern/vfs_mount.c
1840

Ah, fair point. I'll create a followup.

2465

I'd intended to use M_NOWAIT...

sys/kern/vfs_mount.c
1840

I added to umount, I just realized, to allow us to see if the umount was forced or not, which I know now isn't cleared... should we have a different mechanism to communicate that?

What is the status with this change?

sys/kern/vfs_mount.c
1840

I think that this communication mechanism is fine.

What is the status with this change?

I've committed this, but there were problems with it that I was too impatient to wait around to discover in the review :(. So it's there, mostly working but needs followup work which I have on my plate and mostly in review (well, the precursors for it).

A new review? If so, I suggest that you close this one and give a pointer to the new one.

A new review? If so, I suggest that you close this one and give a pointer to the new one.

I'll close this review and add you to the new one when I open it.