osd: some scrub fixes

Don't drop locks just yet; atm this leaves the dout() prefix
exposed to concurrent modifications of pg state.

Don't requeue for scrub if already scrubbing.

Fix missing object detection bugs.

diff --git a/src/osd/OSD.cc b/src/osd/OSD.cc
index 585a7011783aac33571776bd28cd3115d49fbe0e..f86db67529d25e919df10ece4d996d300e5ec605 100644 (file)
--- a/src/osd/OSD.cc
+++ b/src/osd/OSD.cc
@@ -1610,7 +1610,7 @@ void OSD::handle_scrub(MOSDScrub *m)
         p != pg_map.end();
         p++) {
       PG *pg = p->second;
-      if (pg->is_primary())
+      if (pg->is_primary() && !pg->is_scrubbing())
        scrub_wq.queue(pg);
     }
   } else {
@@ -1619,7 +1619,7 @@ void OSD::handle_scrub(MOSDScrub *m)
         p++)
       if (pg_map.count(*p)) {
        PG *pg = pg_map[*p];
-       if (pg->is_primary())
+       if (pg->is_primary() && !pg->is_scrubbing())
          scrub_wq.queue(pg);
       }
   }
@@ -2881,16 +2881,16 @@ void OSD::handle_pg_scrub(MOSDPGScrub *m)
     } else {
       if (pg->is_primary()) {
        if (pg->peer_scrub_map.count(from)) {
-         dout(10) << "handle_pg_scrub got peer osd" << from << " scrub map -- had it already" << dendl;
+         dout(10) << *pg << " already had osd" << from << " scrub map" << dendl;
        } else {
-         dout(10) << "handle_pg_scrub got peer osd" << from << " scrub map" << dendl;
+         dout(10) << *pg << " got osd" << from << " scrub map" << dendl;
          bufferlist::iterator p = m->map.begin();
          pg->peer_scrub_map[from].decode(p);
          pg->kick();
        }
       } else {
        // replica, reply
-       dout(10) << "handle_pg_scrub generating scrub map for primary" << dendl;
+       dout(10) << *pg << " building scrub map for primary" << dendl;
 
        // do this is a separate thread.. FIXME
        ScrubMap map;
@@ -3322,7 +3322,7 @@ void OSD::handle_op(MOSDOp *op)
       }
 
       // scrubbing?
-      if (pg->state_test(PG_STATE_SCRUBBING)) {
+      if (pg->is_scrubbing()) {
        dout(10) << *pg << " is scrubbing, deferring op " << *op << dendl;
        pg->waiting_for_active.push_back(op);
        pg->unlock();

diff --git a/src/osd/PG.cc b/src/osd/PG.cc
index a58c2cdeeb701fb29315e0ecabbf4b21587e743c..5252073e44fed8700306099bbe71a0c23193c07d 100644 (file)
--- a/src/osd/PG.cc
+++ b/src/osd/PG.cc
@@ -1734,7 +1734,7 @@ void PG::build_scrub_map(ScrubMap &map)
   osd->store->collection_list(c, ls);
 
   // sort
-  dout(10) << "sorting" << dendl;
+  dout(10) << "sorting " << ls.size() << " objects" << dendl;
   vector< pair<pobject_t,int> > tab(ls.size());
   vector< pair<pobject_t,int> >::iterator q = tab.begin();
   int i = 0;
@@ -1754,7 +1754,7 @@ void PG::build_scrub_map(ScrubMap &map)
     pos[p->second] = i;
   // now, pos[orig pos] = sorted pos
 
-  dout(10) << " " << ls.size() << " objects" << dendl;
+  dout(10) << " scanning " << ls.size() << " objects" << dendl;
   map.objects.resize(ls.size());
   i = 0;
   for (vector<pobject_t>::iterator p = ls.begin(); 
@@ -1780,7 +1780,7 @@ void PG::build_scrub_map(ScrubMap &map)
 
   // log
   osd->store->read(coll_t(), info.pgid.to_log_pobject(), 0, 0, map.logbl);
-  dout(10) << " log " << map.logbl.length() << " bytes" << dendl;
+  dout(10) << " done.  pg log is " << map.logbl.length() << " bytes" << dendl;
 }
 
 
@@ -1826,22 +1826,24 @@ void PG::scrub()
     wait();
   }
 
-  unlock();
+  //unlock();
 
-  dout(10) << "scrub building my map" << dendl;
+  dout(10) << "scrub  building my map" << dendl;
   ScrubMap scrubmap;
   build_scrub_map(scrubmap);
 
+  /*
   lock();
   if (epoch != info.history.same_since) {
     dout(10) << "scrub  pg changed, aborting" << dendl;
     unlock();
     return;
   }
+  */
 
   while (peer_scrub_map.size() < acting.size() - 1) {
-    dout(10) << " have " << (peer_scrub_map.size()+1) << " / " << acting.size()
-            << " scrub maps, waiting" << dendl;
+    dout(10) << "scrub  has " << (peer_scrub_map.size()+1) << " / " << acting.size()
+            << " maps, waiting" << dendl;
     wait();
 
     if (epoch != info.history.same_since) {
@@ -1851,117 +1853,134 @@ void PG::scrub()
     }
   }
 
+  /*
   unlock();
+  */
 
-  // first, compare scrub maps
-  vector<ScrubMap*> m(acting.size());
-  m[0] = &scrubmap;
-  for (unsigned i=1; i<acting.size(); i++)
-    m[i] = &peer_scrub_map[acting[i]];
-  vector<ScrubMap::object>::iterator p[acting.size()];
-  for (unsigned i=0; i<acting.size(); i++)
-    p[i] = m[i]->objects.begin();
-
-  int num_missing = 0;
-  int num_bad = 0;
-
-  while (1) {
-    ScrubMap::object *po = 0;
-    bool missing = false;
-    for (unsigned i=0; i<acting.size(); i++) {
-      if (p[i] == m[i]->objects.end())
-       continue;
-      if (!po)
-       po = &(*p[i]);
-      else if (po->poid != p[i]->poid) {
-       missing = true;
-       if (po->poid > p[i]->poid)
-         po = &(*p[i]);
-      }
-    }
-    if (!po)
-      break;
-    if (missing) {
-      for (unsigned i=0; i<acting.size(); i++) {
-       if (po->poid != p[i]->poid) {
-         dout(0) << " osd" << acting[i] << " missing " << po->poid << dendl;
-         num_missing++;
-       } else
-         p[i]++;
-      }
-      continue;
-    }
+  if (acting.size() > 1) {
+    dout(10) << "scrub  comparing replica scrub maps" << dendl;
 
-    // compare
-    bool ok = true;
-    for (unsigned i=1; i<acting.size(); i++) {
-      if (po->size != p[i]->size) {
-       dout(0) << " osd" << acting[i] << " " << po->poid
-               << " size " << p[i]->size << " != " << po->size << dendl;
-       ok = false;
-       num_bad++;
+    // first, compare scrub maps
+    vector<ScrubMap*> m(acting.size());
+    m[0] = &scrubmap;
+    for (unsigned i=1; i<acting.size(); i++)
+      m[i] = &peer_scrub_map[acting[i]];
+    vector<ScrubMap::object>::iterator p[acting.size()];
+    for (unsigned i=0; i<acting.size(); i++)
+      p[i] = m[i]->objects.begin();
+    
+    int num_missing = 0;
+    int num_bad = 0;
+    
+    while (1) {
+      ScrubMap::object *po = 0;
+      bool missing = false;
+      for (unsigned i=0; i<acting.size(); i++) {
+       if (p[i] == m[i]->objects.end()) {
+         missing = true;
+         continue;
+       }
+       if (!po)
+         po = &(*p[i]);
+       else if (po->poid != p[i]->poid) {
+         missing = true;
+         if (po->poid > p[i]->poid)
+           po = &(*p[i]);
+       }
       }
-      if (po->attrs.size() != p[i]->attrs.size()) {
-       dout(0) << " osd" << acting[i] << " " << po->poid
-               << " attr count " << p[i]->attrs.size() << " != " << po->attrs.size() << dendl;
-       ok = false;
-       num_bad++;
+      if (!po)
+       break;
+      if (missing) {
+       for (unsigned i=0; i<acting.size(); i++) {
+         if (p[i] == m[i]->objects.end() || po->poid != p[i]->poid) {
+           dout(0) << "scrub  osd" << acting[i] << " missing " << po->poid << dendl;
+           num_missing++;
+         } else
+           p[i]++;
+       }
+       continue;
       }
-      for (map<nstring,bufferptr>::iterator q = po->attrs.begin(); q != po->attrs.end(); q++) {
-       if (p[i]->attrs.count(q->first)) {
-         if (q->second.cmp(p[i]->attrs[q->first])) {
-           dout(0) << " osd" << acting[i] << " " << po->poid
-                   << " attr " << q->first << " value mismatch" << dendl;
+      
+      // compare
+      dout(10) << " po is " << (void*)po << dendl;
+      dout(10) << " po is " << po->poid << dendl;
+
+      bool ok = true;
+      for (unsigned i=1; i<acting.size(); i++) {
+       if (po->size != p[i]->size) {
+         dout(0) << "scrub  osd" << acting[i] << " " << po->poid
+                 << " size " << p[i]->size << " != " << po->size << dendl;
+         ok = false;
+         num_bad++;
+       }
+       if (po->attrs.size() != p[i]->attrs.size()) {
+         dout(0) << "scrub  osd" << acting[i] << " " << po->poid
+                 << " attr count " << p[i]->attrs.size() << " != " << po->attrs.size() << dendl;
+         ok = false;
+         num_bad++;
+       }
+       for (map<nstring,bufferptr>::iterator q = po->attrs.begin(); q != po->attrs.end(); q++) {
+         if (p[i]->attrs.count(q->first)) {
+           if (q->second.cmp(p[i]->attrs[q->first])) {
+             dout(0) << "scrub  osd" << acting[i] << " " << po->poid
+                     << " attr " << q->first << " value mismatch" << dendl;
+             ok = false;
+             num_bad++;
+           }
+         } else {
+           dout(0) << "scrub  osd" << acting[i] << " " << po->poid
+                   << " attr " << q->first << " missing" << dendl;
            ok = false;
            num_bad++;
          }
-       } else {
-         dout(0) << " osd" << acting[i] << " " << po->poid
-                 << " attr " << q->first << " missing" << dendl;
-         ok = false;
-         num_bad++;
        }
       }
+      
+      if (ok)
+       dout(20) << "scrub  " << po->poid << " size " << po->size << " ok" << dendl;
+      
+      // next
+      for (unsigned i=0; i<acting.size(); i++)
+       p[i]++;
+    }
+    
+    if (num_missing || num_bad) {
+      dout(10) << "scrub " << num_missing << " missing, " << num_bad << " bad objects" << dendl;
+      stringstream ss;
+      ss << "scrub " << info.pgid << " " << num_missing << " missing, " << num_bad << " bad objects";
+      string s;
+      getline(ss, s);
+      osd->get_logclient()->log(LOG_ERROR, s);
     }
-
-    if (ok)
-      dout(10) << "scrub " << po->poid << " size " << po->size << " ok" << dendl;
-
-    // next
-    for (unsigned i=0; i<acting.size(); i++)
-      p[i]++;
-  }
-
-  if (num_missing || num_bad) {
-    dout(10) << "scrub " << num_missing << " missing, " << num_bad << " bad objects" << dendl;
-    stringstream ss;
-    ss << "scrub " << info.pgid << " " << num_missing << " missing, " << num_bad << " bad objects";
-    string s;
-    getline(ss, s);
-    osd->get_logclient()->log(LOG_ERROR, s);
   }
 
+  /*
   lock();
   if (epoch != info.history.same_since) {
     dout(10) << "scrub  pg changed, aborting" << dendl;
     unlock();
     return;
   }
+  */
 
   // discard peer scrub info.
   peer_scrub_map.clear();
 
+  /*
   unlock();
-  
+  */
+
   // ok, do the pg-type specific scrubbing
   _scrub(scrubmap);
 
+  /*
   lock();
   if (epoch != info.history.same_since) {
     dout(10) << "scrub  pg changed, aborting" << dendl;
     unlock();
     return;
   }
+  */
 
   // finish up
   info.stats.last_scrub = info.last_update;

diff --git a/src/osd/PG.h b/src/osd/PG.h
index 37cd5f827963f7d672784f22ff39b87532101d92..0e92507bc67ad5a992375327412c218a5a52a113 100644 (file)
--- a/src/osd/PG.h
+++ b/src/osd/PG.h
@@ -506,6 +506,7 @@ public:
     _lock.Unlock();
   }
   void wait() {
+    assert(_lock.is_locked());
     _cond.Wait(_lock);
   }
   void kick() {
@@ -754,6 +755,8 @@ public:
   bool       is_clean() const { return state_test(PG_STATE_CLEAN); }
   bool       is_stray() const { return state_test(PG_STATE_STRAY); }
 
+  bool       is_scrubbing() const { return state_test(PG_STATE_SCRUBBING); }
+
   bool  is_empty() const { return info.last_update == eversion_t(0,0); }
 
   bool is_complete_pg() { return acting.size() == info.pgid.size(); }