After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 679793 - GFileMonitor: Add kqueue(3) support to GIO
GFileMonitor: Add kqueue(3) support to GIO
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gio
2.34.x
Other OpenBSD
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2012-07-12 11:37 UTC by Antoine Jacoutot
Modified: 2013-08-18 01:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add kqueue(3) support to GIO (100.89 KB, patch)
2012-07-12 11:37 UTC, Antoine Jacoutot
none Details | Review
Add kqueue(3) support to GIO, v2 (98.53 KB, patch)
2012-09-17 17:52 UTC, Antoine Jacoutot
reviewed Details | Review
Add kqueue(3) support to GIO, v3 (98.80 KB, patch)
2012-11-11 13:35 UTC, Antoine Jacoutot
none Details | Review
Add kqueue(3) support to GIO, v4 (99.01 KB, patch)
2012-11-14 17:08 UTC, Antoine Jacoutot
committed Details | Review

Description Antoine Jacoutot 2012-07-12 11:37:49 UTC
Created attachment 218615 [details] [review]
Add kqueue(3) support to GIO

Hi.

This patch has been written by Dmitry Matveev as part of GSoC 2011:
http://netbsd-soc.sourceforge.net/projects/kqueue4gio/

It brings native file monitoring support on systems supporting kqueue(3)
(all BSDs) and remove the need to rely on the unmaintained gamin software.
The backend is called GKqueueDirectoryMonitor.

Currently there is a small difference with how this backend works
compared to e.g. the inotify one due to a limitation in kqueue(3):
watching files and directories means opening a FD for each entry which
will render the mount point un-unmountable (i.e. "busy").
For this reason, the backend adds an optionnal configuration file:
/etc/gio-kqueue.conf (and ~/.config/gio-kqueue.conf)
This file contains an exclusion list (one /path/ per line) which will
force using polling over kqueue for monitoring these particular paths
e.g.
/media
/mnt
/run

Another solution would be to force the use of polling for removable
devices if that is prefered.

This patch has been enabled by default in OpenBSD since January and for
about the same amount of time in NetBSD. All "known" bugs have been
fixed as far as I can see and we think it is now time to push this
forward to have a chance to see it included in GLib sources at some
point.

Note that I am opening this ticket on behalf of Dmitry Matveev the
original author, who for personnal reasons will not be able to do any
computer work for the year to come.

I would greatly appreciate some review.
Thanks!
Comment 1 Dan Winship 2012-07-12 12:58:45 UTC
Hey. I'd looked at this back when Dmitry sent it to gtk-devel-list last summer, and I remember that it mostly looked good. I'll try to find a chance to re-review it soon.

In the meantime, you may want to look at bug 532815 and add support for the new flag suggested there. (Assuming this backend has the same problem the inotify one does with hardlinks.)
Comment 2 Colin Walters 2012-07-12 13:11:09 UTC
(In reply to comment #0)
> the backend adds an optionnal configuration file:
> /etc/gio-kqueue.conf (and ~/.config/gio-kqueue.conf)

Gross.  This is an OS-level deficiency; there should be some mechanism for the kernel to force-close the monitors on umount.

Second, in reality I seriously doubt any GNOME-FreeBSD system administrator is going to actually find this file and edit it.  Think about it.  They'll run unmount, hit the "In use" error, then...what?  Somehow know that because the process uses GLib that they should edit the file?
Comment 3 Antoine Jacoutot 2012-07-12 16:19:22 UTC
(In reply to comment #1)
> Hey. I'd looked at this back when Dmitry sent it to gtk-devel-list last summer,
> and I remember that it mostly looked good. I'll try to find a chance to
> re-review it soon.

Cool, thank you.

> In the meantime, you may want to look at bug 532815 and add support for the new
> flag suggested there. (Assuming this backend has the same problem the inotify
> one does with hardlinks.)

Yeah I looked at it but kqueue(2) does watch the files as well; so we don't have any issue with hardlinks.
I made sure by trying the test case in the aforementioned ticket.
Comment 4 Antoine Jacoutot 2012-07-12 16:27:17 UTC
(In reply to comment #2)
> (In reply to comment #0)
> > the backend adds an optionnal configuration file:
> > /etc/gio-kqueue.conf (and ~/.config/gio-kqueue.conf)
> 
> Gross.  This is an OS-level deficiency; there should be some mechanism for the
> kernel to force-close the monitors on umount.

There is none. I think only Mac OS has some kind of a hack to mark file descriptors in a way that they are not busy when used by kqueue(2) or something like that (I have 0 knowledge about Mac OS).

> Second, in reality I seriously doubt any GNOME-FreeBSD system administrator is

Well, so far I have been talking about OpenBSD and NetBSD. I have no idea what the FreeBSD people are after.

> going to actually find this file and edit it.  Think about it.  They'll run

I have _ever_ run into _any_ issue with using that way of doing things. Why? Because the OpenBSD package adds a configuration file that includes the most obvious temporary mount points to the ignore list.
The thing is the people that use GNOME on OpenBSD are either:
* dummy users that don't want to know anything about what is going behind the scenes; this case is covered because hotpluging a USB key will automatically mount on a mount point that is in the ignore list
* power users who know how things are working and are able to edit whatever needs be to comply to their requirements

> unmount, hit the "In use" error, then...what?  Somehow know that because the
> process uses GLib that they should edit the file?

Look, there is no need to be rude here. I actually proposed another way of doing this in my first message: use regular polling for removable devices.
In any case, right-click/umount works without any issue because our hotplug scripts mounts removable devices under a poll-only directory.

I am opened to suggestions to modify the patch according to what GLib people want, I just don't need the OS bashing.
Comment 5 Jasper Lievisse Adriaanse 2012-07-12 16:42:58 UTC
I would have to say I agree with Antoine's statements here. The way the OpenBSD glib package is setup and installed is done in a way that satisfies the dummies and the power users.

I for one have been using this patch for months, without any issues, as have all the other users of GLib on OpenBSD.
Comment 6 Dan Winship 2012-07-12 16:54:04 UTC
(In reply to comment #2)
> Gross.  This is an OS-level deficiency; there should be some mechanism for the
> kernel to force-close the monitors on umount.

Shrug. You could make an equally plausible argument that linux is broken for *not* considering an inotify watch to be an "open file" for purposes of mountpoint busy-ness.


But what is it that's setting up watches on removable devices anyway? Most watches I know of are either in system directories or in $HOME...
Comment 7 Antoine Jacoutot 2012-07-12 16:57:18 UTC
(In reply to comment #6)
> But what is it that's setting up watches on removable devices anyway? Most
> watches I know of are either in system directories or in $HOME...

Indeed, except when it comes to the file manager. e.g. you open /path/to/your/usbkey/mount with Nautilus.

Anyway, if the "only do poll for removable devices" proposal sounds ok to you guys, then I'll fix the patch accordingly.
Comment 8 Colin Walters 2012-07-12 17:02:06 UTC
(In reply to comment #6)

> Shrug. You could make an equally plausible argument that linux is broken for
> *not* considering an inotify watch to be an "open file" for purposes of
> mountpoint busy-ness.

I really struggle to think of a use case where if the system administrator says "unmount", you would want it to be blocked by an application just monitoring the directory.
Comment 9 Dan Winship 2012-07-12 17:06:32 UTC
Yeah, and I struggle to think of a use case where you would say "unmount" and want it to be blocked because you have a shell open in a subdirectory of that mountpoint. But that's what happens.

At any rate, if the problem is "you can't unmount a filesystem from within nautilus when you're looking at that filesystem in nautilus", well, that's easy enough to fix within nautilus; it should just drop its watches before trying to unmount.
Comment 10 Antoine Jacoutot 2012-07-12 17:06:53 UTC
(In reply to comment #8)
> I really struggle to think of a use case where if the system administrator says
> "unmount", you would want it to be blocked by an application just monitoring
> the directory.

No you are right in this case, there is no case. But it's how the kqueue(2) interface works, it requires an fd...
I'm not saying this is clever, it's just the way it is.
Comment 11 Colin Walters 2012-07-12 17:08:36 UTC
(In reply to comment #4)
>
> Look, there is no need to be rude here.

Before we go back into the technical discussion, let me say that I believe blunt criticism is an important part of the peer review process that helps Free Software thrive.   Please don't take it as a personal attack or whatever.  I expect exactly the same from other people when *I* submit patches.
Comment 12 Antoine Jacoutot 2012-07-12 17:12:57 UTC
(In reply to comment #11)
> Before we go back into the technical discussion, let me say that I believe
> blunt criticism is an important part of the peer review process that helps Free
> Software thrive.   Please don't take it as a personal attack or whatever.  I
> expect exactly the same from other people when *I* submit patches.

Fair enough. I have nothing against criticism, on the contrary. It's just that I get my fair amount of regular bashing from both sides (GNOME saying OpenBSD is badly designed or old or whatever; and OpenBSD saying GNOME is bloated and blah blah blah)... so sometimes I just get tired of it because I'm just trying to get things done as I like both projects ;-)
Anyway... back to technical stuff.
Comment 13 Colin Walters 2012-07-12 17:14:22 UTC
(In reply to comment #9)
> Yeah, and I struggle to think of a use case where you would say "unmount" and
> want it to be blocked because you have a shell open in a subdirectory of that
> mountpoint. But that's what happens.

Sure, but that doesn't change the fact that the Linux behavior for inotify is better.

> At any rate, if the problem is "you can't unmount a filesystem from within
> nautilus when you're looking at that filesystem in nautilus", well, that's easy
> enough to fix within nautilus; it should just drop its watches before trying to
> unmount.

Also, there's no reason GFileMonitor couldn't be aware of GDrive too; they both live in Gio.  That would presumably help the Nautilus-on-removable-media case, but admittedly wouldn't fix say a non-GUI app just using GFileMonitor.
Comment 14 Antoine Jacoutot 2012-09-17 17:52:30 UTC
Created attachment 224541 [details] [review]
Add kqueue(3) support to GIO, v2

Hi.

Very sorry about the latency but I have been extremely busy lately...
Anyway, here's a new revision of the gio-kqueue patch with the following
changes to workaround the fact that one cannot unmount a kqueue
monitored directory without using `-f':
* if the path we monitor in under /mnt, then we fallback to polling
* if the mount is g_mount_can_unmount(), then we fallback to polling

I think it is an acceptable solution...
I'd really like to move forward with it so I'd appreciate the input.
Thanks.
Comment 15 Antoine Jacoutot 2012-10-04 14:12:37 UTC
Hi guys.

I would really like some review on this patch so that we can move on it.
I already have a small enhancement I'd like to add for Mac OS X specific kqueue behavior but it doesn't really make sense to add more things right now.
Ideally this would be nice if I could push it somewhat soon and keep enhancing it in-tree.
Now that a new major GLib release was released, I think it may be a good time...

Also it would mean easier testing from BSD users.
Comment 16 Antoine Jacoutot 2012-11-08 08:05:09 UTC
Guys, is there any interest at all in getting this patch in or should BSD platforms keep it as a vendor patch? I am trying here, but the lack of answer kind of demotivates me :-/
Comment 17 Matthias Clasen 2012-11-08 11:44:29 UTC
Sorry for not getting this reviewed faster; I'll try to drum up some reviewers...
Comment 18 Colin Walters 2012-11-09 02:12:28 UTC
Review of attachment 224541 [details] [review]:

In the big picture, this should be OK to merge.  Since I don't know kqueue, this is mostly style stuff.  It's fine to import non-GLib code as long as it's cleanly separate files with a clearly defined coding style, but mixing GNU with the whatever the other one is (strstroup) is ugly.

I'm pretty curious how well this is tested and works.  Did you do some basic checks creating/destroying monitors to be sure we're not leaking memory?

Anyways, let's put a bit of effort into some style and just get this merged; anything more can be iterated on.

::: gio/giomodule.c
@@ +920,3 @@
+#if defined(HAVE_KQUEUE)
+      _g_kqueue_directory_monitor_get_type ();
+      _g_kqueue_file_monitor_get_type ();

Should use g_type_ensure () like the others.

::: gio/kqueue/kqueue-helper.c
@@ +49,3 @@
+G_GNUC_INTERNAL G_LOCK_DEFINE (hash_lock);
+
+int kqueue_descriptor = -1;

Should be static; if it's not, make it so, and add an accessor function for any other compilation units that need it.

::: gio/kqueue/kqueue-thread.c
@@ +49,3 @@
+
+/* TODO: Probably it would be better to pass it as a thread param? */
+extern int kqueue_descriptor;

Add an accessor please.

@@ +216,3 @@
+  waiting.kq_size = 1;
+
+  for (;;) {

Here you're just inconsistent in the same function . The earlier condtional is GNU style, this one should have brace on same line too.

@@ +231,3 @@
+      {
+        KT_W ("kevent failed: %d", errno);
+        if (errno == EINTR)

If KT_W is a printf call or something, it's highly likely it stomped on errno.  You should to save errno right after the kevent() call to be safe.  Or likely better, just move the KT_W to after the EINTR handling.

::: gio/kqueue/kqueue-utils.c
@@ +196,3 @@
+  memset (&st, 0, sizeof (struct stat));
+
+  if (fstat (fd, &st) == -1) {

Brace on new line.

@@ +198,3 @@
+  if (fstat (fd, &st) == -1) {
+      KU_W ("fstat failed, assuming it is just a file");
+      return;

Here we've left *is_dir untouched.  For out values, it's better to always assign them so the caller doesn't have to guess.
Comment 19 Antoine Jacoutot 2012-11-09 07:03:36 UTC
> I'm pretty curious how well this is tested and works.  Did you do some basic
> checks creating/destroying monitors to be sure we're not leaking memory?

As I mentioned in my former post, this patch has been incorporated in the GLib package in OpenBSD since last January. And I have been running with it even before that.

> Anyways, let's put a bit of effort into some style and just get this merged;
> anything more can be iterated on.

Thanks Colin, I'll work on those.

> ::: gio/giomodule.c
> @@ +920,3 @@
> +#if defined(HAVE_KQUEUE)
> +      _g_kqueue_directory_monitor_get_type ();
> +      _g_kqueue_file_monitor_get_type ();
> 
> Should use g_type_ensure () like the others.
> 
> ::: gio/kqueue/kqueue-helper.c
> @@ +49,3 @@
> +G_GNUC_INTERNAL G_LOCK_DEFINE (hash_lock);
> +
> +int kqueue_descriptor = -1;
> 
> Should be static; if it's not, make it so, and add an accessor function for any
> other compilation units that need it.
> 
> ::: gio/kqueue/kqueue-thread.c
> @@ +49,3 @@
> +
> +/* TODO: Probably it would be better to pass it as a thread param? */
> +extern int kqueue_descriptor;
> 
> Add an accessor please.
> 
> @@ +216,3 @@
> +  waiting.kq_size = 1;
> +
> +  for (;;) {
> 
> Here you're just inconsistent in the same function . The earlier condtional is
> GNU style, this one should have brace on same line too.
> 
> @@ +231,3 @@
> +      {
> +        KT_W ("kevent failed: %d", errno);
> +        if (errno == EINTR)
> 
> If KT_W is a printf call or something, it's highly likely it stomped on errno. 
> You should to save errno right after the kevent() call to be safe.  Or likely
> better, just move the KT_W to after the EINTR handling.
> 
> ::: gio/kqueue/kqueue-utils.c
> @@ +196,3 @@
> +  memset (&st, 0, sizeof (struct stat));
> +
> +  if (fstat (fd, &st) == -1) {
> 
> Brace on new line.
> 
> @@ +198,3 @@
> +  if (fstat (fd, &st) == -1) {
> +      KU_W ("fstat failed, assuming it is just a file");
> +      return;
> 
> Here we've left *is_dir untouched.  For out values, it's better to always
> assign them so the caller doesn't have to guess.
Comment 20 Antoine Jacoutot 2012-11-11 13:34:44 UTC
> Anyways, let's put a bit of effort into some style and just get this merged;
> anything more can be iterated on.

Hi Colin.

Here's an updated patch (successfully tested).
I do have a question:

> @@ +231,3 @@
> +      {
> +        KT_W ("kevent failed: %d", errno);
> +        if (errno == EINTR)
> 
> If KT_W is a printf call or something, it's highly likely it stomped on errno. 
> You should to save errno right after the kevent() call to be safe.  Or likely
> better, just move the KT_W to after the EINTR handling.

This part I'm not sure I understand what you meant. I blindly moved the KT_W after EINTR in the new patch _but_ that does not make much sense to me so I probably misunderstood what you meant in the first place...

Thanks!
Comment 21 Antoine Jacoutot 2012-11-11 13:35:21 UTC
Created attachment 228694 [details] [review]
 Add kqueue(3) support to GIO, v3
Comment 22 Simon McVittie 2012-11-14 09:01:44 UTC
(In reply to comment #20)
> > If KT_W is a printf call or something, it's highly likely it stomped on errno. 
> > You should to save errno right after the kevent() call to be safe.  Or likely
> > better, just move the KT_W to after the EINTR handling.
> 
> This part I'm not sure I understand what you meant. I blindly moved the KT_W
> after EINTR in the new patch _but_ that does not make much sense to me so I
> probably misunderstood what you meant in the first place...

Whenever you make a libc call, do something else involving a function call, and then use errno, you run the risk that the "something else" has altered errno, so it no longer contains the error from the libc call. For instance:

  again:
    if (open ("foo", O_WRONLY) < 0)
      {
        g_debug ("%s", g_strerror (errno));

        if (errno == EINTR)        /* line 6 */
          goto again;
      }

If you see EINTR at line 6, it might be because open() failed with EINTR, or it might be because something called by g_debug (or even g_strerror if you're really unlucky) failed with EINTR. You can't tell. Similarly, if errno *isn't* EINTR at line 6, the open() call might still have failed with EINTR - one of the other library calls might have changed errno afterwards.

One way round that is to be careful about ordering, so you only call functions after the last time errno is evaluated:

  again:
    if (open ("foo", O_WRONLY) < 0)
      {
        if (errno == EINTR)   /* no library calls here, can't break errno */
          goto again;

        /* by the time g_strerror is invoked, we have the value of errno
         * on the stack/in a register as a parameter, so this counts as
         * "after" */
        g_debug ("%s", g_strerror (errno));
      }

Another is to save the interesting value of errno before you do anything that might modify it:

  again:
    if (open ("foo", O_WRONLY) < 0)
      {
        int save_errno = errno;

        g_debug ("%s", g_strerror (save_errno));

        if (save_errno == EINTR)
          goto again;
      }
Comment 23 Antoine Jacoutot 2012-11-14 17:08:01 UTC
> > > If KT_W is a printf call or something, it's highly likely it stomped on errno. 
> > > You should to save errno right after the kevent() call to be safe.  Or likely
> > > better, just move the KT_W to after the EINTR handling.
> > 
> > This part I'm not sure I understand what you meant. I blindly moved the KT_W
> > after EINTR in the new patch _but_ that does not make much sense to me so I
> > probably misunderstood what you meant in the first place...

Ah that makes perfect sense, of course. Thanks ;-)
New patch. Hopefully every request have been answered/fixed now...
Comment 24 Antoine Jacoutot 2012-11-14 17:08:31 UTC
Created attachment 228981 [details] [review]
Add kqueue(3) support to GIO, v4
Comment 25 Colin Walters 2012-11-14 18:41:07 UTC
Review of attachment 228981 [details] [review]:

Ok.  There are other miscellaneous bits that I noticed now (e.g. _ku_path_concat() checks for OOM, but it should just use g_build_filename() since all of GLib aborts on  OOM).  But we could bikeshed endlessly about to what degree the code should be "core BSD libc" versus "GLib", so like I said earlier, let's just get it in, and further code cleanups can happen later.
Comment 26 Antoine Jacoutot 2012-11-15 07:17:01 UTC
Colin, it's in as 2054cca.
Thank you again for helping on getting this is.
I'll keep the bug opened until all bikesheding is gone ;-)
Comment 27 Matthias Clasen 2013-08-18 01:41:37 UTC
looks like no more bikeshedding coming