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 <[email protected]>
Message-Id: <VI1PR03MB3773E191D8AA7D58FAEE8FBEA6959@VI1PR03MB3773.eurprd03.prod.outlook.com>

Author: foxeng

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 {

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,
     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>;
+
 }

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>
 #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,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

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;
             }