server: Cache per-connection can_write
commitd60d0f4248610fc1d116dc9f249526d20913c9a3
authorEric Blake <eblake@redhat.com>
Wed, 28 Aug 2019 15:33:42 +0000 (28 10:33 -0500)
committerEric Blake <eblake@redhat.com>
Sat, 31 Aug 2019 22:22:12 +0000 (31 17:22 -0500)
tree652b5bc67b20965f88cd9bbe2eb41d4f2450e3fa
parent883201f4a7b54ca4537961c1b7a5e883ab57c939
server: Cache per-connection can_write

The calculation of eflags skips calling .can_zero, .can_trim, and
.can_fua if .can_write returns false, because 1) time spent calling
.can_zero would be wasted, and 2) plugin writers can be lazy and let
.can_zero return constant true, regardless of what they return for
.can_write, and are relying on nbdkit to not advertise or call .zero.
We don't want to regress on this (that is, it is easy to write a sh
plugin script that would be noticeably slower if we call into
.can_zero in spite of .can_write returning false).  But at the same
time, just as plugins can blindly return .can_zero==true in spite of
.can_write==false, it is also possible for a plugin to blindly return
.can_write==true in spite of the readonly=true argument to .open (that
is, when the command line option -r is in effect), yet we were NOT
skipping that call into .can_write (meaning we can write a sh plugin
to observe a wasted call to .can_write [1]).

Next, consider that although we don't have such a filter today, it is
feasible that someone could write a filter that exposes .can_write==0
to the client but which wants to open the underlying plugin read-write
and uses next_ops->pwrite and next_ops->zero (for example, a scenario
of exposing read-only contents of a single file extracted from an
underlying file system, where the underlying device containing the
file system must be opened read-write even though its file system will
only be mounted read-only, because the act of mounting triggers a
journal replay that modifies the underlying device, but not the file
being extracted).  This implies that -r must only affect the outermost
backend, and not every backend along the chain.

Then, to date, we've documented that such a filter merely need check
next_ops->can_zero before calling next_ops->zero, but do not actually
enforce that; furthermore, we are back to the case of what to do when
a plugin hard-codes can_zero to return true even when can_write is
false.  All filters are in-tree, so we could merely change our
documentation to require the filters to also check
next_ops->can_write, but it is nicer to just do the extra checking as
part of backend.c.

At any rate, adding more calls to can_write implies that it is
important enough to be cached; and that's easy to do using the same
ideas as already used in commit b9d4875b for .get_size.  (Other things
like can_zero and can_fua are also worth caching, but this patch is
already going to be big enough to save that one for later.)

The next consideration is when we should check can_write and can_zero.
If the first time we actually call into the plugin is during
next_ops->zero itself, we have to deal with any errors reported by
next_ops->can_zero - except that the backend interface has no *err
parameter for can_zero, and we are not ensured that there will be a
sane errno to report for the failed .zero.  It would be a lot nicer to
just assert during .zero that the cache for .can_write is already
populated from the handshake phase (eflags computation for the
outermost backend, and during a filter's .prepare for the next
backend), to prove that the caller is not using .zero except when the
backend is writable.  Thankfully, all filters which have both an
override of .can_zero and which call next_ops->zero do check
next_ops->can_zero first, so as long as we fix can_zero to trigger the
call to can_write under the hood, we don't have to modify those
filters for the sake of can_write (although when a later patch starts
caching can_zero, it will be beneficial to hoist any
next_ops->can_zero call into .prepare, to avoid having to deal with
failure later).

Finally, with all these changes in place, we can drop the gating logic
during eflags computation as redundant with the gating logic added in
backend.c; eflags computations can just blindly call all of the
underlying functions.  Likewise, plugins.c no longer has to fall back
to .can_write during its computation of .can_zero (because we have
ensured that .can_zero won't be reached).

[1] Demo time:
$ cat script
case "$1" in
    get_size) echo 1m;;
    can_write) echo can_write >/dev/tty; sleep 1; ${DIE} ;;
    can_zero) echo can_zero >/dev/tty; sleep 2;;
    *) exit 2 ;;
esac

Pre-patch:

$ /bin/time -f %e ./nbdkit -U - -r sh script \
  --run 'qemu-nbd --list -k $unixsocket >/dev/null'
can_write
1.10

Wasted call to .can_write, but at least .can_zero was skipped

$ /bin/time -f %e ./nbdkit -U - sh script \
  --run 'qemu-nbd --list -k $unixsocket >/dev/null'
can_write
can_zero
can_write
4.10

Without -r, eflags computation calls .can_write, then .can_zero; but
then plugin_can_zero calls .can_write again.

$ DIE='exit 3' /bin/time -f %e ./nbdkit -U - sh script \
  --run 'qemu-nbd --list -k $unixsocket >/dev/null'
can_write
1.06

But when .can_write fails, we see how .can_zero was skipped.

$ /bin/time -f %e ./nbdkit -U - -r --filter=nozero sh script zeromode=notrim \
  --run 'qemu-nbd --list -k $unixsocket >/dev/null'
can_zero
can_write
can_write
4.11

The nozero filter probes next_ops->can_zero unconditionally during
.prepare (here, if we had the theoretical filter on top of nozero that
advertises no write support but uses writes under the hood, then this
is actually good - although perhaps it is worth a followup patch to
filters to be smarter during .prepare if .open was passed
readonly=true), then plugin_can_zero calls .can_write.  The second
.can_write comes from eflags computation (since the nozero filter did
not intercept that one).

$ DIE='exit 3' /bin/time -f %e ./nbdkit -U - --filter=nozero sh script \
  zeromode=notrim --run 'qemu-nbd --list -k $unixsocket >/dev/null'
can_zero
can_write
nbdkit: sh[1]: error: zeromode 'notrim' requires plugin zero support
qemu-nbd: Failed to read initial magic: Unexpected end-of-file before all bytes were read
Command exited with non-zero status 1
3.07

A bit faster (we didn't get to eflags computation because of the
nozero failure), but still long.

With this applied:

$ /bin/time -f %e ./nbdkit -U - -r sh script \
  --run 'qemu-nbd --list -k $unixsocket >/dev/null'
0.07

Yay - no time wasted!

$ /bin/time -f %e ./nbdkit -U - -r --filter=nozero sh script zeromode=notrim \
  --run 'qemu-nbd --list -k $unixsocket >/dev/null'
can_write
can_zero
3.10

Note the act of calling .can_zero from nozero's .prepare forces
.can_write to happen first, and because it was cached, we don't see it
again even during eflags computation.

$ DIE='exit 3' /bin/time -f %e ./nbdkit -U - --filter=nozero sh script \
  zeromode=notrim --run 'qemu-nbd --list -k $unixsocket >/dev/null'
can_write
nbdkit: sh[1]: error: zeromode 'notrim' requires plugin zero support
qemu-nbd: Failed to read initial magic: Unexpected end-of-file before all bytes were read
Command exited with non-zero status 1
1.06

We still get the nozero failure, but this time much faster (without
wasting time on the .can_zero call, since the .can_write failure
already constrains us).

Signed-off-by: Eric Blake <eblake@redhat.com>
server/backend.c
server/connections.c
server/internal.h
server/plugins.c
server/protocol-handshake.c