Re: [RFC] usbfs2 aio cancellation code

On Tue, 2 Oct 2007, Zach Brown wrote:

> > Do things really need to be this complicated?
>
> Do you think the reply I sent to Sarah safely relies on usb_unlink_urb
> ()?  I don't know if it provides the guarantees which that proposal
> would rely on.
 
The guarantees it provides aren't exactly what Sarah wrote, but
they are close.
 
> What happens if you call usb_unlink_urb() after the URB callback has  
> been called?

Then usb_unlink_urb() fails.  But it might not return -EBUSY; it might
return some other error code instead.

> I'm hoping that we can *just* call usb_unlink_urb() from ki_cancel  
> and *always* return -EBUSY.  The URB callback will either be called  
> very soon on error or will already be in flight or have been called.

Right, there's no point in having ki_cancel ever complete an aio 
operation.  The operation should complete when the URB completes, 
whether it succeeded, was unlinked, or encountered some other error.

> The trick there is to make sure that the URB callback doesn't free  
> resources that ki_cancel needs to be able to call usb_unlink_urb() to  
> find that it's already been called.  I would propose putting a struct  
> on iocb->private which is freed in the ki_dtor method.  It's called  
> once all the iocb references have been dropped.

Agreed.

> Hmm.  If usb_unlink_urb() returns 0 then operation is still in  
> flight, right?

No.  usb_unlink_urb() is weird; it returns -EINPROGRESS to indicate
success.  Don't ask me why.  Presumably that will be fixed eventually.

But even when usb_unlink_urb() succeeds, it doesn't mean that the URB
is still in flight when the function call returns.  The URB may have
already completed.  And conversely, when usb_unlink_urb() fails, it
doesn't mean the URB has completed yet.

> I got lost in the pseudo code for testing for cancelation and  
> things.  How about this:
> 
> aio_ method:
> 	/* this does not race with ki_cancel */
> 	iocb->private = alloc_urb_goo();
> 	iocb->ki_dtor = free_urb_goo();
> 	queue URB callback
> 	if (read) {
> 		iocb->ki_retry = copy_read_result();
> 		return -EIOCBRETRY:
> 	} else
> 		return -EIOCBQUEUED:
> 
> URB callback:
> 	/* usb_unlink_urb() protects the race with ki_cancel */
> 	/* this cannot race with ki_retry, it triggers it */
> 	if (read)
> 		kick_iocb();
> 	else
> 		aio_complete();
> 
> copy_read_result:
> 	if (error)
> 		return error'
> 	copy_to_user();
> 	return bytes_copied;
> 
> ki_retry:

You mean ki_cancel:

> 	some_usb_goo = iocb->private;
> 	/* usb_unlink_urb() protects us from races */
> 	/* urb goo won't be freed until ki_dtor, which we hold a reference  
> for */
> 	usb_unlink_urb(some_usb_goo);
> 	return -EBUSY;
> 
> ki_dtor:
> 	free(iocb->private);
> 
> That should work, presuming that usb_unlink_urb() has strong internal  
> serialization to have calls from ki_cancel fail if the callback is  
> pending or has been completed already.

Yes, I think this will work.

Alan Stern


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel