GNOME Bugzilla – Bug 592211
No monitoring over NFS mounts
Last modified: 2013-02-05 23:32:29 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?
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.
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.
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...
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?
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.
I've added the special case for home dir, but I had no good idea for how to restructure the _new functions.
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
FWIW, I'm fine with restricting it to home directories. Thanks, Matthias, for adding that.
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.
(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()?).
Beware that most distros will install, by default, libgamin instead of libfam. And gamin does not work over NFS.
gamin does have code for polling NFS, however.
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.
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.
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.
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.
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.
This was pushed
by accident? Not objecting, mind you...
yeah, by accident. didnt' seem worth reverting
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
reopening for the build error on windows
Maybe a better fix is to just disable the new code on Windows? We're probably not going to find NFS there...
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