[sane-devel] USB device locking in the snapscan backend

[Julien BLACHE]

Oliver Schwartz <Oliver.Schwartz at gmx.de> wrote:

Hi Oliver,

> First off, the original code wasn't written by me and I don't claim I 
> fully understand it. The background, as far as I understand it, is 
> this:

Oh yes, sorry I saw that after I wrote the mail, there were a number
of maintainers over the years :)

> The backend may fork a reader process or thread who's only purpose is 
> to fetch image data from the scanner. It seems that this was invented 
> to speed up data transfer from the scanner without depending on the 
> frontend to read the data fast enough. As a result there could be two 

I /think/ this was done in an attempt to avoid backtracking on SCSI
scanners, at the time.

> When USB support was added measures were taken to protect access to 
> the scanner on command level, i.e. only one process/thread can send a 
> command to the scanner and receive the result. This should allow for 
> mixed access from both processes/threads when implemented correctly. 
> It seems, however, that for SCSI no such measures were taken.

Which raises a few ??? and eyebrows at the same time :)

> To make matters more complicated an USB command queueing was 
> introduced in the USB part of the backend. The purpose, according to 
> the comment, was this:
[...]
> As far as I understand it this code makes the locking necessary in the 
> first place, since both processes/threads may theoretically add 
> commands to the queue while the scanner is busy.

If I understand that correctly, the locking is there to avoid a
possible race between two commands from 2 processes that could lead to
the wrong command to be added to the queue. Correct ?

> Currently there are three implementations for the mutex: One for BeOS, 
> one for pthread (used on OS/2 and OS-X), one for fork(). The one that 
> is causing problems is the one for fork() which is used on all Linux 
> platforms IIRC.

Correct. Note that for Linux we /could/ switch to pthread, however I
am a bit reluctent to do that due to possible side effects in the
frontends.

>>  - is that locking really useful in the backend?
>
> Honest answer: I don't know for sure. I'd sleep better if I knew that 
> there was _some_ locking done when there's the remote chance of two 
> processes writing to the same device.

Same here.

>>  - if yes, how do we fix this?
>
> Good question. Maybe the easiest solution is to remove all forking / 
> threading / locking since it probably won't make a difference on any 
> machine faster than 100 MHz. At least I never noticed any difference 
> when testing the backend in single-thread mode. But I'm open to other 
> suggestions (or even implementations, given my lack of time to 
> support the backend...)

The reader task thing is a bit suboptimal anyway; I've got a bug
report about that fact. In a nutshell: due to both tasks having the
same priority wrt scheduling, both have the same timeslice allocated,
and depending on how the main task is designed, it can end up using
its timeslice doing nothing instead of giving it up and having the
reader task benefit from that. You can actually double the throughput
by renicing the tasks.

I was under the impression that in some cases you *need* the reader
task. IIRC one of the reason is that with some scanners (parallel ?)
you can't do non blocking IO and you end up with a frozen frontend
while transferring data, which is not nice for a GUI (and now that's
soooo last century and non-cool ;)).


Now, I really think it'd make more sense to have SANE entirely
decoupled from the frontend, so that the frontend would be relieved
from this kind of issues.

Also some backends use/block signals (among other things), something
which I've never been comfortable with.

Something like saned on steroids, which would be a real scan server,
could handle that and maybe backends could become single threaded
again.

Any thoughts on any of that ? :)

JB.

-- 
Julien BLACHE                                   <http://www.jblache.org> 
<jb at jblache.org>                                  GPG KeyID 0xF5D65169