pw_kvs: Don't use checksum for same value write check
When checking if a same value is being written, don't use the checksum
as part of the check. Checksum includes the transaction ID, which will
always be different, resulting the the check always failing.
Change-Id: I3ef3e336f4e1048b2fb7944f2b8fd0b000415c9b
diff --git a/pw_kvs/key_value_store.cc b/pw_kvs/key_value_store.cc
index fa85c42..50b025b 100644
--- a/pw_kvs/key_value_store.cc
+++ b/pw_kvs/key_value_store.cc
@@ -639,19 +639,15 @@
EntryState new_state,
EntryMetadata* prior_metadata,
const Entry* prior_entry) {
- Entry entry = CreateEntry(key, value, new_state);
-
// If new entry and prior entry have matching value size, state, and checksum,
// check if the values match. Directly compare the prior and new values
// because the checksum can not be depended on to establish equality, it can
// only be depended on to establish inequality.
- if (prior_entry != nullptr &&
- prior_entry->value_size() == entry.value_size() &&
+ if (prior_entry != nullptr && prior_entry->value_size() == value.size() &&
prior_metadata->state() == new_state &&
- prior_entry->checksum() == entry.checksum() &&
prior_entry->ValueMatches(value).ok()) {
- // The new value matches the prior value, don't need to write anything.
- // Just keep the existing entry.
+ // The new value matches the prior value, don't need to write anything. Just
+ // keep the existing entry.
DBG("Write for key 0x%08x with matching value skipped",
unsigned(prior_metadata->hash()));
return Status::OK;
@@ -665,14 +661,8 @@
const size_t entry_size = Entry::size(partition_, key, value);
TRY(GetAddressesForWrite(reserved_addresses, entry_size));
- // Commiting to do the write, time to update last_transaction_id_. Update
- // here, rather in CreateEntry, so last_transaction_id_ only increments for
- // writes that are actually attempted.
- last_transaction_id_ += 1;
- PW_CHECK(last_transaction_id_ == entry.transaction_id());
-
// Write the entry at the first address that was found.
- entry.set_address(reserved_addresses[0]);
+ Entry entry = CreateEntry(reserved_addresses[0], key, value, new_state);
TRY(AppendEntry(entry, key, value));
// After writing the first entry successfully, update the key descriptors.
@@ -1206,7 +1196,8 @@
return FixErrors();
}
-KeyValueStore::Entry KeyValueStore::CreateEntry(string_view key,
+KeyValueStore::Entry KeyValueStore::CreateEntry(Address address,
+ string_view key,
span<const byte> value,
EntryState state) {
// Always bump the transaction ID when creating a new entry.
@@ -1220,20 +1211,19 @@
// 2. The transaction ID is NOT incremented, because of the failure
// 3. (later) A new entry is written, re-using the transaction ID (oops)
//
- // By always burning transaction IDs, the above problem can't happen. The
- // actual updating of last_transaction_id_ is done once the write method is
- // ready to commit to attempting an actual write.
- uint32_t new_transaction_id = last_transaction_id_ + 1;
-
- // Set address to zero, the address of the entry is set later.
- const Address address = 0;
+ // By always burning transaction IDs, the above problem can't happen.
+ last_transaction_id_ += 1;
if (state == EntryState::kDeleted) {
return Entry::Tombstone(
- partition_, address, formats_.primary(), key, new_transaction_id);
+ partition_, address, formats_.primary(), key, last_transaction_id_);
}
- return Entry::Valid(
- partition_, address, formats_.primary(), key, value, new_transaction_id);
+ return Entry::Valid(partition_,
+ address,
+ formats_.primary(),
+ key,
+ value,
+ last_transaction_id_);
}
void KeyValueStore::LogDebugInfo() const {
diff --git a/pw_kvs/key_value_store_binary_format_test.cc b/pw_kvs/key_value_store_binary_format_test.cc
index 97f0922..8ce04d4 100644
--- a/pw_kvs/key_value_store_binary_format_test.cc
+++ b/pw_kvs/key_value_store_binary_format_test.cc
@@ -1019,10 +1019,6 @@
KeyValueStoreBuffer<kMaxEntries, kMaxUsableSectors> kvs_;
};
-// Block of data to use for entry value. Sized to 470 so the total entry results
-// in the 512 byte sector having 16 bytes remaining.
-constexpr uint8_t test_data[470] = {1, 2, 3, 4, 5, 6};
-
// Test a KVS with a number of entries, several sectors that are nearly full
// of stale (reclaimable) space, and not enough writable (free) space to add a
// redundant copy for any of the entries. Tests that the add redundancy step of
@@ -1044,14 +1040,23 @@
EXPECT_EQ(stats.corrupt_sectors_recovered, 0u);
EXPECT_EQ(stats.missing_redundant_entries_recovered, 0u);
- // Add a near-sector size key entry to fill the KVS with a valid large entry
- // and stale data.
+ // Block of data to use for entry value. Sized to 470 so the total entry
+ // results in the 512 byte sector having 16 bytes remaining.
+ uint8_t test_data[470] = {1, 2, 3, 4, 5, 6};
+ // Add a near-sector size key entry to fill the KVS with a valid large entry
+ // and stale data. Modify the value in between Puts so it actually writes
+ // (identical value writes are skipped).
EXPECT_EQ(Status::OK, kvs_.Put("big_key", test_data));
+ test_data[0]++;
EXPECT_EQ(Status::OK, kvs_.Put("big_key", test_data));
+ test_data[0]++;
EXPECT_EQ(Status::OK, kvs_.Put("big_key", test_data));
+ test_data[0]++;
EXPECT_EQ(Status::OK, kvs_.Put("big_key", test_data));
+ test_data[0]++;
EXPECT_EQ(Status::OK, kvs_.Put("big_key", test_data));
+ test_data[0]++;
EXPECT_EQ(Status::OK, kvs_.Put("big_key", test_data));
// Instantiate a new KVS with redundancy of 2. This KVS should add an extra
diff --git a/pw_kvs/key_value_store_test.cc b/pw_kvs/key_value_store_test.cc
index d2f33fc..0c9d48e 100644
--- a/pw_kvs/key_value_store_test.cc
+++ b/pw_kvs/key_value_store_test.cc
@@ -738,9 +738,8 @@
ASSERT_OK(flash.partition.Erase());
// Create and initialize the KVS.
- constexpr EntryFormat format{.magic = 0xBAD'C0D3, .checksum = nullptr};
KeyValueStoreBuffer<kMaxEntries, kMaxUsableSectors> kvs(&flash.partition,
- format);
+ default_format);
ASSERT_OK(kvs.Init());
// Add one entry, with the same key and value, multiple times.
diff --git a/pw_kvs/public/pw_kvs/internal/entry.h b/pw_kvs/public/pw_kvs/internal/entry.h
index a8f297f..3e75aeb 100644
--- a/pw_kvs/public/pw_kvs/internal/entry.h
+++ b/pw_kvs/public/pw_kvs/internal/entry.h
@@ -158,8 +158,6 @@
uint32_t magic() const { return header_.magic; }
- uint32_t checksum() const { return header_.checksum; }
-
uint32_t transaction_id() const { return header_.transaction_id; }
// True if this is a tombstone entry.
diff --git a/pw_kvs/public/pw_kvs/key_value_store.h b/pw_kvs/public/pw_kvs/key_value_store.h
index e1a3b18..f382c60 100644
--- a/pw_kvs/public/pw_kvs/key_value_store.h
+++ b/pw_kvs/public/pw_kvs/key_value_store.h
@@ -459,7 +459,8 @@
Status Repair();
- internal::Entry CreateEntry(std::string_view key,
+ internal::Entry CreateEntry(Address address,
+ std::string_view key,
span<const std::byte> value,
EntryState state);
