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

[Richard W.M. Jones]

On Fri, Feb 19, 2021 at 04:02:48PM -0600, Eric Blake wrote:
> 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.

I see the modified version of this patch went upstream - thanks!

The readahead filter is fairly broken at the moment, and we stopped
using it in virt-v2v.  There are several causes of the brokenness:

 - Not having the ability of using a background thread.  (See TODO)

 - Readahead happens synchronously with the request which makes the
   requests run slower.

 - Readahead causes too large plugin requests which breaks VDDK.  Even
   though strictly this is yet another VDDK-is-crap bug, it was the
   final straw that made us drop the readahead filter.

 - Very naive/stupid readahead policy.

 - Related to previous point and to your message: The naive policy is
   practically incompatible with multi-conn, because if two threads
   are reading different parts of the disk then the readahead buffer
   will be dropped between the requests.

So I believe we should also set can_multi_conn = 0 in this filter.

It might also be a good idea to drop the filter until such time as we
have the background threads feature in the server and can do it
properly.  But at the moment this filter has a very narrow and limited
usefulness if you know what you're doing.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/