From 862715545a034e526416fe1e1c561ed9ed57f3fd Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Thu, 1 Oct 2026 20:47:35 -0300 Subject: [PATCH] machine: Disk.Cache goes; nothing sets it No consumer sets a disk's cache mode (spin never has; #48 dropped the bench axis for that reason), and its own comment argued the default is right. Cache, cacheModes, their three validate cases and --disk-cache go; why the default is right moves to driveArg. A chain given a disk-level format is now asked of validate on its own. Co-Authored-By: Claude Opus 5.5 (1M context) --- cmd/spin-machine/commands_test.go | 4 ++-- cmd/spin-machine/main.go | 5 +---- machine/machine.go | 35 +++++++------------------------ machine/machine_test.go | 8 +++---- machine/qemu_accepts_test.go | 2 +- 5 files changed, 15 insertions(+), 39 deletions(-) diff --git a/cmd/spin-machine/commands_test.go b/cmd/spin-machine/commands_test.go index e2401ba..a4621d6 100644 --- a/cmd/spin-machine/commands_test.go +++ b/cmd/spin-machine/commands_test.go @@ -220,7 +220,7 @@ func TestFlagsAreTheSpec(t *testing.T) { every.Incoming = "file:/s" every.Serial = "" every.Disks = []machine.Disk{{Path: "/d", Format: "raw", Readonly: true, Serial: "ser", - Cache: "none", DirectOverBacking: true}} + DirectOverBacking: true}} every.Cmdline = machine.DefaultCmdline() every.Cmdline.Init, every.Cmdline.Root, every.Cmdline.RootReadonly = "/bin/sh", "/dev/vdb", true every.Cmdline = every.Cmdline.Profiling() @@ -245,7 +245,7 @@ func TestFlagsAreTheSpec(t *testing.T) { }{ {"nothing given", nil, defaults, true}, {"everything given", []string{"--qemu", "/q", "--kernel", "/k", "--initrd", "/i", "--firmware", "/f", "--bios", "bios-256k.bin", - "--disk", "/d", "--disk-format", "raw", "--disk-readonly", "--disk-serial", "ser", "--disk-cache", "none", + "--disk", "/d", "--disk-format", "raw", "--disk-readonly", "--disk-serial", "ser", "--disk-direct-over-backing", "--accel", "tcg", "--memory", "1024", "--max-memory", "4096", "--cpu", "Skylake-Server-v4", "--cpus", "3", "--max-cpus", "8", "--hotplug-disks", "2", "--vsock-cid", "7", "--qmp", "/qmp", "--incoming", "file:/s", "--console", "", "--init", "/bin/sh", diff --git a/cmd/spin-machine/main.go b/cmd/spin-machine/main.go index 6bdb5fc..0afc569 100644 --- a/cmd/spin-machine/main.go +++ b/cmd/spin-machine/main.go @@ -180,8 +180,7 @@ type machineFlags struct { diskFormat string readonly bool serial string - // diskCache and directOverBacking are machine.Disk's Cache and DirectOverBacking. - diskCache string + // directOverBacking is machine.Disk's DirectOverBacking. directOverBacking bool memoryMB int @@ -215,7 +214,6 @@ func (o *machineFlags) register(fs *flag.FlagSet) { fs.StringVar(&o.diskFormat, "disk-format", "qcow2", "format of the disk image; never guessed") fs.BoolVar(&o.readonly, "disk-readonly", false, "open the disk read-only, and mount root ro") fs.StringVar(&o.serial, "disk-serial", "", "serial the guest can resolve the disk by") - fs.StringVar(&o.diskCache, "disk-cache", "", "QEMU cache mode for the disk (default: QEMU's, which is writeback)") fs.BoolVar(&o.directOverBacking, "disk-direct-over-backing", false, "open the disk O_DIRECT and its backing chain through the host page cache (the disk must have a backing file)") @@ -309,7 +307,6 @@ func (o *machineFlags) spec() (machine.Spec, error) { Format: o.diskFormat, Readonly: o.readonly, Serial: o.serial, - Cache: o.diskCache, DirectOverBacking: o.directOverBacking, }} } diff --git a/machine/machine.go b/machine/machine.go index b334a26..e0df891 100644 --- a/machine/machine.go +++ b/machine/machine.go @@ -134,9 +134,6 @@ func qemuOpt(v string) string { return strings.ReplaceAll(v, ",", ",,") } // nobody here has thought about the safety of. var diskFormats = []string{"qcow2", "raw"} -// cacheModes are QEMU's -drive cache= values. -var cacheModes = []string{"none", "writeback", "writethrough", "directsync", "unsafe"} - // memGrowthID names the memory a virtio-mem device hands out. It is a separate // region from pc.ram, the memory the guest boots with: this one is empty until // somebody asks for it. @@ -176,20 +173,6 @@ type Disk struct { // writable disk, and only worth setting when something outside QEMU takes // the same lock to find out whether a VM is running on the image. Locking bool - // Cache overrides how the host caches this disk. Empty leaves QEMU's default, - // which is what this machine wants and is worth saying why. - // - // The usual advice for a production VM is cache=none — bypass the host page - // cache, because the guest caches the same blocks and holding them twice - // wastes RAM. It is the wrong advice here. The read-only base image is one - // file that *every* VM on the host maps through a backing chain, so one copy - // in the host's page cache is one copy shared by all of them; with O_DIRECT - // each VM would fault its own. The overlay is the part that would benefit, - // and it shares a -drive with the base. - // - // cache=none also fails outright on a filesystem with no O_DIRECT — tmpfs, - // which is where a scratch overlay usually lands. - Cache string // DirectOverBacking opens this image with O_DIRECT and leaves every image // under it in its backing chain on the host page cache: cache.direct=on for // this node, and backing.cache.direct=off, which the deeper backing files @@ -335,12 +318,14 @@ func encodeNode(n any) string { // without one then fails with "Could not open backing file: Must specify either driver // or file". A restore that wants every layer left alone declares its chain; see // Disk.Chain. +// +// No cache=: QEMU's default, writeback, is what this machine wants. The read-only base is +// one file every VM on the host maps through a backing chain, so one copy in the host's +// page cache serves all of them, where O_DIRECT would have each VM fault its own; and +// cache=none fails outright on tmpfs. The overlay alone is DirectOverBacking's to decide. func (d Disk) driveArg(i int) string { drive := fmt.Sprintf("file=%s,if=none,id=blk%d,format=%s,aio=io_uring,discard=unmap,file.drop-cache=off", qemuOpt(d.Path), i, d.Format) - if d.Cache != "" { - drive += ",cache=" + d.Cache - } if d.DirectOverBacking { drive += ",cache.direct=on,backing.cache.direct=off" } @@ -940,14 +925,8 @@ func (d Disk) validate(sets map[int]bool) error { return fmt.Errorf("%s has no format: it is not guessed", d.Path) case d.Path != "" && !slices.Contains(diskFormats, d.Format): return fmt.Errorf("%s: format %q is not one of %v", d.Path, d.Format, diskFormats) - case len(d.Chain) != 0 && (d.Cache != "" || d.Format != ""): - return errors.New("a chain carries a format per image and takes QEMU's caching") - case d.Cache != "" && !slices.Contains(cacheModes, d.Cache): - return fmt.Errorf("cache mode %q is not one of %v", d.Cache, cacheModes) - // cache= is itself a setting of cache.direct, so the two together are one option - // given twice with nothing to say which wins. - case d.Cache != "" && d.DirectOverBacking: - return fmt.Errorf("cache=%s and DirectOverBacking both decide cache.direct", d.Cache) + case len(d.Chain) != 0 && d.Format != "": + return errors.New("a chain carries a format per image") } for j, img := range d.Chain { switch { diff --git a/machine/machine_test.go b/machine/machine_test.go index 5d7519c..8240115 100644 --- a/machine/machine_test.go +++ b/machine/machine_test.go @@ -663,6 +663,10 @@ func TestValidateRefuses(t *testing.T) { s.FDSets = []FDSet{{ID: 1, FDs: []FD{{Num: 3}}}} s.Disks = []Disk{{Path: "/a.qcow2", Format: "qcow2", Chain: []Image{{FDSet: 1, Format: "qcow2"}}}} }}, + {"a chain with a format of its own", func(s *Spec) { + s.FDSets = []FDSet{{ID: 1, FDs: []FD{{Num: 3}}}} + s.Disks = []Disk{{Format: "qcow2", Chain: []Image{{FDSet: 1, Format: "qcow2"}}}} + }}, {"a chain image in a set nobody gave", func(s *Spec) { s.Disks = []Disk{{Chain: []Image{{FDSet: 9, Format: "qcow2"}}}} }}, @@ -688,10 +692,6 @@ func TestValidateRefuses(t *testing.T) { s.FDSets = []FDSet{{ID: 1, FDs: []FD{{Num: 3}}}} s.Disks = []Disk{{Chain: []Image{{FDSet: 1, Format: "vmdk"}}}} }}, - {"a cache mode that is not one", func(s *Spec) { s.Disks = []Disk{{Path: "/a", Format: "raw", Cache: "none,aio=threads"}} }}, - {"a cache mode and O_DIRECT over backing", func(s *Spec) { - s.Disks = []Disk{{Path: "/a", Format: "qcow2", Cache: "none", DirectOverBacking: true}} - }}, {"a CPU model with options", func(s *Spec) { s.CPU = "Skylake-Server-v4,enforce=off" }}, {"an accelerator this machine does not run under", func(s *Spec) { s.Accel = "xen" }}, // The kernel command line: each of these boots a guest that is told something diff --git a/machine/qemu_accepts_test.go b/machine/qemu_accepts_test.go index 190650c..b5c8394 100644 --- a/machine/qemu_accepts_test.go +++ b/machine/qemu_accepts_test.go @@ -175,7 +175,7 @@ func TestQEMUAcceptsEveryArgument(t *testing.T) { s.Disks = []Disk{ {Path: rawDisk(t, "base.raw"), Format: "raw", Readonly: true, Serial: "base"}, {Path: qcow2In(t, qemu, t.TempDir(), "overlay.qcow2", ""), Format: "qcow2", - Serial: "overlay", Locking: true, Cache: "writeback"}, + Serial: "overlay", Locking: true}, } return s },