Re: vnode lock and v_numoutput

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

   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);
}

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.)

2. Add nestiobuf_lastdone to do the same, for the last submission in a
nestio transaction, so we don't need to test all call sites now but
have an easy path to fix the broken ones when we can test them.


P.S.  I reviewed all other updates of v_numoutput outside nestiobuf.
With the exception of lfs_segment.c, every vp->v_numoutput++ is
immediately followed either by

(a) biodone(bp) or uvm_aio_aiodone(bp), or by
(b) VOP_STRATEGY(vp, bp) or bdev_strategy(bp),

where vp = bp->b_vp.  For (b), perhaps we could do vp->v_numoutput++
in bdev_strategy or in a wrapper vn_strategy(vp, bp).