Libguestfs - Feb 2020 - Re: [nbdkit PATCH 05/10] plugins: Wire up file-based plugin support for NBD_INFO_INIT_STATE

The NBD protocol is adding an extension to let servers advertise
initialization state to the client: whether the image contains holes,
and whether it is known to read as all zeroes.  For file-based
plugins, we are already probing lseek(SEEK_HOLE) to learn if extents
are supported; a slight tweak to remember if that result is EOF tells
us if we are sparse, and a similar lseek(SEEK_DATA) returning ENXIO
tells us if the entire file is a hole, with at most two lseek calls
during open (rather than one lseek for each of .can_extents,
.init_sparse, and .init_zero).  The split plugin is similar to file.
For partitioning and linuxdisk, we know we are exposing a sparse
image.  For other file-based plugins, like ext2, we do not yet expose
.extents so it is not worth trying to expose .init_sparse or
.init_zero.

A successful test depends on whether the current file system creates
sparse files, but it was easy enough to ensure the test skips rather
than fails when that is not the case.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 plugins/file/file.c                 | 82 ++++++++++++++++++++++++-----
 plugins/linuxdisk/linuxdisk.c       | 13 ++++-
 plugins/partitioning/partitioning.c | 13 ++++-
 plugins/split/split.c               | 59 +++++++++++++++++++--
 tests/Makefile.am                   |  3 +-
 tests/test-file-init.sh             | 69 ++++++++++++++++++++++++
 6 files changed, 219 insertions(+), 20 deletions(-)
 create mode 100755 tests/test-file-init.sh

diff --git a/plugins/file/file.c b/plugins/file/file.c
index 8c2ea07..ca7e2d9 100644
--- a/plugins/file/file.c
+++ b/plugins/file/file.c
@@ -1,5 +1,5 @@
 /* nbdkit
- * Copyright (C) 2013-2019 Red Hat Inc.
+ * Copyright (C) 2013-2020 Red Hat Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
@@ -154,6 +154,9 @@ struct handle {
   bool can_zero_range;
   bool can_fallocate;
   bool can_zeroout;
+  bool can_extents;
+  bool init_sparse;
+  bool init_zero;
 };

 /* Create the per-connection handle. */
@@ -214,6 +217,52 @@ file_open (int readonly)
   h->can_fallocate = true;
   h->can_zeroout = h->is_block_device;

+  h->can_extents = false;
+  h->init_sparse = false;
+  h->init_zero = false;
+#ifdef SEEK_HOLE
+  if (!h->is_block_device) {
+    off_t r;
+
+    /* A simple test to see whether SEEK_DATA/SEEK_HOLE are likely to work on
+     * the current filesystem, and to see if the image is sparse or zero.
+     */
+    ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lseek_lock);
+    r = lseek (h->fd, 0, SEEK_DATA);
+    if (r == -1) {
+      if (errno == ENXIO) {
+        nbdkit_debug ("extents enabled, entire image is hole");
+        h->can_extents = true;
+        h->init_sparse = true;
+        h->init_zero = true;
+      } else {
+        nbdkit_debug ("extents disabled: lseek(SEEK_DATA): %m");
+      }
+    }
+    else {
+      h->can_extents = true;
+      if (r > 0) {
+        nbdkit_debug ("extents enabled, image includes hole before
data");
+        h->init_sparse = true;
+      }
+      else {
+        r = lseek (h->fd, 0, SEEK_HOLE);
+        if (r == -1) {
+          nbdkit_debug ("extents disabled: lseek(SEEK_HOLE): %m");
+          h->can_extents = false;
+        }
+        else if (r == statbuf.st_size) {
+          nbdkit_debug ("extents enabled, image currently all data");
+        }
+        else {
+          nbdkit_debug ("extents enabled, image includes data before
hole");
+          h->init_sparse = true;
+        }
+      }
+    }
+  }
+#endif
+
   return h;
 }

@@ -540,18 +589,8 @@ static int
 file_can_extents (void *handle)
 {
   struct handle *h = handle;
-  off_t r;

-  /* A simple test to see whether SEEK_HOLE etc is likely to work on
-   * the current filesystem.
-   */
-  ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lseek_lock);
-  r = lseek (h->fd, 0, SEEK_HOLE);
-  if (r == -1) {
-    nbdkit_debug ("extents disabled: lseek: SEEK_HOLE: %m");
-    return 0;
-  }
-  return 1;
+  return h->can_extents;
 }

 static int
@@ -622,6 +661,23 @@ file_extents (void *handle, uint32_t count, uint64_t
offset,
   ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lseek_lock);
   return do_extents (handle, count, offset, flags, extents);
 }
+
+/* Initial state. */
+static int
+file_init_sparse (void *handle)
+{
+  struct handle *h = handle;
+
+  return h->init_sparse;
+}
+
+static int
+file_init_zero (void *handle)
+{
+  struct handle *h = handle;
+
+  return h->init_zero;
+}
 #endif /* SEEK_HOLE */

 #if HAVE_POSIX_FADVISE
@@ -668,6 +724,8 @@ static struct nbdkit_plugin plugin = {
 #ifdef SEEK_HOLE
   .can_extents       = file_can_extents,
   .extents           = file_extents,
+  .init_sparse       = file_init_sparse,
+  .init_zero         = file_init_zero,
 #endif
 #if HAVE_POSIX_FADVISE
   .cache             = file_cache,
diff --git a/plugins/linuxdisk/linuxdisk.c b/plugins/linuxdisk/linuxdisk.c
index 99dbc99..0907302 100644
--- a/plugins/linuxdisk/linuxdisk.c
+++ b/plugins/linuxdisk/linuxdisk.c
@@ -1,5 +1,5 @@
 /* nbdkit
- * Copyright (C) 2019 Red Hat Inc.
+ * Copyright (C) 2019-2020 Red Hat Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
@@ -167,6 +167,16 @@ linuxdisk_can_cache (void *handle)
   return NBDKIT_CACHE_EMULATE;
 }

+/* Initial state. */
+static int
+linuxdisk_init_sparse (void *handle)
+{
+  /* region_zero regions mean we are sparse, even if we don't yet
+   * support .extents
+   */
+  return 1;
+}
+
 /* Read data from the virtual disk. */
 static int
 linuxdisk_pread (void *handle, void *buf, uint32_t count, uint64_t offset,
@@ -230,6 +240,7 @@ static struct nbdkit_plugin plugin = {
   .get_size          = linuxdisk_get_size,
   .can_multi_conn    = linuxdisk_can_multi_conn,
   .can_cache         = linuxdisk_can_cache,
+  .init_sparse       = linuxdisk_init_sparse,
   .pread             = linuxdisk_pread,
   .errno_is_preserved = 1,
 };
diff --git a/plugins/partitioning/partitioning.c
b/plugins/partitioning/partitioning.c
index 6e426b9..0b42b2e 100644
--- a/plugins/partitioning/partitioning.c
+++ b/plugins/partitioning/partitioning.c
@@ -1,5 +1,5 @@
 /* nbdkit
- * Copyright (C) 2018-2019 Red Hat Inc.
+ * Copyright (C) 2018-2020 Red Hat Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
@@ -307,6 +307,16 @@ partitioning_can_cache (void *handle)
   return NBDKIT_CACHE_EMULATE;
 }

+/* Initial state. */
+static int
+partitioning_init_sparse (void *handle)
+{
+  /* region_zero regions mean we are sparse, even if we don't yet
+   * support .extents
+   */
+  return 1;
+}
+
 /* Read data. */
 static int
 partitioning_pread (void *handle, void *buf, uint32_t count, uint64_t offset)
@@ -437,6 +447,7 @@ static struct nbdkit_plugin plugin = {
   .get_size          = partitioning_get_size,
   .can_multi_conn    = partitioning_can_multi_conn,
   .can_cache         = partitioning_can_cache,
+  .init_sparse       = partitioning_init_sparse,
   .pread             = partitioning_pread,
   .pwrite            = partitioning_pwrite,
   .flush             = partitioning_flush,
diff --git a/plugins/split/split.c b/plugins/split/split.c
index 70fd4d4..ebb5f5a 100644
--- a/plugins/split/split.c
+++ b/plugins/split/split.c
@@ -97,6 +97,8 @@ split_config (const char *key, const char *value)
 struct handle {
   struct file *files;
   uint64_t size;                /* Total concatenated size. */
+  bool init_sparse;
+  bool init_zero;
 };

 struct file {
@@ -147,6 +149,8 @@ split_open (int readonly)
   }

   offset = 0;
+  h->init_sparse = false; /* set later as needed */
+  h->init_zero = true; /* cleared later as needed */
   for (i = 0; i < nr_files; ++i) {
     h->files[i].offset = offset;

@@ -161,17 +165,44 @@ split_open (int readonly)
                   i, filenames[i], h->files[i].offset, h->files[i].size);

 #ifdef SEEK_HOLE
-    /* Test if this file supports extents. */
+    /* Test if this file supports extents, and for initial state. */
     ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lseek_lock);
     r = lseek (h->files[i].fd, 0, SEEK_DATA);
-    if (r == -1 && errno != ENXIO) {
-      nbdkit_debug ("disabling extents: lseek on %s: %m",
filenames[i]);
-      h->files[i].can_extents = false;
+    if (r == -1) {
+      if (errno == ENXIO) {
+        /* Entire file is hole */
+        h->files[i].can_extents = true;
+        h->init_sparse = true;
+      }
+      else {
+        nbdkit_debug ("disabling extents: lseek(SEEK_DATA) on %s:
%m",
+                      filenames[i]);
+        h->files[i].can_extents = false;
+        h->init_zero = false;
+      }
     }
-    else
+    else {
       h->files[i].can_extents = true;
+      h->init_zero = false;
+      if (r > 0)
+        /* File includes hole before data */
+        h->init_sparse = true;
+      else {
+        r = lseek (h->files[i].fd, 0, SEEK_HOLE);
+        if (r == -1) {
+          nbdkit_debug ("disabling extents: lseek(SEEK_HOLE) on %s:
%m",
+                        filenames[i]);
+          h->files[i].can_extents = false;
+        }
+        else if (r < statbuf.st_size) {
+          /* File includes data before hole */
+          h->init_sparse = true;
+        }
+      }
+    }
 #else
     h->files[i].can_extents = false;
+    h->init_zero = false;
 #endif
   }
   h->size = offset;
@@ -227,6 +258,22 @@ split_can_cache (void *handle)
 #endif
 }

+static int
+split_init_sparse (void *handle)
+{
+  struct handle *h = handle;
+
+  return h->init_sparse;
+}
+
+static int
+split_init_zero (void *handle)
+{
+  struct handle *h = handle;
+
+  return h->init_zero;
+}
+
 /* Helper function to map the offset to the correct file. */
 static int
 compare_offset (const void *offsetp, const void *filep)
@@ -450,6 +497,8 @@ static struct nbdkit_plugin plugin = {
   .close             = split_close,
   .get_size          = split_get_size,
   .can_cache         = split_can_cache,
+  .init_sparse       = split_init_sparse,
+  .init_zero         = split_init_zero,
   .pread             = split_pread,
   .pwrite            = split_pwrite,
 #if HAVE_POSIX_FADVISE
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 4e036a3..a8ff601 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -113,6 +113,7 @@ EXTRA_DIST = \
 	test-export-name.sh \
 	test-extentlist.sh \
 	test-file-extents.sh \
+	test-file-init.sh \
 	test-floppy.sh \
 	test-foreground.sh \
 	test-fua.sh \
@@ -554,7 +555,7 @@ test_file_block_SOURCES = test-file-block.c test.h
 test_file_block_CFLAGS = $(WARNINGS_CFLAGS) $(LIBGUESTFS_CFLAGS)
 test_file_block_LDADD = libtest.la $(LIBGUESTFS_LIBS)

-TESTS += test-file-extents.sh
+TESTS += test-file-extents.sh test-file-init.sh

 # floppy plugin test.
 TESTS += test-floppy.sh
diff --git a/tests/test-file-init.sh b/tests/test-file-init.sh
new file mode 100755
index 0000000..c66ac65
--- /dev/null
+++ b/tests/test-file-init.sh
@@ -0,0 +1,69 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2020 Red Hat Inc.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+# Test the initial state of the file plugin.
+
+source ./functions.sh
+set -e
+
+requires nbdsh -c 'exit (not hasattr (h, "get_init_flags"))'
+requires truncate --help
+requires stat --help
+
+sock=`mktemp -u`
+files="file-init.pid $sock file-init.out"
+rm -f $files
+cleanup_fn rm -f $files
+
+# See if the current file system creates sparse files
+truncate --size=1M file-init.out
+if test "$(stat -c %b file-init.out)" != 0; then
+    echo "$0: unable to create sparse file, skipping this test"
+    exit 77
+fi
+
+# Run nbdkit with the file plugin.
+start_nbdkit -P file-init.pid -U $sock file file-init.out
+
+# The image should start as sparse/zero, before we write to it
+nbdsh --connect "nbd+unix://?socket=$sock" \
+      -c 'assert (h.get_init_flags () == nbd.INIT_SPARSE |
nbd.INIT_ZERO)' \
+      -c 'h.pwrite ("hello world".encode (), 0)'
+
+# The image is still sparse, but no longer zero, before we fill it
+nbdsh --connect "nbd+unix://?socket=$sock" \
+      -c 'assert (h.get_init_flags () == nbd.INIT_SPARSE)' \
+      -c 'h.pwrite (("1" * (1024*1024)).encode (), 0)'
+
+# The image is neither sparse nor zero
+nbdsh --connect "nbd+unix://?socket=$sock" \
+      -c 'assert (h.get_init_flags () == 0)'
-- 
2.24.1

On Mon, Feb 10, 2020 at 03:43:58PM -0600, Eric Blake
wrote:
> @@ -214,6 +217,52 @@ file_open (int readonly)
>    h->can_fallocate = true;
>    h->can_zeroout = h->is_block_device;
> 
> +  h->can_extents = false;
> +  h->init_sparse = false;
> +  h->init_zero = false;
> +#ifdef SEEK_HOLE
> +  if (!h->is_block_device) {
> +    off_t r;
> +
> +    /* A simple test to see whether SEEK_DATA/SEEK_HOLE are likely to work
on
> +     * the current filesystem, and to see if the image is sparse or zero.
> +     */
> +    ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lseek_lock);
> +    r = lseek (h->fd, 0, SEEK_DATA);
> +    if (r == -1) {
> +      if (errno == ENXIO) {
> +        nbdkit_debug ("extents enabled, entire image is hole");
> +        h->can_extents = true;
> +        h->init_sparse = true;
> +        h->init_zero = true;
> +      } else {
> +        nbdkit_debug ("extents disabled: lseek(SEEK_DATA): %m");
> +      }
> +    }
> +    else {
> +      h->can_extents = true;
> +      if (r > 0) {
> +        nbdkit_debug ("extents enabled, image includes hole before
data");
> +        h->init_sparse = true;
> +      }
> +      else {
> +        r = lseek (h->fd, 0, SEEK_HOLE);
> +        if (r == -1) {
> +          nbdkit_debug ("extents disabled: lseek(SEEK_HOLE):
%m");
> +          h->can_extents = false;
> +        }
> +        else if (r == statbuf.st_size) {
> +          nbdkit_debug ("extents enabled, image currently all
data");
> +        }
> +        else {
> +          nbdkit_debug ("extents enabled, image includes data before
hole");
> +          h->init_sparse = true;
> +        }
> +      }
> +    }
> +  }
In the non-block case, can't we stat(2) the file and look at
statbuf.st_blocks?
> +requires nbdsh -c 'exit (not hasattr (h,
"get_init_flags"))'
Not wrong, but it's easier to do it like this:

https://github.com/libguestfs/nbdkit/blob/5e4745641bb4676f607fdb3f8750dbf6e9516877/tests/test-vsock.sh#L48

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

On 2/11/20 4:52 AM, Richard W.M. Jones wrote:
> On Mon, Feb 10, 2020 at 03:43:58PM -0600, Eric Blake wrote:
>> @@ -214,6 +217,52 @@ file_open (int readonly)
>>     h->can_fallocate = true;
>>     h->can_zeroout = h->is_block_device;
>>
>> +  h->can_extents = false;
>> +  h->init_sparse = false;
>> +  h->init_zero = false;
>> +#ifdef SEEK_HOLE
>> +        else if (r == statbuf.st_size) {
>> +          nbdkit_debug ("extents enabled, image currently all
data");
>> +        }
>> +        else {
>> +          nbdkit_debug ("extents enabled, image includes data
before hole");
>> +          h->init_sparse = true;
>> +        }
>> +      }
>> +    }
>> +  }
> 
> In the non-block case, can't we stat(2) the file and look at
statbuf.st_blocks?
True.  In fact, we already did do fstat(), so the information is already 
there.

Note that st_blocks == 0 proves a file is sparse.  But POSIX says 
st_block is not necessarily using the same units as st_blksize, so 
checking st_block * st_blksize < st_size is NOT necessarily evidence 
that a file is sparse.  What's worse, there are some file systems where 
first- and second-level indirect blocks are used for large files, and 
where those indirects are counted towards st_block (but NOT towards 
st_size), so it is possible to have a sparse file where st_block * 
st_blksize > st_size but the file is still sparse.  In short, proving 
that a file has holes is easy with lseek(SEEK_HOLE), but only a 
best-guess heuristic with stat() data.  That said, you're right that it 
is at least worth adding the heuristics in the !SEEK_HOLE case.
> 
>> +requires nbdsh -c 'exit (not hasattr (h,
"get_init_flags"))'
> 
> Not wrong, but it's easier to do it like this:
> 
>
https://github.com/libguestfs/nbdkit/blob/5e4745641bb4676f607fdb3f8750dbf6e9516877/tests/test-vsock.sh#L48
Thanks, that is cleaner.  I will update it (along with the updates 
required by renaming it to two "is_init_*" functions) (affects several
patches in this series).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org