Re: [Qemu-block] [PATCH v3 2/5] blkdebug: Add pass-through write_zero and discard support

[Eric Blake]

On 12/07/2016 07:55 AM, Kevin Wolf wrote:
> Am 02.12.2016 um 20:22 hat Eric Blake geschrieben:
>> In order to test the effects of artificial geometry constraints
>> on operations like write zero or discard, we first need blkdebug
>> to manage these actions.  It also allows us to inject errors on
>> those operations, just like we can for read/write/flush.
>>

>>
>>
>> +static int coroutine_fn blkdebug_co_pwrite_zeroes(BlockDriverState *bs,
>> +                                                  int64_t offset, int count,
>> +                                                  BdrvRequestFlags flags)
>> +{
>> +    BDRVBlkdebugState *s = bs->opaque;
>> +    BlkdebugRule *rule = NULL;
>> +    uint32_t align = MAX(bs->bl.request_alignment,
>> +                         bs->bl.pwrite_zeroes_alignment);
>> +
>> +    /* Only pass through requests that are larger than requested
>> +     * preferred alignment (so that we test the fallback to writes on
>> +     * unaligned portions), and check that the block layer never hands
>> +     * us anything crossing an alignment boundary.  */
>> +    if (count < align) {
>> +        return -ENOTSUP;
>> +    }

This early exit bypasses the checks for error injection - but the block
layer will then fall back to write which will perform the check there.

>> +    assert(QEMU_IS_ALIGNED(offset, align));
>> +    assert(QEMU_IS_ALIGNED(count, align));
>> +    if (bs->bl.max_pwrite_zeroes) {
>> +        assert(count <= bs->bl.max_pwrite_zeroes);
>> +    }
>> +
>> +    QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {
>> +        if (rule->options.inject.offset == -1) {
> 
> We do have offset and bytes parameters in this function, so I guess we
> should check overlaps like in the read/write functions instead of only
> executing the rule if it doesn't specify an offset.

Oh, right. I copied and pasted from flush, when I should have copied
from read.

>> +    return bdrv_co_pwrite_zeroes(bs->file, offset, count, flags);
>> +}
>> +
>> +
> 
> Why two newlines?
> 

Habit; they aren't essential, so I'll trim to 1 for consistency with the
rest of the file.

>> +static int coroutine_fn blkdebug_co_pdiscard(BlockDriverState *bs,
>> +                                             int64_t offset, int count)
>> +{
>> +    BDRVBlkdebugState *s = bs->opaque;
>> +    BlkdebugRule *rule = NULL;
>> +    uint32_t align = bs->bl.pdiscard_alignment;
>> +
>> +    /* Only pass through requests that are larger than requested
>> +     * minimum alignment, and ensure that unaligned requests do not
>> +     * cross optimum discard boundaries. */
>> +    if (count < bs->bl.request_alignment) {
>> +        return -ENOTSUP;
>> +    }

Here, the early exit is a no-op; we never see the error injection
because there is no fallback path at the block layer.  But I guess
that's still okay - when we are discarding the unaligned portion of a
request, there really is no I/O and therefore no way to inject an error
representing failed I/O.

Looks like I get to spin a v4 for the error injection to do specific
range checking.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature