[PATCH 0/2] tests: add virtio-fs DAX manager tests

[Fotis Xenakis]

These add white-box unit tests for the virtio-fs DAX window manager,
exercising the manager's internal operations, using Boost.Test.

TBH, I am not too confident in these being of great value and also not
particularly proud of the result elegance-wise (though this is my first
time doing proper automated testing of C++ code, so my expectations
might be skewed). In any case, I trust your judgement as to whether
these should be merged, modified or not, and would be glad to discuss
any point and address and requests.

P.S. These are the test cases I had originally tested the implementation
with, slightly amended. That testing had been done in isolation though,
using a stubbed-out copy of the code, that's why they hadn't been
included in the first place.

Fotis Xenakis (2):
virtio-fs: make DAX manager testable
tests: add DAX manager white-box unit tests

fs/virtiofs/virtiofs.hh | 9 +-
fs/virtiofs/virtiofs_dax.cc | 55 +++---
fs/virtiofs/virtiofs_dax.hh | 90 ++++++---
fs/virtiofs/virtiofs_vfsops.cc | 5 +-
modules/tests/Makefile | 6 +-
tests/tst-dax.cc | 333 +++++++++++++++++++++++++++++++++
6 files changed, 444 insertions(+), 54 deletions(-)
create mode 100644 tests/tst-dax.cc

--
2.30.1

[Fotis Xenakis]

This mainly includes changes allowing the actual DAX window operations
to be replaced with stubs. To achieve that we:
1. Introduce a new class virtiofs::dax_window_impl, encapsulating the
DAX window and its operations (previously those were members of
dax_manager).
2. Introduce a stub version of the DAX window,
virtiofs::dax_window_stub.
3. Turn the dax_manager class into a template, with the DAX window
flavour (normal or stub) as its parameter. This was deemed cleaner
than going with run-time polymorphism.
4. Make all previously private members of dax_manager protected,
allowing them to be accessed by the test code.

Signed-off-by: Fotis Xenakis <fo...@windowslive.com>
---
fs/virtiofs/virtiofs.hh | 9 +---
fs/virtiofs/virtiofs_dax.cc | 55 +++++++++++++--------
fs/virtiofs/virtiofs_dax.hh | 90 ++++++++++++++++++++++++++--------
fs/virtiofs/virtiofs_vfsops.cc | 5 +-
4 files changed, 108 insertions(+), 51 deletions(-)

diff --git a/fs/virtiofs/virtiofs.hh b/fs/virtiofs/virtiofs.hh
index 8a3b06f2..b474b718 100644
--- a/fs/virtiofs/virtiofs.hh
+++ b/fs/virtiofs/virtiofs.hh
@@ -16,6 +16,7 @@

#include "drivers/virtio-fs.hh"
#include "fuse_kernel.h"
+#include "virtiofs_dax.hh"

#define VIRTIOFS_DEBUG_ENABLED 1

@@ -25,15 +26,9 @@
#define virtiofs_debug(...)
#endif

-// Necessary pre-declaration because virtiofs::dax depends on virtiofs_inode,
-// declared below.
-namespace virtiofs {
-class dax_manager;
-}
-
struct virtiofs_mount_data {
virtio::fs* drv;
- std::shared_ptr<virtiofs::dax_manager> dax_mgr;
+ std::shared_ptr<virtiofs::dax_manager_impl> dax_mgr;
};

struct virtiofs_inode {
diff --git a/fs/virtiofs/virtiofs_dax.cc b/fs/virtiofs/virtiofs_dax.cc
index 38ccdc39..eceff3e0 100644
--- a/fs/virtiofs/virtiofs_dax.cc
+++ b/fs/virtiofs/virtiofs_dax.cc
@@ -8,6 +8,7 @@
#include <algorithm>
#include <mutex>

+#include <api/assert.h>
#include <osv/debug.h>
#include <osv/uio.h>

@@ -18,8 +19,9 @@

namespace virtiofs {

-int dax_manager::read(virtiofs_inode& inode, uint64_t file_handle, u64 read_amt,
- struct uio& uio, bool aggressive)
+template<typename W>
+int dax_manager<W>::read(virtiofs_inode& inode, uint64_t file_handle,
+ u64 read_amt, struct uio& uio, bool aggressive)
{
std::lock_guard<mutex> guard {_lock};

@@ -63,7 +65,7 @@ int dax_manager::read(virtiofs_inode& inode, uint64_t file_handle, u64 read_amt,
}

out:
- auto req_data = _window->addr + (mp.mstart * _chunk_size) + coffset;
+ auto req_data = _window.data() + (mp.mstart * _chunk_size) + coffset;
auto read_amt_act = std::min<size_t>(read_amt,
(mp.nchunks * _chunk_size) - coffset);
// NOTE: It shouldn't be necessary to use the mmio* interface (i.e. volatile
@@ -76,7 +78,8 @@ out:
return error;
}

-int dax_manager::map(uint64_t nodeid, uint64_t file_handle, chunk nchunks,
+template<typename W>
+int dax_manager<W>::map(uint64_t nodeid, uint64_t file_handle, chunk nchunks,
chunk fstart, mapping_part& mapped, bool evict)
{
// If necessary, unmap just enough chunks
@@ -99,7 +102,8 @@ int dax_manager::map(uint64_t nodeid, uint64_t file_handle, chunk nchunks,

// Map new chunks
auto mstart = _window_chunks - empty;
- auto error = map_ll(nodeid, file_handle, to_map, fstart, mstart);
+ auto error = _window.map(nodeid, file_handle, to_map * _chunk_size,
+ fstart * _chunk_size, mstart * _chunk_size);
if (error) {
return error;
}
@@ -119,7 +123,8 @@ int dax_manager::map(uint64_t nodeid, uint64_t file_handle, chunk nchunks,
return 0;
}

-int dax_manager::unmap(chunk nchunks, mapping_part& unmapped, bool deep)
+template<typename W>
+int dax_manager<W>::unmap(chunk nchunks, mapping_part& unmapped, bool deep)
{
// Determine necessary changes
chunk to_unmap = 0;
@@ -148,7 +153,8 @@ int dax_manager::unmap(chunk nchunks, mapping_part& unmapped, bool deep)
// Apply changes
if (deep) {
auto mstart = first_empty() - to_unmap;
- auto error = unmap_ll(to_unmap, mstart);
+ auto error = _window.unmap(to_unmap * _chunk_size,
+ mstart * _chunk_size);
if (error) {
return error;
}
@@ -163,10 +169,10 @@ int dax_manager::unmap(chunk nchunks, mapping_part& unmapped, bool deep)
return 0;
}

-int dax_manager::map_ll(uint64_t nodeid, uint64_t file_handle, chunk nchunks,
- chunk fstart, chunk mstart)
+int dax_window_impl::map(uint64_t nodeid, uint64_t fh, uint64_t len,
+ uint64_t fstart, uint64_t mstart)
{
- assert(mstart + nchunks <= _window_chunks);
+ assert(mstart + len <= _window->len);

// NOTE: There are restrictions on the arguments to FUSE_SETUPMAPPING, from
// the spec: "Alignment constraints for FUSE_SETUPMAPPING and
@@ -177,18 +183,18 @@ int dax_manager::map_ll(uint64_t nodeid, uint64_t file_handle, chunk nchunks,
// - moffset: multiple of map_alignment from FUSE_INIT
// In practice, map_alignment is the host's page size, because foffset and
// moffset are passed to mmap() on the host. These are satisfied by
- // _chunk_size being a multiple of map_alignment.
+ // the caller (chunk size being a multiple of map alignment in dax_manager).

std::unique_ptr<fuse_setupmapping_in> in_args {
new (std::nothrow) fuse_setupmapping_in()};
if (!in_args) {
return ENOMEM;
}
- in_args->fh = file_handle;
- in_args->foffset = fstart * _chunk_size;
- in_args->len = nchunks * _chunk_size;
+ in_args->fh = fh;
+ in_args->foffset = fstart;
+ in_args->len = len;
in_args->flags = 0; // Read-only
- in_args->moffset = mstart * _chunk_size;
+ in_args->moffset = mstart;

virtiofs_debug("inode %lld, setting up mapping (foffset=%lld, len=%lld, "
"moffset=%lld)\n", nodeid, in_args->foffset, in_args->len,
@@ -203,9 +209,9 @@ int dax_manager::map_ll(uint64_t nodeid, uint64_t file_handle, chunk nchunks,
return 0;
}

-int dax_manager::unmap_ll(chunk nchunks, chunk mstart)
+int dax_window_impl::unmap(uint64_t len, uint64_t mstart)
{
- assert(mstart + nchunks <= _window_chunks);
+ assert(mstart + len <= _window->len);

// NOTE: FUSE_REMOVEMAPPING accepts a fuse_removemapping_in followed by
// fuse_removemapping_in.count fuse_removemapping_one arguments in general.
@@ -219,8 +225,8 @@ int dax_manager::unmap_ll(chunk nchunks, chunk mstart)
auto r_one = new (in_args.get() + sizeof(fuse_removemapping_in))
fuse_removemapping_one();
r_in->count = 1;
- r_one->moffset = mstart * _chunk_size;
- r_one->len = nchunks * _chunk_size;
+ r_one->moffset = mstart;
+ r_one->len = len;

// The nodeid is irrelevant for the current implementation of
// FUSE_REMOVEMAPPING. If it needed to be set, would we need to make a
@@ -239,7 +245,9 @@ int dax_manager::unmap_ll(chunk nchunks, chunk mstart)
return 0;
}

-bool dax_manager::find(uint64_t nodeid, chunk fstart, mapping_part& found) const
+template<typename W>
+bool dax_manager<W>::find(uint64_t nodeid, chunk fstart, mapping_part& found)
+ const
{
for (auto& m : _mappings) {
if (m.nodeid == nodeid &&
@@ -256,7 +264,8 @@ bool dax_manager::find(uint64_t nodeid, chunk fstart, mapping_part& found) const
return false;
}

-dax_manager::chunk dax_manager::first_empty() const
+template<typename W>
+typename dax_manager<W>::chunk dax_manager<W>::first_empty() const
{
if (_mappings.empty()) {
return 0;
@@ -265,4 +274,8 @@ dax_manager::chunk dax_manager::first_empty() const
return m.mstart + m.nchunks;
}

+// Explicitly instantiate the only uses of dax_manager.
+template class dax_manager<dax_window_impl>;
+template class dax_manager<dax_window_stub>;
+
}
diff --git a/fs/virtiofs/virtiofs_dax.hh b/fs/virtiofs/virtiofs_dax.hh
index 2b9fa341..62194e24 100644
--- a/fs/virtiofs/virtiofs_dax.hh
+++ b/fs/virtiofs/virtiofs_dax.hh
@@ -5,6 +5,9 @@
* BSD license as described in the LICENSE file in the top-level directory.
*/

+#ifndef VIRTIOFS_DAX_HH
+#define VIRTIOFS_DAX_HH
+
#include <vector>

#include <api/assert.h>
@@ -12,10 +15,61 @@
#include <osv/uio.h>

#include "drivers/virtio-fs.hh"
-#include "virtiofs.hh"
+
+// Necessary pre-declaration because virtiofs_inode is declared in virtiofs.hh,
+// which depends on this for defining virtiofs_mount_data.
+struct virtiofs_inode;

namespace virtiofs {

+// A thin abstraction layer over the actual DAX window, taking care of all the
+// low-level operations, interfacing with the driver.
+class dax_window_impl {
+public:
+ // Construct a new window for the DAX window associated with @drv (as
+ // returned by drv.get_dax()).
+ dax_window_impl(virtio::fs& drv)
+ : _drv {drv},
+ _window {drv.get_dax()} {}
+
+ // Returns the size of the window in bytes.
+ u64 size() const { return _window->len; }
+ // Returns a pointer to the underlying memory region.
+ mmioaddr_t data() const { return _window->addr; }
+ // Returns the map alignment for the window.
+ int map_alignment() const { return _drv.get_map_alignment(); }
+ // Map @len bytes of the file with @nodeid (opened as @fh), starting at
+ // byte @fstart of the file and byte @mstart of the window. Returns
+ // non-zero on failure.
+ int map(uint64_t nodeid, uint64_t fh, uint64_t len, uint64_t fstart,
+ uint64_t mstart);
+ // Unmap @len bytes, starting at byte @mstart of the window. Returns
+ // non-zero on failure.
+ int unmap(uint64_t len, uint64_t mstart);
+
+private:
+ virtio::fs& _drv;
+ const virtio::fs::dax_window* const _window;
+};
+
+// A stub DAX window, used for testing. Defined here to facilitate instantiation
+// of dax_manager<dax_window_stub> (see virtiofs_dax.cc).
+class dax_window_stub {
+public:
+ dax_window_stub(u64 len)
+ : _len {len} {}
+
+ u64 size() const { return _len; }
+ mmioaddr_t data() const { return nullptr; }
+ int map_alignment() const { return 12; /* 4KiB */ }
+ int map(uint64_t nodeid, uint64_t fh, uint64_t len, uint64_t fstart,
+ uint64_t mstart) { return 0; };
+ int unmap(uint64_t len, uint64_t mstart) { return 0; };
+
+private:
+ u64 _len;
+};
+
// A manager for the DAX window of a virtio-fs device. This implements a
// straight-forward scheme for file mappings:
// - The window is split into equally-sized chunks. Each mapping occupies an
@@ -24,21 +78,19 @@ namespace virtiofs {
// - When there are not enough chunks available for a new mapping, the highest
// (i.e. most recently mapped) chunks occupied are evicted. Thus, chunks are
// mapped in a LIFO manner (the window resembles a stack).
+template<typename W>
class dax_manager {
public:
static constexpr size_t DEFAULT_CHUNK_SIZE = 1 << 21; // 2MiB

- // Construct a new manager for the DAX window associated with @drv (as
- // returned by drv.get_dax()). The alignment constraint of the device (as
- // reported by drv.get_map_alignment()) should be compatible with
- // @chunk_size.
- dax_manager(virtio::fs& drv, size_t chunk_size = DEFAULT_CHUNK_SIZE)
- : _drv {drv},
- _window {drv.get_dax()},
+ // Construct a new manager for @window. The @chunk_size should be compatible
+ // with the alignment constraint of @window.
+ dax_manager(W window, size_t chunk_size = DEFAULT_CHUNK_SIZE)
+ : _window {window},
_chunk_size {chunk_size},
- _window_chunks {_window->len / _chunk_size} {
+ _window_chunks {_window.size() / _chunk_size} {

- assert(_chunk_size % (1ull << _drv.get_map_alignment()) == 0);
+ assert(_chunk_size % (1ull << _window.map_alignment()) == 0);

// NOTE: If _window->len % CHUNK_SIZE > 0, that remainder (< CHUNK_SIZE)
// is effectively ignored.
@@ -49,7 +101,7 @@ public:
int read(virtiofs_inode& inode, uint64_t file_handle, u64 read_amt,
struct uio& uio, bool aggressive = false);

-private:
+protected:
// Helper type to better distinguish referring to chunks vs bytes
using chunk = size_t;

@@ -80,14 +132,6 @@ private:
// if @deep. Returns in @unmapped what was unmapped and non-zero on failure.
// Called with _lock held (for writing).
int unmap(chunk nchunks, mapping_part& unmapped, bool deep = false);
- // Map @nchunks chunks of the file with @nodeid (opened as @fh), starting at
- // chunk @fstart of the file and chunk @mstart of the window. Returns
- // non-zero on failure. Called with _lock held (for writing).
- int map_ll(uint64_t nodeid, uint64_t fh, chunk nchunks, chunk fstart,
- chunk mstart);
- // Unmap @nchunks chunks, starting at chunk @mstart of the window. Returns
- // non-zero on failure. Called with _lock held (for writing).
- int unmap_ll(chunk nchunks, chunk mstart);

// Return in @found the largest contiguous existing mapping for @nodeid
// starting at @fstart. If none found, returns false. Called with _lock held
@@ -97,8 +141,7 @@ private:
// window is full. Called with _lock held (for reading).
chunk first_empty() const;

- virtio::fs& _drv;
- const virtio::fs::dax_window* const _window;
+ W _window;
const size_t _chunk_size;
const chunk _window_chunks;
// TODO OPT: Switch to rwlock
@@ -106,4 +149,9 @@ private:
std::vector<mapping> _mappings;
};

+using dax_manager_impl = dax_manager<dax_window_impl>;
+using dax_manager_stub = dax_manager<dax_window_stub>;
+
}
+
+#endif
diff --git a/fs/virtiofs/virtiofs_vfsops.cc b/fs/virtiofs/virtiofs_vfsops.cc
index d44e160d..32572d7f 100644
--- a/fs/virtiofs/virtiofs_vfsops.cc
+++ b/fs/virtiofs/virtiofs_vfsops.cc
@@ -28,7 +28,7 @@ using fuse_request = virtio::fs::fuse_request;
static std::atomic<uint64_t> fuse_unique_id(1);

static struct {
- std::unordered_map<virtio::fs*, std::shared_ptr<virtiofs::dax_manager>,
+ std::unordered_map<virtio::fs*, std::shared_ptr<virtiofs::dax_manager_impl>,
virtio::fs::hasher> mgrs;
mutex lock;
} dax_managers;
@@ -151,7 +151,8 @@ static int virtiofs_mount(struct mount* mp, const char* dev, int flags,
// device is already mounted)
m_data->dax_mgr = found->second;
} else {
- m_data->dax_mgr = std::make_shared<virtiofs::dax_manager>(*drv);
+ auto w {virtiofs::dax_window_impl(*drv)};
+ m_data->dax_mgr = std::make_shared<virtiofs::dax_manager_impl>(w);
if (!m_data->dax_mgr) {
return ENOMEM;
}
--
2.30.1

[Fotis Xenakis]

Signed-off-by: Fotis Xenakis <fo...@windowslive.com>
---

modules/tests/Makefile | 6 +-
tests/tst-dax.cc | 333 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 336 insertions(+), 3 deletions(-)
create mode 100644 tests/tst-dax.cc

diff --git a/modules/tests/Makefile b/modules/tests/Makefile
index 016b72bd..c3e2fd8d 100644
--- a/modules/tests/Makefile
+++ b/modules/tests/Makefile
@@ -175,7 +175,7 @@ common-boost-tests := tst-vfs.so tst-libc-locking.so misc-fs-stress.so \
tst-bsd-tcp1-zsndrcv.so tst-async.so tst-rcu-list.so tst-tcp-listen.so \
tst-poll.so tst-bitset-iter.so tst-timer-set.so tst-clock.so \
tst-rcu-hashtable.so tst-unordered-ring-mpsc.so \
- tst-seek.so tst-ctype.so tst-wctype.so tst-string.so tst-time.so
+ tst-seek.so tst-ctype.so tst-wctype.so tst-string.so tst-time.so tst-dax.so

boost-tests := $(common-boost-tests)

@@ -197,7 +197,7 @@ solaris-tests := tst-solaris-taskq.so
# FIXME: two of the test below can't compile now because of include path
# (BSD and OSv header files get mixed up, etc.).
#zfs-tests := misc-zfs-disk.so misc-zfs-io.so misc-zfs-arc.so
-zfs-tests := misc-zfs-io.so
+zfs-tests := misc-zfs-io.so
solaris-tests += $(zfs-tests)

tests += $(solaris-tests)
@@ -205,7 +205,7 @@ tests += $(solaris-tests)
$(zfs-tests:%=$(out)/tests/%): COMMON+= \
-DBUILDING_ZFS \
-I$(src)/bsd/sys/cddl/contrib/opensolaris/uts/common/fs/zfs \
- -I$(src)/bsd/sys/cddl/contrib/opensolaris/common/zfs
+ -I$(src)/bsd/sys/cddl/contrib/opensolaris/common/zfs

$(solaris-tests:%=$(out)/tests/%): COMMON+= \
-Wno-strict-aliasing \
diff --git a/tests/tst-dax.cc b/tests/tst-dax.cc
new file mode 100644
index 00000000..c99cdd2f
--- /dev/null
+++ b/tests/tst-dax.cc
@@ -0,0 +1,333 @@
+/*
+ * Copyright (C) 2021 Fotis Xenakis
+ *
+ * This work is open source software, licensed under the terms of the
+ * BSD license as described in the LICENSE file in the top-level directory.
+ */
+
+#define BOOST_TEST_MODULE tst-dax
+
+#include <boost/test/unit_test.hpp>
+
+#include <fs/virtiofs/virtiofs_dax.hh>
+
+using virtiofs::dax_manager_stub;
+using virtiofs::dax_window_stub;
+
+static constexpr uint64_t nodeid = 1;
+static constexpr uint64_t file_handle = 1;
+
+// A stub DAX window with a 10-chunk capacity used in all test cases.
+static constexpr u64 window_chunks = 10;
+static constexpr u64 window_len = \
+ window_chunks * dax_manager_stub::DEFAULT_CHUNK_SIZE;
+static auto window = dax_window_stub(window_len);
+
+// A dax_manager with a stub underlying DAX window. Used as a test case fixture.
+class dax_manager_test : public dax_manager_stub {
+public:
+ dax_manager_test()
+ : dax_manager_stub {window} {}
+};
+
+// Tests on an empty window
+BOOST_FIXTURE_TEST_SUITE(empty_window_tests, dax_manager_test)
+
+ BOOST_AUTO_TEST_CASE(first_empty_empty)
+ {
+ BOOST_TEST(first_empty() == 0);
+ }
+
+ BOOST_AUTO_TEST_CASE(map_empty)
+ {
+ mapping_part mp {0, 0};
+
+ BOOST_TEST(map(nodeid, file_handle, 3, 0, mp, false) == 0);
+ BOOST_TEST(mp.mstart == 0);
+ BOOST_TEST(mp.nchunks == 3);
+ BOOST_TEST(_mappings.size() == 1);
+ }
+
+ BOOST_AUTO_TEST_CASE(unmap_empty)
+ {
+ mapping_part mp {0, 0};
+
+ BOOST_TEST(unmap(1, mp) != 0);
+ BOOST_TEST(mp.mstart == 0);
+ BOOST_TEST(mp.nchunks == 0);
+ BOOST_TEST(_mappings.empty());
+ }
+
+ BOOST_AUTO_TEST_CASE(map_multiple_no_coalesce)
+ {
+ mapping_part mp {0, 0};
+
+ BOOST_TEST_REQUIRE(map(nodeid, file_handle, 3, 0, mp, false) == 0);
+
+ BOOST_TEST(map(nodeid, file_handle, 2, 5, mp, false) == 0);
+ BOOST_TEST(mp.mstart == 3);
+ BOOST_TEST(mp.nchunks == 2);
+ BOOST_TEST(_mappings.size() == 2);
+ }
+
+ BOOST_AUTO_TEST_CASE(find_absent)
+ {
+ mapping_part mp {0, 0};
+
+ BOOST_TEST(!find(nodeid + 1, 0, mp));
+ BOOST_TEST(mp.mstart == 0);
+ BOOST_TEST(mp.nchunks == 0);
+ BOOST_TEST(!find(nodeid, 10, mp));
+ BOOST_TEST(mp.mstart == 0);
+ BOOST_TEST(mp.nchunks == 0);
+ }
+
+BOOST_AUTO_TEST_SUITE_END()
+
+// A pre-populated dax_manager_test. Used as a test case fixture.
+class dax_manager_test_populated : public dax_manager_test {
+public:
+ void setup() {
+ mapping_part mp {0, 0};
+
+ BOOST_TEST_REQUIRE(map(nodeid, file_handle, 3, 0, mp, false) == 0);
+ BOOST_TEST_REQUIRE(map(nodeid, file_handle, 2, 5, mp, false) == 0);
+ // At this point the window's state is:
+ // | f[0] | f[1] | f[2] | f[5] | f[6] | empty | empty |...
+ // | mapping #1 | mapping #2 |
+ }
+};
+
+// Tests on a pre-populated window
+BOOST_FIXTURE_TEST_SUITE(populated_window_tests, dax_manager_test_populated)
+
+ BOOST_AUTO_TEST_CASE(first_empty_populated)
+ {
+ BOOST_TEST(first_empty() == 5);
+ }
+
+ BOOST_AUTO_TEST_CASE(map_coalesce)
+ {
+ mapping_part mp {0, 0};
+
+ BOOST_TEST(map(nodeid, file_handle, 3, 7, mp, false) == 0);
+ BOOST_TEST(mp.mstart == 5);
+ BOOST_TEST(mp.nchunks == 3);
+ BOOST_TEST(_mappings.size() == 2);
+ }
+
+ BOOST_AUTO_TEST_CASE(map_stable)
+ {
+ mapping_part mp {0, 0};
+
+ BOOST_TEST(map(nodeid, file_handle, 0, 0, mp, false) == 0);
+ BOOST_TEST(mp.mstart == 5);
+ BOOST_TEST(mp.nchunks == 0);
+ BOOST_TEST(_mappings.size() == 2);
+ }
+
+ BOOST_AUTO_TEST_CASE(unmap_stable)
+ {
+ mapping_part mp {0, 0};
+
+ BOOST_TEST(unmap(0, mp) == 0);
+ BOOST_TEST(mp.mstart == 5);
+ BOOST_TEST(mp.nchunks == 0);
+ BOOST_TEST(_mappings.size() == 2);
+ }
+
+ BOOST_AUTO_TEST_CASE(unmap_one_whole)
+ {
+ mapping_part mp {0, 0};
+
+ BOOST_TEST(unmap(2, mp) == 0);
+ BOOST_TEST(mp.mstart == 3);
+ BOOST_TEST(mp.nchunks == 2);
+ BOOST_TEST(_mappings.size() == 1);
+ }
+
+ BOOST_AUTO_TEST_CASE(unmap_one_part)
+ {
+ mapping_part mp {0, 0};
+
+ BOOST_TEST(unmap(1, mp) == 0);
+ BOOST_TEST(mp.mstart == 4);
+ BOOST_TEST(mp.nchunks == 1);
+ BOOST_TEST(_mappings.size() == 2);
+ }
+
+ BOOST_AUTO_TEST_CASE(unmap_multiple_part)
+ {
+ mapping_part mp {0, 0};
+
+ BOOST_TEST(unmap(3, mp) == 0);
+ BOOST_TEST(mp.mstart == 2);
+ BOOST_TEST(mp.nchunks == 3);
+ BOOST_TEST(_mappings.size() == 1);
+ }
+
+ BOOST_AUTO_TEST_CASE(unmap_all)
+ {
+ mapping_part mp {0, 0};
+
+ BOOST_TEST(unmap(5, mp) == 0);
+ BOOST_TEST(mp.mstart == 0);
+ BOOST_TEST(mp.nchunks == 5);
+ BOOST_TEST(_mappings.empty());
+ }
+
+ BOOST_AUTO_TEST_CASE(unmap_excess)
+ {
+ mapping_part mp {0, 0};
+
+ BOOST_TEST(unmap(6, mp) == 0);
+ BOOST_TEST(mp.mstart == 0);
+ BOOST_TEST(mp.nchunks == 5);
+ BOOST_TEST(_mappings.empty());
+ }
+
+ BOOST_AUTO_TEST_CASE(map_evict_part)
+ {
+ mapping_part mp {0, 0};
+
+ BOOST_TEST(map(nodeid, file_handle, 6, 0, mp, true) == 0);
+ BOOST_TEST(mp.mstart == 4);
+ BOOST_TEST(mp.nchunks == 6);
+ BOOST_TEST(_mappings.size() == 3);
+ }
+
+ BOOST_AUTO_TEST_CASE(map_evict_multiple_part)
+ {
+ mapping_part mp {0, 0};
+
+ BOOST_TEST(map(nodeid, file_handle, 9, 0, mp, true) == 0);
+ BOOST_TEST(mp.mstart == 1);
+ BOOST_TEST(mp.nchunks == 9);
+ BOOST_TEST(_mappings.size() == 2);
+ }
+
+ BOOST_AUTO_TEST_CASE(map_evict_all)
+ {
+ mapping_part mp {0, 0};
+
+ BOOST_TEST(map(nodeid, file_handle, 10, 0, mp, true) == 0);
+ BOOST_TEST(mp.mstart == 0);
+ BOOST_TEST(mp.nchunks == 10);
+ BOOST_TEST(_mappings.size() == 1);
+ }
+
+ BOOST_AUTO_TEST_CASE(find_absent)
+ {
+ mapping_part mp {0, 0};
+
+ BOOST_TEST(!find(nodeid + 1, 0, mp));
+ BOOST_TEST(mp.mstart == 0);
+ BOOST_TEST(mp.nchunks == 0);
+ BOOST_TEST(!find(nodeid, 10, mp));
+ BOOST_TEST(mp.mstart == 0);
+ BOOST_TEST(mp.nchunks == 0);
+ }
+
+ BOOST_AUTO_TEST_CASE(find_present)
+ {
+ mapping_part mp {0, 0};
+
+ BOOST_TEST(find(nodeid, 0, mp));
+ BOOST_TEST(mp.mstart == 0);
+ BOOST_TEST(mp.nchunks == 3);
+
+ BOOST_TEST(find(nodeid, 1, mp));
+ BOOST_TEST(mp.mstart == 1);
+ BOOST_TEST(mp.nchunks == 2);
+
+ BOOST_TEST(find(nodeid, 5, mp));
+ BOOST_TEST(mp.mstart == 3);
+ BOOST_TEST(mp.nchunks == 2);
+
+ BOOST_TEST(find(nodeid, 6, mp));
+ BOOST_TEST(mp.mstart == 4);
+ BOOST_TEST(mp.nchunks == 1);
+ }
+
+BOOST_AUTO_TEST_SUITE_END()
+
+// A full dax_manager_test. Used as a test case fixture.
+class dax_manager_test_full : public dax_manager_test {
+public:
+ void setup() {
+ mapping_part mp {0, 0};
+
+ BOOST_TEST_REQUIRE(map(nodeid, file_handle, 3, 0, mp, false) == 0);
+ BOOST_TEST_REQUIRE(map(nodeid, file_handle, 2, 5, mp, false) == 0);
+ BOOST_TEST_REQUIRE(map(nodeid, file_handle, 5, 10, mp, false) == 0);
+ // At this point the window's state is:
+ // | f[0] | f[1] | f[2] | f[5] | f[6] | f[10] | f[11] | f[12] | f[13] | f[14] |
+ // | mapping #1 | mapping #2 | mapping #3 |
+ }
+};
+
+BOOST_FIXTURE_TEST_SUITE(full_window_tests, dax_manager_test_full)
+
+ BOOST_AUTO_TEST_CASE(first_empty_full)
+ {
+ BOOST_TEST(first_empty() == window_chunks);
+ }
+
+ BOOST_AUTO_TEST_CASE(map_full)
+ {
+ mapping_part mp {0, 0};
+
+ BOOST_TEST(map(nodeid, file_handle, 1, 10, mp, false) != 0);
+ BOOST_TEST(mp.mstart == 10);
+ BOOST_TEST(mp.nchunks == 0);
+ BOOST_TEST(_mappings.size() == 3);
+ }
+
+ BOOST_AUTO_TEST_CASE(map_evict_full)
+ {
+ mapping_part mp {0, 0};
+
+ BOOST_TEST(map(nodeid, file_handle, 1, 0, mp, true) == 0);
+ BOOST_TEST(mp.mstart == 9);
+ BOOST_TEST(mp.nchunks == 1);
+ BOOST_TEST(_mappings.size() == 4);
+ }
+
+ BOOST_AUTO_TEST_CASE(find_absent)
+ {
+ mapping_part mp {0, 0};
+
+ BOOST_TEST(!find(nodeid + 1, 0, mp));
+ BOOST_TEST(mp.mstart == 0);
+ BOOST_TEST(mp.nchunks == 0);
+ BOOST_TEST(!find(nodeid, 20, mp));
+ BOOST_TEST(mp.mstart == 0);
+ BOOST_TEST(mp.nchunks == 0);
+ }
+
+ BOOST_AUTO_TEST_CASE(find_present)
+ {
+ mapping_part mp {0, 0};
+
+ BOOST_TEST(find(nodeid, 0, mp));
+ BOOST_TEST(mp.mstart == 0);
+ BOOST_TEST(mp.nchunks == 3);
+
+ BOOST_TEST(find(nodeid, 1, mp));
+ BOOST_TEST(mp.mstart == 1);
+ BOOST_TEST(mp.nchunks == 2);
+
+ BOOST_TEST(find(nodeid, 5, mp));
+ BOOST_TEST(mp.mstart == 3);
+ BOOST_TEST(mp.nchunks == 2);
+
+ BOOST_TEST(find(nodeid, 6, mp));
+ BOOST_TEST(mp.mstart == 4);
+ BOOST_TEST(mp.nchunks == 1);
+
+ BOOST_TEST(find(nodeid, 11, mp));
+ BOOST_TEST(mp.mstart == 6);
+ BOOST_TEST(mp.nchunks == 4);
+ }
+
+BOOST_AUTO_TEST_SUITE_END()
--
2.30.1

[Waldek Kozaczuk]

Thanks for the patch. Let me find some time to properly review the changes. It is highly valuable to have some automated tests of DAX window functionality.

How confident are you that your changes to the fs/virtiofs/* to make it testable with stubs do not break anything?

[Fotis Xenakis]

On Wednesday, March 10, 2021 at 12:57:01 AM UTC+2 jwkoz...@gmail.com wrote:

Thanks for the patch. Let me find some time to properly review the changes. It is highly valuable to have some automated tests of DAX window functionality.

How confident are you that your changes to the fs/virtiofs/* to make it testable with stubs do not break anything?

The changes actually touching the manager logic are minimal: mostly the change from dax_manager::{un}map_ll() accepting chunk numbers to dax_window_imp::{un}map() accepting bytes.

In addition, testing with the CLI app did not result in any surprises.

Would it possibly be helpful for me to try breaking down the first patch into smaller, more manageable ones?

[Waldek Kozaczuk]

No. I think it looks good. I have tested it as well and things still work.

[Commit Bot]

From: Fotis Xenakis <fo...@windowslive.com>
Committer: Waldemar Kozaczuk <jwkoz...@gmail.com>
Branch: master

virtio-fs: make DAX manager testable

This mainly includes changes allowing the actual DAX window operations
to be replaced with stubs. To achieve that we:
1. Introduce a new class virtiofs::dax_window_impl, encapsulating the
DAX window and its operations (previously those were members of
dax_manager).
2. Introduce a stub version of the DAX window,
virtiofs::dax_window_stub.
3. Turn the dax_manager class into a template, with the DAX window
flavour (normal or stub) as its parameter. This was deemed cleaner
than going with run-time polymorphism.
4. Make all previously private members of dax_manager protected,
allowing them to be accessed by the test code.

Signed-off-by: Fotis Xenakis <fo...@windowslive.com>

Message-Id: <VI1PR03MB3773E191D8...@VI1PR03MB3773.eurprd03.prod.outlook.com>

---
diff --git a/fs/virtiofs/virtiofs.hh b/fs/virtiofs/virtiofs.hh

--- a/fs/virtiofs/virtiofs.hh
+++ b/fs/virtiofs/virtiofs.hh
@@ -16,6 +16,7 @@

#include "drivers/virtio-fs.hh"
#include "fuse_kernel.h"
+#include "virtiofs_dax.hh"

#define VIRTIOFS_DEBUG_ENABLED 1

@@ -25,15 +26,9 @@
#define virtiofs_debug(...)
#endif

-// Necessary pre-declaration because virtiofs::dax depends on virtiofs_inode,
-// declared below.
-namespace virtiofs {
-class dax_manager;
-}
-
struct virtiofs_mount_data {
virtio::fs* drv;
- std::shared_ptr<virtiofs::dax_manager> dax_mgr;
+ std::shared_ptr<virtiofs::dax_manager_impl> dax_mgr;
};

struct virtiofs_inode {
diff --git a/fs/virtiofs/virtiofs_dax.cc b/fs/virtiofs/virtiofs_dax.cc

--- a/fs/virtiofs/virtiofs_dax.cc
+++ b/fs/virtiofs/virtiofs_dax.cc
@@ -8,6 +8,7 @@
#include <algorithm>
#include <mutex>

+#include <api/assert.h>
#include <osv/debug.h>
#include <osv/uio.h>

@@ -18,8 +19,9 @@

namespace virtiofs {

-int dax_manager::read(virtiofs_inode& inode, uint64_t file_handle, u64 read_amt,
- struct uio& uio, bool aggressive)
+template<typename W>
+int dax_manager<W>::read(virtiofs_inode& inode, uint64_t file_handle,
+ u64 read_amt, struct uio& uio, bool aggressive)
{
std::lock_guard<mutex> guard {_lock};

@@ -63,7 +65,7 @@ int dax_manager::read(virtiofs_inode& inode, uint64_t file_handle, u64 read_amt,
}

out:
- auto req_data = _window->addr + (mp.mstart * _chunk_size) + coffset;
+ auto req_data = _window.data() + (mp.mstart * _chunk_size) + coffset;
auto read_amt_act = std::min<size_t>(read_amt,
(mp.nchunks * _chunk_size) - coffset);
// NOTE: It shouldn't be necessary to use the mmio* interface (i.e. volatile

@@ -76,7 +78,8 @@ int dax_manager::read(virtiofs_inode& inode, uint64_t file_handle, u64 read_amt,

--- a/fs/virtiofs/virtiofs_dax.hh
+++ b/fs/virtiofs/virtiofs_dax.hh
@@ -5,17 +5,71 @@

* BSD license as described in the LICENSE file in the top-level directory.
*/

+#ifndef VIRTIOFS_DAX_HH
+#define VIRTIOFS_DAX_HH
+
#include <vector>

#include <api/assert.h>

#include <osv/mutex.h>

@@ -97,13 +141,17 @@ private:

// window is full. Called with _lock held (for reading).
chunk first_empty() const;

- virtio::fs& _drv;
- const virtio::fs::dax_window* const _window;
+ W _window;
const size_t _chunk_size;
const chunk _window_chunks;
// TODO OPT: Switch to rwlock

mutex _lock;

std::vector<mapping> _mappings;
};

+using dax_manager_impl = dax_manager<dax_window_impl>;
+using dax_manager_stub = dax_manager<dax_window_stub>;
+
}
+
+#endif
diff --git a/fs/virtiofs/virtiofs_vfsops.cc b/fs/virtiofs/virtiofs_vfsops.cc

[Commit Bot]

From: Fotis Xenakis <fo...@windowslive.com>
Committer: Waldemar Kozaczuk <jwkoz...@gmail.com>
Branch: master

tests: add DAX manager white-box unit tests

Signed-off-by: Fotis Xenakis <fo...@windowslive.com>
Message-Id: <VI1PR03MB377366B13B...@VI1PR03MB3773.eurprd03.prod.outlook.com>

---
diff --git a/modules/tests/Makefile b/modules/tests/Makefile

--- a/modules/tests/Makefile
+++ b/modules/tests/Makefile
@@ -175,7 +175,7 @@ common-boost-tests := tst-vfs.so tst-libc-locking.so misc-fs-stress.so \
tst-bsd-tcp1-zsndrcv.so tst-async.so tst-rcu-list.so tst-tcp-listen.so \
tst-poll.so tst-bitset-iter.so tst-timer-set.so tst-clock.so \
tst-rcu-hashtable.so tst-unordered-ring-mpsc.so \
- tst-seek.so tst-ctype.so tst-wctype.so tst-string.so tst-time.so
+ tst-seek.so tst-ctype.so tst-wctype.so tst-string.so tst-time.so tst-dax.so

boost-tests := $(common-boost-tests)

@@ -197,15 +197,15 @@ solaris-tests := tst-solaris-taskq.so

# FIXME: two of the test below can't compile now because of include path
# (BSD and OSv header files get mixed up, etc.).
#zfs-tests := misc-zfs-disk.so misc-zfs-io.so misc-zfs-arc.so
-zfs-tests := misc-zfs-io.so
+zfs-tests := misc-zfs-io.so
solaris-tests += $(zfs-tests)

tests += $(solaris-tests)

$(zfs-tests:%=$(out)/tests/%): COMMON+= \
-DBUILDING_ZFS \
-I$(src)/bsd/sys/cddl/contrib/opensolaris/uts/common/fs/zfs \
- -I$(src)/bsd/sys/cddl/contrib/opensolaris/common/zfs
+ -I$(src)/bsd/sys/cddl/contrib/opensolaris/common/zfs

$(solaris-tests:%=$(out)/tests/%): COMMON+= \
-Wno-strict-aliasing \
diff --git a/tests/tst-dax.cc b/tests/tst-dax.cc

--- a/tests/tst-dax.cc