Re: [Qemu-devel] [Nbd] [PATCH v2] doc: Add NBD_CMD_BLOCK_STATUS extension

[Markus Pargmann]

On Tuesday 05 April 2016 22:50:51 Wouter Verhelst wrote:
> On Tue, Apr 05, 2016 at 08:14:01AM -0600, Eric Blake wrote:
> > On 04/05/2016 03:24 AM, Markus Pargmann wrote:
> > 
> > >> +        requested.
> > >> +
> > >> +    The client SHOULD NOT read from an area that has both
> > >> +    `NBD_STATE_HOLE` set and `NBD_STATE_ZERO` clear.
> > > 
> > > Why not? If we don't execute CMD_BLOCK_STATUS we wouldn't know about the
> > > situation and would simply read directly. To fulfill this statement we
> > > would need to receive the block status before every read operation.
> > 
> > Because we already state that for NBD_CMD_TRIM, the client SHOULD NOT
> > read an area where a trim was requested without an intervening write,
> 
> Actually, we state that a client SHOULD NOT *make any assumption* about
> the state (emphasis added). There's a difference.
> 
> If the client wants to write, there is no problem (but he'll probably
> get garbage).
> 
> [...]
> > > Also something that is kind of missing from the document so far is
> > > concurrency with other NBD clients. Certainly most users do not use NBD
> > > for concurrent access to the backend storage. But for example the
> > > sentence above ignores the fact that another client may work on the
> > > backend and that the state may change after some time so that it may
> > > still be necessary to read from an area with NBD_STATE_HOLE and
> > > NBD_STATE_ZERO set.
> > 
> > That's missing from NBD in general, and I don't think this is the patch
> > to add it.  We already have concurrency with self issues, because NBD
> > server can handle requests out of order (within the bounds of FLUSH and
> > FUA modifiers).
> 
> I don't think NBD *needs* to add much in the way of concurrency, other
> than write barriers etc; but having an explicit message "some other
> process just modified offset X length Y" seems out of scope for NBD.

Yes it certainly is out of scope to add explicit messages for concurrency. I
only thought about some general information for the reader to have a small
paragraph in the protocol documentation that states that concurrent access can
occur at all times and that no assumptions can be made about the state of the
server and its backend storage at any times.

> 
> > > Also it is uncertain if these status bits may change over time through
> > > reorganization of backend storage, for example holes may be removed in
> > > the backend and so on. Is it safe to cache this stuff?
> > 
> > If the client is the only thing modifying the drive, maybe we want to
> > make that additional constraint on the server.  But how best to word it,
> > or is it too tight of a specification?
> 
> I think it's perfectly fine for the protocol to assume in general that
> the client is the only writer, and that no other writers are
> concurrently accessing the same device. Anything else would make the
> protocol much more complex; and one of the features of NBD is, I think,
> the fact that the protocol is not *too* complex.

Yes this definitely is a feature. I just think that concurrent access was
always possible and we should not by accident remove that possibility. But as
far as I can see that's not an issue here.

Best Regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

signature.asc
Description: This is a digitally signed message part.