On Wed, Jun 26, 2024 at 11:42 AM Kevin Wolf <kw...@redhat.com> wrote:
> Am 24.06.2024 um 23:12 hat Nir Soffer geschrieben: > > On Mon, Jun 24, 2024 at 7:08 PM Kevin Wolf <kw...@redhat.com> wrote: > > > > > Am 24.06.2024 um 17:23 hat Stefan Hajnoczi geschrieben: > > > > On Wed, Jun 19, 2024 at 08:43:25PM +0300, Nir Soffer wrote: > > > > > Tested using: > > > > > > > > Hi Nir, > > > > This looks like a good candidate for the qemu-iotests test suite. > Adding > > > > it to the automated tests will protect against future regressions. > > > > > > > > Please add the script and the expected output to > > > > tests/qemu-iotests/test/write-zeroes-unmap and run it using > > > > `(cd build && tests/qemu-iotests/check write-zeroes-unmap)`. > > > > > > > > See the existing test cases in tests/qemu-iotests/ and > > > > tests/qemu-iotests/tests/ for examples. Some are shell scripts and > > > > others are Python. I think shell makes sense for this test case. You > > > > can copy the test framework boilerplate from an existing test case. > > > > > > 'du' can't be used like this in qemu-iotests because it makes > > > assumptions that depend on the filesystem. A test case replicating what > > > Nir did manually would likely fail on XFS with its preallocation. > > > > This is why I did not try to add a new qemu-iotest yet. > > > > > Maybe we could operate on a file exposed by the FUSE export that is > > > backed by qcow2, and then you can use 'qemu-img map' on that qcow2 > image > > > to verify the allocation status. Somewhat complicated, but I think it > > > could work. > > > > Do we have examples of using the FUSE export? It sounds complicated but > > being able to test on any file system is awesome. The complexity can be > > hidden behind simple test helpers. > > We seem to have a few tests that use it, and then the fuse protocol > implementation, too. 308 and file-io-error look relevant. > > > Another option is to use a specific file system created for the tests, > > for example on a loop device. We used userstorage[1] in ovirt to test > > on specific file systems with known sector size. > > Creating loop devices requires root privileges. If I understand > correctly, userstorage solved that by having a setup phase as root > before running the tests as a normal user? We don't really have that in > qemu-iotests. > > Some tests require passwordless sudo and are skipped otherwise, but this > means that in practice they are almost always skipped. > Yes, this is the assumption the storage is being created before running the tests, for example when setting up a development or CI environment, and the tests can run with unprivileged user. > But more important, are you ok with the change? > > > > I'm not sure about not creating sparse images by default - this is not > > consistent with qemu-img convert and qemu-nbd, which do sparsify by > > default. The old behavior seems better. > > Well, your patches make it do what we always claimed it would do, so > that consistency is certainly a good thing. Unmapping on write_zeroes > and ignoring truncate is a weird combination anyway that doesn't really > make any sense to me, so I don't think it's worth preserving. The other > way around could have been more defensible, but that's not how our bug > works. > > Now, if ignoring all discard requests is a good default these days is a > separate question and I'm not sure really. Maybe discard=unmap should > be the default (and apply to both discard are write_zeroes, of course). > OK, lets limit the scope to fix the code to match the current docs. We can tweak the defaults later.
