GNU bug report logs - #67490
[PATCH v2] tail: fix tailing sysfs files on large page kernels

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 67490 in the body.
You can then email your comments to 67490 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: dann frazier <dann.frazier <at> canonical.com>
To: bug-coreutils <at> gnu.org
Subject: [PATCH] tail: fix following /proc and /sys files when using a 64K
 page size
Date: Mon, 27 Nov 2023 09:24:33 -0700

* src/tail.c (file_lines): Use fstat() to determine a file's block
size and dynamically allocate a buffer of that size for traversing
backwards.
---
 src/tail.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/src/tail.c b/src/tail.c
index c45f3b65a..437a38204 100644
--- a/src/tail.c
+++ b/src/tail.c
@@ -518,18 +518,30 @@ static bool
 file_lines (char const *pretty_filename, int fd, uintmax_t n_lines,
             off_t start_pos, off_t end_pos, uintmax_t *read_pos)
 {
-  char buffer[BUFSIZ];
+  char *buffer;
   size_t bytes_read;
+  blksize_t bufsize;
   off_t pos = end_pos;
+  bool ok = true;
+  struct stat stats;
 
   if (n_lines == 0)
     return true;
 
+  if (fstat (fd, &stats) != 0)
+    {
+      error (0, errno, _("cannot fstat %s"), quoteaf (pretty_filename));
+      return false;
+    }
+
+  bufsize = ST_BLKSIZE (stats);
+  buffer = xmalloc (bufsize);
+
   /* Set 'bytes_read' to the size of the last, probably partial, buffer;
      0 < 'bytes_read' <= 'BUFSIZ'.  */
-  bytes_read = (pos - start_pos) % BUFSIZ;
+  bytes_read = (pos - start_pos) % bufsize;
   if (bytes_read == 0)
-    bytes_read = BUFSIZ;
+    bytes_read = bufsize;
   /* Make 'pos' a multiple of 'BUFSIZ' (0 if the file is short), so that all
      reads will be on block boundaries, which might increase efficiency.  */
   pos -= bytes_read;
@@ -538,7 +550,8 @@ file_lines (char const *pretty_filename, int fd, uintmax_t n_lines,
   if (bytes_read == SAFE_READ_ERROR)
     {
       error (0, errno, _("error reading %s"), quoteaf (pretty_filename));
-      return false;
+      ok = false;
+      goto free_buffer;
     }
   *read_pos = pos + bytes_read;
 
@@ -565,7 +578,7 @@ file_lines (char const *pretty_filename, int fd, uintmax_t n_lines,
               xwrite_stdout (nl + 1, bytes_read - (n + 1));
               *read_pos += dump_remainder (false, pretty_filename, fd,
                                            end_pos - (pos + bytes_read));
-              return true;
+              goto free_buffer;
             }
         }
 
@@ -577,23 +590,26 @@ file_lines (char const *pretty_filename, int fd, uintmax_t n_lines,
           xlseek (fd, start_pos, SEEK_SET, pretty_filename);
           *read_pos = start_pos + dump_remainder (false, pretty_filename, fd,
                                                   end_pos);
-          return true;
+          goto free_buffer;
         }
-      pos -= BUFSIZ;
+      pos -= bufsize;
       xlseek (fd, pos, SEEK_SET, pretty_filename);
 
-      bytes_read = safe_read (fd, buffer, BUFSIZ);
+      bytes_read = safe_read (fd, buffer, bufsize);
       if (bytes_read == SAFE_READ_ERROR)
         {
           error (0, errno, _("error reading %s"), quoteaf (pretty_filename));
-          return false;
+          ok = false;
+          goto free_buffer;
         }
 
       *read_pos = pos + bytes_read;
     }
   while (bytes_read > 0);
 
-  return true;
+free_buffer:
+  free (buffer);
+  return ok;
 }
 
 /* Print the last N_LINES lines from the end of the standard input,
-- 
2.42.0

Message #8 received at 67490 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: dann frazier <dann.frazier <at> canonical.com>, 67490 <at> debbugs.gnu.org
Subject: Re: bug#67490: [PATCH] tail: fix following /proc and /sys files when
 using a 64K page size
Date: Mon, 27 Nov 2023 19:10:26 +0000

On 27/11/2023 16:24, dann frazier wrote:
> * src/tail.c (file_lines): Use fstat() to determine a file's block
> size and dynamically allocate a buffer of that size for traversing
> backwards.

Thanks for the patch.
Could you describe it a bit more.
What happens if we use smaller reads?
Also what about all the other safe_read() calls in tail.c ?

thanks,
Pádraig

Message #11 received at 67490 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: dann frazier <dann.frazier <at> canonical.com>, 67490 <at> debbugs.gnu.org
Subject: Re: bug#67490: [PATCH] tail: fix following /proc and /sys files when
 using a 64K page size
Date: Mon, 27 Nov 2023 11:17:57 -0800

On 2023-11-27 08:24, dann frazier wrote:
> +  if (fstat (fd, &stats) != 0)
> +    {
> +      error (0, errno, _("cannot fstat %s"), quoteaf (pretty_filename));
> +      return false;
> +    }
> +
> +  bufsize = ST_BLKSIZE (stats);


If fstat fails, that's no reason to exit. Just use bufsize = BUFSIZ and 
keep going. fstat can fail for reasons unrelated to what 'tail' needs 
(e.g., time_t overflow).

Message #14 received at 67490 <at> debbugs.gnu.org (full text, mbox):

From: dann frazier <dann.frazier <at> canonical.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 67490 <at> debbugs.gnu.org
Subject: Re: bug#67490: [PATCH] tail: fix following /proc and /sys files when
 using a 64K page size
Date: Tue, 28 Nov 2023 17:38:52 -0700

On Mon, Nov 27, 2023 at 11:17:57AM -0800, Paul Eggert wrote:
> On 2023-11-27 08:24, dann frazier wrote:
> > +  if (fstat (fd, &stats) != 0)
> > +    {
> > +      error (0, errno, _("cannot fstat %s"), quoteaf (pretty_filename));
> > +      return false;
> > +    }
> > +
> > +  bufsize = ST_BLKSIZE (stats);
> 
> 
> If fstat fails, that's no reason to exit. Just use bufsize
> = BUFSIZ and keep going. fstat can fail for reasons
> unrelated to what 'tail' needs (e.g., time_t overflow).

Thanks for the suggestion - I'll make that change.

  -dann

Message #17 received at 67490 <at> debbugs.gnu.org (full text, mbox):

From: dann frazier <dann.frazier <at> canonical.com>
To: Pádraig Brady <P <at> draigbrady.com>
Cc: 67490 <at> debbugs.gnu.org
Subject: Re: bug#67490: [PATCH] tail: fix following /proc and /sys files when
 using a 64K page size
Date: Wed, 29 Nov 2023 07:34:26 -0700

On Mon, Nov 27, 2023 at 07:10:26PM +0000, Pádraig Brady wrote:
> On 27/11/2023 16:24, dann frazier wrote:
> > * src/tail.c (file_lines): Use fstat() to determine a file's block
> > size and dynamically allocate a buffer of that size for traversing
> > backwards.
> 
> Thanks for the patch.
> Could you describe it a bit more.
> What happens if we use smaller reads?
> Also what about all the other safe_read() calls in tail.c ?

Yes, this warrants more detail - I'll try to capture that in a
comment. Thanks Pádraig!

  -dann

Message #20 received at 67490 <at> debbugs.gnu.org (full text, mbox):

From: dann frazier <dann.frazier <at> canonical.com>
To: 67490 <at> debbugs.gnu.org
Subject: [PATCH v2] tail: fix tailing sysfs files on large page kernels
Date: Wed, 29 Nov 2023 18:32:34 -0700

Changes from v1:
 - Fallback to BUFSIZ if fstat fails instead of exiting.
 - Add a comment block to explain why the blksize is used in file_lines()
   (which I hope also clarifies why it is not needed elsewhere).
 - Only use blksize if it is > BUFSIZ (I had regressed to 4K reads, oops).
 - Update comments to reference bufsize instead of BUFSIZ.
 - Update subject because /proc is not affected, and it isn't limited
   to follow mode.
 - Add a test.

* src/tail.c (file_lines): Consider a full blocksize when
searching backwards to find sysfs file content on systems
with large page sizes.
* tests/tail/tail-1-sysfs.sh: Add a new test.
* tests/local.mk: Reference the new test.
Fixes https://bugs.gnu.org/67490
---
 src/tail.c                 | 41 +++++++++++++++++++++++++++-----------
 tests/local.mk             |  1 +
 tests/tail/tail-1-sysfs.sh | 31 ++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+), 12 deletions(-)
 create mode 100755 tests/tail/tail-1-sysfs.sh

diff --git a/src/tail.c b/src/tail.c
index c45f3b65a..f3219757d 100644
--- a/src/tail.c
+++ b/src/tail.c
@@ -518,19 +518,32 @@ static bool
 file_lines (char const *pretty_filename, int fd, uintmax_t n_lines,
             off_t start_pos, off_t end_pos, uintmax_t *read_pos)
 {
-  char buffer[BUFSIZ];
+  char *buffer;
   size_t bytes_read;
+  blksize_t bufsize = BUFSIZ;
   off_t pos = end_pos;
+  bool ok = true;
+  struct stat stats;
 
   if (n_lines == 0)
     return true;
 
+  if (fstat (fd, &stats) == 0)
+    /* sysfs files have a blksize == kernel page size, which can be > BUFSIZ.
+       lseek SEEK_END moves to the next page, which is likely more than
+       BUFSIZ away from the end of the actual data. Use a buffer large enough
+       to hold an entire page, so that the window we search for data is large
+       enough to find it.  */
+    bufsize = MAX(BUFSIZ, ST_BLKSIZE (stats));
+
+  buffer = xmalloc (bufsize);
+
   /* Set 'bytes_read' to the size of the last, probably partial, buffer;
-     0 < 'bytes_read' <= 'BUFSIZ'.  */
-  bytes_read = (pos - start_pos) % BUFSIZ;
+     0 < 'bytes_read' <= 'bufsize'.  */
+  bytes_read = (pos - start_pos) % bufsize;
   if (bytes_read == 0)
-    bytes_read = BUFSIZ;
-  /* Make 'pos' a multiple of 'BUFSIZ' (0 if the file is short), so that all
+    bytes_read = bufsize;
+  /* Make 'pos' a multiple of 'bufsize' (0 if the file is short), so that all
      reads will be on block boundaries, which might increase efficiency.  */
   pos -= bytes_read;
   xlseek (fd, pos, SEEK_SET, pretty_filename);
@@ -538,7 +551,8 @@ file_lines (char const *pretty_filename, int fd, uintmax_t n_lines,
   if (bytes_read == SAFE_READ_ERROR)
     {
       error (0, errno, _("error reading %s"), quoteaf (pretty_filename));
-      return false;
+      ok = false;
+      goto free_buffer;
     }
   *read_pos = pos + bytes_read;
 
@@ -565,7 +579,7 @@ file_lines (char const *pretty_filename, int fd, uintmax_t n_lines,
               xwrite_stdout (nl + 1, bytes_read - (n + 1));
               *read_pos += dump_remainder (false, pretty_filename, fd,
                                            end_pos - (pos + bytes_read));
-              return true;
+              goto free_buffer;
             }
         }
 
@@ -577,23 +591,26 @@ file_lines (char const *pretty_filename, int fd, uintmax_t n_lines,
           xlseek (fd, start_pos, SEEK_SET, pretty_filename);
           *read_pos = start_pos + dump_remainder (false, pretty_filename, fd,
                                                   end_pos);
-          return true;
+          goto free_buffer;
         }
-      pos -= BUFSIZ;
+      pos -= bufsize;
       xlseek (fd, pos, SEEK_SET, pretty_filename);
 
-      bytes_read = safe_read (fd, buffer, BUFSIZ);
+      bytes_read = safe_read (fd, buffer, bufsize);
       if (bytes_read == SAFE_READ_ERROR)
         {
           error (0, errno, _("error reading %s"), quoteaf (pretty_filename));
-          return false;
+          ok = false;
+          goto free_buffer;
         }
 
       *read_pos = pos + bytes_read;
     }
   while (bytes_read > 0);
 
-  return true;
+free_buffer:
+  free (buffer);
+  return ok;
 }
 
 /* Print the last N_LINES lines from the end of the standard input,
diff --git a/tests/local.mk b/tests/local.mk
index a5fb62d96..af41337b1 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -257,6 +257,7 @@ all_tests =					\
   tests/seq/seq-precision.sh			\
   tests/head/head.pl				\
   tests/head/head-elide-tail.pl			\
+  tests/tail/tail-1-sysfs.sh			\
   tests/tail/tail-n0f.sh			\
   tests/ls/ls-misc.pl				\
   tests/date/date.pl				\
diff --git a/tests/tail/tail-1-sysfs.sh b/tests/tail/tail-1-sysfs.sh
new file mode 100755
index 000000000..078366588
--- /dev/null
+++ b/tests/tail/tail-1-sysfs.sh
@@ -0,0 +1,31 @@
+#!/bin/sh
+# sysfs files have weird properties that can be influenced by page size
+
+# Copyright 2023 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
+print_ver_ tail
+
+file=/sys/kernel/profiling
+
+if test -r $file; then
+  cp -f $file exp || framework_failure_
+  tail -1 $file > out || fail=1
+
+  compare exp out || fail=1
+fi
+
+Exit $fail
-- 
2.43.0

Message #25 received at 67490 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: dann frazier <dann.frazier <at> canonical.com>, 67490 <at> debbugs.gnu.org
Subject: Re: bug#67490: [PATCH v2] tail: fix tailing sysfs files on large page
 kernels
Date: Thu, 30 Nov 2023 20:11:52 +0000

Much clearer thanks.

On my system:

  $ stat /sys/kernel/profiling
    File: /sys/kernel/profiling
    Size: 4096      	Blocks: 0          IO Block: 4096   regular file

I can easily repro by setting the buffer size < PAGE_SIZE.

So this patch handles the case where sysfs reports a file is a certain size,
but it isn't really. In that case seeking to anywhere other than the start
doesn't give an error, but reading returns nothing. So we use a buffer size
large enough (>= PAGE_SIZE as inferred from st_blksize) so that we'll be
reading from the start of the file in this case.

Note st_blksize can have unusual values, so it might be better
to use the io_blksize() wrapper to sanitize the values.
Though that will generally give 128K, which is good when processing all of a file,
but perhaps overkill when processing just the last part of a file.

So perhaps it's better to use buffer_size = MAX (BUFSIZ, getpagesize ())
and avoid all the st_blksize edge cases.

I'll think about it a little, and make the adjustments.

thanks!
Pádraig.

Message #28 received at 67490 <at> debbugs.gnu.org (full text, mbox):

From: dann frazier <dann.frazier <at> canonical.com>
To: Pádraig Brady <P <at> draigbrady.com>
Cc: 67490 <at> debbugs.gnu.org
Subject: Re: bug#67490: [PATCH v2] tail: fix tailing sysfs files on large
 page kernels
Date: Thu, 30 Nov 2023 13:37:25 -0700

On Thu, Nov 30, 2023 at 08:11:52PM +0000, Pádraig Brady wrote:
> Much clearer thanks.
> 
> On my system:
> 
>   $ stat /sys/kernel/profiling
>     File: /sys/kernel/profiling
>     Size: 4096      	Blocks: 0          IO Block: 4096   regular file
> 
> I can easily repro by setting the buffer size < PAGE_SIZE.

Oh, clever.

> So this patch handles the case where sysfs reports a file is a certain size,
> but it isn't really. In that case seeking to anywhere other than the start
> doesn't give an error, but reading returns nothing. So we use a buffer size
> large enough (>= PAGE_SIZE as inferred from st_blksize) so that we'll be
> reading from the start of the file in this case.

Exactly.

> Note st_blksize can have unusual values, so it might be better
> to use the io_blksize() wrapper to sanitize the values.
> Though that will generally give 128K, which is good when processing all of a file,
> but perhaps overkill when processing just the last part of a file.
> 
> So perhaps it's better to use buffer_size = MAX (BUFSIZ, getpagesize ())
> and avoid all the st_blksize edge cases.

Is there any performance concern about switching to 64K reads for
"normal" files (on these systems)?

> I'll think about it a little, and make the adjustments.

Appreciated!

  -dann

> thanks!
> Pádraig.

Message #31 received at 67490 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>,
 dann frazier <dann.frazier <at> canonical.com>, 67490 <at> debbugs.gnu.org
Subject: Re: bug#67490: [PATCH v2] tail: fix tailing sysfs files on large page
 kernels
Date: Thu, 30 Nov 2023 17:54:03 -0800

On 11/30/23 12:11, Pádraig Brady wrote:
> Though that will generally give 128K, which is good when processing all 
> of a file,
> but perhaps overkill when processing just the last part of a file.

The 128 KiB number was computed as being better for apps like 'sed' that 
typically read all or most of the file. 'tail' sometimes behaves that 
way (e.g., 'tail -c +10') and so 'tail' should use 128 KiB in those 
cases. The simplest way to do that is for 'tail' to use 128 KiB all the 
time - that would cost little for uses like plain 'tail' and it could be 
a significant win for uses like 'tail -c +10'.

(As an aside, the 128 KiB number was computed in 2014. These days 256 
KiB might be better if someone could take the time to measure....)

Message #36 received at 67490-done <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>,
 dann frazier <dann.frazier <at> canonical.com>, 67490-done <at> debbugs.gnu.org
Subject: Re: bug#67490: [PATCH v2] tail: fix tailing sysfs files on large page
 kernels
Date: Fri, 1 Dec 2023 23:23:19 +0000

On 01/12/2023 01:54, Paul Eggert wrote:
> On 11/30/23 12:11, Pádraig Brady wrote:
>> Though that will generally give 128K, which is good when processing all
>> of a file,
>> but perhaps overkill when processing just the last part of a file.
> 
> The 128 KiB number was computed as being better for apps like 'sed' that
> typically read all or most of the file. 'tail' sometimes behaves that
> way (e.g., 'tail -c +10') and so 'tail' should use 128 KiB in those
> cases. The simplest way to do that is for 'tail' to use 128 KiB all the
> time - that would cost little for uses like plain 'tail' and it could be
> a significant win for uses like 'tail -c +10'.

Yes I agree we should use io_blksize() in other routines in tail
where we may dump lots of a file. However in this (most common) case
the routine is dealing with the end of a regular file,
so it's probably best to somewhat minimize the amount of data read,
and more directly check the page_size which is issue at hand.
I've pushed the fix at https://github.com/coreutils/coreutils/commit/73d119f4f
where the adjustment (which also corresponds to what we do in wc) is:

  if (sb->st_size % page_size == 0)
    bufsize = MAX (BUFSIZ, page_size);

I'll follow up with another patch to address the performance aspect,
which uses io_blksize() where appropriate.

> (As an aside, the 128 KiB number was computed in 2014. These days 256
> KiB might be better if someone could take the time to measure....)

I periodically check with the documented script,
but I should update the comment when I do that.
I'll update the date in the comment now at least.
I quickly tested a few systems here,
which suggests 256KiB _may_ be a more appropriate default now.
More testing required.

Marking this as done.

thanks,
Pádraig

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.