Re: [Bug-tar] Detection of sparse files is broken on btrfs
diff mbox

Message ID 5923534.bzxSDfjug7@nb.usersys.redhat.com
State New
Headers show

Commit Message

Pavel Raiskup Jan. 9, 2018, 7:46 a.m. UTC
On Monday, January 8, 2018 3:29:17 AM CET Mark H Weaver wrote:
> I propose that we revisit this bug and fix it.  We clearly cannot assume that
> st_blocks == 0 implies that the file contains only zeroes.

. only on btrfs, as far as we know, because of some race condition.  So
what about special casing that filesystem, where we can lseek() for holes
anyway?  Since I would prefer fixing btrfs, I'm CC'ing devels again.

I'm attaching tar patch (public domain, use as you wish) mostly for
discussion about the idea (I can or anybody finalize the ifdef-hell,
etc.).  Note this fixes the failing sparse03.at for me (Fedora 27 x86_64 +
btrfs).

references for btrfs guys:
https://www.mail-archive.com/bug-tar@gnu.org/msg05453.html
https://www.spinics.net/lists/linux-btrfs/msg56768.html

Pavel

Comments

Paul Eggert Jan. 9, 2018, 7:59 a.m. UTC | #1
Pavel Raiskup wrote:
> So
> what about special casing that filesystem, where we can lseek() for holes
> anyway?

If we can lseek for holes, then why not just do that? We shouldn't need 
special-case code for btrfs per se. Any filesystem where we can lseek for holes 
should take advantage of that optimization.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pavel Raiskup Jan. 9, 2018, 8:25 a.m. UTC | #2
On Tuesday, January 9, 2018 8:59:06 AM CET Paul Eggert wrote:
> Pavel Raiskup wrote:
> > So what about special casing that filesystem, where we can lseek() for
> > holes anyway?
> 
> If we can lseek for holes, then why not just do that?

Checking whether lseek() actually works costs some additional syscalls _per
sparse_ file;  checking for ST_NBLOCKS() is without this penalty.

> We shouldn't need special-case code for btrfs per se. Any filesystem
> where we can lseek for holes should take advantage of that optimization.

It is done so actually, the 'wholesparse' is another optimization on top
of that (but usable also in cases where SEEK_HOLE isn't defined at all).

Pavel



--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joerg Schilling Jan. 9, 2018, 10:12 a.m. UTC | #3
Paul Eggert <eggert@cs.ucla.edu> wrote:

> If we can lseek for holes, then why not just do that? We shouldn't need 
> special-case code for btrfs per se. Any filesystem where we can lseek for holes 
> should take advantage of that optimization.

This is what star uses since 13 years ;-)

Jörg
Joerg Schilling Jan. 9, 2018, 10:15 a.m. UTC | #4
Pavel Raiskup <praiskup@redhat.com> wrote:

> On Tuesday, January 9, 2018 8:59:06 AM CET Paul Eggert wrote:
> > Pavel Raiskup wrote:
> > > So what about special casing that filesystem, where we can lseek() for
> > > holes anyway?
> > 
> > If we can lseek for holes, then why not just do that?
>
> Checking whether lseek() actually works costs some additional syscalls _per
> sparse_ file;  checking for ST_NBLOCKS() is without this penalty.

Well, star does this since a long time and the penalty is a few microseconds.

"~A
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pavel Raiskup Jan. 10, 2018, noon UTC | #5
On Tuesday, January 9, 2018 11:15:56 AM CET Joerg Schilling wrote:
> Pavel Raiskup <praiskup@redhat.com> wrote:
> 
> > On Tuesday, January 9, 2018 8:59:06 AM CET Paul Eggert wrote:
> > > Pavel Raiskup wrote:
> > > > So what about special casing that filesystem, where we can lseek() for
> > > > holes anyway?
> > > 
> > > If we can lseek for holes, then why not just do that?
> >
> > Checking whether lseek() actually works costs some additional syscalls _per
> > sparse_ file;  checking for ST_NBLOCKS() is without this penalty.
> 
> Well, star does this since a long time and the penalty is a few microseconds.

It would be interesting to see how network filesystems are affected.

Pavel



--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/src/sparse.c b/src/sparse.c
index d41c0ea..d0a7a55 100644
--- a/src/sparse.c
+++ b/src/sparse.c
@@ -18,6 +18,7 @@ 
 #include <system.h>
 #include <inttostr.h>
 #include <quotearg.h>
+#include <sys/statfs.h>
 #include "common.h"
 
 struct tar_sparse_file;
@@ -261,12 +262,58 @@  sparse_scan_file_raw (struct tar_sparse_file *file)
   return tar_sparse_scan (file, scan_end, NULL);
 }
 
+enum sparse_fs_behavior
+  {
+    sparse_fs_behavior_init = 0,
+    sparse_fs_behavior_fine,
+    sparse_fs_behavior_uncertain
+  };
+
+static enum sparse_fs_behavior
+check_sparse_behavior (int fd)
+{
+  struct statfs buf;
+  if (fstatfs (fd, &buf))
+    return sparse_fs_behavior_fine;
+
+  if (buf.f_type == 0x9123683e)
+    return sparse_fs_behavior_uncertain; /* btrfs */
+
+  return sparse_fs_behavior_fine;
+}
+
+static bool
+wholesparse_detection_prohibited (struct tar_stat_info *st)
+{
+  static dev_t cached_device = 0;
+  static enum sparse_fs_behavior behavior;
+
+  if (behavior == sparse_fs_behavior_init
+      || cached_device != st->stat.st_dev)
+    {
+      cached_device = st->stat.st_dev;
+      behavior = check_sparse_behavior (st->fd);
+    }
+
+  return behavior == sparse_fs_behavior_uncertain;
+}
+
+
 static bool
 sparse_scan_file_wholesparse (struct tar_sparse_file *file)
 {
   struct tar_stat_info *st = file->stat_info;
   struct sp_array sp = {0, 0};
 
+  /* Some file-systems report st_blksize=0 for files which have some
+     inode-inlined data.  This is, per bug-tar@, rather unfortunate
+     behavior, but we need to deal with these filesystems somehow.  So,
+     let's prohibit the "wholesparse" detection method for such filesystems,
+     and let's hope that 'SEEK_HOLE/SEEK_DATA' works (if not, we fallback to
+     slow-but-safe 'raw' method anyway).  */
+  if (wholesparse_detection_prohibited (file->stat_info))
+    return false;
+
   /* Note that this function is called only for truly sparse files of size >= 1
      block size (checked via ST_IS_SPARSE before).  See the thread
      http://www.mail-archive.com/bug-tar@gnu.org/msg04209.html for more info */