commit 1541d61a83045287b8b42f1f6b67c5fb84bcf6ab
author David Rogers <davidrogers@google.com> Thu Feb 06 23:47:02 2020 -0800
committer David Rogers <davidrogers@google.com> Thu Feb 06 23:47:02 2020 -0800
tree 7e7554692be985475c3bec9f657ebf8152be2557
parent 2be76b00d2366eea438f6faa39ab056370984ad7

pw_kvs: Fix 2 bugs in finding sectors with space

Bug 1: In FindSectorWithSpace() the loop over all sectors starting after
last new sector only searched for sector_map_size_ - 1 sectors, missing
the last new sector.
Solution 1: Cleaned up the for() logic to loop over the full range of
active sectors.

Bug 2: GarbageCollectOneSector() returns the sector cleaned, which is
then used for the write that triggered the GC, resulting in violation of
the always keep a sector free rule.
Solution 2: GarbageCollectOneSector() no longer takes an arg for the
sector descriptor to return. Once GC is done, do a new FindSectorWithSpace()
to find the best write location of the post-GC sectors.

Change-Id: I041cda9d5229f7686ab9a255f847b072d8a5ea51

pw_kvs/key_value_store.cc

diff --git a/pw_kvs/key_value_store.cc b/pw_kvs/key_value_store.cc
index 617eb6b..07ff99a 100644
--- a/pw_kvs/key_value_store.cc
+++ b/pw_kvs/key_value_store.cc
@@ -587,8 +587,8 @@
   // Look for a partial sector to use with enough space. Immediately use the
   // first one of those that is found. While scanning for a partial sector, keep
   // track of the first empty sector and if a second sector was seen.
-  for (size_t i = start; i != last_new_sector_index_;
-       i = (i + 1) % sector_map_size_) {
+  for (size_t j = 0; j < sector_map_size_; j++) {
+    size_t i = (j + start) % sector_map_size_;
     SectorDescriptor& sector = sector_map_[i];
 
     if (sector_to_skip == &sector) {
@@ -639,7 +639,9 @@
     return result;
   }
   if (options_.partial_gc_on_write) {
-    return GarbageCollectOneSector(sector);
+    // Garbage collect and then try again to find the best sector.
+    TRY(GarbageCollectOneSector());
+    return FindSectorWithSpace(sector, size);
   }
   return result;
 }
@@ -676,7 +678,7 @@
   return sector_candidate;
 }
 
-Status KeyValueStore::GarbageCollectOneSector(SectorDescriptor** sector) {
+Status KeyValueStore::GarbageCollectOneSector() {
   DBG("Garbage Collect a single sector");
 
   // Step 1: Find the sector to garbage collect
@@ -709,7 +711,6 @@
   sector_to_gc->tail_free_bytes = partition_.sector_size_bytes();
 
   DBG("  Garbage Collect complete");
-  *sector = sector_to_gc;
   return Status::OK;
 }
 

pw_kvs/public/pw_kvs/key_value_store.h

diff --git a/pw_kvs/public/pw_kvs/key_value_store.h b/pw_kvs/public/pw_kvs/key_value_store.h
index 792682a..886c81d 100644
--- a/pw_kvs/public/pw_kvs/key_value_store.h
+++ b/pw_kvs/public/pw_kvs/key_value_store.h
@@ -311,7 +311,7 @@
 
   Status FindOrRecoverSectorWithSpace(SectorDescriptor** sector, size_t size);
 
-  Status GarbageCollectOneSector(SectorDescriptor** sector);
+  Status GarbageCollectOneSector();
 
   SectorDescriptor* FindSectorToGarbageCollect();