On Wed, 7 Dec 2011, Sarah Sharp wrote: > > > I feel like I'm going > > > in mental circles, and missing information about how the SCSI layer > > > works. > > > > What information? > > I'm trying to figure out which functions in the UAS driver are entered > with the SCSI host lock taken by the SCSI core. In reality, none of them. > You've already said > that the function to queue commands will have the SCSI host lock held. Look more closely. There's uas_queuecommand_lck(), which does the real work, and uas_queuecommand(), created by the DEF_SCSI_QCMD macro. That one just grabs the host lock, assigns a serial number to the command, calls the first routine, and releases the lock. You can bypass the macro entirely if you want. > There's an interesting note in include/scsi/scsi_host.h: > > /* > * This is an error handling strategy routine. You don't need to > * define one of these if you don't want to - there is a default > * routine that is present that should work in most cases. For those > * driver authors that have the inclination and ability to write their > * own strategy routine, this is where it is specified. Note - the > * strategy routine is *ALWAYS* run in the context of the kernel eh > * thread. Thus you are guaranteed to *NOT* be in an interrupt > * handler when you execute this, and you are also guaranteed to > * *NOT* have any other commands being queued while you are in the > * strategy routine. When you return from this function, operations > * return to normal. > * > * See scsi_error.c scsi_unjam_host for additional comments about > * what this function should and should not be attempting to do. > * > * Status: REQUIRED (at least one of them) > */ > int (* eh_abort_handler)(struct scsi_cmnd *); > int (* eh_device_reset_handler)(struct scsi_cmnd *); > int (* eh_target_reset_handler)(struct scsi_cmnd *); > int (* eh_bus_reset_handler)(struct scsi_cmnd *); > int (* eh_host_reset_handler)(struct scsi_cmnd *); > > Does that note mean that the SCSI host lock is held when > eh_abort_handler and eh_host_reset_handler are called? No. Those methods are called in process context with no spinlocks held. > The note also seems to imply that since there's only one thread that's > calling into the error handling methods, only one of the methods will be > called at a time. No, that's not right. There is only one error handler thread, but other threads can call those methods. For example, a user program can ask the SCSI-generic driver to do a device, target, bus, or host reset. > If the SCSI lock is held when those methods are > called, and only one of them is called at once, then that loosens the > constraints that I have to place in the UAS driver. > > I asked James Bottomley on IRC about it yesterday, but he didn't answer. Unfortunately he often doesn't respond quickly (or at all) to technical questions. It can be frustrating... > Your suggestion about how to synchronize the resetting flag involved > using the SCSI host lock. But if the lock is taken by the SCSI core > before it calls eh_host_reset_handler and then another driver attempts > to reset the device and pre-reset tries to take the lock to sync the > resetting flag, then we have a deadlock. Nope, the lock isn't held. And of course, you don't have to have to use the host lock if you don't want to. Just put a private lock in your devinfo structure. You already have a lock for the workqueue anyway. (BTW, why is the devinfo structure allocated separately? Is there any reason not to embed it in the Scsi_Host structure, the way usb-storage does?) > > > It would be best if you wrote this patch yourself. > > > > > > There's nothing strictly speaking wrong with the original patchset, > > > aside from the race condition between the abort handler and > > > uas_stat_cmplt, is there? > > > > I don't know; I didn't look at the patches in any detail. > > Ok, you didn't look at the whole patchset. Did you look at this patch > in detail? Probably not enough detail. The description didn't seem right, so there wasn't much point poring through the code. On the whole it appears more or less okay. There are a few small things. For example, the srcu_struct should be embedded in devinfo rather than allocated dynamically. > What will the SCSI layer do if it tries to cancel a SCSI command, the > abort fails, but then the scsi_done function gets called with a > successful status? I don't know, because I'm not a SCSI expert. Once a command times out, scsi_done doesn't do anything. I think you don't want the abort routine to return failure when a reset is in progress -- if it does, the SCSI error handler will start its own device reset. (In fact, it looks like usb-storage returns the wrong value here.) Just wait until the command completes, one way or another, and return success. Or maybe return FAST_IO_FAIL, which doesn't appear to be documented anywhere but is used in the SCSI error handler. > From a UAS perspective, I think we're fine. If the completion handler > is running at the same time as the abort handler, we'll issue an ABORT > TASK command for that tag. If the device has completed the tag, it will > just respond with an error status to the ABORT TASK and continue on its > way. We're in abort handler and that note says we won't see new > commands issued while we're in an error handling function. That means > even if the tag completes and the UAS driver marks it as being free for > new commands, we won't issue a new command with that tag until the abort > handler returns. So the UAS driver can't ever accidentally abort a tag > it didn't mean to abort. > > If the ABORT TASK does return successfully, then we know the completion > handler for the status URB is never going to run, and we just kill them. (If you kill an URB, its completion handler does nevertheless run.) The only thing to watch out for would be if the completion handler gets rid of some data structure while the abort handler is still using it. If that's not an issue in uas then there's nothing to worry about. > (BTW, I realized how awful it sounds to talk about killing URBs in > public when I was trying to explain this issue with my husband in a > restaurant last night. I said "I just don't care about them, so we'll > kill them off" a little too loud.) Did you get any funny looks from other people? :-) > > What you should do is break the problem into two pieces. Work on > > aborting first, and then add resetting. I suspect aborting may be more > > complex then resetting. > > > > For aborting, the races to worry about are abort/resubmit and > > abort/complete. As mentioned above, abort/resubmit could be handled > > via SRCU but abort/complete may need some sort of lock. In short, the abort handler probably shouldn't return until the command has completed, either successfully or unsuccessfully. (My personal impression is that the design of the SCSI and block layers is far too baroque; parts of it -- like this one -- ought to be simplified.) > Is there any problem with using two SRCU reader locks in the same > driver? No. > Can a function take an SRCU reader lock (say for the resetting > flag) and also synchronize the readers for another flag (say the command > abort flag)? Yes. But it mustn't call synchronize_srcu() if it's holding the corresponding read lock, obviously. > > For resetting, the races to worry about are reset/queuecommand and > > waiting for all outstanding commands to complete. The queuecommand > > race could be handled with either regular RCU or a spinlock. Either a > > private lock or the host lock (you know, uas_queuecommand doesn't > > have to be called with the host lock held if you don't want it). > > Do you mean I can ask the SCSI core to not hold the host lock when it > calls into the queue function? Or is that always true and the driver > needs to grab the scsi host lock? See above. > > Waiting for the outstanding commands to complete can be done without > > locking if you don't mind wasting a little time in pre_reset. All you > > have to do is test an atomic commands_in_flight counter, and if it is > > non-zero then sleep for a few tens of milliseconds and try again. > > There's also a race condition between the pre-reset function and the > abort handler, since the abort handler can submit URBs to issue the > ABORT TASK command. > > Say there's an atomic counter, and a resetting flag. > > 1. The abort handler starts to run, and doesn't see the resetting flag, > but gets scheduled before it can submit the ABORT TASK command and > increment the number of outstanding commands. > > 2. The completion handler for the status URB runs and decrements the > count to zero. > > 3. Meanwhile, another driver asks for a reset, and pre-reset is called, > which finds the outstanding count zero. Before it can tear down the > stream rings, the context is swapped out for the abort handler thread. > > 4. The abort handler thread submits the ABORT TASK command and waits on > it. At this point, if the pre-reset handler is swapped back in, > it's going to happily free the stream rings while the abort task > command is still on them. Yes, this could be another application for SRCU. Or you could simply use your spinlock, since neither the abort handler nor the reset routine is on the fast path. > Faced with this sort of race condition, and a lack of knowledge about > how the SCSI layers work with the scsi_host lock, this is where I start > walking in mental circles, trying to devise a solution that doesn't > involve the original RCU synchronization between the pre-reset function > and the abort handler. I hope things are a little clearer now. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
