Re: [Qemu-devel] [PATCH v2 9/9] tests: Add coverage for recent block geometry fixes

[Kevin Wolf]

Am 19.11.2016 um 23:17 hat Max Reitz geschrieben:
> On 19.11.2016 22:45, Max Reitz wrote:
> > On 18.11.2016 02:28, Eric Blake wrote:
> >> On 11/17/2016 05:42 PM, Max Reitz wrote:
> >>> On 17.11.2016 21:14, Eric Blake wrote:
> >>>> Use blkdebug's new geometry constraints to emulate setups that
> >>>> have caused recent regression fixes: write zeroes asserting
> >>>> when running through a loopback block device with max-transfer
> >>>> smaller than cluster size, and discard rounding away requests
> >>>> that were not aligned to power-of-two boundaries.  Also, add
> >>>> coverage that the block layer is honoring max transfer limits.
> >>>>
> >>>> For now, a single iotest performs all actions, with the idea
> >>>> that we can add future blkdebug constraint test cases in the
> >>>> same file; but it can be split into multiple iotests if we find
> >>>> reason to run one portion of the test in more setups than what
> >>>> are possible in the other.
> >>>>
> >>
> >>>
> >>>> +# Dell Equallogic iSCSI device is unusual with its 15M page size
> >>>> +echo
> >>>> +echo "== non-power-of-2 write zeroes =="
> >>>> +
> >>>> +limits=opt-write-zero=15M,max-write-zero=15M,opt-discard=15M,max-discard=15M
> >>>> +$QEMU_IO -c "open -o $options,$limits blkdebug::$TEST_IMG" \
> >>>> +         -c "write -z 32M 32M" | _filter_qemu_io
> >>>> +
> >>>> +echo
> >>>> +echo "== non-power-of-2 discard =="
> >>>> +
> >>>> +limits=opt-write-zero=15M,max-write-zero=15M,opt-discard=15M,max-discard=15M
> >>>> +$QEMU_IO -c "open -o $options,$limits blkdebug::$TEST_IMG" \
> >>>> +         -c "discard 80000000 30M" | _filter_qemu_io
> >>>
> >>> Question: What does this test has to do with iscsi?
> >>>
> >>> The first case just tests that we fall back to writing the head and tail
> >>> as full zeroes when the driver returns -ENOTSUP.
> >>
> >> The first one isn't all that interesting, so much as making sure we
> >> don't regress. I couldn't make it fail, pre-patch.  The real test is the
> >> second one...
> >>
> >>>
> >>> The second test, as far as I can see, just gives some discard request to
> >>> blkdebug (split up into head, mid and tail), which blkdebug just passes
> >>> on (because 80000000 is a multiple of 512). qcow2 then discards part of
> >>> that and drops the head and tail of the request it receives (but head
> >>> and tail are now calculated based on qcow2's 64k limit).
> >>
> >> Thanks to the opt-discard=15M, the blkdebug layer is forcing the block
> >> layer to break it into head/middle/tail on 15M boundaries, but throwing
> >> away the head and tail without giving blkdebug a chance, so it only
> >> zeroed 90-105M.  Then, with the block layer fixed to pass the head on
> >> through anyways, but without patch 2/9, the qcow2 code was seeing that
> >> the start offset was not cluster-aligned ($TEST_IMG has 1M clusters),
> >> and with patch 4/9 that was making qcow2 return -ENOTSUP, and still
> >> ignoring everything up to 90M.  It took all of 2, 4, and 5 before the
> >> discard finally affected the range 77M-90M (since 80000000 is just
> >> before 77M).
> > 
> > OK, thank you for the explanation, but again, I don't know how that is
> > related to the iscsi case.
> 
> And now with my last response to patch 5, I do know. So because the
> "align" option isn't set, the request_alignment defaults to 512.
> blkdebug will accept requests and pass them on even if they're not
> aligned to pdiscard_alignment, so that's similar to how iscsi only drops
> discard requests that are not aligned to request_alignment but passes
> everything on regardless of whether it's aligned to pdiscard_alignment.

If the 512 byte alignment is important, please make it explicit. I have
a patch to make blkdebug work with byte alignment and then the default
will be 1.

Kevin

pgpr1dNQ9Xl2X.pgp
Description: PGP signature