From 3d069d3ee0c5bf50fd0fc2f834cff884f671324f Mon Sep 17 00:00:00 2001 From: Gleb Chesnokov Date: Wed, 14 Oct 2026 15:51:19 +0300 Subject: [PATCH] tests: Cover fileio sync across backend detach and reactivation The sysfs sync regression in issue #394 originates in commit 530af782ed0c ("scst_vdisk: Add 'sync' sysfs attribute"). It dereferenced a closed file. Require a management rejection for inactive devices, the last detach and ALUA states that close the backend, then verify recovery. Remove only the fixture device's internal copy-manager mappings when checking the last attachment: an external LUN count alone does not prove that the backend is closed. Use an explicit local session for successful sync and preserve unrelated devices. Put session LUNs in an explicit initiator group because scstadmin does not report the default group map in its session listing. Drop the temporary known-crash label, opt-in setting and example filter. The inactive-sync regression now belongs to the default basic selection. Link: https://github.com/SCST-project/scst/issues/394 --- tests/ginkgo/README.md | 15 +++-- tests/ginkgo/config.example.json | 1 - tests/ginkgo/internal/testcfg/config.go | 1 - tests/ginkgo/internal/testcfg/config_test.go | 6 +- tests/ginkgo/suite/alua_state_test.go | 25 ++++++-- tests/ginkgo/suite/fileio_test.go | 66 +++++++++++++++++--- 6 files changed, 86 insertions(+), 28 deletions(-) diff --git a/tests/ginkgo/README.md b/tests/ginkgo/README.md index 4280c2efc..ee22f7ad4 100644 --- a/tests/ginkgo/README.md +++ b/tests/ginkgo/README.md @@ -52,16 +52,15 @@ the backend. For example, `local && io && fileio` needs no physical disk. ```sh # List every spec and its labels without accessing either host: make ginkgo-list GINKGO_LABEL='' -# All except the known-crash case; prepare both hosts and a block device first: -make ginkgo-test-build GINKGO_CONFIG=tests/ginkgo/config.local.json GINKGO_LABEL='!known-crash' +# All specs; prepare both hosts and a block device first: +make ginkgo-test-build GINKGO_CONFIG=tests/ginkgo/config.local.json GINKGO_LABEL='' ``` -An empty label filter selects all specs. The `known-crash` case (sync on -inactive fileio) also requires `allow_known_crash=true` and can crash affected -kernels. `clear-config` tests invoke global `scstadmin -clear_config -force`. -Only selected transport groups access hosts. Local teardown finishes before -iSCSI setup; an empty selection or suite failure stops the run. Parallel runs -are unsupported: use the same `lock_file` and runner user on each host. +An empty label filter selects all specs. `clear-config` tests invoke global +`scstadmin -clear_config -force`. Only selected transport groups access hosts. +Local teardown finishes before iSCSI setup; an empty selection or suite +failure stops the run. Parallel runs are unsupported: use the same +`lock_file` and runner user on each host. ## Configuration diff --git a/tests/ginkgo/config.example.json b/tests/ginkgo/config.example.json index 9864a18f9..cf78d6bbe 100644 --- a/tests/ginkgo/config.example.json +++ b/tests/ginkgo/config.example.json @@ -15,7 +15,6 @@ "fileio_thin_provisioning": true, "stress_iterations": 8, "scale_luns": 256, - "allow_known_crash": false, "io": { "block_device": "", "region_mb": 64, diff --git a/tests/ginkgo/internal/testcfg/config.go b/tests/ginkgo/internal/testcfg/config.go index 306e2a56e..a93871b32 100644 --- a/tests/ginkgo/internal/testcfg/config.go +++ b/tests/ginkgo/internal/testcfg/config.go @@ -32,7 +32,6 @@ type Config struct { FileioThinProvisioning bool `json:"fileio_thin_provisioning"` StressIterations int `json:"stress_iterations"` ScaleLUNs int `json:"scale_luns"` - AllowKnownCrash bool `json:"allow_known_crash"` IO IO `json:"io"` ISCSI *ISCSI `json:"iscsi,omitempty"` } diff --git a/tests/ginkgo/internal/testcfg/config_test.go b/tests/ginkgo/internal/testcfg/config_test.go index 40ffdab38..18448b833 100644 --- a/tests/ginkgo/internal/testcfg/config_test.go +++ b/tests/ginkgo/internal/testcfg/config_test.go @@ -29,7 +29,7 @@ func TestLoadDefaultsAndPaths(t *testing.T) { if cfg.DisposableHost || cfg.CommandTimeout != "30s" || cfg.BuildJobs != 2 || cfg.BuildTimeout != "10m" { t.Fatalf("unsafe or missing defaults: %+v", cfg) } - if cfg.AllowKnownCrash || !cfg.FileioThinProvisioning || cfg.StressIterations != 8 || cfg.ScaleLUNs != 256 || cfg.BackingDir != filepath.Join(base, "scratch") { + if !cfg.FileioThinProvisioning || cfg.StressIterations != 8 || cfg.ScaleLUNs != 256 || cfg.BackingDir != filepath.Join(base, "scratch") { t.Fatalf("unsafe or missing scenario defaults: %+v", cfg) } } @@ -72,11 +72,11 @@ func TestRejectMalformedConfiguration(t *testing.T) { } func TestScenarioOverrides(t *testing.T) { - c, err := Load(configFile(t, `{"stress_iterations":12,"scale_luns":1000,"allow_known_crash":true,"fileio_thin_provisioning":false}`)) + c, err := Load(configFile(t, `{"stress_iterations":12,"scale_luns":1000,"fileio_thin_provisioning":false}`)) if err != nil { t.Fatal(err) } - if c.StressIterations != 12 || c.ScaleLUNs != 1000 || !c.AllowKnownCrash || c.FileioThinProvisioning || c.BackingDir != "" { + if c.StressIterations != 12 || c.ScaleLUNs != 1000 || c.FileioThinProvisioning || c.BackingDir != "" { t.Fatalf("scenario overrides lost: %+v", c) } } diff --git a/tests/ginkgo/suite/alua_state_test.go b/tests/ginkgo/suite/alua_state_test.go index ca768b421..f7dec9067 100644 --- a/tests/ginkgo/suite/alua_state_test.go +++ b/tests/ginkgo/suite/alua_state_test.go @@ -31,9 +31,14 @@ var _ = Describe("ALUA device activation", Label("basic", "alua", "fileio", "iss func(ctx SpecContext, mapped bool) { d := f.fileDevice(ctx, "disk", f.backingFile("disk"), "bind_alua_state=1") neighbor := f.fileDevice(ctx, "neighbor", f.backingFile("neighbor"), "bind_alua_state=1") - t, dg := f.target(ctx, "target"), f.deviceGroup(ctx, "devices") + session := f.name("session") + t, dg := f.target(ctx, "target", "session_name="+session), f.deviceGroup(ctx, "devices") + f.detachCopyManager(ctx, d) if mapped { - f.at(ctx, t, "", "-add_lun", "0", "-device", d) + f.at(ctx, t, "", "-add_group", "access") + f.at(ctx, t, "access", "-add_lun", "0", "-device", d) + f.at(ctx, t, "access", "-add_init", session) + f.sessionLUNs(ctx, t, session, map[int]string{0: d}) } f.do(ctx, "-add_dgrp_dev", d, "-dev_group", dg) tg := f.targetGroup(ctx, dg, "local", t, "11") @@ -41,15 +46,21 @@ var _ = Describe("ALUA device activation", Label("basic", "alua", "fileio", "iss Expect(f.deviceAttr(ctx, d, "active")).To(Equal("1")) f.aluaState(ctx, dg, tg, "transitioning") Expect(f.deviceAttr(ctx, d, "active")).To(Equal("0")) + f.reject(ctx, "-set_dev_attr", d, "-attributes", "sync=1") Expect(f.deviceAttr(ctx, neighbor, "active")).To(Equal("1")) path := f.backingFile("replacement") f.setDevice(ctx, d, "filename="+path) Expect(f.deviceAttr(ctx, d, "filename")).To(Equal(path)) f.aluaState(ctx, dg, tg, "nonoptimized") Expect(f.deviceAttr(ctx, d, "active")).To(Equal("1")) - f.setDevice(ctx, d, "sync=1") + if mapped { + f.setDevice(ctx, d, "sync=1") + } else { + f.reject(ctx, "-set_dev_attr", d, "-attributes", "sync=1") + } f.aluaState(ctx, dg, tg, "standby") Expect(f.deviceAttr(ctx, d, "active")).To(Equal("0")) + f.reject(ctx, "-set_dev_attr", d, "-attributes", "sync=1") f.aluaState(ctx, dg, tg, "active") Expect(f.deviceAttr(ctx, d, "active")).To(Equal("1")) f.reject(ctx, "-set_tgrp_attr", tg, "-dev_group", dg, "-attributes", "state=invalid_state") @@ -57,13 +68,14 @@ var _ = Describe("ALUA device activation", Label("basic", "alua", "fileio", "iss Expect(f.deviceAttr(ctx, d, "active")).To(Equal("1")) Expect(f.deviceAttr(ctx, neighbor, "active")).To(Equal("1")) if mapped { - Expect(f.luns(ctx, t, "")).To(Equal(map[int]string{0: d})) + Expect(f.luns(ctx, t, "access")).To(Equal(map[int]string{0: d})) } }, Entry("with a LUN mapping", true), Entry("without a LUN mapping", false)) It("keeps local devices active when only a remote group changes", func(ctx SpecContext) { d := f.fileDevice(ctx, "disk", f.backingFile("disk"), "bind_alua_state=1") - t, dg := f.target(ctx, "target"), f.deviceGroup(ctx, "devices") + t, _ := f.attachFileio(ctx, d) + dg := f.deviceGroup(ctx, "devices") f.do(ctx, "-add_dgrp_dev", d, "-dev_group", dg) local := f.targetGroup(ctx, dg, "local", t, "11") remote := f.targetGroup(ctx, dg, "remote", f.name("remote_target"), "12") @@ -78,7 +90,8 @@ var _ = Describe("ALUA device activation", Label("basic", "alua", "fileio", "iss It("leaves unbound fileio active during a local ALUA transition", func(ctx SpecContext) { d := f.fileDevice(ctx, "disk", f.backingFile("disk"), "bind_alua_state=0") - t, dg := f.target(ctx, "target"), f.deviceGroup(ctx, "devices") + t, _ := f.attachFileio(ctx, d) + dg := f.deviceGroup(ctx, "devices") f.do(ctx, "-add_dgrp_dev", d, "-dev_group", dg) tg := f.targetGroup(ctx, dg, "local", t, "11") f.aluaState(ctx, dg, tg, "active") diff --git a/tests/ginkgo/suite/fileio_test.go b/tests/ginkgo/suite/fileio_test.go index 360491606..f66f3c89d 100644 --- a/tests/ginkgo/suite/fileio_test.go +++ b/tests/ginkgo/suite/fileio_test.go @@ -2,10 +2,37 @@ package suite import ( + "context" + "strconv" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) +// Drop only this fixture's copy-manager mappings. Without this, a device can +// retain an open backend after the last external LUN has been removed. +func (f *fixture) detachCopyManager(ctx context.Context, device string) { + GinkgoHelper() + for lun, d := range copyManagerLUNs(ctx) { + if d == device { + f.do(ctx, "-rem_lun", strconv.Itoa(lun), "-driver", "copy_manager", "-target", "copy_manager_tgt") + } + } + Expect(copyManagerLUNs(ctx)).NotTo(ContainElement(device)) +} + +func (f *fixture) attachFileio(ctx context.Context, device string) (string, string) { + GinkgoHelper() + session := f.name("session") + target := f.target(ctx, "target", "session_name="+session) + // scstadmin reports session LUNs only for explicit initiator groups. + f.at(ctx, target, "", "-add_group", "access") + f.at(ctx, target, "access", "-add_lun", "0", "-device", device) + f.at(ctx, target, "access", "-add_init", session) + f.sessionLUNs(ctx, target, session, map[int]string{0: device}) + return target, session +} + var _ = Describe("File-backed devices", Label("basic", "fileio"), func() { var f *fixture BeforeEach(func() { f = newFixture() }) @@ -13,6 +40,7 @@ var _ = Describe("File-backed devices", Label("basic", "fileio"), func() { It("syncs an active device and keeps its configuration across reactivation", func(ctx SpecContext) { path := f.backingFile("disk") d := f.fileDevice(ctx, "disk", path) + f.attachFileio(ctx, d) Expect(f.deviceAttr(ctx, d, "active")).To(Equal("1")) f.setDevice(ctx, d, "sync=1") f.setDevice(ctx, d, "active=0") @@ -27,6 +55,7 @@ var _ = Describe("File-backed devices", Label("basic", "fileio"), func() { It("rejects an active backing change and recovers from a failed activation", Label("negative"), func(ctx SpecContext) { first, second := f.backingFile("first"), f.backingFile("second") d := f.fileDevice(ctx, "disk", first, "thin_provisioned=0") + f.attachFileio(ctx, d) f.reject(ctx, "-set_dev_attr", d, "-attributes", "filename="+second) Expect(f.deviceAttr(ctx, d, "filename")).To(Equal(first)) Expect(f.deviceAttr(ctx, d, "active")).To(Equal("1")) @@ -91,23 +120,42 @@ var _ = Describe("File-backed devices", Label("basic", "fileio"), func() { }, Entry("disabled", "0"), Entry("enabled on a capable filesystem", "1")) }) -var _ = It("handles sync on an inactive fileio device without losing manageability", Label("fileio", "known-crash", "issue-394"), func(ctx SpecContext) { - if !cfg.AllowKnownCrash { - Skip("issue #394 can crash the kernel; requires allow_known_crash=true and an explicit label selection") - } +var _ = It("handles sync on an inactive fileio device without losing manageability", Label("basic", "fileio", "negative", "issue-394"), func(ctx SpecContext) { f := newFixture() path := f.backingFile("disk") d := f.fileDevice(ctx, "disk", path) + f.attachFileio(ctx, d) f.setDevice(ctx, d, "active=0") Expect(f.deviceAttr(ctx, d, "active")).To(Equal("0")) - // The public contract does not specify rejection versus a no-op. Either - // completed CLI outcome is acceptable; an execution failure is not. - r, err := cli.Execute(ctx, "-set_dev_attr", d, "-attributes", "sync=1") - Expect(err).NotTo(HaveOccurred(), r.String()) - AddReportEntry("inactive sync result", r.String()) + f.reject(ctx, "-set_dev_attr", d, "-attributes", "sync=1") Expect(f.deviceAttr(ctx, d, "active")).To(Equal("0")) Expect(f.deviceAttr(ctx, d, "filename")).To(Equal(path)) + Expect(f.deviceAttr(ctx, d, "size_mb")).To(Equal("16")) f.setDevice(ctx, d, "active=1") Expect(f.deviceAttr(ctx, d, "active")).To(Equal("1")) f.setDevice(ctx, d, "sync=1") }) + +var _ = It("rejects sync after the last attachment and reopens on a new attachment", Label("basic", "fileio", "negative", "issue-394"), func(ctx SpecContext) { + f := newFixture() + path := f.backingFile("disk") + d := f.fileDevice(ctx, "disk", path) + f.detachCopyManager(ctx, d) + Expect(f.deviceAttr(ctx, d, "active")).To(Equal("1")) + f.reject(ctx, "-set_dev_attr", d, "-attributes", "sync=1") + // Reactivation alone must not invent a backend attachment. + f.setDevice(ctx, d, "active=0") + f.setDevice(ctx, d, "active=1") + f.reject(ctx, "-set_dev_attr", d, "-attributes", "sync=1") + target, session := f.attachFileio(ctx, d) + f.setDevice(ctx, d, "sync=1") + f.at(ctx, target, "access", "-rem_lun", "0") + f.sessionLUNs(ctx, target, session, map[int]string{}) + f.reject(ctx, "-set_dev_attr", d, "-attributes", "sync=1") + Expect(f.deviceAttr(ctx, d, "active")).To(Equal("1")) + Expect(f.deviceAttr(ctx, d, "filename")).To(Equal(path)) + Expect(f.deviceAttr(ctx, d, "size_mb")).To(Equal("16")) + f.at(ctx, target, "access", "-add_lun", "0", "-device", d) + f.sessionLUNs(ctx, target, session, map[int]string{0: d}) + f.setDevice(ctx, d, "sync=1") +})