Subject: Re: PR/34293 CVS commit: src/sys/dev
To: None <kern-bug-people@netbsd.org, gnats-admin@netbsd.org,>
From: Manuel Bouyer <bouyer@antioche.eu.org>
List: netbsd-bugs
Date: 09/04/2006 20:55:06
The following reply was made to PR kern/34293; it has been noted by GNATS.

From: Manuel Bouyer <bouyer@antioche.eu.org>
To: Michael van Elst <mlelstv@serpens.de>
Cc: gnats-bugs@NetBSD.org, kern-bug-people@NetBSD.org,
	gnats-admin@NetBSD.org, netbsd-bugs@NetBSD.org
Subject: Re: PR/34293 CVS commit: src/sys/dev
Date: Mon, 4 Sep 2006 22:51:02 +0200

 On Sun, Sep 03, 2006 at 10:32:54PM +0200, Michael van Elst wrote:
 > On Sun, Sep 03, 2006 at 08:00:10PM +0000, Manuel Bouyer wrote:
 > 
 > >  it's not correct to tsleep() in a strategy routine, which may be called from
 > >  interrupt context.
 > 
 > vndstrategy isn't called from interrupt context.
 
 It is, if called from another driver (e.g. Xen block device).
 
 > But if that is
 > a problem, the feedback loop can be put into both vndread()/vndwrite().
 
 I'm not sure we're allowed to tsleep() here either.
 
 > 
 > 
 > >  Unfortunably this reopens PR/10731, PR/12189, PR/20296, PR/34293
 > >  As for what the correct fix it, this needs to be analysed deeper.
 > 
 > Then I suggest you do that before you break vnd.
 
 This patch broke vnd for all Xen users, where it worked fine before.
 
 > 
 > 
 > >  I suspect
 > >  throttling the caller in vnd only hides the problem;
 > 
 > No, it prevents the problem. The writer needs to be throttled to
 > not overwhelm the consumer because in this case it just eats up
 > all memory. That necessary feedback loop is created by the patch.
 > 
 > 
 > >  the same caller writing
 > >  to some other device could exaust all buffers as well.
 > 
 > This is a general problem with every device but so far not seen as
 > a problem. If there is memory to buffer the requests, the current
 > buffering policy is to allow all buffers to be used. This is more or
 > less harmless as normal device drivers will only consume and free
 > buffers on the queue. 
 > 
 > However, vnd is special in that it requires further buffers to be
 > allocated to consume and free buffers on the queue. There is also
 > no need to double buffer writes to vnd this way, once in the
 > device queue and once in the underlying filesystem. That strategy
 > is broken and this is also fixed by the patch that limits the
 > double-buffer to whatever the device thread is prepared to consume.
 
 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.
 > 
 > 
 > >  If this driver doesn't
 > >  need to allocate buffer this won't cause a deadlock, but it's bad for
 > >  performances on systems with e.g. multiple drives.
 > 
 > Preventing vnd from consuming all buffers cannot be bad for
 > performances on systems with multiple drives.
 
 No, I meant if all buffers end up in a single device queue.
 
 -- 
 Manuel Bouyer <bouyer@antioche.eu.org>
      NetBSD: 26 ans d'experience feront toujours la difference
 --