nbdkit test-checkwrite.sh failing occasionally on ppc64le
It's not 100% reproducible, but it seems to happen reasonably often. Unfortunately it only happens on ppc64le (so far). The error message lacks sufficient detail to tell what's going on. nbdkit: backend.c:624: backend_pread: Assertion `backend_valid_range (c, offset, count)' failed. nbdkit: backend.c:624: backend_pread: Assertion `backend_valid_range (c, offset, count)' failed. Full log attached. 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 07/13/22 13:27, Richard W.M. Jones wrote:
It's not 100% reproducible, but it seems to happen reasonably often. Unfortunately it only happens on ppc64le (so far). The error message lacks sufficient detail to tell what's going on.
nbdkit: backend.c:624: backend_pread: Assertion `backend_valid_range (c, offset, count)' failed. nbdkit: backend.c:624: backend_pread: Assertion `backend_valid_range (c, offset, count)' failed.
Full log attached.
"tests/disk" has size 105923072. Considering all the count=... offset=... lines in the last section of the log, there isn't one line where backend_valid_range(): return count > 0 && offset <= c->exportsize && offset + count <= c->exportsize; would return false. I've printed a table, with offset, count, and offset+count colums, from the log entries, then sorted the table three times separately, each time by a different column. I got the following min and max elements: offset count offset+count --------- --------- ----------- 0 105923072 105923072 (min by offset) 105906176 16896 105923072 (max by offset) 0 4096 4096 (min by count) 0 105923072 105923072 (max by count) 0 4096 4096 (min by end) 105906176 16896 105923072 (max by end) All of these look valid... I think you'll have to look at the coredump with gdb. Laszlo
On Wed, Jul 13, 2022 at 03:10:37PM +0200, Laszlo Ersek wrote:
On 07/13/22 13:27, Richard W.M. Jones wrote:
It's not 100% reproducible, but it seems to happen reasonably often. Unfortunately it only happens on ppc64le (so far). The error message lacks sufficient detail to tell what's going on.
nbdkit: backend.c:624: backend_pread: Assertion `backend_valid_range (c, offset, count)' failed. nbdkit: backend.c:624: backend_pread: Assertion `backend_valid_range (c, offset, count)' failed.
Full log attached.
"tests/disk" has size 105923072.
Considering all the count=... offset=... lines in the last section of the log, there isn't one line where backend_valid_range():
return count > 0 && offset <= c->exportsize && offset + count <= c->exportsize;
would return false.
I've printed a table, with offset, count, and offset+count colums, from the log entries, then sorted the table three times separately, each time by a different column. I got the following min and max elements:
offset count offset+count --------- --------- ----------- 0 105923072 105923072 (min by offset) 105906176 16896 105923072 (max by offset) 0 4096 4096 (min by count) 0 105923072 105923072 (max by count) 0 4096 4096 (min by end) 105906176 16896 105923072 (max by end)
All of these look valid... I think you'll have to look at the coredump with gdb.
Indeed - unfortunately the coredump is on a Fedora build system :-( I'll reserve a POWER machine later (unless someone gets there first). Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On Wed, Jul 13, 2022 at 02:13:14PM +0100, Richard W.M. Jones wrote:
On Wed, Jul 13, 2022 at 03:10:37PM +0200, Laszlo Ersek wrote:
On 07/13/22 13:27, Richard W.M. Jones wrote:
It's not 100% reproducible, but it seems to happen reasonably often. Unfortunately it only happens on ppc64le (so far). The error message lacks sufficient detail to tell what's going on.
nbdkit: backend.c:624: backend_pread: Assertion `backend_valid_range (c, offset, count)' failed. nbdkit: backend.c:624: backend_pread: Assertion `backend_valid_range (c, offset, count)' failed.
Full log attached.
"tests/disk" has size 105923072.
Considering all the count=... offset=... lines in the last section of the log, there isn't one line where backend_valid_range():
return count > 0 && offset <= c->exportsize && offset + count <= c->exportsize;
would return false.
I've printed a table, with offset, count, and offset+count colums, from the log entries, then sorted the table three times separately, each time by a different column. I got the following min and max elements:
offset count offset+count --------- --------- ----------- 0 105923072 105923072 (min by offset) 105906176 16896 105923072 (max by offset) 0 4096 4096 (min by count) 0 105923072 105923072 (max by count) 0 4096 4096 (min by end) 105906176 16896 105923072 (max by end)
All of these look valid... I think you'll have to look at the coredump with gdb.
Indeed - unfortunately the coredump is on a Fedora build system :-(
I'll reserve a POWER machine later (unless someone gets there first).
I can't reproduce this after a few hours. I'll leave it running overnight, but the bug seems peculiarly rare on this reserved system, versus Fedora Koji where I observed it quite frequently. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
Arrrgh! https://koji.fedoraproject.org/koji/taskinfo?taskID=89618509 FAIL: test-checkwrite.sh Last week I reserved a ppc64le machine which had almost exactly the same spec as one of the Koji builders and ran both the full test suite and this single test thousands of times over a few days and it didn't fail even once. Ideas gratefully accepted :-/ Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
No idea why this was only failing on ppc64le, since I've finally got a reproducer that works everywhere (see attached). $ ./bound.sh + rm -f /tmp/sock + sleep 1 + nbdkit --exit-with-parent -U /tmp/sock sh - --filter=checkwrite + nbdsh -u 'nbd+unix:///?socket=/tmp/sock' -c 'h.zero (655360, 262144, 0)' nbdkit: backend.c:624: backend_pread: Assertion `backend_valid_range (c, offset, count)' failed. nbdsh: command line script failed: nbd_zero: write-zeroes: command failed: Transport endpoint is not connected ./bound.sh: line 24: 2004832 Aborted (core dumped) nbdkit --exit-with-parent -U /tmp/sock sh - --filter=checkwrite <<'EOF' I'm not quite sure how to fix it yet, but it involves this code: https://gitlab.com/nbdkit/nbdkit/-/blob/33cdb16e65c59b5989ad967ec0cccb2ad249... When we get to the end of one extent, this loop sets buflen = 0 and calls next->pread (...count = 0) (which causes the assert fail), instead of moving to the next extent. I'll take a look when I'm back at work. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
Fixed in: https://gitlab.com/nbdkit/nbdkit/-/commit/16a13fa9e1f3c4cbc0fae89e364f832e1a... https://gitlab.com/nbdkit/nbdkit/-/commit/2c9a557f2ebe5a8647cc44dbdd7c157356... https://gitlab.com/nbdkit/nbdkit/-/commit/aaf5484462cd06505b7a1a8103a899d355... -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On 07/19/22 11:45, Richard W.M. Jones wrote:
Fixed in: https://gitlab.com/nbdkit/nbdkit/-/commit/16a13fa9e1f3c4cbc0fae89e364f832e1a... https://gitlab.com/nbdkit/nbdkit/-/commit/2c9a557f2ebe5a8647cc44dbdd7c157356... https://gitlab.com/nbdkit/nbdkit/-/commit/aaf5484462cd06505b7a1a8103a899d355...
I think the new assert from commit aaf5484462cd is wrong; from the filter's perspective, it is OK if the trim/zero request extends beyond the currently allocated disk size. The checkwrite_trim_zero() function starts with this comment:
/* Trim and zero are effectively the same operation for this plugin. * We have to check that the underlying plugin contains all zeroes. * * Note we don't check that the extents exactly match since a valid * copying operation is to either add sparseness (qemu-img convert -S) * or create a fully allocated target (nbdcopy --allocated).
The note states that the trim request/zero request (i.e., "count") may extend beyond the extent list that is available from the underlying plugin. IOW, I think nbdkit_extents_full() does not guarantee that the final extent ends exactly where "count" does. Here's the function:
/* This is a convenient wrapper around next_ops->extents which can be * used from filters where you want to get a complete set of extents * covering the region [offset..offset+count-1]. */ NBDKIT_DLL_PUBLIC struct nbdkit_extents * nbdkit_extents_full (struct context *next_c, uint32_t count, uint64_t offset, uint32_t flags, int *err) { struct nbdkit_next_ops *next = &next_c->next; struct nbdkit_extents *ret;
/* Clear REQ_ONE to ask the plugin for as much information as it is * willing to return (the plugin may still truncate if it is too * costly to provide everything). */ flags &= ~NBDKIT_FLAG_REQ_ONE;
ret = nbdkit_extents_new (offset, offset+count);
So this sets ret->start=offset and ret->end=offset+count, and ret->extents to the empty vector.
if (ret == NULL) goto error0;
while (count > 0) { const uint64_t old_offset = offset; size_t i;
CLEANUP_EXTENTS_FREE struct nbdkit_extents *t = nbdkit_extents_new (offset, offset+count); if (t == NULL) goto error1;
if (next->extents (next_c, count, offset, flags, t, err) == -1) goto error0;
for (i = 0; i < nbdkit_extents_count (t); ++i) { const struct nbdkit_extent e = nbdkit_get_extent (t, i); if (nbdkit_add_extent (ret, e.offset, e.length, e.type) == -1) goto error1;
So here we add the underlying plugin's extents one by one, trimming each in nbdkit_add_extent() if necessary. However, if the source extent list does not extend up to offset+count, then our output ("ret") extent list will also not extend up to that point. One example: what if the input file is completely empty (zero bytes in size)? I think that would even mean an extent list that has 0 entries. Thus, I think the properties of the extent list produced by nbdkit_extents_full() are crucial. They are: - There may be 0 extents in the extent list. - The first extent starts precisely at "offset". - The (exclusive) end of the last extent is *less than or equal to* "offset+count". Based on these properties (especially the last two), I think we should / could modify the checkwrite_trim_zero() function as follows:
diff --git a/filters/checkwrite/checkwrite.c b/filters/checkwrite/checkwrite.c index ce8fa137aa80..9abe64ca7aa0 100644 --- a/filters/checkwrite/checkwrite.c +++ b/filters/checkwrite/checkwrite.c @@ -164,91 +164,84 @@ static int checkwrite_trim_zero (nbdkit_next *next, void *handle, uint32_t count, uint64_t offset, uint32_t flags, int *err) { /* If the plugin supports extents, speed this up by using them. */ if (next->can_extents (next)) { size_t i, n; CLEANUP_EXTENTS_FREE struct nbdkit_extents *exts = nbdkit_extents_full (next, count, offset, 0, err); if (exts == NULL) return -1;
n = nbdkit_extents_count (exts); - for (i = 0; i < n && count > 0; ++i) { + for (i = 0; i < n; ++i) { const struct nbdkit_extent e = nbdkit_get_extent (exts, i); - const uint64_t next_extent_offset = e.offset + e.length; + uint64_t left_in_extent = e.length; + + assert (count >= left_in_extent);
/* Anything that reads back as zero is good. */ if ((e.type & NBDKIT_EXTENT_ZERO) != 0) { - const uint64_t zerolen = MIN (count, next_extent_offset - offset); - - offset += zerolen; - count -= zerolen; + offset += left_in_extent; + count -= left_in_extent; continue; }
/* Otherwise we have to read the underlying data and check. */ if (flags & NBDKIT_FLAG_FAST_ZERO) { *err = ENOTSUP; return -1; } - while (offset < next_extent_offset && count > 0) { - size_t buflen = MIN (MAX_REQUEST_SIZE, count); - buflen = MIN (buflen, next_extent_offset - offset); + while (left_in_extent > 0) { + size_t buflen = MIN (MAX_REQUEST_SIZE, left_in_extent);
CLEANUP_FREE char *buf = malloc (buflen); if (buf == NULL) { *err = errno; nbdkit_error ("malloc: %m"); return -1; }
if (next->pread (next, buf, buflen, offset, 0, err) == -1) return -1; if (! is_zero (buf, buflen)) return data_does_not_match (err);
count -= buflen; offset += buflen; + left_in_extent -= buflen; } } /* for extent */ - - /* Assert that the loop above has actually checked the whole - * region. If this fires then it could be because - * nbdkit_extents_full isn't returning a full range of extents for - * the whole region ... or maybe the loop above is wrong. - */ - assert (count == 0); }
/* Otherwise the plugin does not support extents, so do this the * slow way. */ else { ... }
return 0; }
Note that, with this change, the *only* things we use "count" for are: - the initial nbdkit_extents_full() call, - an assertion that we're still "inside count". This makes sense, based on the properties of nbdkit_extents_full(). nbdkit_extents_full() gives us an extent list that starts precisely at "offset", and extends as far as possible towards "offset+count". Once we have those extents, it's enough to just check the extents' contents; we can ignore "count" altogether. If the current extent is a zero extent, we can skip it in full; if it is a data extent, then we need to chunk it with MAX_REQUEST_SIZE. Either way, "count" will never run out. (NB: the "assert (count == 0)" is an independent revert of aaf5484462cd.) A more aggressive simplification would be (for the same end goal) the following. This eliminates the usage of the "count" variable altogether, except in the initial nbdkit_extents_full() call:
diff --git a/filters/checkwrite/checkwrite.c b/filters/checkwrite/checkwrite.c index ce8fa137aa80..9dac501904eb 100644 --- a/filters/checkwrite/checkwrite.c +++ b/filters/checkwrite/checkwrite.c @@ -164,91 +164,80 @@ static int checkwrite_trim_zero (nbdkit_next *next, void *handle, uint32_t count, uint64_t offset, uint32_t flags, int *err) { /* If the plugin supports extents, speed this up by using them. */ if (next->can_extents (next)) { size_t i, n; CLEANUP_EXTENTS_FREE struct nbdkit_extents *exts = nbdkit_extents_full (next, count, offset, 0, err); if (exts == NULL) return -1;
n = nbdkit_extents_count (exts); - for (i = 0; i < n && count > 0; ++i) { + for (i = 0; i < n; ++i) { const struct nbdkit_extent e = nbdkit_get_extent (exts, i); - const uint64_t next_extent_offset = e.offset + e.length; + uint64_t left_in_extent = e.length;
/* Anything that reads back as zero is good. */ if ((e.type & NBDKIT_EXTENT_ZERO) != 0) { - const uint64_t zerolen = MIN (count, next_extent_offset - offset); - - offset += zerolen; - count -= zerolen; + offset += left_in_extent; continue; }
/* Otherwise we have to read the underlying data and check. */ if (flags & NBDKIT_FLAG_FAST_ZERO) { *err = ENOTSUP; return -1; } - while (offset < next_extent_offset && count > 0) { - size_t buflen = MIN (MAX_REQUEST_SIZE, count); - buflen = MIN (buflen, next_extent_offset - offset); + while (left_in_extent > 0) { + size_t buflen = MIN (MAX_REQUEST_SIZE, left_in_extent);
CLEANUP_FREE char *buf = malloc (buflen); if (buf == NULL) { *err = errno; nbdkit_error ("malloc: %m"); return -1; }
if (next->pread (next, buf, buflen, offset, 0, err) == -1) return -1; if (! is_zero (buf, buflen)) return data_does_not_match (err);
- count -= buflen; offset += buflen; + left_in_extent -= buflen; } } /* for extent */ - - /* Assert that the loop above has actually checked the whole - * region. If this fires then it could be because - * nbdkit_extents_full isn't returning a full range of extents for - * the whole region ... or maybe the loop above is wrong. - */ - assert (count == 0); }
/* Otherwise the plugin does not support extents, so do this the * slow way. */ else { ... }
return 0; }
Laszlo
On Tue, Jul 19, 2022 at 03:21:05PM +0200, Laszlo Ersek wrote:
On 07/19/22 11:45, Richard W.M. Jones wrote:
Fixed in: https://gitlab.com/nbdkit/nbdkit/-/commit/16a13fa9e1f3c4cbc0fae89e364f832e1a... https://gitlab.com/nbdkit/nbdkit/-/commit/2c9a557f2ebe5a8647cc44dbdd7c157356... https://gitlab.com/nbdkit/nbdkit/-/commit/aaf5484462cd06505b7a1a8103a899d355...
I think the new assert from commit aaf5484462cd is wrong;
[linking to the assert in the context of the function: https://gitlab.com/nbdkit/nbdkit/-/blob/master/filters/checkwrite/checkwrite...
from the filter's perspective, it is OK if the trim/zero request extends beyond the currently allocated disk size. The checkwrite_trim_zero() function starts with this comment:
/* Trim and zero are effectively the same operation for this plugin. * We have to check that the underlying plugin contains all zeroes. * * Note we don't check that the extents exactly match since a valid * copying operation is to either add sparseness (qemu-img convert -S) * or create a fully allocated target (nbdcopy --allocated).
The note states that the trim request/zero request (i.e., "count") may extend beyond the extent list that is available from the underlying plugin.
IOW, I think nbdkit_extents_full() does not guarantee that the final extent ends exactly where "count" does. Here's the function:
/* This is a convenient wrapper around next_ops->extents which can be * used from filters where you want to get a complete set of extents * covering the region [offset..offset+count-1]. */ NBDKIT_DLL_PUBLIC struct nbdkit_extents * nbdkit_extents_full (struct context *next_c, uint32_t count, uint64_t offset, uint32_t flags, int *err) { struct nbdkit_next_ops *next = &next_c->next; struct nbdkit_extents *ret;
/* Clear REQ_ONE to ask the plugin for as much information as it is * willing to return (the plugin may still truncate if it is too * costly to provide everything). */ flags &= ~NBDKIT_FLAG_REQ_ONE;
ret = nbdkit_extents_new (offset, offset+count);
So this sets ret->start=offset and ret->end=offset+count, and ret->extents to the empty vector.
if (ret == NULL) goto error0;
while (count > 0) { const uint64_t old_offset = offset; size_t i;
CLEANUP_EXTENTS_FREE struct nbdkit_extents *t = nbdkit_extents_new (offset, offset+count); if (t == NULL) goto error1;
if (next->extents (next_c, count, offset, flags, t, err) == -1) goto error0;
for (i = 0; i < nbdkit_extents_count (t); ++i) { const struct nbdkit_extent e = nbdkit_get_extent (t, i); if (nbdkit_add_extent (ret, e.offset, e.length, e.type) == -1) goto error1;
So here we add the underlying plugin's extents one by one, trimming each in nbdkit_add_extent() if necessary. However, if the source extent list does not extend up to offset+count, then our output ("ret") extent list will also not extend up to that point.
One example: what if the input file is completely empty (zero bytes in size)? I think that would even mean an extent list that has 0 entries.
My understanding of the contract of nbdkit_extents_full is that it should always return information to the end of offset+count-1. Moreover a plugin which advertises can_extents == true should always return at least one extent for any request within the range of the disk, this is the "a client can always make progress" requirement in the spec: https://github.com/NetworkBlockDevice/nbd/blob/master/doc/proto.md#request-t... (I cannot link to it directly, but if you search for those words you'll find it.) In fact you can observe this if you try to create an nbdkit plugin which doesn't return a full set of extents covering the disk, nbdkit prints an error and returns an EINVAL to the client: $ nbdkit -U - eval \ get_size=' echo 1048576 ' \ pread=' dd if=/dev/zero count=$3 iflag=count_bytes ' \ extents=' echo 0 262144 3 ' \ --run 'nbdinfo --map "$uri"' nbdkit: eval[1]: error: extents: plugin must return at least one extent nbdinfo: nbd_block_status: block-status: command failed: Invalid argument (I used the same plugin with checkwrite and it gave the same error from nbdkit_extents_full). Anyway if that's true then I believe I have a different and simpler argument for keeping the current assertion: - When we call checkwrite_trim_zero, the client is asking to trim / zero the range [offset..offset+count-1] - To check this operation is valid, checkwrite must inspect every byte in the range, whether that is by using extents or pread or whatever. - At each place in the code, count is decremented (and offset incremented) by exactly the amount of bytes that checkwrite has inspected. - Therefore at the end of this, count must be zero when we leave the function. (We could validly move the assert later, but it's kind of obvious already for the else-clause.) Note I'm not claiming the assertion will not fail! Just that the assertion is correct, and if it fails we've got a bug. Rich.
Thus, I think the properties of the extent list produced by nbdkit_extents_full() are crucial. They are:
- There may be 0 extents in the extent list.
- The first extent starts precisely at "offset".
- The (exclusive) end of the last extent is *less than or equal to* "offset+count".
Based on these properties (especially the last two), I think we should / could modify the checkwrite_trim_zero() function as follows:
diff --git a/filters/checkwrite/checkwrite.c b/filters/checkwrite/checkwrite.c index ce8fa137aa80..9abe64ca7aa0 100644 --- a/filters/checkwrite/checkwrite.c +++ b/filters/checkwrite/checkwrite.c @@ -164,91 +164,84 @@ static int checkwrite_trim_zero (nbdkit_next *next, void *handle, uint32_t count, uint64_t offset, uint32_t flags, int *err) { /* If the plugin supports extents, speed this up by using them. */ if (next->can_extents (next)) { size_t i, n; CLEANUP_EXTENTS_FREE struct nbdkit_extents *exts = nbdkit_extents_full (next, count, offset, 0, err); if (exts == NULL) return -1;
n = nbdkit_extents_count (exts); - for (i = 0; i < n && count > 0; ++i) { + for (i = 0; i < n; ++i) { const struct nbdkit_extent e = nbdkit_get_extent (exts, i); - const uint64_t next_extent_offset = e.offset + e.length; + uint64_t left_in_extent = e.length; + + assert (count >= left_in_extent);
/* Anything that reads back as zero is good. */ if ((e.type & NBDKIT_EXTENT_ZERO) != 0) { - const uint64_t zerolen = MIN (count, next_extent_offset - offset); - - offset += zerolen; - count -= zerolen; + offset += left_in_extent; + count -= left_in_extent; continue; }
/* Otherwise we have to read the underlying data and check. */ if (flags & NBDKIT_FLAG_FAST_ZERO) { *err = ENOTSUP; return -1; } - while (offset < next_extent_offset && count > 0) { - size_t buflen = MIN (MAX_REQUEST_SIZE, count); - buflen = MIN (buflen, next_extent_offset - offset); + while (left_in_extent > 0) { + size_t buflen = MIN (MAX_REQUEST_SIZE, left_in_extent);
CLEANUP_FREE char *buf = malloc (buflen); if (buf == NULL) { *err = errno; nbdkit_error ("malloc: %m"); return -1; }
if (next->pread (next, buf, buflen, offset, 0, err) == -1) return -1; if (! is_zero (buf, buflen)) return data_does_not_match (err);
count -= buflen; offset += buflen; + left_in_extent -= buflen; } } /* for extent */ - - /* Assert that the loop above has actually checked the whole - * region. If this fires then it could be because - * nbdkit_extents_full isn't returning a full range of extents for - * the whole region ... or maybe the loop above is wrong. - */ - assert (count == 0); }
/* Otherwise the plugin does not support extents, so do this the * slow way. */ else { ... }
return 0; }
Note that, with this change, the *only* things we use "count" for are:
- the initial nbdkit_extents_full() call,
- an assertion that we're still "inside count".
This makes sense, based on the properties of nbdkit_extents_full(). nbdkit_extents_full() gives us an extent list that starts precisely at "offset", and extends as far as possible towards "offset+count". Once we have those extents, it's enough to just check the extents' contents; we can ignore "count" altogether.
If the current extent is a zero extent, we can skip it in full; if it is a data extent, then we need to chunk it with MAX_REQUEST_SIZE. Either way, "count" will never run out.
(NB: the "assert (count == 0)" is an independent revert of aaf5484462cd.)
A more aggressive simplification would be (for the same end goal) the following. This eliminates the usage of the "count" variable altogether, except in the initial nbdkit_extents_full() call:
diff --git a/filters/checkwrite/checkwrite.c b/filters/checkwrite/checkwrite.c index ce8fa137aa80..9dac501904eb 100644 --- a/filters/checkwrite/checkwrite.c +++ b/filters/checkwrite/checkwrite.c @@ -164,91 +164,80 @@ static int checkwrite_trim_zero (nbdkit_next *next, void *handle, uint32_t count, uint64_t offset, uint32_t flags, int *err) { /* If the plugin supports extents, speed this up by using them. */ if (next->can_extents (next)) { size_t i, n; CLEANUP_EXTENTS_FREE struct nbdkit_extents *exts = nbdkit_extents_full (next, count, offset, 0, err); if (exts == NULL) return -1;
n = nbdkit_extents_count (exts); - for (i = 0; i < n && count > 0; ++i) { + for (i = 0; i < n; ++i) { const struct nbdkit_extent e = nbdkit_get_extent (exts, i); - const uint64_t next_extent_offset = e.offset + e.length; + uint64_t left_in_extent = e.length;
/* Anything that reads back as zero is good. */ if ((e.type & NBDKIT_EXTENT_ZERO) != 0) { - const uint64_t zerolen = MIN (count, next_extent_offset - offset); - - offset += zerolen; - count -= zerolen; + offset += left_in_extent; continue; }
/* Otherwise we have to read the underlying data and check. */ if (flags & NBDKIT_FLAG_FAST_ZERO) { *err = ENOTSUP; return -1; } - while (offset < next_extent_offset && count > 0) { - size_t buflen = MIN (MAX_REQUEST_SIZE, count); - buflen = MIN (buflen, next_extent_offset - offset); + while (left_in_extent > 0) { + size_t buflen = MIN (MAX_REQUEST_SIZE, left_in_extent);
CLEANUP_FREE char *buf = malloc (buflen); if (buf == NULL) { *err = errno; nbdkit_error ("malloc: %m"); return -1; }
if (next->pread (next, buf, buflen, offset, 0, err) == -1) return -1; if (! is_zero (buf, buflen)) return data_does_not_match (err);
- count -= buflen; offset += buflen; + left_in_extent -= buflen; } } /* for extent */ - - /* Assert that the loop above has actually checked the whole - * region. If this fires then it could be because - * nbdkit_extents_full isn't returning a full range of extents for - * the whole region ... or maybe the loop above is wrong. - */ - assert (count == 0); }
/* Otherwise the plugin does not support extents, so do this the * slow way. */ else { ... }
return 0; }
Laszlo
-- 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 07/19/22 16:17, Richard W.M. Jones wrote:
Note I'm not claiming the assertion will not fail! Just that the assertion is correct, and if it fails we've got a bug.
Fair enough, but how do we explain the second paragraph of the leading comment on the function then? * Note we don't check that the extents exactly match since a valid * copying operation is to either add sparseness (qemu-img convert -S) * or create a fully allocated target (nbdcopy --allocated). I think it directly contradicts the assertion. That, or I don't understand the comment at all. (It's from commit 2d0954dd6431, "New filter: checkwrite", 2020-12-22.) Laszlo
On Tue, Jul 19, 2022 at 04:41:02PM +0200, Laszlo Ersek wrote:
On 07/19/22 16:17, Richard W.M. Jones wrote:
Note I'm not claiming the assertion will not fail! Just that the assertion is correct, and if it fails we've got a bug.
Fair enough, but how do we explain the second paragraph of the leading comment on the function then?
* Note we don't check that the extents exactly match since a valid * copying operation is to either add sparseness (qemu-img convert -S) * or create a fully allocated target (nbdcopy --allocated).
This is slightly different from the issue of the range covered by extents, but it does require explanation. What I meant here was that we treat an extent marked with the zero flag, and an extent marked with the data flag[0] that returns zeroes when read as being the same thing. However extents returned by nbdkit_extents_full are still assumed to cover the entire range of [offset..offset+count-1]. The following disks would be logically equivalent[1]: $ truncate -s 1M empty $ nbdkit file empty --run 'nbdinfo --map $uri' 0 1048576 3 hole,zero $ dd if=/dev/zero of=empty2 count=1 bs=1M $ nbdkit file empty2 --run 'nbdinfo --map $uri' 0 1048576 0 data nbdkit-checkwrite-filter should permit you to either write zeroes (using pwrite), trim or zero (or any combination) both of those disks. It should reject with EIO attempts to write non-zeroes anywhere. The reason for this is because this filter is designed for testing copying tools like qemu-img convert & nbdcopy. You could back it with a local file like one of the ones above, but more fun is to use a randomly created virtual disk, eg: $ nbdkit linuxdisk ~/d/nbdkit --filter=checkwrite --run 'nbdcopy "$uri" "$uri"' $ nbdkit pattern 1G --filter=checkwrite --run 'nbdcopy --allocated "$uri" "$uri"' Because those tools commonly sparsify (-S) or allocate (--allocated) when they copy, what is written can contain trims or zeroes where the source disk had data reading as zero, or vice versa. Some fun facts ... Some plugins already have the equivalent of checkwrite functionality, eg this just works without the filter: $ nbdkit sparse-random 1T --run 'qemu-img convert -p "$uri" "$uri"' qemu-img has new(?) behaviour that breaks some things: $ nbdkit linuxdisk ~/d/nbdkit --filter=checkwrite --run 'qemu-img convert "$uri" "$uri"' nbdkit: linuxdisk.0: error: data written does not match expected qemu-img: nbd://localhost:10809: error while converting raw: Failed to clear the new image's first sector: Input/output error I understand why this happens, but also it's kind of a bug ... We should probably modify checkwrite to handle this, but it's rather complicated.
I think it directly contradicts the assertion. That, or I don't understand the comment at all.
(It's from commit 2d0954dd6431, "New filter: checkwrite", 2020-12-22.)
Laszlo
Rich. [0] More precisely: an extent with type == 0. [1] This is actually the same way that qemu-img compare works. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
On 07/19/22 17:45, Richard W.M. Jones wrote:
On Tue, Jul 19, 2022 at 04:41:02PM +0200, Laszlo Ersek wrote:
On 07/19/22 16:17, Richard W.M. Jones wrote:
Note I'm not claiming the assertion will not fail! Just that the assertion is correct, and if it fails we've got a bug.
Fair enough, but how do we explain the second paragraph of the leading comment on the function then?
* Note we don't check that the extents exactly match since a valid * copying operation is to either add sparseness (qemu-img convert -S) * or create a fully allocated target (nbdcopy --allocated).
This is slightly different from the issue of the range covered by extents, but it does require explanation.
What I meant here was that we treat an extent marked with the zero flag, and an extent marked with the data flag[0] that returns zeroes when read as being the same thing.
Ah, I completely misunderstood the comment then. By "exact match" you meant "extent type match" (which we don't need), but I understood it as "exact range match". (I did understand that we accepted both zero extents, and zero-filled data extents, as a match at the data level.) Thanks! Laszlo
However extents returned by nbdkit_extents_full are still assumed to cover the entire range of [offset..offset+count-1].
The following disks would be logically equivalent[1]:
$ truncate -s 1M empty $ nbdkit file empty --run 'nbdinfo --map $uri' 0 1048576 3 hole,zero
$ dd if=/dev/zero of=empty2 count=1 bs=1M $ nbdkit file empty2 --run 'nbdinfo --map $uri' 0 1048576 0 data
nbdkit-checkwrite-filter should permit you to either write zeroes (using pwrite), trim or zero (or any combination) both of those disks. It should reject with EIO attempts to write non-zeroes anywhere.
The reason for this is because this filter is designed for testing copying tools like qemu-img convert & nbdcopy. You could back it with a local file like one of the ones above, but more fun is to use a randomly created virtual disk, eg:
$ nbdkit linuxdisk ~/d/nbdkit --filter=checkwrite --run 'nbdcopy "$uri" "$uri"' $ nbdkit pattern 1G --filter=checkwrite --run 'nbdcopy --allocated "$uri" "$uri"'
Because those tools commonly sparsify (-S) or allocate (--allocated) when they copy, what is written can contain trims or zeroes where the source disk had data reading as zero, or vice versa.
Some fun facts ...
Some plugins already have the equivalent of checkwrite functionality, eg this just works without the filter:
$ nbdkit sparse-random 1T --run 'qemu-img convert -p "$uri" "$uri"'
qemu-img has new(?) behaviour that breaks some things:
$ nbdkit linuxdisk ~/d/nbdkit --filter=checkwrite --run 'qemu-img convert "$uri" "$uri"' nbdkit: linuxdisk.0: error: data written does not match expected qemu-img: nbd://localhost:10809: error while converting raw: Failed to clear the new image's first sector: Input/output error
I understand why this happens, but also it's kind of a bug ... We should probably modify checkwrite to handle this, but it's rather complicated.
I think it directly contradicts the assertion. That, or I don't understand the comment at all.
(It's from commit 2d0954dd6431, "New filter: checkwrite", 2020-12-22.)
Laszlo
Rich.
[0] More precisely: an extent with type == 0.
[1] This is actually the same way that qemu-img compare works.
On Tue, Jul 19, 2022 at 04:45:53PM +0100, Richard W.M. Jones wrote:
qemu-img has new(?) behaviour that breaks some things:
$ nbdkit linuxdisk ~/d/nbdkit --filter=checkwrite --run 'qemu-img convert "$uri" "$uri"' nbdkit: linuxdisk.0: error: data written does not match expected qemu-img: nbd://localhost:10809: error while converting raw: Failed to clear the new image's first sector: Input/output error
I understand why this happens, but also it's kind of a bug ... We should probably modify checkwrite to handle this, but it's rather complicated.
I see you fixed this in the meantime (commit c7420c04), but repeating the context here: qemu-img convert normally assumes that source and destination are not the same, and assumes that when creating a destination on an existing block device (such as NBD), it must wipe any pre-existing signature in the first sector of the destination to make sure we don't probe an incorrect format. It is this attempt to wipe that writes different data; using 'qemu-img convert -n' skips the creation step and therefore the wipe. Perhaps qemu-img convert could be taught to special-case when -n is not in use by first reading the destination to see if it reads identically and only do the wipe if not; but that's asking a lot when we can solve it by using -n. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
participants (3)
-
Eric Blake -
Laszlo Ersek -
Richard W.M. Jones