Re: [RFC][PATCH] nbd driver for 2.5+: fix locking issues with ioctl

[Lou Langholtz]

Paul Clements wrote:

>. . .
>This patch introduces a locking hierarchy between the lo->tx_lock and
>lo->queue_lock. The existing driver does not have this limitation.
>I would feel a lot better about this patch if you were to recode it
>to avoid this.
>
Did Al's statements regarding this make this feel better? The code could 
be redone so that the queuelist is added to before the down but then 
more often it will have to be removed from since the lo->sock check 
wouldn't have been done yet. On the other hand I've been thinking that I 
might be able to take advantage of the irq locked condition imposed by 
the q->queue_lock and just use nbd_lock to replace q->queue_lock then. 
Al and Andrew seem to have a much deeper understanding though for 
spinlocking though so I'll defer to there comments on this idea (of 
replacing lo->queue_lock by use of nbd_lock). This has the added 
attraction of already having nbd_lock locked when in do_nbd_request.

>Also, I noticed that you've removed the forcible shutdown of the
>socket at the end of NBD_DO_IT. Was there a particular reason for
>that?
>
Yes. The forcible shutdown that was put in place (while closing one race 
condition) still left a panicable race condition with the nbd-client 
tool. I forget exactly what this was at the moment. Suffice it to say 
that since the user space tool opened the socket to begin with, it seems 
a better design to have the user space tool do the close as well. When 
nbd-client exits, it'd effect close of this socket anyway even if 
killed. What still needs to be done though, is to have the 
implementation of the NBD_DISCONNECT ioctl in the driver wait until 
nbd_do_it sets lo->sock back to NULL. That way the NBD_CLEAR_QUE ioctl 
can return -EBUSY if lo->sock, while not getting called by nbd-client 
till lo->sock == NULL so that the que clear works.

>>... the new NBD_DO_IT style interface 
>>could be introduced instead as another ioctl completely and these 3 
>>ioctls could be maintained for backward compatibility for a while 
>>longer. 
>>    
>>
>
>It would be really nice if the driver remained (as much as
>possible) compatible with the 2.4 version...unless you have
>a really good reason to break things... :)
>  
>
So you'd prefer to have a new ioctl then to do this rolled together 
NBD_DO_IT function? Say NBD_RUN or something that takes the sock 
argument. That will require the nbd_device structure to maintain that 
file pointer again and possibly leave open the races that seem inherent 
to having the three seperate ioctl's. Someone else long ago commented in 
the driver "possible FIXME: make set_sock / set_blksize / set_size / 
do_it one syscall. Why not: would need verify_area and friends, would 
share yet another structure with userland". So it seems someone was long 
ago realizing there were problems having the three seperate ioctls for 
set_sock, do_it, and clear_sock. Of course this doesn't address 
set_size, set_blksize, but I don't see them as problematic w.r.t. 
locking once set_sock, do_it, and clear_sock are rolled together.

Seems like we're better off deprecating at least the set_sock and 
clear_sock ioctls to return some error code and having the nbd-client 
tool runtime switch off of something like their return value (or a 
release level value as was also suggested). It's maybe more painful in 
requiring people to update their nbd-client's but we  live with those 
kinds of required updates all the time (example mod-utils insmod, rmmod, 
from Rusty to boot 2.5 kernels). The benefit will be memory saving and 
better kernel stability. I'm more torn between the idea of deprecating 
all three ioctls and adding a NBD_RUN versus just deprecating 
NBD_SET_SOCK and NBD_CLEAR_SOCK and changing the NBD_DO_IT ioctl to 
require the socket descriptor.

Comments?