New issue
Advanced search Search tips

Issue 850998 link

Starred by 15 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 12
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Feature



Sign in to add a comment

Add discard support for crosvm disk image using virtio-blk discard and write zeroes feature

Project Member Reported by smbar...@chromium.org, Jun 8 2018

Issue description

We should add support for discard to shrink the stateful disk image when possible.

This will depend on implementing virtio-scsi.
 
Labels: -Pri-3 Hotlist-Crostini-Platform Pri-2
Status: Available (was: Untriaged)
Summary: Add discard support for crosvm disk image (was: discard support for crosvm)
Cc: dverkamp@chromium.org dgreid@chromium.org
Labels: -Type-Bug -Pri-2 M-71 Pri-1 Type-Feature
Summary: Add discard support for crosvm disk image using virtio-blk discard and write zeroes feature (was: Add discard support for crosvm disk image)
This was approved by the virtio committee here: https://github.com/oasis-tcs/virtio-spec/commit/88c8553838346b26be4460485cc57c38850b36f7

This should be implemented instead of virtio-scsi in crosvm and linux kernel.
Looks like the current revision of the kernel patch to add support for discard and write zeroes to virtio-blk is here:
https://lists.linuxfoundation.org/pipermail/virtualization/2018-June/038299.html
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 16

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/crosvm/+/d0a0828b2e3a5dde4338f5b58ce355c75a79fa9d

commit d0a0828b2e3a5dde4338f5b58ce355c75a79fa9d
Author: Daniel Verkamp <dverkamp@chromium.org>
Date: Thu Aug 16 03:36:11 2018

devices: block: define config space as a struct

Define a struct to represent the virtio block configuration as defined
in the current revision of the specification.  Only the capacity field
is set; all other fields are defaulted to 0, which should be safe, since
they are all controlled by feature flags that our device does not
advertise.

This is prep work for adding discard/write zeroes support to the block
device, since these commands require additional config fields that will
be added at the end of the config struct.

BUG= chromium:850998 
TEST=cargo test -p devices; verify that container still boots and
reports the correct size

Change-Id: I8beb76195e446eb3dcbf1a99cc246ddd8cc8a7df
Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1175235
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Reviewed-by: Zach Reizner <zachr@chromium.org>

[modify] https://crrev.com/d0a0828b2e3a5dde4338f5b58ce355c75a79fa9d/devices/src/virtio/block.rs

Owner: dverkamp@chromium.org
Status: Started (was: Available)
Project Member

Comment 6 by bugdroid1@chromium.org, Sep 5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/crosvm/+/0e8f6fa96e454f8db36787eda91327e32e1b159a

commit 0e8f6fa96e454f8db36787eda91327e32e1b159a
Author: Daniel Verkamp <dverkamp@chromium.org>
Date: Wed Sep 05 08:25:49 2018

sys_util: add safe wrapper for fallocate()

BUG= chromium:850998 
TEST=None

Change-Id: I1b6864f7d508cf7f24248a8cc9783af2d8b00891
Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1187016
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Reviewed-by: Stephen Barber <smbarber@chromium.org>

[modify] https://crrev.com/0e8f6fa96e454f8db36787eda91327e32e1b159a/sys_util/src/lib.rs

Project Member

Comment 7 by bugdroid1@chromium.org, Sep 5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/crosvm/+/a3d11edaa6d728adcad0693ab63a7e9e49690126

commit a3d11edaa6d728adcad0693ab63a7e9e49690126
Author: Daniel Verkamp <dverkamp@chromium.org>
Date: Wed Sep 05 08:25:49 2018

sys_util: add WriteZeroes trait and impl for File

BUG= chromium:850998 
TEST=cargo test -p sys_util write_zeroes

Change-Id: I90f670fffed055f39e141b26819d56948b1bf30d
Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1187017
Reviewed-by: Stephen Barber <smbarber@chromium.org>

[modify] https://crrev.com/a3d11edaa6d728adcad0693ab63a7e9e49690126/sys_util/src/lib.rs
[add] https://crrev.com/a3d11edaa6d728adcad0693ab63a7e9e49690126/sys_util/src/write_zeroes.rs

Project Member

Comment 8 by bugdroid1@chromium.org, Sep 5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/crosvm/+/95a8868aef2c1018b13b30f3afc2e11bb3ae2066

commit 95a8868aef2c1018b13b30f3afc2e11bb3ae2066
Author: Daniel Verkamp <dverkamp@chromium.org>
Date: Wed Sep 05 08:25:50 2018

qcow: implement WriteZeroes for QcowFile

Add a simple implementation of WriteZeroes for QcowFile that just
writes zeroes to allocated clusters and skips clusters that are already
unallocated (since they already read back as zeroes).

BUG= chromium:850998 
TEST=cargo test -p qcow

Change-Id: I8f26c8cc4016c129850aaf08c7188dfe08d6dacb
Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1187018
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Reviewed-by: Zach Reizner <zachr@chromium.org>

[modify] https://crrev.com/95a8868aef2c1018b13b30f3afc2e11bb3ae2066/qcow/Cargo.toml
[modify] https://crrev.com/95a8868aef2c1018b13b30f3afc2e11bb3ae2066/qcow/src/qcow.rs

Project Member

Comment 9 by bugdroid1@chromium.org, Sep 5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/crosvm/+/9c7cd8632563ba50d1b0452b2ac1a8defd63e5f3

commit 9c7cd8632563ba50d1b0452b2ac1a8defd63e5f3
Author: Daniel Verkamp <dverkamp@chromium.org>
Date: Wed Sep 05 08:25:50 2018

qcow: deallocate clusters in write_zeroes

When a write_zeroes call covers a whole cluster, we can deallocate the
storage for that cluster rather than writing zeroes.

This is currently implemented by removing the cluster allocation from
the mapping tables, then attempting to release the backing storage using
fallocate() with FALLOC_FL_PUNCH_HOLE.

BUG= chromium:850998 
TEST=cargo test -p qcow

Change-Id: Ie4edb2e02bfaa1df9a19919b77eeb3c58c112d1c
Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1187019
Reviewed-by: Dylan Reid <dgreid@chromium.org>

[modify] https://crrev.com/9c7cd8632563ba50d1b0452b2ac1a8defd63e5f3/qcow/src/qcow.rs

Project Member

Comment 10 by bugdroid1@chromium.org, Sep 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/crosvm/+/7621d910f56ff85400b252f88fdef324a1cc13d6

commit 7621d910f56ff85400b252f88fdef324a1cc13d6
Author: Daniel Verkamp <dverkamp@chromium.org>
Date: Mon Sep 10 20:33:46 2018

devices: block: implement discard and write zeroes

Discard and Write Zeroes commands have been added to the virtio block
specification:
https://github.com/oasis-tcs/virtio-spec/commit/88c8553838346b26be4460485cc57c38850b36f7

Implement both commands using the WriteZeroes trait.

BUG= chromium:850998 
TEST=fstrim within termina on a writable qcow image

Change-Id: I33e54e303202328c10f7f2d6e69ab19f419f3998
Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1188680
Reviewed-by: Stephen Barber <smbarber@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>

[modify] https://crrev.com/7621d910f56ff85400b252f88fdef324a1cc13d6/seccomp/x86_64/block_device.policy
[modify] https://crrev.com/7621d910f56ff85400b252f88fdef324a1cc13d6/devices/src/virtio/block.rs
[modify] https://crrev.com/7621d910f56ff85400b252f88fdef324a1cc13d6/seccomp/arm/block_device.policy

Project Member

Comment 11 by bugdroid1@chromium.org, Sep 11

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/8ac58d10d3dbfc81dc619f2eab4f9012e402890f

commit 8ac58d10d3dbfc81dc619f2eab4f9012e402890f
Author: Daniel Verkamp <dverkamp@chromium.org>
Date: Tue Sep 11 00:17:58 2018

vm_tools: maitred: enable discard for stateful fs

Add the BTRFS 'discard' mount option so that space is automatically
deallocated when deleting files in the guest.

This depends on the crosvm virtio block discard patch in order to have
an effect.

BUG= chromium:850998 
TEST=Delete a 1 GB file in the guest and verify qcow disk usage shrinks
CQ-DEPEND=CL:1188680

Change-Id: I602d65178c47e7b070ed73f8e8e5504405dc2386
Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1205574
Reviewed-by: Stephen Barber <smbarber@chromium.org>

[modify] https://crrev.com/8ac58d10d3dbfc81dc619f2eab4f9012e402890f/vm_tools/maitred/service_impl.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Sep 14

Labels: merge-merged-chromeos-4.14
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/aac93a82783f1c26460ca6e195eb75d1e319bfc8

commit aac93a82783f1c26460ca6e195eb75d1e319bfc8
Author: Changpeng Liu <changpeng.liu@intel.com>
Date: Fri Sep 14 01:58:44 2018

BACKPORT: FROMLIST: virtio_blk: add discard and write zeroes support

In commit 88c85538, "virtio-blk: add discard and write zeroes features
to specification" (https://github.com/oasis-tcs/virtio-spec), the virtio
block specification has been extended to add VIRTIO_BLK_T_DISCARD and
VIRTIO_BLK_T_WRITE_ZEROES commands.  This patch enables support for
discard and write zeroes in the virtio-blk driver when the device
advertises the corresponding features, VIRTIO_BLK_F_DISCARD and
VIRTIO_BLK_F_WRITE_ZEROES.

Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>

BUG= chromium:850998 
TEST=fstrim on nami inside termina VM with crosvm patched to use discard

(am from https://lists.linuxfoundation.org/pipermail/virtualization/2018-August/039143.html)

Backport: Use queue_flag_set_unlocked() in place of blk_queue_flag_set(),
as the latter is not available in 4.14.

Change-Id: Ib57f2a1221e715d56d512925222f978fc2c81b32
Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1205611
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Reviewed-by: Stephen Barber <smbarber@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>

[modify] https://crrev.com/aac93a82783f1c26460ca6e195eb75d1e319bfc8/include/uapi/linux/virtio_blk.h
[modify] https://crrev.com/aac93a82783f1c26460ca6e195eb75d1e319bfc8/drivers/block/virtio_blk.c

Project Member

Comment 13 by bugdroid1@chromium.org, Sep 26

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/crosvm/+/36d4ec520eea957564ef47bc11dbbcbcb3756460

commit 36d4ec520eea957564ef47bc11dbbcbcb3756460
Author: Daniel Verkamp <dverkamp@chromium.org>
Date: Wed Sep 26 17:33:14 2018

sys_util: use fallocate64 for large file support

Rust's libc crate exports the default off_t definition on 32-bit
platforms, rather than the _FILE_OFFSET_BITS=64 variant, so we need to
explicitly use the 64-bit API to get support for files larger than 2 GB.

The Rust libc crate does not currently export fallocate64, so declare it
ourselves for now.  This declaration can be removed once fallocate64 is
added upstream.

BUG= chromium:850998 
TEST=Run fstrim on Kevin (32-bit ARM) and verify it works

Change-Id: Id0aa7a6e7e6080f4c53e10c3ad1d105f15ee2549
Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1238850
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Reviewed-by: Zach Reizner <zachr@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>

[modify] https://crrev.com/36d4ec520eea957564ef47bc11dbbcbcb3756460/sys_util/src/lib.rs

Project Member

Comment 14 by bugdroid1@chromium.org, Oct 5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/crosvm/+/b1570f2672ada3f5c4b9592887186413d80e66de

commit b1570f2672ada3f5c4b9592887186413d80e66de
Author: Daniel Verkamp <dverkamp@chromium.org>
Date: Fri Oct 05 14:54:49 2018

qcow: track deallocated clusters as unreferenced

In deallocate_cluster(), we call set_cluster_refcount() to unref the
cluster that is being deallocated, but we never actually added the
deallocated cluster to the unref_clusters list.  Add clusters whose
refcounts reach 0 to the unref_clusters list as well.

Also add mremap() to the seccomp whitelist for the block device, since
this is being triggered by libc realloc() and other devices already
include it in the whitelist.

BUG= chromium:850998 
TEST=cargo test -p qcow; test crosvm on nami and verify that qcow file
     size stays bounded when creating a 1 GB file and deleting it
     repeatedly

Change-Id: I1bdd96b2176dc13069417e0ac77f0768f9f26012
Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1259404
Reviewed-by: Dylan Reid <dgreid@chromium.org>

[modify] https://crrev.com/b1570f2672ada3f5c4b9592887186413d80e66de/seccomp/x86_64/block_device.policy
[modify] https://crrev.com/b1570f2672ada3f5c4b9592887186413d80e66de/qcow/src/qcow.rs
[modify] https://crrev.com/b1570f2672ada3f5c4b9592887186413d80e66de/seccomp/arm/block_device.policy

Status: Fixed (was: Started)
This has been implemented for a while now.

Sign in to add a comment