fix I/O util for multiple devXX dirs on the same FS v0.5.0

The "watch" sidechannel command no longer gets confused in case
two "devXX" directories share the same filesystem (and thus
st_dev).  This case should be rare in production setups, but
happens frequently in testing.

Perl mogstored does not have this bug.

2 files changed, 115 insertions, 9 deletions

diff --git a/svc_dev.c b/svc_dev.c
index 56f9845..9c8307d 100644
--- a/svc_dev.c
+++ b/svc_dev.c
@@ -5,19 +5,89 @@
 #include "cmogstored.h"
 #include "compat_memstream.h"
 
-static size_t dev_hash(const void *x, size_t tablesize)
+/*
+ * maps multiple "devXXX" directories to the device.
+ * This is uncommon in real world deployments (multiple mogdevs sharing
+ * the same system device), but happens frequently in testing
+ */
+struct mog_devlist {
+        dev_t st_dev;
+        Hash_table *by_mogdevid;
+};
+
+static size_t devlist_hash(const void *x, size_t tablesize)
+{
+        const struct mog_devlist *devlist = x;
+
+        return devlist->st_dev % tablesize;
+}
+
+static bool devlist_cmp(const void *a, const void *b)
+{
+        const struct mog_devlist *devlist_a = a;
+        const struct mog_devlist *devlist_b = b;
+
+        return devlist_a->st_dev == devlist_b->st_dev;
+}
+
+static void devlist_free(void *x)
+{
+        struct mog_devlist *devlist = x;
+
+        hash_free(devlist->by_mogdevid);
+        free(devlist);
+}
+
+static size_t devid_hash(const void *x, size_t tablesize)
 {
         const struct mog_dev *dev = x;
 
-        return dev->st_dev % tablesize;
+        return dev->devid % tablesize;
 }
 
-static bool dev_cmp(const void *a, const void *b)
+static bool devid_cmp(const void *a, const void *b)
 {
         const struct mog_dev *dev_a = a;
         const struct mog_dev *dev_b = b;
 
-        return dev_a->st_dev == dev_b->st_dev;
+        return dev_a->devid == dev_b->devid;
+}
+
+static struct mog_devlist * mog_devlist_new(dev_t st_dev)
+{
+        struct mog_devlist *devlist = xmalloc(sizeof(struct mog_devlist));
+
+        devlist->st_dev = st_dev;
+        devlist->by_mogdevid = hash_initialize(7, NULL,
+                                               devid_hash, devid_cmp, free);
+
+        return devlist;
+}
+
+/* ensures svc has a devlist, this must be called with devstats_lock held */
+static struct mog_devlist * svc_devlist(struct mog_svc *svc, dev_t st_dev)
+{
+        struct mog_devlist *devlist;
+        struct mog_devlist finder;
+
+        assert(svc->by_st_dev && "by_st_dev unintialized in svc");
+
+        finder.st_dev = st_dev;
+        devlist = hash_lookup(svc->by_st_dev, &finder);
+
+        if (devlist == NULL) {
+                devlist = mog_devlist_new(st_dev);
+                switch (hash_insert_if_absent(svc->by_st_dev, devlist, NULL)) {
+                case 0:
+                        assert(0 && "race condition, devlist should insert "
+                               "without error");
+                        abort();
+                        break;
+                case 1: break; /* OK, inserted */
+                default: mog_oom(); /* -1 */
+                }
+        }
+        return devlist;
 }
 
 static void svc_init_dev_hash(struct mog_svc *svc)
@@ -26,8 +96,11 @@ static void svc_init_dev_hash(struct mog_svc *svc)
                 hash_clear(svc->by_st_dev);
                 return;
         }
-        svc->by_st_dev = hash_initialize(7, NULL, dev_hash, dev_cmp, free);
-        if (!svc->by_st_dev) mog_oom();
+
+        svc->by_st_dev = hash_initialize(7, NULL, devlist_hash,
+                                         devlist_cmp, devlist_free);
+        if (!svc->by_st_dev)
+                mog_oom();
 }
 
 static int svc_scandev(struct mog_svc *svc, size_t *nr, mog_scandev_cb cb)
@@ -46,6 +119,8 @@ static int svc_scandev(struct mog_svc *svc, size_t *nr, mog_scandev_cb cb)
                 char *end;
                 size_t len = strlen(ent->d_name);
                 struct mog_dev *dev;
+                struct mog_devlist *devlist;
+                Hash_table *devhash;
 
                 if (len <= 3) continue;
                 if (memcmp("dev", ent->d_name, 3) != 0) continue;
@@ -57,8 +132,11 @@ static int svc_scandev(struct mog_svc *svc, size_t *nr, mog_scandev_cb cb)
                 dev = mog_dev_new(svc, (uint32_t)mog_devid);
                 if (!dev) continue;
 
+                devlist = svc_devlist(svc, dev->st_dev);
+                devhash = devlist->by_mogdevid;
+
                 if (cb) rc |= cb(dev, svc); /* mog_dev_mkusage */
-                switch (hash_insert_if_absent(svc->by_st_dev, dev, NULL)) {
+                switch (hash_insert_if_absent(devhash, dev, NULL)) {
                 case 0:
                         free(dev);
                         break;
@@ -73,7 +151,7 @@ static int svc_scandev(struct mog_svc *svc, size_t *nr, mog_scandev_cb cb)
         return rc;
 }
 
-static bool write_devstats(void *entry, void *file)
+static bool write_dev_stats(void *entry, void *file)
 {
         struct mog_dev *dev = entry;
         FILE *fp = file;
@@ -87,6 +165,15 @@ static bool write_devstats(void *entry, void *file)
         return true;
 }
 
+static bool write_devlist_stats(void *entry, void *file)
+{
+        struct mog_devlist *devlist = entry;
+
+        hash_do_for_each(devlist->by_mogdevid, write_dev_stats, file);
+
+        return true;
+}
+
 /* updates per-svc device stats from the global mount list */
 static size_t devstats_stringify(struct mog_svc *svc, char **dst)
 {
@@ -98,7 +185,7 @@ static size_t devstats_stringify(struct mog_svc *svc, char **dst)
         fp = open_memstream(dst, &bytes);
         if (!fp) mog_oom();
 
-        hash_do_for_each(svc->by_st_dev, write_devstats, fp);
+        hash_do_for_each(svc->by_st_dev, write_devlist_stats, fp);
         fprintf(fp, ".\n");
 
         CHECK(int, 0, my_memstream_close(fp, dst, &bytes));
diff --git a/test/mgmt.rb b/test/mgmt.rb
index a7b6a1a..694547c 100644
--- a/test/mgmt.rb
+++ b/test/mgmt.rb
@@ -247,4 +247,23 @@ class TestMgmt < Test::Unit::TestCase
     assert_equal ".\n", @client.gets
   end if `which iostat 2>/dev/null`.chomp.size != 0 &&
          RUBY_PLATFORM !~ /kfreebsd-gnu/
+
+  def test_iostat_watch_multidir
+    Dir.mkdir("#@tmpdir/dev666")
+    Dir.mkdir("#@tmpdir/dev999")
+    @client.write "watch\n"
+
+    # wait for iostat to catch up
+    3.times { assert_kind_of String, @client.gets }
+    util = RUBY_PLATFORM =~ /linux/ ? %r{\d+\.\d\d} : %r{\d+(?:\.\d+)?}
+    lines = []
+    lines << @client.gets
+    lines << @client.gets
+    assert_match(/^(666|999)\t#{util}\n/, lines[0])
+    assert_match(/^(666|999)\t#{util}\n/, lines[1])
+    assert_not_equal(lines[0], lines[1])
+
+    assert_equal ".\n", @client.gets
+  end if `which iostat 2>/dev/null`.chomp.size != 0 &&
+         RUBY_PLATFORM !~ /kfreebsd-gnu/
 end