Eric Blake
2019-Mar-28 02:45 UTC
Re: [Libguestfs] [PATCH nbdkit v4 05/15] cow: Disable extents information in this filter.
On 3/26/19 4:17 PM, Richard W.M. Jones wrote:> The cow filter doesn't support trimming and we should assume the > underlying plugin is fully allocated too. Note that both of these > limitations might be lifted with a more advanced filter > implementation. > > However we ought to support this in future because xz files contain > sparseness information, so add a note.Stale copy-and-paste comment? Also, since the series is mostly alphabetical, I suspect you're missing 4.5/15 for filters/cache/cache.c (blindly passing through .extents to the underlying plugin); easiest is probably forcing .can_extents false, but nicer would be remembering status of portions that we have cached locally to avoid repeated callouts to the plugin. ACK for the cow filter. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Richard W.M. Jones
2019-Mar-28 10:32 UTC
Re: [Libguestfs] [PATCH nbdkit v4 05/15] cow: Disable extents information in this filter.
On Wed, Mar 27, 2019 at 09:45:58PM -0500, Eric Blake wrote:> On 3/26/19 4:17 PM, Richard W.M. Jones wrote: > > The cow filter doesn't support trimming and we should assume the > > underlying plugin is fully allocated too. Note that both of these > > limitations might be lifted with a more advanced filter > > implementation. > > > > However we ought to support this in future because xz files contain > > sparseness information, so add a note. > > Stale copy-and-paste comment?Yes that was a copy-paste bug (or rather commit -c). I've fixed it.> Also, since the series is mostly alphabetical, I suspect you're missing > 4.5/15 for filters/cache/cache.c (blindly passing through .extents to > the underlying plugin); easiest is probably forcing .can_extents false, > but nicer would be remembering status of portions that we have cached > locally to avoid repeated callouts to the plugin.I examined the cache filter and I don't think it needs any changes for correctness. It could - as you note - be improved so it caches extents information, but I think if it does nothing then it's still correct. Note that nbdkit will automatically pass the extents call through - we don't need to add an explicit extents callback in filters which don't want to modify the result. Also .can_extents is properly taken into account. (I think the man page should be clearer about all this.) 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
Eric Blake
2019-Mar-28 12:02 UTC
Re: [Libguestfs] [PATCH nbdkit v4 05/15] cow: Disable extents information in this filter.
On 3/28/19 5:32 AM, Richard W.M. Jones wrote:> On Wed, Mar 27, 2019 at 09:45:58PM -0500, Eric Blake wrote: >> On 3/26/19 4:17 PM, Richard W.M. Jones wrote: >>> The cow filter doesn't support trimming and we should assume the >>> underlying plugin is fully allocated too. Note that both of these >>> limitations might be lifted with a more advanced filter >>> implementation. >>> >>> However we ought to support this in future because xz files contain >>> sparseness information, so add a note. >> >> Stale copy-and-paste comment? > > Yes that was a copy-paste bug (or rather commit -c). I've fixed it. > >> Also, since the series is mostly alphabetical, I suspect you're missing >> 4.5/15 for filters/cache/cache.c (blindly passing through .extents to >> the underlying plugin); easiest is probably forcing .can_extents false, >> but nicer would be remembering status of portions that we have cached >> locally to avoid repeated callouts to the plugin. > > I examined the cache filter and I don't think it needs any changes for > correctness. It could - as you note - be improved so it caches > extents information, but I think if it does nothing then it's still > correct.Still, it may be worth a comment in the file, to specifically document that we've thought about it, and that if performance proves to be slow (vddk, anyone? and qemu-img's current known inefficiencies with REQ_ONE) that it may be helpful as a future addition.> > Note that nbdkit will automatically pass the extents call through - we > don't need to add an explicit extents callback in filters which don't > want to modify the result. Also .can_extents is properly taken into > account. (I think the man page should be clearer about all this.)A doc update might not hurt, but yeah, I already realized that the default behavior (if you don't do anything else) is for both .can_extents and .extents to blindly pass through. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Possibly Parallel Threads
- Re: [PATCH nbdkit v4 05/15] cow: Disable extents information in this filter.
- [PATCH nbdkit v4 05/15] cow: Disable extents information in this filter.
- Re: [PATCH nbdkit v4 01/15] server: Implement extents/can_extents calls for plugins and filters.
- [PATCH nbdkit v4 01/15] server: Implement extents/can_extents calls for plugins and filters.
- Re: [PATCH nbdkit v4 01/15] server: Implement extents/can_extents calls for plugins and filters.