Implement support for both F_DISCARD and F_WRITE_ZEROES

[slp]

Imago supports both operations, so let's just enable them, as they may have a significant impact on both performance and disk usage on the host.

[slp]

Fixes #360

[slp]

This one needs an update of the imago crate.

[tylerfanelli]

You mention:

Since, in practice, we won't have multiple threads accessing the
lock, the overhead should be very small (one or two operations on
atomics).

Curious as to why we can be sure that multiple threads won't access the lock? Is there a dedicated thread for this?

[mtjhrc]

Curious as to why we can be sure that multiple threads won't access the lock? Is there a dedicated thread for this?

I think that is accurate, there is a single dedicated worker thread for the device and the main event loop thread in libkrun that will end up activating the device.

But still, I am a not really happy about adding the Mutex, because I think it should be avoidable if you are clear about the actual ownership of the disk object at any given time. Having had a quick look, to do this the object should be moved into the worker thread in BlockWorker::new and returned from the BlockWorker::work(), which would then return it from .join() on the thread, so you get the object back in the Block::reset(), you can then store it and move it into the worker thread on the next activate().

I'm not really worried about the performance here, but the code quality if we were to keep adding mutexes to places where they shouldn't be necessary.

[slp]

Curious as to why we can be sure that multiple threads won't access the lock? Is there a dedicated thread for this?

I think that is accurate, there is a single dedicated worker thread for the device and the main event loop thread in libkrun that will end up activating the device.

Correct.

I'm not really worried about the performance here, but the code quality if we were to keep adding mutexes to places where they shouldn't be necessary.

This is pretty much a one-time thing only. And it'll go away once imago 0.2.0 gets released. @XanClic please keep me honest here.

[XanClic]

I'm not really worried about the performance here, but the code quality if we were to keep adding mutexes to places where they shouldn't be necessary.

This is pretty much a one-time thing only. And it'll go away once imago 0.2.0 gets released. @XanClic please keep me honest here.

It wasn’t part of the plan originally, but it’s certainly doable for 0.2.

Note though that discarding (or write_zeroes) on an immutable reference will be unsafe. It’s fine for a single thread, but for multiple threads, you have to be careful e.g. about reading from an area while simultaneously discarding it, because that may return data that shouldn’t be there:

• Thread 1 begins reading from the area, gets the corresponding data offset in the underlying file to read from
• Thread 2 discards the data, relinquishing the allocation on that offset
• Thread 2 (or some different thread entirely) writes to some completely different area of the qcow2 disk that’s currently unallocated, requiring an allocation, which reuses the offset that was just discard, writing the data there
• Thread 1 reads that data that’s supposed to be in a completely different area of the disk

That would be a security risk.

I’ll try to mitigate this problem in imago, but probably not for 0.2.0. Then again, it’s all good as long as I can do it before you use multiple threads.

[slp]

I'm not really worried about the performance here, but the code quality if we were to keep adding mutexes to places where they shouldn't be necessary.

This is pretty much a one-time thing only. And it'll go away once imago 0.2.0 gets released. @XanClic please keep me honest here.

It wasn’t part of the plan originally, but it’s certainly doable for 0.2.

Note though that discarding (or write_zeroes) on an immutable reference will be unsafe. It’s fine for a single thread, but for multiple threads, you have to be careful e.g. about reading from an area while simultaneously discarding it, because that may return data that shouldn’t be there:

• Thread 1 begins reading from the area, gets the corresponding data offset in the underlying file to read from
• Thread 2 discards the data, relinquishing the allocation on that offset
• Thread 2 (or some different thread entirely) writes to some completely different area of the qcow2 disk that’s currently unallocated, requiring an allocation, which reuses the offset that was just discard, writing the data there
• Thread 1 reads that data that’s supposed to be in a completely different area of the disk

That would be a security risk.

I’ll try to mitigate this problem in imago, but probably not for 0.2.0. Then again, it’s all good as long as I can do it before you use multiple threads.

Got it. Thanks a lot, Hanna!

[mtjhrc]

This is pretty much a one-time thing only. And it'll go away once imago 0.2.0 gets released. @XanClic please keep me honest here.

It wasn’t part of the plan originally, but it’s certainly doable for 0.2.

Ah, ok it's fine for now then, we can reevaluate when imago handles this, don't worry.

[slp]

@mtjhrc @tylerfanelli we need an approval here

[tylerfanelli]

LGTM.