Libguestfs - Jun 2022 - [nbdkit PATCH 3/3] tests: Add regression test for NBDKIT_EMULATE_CACHE fix

On Fri, Jun 10, 2022 at 03:21:17PM -0500, Eric Blake
wrote:
> On Fri, Jun 10, 2022 at 05:21:19PM +0100, Richard W.M. Jones wrote:
> > 
> > Series looks good:
> > 
> > Acked-by: Richard W.M. Jones <rjones at redhat.com>
> > 
> > I think nbdkit-ext2-filter is still wrong (although at least it should
> > no longer corrupt disk images) because it unnecessarily calls
> > next->can_zero, so it might be worth dropping those two lines.
> 
> ext2 does not have an interface (yet?) for emulating fallocate()
> operations (or other bulk-zeroing operation) against a file; if it
> _did_ have one, then that's what ext2's .zero should do.  But we
_do_
> want to allow the compressed transmission effects of
> NBD_CMD_WRITE_ZEROES over the network, so having .can_zero return
> NBDKIT_ZERO_EMULATE is right.  On the other hand, ext2 DOES have a way
> for the file system itself to request bulk-zeroing of a portion of the
> underlying disk, so we _do_ call into the plugin's next->zero(),
which
> means we _do_ need to check next->can_zero() up front (see the io.c
> callback io_zeroout).  I don't see any bugs in this area, once this
> series is in.
I mean these two lines:

https://gitlab.com/nbdkit/nbdkit/-/blob/b4b8bd78ee66e5e1bc3d9b5464b10be0719445f1/filters/ext2/ext2.c#L342-L343

Before your patch series, they avoid the same assertion fail that we
saw in the luks filter.  The mechanism is that when ZERO_EMULATE
caused filter zero to get forwarded to the plugin, because plugin
can_zero had never been called, the assertion fired.  Those two lines
initialize plugin can_zero, removing the assertion but causing the
.zero call to be forwarded to the plugin unmodified (randomly zeroing
some part of the disk).

After your patch series, the two lines just do nothing at all.

Anyway, I will remove them.
> > 
> > Regression test for the LUKS case might be useful.  But note that the
> > test should not only check that zeroes work, but also that the result
> > is still a LUKS image.  The reason is that a naive fix for the earlier
> > problem can (as I discovered) corrupt the disk image, because it will
> > forward zeroes (eg to offset 0) straight through to the plugin.  If
> > this is too much, then I can write this test case tomorrow.
> 
> I'll let you handle that one, then.  These three patches pushed as:
> 
> 09a0e4dc..6ced998b
Will do, thanks.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit

On Fri, Jun 10, 2022 at 10:03:23PM +0100, Richard W.M. Jones
wrote:
> On Fri, Jun 10, 2022 at 03:21:17PM -0500, Eric Blake wrote:
> > On Fri, Jun 10, 2022 at 05:21:19PM +0100, Richard W.M. Jones wrote:
> > > 
> > > Series looks good:
> > > 
> > > Acked-by: Richard W.M. Jones <rjones at redhat.com>
> > > 
> > > I think nbdkit-ext2-filter is still wrong (although at least it
should
> > > no longer corrupt disk images) because it unnecessarily calls
> > > next->can_zero, so it might be worth dropping those two lines.
> > 
> > ext2 does not have an interface (yet?) for emulating fallocate()
> > operations (or other bulk-zeroing operation) against a file; if it
> > _did_ have one, then that's what ext2's .zero should do.  But
we _do_
> > want to allow the compressed transmission effects of
> > NBD_CMD_WRITE_ZEROES over the network, so having .can_zero return
> > NBDKIT_ZERO_EMULATE is right.  On the other hand, ext2 DOES have a way
> > for the file system itself to request bulk-zeroing of a portion of the
> > underlying disk, so we _do_ call into the plugin's
next->zero(), which
> > means we _do_ need to check next->can_zero() up front (see the io.c
> > callback io_zeroout).  I don't see any bugs in this area, once
this
> > series is in.
> 
> I mean these two lines:
> 
>
https://gitlab.com/nbdkit/nbdkit/-/blob/b4b8bd78ee66e5e1bc3d9b5464b10be0719445f1/filters/ext2/ext2.c#L342-L343
> 
> Before your patch series, they avoid the same assertion fail that we
> saw in the luks filter.  The mechanism is that when ZERO_EMULATE
> caused filter zero to get forwarded to the plugin, because plugin
> can_zero had never been called, the assertion fired.  Those two lines
> initialize plugin can_zero, removing the assertion but causing the
> .zero call to be forwarded to the plugin unmodified (randomly zeroing
> some part of the disk).
> 
> After your patch series, the two lines just do nothing at all.
No, they are still useful. If we don't call them during handshake,
then the later call to next->can_zero in io.c may fail without setting
errno.  Any time we want to guarantee a next->can_* function will
succeed at all later points where we need a sane errno, we do so by
pre-caching it during handshake.
> 
> Anyway, I will remove them.
Please don't. But adding comments for why they are important is okay.

I also fixed another bug today: the eval plugin is smart enough to
synthesize several .can_* helpers if the corresponding function exists
(for example, with sh, you have to explicitly implement can_flush to
get flush called, but not with eval); but the eval plugin wasn't doing
a good synthesis for .can_cache.

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