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 592211 - No monitoring over NFS mounts
No monitoring over NFS mounts
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gio
2.20.x
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2009-08-18 12:53 UTC by Josselin Mouette
Modified: 2013-02-05 23:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
glib2-force-fam-for-remote-fs.patch (8.52 KB, patch)
2011-01-24 15:53 UTC, Hans Petter Jansson
needs-work Details | Review
file monitor: Use fam on nfs (9.74 KB, patch)
2013-01-18 11:01 UTC, Matthias Clasen
none Details | Review
file monitor: Use fam on nfs (8.95 KB, patch)
2013-01-18 15:35 UTC, Hans Petter Jansson
none Details | Review
giomodule: add a new "get default" function (5.81 KB, patch)
2013-01-18 23:56 UTC, Allison Karlitskaya (desrt)
none Details | Review
file monitors: use new giomodule function (5.77 KB, patch)
2013-01-18 23:56 UTC, Allison Karlitskaya (desrt)
none Details | Review
localfile: add support for monitoring on NFS (9.35 KB, patch)
2013-01-18 23:56 UTC, Allison Karlitskaya (desrt)
none Details | Review
fam: implement gio-nfs-{file,directory}-monitor (1.89 KB, patch)
2013-01-18 23:56 UTC, Allison Karlitskaya (desrt)
none Details | Review
build: fix Windows compilation (2.82 KB, patch)
2013-02-01 02:25 UTC, Marc-Andre Lureau
none Details | Review

Description Josselin Mouette 2009-08-18 12:53:34 UTC
GIO is able to monitor using either inotify or fam, but will unconditionnally use inotify if supported by the kernel. However, there is no point using inotify for a NFS mount. It will not return any events originating from a different NFS client.

Currently, only remote FAM monitoring is able to detect changes over remote filesystems. I know FAM sucks, but it’s better than nothing, really.

Fixing this requires restructuring the way file monitoring extensions are handled. Instead of initializing the default one only, it needs to be able to initialize several of them and to select one depending on the filesystem type. Which in return requires improving the extensions interface, to select which one is supported for a given path.

Does it sound reasonable? Or do we just choose to ignore monitoring on remote filesystems?
Comment 1 Matthias Clasen 2011-01-19 15:58:32 UTC
It sounds reasonable to me, from afar.

But I agree that not monitoring remote file systems seems at all seems far better than having remote monitoring that doesn't quite work or works only sometimes, or hangs up your system.
Comment 2 Hans Petter Jansson 2011-01-24 15:53:03 UTC
Created attachment 179179 [details] [review]
glib2-force-fam-for-remote-fs.patch

This patch solves the immediate problem by hardcoding FAM for file systems of type 'nfs' or 'nfs4'; inotify will be used as a fallback. Not perfect, but it's a start.

In order to avoid hardcoding, we'd need to introduce a framework for supplying hints to GIO extensions and having them report priorities based on that, or some kind of capabilities system where "local" and "remote" monitoring are potential capabilities, and rank extensions by matching capabilities and then priority.

Additionally, the patch introduces a blocking statfs()/statvfs() call for each monitor created. I'm not sure we can avoid that unless we make the whole extension loading business asynchronous, though.

A much simpler option would be to make the monitoring extension externally selectable through a configuration mechanism, with the drawback of forcing anyone who wants NFS monitoring to work, to use FAM for everything.
Comment 3 Allison Karlitskaya (desrt) 2013-01-12 17:23:12 UTC
I'm using GFileMonitor for the keyfile backend in dconf now.  Would be great if that worked reliably with NFS without me needing to special-case it...
Comment 4 Allison Karlitskaya (desrt) 2013-01-12 18:13:28 UTC
Review of attachment 179179 [details] [review]:

This generally looks pretty good.  A few comments:

You took an evil function -- _g_local_file_monitor_new() -- and made it very evil.  That pass-by-reference-the-first-time business was already scary enough... There could possibly be some reworking there.

The stat() is bad but not too bad.  It's going to be pretty fast on non-NFS and one way or another we're probably just about to stat()/inotify()/whatever that file anyway... so this just serves to pull it into cache faster.  On NFS this patch is a net win, so the performance hit doesn't bother me too much.  Perhaps we can do better if we try to target a smaller problem, though: NFS home directory (the 99% case).  Then we would only have to do the detection once and then just check for the requested path being contained in ~.

Thoughts?
Comment 5 Matthias Clasen 2013-01-18 11:01:27 UTC
Created attachment 233743 [details] [review]
file monitor: Use fam on nfs

When monitoring a 'remote' file or directory, inotify is not
going to work, so use fam in that case. We avoid too much extra
stat() overhead by special-casing the 'nfs homedir' use case.
Comment 6 Matthias Clasen 2013-01-18 11:04:14 UTC
I've added the special case for home dir, but I had no good idea for how to restructure the _new functions.
Comment 7 Allison Karlitskaya (desrt) 2013-01-18 14:23:35 UTC
Review of attachment 233743 [details] [review]:

Interesting 'hybrid' approach.  I was suggesting that we _only_ care about the NFS homedir case.

What are your higher-level thoughts on the FAM thing?  By default it would appear that we don't even build the FAM backend...

::: gio/glocalfile.c
@@ +2463,3 @@
+
+  home = g_get_home_dir ();
+  if (g_str_has_prefix (filename, home))

If my homedir is /home/rob then this will also match /home/robert
Comment 8 Hans Petter Jansson 2013-01-18 15:24:44 UTC
FWIW, I'm fine with restricting it to home directories. Thanks, Matthias, for adding that.
Comment 9 Hans Petter Jansson 2013-01-18 15:35:35 UTC
Created attachment 233767 [details] [review]
file monitor: Use fam on nfs

Minor fix: I replaced the call to g_str_has_prefix() with path_has_prefix(), which is already present in the file, and seems to do what we want.
Comment 10 Hans Petter Jansson 2013-01-18 15:49:05 UTC
(By the way, implementations of this path_has_prefix() business pop up here and there - maybe there is a case for g_path_has_prefix()?).
Comment 11 Josselin Mouette 2013-01-18 15:54:15 UTC
Beware that most distros will install, by default, libgamin instead of libfam. And gamin does not work over NFS.
Comment 12 Allison Karlitskaya (desrt) 2013-01-18 18:00:51 UTC
gamin does have code for polling NFS, however.
Comment 13 Allison Karlitskaya (desrt) 2013-01-18 23:56:23 UTC
Created attachment 233814 [details] [review]
giomodule: add a new "get default" function

_gio_module_get_default() is a very convenient function for modules
implementing a singleton -- it finds the default module by priority
subject to override by a given environment variable name, instantiates
it, and caches the instance for future calls.  It also has the ability
to query instances for being 'active' using a callback.

It doesn't work very well for non-singletons (like file monitors).

Add a new function _gio_module_get_default_type() that skips the
instantiation, returning the GType instead.  As a replacement for the
'active' callback, a vtable offset can be given for a virtual function
to use to query if a particular backend is supported.
Comment 14 Allison Karlitskaya (desrt) 2013-01-18 23:56:28 UTC
Created attachment 233815 [details] [review]
file monitors: use new giomodule function

Get rid of the complicated default module detection code in
GLocalFileMonitor and GLocalDirectoryMonitor and use the new
_gio_module_get_default_type() function instead.

This change also adds the ability to override the default file monitor
via the GIO_USE_FILE_MONITOR environment variable in the same way as can
be done for GIO_USE_VFS.
Comment 15 Allison Karlitskaya (desrt) 2013-01-18 23:56:30 UTC
Created attachment 233816 [details] [review]
localfile: add support for monitoring on NFS

Add a pair of new extension points: 'gio-nfs-file-monitor' and
'gio-nfs-directory-monitor'.

Add a check to GLocalFile when creating a file monitor.  If the
requested file is in the user's home directory and the user has an NFS
home directory then attempt to use an implementation of one of the new
extension points.  If we don't have any implementations then fall back
to the normal "local" monitors.
Comment 16 Allison Karlitskaya (desrt) 2013-01-18 23:56:33 UTC
Created attachment 233817 [details] [review]
fam: implement gio-nfs-{file,directory}-monitor

Declare explicit support for monitor NFS from the fam file monitoring
backend.  This will cause it to be preferred for monitoring on NFS, if
it is installed.
Comment 17 Matthias Clasen 2013-01-19 01:32:55 UTC
Looks much nicer, indeed.
Only question left is if we want to do something about mounts inside an nfs homedir. Not sure it is a huge issue - after all, using fam will still work in those.
Comment 18 Matthias Clasen 2013-01-19 19:39:57 UTC
This was pushed
Comment 19 Allison Karlitskaya (desrt) 2013-01-20 04:18:27 UTC
by accident?

Not objecting, mind you...
Comment 20 Matthias Clasen 2013-01-20 19:46:09 UTC
yeah, by accident. didnt' seem worth reverting
Comment 21 Marc-Andre Lureau 2013-02-01 02:25:12 UTC
Created attachment 234943 [details] [review]
build: fix Windows compilation

glocalfile.c: In function 'is_remote_fs':
glocalfile.c:2434:7: error: 'statfs_result' undeclared (first use in this function)
glocalfile.c:2434:7: note: each undeclared identifier is reported only once for each function it appears in
glocalfile.c: In function 'is_remote':
glocalfile.c:2467:3: error: implicit declaration of function 'path_has_prefix' [-Werror=implicit-function-declaration]
cc1: some warnings being treated as errors
Comment 22 Marc-Andre Lureau 2013-02-01 02:25:58 UTC
reopening for the build error on windows
Comment 23 Allison Karlitskaya (desrt) 2013-02-01 04:55:15 UTC
Maybe a better fix is to just disable the new code on Windows?  We're probably not going to find NFS there...
Comment 24 Marc-Andre Lureau 2013-02-05 23:32:29 UTC
With a reference to the commit would be helpful.

commit 59372663f2ed57b60de7cbf2a3241cf9d7adbbea
Author: Matthias Clasen <mclasen@redhat.com>
Date:   Sat Feb 2 00:19:15 2013 -0500

    Don't try to find nfs mounts on Windows