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??
}
> 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.
> > // Open the device to force reread of the partition table.
> > if (!fork()) {
> > char *s = strrchr(device, '/');
> > sprintf(data, "/sys/block/%.32s/pid", s ? s+1 :
> > device); // Is it up yet?
> > for (;;) {
> > temp = open(data, O_RDONLY);
> > if (temp == -1) sleep(1);
> > else {
> > close(temp);
> > break;
> > }
> > }
> > close(open(device, O_RDONLY));
> > exit(0);
> > }
> >
> > Why do you do it in a child? nbd-client's parent might actually
> > prefer to know that this step is complete before nbd-client
> > exits. As it stands, now reread happens "somewhere in the future,
> > maybe in 2050, who knows".
>
> A) They can't happen in the same process, ioctl(NBD_DO_IT) blocks and the
> open
> won't succeed until some process is in that ioctl() handling packets.
>
> B) The NBD daemon doesn't _use_ the partition information, and in its normal
> use case it daemonizes so you can't tell when it's done anyway. The
> information propogates through udev/mdev and you can trigger on new devices
> showing up that way, so it's already an external hotplug callback.
>
> Calling the ioctl(NBD_DO_IT) sits there and processes NBD requests, it
> doesn't
> return until you disconnect the NBD from the server. That IOCTL turns the
> process that calls it into a dedicated NBD request handler. The partition
> table doesn't get re-read until the first open() of the NBD _after_ the
> request
> handler gets set up (due to some race in the kernel that makes it hard to do
> in-kernel).
>
> 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.
> > // 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?
--
vda
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
