Re: [Nbd] [PATCH][V3] nbd: add multi-connection support

From: Alex Bligh <alex@alex.org.uk>
To: Josef Bacik <jbacik@fb.com>
Cc: Alex Bligh <alex@alex.org.uk>,
	Christoph Hellwig <hch@infradead.org>,
	Wouter Verhelst <w@uter.be>, Jens Axboe <axboe@fb.com>,
	"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
	Kernel Team <Kernel-team@fb.com>,
	"nbd-general@lists.sourceforge.net" 
	<nbd-general@lists.sourceforge.net>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [Nbd] [PATCH][V3] nbd: add multi-connection support
Date: Mon, 3 Oct 2016 15:46:53 +0100	[thread overview]
Message-ID: <A8EF4CD3-61BC-4E29-8796-1013FDBC96F1@alex.org.uk> (raw)
In-Reply-To: <5cf31839-3642-166a-55ab-8d4313025b0c@fb.com>

Josef,

> On 3 Oct 2016, at 15:32, Josef Bacik <jbacik@fb.com> wrote:
> 
> Ok I understand your objections now.  You aren't arguing that we are unsafe by default, only that we are unsafe with servers that do something special beyond simply writing to a single disk or file.  I agree this is problematic, but you simply don't use this feature if your server can't deal with it well.

Not quite. I am arguing that:

1. Parallel channels as currently implemented are inherently unsafe on some servers - I have given an example of such servers

2. Parallel channels as currently implemented may be unsafe on the reference server on some or all platforms (depending on the behaviour of fdatasync() which might varies between platforms)

3. Either the parallel channels stuff should issue flush requests on all channels, or the protocol should protect the unwitting user by negotiating an option to do so (or refusing multi-channel connects). It is not reasonable for an nbd client to have to know the intimate details of the server and its implementation of synchronisation primitives - saying 'the user should disable multiple channels' is not good enough, as this is what we have a protocol for.

"unsafe" here means 'do not conform to the kernel's requirements for flush semantics, whereas they did without multiple channels'

So from my point of view either we need to have an additional connection flag ("don't use multiple channels unless you are going to issue a flush on all channels") OR flush on all channels should be the default.

Whatever SOME more work needs to be done on your patch because there it current doesn't issue flush on all channels, and it currently does not have any way to prevent use of multiple channels.

I'd really like someone with linux / POSIX knowledge to confirm the linux and POSIX position on fdatasync of an fd opened by one process on writes made to the same file by another process. If on all POSIX platforms and all filing systems this is guaranteed to flush the second platform's writes, then I think we could argue "OK there may be some weird servers which might not support multiple channels and they just need a way of signalling that". If on the other hand there is no such cross platform guarantee, I think this means in essence even with the reference server, this patch is unsafe, and it needs adapting to send flushes on all channels - yes it might theoretically be possible to introduce IPC to the current server, but you'd still need some way of tying together channels from a single client.

-- 
Alex Bligh