Re: [Qemu-devel] [PATCH 1/3] nbd: Only try to send flush/discard commands if connected to the NBD server

[Kevin Wolf <kwolf@redhat.com>]

Am 23.10.2012 13:08, schrieb Nicholas Thomas:
> On Tue, 2012-10-23 at 12:33 +0200, Kevin Wolf wrote:
>> Am 22.10.2012 13:09, schrieb nick@bytemark.co.uk:
>>>
>>> This is unlikely to come up now, but is a necessary prerequisite for reconnection
>>> behaviour.
>>>
>>> Signed-off-by: Nick Thomas <nick@bytemark.co.uk>
>>> ---
>>>  block/nbd.c |   13 +++++++++++--
>>>  1 files changed, 11 insertions(+), 2 deletions(-)
>>
>> What's the real requirement here? Silently ignoring a flush and
>> returning success for it feels wrong. Why is it correct?
>>
>> Kevin
> 
> I just needed to avoid socket operations while s->sock == -1, and
> extending the existing case of "can't do the command, so pretend I did
> it" to "can't do the command right now, so pretend..." seemed like an
> easy way out.

The difference is that NBD_FLAG_SEND_FLUSH is only clear if the server
is configured like that, i.e. it's completely in the control of the
user, and it ignores flushes consistently (for example because it
already uses a writethrough mode so that flushes aren't required).

Network outage usually isn't in the control of the user and ignoring a
flush request when the server actually asked for flushes is completely
surprising and dangerous.

> In the Bytemark case, the NBD server always opens the file O_SYNC, so
> nbd_co_flush could check in_flight == 0 and return 0/1 based on that;
> but I'd be surprised if that's true for all NBD servers. Should we be
> returning 1 here for both "not supported" and "can't do it right now",
> instead? 

The best return value is probably -ENOTCONN or -EIO if you must return
an error. But maybe using the same logic as for writes would be
appropriate, i.e. try to reestablish the connection before you return an
error.

Kevin