Re: vnode lock and v_numoutput

[Manuel Bouyer <bouyer%antioche.eu.org@localhost>]

On Mon, Jan 26, 2015 at 09:10:54AM +0000, Taylor R Campbell wrote:
>    Date: Sat, 24 Jan 2015 22:48:33 +0100
>    From: Manuel Bouyer <bouyer%antioche.eu.org@localhost>
> 
>    On Sat, Jan 24, 2015 at 05:32:41PM +0000, Taylor R Campbell wrote:
>    > But is that wrong for the parent buf of a nestiobuf transaction?
>    > These don't actually correspond to data themselves, and it seems that
>    > the purpose of the v_numoutput accounting is to notify fsync when all
>    > data submitted to disk have been written.  So based on this analysis I
>    > suspect it would be safe to move the v_numoutput++ to just before the
>    > call to nestiobuf_done in handle_with_strategy.
> 
>    No, it's not: the call to nestiobuf_done() may be a noop (it probably is
>    in most case) when skipped is 0. The real biodone() may happen before,
>    we need to v_numoutput++ before that.
> 
> You're right, I missed that, thanks.  But the real biodone does happen
> here iff skipped is nonzero.  So maybe rather than call nestiobuf_done
> at the end, we could do:
> 
> if (skipped) {
> 	bump v_numoutput;
> 	nestiobuf_done(mbp, skipped, error);
> }

I'm not sure I understand; the "bump v_numoutput" is always needed;
but if skipped is 0 the last nestiobuf_done() call is done by
biodone() callback and not by the called. So the bump has to happen before the
last buf is queued.

But you're right, my patch is wrong for the skipped != case, because
in this case the bump doesn't happen.
Attached is a new patch.

> 
> Then rather than put that into vnd and make copypasta of it in all
> other callers, we can easily make it a part of the nestiobuf API.  Two
> easy possibilities:
> 
> 1. Change (the public) nestiobuf_done to bump v_numoutput before
> calling biodone, and change the signature so we have to review all
> call sites.  (The private nestiobuf_iodone would obviously not bump
> v_numoutput.)

The private version still have to bump it too, see above.

I agree that hidding this to the nestiobuf API is probably the right thing
to do. But I'm also looking for a fix that is safe to pull up to netbsd-5
and netbsd-6 (the problem was first noticed on netbsd-5, and I reproduced
it, and tested the patch, on netbsd-6). So if the attached
patch looks semantically correct, I'll commit it and request pullups.
Then I'll look at a better fix that can be commited to HEAD pulled up
to netbsd-7.

-- 
Manuel Bouyer <bouyer%antioche.eu.org@localhost>
     NetBSD: 26 ans d'experience feront toujours la difference
--