Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use FALLOC_FL_ZERO_RANGE for write_zeroes #109

Closed
lauralt opened this issue Nov 17, 2020 · 2 comments
Closed

use FALLOC_FL_ZERO_RANGE for write_zeroes #109

lauralt opened this issue Nov 17, 2020 · 2 comments

Comments

@lauralt
Copy link
Contributor

lauralt commented Nov 17, 2020

While working on the virtio block discard/write_zeroes implementation, I noticed that write_zeroes() is using fallocate with FALLOC_FL_PUNCH_HOLE mode instead of FALLOC_FL_ZERO_RANGE. From the virtio specification
it makes sense to use FALLOC_FL_PUNCH_HOLE for write zeroes request only if unmap bit is also set, otherwise the zeroed region shouldn't be deallocated.

I think it would make sense to either update write_zeroes() with FALLOC_FL_ZERO_RANGE or add an extra parameter to that function (mode for example) which can be any FallocateMode variant.

Moreover, we can completely switch to the latest crosvm write_zeroes which is using FALLOC_FL_ZERO_RANGE for File and adds a new trait WriteZeroesAt which lets you give the offset in file instead of using the current cursor. It also allows doing custom implementations for write_zeroes() (which might be useful for example for qcow file wrappers, example here).

Any thoughts / suggestions?

@jiangliu
Copy link
Member

The latest implementation from crosvm is preferred. Then the virtio-blk driver chooses to call punch_hole() or write_zeroes_at() according to the unmap flag for VIRTIO_BLK_T_WRITE_ZEROES requests.

@lauralt
Copy link
Contributor Author

lauralt commented Nov 18, 2020

That's how I was seeing it too, thanks! I'll open a PR with the switch.

lauralt added a commit to lauralt/vmm-sys-util that referenced this issue Nov 19, 2020
Switched to the latest crosvm `write_zeroes` (commit f4a260d) which
uses FALLOC_FL_ZERO_RANGE instead of FALLOC_FL_PUNCH_HOLE for
writing zeroes to a file. It also adds a new trait, `WriteZeroesAt`,
which lets you give the offset in file instead of using the current
cursor. This trait allows doing custom implementations for
write_zeroes ops too.
There is also a new function, write_zeroes_all(), that allows
write_zeroes() implementations that don't necessarily fulfill the
entire requested length.
Fixes: rust-vmm#109.
Also added more tests to this module. I'd say this fixes
rust-vmm#98 too since
making fallocate() actually fail such that some bytes will be written
in the fallback option doesn't seem possible to achieve.

Signed-off-by: Laura Loghin <lauralg@amazon.com>
lauralt added a commit to lauralt/vmm-sys-util that referenced this issue Nov 19, 2020
Switched to the latest crosvm `write_zeroes` (commit f4a260d) which
uses FALLOC_FL_ZERO_RANGE instead of FALLOC_FL_PUNCH_HOLE for
writing zeroes to a file. It also adds a new trait, `WriteZeroesAt`,
which lets you give the offset in file instead of using the current
cursor. This trait allows doing custom implementations for
write_zeroes ops too.
There is also a new function, write_zeroes_all(), that allows
write_zeroes() implementations that don't necessarily fulfill the
entire requested length.
Fixes: rust-vmm#109.
Also added more tests to this module. I'd say this fixes
rust-vmm#98 too since
making fallocate() actually fail such that some bytes will be written
in the fallback option doesn't seem possible to achieve.

Signed-off-by: Laura Loghin <lauralg@amazon.com>
lauralt added a commit to lauralt/vmm-sys-util that referenced this issue Nov 20, 2020
Switched to the latest crosvm `write_zeroes` (commit f4a260d) which
uses FALLOC_FL_ZERO_RANGE instead of FALLOC_FL_PUNCH_HOLE for
writing zeroes to a file. It also adds a new trait, `WriteZeroesAt`,
which lets you give the offset in file instead of using the current
cursor. This trait allows doing custom implementations for
write_zeroes ops too.
There is also a new function, write_all_zeroes(), that allows
write_zeroes() implementations that don't necessarily fulfill the
entire requested length.
Fixes: rust-vmm#109.
Also added more tests to this module. I'd say this fixes
rust-vmm#98 too since
making fallocate() actually fail such that some bytes will be written
in the fallback option doesn't seem possible to achieve in unit tests.

Signed-off-by: Laura Loghin <lauralg@amazon.com>
lauralt added a commit to lauralt/vmm-sys-util that referenced this issue Nov 23, 2020
Switched to the latest crosvm `write_zeroes` (commit f4a260d) which
uses FALLOC_FL_ZERO_RANGE instead of FALLOC_FL_PUNCH_HOLE for
writing zeroes to a file. It also adds a new trait, `WriteZeroesAt`,
which lets you give the offset in file instead of using the current
cursor. This trait allows doing custom implementations for
write_zeroes ops too.
There is also a new function, write_all_zeroes(), that allows
write_zeroes() implementations that don't necessarily fulfill the
entire requested length.
Fixes: rust-vmm#109.
Also added more tests to this module. I'd say this fixes
rust-vmm#98 too since
making fallocate() actually fail such that some bytes will be written
in the fallback option doesn't seem possible to achieve in unit tests.

Signed-off-by: Laura Loghin <lauralg@amazon.com>
lauralt added a commit to lauralt/vmm-sys-util that referenced this issue Nov 23, 2020
Switched to the latest crosvm `write_zeroes` (commit f4a260d) which
uses FALLOC_FL_ZERO_RANGE instead of FALLOC_FL_PUNCH_HOLE for
writing zeroes to a file. It also adds a new trait, `WriteZeroesAt`,
which lets you give the offset in file instead of using the current
cursor. This trait allows doing custom implementations for
write_zeroes ops too.
There is also a new function, write_all_zeroes(), that allows
write_zeroes() implementations that don't necessarily fulfill the
entire requested length.
Fixes: rust-vmm#109.
Also added more tests to this module. I'd say this fixes
rust-vmm#98 too since
making fallocate() actually fail such that some bytes will be written
in the fallback option doesn't seem possible to achieve in unit tests.

Signed-off-by: Laura Loghin <lauralg@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants