14517 bhyve EVF_VNODE mevent on plain file fires on every data change

Reviewed by: Jason King <jason.brian.king+illumos@gmail.com>
Reviewed by: Patrick Mooney <pmooney@pfmooney.com>
Approved by: Dan McDonald <danmcd@joyent.com>

4 files changed, 52 insertions, 120 deletions

diff --git a/usr/src/cmd/bhyve/Makefile b/usr/src/cmd/bhyve/Makefile
index eac7311d30..6394241ca9 100644
--- a/usr/src/cmd/bhyve/Makefile
+++ b/usr/src/cmd/bhyve/Makefile
@@ -152,13 +152,12 @@ $(PROG) := LDLIBS += \
 	-ldladm \
 	-lmd \
 	-lnvpair \
-	-lproc \
 	-lcrypto \
 	-luuid \
 	-lvmmapi \
 	-lz
 NATIVE_LIBS += libz.so libcrypto.so
-$(MEVENT_TEST_PROG) := LDLIBS += -lsocket -lproc
+$(MEVENT_TEST_PROG) := LDLIBS += -lsocket
 $(PROG) := LDFLAGS += $(ZASLR)
 
 .KEEP_STATE:
diff --git a/usr/src/cmd/bhyve/mevent.c b/usr/src/cmd/bhyve/mevent.c
index 09f39ed06b..29f7d7fcfc 100644
--- a/usr/src/cmd/bhyve/mevent.c
+++ b/usr/src/cmd/bhyve/mevent.c
@@ -66,7 +66,7 @@ __FBSDID("$FreeBSD$");
 #include <sys/siginfo.h>
 #include <sys/queue.h>
 #include <sys/debug.h>
-#include <libproc.h>
+#include <sys/stat.h>
 #endif
 #include <sys/time.h>
 
@@ -114,8 +114,6 @@ struct mevent {
 	port_notify_t	me_notify;
 	struct sigevent	me_sigev;
 	boolean_t	me_auto_requeue;
-	struct file_obj	me_fobj;
-	char		*me_fname;
 	struct {
 		int	mp_fd;
 		off_t	mp_size;
@@ -356,41 +354,6 @@ mevent_clarify_state(struct mevent *mevp)
 	return (B_TRUE);
 }
 
-static char *
-mevent_fdpath(int fd)
-{
-	prfdinfo_t *fdinfo;
-	char *path;
-	size_t len;
-
-	fdinfo = proc_get_fdinfo(getpid(), fd);
-	if (fdinfo == NULL) {
-		(void) fprintf(stderr, "%s: proc_get_fdinfo(%d) failed: %s\n",
-		    __func__, fd, strerror(errno));
-		path = NULL;
-	} else {
-		path = (char *)proc_fdinfo_misc(fdinfo, PR_PATHNAME, &len);
-	}
-
-	if (path == NULL) {
-		(void) fprintf(stderr, "%s: Fall back to /proc/self/fd/%d\n",
-		    __func__, fd);
-		(void) asprintf(&path, "/proc/self/fd/%d", fd);
-	} else {
-		path = strdup(path);
-	}
-
-	proc_fdinfo_free(fdinfo);
-
-	if (path == NULL) {
-		(void) fprintf(stderr,
-		    "%s: Error building path for fd %d: %s\n", __func__,
-		    fd, strerror(errno));
-	}
-
-	return (path);
-}
-
 static void
 mevent_poll_file_attrib(int fd, enum ev_type type, void *param)
 {
@@ -398,18 +361,17 @@ mevent_poll_file_attrib(int fd, enum ev_type type, void *param)
 	struct stat st;
 
 	if (fstat(mevp->me_poll.mp_fd, &st) != 0) {
-		(void) fprintf(stderr, "%s: fstat(%d) \"%s\" failed: %s\n",
-		    __func__, fd, mevp->me_fname, strerror(errno));
+		(void) fprintf(stderr, "%s: fstat(%d) failed: %s\n",
+		    __func__, fd, strerror(errno));
 		return;
 	}
 
-	if (mevp->me_poll.mp_size != st.st_size ||
-	    mevp->me_fobj.fo_ctime.tv_sec != st.st_ctim.tv_sec ||
-	    mevp->me_fobj.fo_ctime.tv_nsec != st.st_ctim.tv_nsec) {
+	/*
+	 * The only current consumer of file attribute monitoring is
+	 * blockif, which wants to know about size changes.
+	 */
+	if (mevp->me_poll.mp_size != st.st_size) {
 		mevp->me_poll.mp_size = st.st_size;
-		mevp->me_fobj.fo_atime = st.st_atim;
-		mevp->me_fobj.fo_mtime = st.st_mtim;
-		mevp->me_fobj.fo_ctime = st.st_ctim;
 
 		(*mevp->me_poll.mp_func)(mevp->me_poll.mp_fd, EVF_VNODE,
 		    mevp->me_poll.mp_param);
@@ -508,13 +470,10 @@ mevent_update_one_timer(struct mevent *mevp)
 static void
 mevent_update_one_vnode(struct mevent *mevp)
 {
-	int portfd = mevp->me_notify.portnfy_port;
-
-	mevp->me_auto_requeue = B_FALSE;
-
 	switch (mevp->me_state) {
 	case EV_ENABLE:
 	{
+		struct stat st;
 		int events = 0;
 
 		if ((mevp->me_fflags & EVFF_ATTRIB) != 0)
@@ -522,76 +481,44 @@ mevent_update_one_vnode(struct mevent *mevp)
 
 		assert(events != 0);
 
-		if (mevp->me_fname == NULL) {
-			mevp->me_fname = mevent_fdpath(mevp->me_fd);
-			if (mevp->me_fname == NULL)
-				return;
-		}
-
-		bzero(&mevp->me_fobj, sizeof (mevp->me_fobj));
-		mevp->me_fobj.fo_name = mevp->me_fname;
-
-		if (port_associate(portfd, PORT_SOURCE_FILE,
-		    (uintptr_t)&mevp->me_fobj, events, mevp) != 0) {
-			/*
-			 * If this file does not support event ports
-			 * (e.g. ZVOLs do not yet have support)
-			 * then convert this to a timer event and poll for
-			 * file attribute changes.
-			 */
-			struct stat st;
-
-			if (errno != ENOTSUP) {
-				(void) fprintf(stderr,
-				    "port_associate fd %d (%s) %p failed: %s"
-				    ", polling instead\n",
-				    mevp->me_fd, mevp->me_fname, mevp,
-				    strerror(errno));
-			}
+		/*
+		 * It is tempting to use the PORT_SOURCE_FILE type for this in
+		 * conjunction with the FILE_ATTRIB event type. Unfortunately
+		 * this event type triggers on any change to the file's
+		 * ctime, and therefore for every write as well as attribute
+		 * changes. It also does not work for ZVOLs.
+		 *
+		 * Convert this to a timer event and poll for the file
+		 * attribute changes that we care about.
+		 */
 
-			if (fstat(mevp->me_fd, &st) != 0) {
-				(void) fprintf(stderr,
-				    "fstat(%d) \"%s\" failed: %s\n",
-				    mevp->me_fd, mevp->me_fname,
-				    strerror(errno));
-				return;
-			}
+		if (fstat(mevp->me_fd, &st) != 0) {
+			(void) fprintf(stderr, "fstat(%d) failed: %s\n",
+			    mevp->me_fd, strerror(errno));
+			return;
+		}
 
-			mevp->me_fobj.fo_atime = st.st_atim;
-			mevp->me_fobj.fo_mtime = st.st_mtim;
-			mevp->me_fobj.fo_ctime = st.st_ctim;
+		mevp->me_poll.mp_fd = mevp->me_fd;
+		mevp->me_poll.mp_size = st.st_size;
 
-			mevp->me_poll.mp_fd = mevp->me_fd;
-			mevp->me_poll.mp_size = st.st_size;
+		mevp->me_poll.mp_func = mevp->me_func;
+		mevp->me_poll.mp_param = mevp->me_param;
+		mevp->me_func = mevent_poll_file_attrib;
+		mevp->me_param = mevp;
 
-			mevp->me_poll.mp_func = mevp->me_func;
-			mevp->me_poll.mp_param = mevp->me_param;
-			mevp->me_func = mevent_poll_file_attrib;
-			mevp->me_param = mevp;
+		mevp->me_type = EVF_TIMER;
+		mevp->me_timid = -1;
+		mevp->me_msecs = mevent_file_poll_interval_ms;
+		mevent_update_one_timer(mevp);
 
-			mevp->me_type = EVF_TIMER;
-			mevp->me_timid = -1;
-			mevp->me_msecs = mevent_file_poll_interval_ms;
-			mevent_update_one_timer(mevp);
-		}
 		return;
 	}
 	case EV_DISABLE:
 	case EV_DELETE:
 		/*
-		 * A disable that comes in while an event is being
-		 * handled will result in an ENOENT.
+		 * These events do not really exist as they are converted to
+		 * timers; fall through to abort.
 		 */
-		if (port_dissociate(portfd, PORT_SOURCE_FILE,
-		    (uintptr_t)&mevp->me_fobj) != 0 &&
-		    errno != ENOENT) {
-			(void) fprintf(stderr, "port_dissociate "
-			    "portfd %d fd %d mevp %p failed: %s\n",
-			    portfd, mevp->me_fd, mevp, strerror(errno));
-		}
-		free(mevp->me_fname);
-		mevp->me_fname = NULL;
-		return;
 	default:
 		(void) fprintf(stderr, "%s: unhandled state %d\n", __func__,
 		    mevp->me_state);
@@ -654,7 +581,6 @@ mevent_update_pending()
 		LIST_REMOVE(mevp, me_list);
 
 		if (mevp->me_state & EV_DELETE) {
-			free(mevp->me_fname);
 			free(mevp);
 		} else {
 			LIST_INSERT_HEAD(&global_head, mevp, me_list);
diff --git a/usr/src/cmd/bhyve/test/Makefile.com b/usr/src/cmd/bhyve/test/Makefile.com
index 1d1aa1e630..df86846eff 100644
--- a/usr/src/cmd/bhyve/test/Makefile.com
+++ b/usr/src/cmd/bhyve/test/Makefile.com
@@ -29,8 +29,6 @@ CPPFLAGS =	-I$(COMPAT)/bhyve -I$(CONTRIB)/bhyve \
 		-I$(SRC)/cmd/bhyve \
 		-DWITHOUT_CAPSICUM
 
-LDFLAGS +=	-lproc
-
 SMOFF += all_func_returns
 
 CLOBBERFILES +=	$(PROG)
diff --git a/usr/src/cmd/bhyve/test/tests/mevent/vnode_file.c b/usr/src/cmd/bhyve/test/tests/mevent/vnode_file.c
index 850ac1be27..c3df405edb 100644
--- a/usr/src/cmd/bhyve/test/tests/mevent/vnode_file.c
+++ b/usr/src/cmd/bhyve/test/tests/mevent/vnode_file.c
@@ -29,7 +29,7 @@
 #include "testlib.h"
 #include "mevent.h"
 
-static char *cookie = "Chocolate chip with fudge stripes";
+static char *cookie = "Shortcake";
 
 static pthread_mutex_t mtx = PTHREAD_MUTEX_INITIALIZER;
 static pthread_cond_t cv = PTHREAD_COND_INITIALIZER;
@@ -67,8 +67,6 @@ test_fd(int fd, char *tag)
 	for (uint_t i = 0; cookie[i] != '\0'; i++) {
 		ssize_t written;
 
-		pthread_mutex_lock(&mtx);
-
 		if (i > 0) {
 			/*
 			 * Check that no events are emitted for writes which do
@@ -78,12 +76,21 @@ test_fd(int fd, char *tag)
 				FAIL_ERRNO("lseek");
 			if (write(fd, "X", 1) == -1)
 				FAIL_ERRNO("write");
+			/*
+			 * Allow time for the callback to fire if it is going
+			 * to.
+			 */
+			VERBOSE(("Write within"));
+			usleep(100);
 		}
 
+		pthread_mutex_lock(&mtx);
+
 		written = write(fd, cookie + i, 1);
 		if (written < 0)
 			FAIL_ERRNO("bad write");
 		ASSERT_INT64_EQ(("write byte %d of cookie", i), written, 1);
+		VERBOSE(("Write extend"));
 
 		/* Wait for the size change to be processed */
 		pthread_cond_wait(&cv, &mtx);
@@ -93,7 +100,7 @@ test_fd(int fd, char *tag)
 		 * for mevent to re-associate the port or the next write could
 		 * be missed.
 		 */
-		usleep(500);
+		usleep(100);
 	}
 
 	err = mevent_disable(evp);
@@ -105,10 +112,12 @@ test_fd(int fd, char *tag)
 int
 main(int argc, const char **argv)
 {
-	start_test(argv[0], 5);
-	start_event_thread();
 	int fd;
 
+	start_test(argv[0], 20);
+	set_mevent_file_poll_interval_ms(500);
+	start_event_thread();
+
 	/* Test with a temporary file in /tmp */
 	char *template = strdup("/tmp/mevent.vnode.XXXXXX");
 	ASSERT_PTR_NEQ(("strdup"), template, NULL);