On Wed, Sep 06, 2006 at 11:23:02PM +0200, Manuel Bouyer wrote:

Hi Manuel,

> > I believe that this is not allowed. The strategy routine is
> > part of the top half of the driver, it operates in process
> > context.
> 
> There was a discussion on tech-kern when I introduced the Xen
> backend driver, and the conclusion was that it's valid to call
> a strategy routine from interrupt context, and drivers should deal
> with that.

I now read that discussion (and one from 1996 when ccd had problems
and one more when cgd had problems).

So, in 4.4BSD the strategy functions operated in process context,
and a few of them even used this to handle ressource shortages
on the lower parts of the driver. The original vn driver does
call VOP_STRATEGY itself, so did our vnd driver until you added
the thread.

Since strategy routines are not supposed to sleep for performance
reasons and most therefore didn't, it was concluded that strategy
routines have to be interrupt safe.

I believe this change in the interface model was a bad decision,
and it was done to make life for ccd, cgd and later xbd simpler.


> > Saying that, I scanned other drivers and found several that may
> > sleep in their strategy() routine. Most promiment scsistrategy()
> > does this (by virtue of calling scsipi_command which waits for
> > a free scsi transfer context in scsipi_get_xs).
> 
> This is a pseudo-strategy, part of the SCSI-specific ioctls implementation.
> It's internal details, it's not exported to other drivers.

I noticed. There are some left that do spin-locking (possible to do 
in interrupts) and an ancient broken VAX driver that still shows
that the BSD design let strategy routines operate in process context
and sleep.


> > >  This patch broke vnd for all Xen users, where it worked fine before.
> > 
> > I agree. The patch probably leaves a lot space for improvement. But
> > the real bug is calling the strategy() routine from interrupt context.
> 
> You're redefining the strategy() interface here.

No, some people did that 2 years ago (I believe Jason was the first caller,
but others agreed). This change was done to accomodate cgd.


> > >  I understand all of this. But 1) the patch is broken (calling tsleep in
> > >  a strategy routine) 2) whenever it's not a problem for other drivers is
> > >  a matter of opinion. If has probably performances inpacts.
> > 
> > Calling tsleep() in strategy is allowed.
> 
> Where did you get this ?

That is 4.4BSD.


> > >  > performances on systems with multiple drives.
> > >  
> > >  No, I meant if all buffers end up in a single device queue.
> > 
> > That's what happening without the patch.
> 
> And that's what happens in other driver's queue too.

True. But with other drivers this does not end in a deadlock as
they don't do the circle to the VOP layer. It still is a bad
thing, maybe that SoC project to add some 'congestion control'
to the filesystems develops a solution to that. The vnd driver
itself however should protect itself against this deadlock
that it generates itself. N.B. there is a second deadlock
for io buffers, that only gets exhibited when memory runs
out, but when the buffer deadlock is solved, this will show
up.

I am not sure about ccd, it calls VOP_STRATEGY() which then
calls DEV_STRATEGY() but has some code path that may sleep.


Greetings,
-- 
                                Michael van Elst
Internet: mlelstv@serpens.de
                                "A potential Snark may lurk in every tree."