[chord] Problem with dhashcli

[Emil Sit]

On Sun, 10 February 2008 at 07:38 (+0100), Katarzyna Stefanowicz wrote:
> do you know Polish?

Mmm, tylko troche... :-)

> Result of running dbdump is the same (1 entry). Do you have any ideas 
> what may be wrong?

That was a great bug report, thanks.  The symptom was actually related
to a deeper bug that I've been introducing for the past few months; I
probably should actually run some test suites on the code more
regularly.  Can you try this patch to see if it fixes the problem?

# HG changeset patch
# User Emil Sit <sit at mit.edu>
# Date 1202699792 18000
# Node ID 8ad64ed53a98820b08d9ef865765f0b0312eac80
# Parent  83fb3e1279d84d7a7fa5f8dee05b6e9bece04518
Fix update writes to mutable objects.

Since expiration removes keys from the Merkle tree, even though
they may still be on disk, dbns::del would return ADB_NOTFOUND
erroneously to dhblock_{noauth,keyhash}_srv's delete calls during
an update.  The correct approach is to check for the object in
the metadatadb first, not in the Merkle tree.

Change c37026ba75f3 attempted to avoid duplicate writes by
checking that an object was not present in the metadata database before
allowing dbns::insert to complete.  While this works fine for
content-hash immutable objects, it is completely wrong for
noauth/keyhash mutable objects.  Only apply this check for
immutable dbns (i.e., !hasaux ()), and note the reason why in
a comment.

Found, in part, by Katarzyna Stefanowicz <kate_stefik at tlen.pl>

diff -r 83fb3e1279d8 -r 8ad64ed53a98 lsd/adbd.C
--- a/lsd/adbd.C	Thu Jan 31 22:37:14 2008 -0500
+++ b/lsd/adbd.C	Sun Feb 10 22:16:32 2008 -0500
@@ -404,12 +404,20 @@ dbns::insert (const chordID &key, DBT &d
   r = dbfe_txn_begin (dbe, &t);
   assert (r == 0);
 
-  adb_metadata_t oldmetadata;
-  r = get_metadata (key, oldmetadata, t);
-  if (r != DB_NOTFOUND) {
-    dbfe_txn_abort (dbe, t);
-    return r;
+  if (!hasaux ()) {
+    // Don't write duplicate immutable objects.
+    adb_metadata_t oldmetadata;
+    r = get_metadata (key, oldmetadata, t);
+    if (r != DB_NOTFOUND) {
+      dbfe_txn_abort (dbe, t);
+      return r;
+    }
   }
+  // For mutable objects, since the Merkle tree may not contain
+  // information about the current object on disk, we have no
+  // way of telling, short of reading the old data, if the new
+  // data is the same as the old data.  Rely on checking from
+  // the client to avoid dupes.
 
   r = update_metadata (true, data.size, exptime, t);
   if (r) {
@@ -468,7 +476,7 @@ dbns::insert (const chordID &key, DBT &d
   err = "metadatadb->put";
   r = metadatadb->put (metadatadb, t, &skey, &metadata, 0);
   if (r) {
-    assert (r != DB_KEYEXIST); // Already checked.
+    warner ("dbns::insert", "metadatadb put", r);
     ret = dbfe_txn_abort (dbe, t);
   } else {
     ret = dbfe_txn_commit (dbe, t);
@@ -560,20 +568,20 @@ dbns::del (const chordID &key, u_int32_t
   }
 
   char *err = "";
-  do {
+  err = "metadatadb->del";
+  r = metadatadb->del (metadatadb, t, &skey, 0);
+  if (!r) {
+    // Only attempt to update Merkle tree if object was present.
+    // Ignore any errors to Merkle tree updates since the Merkle
+    // key may have been removed by expiration.
     if (hasaux ()) {
       err = "mtree->remove aux";
-      r = mtree->remove (key, auxdata, t);
-      if (r) break;
+      (void) mtree->remove (key, auxdata, t);
     } else {
       err = "mtree->remove";
-      r = mtree->remove (key, t);
-      if (r) break;
+      (void) mtree->remove (key, t);
     }
-    err = "metadatadb->del";
-    r = metadatadb->del (metadatadb, t, &skey, 0);
-    if (r) break;
-  } while (0);
+  }
 
   int ret = 0;
   if (r) {



-- 
Emil Sit / MIT CSAIL PDOS / http://pdos.csail.mit.edu/chord/