Libguestfs - Mar 2019 - Re: [PATCH nbdkit v5 FINAL 12/19] truncate: Implement extents for beyond end of truncated region.

On 3/28/19 11:18 AM, Richard W.M. Jones wrote:
> ---
>  filters/truncate/truncate.c | 55 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> +
> +  /* If the entire request is beyond the end of the underlying plugin
> +   * then this is the easy case: return a hole.
> +   */
> +  if (offset >= real_size_copy)
> +    return nbdkit_add_extent (extents, offset, (uint64_t) count,
> +                              NBDKIT_EXTENT_ZERO|NBDKIT_EXTENT_HOLE);
Why not return a hole up to the end of the file, rather than limiting
things at the end of the client's request?
> +
> +  /* We're asked first for extents information about the plugin, then
> +   * possibly (if truncating larger) for the hole after the plugin.
> +   * Since we're not required to provide all of this information, the
> +   * easiest thing is to only return data from the plugin.  We will be
> +   * called later about the hole.  However we do need to make sure
> +   * that the extents array is truncated to the real size, hence we
> +   * have to create a new extents array, ask the plugin, then copy the
> +   * returned data to the original array.
> +   */
> +  extents2 = nbdkit_extents_new (0, real_size_copy);
Why 0 for start instead of offset?  You get the same result either way
(since the copying code ignores the prefix), but it's probably a lot
more efficient to not have to copy the extents for the prefix of the file.

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

On Thu, Mar 28, 2019 at 08:57:56PM -0500, Eric Blake
wrote:
> On 3/28/19 11:18 AM, Richard W.M. Jones wrote:
> > ---
> >  filters/truncate/truncate.c | 55
+++++++++++++++++++++++++++++++++++++
> >  1 file changed, 55 insertions(+)
> > 
> 
> > +
> > +  /* If the entire request is beyond the end of the underlying plugin
> > +   * then this is the easy case: return a hole.
> > +   */
> > +  if (offset >= real_size_copy)
> > +    return nbdkit_add_extent (extents, offset, (uint64_t) count,
> > +                              NBDKIT_EXTENT_ZERO|NBDKIT_EXTENT_HOLE);
> 
> Why not return a hole up to the end of the file, rather than limiting
> things at the end of the client's request?
Yes, very true - I'll add a small fix for this.
> > +  /* We're asked first for extents information about the plugin,
then
> > +   * possibly (if truncating larger) for the hole after the plugin.
> > +   * Since we're not required to provide all of this information,
the
> > +   * easiest thing is to only return data from the plugin.  We will
be
> > +   * called later about the hole.  However we do need to make sure
> > +   * that the extents array is truncated to the real size, hence we
> > +   * have to create a new extents array, ask the plugin, then copy
the
> > +   * returned data to the original array.
> > +   */
> > +  extents2 = nbdkit_extents_new (0, real_size_copy);
> 
> Why 0 for start instead of offset?  You get the same result either way
> (since the copying code ignores the prefix), but it's probably a lot
> more efficient to not have to copy the extents for the prefix of the file.
Yup that's a mistake too :-(

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

On 3/29/19 3:33 AM, Richard W.M. Jones wrote:
>>> +  /* We're asked first for extents information about the
plugin, then
>>> +   * possibly (if truncating larger) for the hole after the
plugin.
>>> +   * Since we're not required to provide all of this
information, the
>>> +   * easiest thing is to only return data from the plugin.  We
will be
>>> +   * called later about the hole.  However we do need to make sure
>>> +   * that the extents array is truncated to the real size, hence
we
>>> +   * have to create a new extents array, ask the plugin, then copy
the
>>> +   * returned data to the original array.
>>> +   */
>>> +  extents2 = nbdkit_extents_new (0, real_size_copy);
>>
>> Why 0 for start instead of offset?  You get the same result either way
>> (since the copying code ignores the prefix), but it's probably a
lot
>> more efficient to not have to copy the extents for the prefix of the
file.
> 
> Yup that's a mistake too :-(
Thanks for the quick fix. I realized that it is not just an efficiency
problem, but an actual bug - if the plugin returns exactly one extent,
it ends up not covering the start offset of the original extents, at
which point the truncate filter returns success with 0 extents instead
of the requirement of at least 1.  While the 'end' parameter does not
drive correct results, the 'start' parameter does :)


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