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 33065
Build 30445: 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 ↗(On Diff #75464)

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

5947 ↗(On Diff #75464)

Does it make sense to decode 'reload' flag ?

5987 ↗(On Diff #75464)

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 ↗(On Diff #75464)

!= 0

6011 ↗(On Diff #75464)

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 ↗(On Diff #75464)

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 ↗(On Diff #75464)

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

sys/kern/subr_bus.c
5987 ↗(On Diff #75464)

/dev/devctl uses nowait malloc anyway, AFAIR ?

6011 ↗(On Diff #75464)

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 ↗(On Diff #75464)

Yes. It does.

6011 ↗(On Diff #75464)

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–96

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

sys/kern/subr_bus.c
46 ↗(On Diff #75587)

Hmm, stray now and can be removed.

5947 ↗(On Diff #75464)

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
96–97

static const

sys/kern/vfs_mount.c
44

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

1843

coveredvp is still locked there.

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

2460

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
1843

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

2460

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

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

sys/amd64/conf/GENERIC
74

this won't be in the commit.

sys/kern/vfs_mount.c
1843

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.

2470

buf cannot be null.

sys/kern/vfs_mount.c
1843

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

2470

I'd intended to use M_NOWAIT...

sys/kern/vfs_mount.c
1843

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
1843

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.