Re: [PATCH] nbd/server: Advertise MULTI_CONN for shared writable exports

[Eric Blake]

On Fri, Aug 27, 2021 at 05:48:10PM +0100, Richard W.M. Jones wrote:
> On Fri, Aug 27, 2021 at 10:09:16AM -0500, Eric Blake wrote:
> > +# Parallel client connections are easier with libnbd than qemu-io:
> > +if ! (nbdsh --version) >/dev/null 2>&1; then
> 
> I'm curious why the parentheses are needed here?

Habit - some earlier Bourne shells would leak an error message if
nbdsh was not found unless you guarantee that stderr is redirected
first by using the subshell (or maybe that's what happen when you do
feature tests of a shell builtin, rather than an external app).  But
since this test uses bash, you're right that it's not necessary in
this instance, so I'll simplify.

> 
> > +export nbd_unix_socket
> > +nbdsh -c '
> > +import os
> > +sock = os.getenv("nbd_unix_socket")
> > +h = []
> > +
> > +for i in range(3):
> > +  h.append(nbd.NBD())
> > +  h[i].connect_unix(sock)
> > +  assert h[i].can_multi_conn()
> > +
> > +buf1 = h[0].pread(1024 * 1024, 0)
> > +if buf1 != b"\x01" * 1024 * 1024:
> > +  print("Unexpected initial read")
> > +buf2 = b"\x03" * 1024 * 1024
> > +h[1].pwrite(buf2, 0)                   #1
> > +h[2].flush()
> > +buf1 = h[0].pread(1024 * 1024, 0)
> > +if buf1 == buf2:
> > +  print("Flush appears to be consistent across connections")
> 
> The test is ... simple.
> 
> Would it be a better test if the statement at line #1 above was
> h.aio_pwrite instead, so the operation is only started?  This would
> depend on some assumptions inside libnbd: That the h[1] socket is
> immediately writable and that libnbd's statement will write before
> returning, which are likely to be true, but perhaps you could do
> something with parsing libnbd debug statements to check that the state
> machine has started the write.

The rules on consistent operations are tricky - it is not enough that
the client started the request in order, but that the server processed
things in order.  Even though you can have two clients in one process
and enough happens-between code in that you are sure that client A
sent NBD_CMD_WRITE prior to client B sending NBD_CMD_FLUSH, you do NOT
have enough power over TCP to prove that the server did not receive
client B's request first, unless you ALSO ensure that client A waited
for the server's response to the NBD_CMD_WRITE.  Similarly for client
C sending NBD_CMD_READ prior to client B getting the server's response
to NBD_CMD_FLUSH.  As soon as out-of-order request processing can
happen (which is more likely when you have parallel sockets), you can
only guarantee the cross-client consistency if you can also guarantee
which replies were received prior to sending a given request.  Using
asynch I/O does not provide those guarantees; the only reliable way to
test cross-client consistency is with synchronous commands.

Hmm - I should probably tweak the qemu-nbd command to set an explicit
cache mode (if it accidentally settles on cache=writethrough, that
defeats the point of an independent client's flush impacting all other
clients, since the client doing the write will also be flushing).

> 
> Another idea would be to make the write at line #1 be very large, so
> that perhaps the qemu block layer would split it up.  After the flush
> you would read the beginning and end bytes of the written part to
> approximate a check that the whole write has been flushed so parts of
> it are not hanging around in the cache of the first connection.
> 
> Anyway the patch as written seems fine to me so:
> 
> Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
> 

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