On 01/17/2018 04:38 PM, Eric Blake wrote:
> On 01/17/2018 02:53 PM, Richard W.M. Jones wrote:
>> Previously the file plugin supported ‘rdelay’ and ‘wdelay’ parameters
>> for injecting delays (for testing) into read and write requests. This
>> moves the functionality to a new delay filter so that it can be used
>> with any plugin.
>> ---
>
>> +/* Write data. */
>> +static int
>> +delay_pwrite (struct nbdkit_next *next, void *nxdata,
>> + void *handle,
>> + const void *buf, uint32_t count, uint64_t offset)
>> +{
>> + write_delay ();
>> + return next->pwrite (nxdata, buf, count, offset);
>> +}
>> +
>> +/* Zero data. */
>> +static int
>> +delay_zero (struct nbdkit_next *next, void *nxdata,
>> + void *handle, uint32_t count, uint64_t offset, int may_trim)
>> +{
>> + write_delay ();
>> + return next->zero (nxdata, count, offset, may_trim);
>
> If next->zero() fails with EOPNOTSUPP, that means we will delay once in
> trying the underlying command, and again for each iteration of the
> fallback loop as it calls delay_pwrite(). Is that okay, or do we want
> to reproduce some fallback logic here and directly call next->pwrite on
> EOPNOTSUPP so as to only have a single write delay in that case?Actually, thinking about this more: When I added zero support, I documented in commit ac3f294a that for the file plugin, wdelay is indeed doubled on systems lacking efficient zero support. But the fallback for handling EOPNOTSUPP is currently done at the plugins.c level (ie. it is part of next->zero()) - so a filter should never see a plugin returning EOPNOTSUPP. Or put another way, your movement of wdelay from being part of the file plugin to now being a separate filter actually FIXES the double delay. On the other hand, a filter can itself return EOPNOTSUPP (rather than calling next->zero), in which case we still need to support the fallback to .pwrite higher up in the call stack Along the same lines, with my FUA patches, if we keep the fallback of calling .flush in connections.c, then the flush will pass through the entire filter chain; but I had proposed moving the fallback flush into plugins.c, at which point the fallback flush is done at the layer that lacks FUA support. And maybe we want a flush delay to mirror the existing rdelay and wdelay (simulating a long flush time can indeed be useful in tests); where it definitely matters whether the flush delay is triggered as part of FUA fallback, or only triggered on an actual NBD_CMD_FLUSH from the client. I'm also thinking ahead to expansions - for example, there are proposals to add resize and block status commands to NBD. In your implementation, if a filter does not define .can_flush or .flush, then its .can_flush implementation merely returns whatever the underlying backend's version would report. But as we add new callbacks, such as a new .can_resize, we need to make sure we have sane defaults. For example, the offset filter should probably not allow resizes (even if the underlying plugin does). If the offset filter compiled in a version of nbdkit that lacks the .can_resize hook is run by nbdkit that has added the hook, we therefore want to default .can_resize to false as part of loading the smaller filter struct into memory (and NOT default to falling through to the plugin's .can_resize); a default based on fall-through should only happen if the filter had a size large enough to admit that the user explicitly omitted the callback in the filter, rather than it being NULL because of version mismatch. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Libguestfs mailing list [email protected] https://www.redhat.com/mailman/listinfo/libguestfs
