On 2/25/21 8:55 AM, Vladimir Sementsov-Ogievskiy wrote:
18.02.2021 23:15, Eric Blake wrote:
Previous patches mentioned how the blkdebug filter driver demonstrates
a bug present in our NBD server (for example, commit b0245d64); the
bug is also present with the raw format driver when probing
occurs. Basically, if we specify a particular alignment > 1, but defer
the actual block status to the underlying file, and the underlying
file has a smaller alignment, then the use of BDRV_BLOCK_RAW to defer
to the underlying file can end up with status split at an alignment
unacceptable to our layer. Many callers don't care, but NBD does - it
is a violation of the NBD protocol to send status or read results
split on an unaligned boundary (in 737d3f5244, we taught our 4.0
client to be tolerant of such violations because the problem was even
more pronounced with qemu 3.1 as server; but we still need to fix our
server for the sake of other stricter clients).
+ * - BDRV_BLOCK_ALLOCATED is set if the flag is set for at least one
subregion
Hmm about this..
We already have mess around ALLOCATED:
[1] for format drivers it means that "read is handled by this layer,
not by backing", i.e. data (or zero) is placed exactly on that layer of
backing-chain
If we're reading at a given granularity, then the 4k read is satisfied
at this layer even if portions of the read came from lower layers. So
the logic works here.
[2] for protocol drivers it's up to the driver, which may always report
ALLOCATED (being more compatible with format drivers) or it may
sometimes return UNALLOCATED to show that data is not allocated in FS..
We've been moving away from this particular overload. What's more, most
protocol drivers that set it at all set it for every single byte,
because protocol layers don't have a notion of a backing file; which
means that if it is set at all, it will be set for every byte anyway, so
the logic works here.
And this way, bdrv_co_block_status_aligned() is compatible with protocol
drivers but not with format drivers (as you can't combine
"go-to-backing" information of several flags, as for some scenarios it's
safer to consider the whole region ALLOCATED and for another it's safer
to consider it UNALLOCATED.
For example for stream target it's safer to consider target block
UNALLOCATED and do extra copy-on-read operation. And for stream base
it's safer to consider block ALLOCATED (and again do extra copying, not
missing something significant).
I think, to avoid increasing of the mess, we should first split [1] from
[2] somehow..
Assume we change it to BDRV_BLOCK_PROTOCOL_ALLOCATED and
BDRV_BLOCK_GO_TO_BACKING.
Maybe it is indeed worth splitting out two different flags to fully
distinguish between the two overloaded meanings, but that seems like an
independent patch to this series.
Then, for BDRV_BLOCK_PROTOCOL_ALLOCATED we probably can just report
BDRV_BLOCK_PROTOCOL_ALLOCATED if at least one of extents reports
BDRV_BLOCK_PROTOCOL_ALLOCATED. (probably we don't need
BDRV_BLOCK_PROTOCOL_ALLOCATED at all and can drop this logic)
But for BDRV_BLOCK_GO_TO_BACKING we'll have to also add
BDRV_BLOCK_GO_TO_BACKING_VALID and report
* BDRV_BLOCK_GO_TO_BACKING | BDRV_BLOCK_GO_TO_BACKING_VALID if all
extents report BDRV_BLOCK_GO_TO_BACKING
* BDRV_BLOCK_GO_TO_BACKING if all extents report no
BDRV_BLOCK_GO_TO_BACKING
* <nothing> if some extents report BDRV_BLOCK_GO_TO_BACKING but others
not.
Hmm.. And, I think that there is a problem is in NBD protocol. Actually,
with allocation-depth context we started to report internal layers of
backing chain. And if we have layers with different request-alignment
it's not correct to report allocation-depth "aligned" to top image
request-alignment.. So, for allocation-depth to work correctly we should
extend NBD protocol to allow unaligned chunks in BLOCK_STATUS report.
The NBD protocol says that base:allocation must obey allocation rules.
If we want to declare that "because qemu:allocation-depth is an
extension, we choose to NOT obey allocation rules, and if your client
connects to our extension, it MUST be prepared for what would normally
be non-compliant responses to NBD_CMD_BLOCK_STATUS", then we are free to
do so (it is our extension, after all). Particularly since the only way
I could actually trigger it was with blkdebug (most format layers
support byte-level access, even when layered on top of a protocol layer
with a 512 or 4k minimum byte access).
So if you think it is better for me to respin the patch to fix ONLY
base:allocation bugs, but not qemu:allocation-depth, and instead merely
document the issue there, I could be persuaded to do so.
So, finally:
1. making bitmap export extents aligned is a separate simple thing -
that's OK
2. making base:allocation aligned is possible due to good NBD_STATE_HOLE
definition. So for it it's correct to combine BDRV_BLOCK_ALLOCATED as
you do even keeping in mind format layers. We call block_status_above
for the whole chain. ALLOCATED=0 only if all format layers refer to
backing and bottom protocol driver(if exists) reports "unallocated in
FS" so that correspond to
"If an extent is marked with NBD_STATE_HOLE at that context, this means
that the given extent is not allocated in the backend storage, and that
writing to the extent MAY result in the NBD_ENOSPC error"
And this way, I'd prefer to fix exactly base:allocation context
handling in nbd-server not touching generic block_status which already
has enough mess.
3. fixing qemu:allocation-depth I think is impossible without allowing
unaligned chunks in NBD spec (really, why we should restrict all
possible metadata contexts so hard?) Or, if we are still going to align
allocation-depth results to top layer request-alignment we should change
allocation-depth specification so that that's not "actual allocation
depth" but something >= than actual allocation depth of all subchunks...
And that becomes useless.