Subject: Re: PR/34293 CVS commit: src/sys/dev
To: Michael van Elst <mlelstv@serpens.de>
From: Manuel Bouyer <bouyer@antioche.eu.org>
List: netbsd-bugs
Date: 09/04/2006 22:51:02
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
--