Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1480323 > unrolled thread

Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Wouter Verhelst <w@uter.be> - 2016-09-09 22:20 +0200
    Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Wouter Verhelst <w@uter.be> - 2016-09-09 23:00 +0200
      Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Jens Axboe <axboe@kernel.dk> - 2016-09-10 01:40 +0200
    Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Wouter Verhelst <w@uter.be> - 2016-09-15 13:00 +0200
      Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Alex Bligh <alex@alex.org.uk> - 2016-09-15 13:20 +0200
        Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Wouter Verhelst <w@uter.be> - 2016-09-15 13:40 +0200
          Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Christoph Hellwig <hch@infradead.org> - 2016-09-15 13:50 +0200
            Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Alex Bligh <alex@alex.org.uk> - 2016-09-15 13:50 +0200
              Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Christoph Hellwig <hch@infradead.org> - 2016-09-15 14:00 +0200
                Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Wouter Verhelst <w@uter.be> - 2016-09-15 14:10 +0200
                  Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Christoph Hellwig <hch@infradead.org> - 2016-09-15 14:30 +0200
                  Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Christoph Hellwig <hch@infradead.org> - 2016-09-15 14:30 +0200
                    Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Wouter Verhelst <w@uter.be> - 2016-09-15 14:30 +0200
                Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Alex Bligh <alex@alex.org.uk> - 2016-09-15 14:10 +0200
        Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Christoph Hellwig <hch@infradead.org> - 2016-09-15 13:40 +0200
        Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Eric Blake <eblake@redhat.com> - 2016-09-15 15:40 +0200
          Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Paolo Bonzini <pbonzini@redhat.com> - 2016-09-15 16:10 +0200
            Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Alex Bligh <alex@alex.org.uk> - 2016-09-15 17:30 +0200
              Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Paolo Bonzini <pbonzini@redhat.com> - 2016-09-15 23:20 +0200
          Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Alex Bligh <alex@alex.org.uk> - 2016-09-15 17:30 +0200
      Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Christoph Hellwig <hch@infradead.org> - 2016-09-15 13:50 +0200
        Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Alex Bligh <alex@alex.org.uk> - 2016-09-15 14:00 +0200
      Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Alex Bligh <alex@alex.org.uk> - 2016-09-15 13:50 +0200
      Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Wouter Verhelst <w@uter.be> - 2016-09-15 14:00 +0200
        Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Christoph Hellwig <hch@infradead.org> - 2016-09-15 14:10 +0200
          Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Alex Bligh <alex@alex.org.uk> - 2016-09-15 14:20 +0200
            Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Christoph Hellwig <hch@infradead.org> - 2016-09-15 14:20 +0200
              Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Alex Bligh <alex@alex.org.uk> - 2016-09-15 14:30 +0200
          Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Wouter Verhelst <w@uter.be> - 2016-09-15 14:30 +0200
            Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Christoph Hellwig <hch@infradead.org> - 2016-09-15 14:30 +0200
              Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Alex Bligh <alex@alex.org.uk> - 2016-09-15 14:40 +0200
                Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Alex Bligh <alex@alex.org.uk> - 2016-09-15 14:40 +0200
                  Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Christoph Hellwig <hch@infradead.org> - 2016-09-15 14:50 +0200
                    Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Alex Bligh <alex@alex.org.uk> - 2016-09-15 14:50 +0200
                      Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Wouter Verhelst <w@uter.be> - 2016-09-15 15:20 +0200
                        Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Alex Bligh <alex@alex.org.uk> - 2016-09-15 17:20 +0200
                        Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Alex Bligh <alex@alex.org.uk> - 2016-09-15 18:10 +0200
                          Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Wouter Verhelst <w@uter.be> - 2016-09-15 18:30 +0200
                            Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Alex Bligh <alex@alex.org.uk> - 2016-09-15 18:50 +0200
                            Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Eric Blake <eblake@redhat.com> - 2016-09-15 21:10 +0200
                Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Christoph Hellwig <hch@infradead.org> - 2016-09-15 14:40 +0200
      Re: [Nbd] [RESEND][PATCH 0/5] nbd improvements Christoph Hellwig <hch@infradead.org> - 2016-09-15 14:20 +0200

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

#1484021

On Thu, Sep 15, 2016 at 12:43:35PM +0100, Alex Bligh wrote:
> Sure, it's at:
> 
> https://github.com/yoe/nbd/blob/master/doc/proto.md#ordering-of-messages-and-writes
> 
> and that link takes you to the specific section.
> 
> The treatment of FLUSH and FUA is meant to mirror exactly the
> linux block layer (or rather how the linux block layer was a few
> years ago). I even asked on LKML to verify a few points.

Linux never expected ordering on the wire.  Before 2010 we had barriers
in the kernel that provided ordering to the caller, but we never
required it from the protocol / hardware.

[toc] | [prev] | [next] | [standalone]

#1484042

> On 15 Sep 2016, at 12:46, Christoph Hellwig <hch@infradead.org> wrote:
> 
> On Thu, Sep 15, 2016 at 12:43:35PM +0100, Alex Bligh wrote:
>> Sure, it's at:
>> 
>> https://github.com/yoe/nbd/blob/master/doc/proto.md#ordering-of-messages-and-writes
>> 
>> and that link takes you to the specific section.
>> 
>> The treatment of FLUSH and FUA is meant to mirror exactly the
>> linux block layer (or rather how the linux block layer was a few
>> years ago). I even asked on LKML to verify a few points.
> 
> Linux never expected ordering on the wire.  Before 2010 we had barriers
> in the kernel that provided ordering to the caller, but we never
> required it from the protocol / hardware.

Sure. And I think the doc section reflects exactly Linux's post-2010
expectations (note the link to the kernel documentation). IE
servers are not required to order reads/writes (save that all
writes that the server completes prior to reply to an NBD_CMD_FLUSH
must be persisted prior to the reply to that NBD_CMD_FLUSH
which I believe to be exactly the same behaviour as the kernel
dealing with an empty bio with REQ_FLUSH set).

Perhaps the section should be called "No ordering of messages
and writes"!

My point was that *in practice* disordering is not well tested
as *in practice* many server implementations do in fact process
each command in order, though that's changed recently.

-- 
Alex Bligh

[toc] | [prev] | [next] | [standalone]

#1484026

Christoph,

> On 15 Sep 2016, at 12:38, Christoph Hellwig <hch@infradead.org> wrote:
> 
> On Thu, Sep 15, 2016 at 12:49:35PM +0200, Wouter Verhelst wrote:
>> A while back, we spent quite some time defining the semantics of the
>> various commands in the face of the NBD_CMD_FLUSH and NBD_CMD_FLAG_FUA
>> write barriers. At the time, we decided that it would be unreasonable
>> to expect servers to make these write barriers effective across
>> different connections.
> 
> Do you have a nbd protocol specification?  treating a flush or fua
> as any sort of barrier is incredibly stupid.  Is it really documented
> that way, and if yes, why?

Sure, it's at:

https://github.com/yoe/nbd/blob/master/doc/proto.md#ordering-of-messages-and-writes

and that link takes you to the specific section.

The treatment of FLUSH and FUA is meant to mirror exactly the
linux block layer (or rather how the linux block layer was a few
years ago). I even asked on LKML to verify a few points.

-- 
Alex Bligh

[toc] | [prev] | [next] | [standalone]

#1484043

On Thu, Sep 15, 2016 at 04:38:07AM -0700, Christoph Hellwig wrote:
> On Thu, Sep 15, 2016 at 12:49:35PM +0200, Wouter Verhelst wrote:
> > A while back, we spent quite some time defining the semantics of the
> > various commands in the face of the NBD_CMD_FLUSH and NBD_CMD_FLAG_FUA
> > write barriers. At the time, we decided that it would be unreasonable
> > to expect servers to make these write barriers effective across
> > different connections.
> 
> Do you have a nbd protocol specification?

https://github.com/yoe/nbd/blob/master/doc/proto.md

> treating a flush or fua as any sort of barrier is incredibly stupid.

Maybe I'm not using the correct terminology here. The point is that
after a FLUSH, the server asserts that all write commands *for which a
reply has already been sent to the client* will also have reached
permanent storage. Nothing is asserted about commands for which the
reply has not yet been sent, regardless of whether they were sent to the
server before or after the FLUSH; they may reach permanent storage as a
result of the FLUSH, or they may not, we don't say.

With FUA, we only assert that the FUA-flagged command reaches permanent
storage before its reply is sent, nothing else.

If that's not a write barrier, then I was using the wrong terminology
(and offer my apologies for the confusion).

The point still remains that "X was sent before Y" is difficult to
determine on the client side if X was sent over a different TCP channel
than Y, because a packet might be dropped (requiring a retransmit) for
X, and similar things. If blk-mq can deal with that, we're good and
nothing further needs to be done. If not, this should be evaluated by
someone more familiar with the internals of the kernel block subsystem
than me.

> Is it really documented that way, and if yes, why?

The documentation does use the term write barrier, but only right before
the same explanation as above. It does so because I assumed that that is
what a write barrier is, and that this would make things easier to
understand. If you tell me that that term is wrong as used there, I can
easily remove it (it's not critical to the rest of the documentation).

Regards,

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
       people in the world who think they really understand all of its rules,
       and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12

[toc] | [prev] | [next] | [standalone]

#1484067

On Thu, Sep 15, 2016 at 01:55:14PM +0200, Wouter Verhelst wrote:
> Maybe I'm not using the correct terminology here. The point is that
> after a FLUSH, the server asserts that all write commands *for which a
> reply has already been sent to the client* will also have reached
> permanent storage. Nothing is asserted about commands for which the
> reply has not yet been sent, regardless of whether they were sent to the
> server before or after the FLUSH; they may reach permanent storage as a
> result of the FLUSH, or they may not, we don't say.
> 
> With FUA, we only assert that the FUA-flagged command reaches permanent
> storage before its reply is sent, nothing else.

Yes, that's the correct semantics.

> If that's not a write barrier, then I was using the wrong terminology
> (and offer my apologies for the confusion).

It's not a write barrier - a write barrier was command that ensured that

 a) all previous writes were completed to the host/client
 b) all previous writes were on non-volatile storage

and

 c) the actual write with the barrier bit was on non-volatile storage

> The point still remains that "X was sent before Y" is difficult to
> determine on the client side if X was sent over a different TCP channel
> than Y, because a packet might be dropped (requiring a retransmit) for
> X, and similar things. If blk-mq can deal with that, we're good and
> nothing further needs to be done. If not, this should be evaluated by
> someone more familiar with the internals of the kernel block subsystem
> than me.

The important bit in all the existing protocols, and which Linux relies
on is that any write the Linux block layer got a completion for earlier
needs to be flushed out to non-volatile storage when a FLUSH command is
set.  Anything that still is in flight does not matter.  Which for
NBD means anything that you already replies to need to be flushed.

Or to say it more practicly - in the nbd server you simply need to
call fdatasync on the backing device or file whenever you get a FLUSH
requires, and it will do the right thing.

[toc] | [prev] | [next] | [standalone]

#1484092

Christoph,

> It's not a write barrier - a write barrier was command that ensured that
> 
> a) all previous writes were completed to the host/client
> b) all previous writes were on non-volatile storage
> 
> and
> 
> c) the actual write with the barrier bit was on non-volatile storage

Ah! the bit you are complaining about is not the bit I pointed to you, but:

> NBD_CMD_FLUSH (3)
> 
> A flush request; a write barrier. 

I can see that's potentially confusing as isn't meant to mean 'an old-style
linux kernel block device write barrier'. I think in general terms it
probably is some form of barrier, but I see no problem in deleting the
words "a write barrier" from the spec text if only to make it
clearer. However, I think the description of the command itself:

> The server MUST NOT send a successful reply header for this request before all write requests for which a reply has already been sent to the client have reached permanent storage (using fsync() or similar).

and the ordering section I pointed you to before, were both correct, yes?


>> The point still remains that "X was sent before Y" is difficult to
>> determine on the client side if X was sent over a different TCP channel
>> than Y, because a packet might be dropped (requiring a retransmit) for
>> X, and similar things. If blk-mq can deal with that, we're good and
>> nothing further needs to be done. If not, this should be evaluated by
>> someone more familiar with the internals of the kernel block subsystem
>> than me.
> 
> The important bit in all the existing protocols, and which Linux relies
> on is that any write the Linux block layer got a completion for earlier
> needs to be flushed out to non-volatile storage when a FLUSH command is
> set.  Anything that still is in flight does not matter.  Which for
> NBD means anything that you already replies to need to be flushed.

... that's what it says (I hope).

> Or to say it more practicly - in the nbd server you simply need to
> call fdatasync on the backing device or file whenever you get a FLUSH
> requires, and it will do the right thing.

actually fdatasync() technically does more than is necessary, as it
will also flush commands that have been processed, but for which no
reply has yet been sent - that's no bad thing.

-- 
Alex Bligh

[toc] | [prev] | [next] | [standalone]

#1484094

On Thu, Sep 15, 2016 at 01:11:24PM +0100, Alex Bligh wrote:
> > NBD_CMD_FLUSH (3)
> > 
> > A flush request; a write barrier. 
> 
> I can see that's potentially confusing as isn't meant to mean 'an old-style
> linux kernel block device write barrier'. I think in general terms it
> probably is some form of barrier, but I see no problem in deleting the
> words "a write barrier" from the spec text if only to make it
> clearer.

Yes, please do that.  A "barrier" implies draining of the queue.

> However, I think the description of the command itself:
> 
> > The server MUST NOT send a successful reply header for this request before all write requests for which a reply has already been sent to the client have reached permanent storage (using fsync() or similar).
> 
> and the ordering section I pointed you to before, were both correct, yes?

Yes, this seems correct.

> actually fdatasync() technically does more than is necessary, as it
> will also flush commands that have been processed, but for which no
> reply has yet been sent - that's no bad thing.

Yes.  But without an actual barrier it's hard to be exact - and
fdatasync does the right thing by including false positives.

[toc] | [prev] | [next] | [standalone]

#1484103

> On 15 Sep 2016, at 13:18, Christoph Hellwig <hch@infradead.org> wrote:
> 
> Yes, please do that.  A "barrier" implies draining of the queue.

Done

-- 
Alex Bligh

[toc] | [prev] | [next] | [standalone]

#1484105

On Thu, Sep 15, 2016 at 05:01:25AM -0700, Christoph Hellwig wrote:
> On Thu, Sep 15, 2016 at 01:55:14PM +0200, Wouter Verhelst wrote:
> > If that's not a write barrier, then I was using the wrong terminology
> > (and offer my apologies for the confusion).
> 
> It's not a write barrier - a write barrier was command that ensured that
[...]

Okay, I'll update the spec to remove that term then.

> > The point still remains that "X was sent before Y" is difficult to
> > determine on the client side if X was sent over a different TCP channel
> > than Y, because a packet might be dropped (requiring a retransmit) for
> > X, and similar things. If blk-mq can deal with that, we're good and
> > nothing further needs to be done. If not, this should be evaluated by
> > someone more familiar with the internals of the kernel block subsystem
> > than me.
> 
> The important bit in all the existing protocols, and which Linux relies
> on is that any write the Linux block layer got a completion for earlier
> needs to be flushed out to non-volatile storage when a FLUSH command is
> set.  Anything that still is in flight does not matter.  Which for
> NBD means anything that you already replies to need to be flushed.
> 
> Or to say it more practicly - in the nbd server you simply need to
> call fdatasync on the backing device or file whenever you get a FLUSH
> requires, and it will do the right thing.

Right. So do I understand you correctly that blk-mq currently doesn't
look at multiple queues, and just assumes that if a FLUSH is sent over
any one of the queues, it applies to all queues?

If so, I'll update the spec to clarify that servers should ensure this
holds in the case of multiple connections, or not allow writes, or not
allow multiple connections.

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
       people in the world who think they really understand all of its rules,
       and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12

[toc] | [prev] | [next] | [standalone]

#1484106

On Thu, Sep 15, 2016 at 02:21:20PM +0200, Wouter Verhelst wrote:
> Right. So do I understand you correctly that blk-mq currently doesn't
> look at multiple queues, and just assumes that if a FLUSH is sent over
> any one of the queues, it applies to all queues?

Yes.  The same is true at the protocol level for NVMe or SCSI transports
that can make use of multiple queues.

[toc] | [prev] | [next] | [standalone]

#1484111

> On 15 Sep 2016, at 13:23, Christoph Hellwig <hch@infradead.org> wrote:
> 
> On Thu, Sep 15, 2016 at 02:21:20PM +0200, Wouter Verhelst wrote:
>> Right. So do I understand you correctly that blk-mq currently doesn't
>> look at multiple queues, and just assumes that if a FLUSH is sent over
>> any one of the queues, it applies to all queues?
> 
> Yes.  The same is true at the protocol level for NVMe or SCSI transports
> that can make use of multiple queues.

At an implementation level that is going to be a little difficult
for some NBD servers, e.g. ones that fork() a different process per
connection. There is in general no IPC to speak of between server
instances. Such servers would thus be unsafe with more than one
connection if FLUSH is in use.

I believe such servers include the reference server where there is
process per connection (albeit possibly with several threads).

Even single process servers (including mine - gonbdserver) would
require logic to pair up multiple connections to the same
device.

I suspect ALL nbd servers would require a change. So I think we should
think carefully before introducing this protocol change.

It might be easier to make the client code change work around this
issue (somehow).

-- 
Alex Bligh

[toc] | [prev] | [next] | [standalone]

#1484113

> On 15 Sep 2016, at 13:36, Christoph Hellwig <hch@infradead.org> wrote:
> 
> On Thu, Sep 15, 2016 at 01:33:20PM +0100, Alex Bligh wrote:
>> At an implementation level that is going to be a little difficult
>> for some NBD servers, e.g. ones that fork() a different process per
>> connection. There is in general no IPC to speak of between server
>> instances. Such servers would thus be unsafe with more than one
>> connection if FLUSH is in use.
>> 
>> I believe such servers include the reference server where there is
>> process per connection (albeit possibly with several threads).
>> 
>> Even single process servers (including mine - gonbdserver) would
>> require logic to pair up multiple connections to the same
>> device.
> 
> Why?  If you only send the completion after your I/O syscall returned
> your are fine if fsync comes from a difference process, no matter
> if you're using direct or buffered I/O underneath.

That's probably right in the case of file-based back ends that
are running on a Linux OS. But gonbdserver for instance supports
(e.g.) Ceph based backends, where each connection might be talking
to a completely separate ceph node, and there may be no cache
consistency between connections.

-- 
Alex Bligh

[toc] | [prev] | [next] | [standalone]

#1484134

On Thu, Sep 15, 2016 at 01:39:11PM +0100, Alex Bligh wrote:
> That's probably right in the case of file-based back ends that
> are running on a Linux OS. But gonbdserver for instance supports
> (e.g.) Ceph based backends, where each connection might be talking
> to a completely separate ceph node, and there may be no cache
> consistency between connections.

Yes, if you don't have a cache coherent backend you are generally
screwed with a multiqueue protocol.

[toc] | [prev] | [next] | [standalone]

#1484135

> On 15 Sep 2016, at 13:41, Christoph Hellwig <hch@infradead.org> wrote:
> 
> On Thu, Sep 15, 2016 at 01:39:11PM +0100, Alex Bligh wrote:
>> That's probably right in the case of file-based back ends that
>> are running on a Linux OS. But gonbdserver for instance supports
>> (e.g.) Ceph based backends, where each connection might be talking
>> to a completely separate ceph node, and there may be no cache
>> consistency between connections.
> 
> Yes, if you don't have a cache coherent backend you are generally
> screwed with a multiqueue protocol.

I wonder if the ability to support multiqueue should be visible
in the negotiation stage. That would allow the client to refuse
to select multiqueue where it isn't safe.

Wouter?

-- 
Alex Bligh

[toc] | [prev] | [next] | [standalone]

#1484158

On Thu, Sep 15, 2016 at 01:44:29PM +0100, Alex Bligh wrote:
> 
> > On 15 Sep 2016, at 13:41, Christoph Hellwig <hch@infradead.org> wrote:
> > 
> > On Thu, Sep 15, 2016 at 01:39:11PM +0100, Alex Bligh wrote:
> >> That's probably right in the case of file-based back ends that
> >> are running on a Linux OS. But gonbdserver for instance supports
> >> (e.g.) Ceph based backends, where each connection might be talking
> >> to a completely separate ceph node, and there may be no cache
> >> consistency between connections.
> > 
> > Yes, if you don't have a cache coherent backend you are generally
> > screwed with a multiqueue protocol.
> 
> I wonder if the ability to support multiqueue should be visible
> in the negotiation stage. That would allow the client to refuse
> to select multiqueue where it isn't safe.

The server can always refuse to allow multiple connections.

I was thinking of changing the spec as follows:

diff --git a/doc/proto.md b/doc/proto.md
index 217f57e..cb099e2 100644
--- a/doc/proto.md
+++ b/doc/proto.md
@@ -308,6 +308,23 @@ specification, the
 [kernel documentation](https://www.kernel.org/doc/Documentation/block/writeback_cache_control.txt)
 may be useful.
 
+For performance reasons, clients MAY open multiple connections to the
+same server. To support such clients, servers SHOULD ensure that at
+least one of the following conditions hold:
+
+* Flush commands are processed for ALL connections. That is, when an
+  `NBD_CMD_WRITE` is processed on one connection, and then an
+  `NBD_CMD_FLUSH` is processed on another connection, the data of the
+  `NBD_CMD_WRITE` on the first connection MUST reach permanent storage
+  before the reply of the `NBD_CMD_FLUSH` is sent.
+* The server allows `NBD_CMD_WRITE` and `NBD_CMD_FLUSH` on at most one
+  connection
+* Multiple connections are not allowed
+
+In addition, clients using multiple connections SHOULD NOT send
+`NBD_CMD_FLUSH` if an `NBD_CMD_WRITE` for which they care in relation to
+the flush has not been replied to yet.
+
 #### Request message
 
 The request message, sent by the client, looks as follows:

The latter bit (on the client side) is because even if your backend has
no cache coherency issues, TCP does not guarantee ordering between
multiple connections. I don't know if the above is in line with what
blk-mq does, but consider the following scenario:

- A client sends two writes to the server, followed (immediately) by a
  flush, where at least the second write and the flush are not sent over
  the same connection.
- The first write is a small one, and it is handled almost immediately.
- The second write takes a little longer, so the flush is handled
  earlier than the second write
- The network packet containing the flush reply gets lost for whatever
  reason, so the client doesn't get it, and we fall into TCP
  retransmits.
- The second write finishes, and its reply header does not get lost
- After the second write reply reaches the client, the TCP retransmits
  for the flush reply are handled.

In the above scenario, the flush reply arrives on the client side after
a write reply which it did not cover; so the client will (incorrectly)
assume that the write has reached permanent storage when in fact it may
not have done so yet.

If the kernel does not care about the ordering of the two writes versus
the flush, then there is no problem. I don't know how blk-mq works in
that context, but if the above is a likely scenario, we may have to
reconsider adding blk-mq to nbd.

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
       people in the world who think they really understand all of its rules,
       and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12

[toc] | [prev] | [next] | [standalone]

#1484303

Josef,

> On 15 Sep 2016, at 14:57, Josef Bacik <jbacik@fb.com> wrote:
> 
> This isn't an NBD problem, this is an application problem.  The application must wait for all writes it cares about _before_ issuing a flush.  This is the same as for normal storage as it is for NBD.  It is not NBD's responsibility to maintain coherency between multiple requests across connections, just simply to act on and respond to requests.
> 
> I think changing the specification to indicate that this is the case for multiple connections is a good thing, to keep NBD servers from doing weird things like sending different connections to the same export to different backing stores without some sort of synchronization.  It should definitely be explicitly stated somewhere that NBD does not provide any ordering guarantees and that is up to the application.  Thanks,

I don't think that's correct.

The block stack issues a flush to mean (precisely) "do not reply to this until all preceding writes THAT HAVE BEEN REPLIED TO have been persisted to non-volatile storage".

The danger is with multiple connections (where apparently only one flush is sent - let's say down connection 1) that not al the writes that have been replied to on connection 2 have been persisted to non-volatile storage. Only the ones on connection 1 have been persisted (this is assuming the nbd server doesn't 'link' in some way the connections).

There's nothing the 'application' (here meaning the kernel or higher level) can do to mitigate this. Sure it can wait for all the replies, but this doesn't guarantee the writes have been persisted to non-volatile storage, precisely because writes may return prior to this.

-- 
Alex Bligh

[toc] | [prev] | [next] | [standalone]

#1484373

Wouter,

> The server can always refuse to allow multiple connections.

Sure, but it would be neater to warn the client of that
at negotiation stage (it would only be one flag, e.g.
'multiple connections unsafe'). That way the connection
won't fail with a cryptic EBUSY or whatever, but will
just negotiate a single connection.

> I was thinking of changing the spec as follows:
> 
> diff --git a/doc/proto.md b/doc/proto.md
> index 217f57e..cb099e2 100644
> --- a/doc/proto.md
> +++ b/doc/proto.md
> @@ -308,6 +308,23 @@ specification, the
> [kernel documentation](https://www.kernel.org/doc/Documentation/block/writeback_cache_control.txt)
> may be useful.
> 
> +For performance reasons, clients MAY open multiple connections to the
> +same server. To support such clients, servers SHOULD ensure that at
> +least one of the following conditions hold:
> +
> +* Flush commands are processed for ALL connections. That is, when an
> +  `NBD_CMD_WRITE` is processed on one connection, and then an
> +  `NBD_CMD_FLUSH` is processed on another connection, the data of the
> +  `NBD_CMD_WRITE` on the first connection MUST reach permanent storage
> +  before the reply of the `NBD_CMD_FLUSH` is sent.
> +* The server allows `NBD_CMD_WRITE` and `NBD_CMD_FLUSH` on at most one
> +  connection
> +* Multiple connections are not allowed
> +
> +In addition, clients using multiple connections SHOULD NOT send
> +`NBD_CMD_FLUSH` if an `NBD_CMD_WRITE` for which they care in relation to
> +the flush has not been replied to yet.
> +

I don't think that should be a mandatory behaviour. For once, it would
be reasonably easy on gonbdserver but a PITA on the reference server.
You'd need to put in IPC between each of the forked processes OR rely
on fdatasync() only - I'm not sure that would necessarily work
safely with (e.g.) the 'treefiles' / COW options.

I think better would be to say that the server MUST either

* Not support NBD_CMD_FLUSH at all
* Support NBD_CMD_FLUSH across channels (as you set out above), or
* Indicate that it does not support multiple channels.

> #### Request message
> 
> The request message, sent by the client, looks as follows:
> 
> The latter bit (on the client side) is because even if your backend has
> no cache coherency issues, TCP does not guarantee ordering between
> multiple connections. I don't know if the above is in line with what
> blk-mq does, but consider the following scenario:
> 
> - A client sends two writes to the server, followed (immediately) by a
>  flush, where at least the second write and the flush are not sent over
>  the same connection.
> - The first write is a small one, and it is handled almost immediately.
> - The second write takes a little longer, so the flush is handled
>  earlier than the second write
> - The network packet containing the flush reply gets lost for whatever
>  reason, so the client doesn't get it, and we fall into TCP
>  retransmits.
> - The second write finishes, and its reply header does not get lost
> - After the second write reply reaches the client, the TCP retransmits
>  for the flush reply are handled.
> 
> In the above scenario, the flush reply arrives on the client side after
> a write reply which it did not cover; so the client will (incorrectly)
> assume that the write has reached permanent storage when in fact it may
> not have done so yet.
> 
> If the kernel does not care about the ordering of the two writes versus
> the flush, then there is no problem. I don't know how blk-mq works in
> that context, but if the above is a likely scenario, we may have to
> reconsider adding blk-mq to nbd.

Actually I think this is a problem anyway. A simpler failure case is
one where (by chance) one channel gets the writes, and one channel
gets the flushes. The flush reply is delayed beyond the replies to
unconnected writes (on the other channel) and hence the kernel thinks
replied-to writes have been persisted when they have not been.

The only way to fix that (as far as I can see) without changing flush
semantics is for the block layer to issue flush requests on each
channel when flushing on one channel. This, incidentally, would
resolve every other issue!

-- 
Alex Bligh

[toc] | [prev] | [next] | [standalone]

#1484390

On Thu, Sep 15, 2016 at 05:08:21PM +0100, Alex Bligh wrote:
> Wouter,
> 
> > The server can always refuse to allow multiple connections.
> 
> Sure, but it would be neater to warn the client of that at negotiation
> stage (it would only be one flag, e.g.  'multiple connections
> unsafe').

I suppose that's not a bad idea.

[...]
> > I was thinking of changing the spec as follows:
> > 
> > diff --git a/doc/proto.md b/doc/proto.md
> > index 217f57e..cb099e2 100644
> > --- a/doc/proto.md
> > +++ b/doc/proto.md
> > @@ -308,6 +308,23 @@ specification, the
> > [kernel documentation](https://www.kernel.org/doc/Documentation/block/writeback_cache_control.txt)
> > may be useful.
> > 
> > +For performance reasons, clients MAY open multiple connections to the
> > +same server. To support such clients, servers SHOULD ensure that at
> > +least one of the following conditions hold:
> > +
> > +* Flush commands are processed for ALL connections. That is, when an
> > +  `NBD_CMD_WRITE` is processed on one connection, and then an
> > +  `NBD_CMD_FLUSH` is processed on another connection, the data of the
> > +  `NBD_CMD_WRITE` on the first connection MUST reach permanent storage
> > +  before the reply of the `NBD_CMD_FLUSH` is sent.
> > +* The server allows `NBD_CMD_WRITE` and `NBD_CMD_FLUSH` on at most one
> > +  connection
> > +* Multiple connections are not allowed
> > +
> > +In addition, clients using multiple connections SHOULD NOT send
> > +`NBD_CMD_FLUSH` if an `NBD_CMD_WRITE` for which they care in relation to
> > +the flush has not been replied to yet.
> > +
> 
> I don't think that should be a mandatory behaviour.

Which part of it?

> For once, it would
> be reasonably easy on gonbdserver but a PITA on the reference server.
> You'd need to put in IPC between each of the forked processes OR rely
> on fdatasync() only - I'm not sure that would necessarily work
> safely with (e.g.) the 'treefiles' / COW options.
> 
> I think better would be to say that the server MUST either
> 
> * Not support NBD_CMD_FLUSH at all

I think we should discourage not supporting FLUSH, rather than
suggesting it. 

> * Support NBD_CMD_FLUSH across channels (as you set out above), or
> * Indicate that it does not support multiple channels.

You dropped the one with no writes. I said "at most" there for a reason.
Originally I was going to say "if the server is read-only", but then
thought that it could work to do the "at most" thing. After having given
that some more thought, I now realize that if you write, you need to
flush across to other channels, regardless of whether they write too, so
that bit of it is moot now anyway.

Still, a server which exports read-only should still be safe for
multiple connections, even if there is no cache coherency (since
presumably nothing's going to change anyway).

[...]
> > The latter bit (on the client side) is because even if your backend has
> > no cache coherency issues, TCP does not guarantee ordering between
> > multiple connections. I don't know if the above is in line with what
> > blk-mq does, but consider the following scenario:
> > 
> > - A client sends two writes to the server, followed (immediately) by a
> >  flush, where at least the second write and the flush are not sent over
> >  the same connection.
> > - The first write is a small one, and it is handled almost immediately.
> > - The second write takes a little longer, so the flush is handled
> >  earlier than the second write
> > - The network packet containing the flush reply gets lost for whatever
> >  reason, so the client doesn't get it, and we fall into TCP
> >  retransmits.
> > - The second write finishes, and its reply header does not get lost
> > - After the second write reply reaches the client, the TCP retransmits
> >  for the flush reply are handled.
> > 
> > In the above scenario, the flush reply arrives on the client side after
> > a write reply which it did not cover; so the client will (incorrectly)
> > assume that the write has reached permanent storage when in fact it may
> > not have done so yet.
> > 
> > If the kernel does not care about the ordering of the two writes versus
> > the flush, then there is no problem. I don't know how blk-mq works in
> > that context, but if the above is a likely scenario, we may have to
> > reconsider adding blk-mq to nbd.
> 
> Actually I think this is a problem anyway. A simpler failure case is
> one where (by chance) one channel gets the writes, and one channel
> gets the flushes. The flush reply is delayed beyond the replies to
> unconnected writes (on the other channel) and hence the kernel thinks
> replied-to writes have been persisted when they have not been.

Yes, that is another example of essentially the same problem.

> The only way to fix that (as far as I can see) without changing flush
> semantics is for the block layer to issue flush requests on each
> channel when flushing on one channel.

Christoph just said that that doesn't (currently) happen; I don't know
whether the kernel currently already (optimistically) sends out flush
requests before the writes that it expects to hit permanent storage have
finished, but if it doesn't do that, then there is no problem and my
suggested bit of spec would be okay.

If there are good reasons to do so, however, we do indeed have a problem
and something else is necessary. I don't think flushing across all
connections is the best solution, though.

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
       people in the world who think they really understand all of its rules,
       and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12

[toc] | [prev] | [next] | [standalone]

#1484410

Wouter,

> On 15 Sep 2016, at 17:27, Wouter Verhelst <w@uter.be> wrote:
> 
> On Thu, Sep 15, 2016 at 05:08:21PM +0100, Alex Bligh wrote:
>> Wouter,
>> 
>>> The server can always refuse to allow multiple connections.
>> 
>> Sure, but it would be neater to warn the client of that at negotiation
>> stage (it would only be one flag, e.g.  'multiple connections
>> unsafe').
> 
> I suppose that's not a bad idea.

Good.

> [...]
>>> I was thinking of changing the spec as follows:
>>> 
>>> diff --git a/doc/proto.md b/doc/proto.md
>>> index 217f57e..cb099e2 100644
>>> --- a/doc/proto.md
>>> +++ b/doc/proto.md
>>> @@ -308,6 +308,23 @@ specification, the
>>> [kernel documentation](https://www.kernel.org/doc/Documentation/block/writeback_cache_control.txt)
>>> may be useful.
>>> 
>>> +For performance reasons, clients MAY open multiple connections to the
>>> +same server. To support such clients, servers SHOULD ensure that at
>>> +least one of the following conditions hold:
>>> +
>>> +* Flush commands are processed for ALL connections. That is, when an
>>> +  `NBD_CMD_WRITE` is processed on one connection, and then an
>>> +  `NBD_CMD_FLUSH` is processed on another connection, the data of the
>>> +  `NBD_CMD_WRITE` on the first connection MUST reach permanent storage
>>> +  before the reply of the `NBD_CMD_FLUSH` is sent.
>>> +* The server allows `NBD_CMD_WRITE` and `NBD_CMD_FLUSH` on at most one
>>> +  connection
>>> +* Multiple connections are not allowed
>>> +
>>> +In addition, clients using multiple connections SHOULD NOT send
>>> +`NBD_CMD_FLUSH` if an `NBD_CMD_WRITE` for which they care in relation to
>>> +the flush has not been replied to yet.
>>> +
>> 
>> I don't think that should be a mandatory behaviour.
> 
> Which part of it?

I've read it again :-) The wording was slightly contorted. I think
what I mean is that if you don't support flush at all, that's
another option.

The final paragraph I am not sure is right, as that's not what the kernel
currently does. If we are going to suggest a change in our main client's
behaviour, should we not just request that flush is done on all channels?

>> For once, it would
>> be reasonably easy on gonbdserver but a PITA on the reference server.
>> You'd need to put in IPC between each of the forked processes OR rely
>> on fdatasync() only - I'm not sure that would necessarily work
>> safely with (e.g.) the 'treefiles' / COW options.
>> 
>> I think better would be to say that the server MUST either
>> 
>> * Not support NBD_CMD_FLUSH at all
> 
> I think we should discourage not supporting FLUSH, rather than
> suggesting it. 

Sure, but some backends just don't support flush. For them, this
aspect at least is not a problem.

>> * Support NBD_CMD_FLUSH across channels (as you set out above), or
>> * Indicate that it does not support multiple channels.
> 
> You dropped the one with no writes. I said "at most" there for a reason.
> Originally I was going to say "if the server is read-only", but then
> thought that it could work to do the "at most" thing. After having given
> that some more thought, I now realize that if you write, you need to
> flush across to other channels, regardless of whether they write too, so
> that bit of it is moot now anyway.
> 
> Still, a server which exports read-only should still be safe for
> multiple connections, even if there is no cache coherency (since
> presumably nothing's going to change anyway).

Yes

>> Actually I think this is a problem anyway. A simpler failure case is
>> one where (by chance) one channel gets the writes, and one channel
>> gets the flushes. The flush reply is delayed beyond the replies to
>> unconnected writes (on the other channel) and hence the kernel thinks
>> replied-to writes have been persisted when they have not been.
> 
> Yes, that is another example of essentially the same problem.

Yeah, I was just trying to simplify things.

>> The only way to fix that (as far as I can see) without changing flush
>> semantics is for the block layer to issue flush requests on each
>> channel when flushing on one channel.
> 
> Christoph just said that that doesn't (currently) happen; I don't know
> whether the kernel currently already (optimistically) sends out flush
> requests before the writes that it expects to hit permanent storage have
> finished, but if it doesn't do that, then there is no problem and my
> suggested bit of spec would be okay.
> 
> If there are good reasons to do so, however, we do indeed have a problem
> and something else is necessary. I don't think flushing across all
> connections is the best solution, though.

Well, the way I look at it is that we have a proposed change in
client behaviour (multiple channels) which causes problems at
least with flush and also (I think) with cache coherency (see other
email). We should either not make that change, or ensure other changes
are added which mitigate these issues.

Flush is actually the obvious one. Cache coherency is far more
subtle (though possibly fixable by something in the spec that
states that if multiple connections are supported, cache must
be coherent between them).

-- 
Alex Bligh

[toc] | [prev] | [next] | [standalone]

#1484473

[Multipart message — attachments visible in raw view] — view raw

On 09/15/2016 11:27 AM, Wouter Verhelst wrote:
> On Thu, Sep 15, 2016 at 05:08:21PM +0100, Alex Bligh wrote:
>> Wouter,
>>
>>> The server can always refuse to allow multiple connections.
>>
>> Sure, but it would be neater to warn the client of that at negotiation
>> stage (it would only be one flag, e.g.  'multiple connections
>> unsafe').
> 
> I suppose that's not a bad idea.

Seems like it may need to be a per-export flag, rather than a global
flag (as a given server may be able to serve multiple types of files,
where the safety depends on the type of file being served).


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

[toc] | [prev] | [next] | [standalone]

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

Back to top | Article view | linux.kernel

csiph-web