Disk Cache: Add more corruption tracking histograms.

net/disk_cache/rankings.cc

Index: net/disk_cache/rankings.cc
===================================================================
--- net/disk_cache/rankings.cc	(revision 133063)
+++ net/disk_cache/rankings.cc	(working copy)
@@ -724,14 +724,6 @@
   control_data_->operation = 0;
 }
 
-bool Rankings::CheckEntry(CacheRankingsBlock* rankings) {
-  if (rankings->VerifyHash())
-    return true;
-
-  // If this entry is not dirty, it is a serious problem.
-  return backend_->GetCurrentEntryId() != rankings->Data()->dirty;
-}
-
 bool Rankings::CheckLinks(CacheRankingsBlock* node, CacheRankingsBlock* prev,
                           CacheRankingsBlock* next, List* list) {
   CacheAddr node_addr = node->address().value();
@@ -787,44 +779,120 @@
 }
 
 int Rankings::CheckList(List list) {
-  Addr& my_head = heads_[list];
-  Addr& my_tail = tails_[list];
-  if (!my_head.is_initialized()) {
-    if (!my_tail.is_initialized())
-      return 0;
-    // If there is no head, having a tail is an error.
-    return ERR_INVALID_TAIL;
+  Addr last1, last2;
+  bool forward = true;
+  int num_items;
+  int rv = CheckListSection(list, last1, last2, forward,
+                            &last1, &last2, &num_items);
+  if (rv == 0)
+    return num_items;
+
+  Addr last3, last4;
+  int num_items2;
+  forward = false;

[gavinp, 2012/04/23 19:10:37]

forward = false is confusing.  You're right that I don't like bool params.  I
think this would be much more readable if in Rankings there was an enum {
CHECK_FORWARDS, CHECK_BACKWARDS };

[rvargas (doing something else), 2012/04/23 21:22:26]

why? What would be an appropriate name for a bool variable that says if the
direction to search is forward or not? is_forward ?
What I don't like about this (other than the nth redefinition of 0 and 1) is
that the enum has to be declared on the header file: noise for everybody instead
of being something confined to the cc file, that nobody will see unless they are
reading the one function that cares about it.

[gavinp, 2012/04/25 12:40:23]

is_forward is definitely a better name.

But I still think the enum is the way to go.  The language is already full of
ways to stay the same number two ways; that argument is unpersuasive.

I hear you about the header file; but I think the result will be that the
signature of CheckListSection is easier to read; and really, folks aren't trying
to read through the private section of an header file unless they wish to
understand a lot of private interface.

But why not move CheckList and CheckListSection into an anon namespace?  It
seems that CheckList only derefs this to call CheckListSection, and
CheckListSection defefs this for heads_ and tails_.  If you added those as
params, you could move these to an anon namespace, and the enum would be in this
file.

On 2012/04/23 21:22:26, rvargas wrote:

+  int rv2 = CheckListSection(list, last1, last2, forward,
+                             &last3, &last4, &num_items2);
+
+  if (!num_items && rv != ERR_INVALID_NEXT)
+    rv = ERR_INVALID_HEAD;
+
+  if (!num_items2 && rv2 != ERR_INVALID_NEXT)
+    rv2 = ERR_INVALID_HEAD;
+
+  int expected = control_data_->sizes[list];
+  int total_items = num_items + num_items2;
+
+  if (!count_lists_) {
+    // There's no expected value, so we'll use something else.
+    if (last3 == last1 || last3 == last2 || last4 == last1 || last4 == last2)
+      expected = total_items + 1;
   }
-  // If there is no tail, having a head is an error.
-  if (!my_tail.is_initialized())
-    return ERR_INVALID_HEAD;
 
-  if (my_tail.is_separate_file())
-    return ERR_INVALID_TAIL;
+  if (expected) {
+    // This histogram has an offset so that we can see small negative values. In
+    // practice, it is linear from -9 to +8.
+    UMA_HISTOGRAM_CUSTOM_COUNTS("DiskCache.LostItems",
+                                expected - total_items + 10, 0, 2000, 75);
+  }
 
-  if (my_head.is_separate_file())
+  const int kInvalidHead = 1;
+  const int kInvalidTail = 2;
+  const int kInvalidHeadAndTail = 3;
+  const int kOneInvalidEntry = 4;
+  const int kTwoInvalidEntries = 5;
+  const int kOneInvalidLink = 6;
+  const int kTwoInvalidLinks = 7;
+  const int kOneInvalidEntryOneInvalidLink = 8;
+
+  int error = list * 10;
+  if (rv == ERR_INVALID_HEAD && rv2 != ERR_INVALID_HEAD) {
+    error += kInvalidHead;
+  } else if (rv == ERR_INVALID_HEAD && rv2 == ERR_INVALID_HEAD) {
+    error += kInvalidHeadAndTail;
+  } else if (rv != ERR_INVALID_HEAD && rv2 == ERR_INVALID_HEAD) {
+    error += kInvalidTail;
+  } else if (rv == ERR_INVALID_ENTRY && rv2 == ERR_INVALID_ENTRY) {
+    error += kTwoInvalidEntries;
+  } else if (rv == ERR_INVALID_ENTRY && rv2 == 0) {
+    error += kOneInvalidEntry;
+  } else if (rv == ERR_INVALID_ENTRY || rv2 == ERR_INVALID_ENTRY) {
+    error += kOneInvalidEntryOneInvalidLink;
+  } else if (rv2 != 0) {
+    error += kTwoInvalidLinks;
+  } else {
+    error += kOneInvalidLink;
+  }
+  CACHE_UMA(CACHE_ERROR, "ListError", 0, error);
+
+  return rv;
+}
+
+// Note that the returned error codes assume a forward walk (from head to tail)
+// so they have to be adjusted accordingly by the caller. We use two stop values
+// to be able to detect a corrupt node at the end that is not linked going back.
+int Rankings::CheckListSection(List list, Addr stop1, Addr stop2, bool forward,

[rvargas (doing something else), 2012/04/19 21:45:09]

I know you don't like boolean arguments, but this is a private method, called
from a single method (and I'm overly explicit when making the call)

+                               Addr* last1, Addr* last2, int* num_items) {
+  Addr current = forward ? heads_[list] : tails_[list];
+  *last1 = *last2 = current;
+  *num_items = 0;
+  if (!current.is_initialized())
+    return 0;
+
+  if (!current.SanityCheckForRankings())
     return ERR_INVALID_HEAD;
 
-  int num_items = 0;
-  Addr address(my_head.value());
-  Addr prev(my_head.value());
   scoped_ptr<CacheRankingsBlock> node;
+  Addr prev_addr(current);
   do {
-    node.reset(new CacheRankingsBlock(backend_->File(address), address));
+    node.reset(new CacheRankingsBlock(backend_->File(current), current));
     node->Load();
-    if (node->Data()->prev != prev.value())
-      return ERR_INVALID_PREV;
-    if (!CheckEntry(node.get()))
+    if (!SanityCheck(node.get(), true))
       return ERR_INVALID_ENTRY;
 
-    prev.set_value(address.value());
-    address.set_value(node->Data()->next);
-    if (!address.is_initialized() || address.is_separate_file())
+    CacheAddr next = forward ? node->Data()->next : node->Data()->prev;
+    CacheAddr prev = forward ? node->Data()->prev : node->Data()->next;
+
+    if (prev != prev_addr.value())
+      return ERR_INVALID_PREV;
+
+    Addr next_addr(next);
+    if (!next_addr.SanityCheckForRankings())
       return ERR_INVALID_NEXT;
 
-    num_items++;
-  } while (node->address().value() != address.value());
-  return num_items;
+    prev_addr = current;
+    current = next_addr;
+    *last2 = *last1;
+    *last1 = current;
+    (*num_items)++;
+
+    if (next_addr == prev_addr) {
+      Addr last = forward ? tails_[list] : heads_[list];
+      if (next_addr == last)
+        return 0;
+      return ERR_INVALID_TAIL;
+    }
+  } while (current != stop1 && current != stop2);
+  return 0;
 }
 
 bool Rankings::IsHead(CacheAddr addr, List* list) const {