Standalone network block device client now working. :)

[Rob Landley]

On Wednesday 22 September 2010 19:09:10 Denys Vlasenko wrote:
> On Wednesday 22 September 2010 21:09, Rob Landley wrote:
> > On Tuesday 21 September 2010 19:19:17 Denys Vlasenko wrote:
> > > Outer for(;;) loop looks wrong. For one, on the second iteration
> > > nbd file descriptor will be closed. I have doubts ioctls can work
> > > on closed fds. Repeatedly daemonizing also looks strange.
> > >
> > > I imagine the loop should just be removed, body should execute just
> > > once.
> >
> > The problem is the ioctl _can_ exit.  I'm not sure the circumstances
> > under which it does so, could be a certain number of dropped packets, or
> > it could be the server rebooted.  Not sure.  (I haven't rewritten the
> > server side yet, no immediate call for it here.)
>
> My problem with this loop is here:
>
>         // Make sure the /dev/nbd exists.
>         nbd = xopen(device, O_RDWR);
>
>         // Repeat until spanked
>         for (;;) {
> ...
>                 // Daemonize here.
>                 daemon(0, 0);   <==== why do we daemonize on each
> iteration??
>
>                 // Process NBD requests until further notice.
>                 if (ioctl(nbd, NBD_DO_IT)>=0 || errno==EBADR)
>                         break;
>                 close(sock);
>                 close(nbd);   <==== ?! so on 2nd iteration nbd is closed??
>         }

Because I wasn't paying attention. :)

Both should probably move outside the loop.

> > In theory you could call a non-daemon version of NBD from an upstart
> > variant that respawns it when it goes away, and thus bump this retry up
> > to an external level.  However, the retry doesn't re-login, it only
> > retries with the same login credentials, and if it gets an EBADR return
> > it goes "oh, login credentis are stale, exit" so it doesn't try to swap
> > in a _different_ block device under an existing active mount (which would
> > be bad).
> >
> > So without the retry, it's less reliable. With an externally imposed
> > retry, it may increase the likelihood of data corruption.  (I also dunno
> > under what circumstances a mount on top of the network block device fails
> > and unmounts itself.)
> >
> > If the loop isn't working right now, removing it is reasonable.
>
> I am thinking along "it is obviously broken as it is coded right now. It
> can't possibly work if it does more than one iteration" lines.

Agreed.  Obviously the loop hasn't _triggered_ for me yet.  As I said, I dunno 
under what circumstances the ioctl() exits and needs to be restarted.  
Possibly current kernels don't, possibly it's when the nbd-server reboots...

> > So the process that calls ioctl(NBD_DO_IT) can't be the one that calls
> > the open() to force the partition table re-read, because the open() won't
> > succeed until there's already a request handler running, and the request
> > handler doesn't return while the NBD is connected.
>
> Understood.
>
> We have a problem here. fork() is not available on NOMMU.

Well, for the daemonize() presumably there's an iommu-safe libbb variant.

In theory the connection check it's doing (forcing a partition reread) is also 
done via the -c option (I.E. "nbd-client -c /dev/nbd0"), so you could 
implement that, then call whatever libbb function does the 
exec(/proc/self/exe) trick (it was xexec() in toybox, I forget what name 
busybox used), and then have the vfork() xexec() "nbd-client -c $1".

Sound like a thing?

> > >                 // Process NBD requests until further notice.
> > >
> > >                 if (ioctl(nbd, NBD_DO_IT)>=0 || errno==EBADR) break;
> > >
> > > Need more comments here.
> > > Does this ioctl block for a long time, processing many requests?
> >
> > Yes.  It's the in-kernel request handler, which requires a process
> > context. It's where the daemon spends the rest of its runtime after doing
> > the setup.
>
> I committed the code to git after fixing it up so that it looks non-buggy
> wrt closed fd problem and NOMMU problem.
>
> Can you test current git?

Not near the ndb-server at the moment, but I'll give it a whack tomorrow.

Thanks,

Rob
-- 
GPLv3: as worthy a successor as The Phantom Menace, as timely as Duke Nukem 
Forever, and as welcome as New Coke.