[ENBD] Re: the files of ENBD

[Peter T. Breuer]

"A month of sundays ago Wang Gang wrote:"
[Attachment, skipping...]

Hi .. let me take the opportunity onlist to thank you for the patched
files.  I believe they're against nbd-2.2.28. Let me try and comment
what I see:


driver (nbd.c):
  Change the parameter sync_intvl from a module load option to settable
  via a new ioctl.

This param determines how long before a request that's been taken up by 
a daemon will be waited for before the kernel gives up on the daemon
and offers it to someone else instead ("rollback"). It's normally 5s.
Surely there is no point in changing this after the module is loaded?

  Uncomment lines in nbd_do_req that will cause the device to fail incoming
  kernel requests when there are no client daemons registered.
  
I'd be very interested in hearing a report on the effect of that. It
may make the device error out requests that arrive just as it is
closing down, instead of letting the requests complete and reporting
error to the close. Can you look carefully at the release code and
see if you can find some race conditions?

But under linux closing a device seems to be an insoluble race
condition! An error return from close() is universially ignored by
programmers.

  Add a call to soft_reset in the clr_sock code that will cause the
  device to rollback and then error any pending requests when the last
  client daemon dies.
  
I think this addition is probably OK. The intent is to cause grevious
damage and to yell and shout when the daemons die. The only problem
is that you have a race condition when you want to turn the device off
_cleanly_, because the vfs may have requests still outstanding for us.

   Add a flag to say that you've set the timeout you intruduced and
   check for it in set_sock, which is when the daemons call to
   register themselves with the kernel.

This means that somebody has to set the flag via an ioctl. The
first daemon? The manager daemon? What's wrong with a default
value? I don't see why this value has to be set explicitly or
else death.

   In nbd.h, make appropriate changes, viz add flag and timeout to
   device struct.

One of us made a mistake in the flags for the device (ahhh, I see, you
are looking at 2.4.15? It's me). Your copy has:

  #define NBD_BLKSIZED    0x0040
  #define NBD_SYNC_REQD   0x0080
  #define NBD_SYNC_REQD   0x0080
  #define NBD_RLSE_REQD   0x0080

and I presume you and I meant not to repeat and to use 0x0100 for the
last! It doesn't matter. That was an experiment. I must have been 
trying to logically analyse the race condition at device turnoff.
Corrected!


client daemon (nbd-client.c):

   add new globals pulse_interval and data_timeout, in addition to
   the existing negotiate_timeout. Make pulse_interval a commandline
   option. Make pulse_interval negotiated with the server (choose
   max of two). SSSSend it down to kernel via an ioctl. set data
   timeout to three times pulse interval.

I would imagine you are differentiating different timeouts here. I
agree. that's good. Pulse interval is proably for the heartbeat timer.
Data timeout is probably the client's patience limit on getting a
reply from the server. I like that. Yes, you replaced my hardcoded
60s constant. Good.

I'm not sure of the sense here. Mmm .. maybe. Negotiation allows
one to leave more up to the daemons and less up to the command
line on each side, which is good. I'm glad I wrote a code skeleton
that you could follow!


server daemon (nbd-server.c):

  corresponding changes in negotiation  and a new -p commandline option
  for pulse interval. Add corresponding timeout variables as globals.
  Not sure, but I think there might be a new low level timeout in
  read/write to the net. 

I can go with all those. All fine code development. I'll try and work
in the timeout stuff now. But there is something wrong with unix
alarms plus use of select. And the alarms don't stack, they preempt
each other. That's the real problem.

Peter