From 95be00a8bebedded3d7d7f71faccab4d6589d976 Mon Sep 17 00:00:00 2001 From: Steve Atherton Date: Wed, 3 May 2023 10:39:13 -0700 Subject: [PATCH] Move AsyncFileWriteChecker to right above SimpleFile in the file stack in simulation, which is analogous to where it is created in production and prevents false positive errors caused by stacking it on top of AsyncFileNonDurable multiple times for different users of the same file. --- fdbrpc/include/fdbrpc/AsyncFileWriteChecker.h | 14 ++----------- fdbrpc/sim2.actor.cpp | 20 +++++++++---------- 2 files changed, 11 insertions(+), 23 deletions(-) diff --git a/fdbrpc/include/fdbrpc/AsyncFileWriteChecker.h b/fdbrpc/include/fdbrpc/AsyncFileWriteChecker.h index fece83cc82..b7251590f5 100644 --- a/fdbrpc/include/fdbrpc/AsyncFileWriteChecker.h +++ b/fdbrpc/include/fdbrpc/AsyncFileWriteChecker.h @@ -35,12 +35,7 @@ public: // Lambda must hold a reference to this to keep it alive until after the read auto self = Reference::addRef(this); return map(m_f->read(data, length, offset), [self, data, offset](int r) { - // Do not check the checksum if self is the sole owner of the reference because the end user has dropped the - // file handle and may already have re-opened the file and written to it before this read completed so our - // stored checksum can be wrong. - if (!self->isSoleOwner()) { - self->updateChecksumHistory(false, offset, r, (uint8_t*)data); - } + self->updateChecksumHistory(false, offset, r, (uint8_t*)data); return r; }); } @@ -48,12 +43,7 @@ public: // Lambda must hold a reference to this to keep it alive until after the read auto self = Reference::addRef(this); return map(m_f->readZeroCopy(data, length, offset), [self, data, length, offset](Void r) { - // Do not check the checksum if self is the sole owner of the reference because the end user has dropped the - // file handle and may already have re-opened the file and written to it before this read completed so our - // stored checksum can be wrong. - if (!self->isSoleOwner()) { - self->updateChecksumHistory(false, offset, *length, (uint8_t*)data); - } + self->updateChecksumHistory(false, offset, *length, (uint8_t*)data); return r; }); } diff --git a/fdbrpc/sim2.actor.cpp b/fdbrpc/sim2.actor.cpp index c98b3c3e0d..2f9e88f313 100644 --- a/fdbrpc/sim2.actor.cpp +++ b/fdbrpc/sim2.actor.cpp @@ -2882,10 +2882,6 @@ Future> Sim2FileSystem::open(const std::string& file auto partFile = machineCache.find(actualFilename); if (partFile != machineCache.end()) { Future> f = AsyncFileDetachable::open(partFile->second.get()); - if (FLOW_KNOBS->PAGE_WRITE_CHECKSUM_HISTORY > 0) - f = map(f, [=](Reference r) { - return Reference(new AsyncFileWriteChecker(r)); - }); return f; } } @@ -2897,11 +2893,15 @@ Future> Sim2FileSystem::open(const std::string& file // This way, they can both keep up with the time to start the next operation auto diskParameters = makeReference(FLOW_KNOBS->SIM_DISK_IOPS, FLOW_KNOBS->SIM_DISK_BANDWIDTH); - f = AsyncFileNonDurable::open(filename, - actualFilename, - SimpleFile::open(filename, flags, mode, diskParameters, false), - diskParameters, - (flags & IAsyncFile::OPEN_NO_AIO) == 0); + + f = SimpleFile::open(filename, flags, mode, diskParameters, false); + if (FLOW_KNOBS->PAGE_WRITE_CHECKSUM_HISTORY > 0) { + f = map(f, + [=](Reference r) { return Reference(new AsyncFileWriteChecker(r)); }); + } + + f = AsyncFileNonDurable::open( + filename, actualFilename, f, diskParameters, (flags & IAsyncFile::OPEN_NO_AIO) == 0); machineCache[actualFilename] = UnsafeWeakFutureReference(f); } else { @@ -2909,8 +2909,6 @@ Future> Sim2FileSystem::open(const std::string& file } f = AsyncFileDetachable::open(f); - if (FLOW_KNOBS->PAGE_WRITE_CHECKSUM_HISTORY > 0) - f = map(f, [=](Reference r) { return Reference(new AsyncFileWriteChecker(r)); }); if (FLOW_KNOBS->ENABLE_CHAOS_FEATURES) f = map(f, [=](Reference r) { return Reference(new AsyncFileChaos(r)); }); if (flags & IAsyncFile::OPEN_ENCRYPTED)