Re: [Nbd] Question about the expected behaviour of nbd-server for async ops

[Goswin von Brederlow <goswin-v-b@...186...>]

Wouter Verhelst <w@...112...> writes:

> On Sun, May 29, 2011 at 09:53:44AM +0100, Alex Bligh wrote:
>> >> > * NBD_CMD_DISC: Wait for all pending requests to finish, close socket
>> >>
>> >> You should reply to all pending requests prior to closing the socket
>> >> I believe, mostly as it's polite. I believe the current client doesn't
>> >> send a disconnect until all replies are in,
>> >
>> > I believe so too, yes.
>> >
>> > [...]
>> >> and I also think the server may behave a little badly here.
>> >
>> > How so?
>> 
>> I /believe/ what happens is that the server just closes the socket and
>> quits.
>
> True.

But since all requests are done synchronously there are no pending
requests and closing the socket is all there is left to do. So currently
that behaviour is as it should be.

>> I think it does not wait until any unsent replies have been sent
>> (i.e replies that have been generated and are queued in the socket
>> layer), which may result in them being lost. This is just code reading
>> rather than testing. It's not a problem with the current client
>> because it waits for all replies to come in before sending
>> NBD_CMD_DISC.
>
> Hmm. I must admit I hadn't considered what would happen if you closed a
> socket that still had unsent data in its buffers; I guess I just blindly
> assumed it would flush its buffers, but that might have been wrong.
>
> This is what posix has to say about the matter:
>
>        If  fildes  refers  to  a  socket, close() shall cause the socket to be
>        destroyed. If the socket  is  in  connection-mode,  and  the  SO_LINGER
>        option  is set for the socket with non-zero linger time, and the socket
>        has untransmitted data, then close() shall block for up to the  current
>        linger interval until all data is transmitted.
>
> (http://pubs.opengroup.org/onlinepubs/009695399/functions/close.html)
>
> So, in other words, we should probably be setting the SO_LINGER sockopt
> on our sockets. I'll look into that.

Here is what i think should happen:
- on recieving a NBD_CMD_DISC request you shutdown(fd, SHUT_RD)
- process and reply any pending requests
- fsync() /* implicit flush, just to be nice */
- shutdown(fd, SHUT_WR)
- close(fd)
- exit()

I think SO_LINGER does not affect wether data is send over the socket
or not. It only affects the time the kerel tries to send the data and
makes close() wait for that time (or all data being send).

Using SO_LINGER would help free up sockets faster if the client doesn't
shutdown gracefully I think. Shouldn't normaly affect what gets send
over the socket.

>> >> >   Should this flush data before closing the socket? And if so what if
>> >> >   there is an error on flush? I guess clients should send NBD_CMD_FLUSH
>> >> >   prior to NBD_CMD_DISC if they care.
>> >>
>> >> No, you should not rely on this happening. Even umount of an ext2 volume
>> >> will not send NBD_FLUSH where kernel, client, and server support it.
>> >> You don't need to write it then and there (in fact there is no 'then
>> >> and there' as an NBD_CMD_DISC has no reply),
>> >
>> > It does have one -- the FIN packet. But yeah, it's not an
>> > application-layer reply, that much is true.
>> 
>> Which is mildly irritating, particularly if you want to make sure the
>> client doesn't do other stuff until you've processed the disconnect.
>
> I suppose we should return an error if a client tries to do anything
> else after having sent NBD_CMD_DISC.
>
>> >> >   What if there are more requests after this while waiting for pending
>> >> >   requests to finish? Should they be ignored or return an error?
>> >>
>> >> I believe it is an, um, undocumented implicit assumption that no
>> >> commands are sent after NBD_CMD_DISC is sent. The current server
>> >> just closes the socket, which will probably result in an EPIPE
>> >> upstream if the FIN packet gets back before these other commands
>> >> are written.
>> >
>> > The client will flush its outgoing queue before sending a disconnect
>> > request. Indeed, if it didn't do that, badness would ensue.
>> 
>> Oh sure. I think Goswin was asking "can it change its mind and send
>> other stuff afterwards?" - the answer is no, NBD_CMD_DISC must be
>> the last command sent.
>
> Right.

I was thinking of a buggy or malicious client. Say there is a bug in the
linux kernel so it sends the NBD_CMD_DISC followed by a NBD_CMD_READ.
Then we tear down the connection and never reply to the READ. Is that
better than replying with an error to the READ?

MfG
        Goswin