Re: tail + inotify over nfs

[Jim Meyering]

Pádraig Brady wrote:

> On 16/12/09 12:36, Jim Meyering wrote:
>> Pádraig Brady wrote:
>>> I got a few minutes to look at this today,
>>> and the attached patch seems to work with a very quick test.
>>>
>>> It doesn't handle the above remount case though
>>> as if I mount the parent dir of a file or bind mount the file itself
>>> then there are no inotify notifications. This remounting issue is
>>> independent of nfs anyway. So can inotify handle this or will we
>>> have to periodically check with a select rather than a blocking read?
>>
>> Thanks for starting on this.
>> Please filter through cppi to indent the new cpp directives.
>> I should make syntax-check automate that check.  There used to be
>> a cvs commit hook to enforce it.
>>
>> Also, please spell "file system" with two words, not one,
>> to get by the syntax-check for that.
>>
>> Not to look the gift horse in the mouth, but what do you
>> think about adding a test for this?  Maybe we can exercise
>> it via a FUSE-based file system.  That should be easier than
>> setting up NFS.
>
> I had a quick look at this and it seemed messy as
> I couldn't see any builtin fuse filesystems I could use.
>
> Attached is the latest version with the unlikely edge cases that
> aren't handled mentioned in this comment:
>
>   FIXME: inotify doesn't give any notification when a new
>   (remote) file or directory is mounted on top a watched file.
>   When follow_mode == Follow_name we would ideally like to detect that.
>   Note if there is a change to the original file then we'll
>   recheck it and follow the new file, or ignore it if the
>   file has changed to being remote.

Hi Pádraig,

Thanks for doing all that.
Here are some grammar nits:

diff --git a/src/tail.c b/src/tail.c
index 0256804..a4d376e 100644
--- a/src/tail.c
+++ b/src/tail.c
@@ -1174,11 +1174,11 @@ tail_forever (struct File_spec *f, size_t n_files, 
double sleep_interval)

 #if HAVE_INOTIFY

-/* Return true if any of the N_FILES files in F are remote, i.e., have
-   open file descriptors and are on network file systems.  */
+/* Return true if any of the N_FILES files in F is remote, i.e., has
+   an open file descriptor and is on a network file system.  */

 static bool
-any_remote_files (const struct File_spec *f, size_t n_files)
+any_remote_file (const struct File_spec *f, size_t n_files)
 {
   size_t i;

@@ -1188,8 +1188,8 @@ any_remote_files (const struct File_spec *f, size_t 
n_files)
   return false;
 }

-/* Return true if any of the N_FILES files in F represent
-   stdin and are tailable.  */
+/* Return true if any of the N_FILES files in F represents
+   stdin and is tailable.  */

 static bool
 tailable_stdin (const struct File_spec *f, size_t n_files)
@@ -2112,7 +2112,7 @@ main (int argc, char **argv)
          and hooked up to stdin is not trivial, while reverting to
          non-inotify-based tail_forever is easy and portable.

-         any_remote_files() checks if the user has specified any
+         any_remote_file() checks if the user has specified any
          files that reside on remote file systems.  inotify is not used
          in this case because it would miss any updates to the file
          that were not initiated from the local system.
@@ -2123,7 +2123,7 @@ main (int argc, char **argv)
          Note if there is a change to the original file then we'll
          recheck it and follow the new file, or ignore it if the
          file has changed to being remote.  */
-      if (tailable_stdin (F, n_files) || any_remote_files (F, n_files))
+      if (tailable_stdin (F, n_files) || any_remote_file (F, n_files))
         disable_inotify = true;

       if (!disable_inotify)

-----------------------------------

Here's a bigger patch (superset of the above) that also makes it so
fremote can be used without those annoyingly unreadable in-function #ifdefs.
It moves the declaration of fremote "up" to precede the first use
and ensures that it's defined to "false" in the !HAVE_INOTIFY case.
That makes it so the two uses work properly when !HAVE_INOTIFY.

diff --git a/src/tail.c b/src/tail.c
index 0256804..4e045ae 100644
--- a/src/tail.c
+++ b/src/tail.c
@@ -160,8 +160,6 @@ struct File_spec
    directories.  */
 const uint32_t inotify_wd_mask = (IN_MODIFY | IN_ATTRIB | IN_DELETE_SELF
                                   | IN_MOVE_SELF);
-
-static bool fremote (int fd, const char *name);
 #endif

 /* Keep trying to open a file even if it is inaccessible when tail starts
@@ -875,6 +873,50 @@ start_lines (const char *pretty_filename, int fd, 
uintmax_t n_lines,
     }
 }

+#if HAVE_INOTIFY
+static bool
+fremote (int fd, const char *name)
+{
+  bool remote = true;           /* be conservative (poll by default).  */
+
+# if HAVE_FSTATFS && HAVE_STRUCT_STATFS_F_TYPE && defined __linux__
+  struct statfs buf;
+  int err = fstatfs (fd, &buf);
+  if (err != 0)
+    {
+      error (0, errno, _("cannot determine location of %s. "
+                         "reverting to polling"), quote (name));
+    }
+  else
+    {
+      switch (buf.f_type)
+        {
+        case S_MAGIC_AFS:
+        case S_MAGIC_CIFS:
+        case S_MAGIC_CODA:
+        case S_MAGIC_FUSEBLK:
+        case S_MAGIC_FUSECTL:
+        case S_MAGIC_GFS:
+        case S_MAGIC_KAFS:
+        case S_MAGIC_LUSTRE:
+        case S_MAGIC_NCP:
+        case S_MAGIC_NFS:
+        case S_MAGIC_NFSD:
+        case S_MAGIC_OCFS2:
+        case S_MAGIC_SMB:
+          break;
+        default:
+          remote = false;
+        }
+    }
+# endif
+
+  return remote;
+}
+#else
+# define fremote(fd, name) false
+#endif
+
 /* FIXME: describe */

 static void
@@ -931,7 +973,6 @@ recheck (struct File_spec *f, bool blocking)
              quote (pretty_name (f)));
       f->ignore = true;
     }
-#if HAVE_INOTIFY
   else if (!disable_inotify && fremote (fd, pretty_name (f)))
     {
       ok = false;
@@ -941,7 +982,6 @@ recheck (struct File_spec *f, bool blocking)
       f->ignore = true;
       f->remote = true;
     }
-#endif
   else
     {
       f->errnum = 0;
@@ -1174,11 +1214,11 @@ tail_forever (struct File_spec *f, size_t n_files, 
double sleep_interval)

 #if HAVE_INOTIFY

-/* Return true if any of the N_FILES files in F are remote, i.e., have
-   open file descriptors and are on network file systems.  */
+/* Return true if any of the N_FILES files in F is remote, i.e., has
+   an open file descriptor and is on a network file system.  */

 static bool
-any_remote_files (const struct File_spec *f, size_t n_files)
+any_remote_file (const struct File_spec *f, size_t n_files)
 {
   size_t i;

@@ -1188,8 +1228,8 @@ any_remote_files (const struct File_spec *f, size_t 
n_files)
   return false;
 }

-/* Return true if any of the N_FILES files in F represent
-   stdin and are tailable.  */
+/* Return true if any of the N_FILES files in F represents
+   stdin and is tailable.  */

 static bool
 tailable_stdin (const struct File_spec *f, size_t n_files)
@@ -1202,46 +1242,6 @@ tailable_stdin (const struct File_spec *f, size_t 
n_files)
   return false;
 }

-static bool
-fremote (int fd, const char *name)
-{
-  bool remote = true;           /* be conservative (poll by default).  */
-
-# if HAVE_FSTATFS && HAVE_STRUCT_STATFS_F_TYPE && defined __linux__
-  struct statfs buf;
-  int err = fstatfs (fd, &buf);
-  if (err != 0)
-    {
-      error (0, errno, _("cannot determine location of %s. "
-                         "reverting to polling"), quote (name));
-    }
-  else
-    {
-      switch (buf.f_type)
-        {
-        case S_MAGIC_AFS:
-        case S_MAGIC_CIFS:
-        case S_MAGIC_CODA:
-        case S_MAGIC_FUSEBLK:
-        case S_MAGIC_FUSECTL:
-        case S_MAGIC_GFS:
-        case S_MAGIC_KAFS:
-        case S_MAGIC_LUSTRE:
-        case S_MAGIC_NCP:
-        case S_MAGIC_NFS:
-        case S_MAGIC_NFSD:
-        case S_MAGIC_OCFS2:
-        case S_MAGIC_SMB:
-          break;
-        default:
-          remote = false;
-        }
-    }
-# endif
-
-  return remote;
-}
-
 static size_t
 wd_hasher (const void *entry, size_t tabsize)
 {
@@ -1739,9 +1739,7 @@ tail_file (struct File_spec *f, uintmax_t n_units)
                  to avoid a race condition described by Ken Raeburn:
         http://mail.gnu.org/archive/html/bug-textutils/2003-05/msg00007.html */
               record_open_fd (f, fd, read_pos, &stats, (is_stdin ? -1 : 1));
-#if HAVE_INOTIFY
               f->remote = fremote (fd, pretty_name (f));
-#endif
             }
         }
       else
@@ -2112,7 +2110,7 @@ main (int argc, char **argv)
          and hooked up to stdin is not trivial, while reverting to
          non-inotify-based tail_forever is easy and portable.

-         any_remote_files() checks if the user has specified any
+         any_remote_file() checks if the user has specified any
          files that reside on remote file systems.  inotify is not used
          in this case because it would miss any updates to the file
          that were not initiated from the local system.
@@ -2123,7 +2121,7 @@ main (int argc, char **argv)
          Note if there is a change to the original file then we'll
          recheck it and follow the new file, or ignore it if the
          file has changed to being remote.  */
-      if (tailable_stdin (F, n_files) || any_remote_files (F, n_files))
+      if (tailable_stdin (F, n_files) || any_remote_file (F, n_files))
         disable_inotify = true;

       if (!disable_inotify)