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.
cheers,
Pádraig.
>From 0229b8ae7c2bac52209a7dc980b4be8ea0dad895 Mon Sep 17 00:00:00 2001
From: =?utf-8?q?P=C3=A1draig=20Brady?= <p...@draigbrady.com>
Date: Mon, 14 Dec 2009 22:45:34 +0000
Subject: [PATCH] tail: fix --follow to not use inotify on remote files
* src/tail.c (struct File_spec): Add a flag to record if file is remote.
(recheck): If we're using inotify then check if the file has gone remote
and if so, drop it with a warning.
(any_remote_files): A new function to check for any open remote files.
(tailable_stdin): A new function to refactor the check for whether
a tailable file was specified through stdin.
(fremote): A new function to check if a file descriptor
refers to a remote file.
(tail_forever_inotify): Add some comments.
(tail_file): Record if a file is remote when initially opened.
(main): Disable inotify if any remote files specified.
Also document the caveat about remounted files not
being noticed by inotify.
* NEWS: Mention the fix.
---
NEWS | 5 ++
src/tail.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++++++++------
2 files changed, 121 insertions(+), 14 deletions(-)
diff --git a/NEWS b/NEWS
index ac5bd07..8395d91 100644
--- a/NEWS
+++ b/NEWS
@@ -13,6 +13,11 @@ GNU coreutils NEWS -*- outline -*-
and rpc_pipefs. Also Minix V3 is displayed correctly as minix3, not minux3.
[bug introduced in coreutils-8.1]
+ tail -f (inotify-enabled) once again works with remote files.
+ The use of inotify with remote files meant that any changes to those
+ files that was not done from the local system would go unnoticed.
+ [bug introduced in coreutils-7.5]
+
touch -a once again guarantees that a file's change time is
adjusted, working around a bug in current Linux kernels.
[bug introduced in coreutils-8.1]
diff --git a/src/tail.c b/src/tail.c
index 71f8a32..0256804 100644
--- a/src/tail.c
+++ b/src/tail.c
@@ -52,6 +52,12 @@
# include <sys/inotify.h>
/* `select' is used by tail_forever_inotify. */
# include <sys/select.h>
+
+/* inotify needs to know if a file is local. */
+# include "fs.h"
+# if HAVE_SYS_STATFS_H
+# include <sys/statfs.h>
+# endif
#endif
/* The official name of this program (e.g., no `g' prefix). */
@@ -143,6 +149,9 @@ struct File_spec
/* Offset in NAME of the basename part. */
size_t basename_start;
+
+ /* inotify doesn't work for remotely updated files. */
+ bool remote;
#endif
};
@@ -151,6 +160,8 @@ 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
@@ -201,8 +212,7 @@ static bool have_read_stdin;
more expensive) code unconditionally. Intended solely for testing. */
static bool presume_input_pipe;
-/* If nonzero then don't use inotify even if available.
- Intended solely for testing. */
+/* If nonzero then don't use inotify even if available. */
static bool disable_inotify;
/* For long options that have no equivalent short option, use a
@@ -921,6 +931,17 @@ 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;
+ f->errnum = -1;
+ error (0, 0, _("%s has been replaced with a remote file. "
+ "giving up on this name"), quote (pretty_name (f)));
+ f->ignore = true;
+ f->remote = true;
+ }
+#endif
else
{
f->errnum = 0;
@@ -1153,6 +1174,74 @@ 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. */
+
+static bool
+any_remote_files (const struct File_spec *f, size_t n_files)
+{
+ size_t i;
+
+ for (i = 0; i < n_files; i++)
+ if (0 <= f[i].fd && f[i].remote)
+ return true;
+ return false;
+}
+
+/* Return true if any of the N_FILES files in F represent
+ stdin and are tailable. */
+
+static bool
+tailable_stdin (const struct File_spec *f, size_t n_files)
+{
+ size_t i;
+
+ for (i = 0; i < n_files; i++)
+ if (!f[i].ignore && STREQ (f[i].name, "-"))
+ return true;
+ 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)
{
@@ -1310,7 +1399,7 @@ tail_forever_inotify (int wd, struct File_spec *f, size_t n_files,
struct inotify_event *ev;
/* When watching a PID, ensure that a read from WD will not block
- indefinetely. */
+ indefinitely. */
if (pid)
{
if (writer_is_dead)
@@ -1362,7 +1451,7 @@ tail_forever_inotify (int wd, struct File_spec *f, size_t n_files,
ev = (struct inotify_event *) (evbuf + evbuf_off);
evbuf_off += sizeof (*ev) + ev->len;
- if (ev->len)
+ if (ev->len) /* event on ev->name in watched directory */
{
for (i = 0; i < n_files; i++)
{
@@ -1377,6 +1466,7 @@ tail_forever_inotify (int wd, struct File_spec *f, size_t n_files,
if (i == n_files)
continue;
+ /* It's fine to add the same file more than once. */
f[i].wd = inotify_add_watch (wd, f[i].name, inotify_wd_mask);
if (f[i].wd < 0)
@@ -1649,6 +1739,9 @@ 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
@@ -2012,19 +2105,28 @@ main (int argc, char **argv)
if (forever && ignore_fifo_and_pipe (F, n_files))
{
#if HAVE_INOTIFY
- /* If the user specifies stdin via a command line argument of "-",
- or implicitly by providing no arguments, we won't use inotify.
+ /* tailable_stdin() checks if the user specifies stdin via "-",
+ or implicitly by providing no arguments. If so, we won't use inotify.
Technically, on systems with a working /dev/stdin, we *could*,
but would it be worth it? Verifying that it's a real device
and hooked up to stdin is not trivial, while reverting to
- non-inotify-based tail_forever is easy and portable. */
- bool stdin_cmdline_arg = false;
-
- for (i = 0; i < n_files; i++)
- if (!F[i].ignore && STREQ (F[i].name, "-"))
- stdin_cmdline_arg = true;
-
- if (!disable_inotify && !stdin_cmdline_arg)
+ non-inotify-based tail_forever is easy and portable.
+
+ any_remote_files() 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.
+
+ 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. */
+ if (tailable_stdin (F, n_files) || any_remote_files (F, n_files))
+ disable_inotify = true;
+
+ if (!disable_inotify)
{
int wd = inotify_init ();
if (wd < 0)
--
1.6.2.5
