Libguestfs - Feb 2021 - [nbdkit PATCH] readahead: Fix multi-conn inconsistency

On 2/19/21 3:03 PM, Eric Blake wrote:
> The readahead filter was blindly passing through the plugin's
> .can_multi_conn, but does not kill the readahead buffer on a flush.
> This is a bug in the following scenario, when the plugin advertised
> multi-conn:
> 
> In order to preserve the plugin's multi-conn status, we MUST drop our
> cache any time we flush, so that our next read picks up whatever got
> flushed from another connection.
> 
> But when the server lacks multi-conn, we don't have to kill our
> readahead buffer on flush, because the client already has no guarantee
> that a flush from one connection will affect the read seen by another
> (killing on write was already good enough to maintain semantics).
> ---
> +static int
> +readahead_flush (struct nbdkit_next_ops *next_ops, void *nxdata,
> +                 void *handle, uint32_t flags, int *err)
> +{
> +  if (next_ops->can_multi_conn(nxdata) != 1)
That should be ==, to match my commit message.
> +    kill_readahead ();
> +  return next_ops->flush (nxdata, flags, err);
> +}
> +
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 2/19/21 3:18 PM, Eric Blake wrote:
> On 2/19/21 3:03 PM, Eric Blake wrote:
>> The readahead filter was blindly passing through the plugin's
>> .can_multi_conn, but does not kill the readahead buffer on a flush.
>> This is a bug in the following scenario, when the plugin advertised
>> multi-conn:
>>
> 
>> In order to preserve the plugin's multi-conn status, we MUST drop
our
>> cache any time we flush, so that our next read picks up whatever got
>> flushed from another connection.
>>
>> But when the server lacks multi-conn, we don't have to kill our
>> readahead buffer on flush, because the client already has no guarantee
>> that a flush from one connection will affect the read seen by another
>> (killing on write was already good enough to maintain semantics).
>> ---
> 
>> +static int
>> +readahead_flush (struct nbdkit_next_ops *next_ops, void *nxdata,
>> +                 void *handle, uint32_t flags, int *err)
>> +{
>> +  if (next_ops->can_multi_conn(nxdata) != 1)
> 
> That should be ==, to match my commit message.
> 
>> +    kill_readahead ();
>> +  return next_ops->flush (nxdata, flags, err);
>> +}
>> +
> 
In fact, the more I think about it, the more I want it to be
unconditional, killing readahead even when not multi-conn.  Why?
Because when multi-conn is not advertised, it seems odd that there is no
way at all to force the NBD server to flush its caches; it seems like
clients should be able to expect a flush on connA (to write pending data
to cache), then a flush on connB (to drop any readahead data), prior to
a read on connB, should let connB see what connA wrote.  All that
multi-conn buys you is that you can skip one of the two flush calls.

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