[blobfs] Make fsck continue after errors
Fsck used to early exit when it detected corruption in the backup
superblock, which is not very useful since it hides the other issues
with the image. Change this to let fsck proceed through all checks
before returning whether the image is valid or not.
Tested: unit.
Change-Id: Ibfca83a2615bdf3521ec594eef7f7227c2dd8cbd
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/506798
Reviewed-by: Stephen Demos <sdemos@google.com>
Commit-Queue: James Sullivan <jfsulliv@google.com>
(cherry picked from commit c454c5545ef80ed2e9fd0c8905fdbf03c8eddf51)
diff --git a/src/storage/blobfs/blobfs-checker.cc b/src/storage/blobfs/blobfs-checker.cc
index 617dc92..cfdcb34 100644
--- a/src/storage/blobfs/blobfs-checker.cc
+++ b/src/storage/blobfs/blobfs-checker.cc
@@ -26,36 +26,37 @@
namespace blobfs {
-zx_status_t BlobfsChecker::CheckBackupSuperblock() {
+bool BlobfsChecker::CheckBackupSuperblock() {
if ((blobfs_->Info().flags & kBlobFlagFVM) == 0 ||
blobfs_->Info().oldest_revision < kBlobfsRevisionBackupSuperblock)
- return ZX_OK;
+ return true;
auto superblock_or = blobfs_->ReadBackupSuperblock();
if (superblock_or.is_error()) {
- FX_LOGS(ERROR) << "could not read backup superblock";
- return superblock_or.status_value();
+ FX_LOGS(ERROR) << "could not read backup superblock: " << superblock_or.status_value();
+ return false;
}
if (zx_status_t status =
CheckSuperblock(superblock_or.value().get(), TotalBlocks(*superblock_or.value()));
status != ZX_OK) {
- FX_LOGS(ERROR) << "bad backup superblock";
- return status;
+ FX_LOGS(ERROR) << "bad backup superblock: " << status;
+ return false;
}
- return ZX_OK;
+ return true;
}
-void BlobfsChecker::TraverseInodeBitmap() {
+bool BlobfsChecker::TraverseInodeBitmap() {
+ bool valid = true;
for (unsigned n = 0; n < blobfs_->info_.inode_count; n++) {
auto inode = blobfs_->GetNode(n);
ZX_ASSERT_MSG(inode.is_ok(), "Failed to get node %u: status=%d", n, inode.status_value());
if (inode->header.IsAllocated()) {
alloc_inodes_++;
if (inode->header.IsExtentContainer()) {
- // TODO(smklein): sanity check these containers.
+ // Extent containers need no validation since we're going to validate the data of all blobs.
continue;
}
- bool valid = true;
+ bool blob_valid = true;
auto extents = AllocatedExtentIterator::Create(blobfs_->GetNodeFinder(), n);
ZX_ASSERT_MSG(extents.is_ok(), "Failed to create extent iterator for inode %u: status=%d", n,
@@ -67,13 +68,13 @@
if (status != ZX_OK) {
FX_LOGS(ERROR) << "check: Failed to acquire extent " << extents->ExtentIndex()
<< " within inode " << n << ": " << *inode.value();
- valid = false;
+ blob_valid = false;
break;
}
if (extent->Length() == 0) {
FX_LOGS(ERROR) << "check: Found zero-length extent at idx " << extents->ExtentIndex()
<< " within inode " << n << ": " << *inode.value();
- valid = false;
+ blob_valid = false;
break;
}
@@ -86,42 +87,46 @@
<< "). "
"Not fully allocated in block bitmap; first unset @"
<< first_unset;
- valid = false;
+ blob_valid = false;
}
inode_blocks_ += extent->Length();
}
- if (valid && blobfs_->LoadAndVerifyBlob(n) != ZX_OK) {
- FX_LOGS(ERROR) << "check: detected inode " << n << " with bad state";
- valid = false;
+ if (blob_valid) {
+ if (zx_status_t status = blobfs_->LoadAndVerifyBlob(n); status != ZX_OK) {
+ FX_LOGS(ERROR) << "check: detected inode " << n << " with bad state: " << status;
+ blob_valid = false;
+ }
}
- if (!valid) {
- error_blobs_++;
+ if (!blob_valid) {
+ valid = false;
}
}
}
+ return valid;
}
-void BlobfsChecker::TraverseBlockBitmap() {
+bool BlobfsChecker::TraverseBlockBitmap() {
for (uint64_t n = 0; n < blobfs_->info_.data_block_count; n++) {
if (blobfs_->CheckBlocksAllocated(n, n + 1)) {
alloc_blocks_++;
}
}
+ return true;
}
-zx_status_t BlobfsChecker::CheckAllocatedCounts() const {
- zx_status_t status = ZX_OK;
+bool BlobfsChecker::CheckAllocatedCounts() const {
+ bool valid = true;
if (alloc_blocks_ != blobfs_->info_.alloc_block_count) {
FX_LOGS(ERROR) << "check: incorrect allocated block count " << blobfs_->info_.alloc_block_count
<< " (should be " << alloc_blocks_ << ")";
- status = ZX_ERR_BAD_STATE;
+ valid = false;
}
if (alloc_blocks_ < kStartBlockMinimum) {
FX_LOGS(ERROR) << "check: allocated blocks (" << alloc_blocks_ << ") are less than minimum ("
<< kStartBlockMinimum << ")";
- status = ZX_ERR_BAD_STATE;
+ valid = false;
}
if (inode_blocks_ + kStartBlockMinimum != alloc_blocks_) {
@@ -129,28 +134,27 @@
<< ") do not match inode allocated blocks "
"("
<< inode_blocks_ + kStartBlockMinimum << ")";
- status = ZX_ERR_BAD_STATE;
+ valid = false;
}
if (alloc_inodes_ != blobfs_->info_.alloc_inode_count) {
FX_LOGS(ERROR) << "check: incorrect allocated inode count " << blobfs_->info_.alloc_inode_count
<< " (should be " << alloc_inodes_ << ")";
- status = ZX_ERR_BAD_STATE;
+ valid = false;
}
-
- if (error_blobs_) {
- status = ZX_ERR_BAD_STATE;
- }
-
- return status;
+ return valid;
}
-zx_status_t BlobfsChecker::Check() {
- if (zx_status_t status = CheckBackupSuperblock(); status != ZX_OK)
- return status;
- TraverseInodeBitmap();
- TraverseBlockBitmap();
- return CheckAllocatedCounts();
+bool BlobfsChecker::Check() {
+ bool valid = true;
+ FX_LOGS(INFO) << "Checking backup superblock...";
+ valid &= CheckBackupSuperblock();
+ FX_LOGS(INFO) << "Verifying inodes and blob data...";
+ valid &= TraverseInodeBitmap();
+ FX_LOGS(INFO) << "Checking allocation counts...";
+ valid &= TraverseBlockBitmap();
+ valid &= CheckAllocatedCounts();
+ return valid;
}
BlobfsChecker::BlobfsChecker(std::unique_ptr<Blobfs> blobfs, Options options)
diff --git a/src/storage/blobfs/blobfs-checker.h b/src/storage/blobfs/blobfs-checker.h
index 98f35ae..8e69628 100644
--- a/src/storage/blobfs/blobfs-checker.h
+++ b/src/storage/blobfs/blobfs-checker.h
@@ -31,20 +31,20 @@
// Check validates the blobfs filesystem provided when the Checker was
// constructed. It walks each of the inode and block allocation bitmaps
// only once.
- zx_status_t Check();
+ // Returns true if the filesystem is valid.
+ bool Check();
private:
std::unique_ptr<Blobfs> blobfs_;
uint32_t alloc_inodes_ = 0;
uint32_t alloc_blocks_ = 0;
- uint32_t error_blobs_ = 0;
uint32_t inode_blocks_ = 0;
const Options options_;
- zx_status_t CheckBackupSuperblock();
- void TraverseInodeBitmap();
- void TraverseBlockBitmap();
- zx_status_t CheckAllocatedCounts() const;
+ bool CheckBackupSuperblock();
+ bool TraverseInodeBitmap();
+ bool TraverseBlockBitmap();
+ bool CheckAllocatedCounts() const;
};
#ifdef __Fuchsia__
diff --git a/src/storage/blobfs/fsck-host.cc b/src/storage/blobfs/fsck-host.cc
index 5e397a3..bd7e486 100644
--- a/src/storage/blobfs/fsck-host.cc
+++ b/src/storage/blobfs/fsck-host.cc
@@ -7,9 +7,12 @@
#include <memory>
#include "src/storage/blobfs/blobfs-checker.h"
+#include "zircon/errors.h"
namespace blobfs {
-zx_status_t Fsck(std::unique_ptr<Blobfs> blob) { return BlobfsChecker(std::move(blob)).Check(); }
+zx_status_t Fsck(std::unique_ptr<Blobfs> blob) {
+ return BlobfsChecker(std::move(blob)).Check() ? ZX_OK : ZX_ERR_IO_DATA_INTEGRITY;
+}
} // namespace blobfs
diff --git a/src/storage/blobfs/fsck.cc b/src/storage/blobfs/fsck.cc
index a59eed5..a44d165 100644
--- a/src/storage/blobfs/fsck.cc
+++ b/src/storage/blobfs/fsck.cc
@@ -13,6 +13,7 @@
#include "src/storage/blobfs/blobfs-checker.h"
#include "src/storage/blobfs/blobfs.h"
#include "src/storage/blobfs/iterator/extent-iterator.h"
+#include "zircon/errors.h"
namespace blobfs {
@@ -20,21 +21,23 @@
async::Loop loop(&kAsyncLoopConfigNoAttachToCurrentThread);
zx_status_t status = loop.StartThread();
if (status != ZX_OK) {
- FX_LOGS(ERROR) << "Cannot initialize dispatch loop";
+ FX_LOGS(ERROR) << "Cannot initialize dispatch loop: " << zx_status_get_string(status);
return status;
}
std::unique_ptr<Blobfs> blobfs;
status = Blobfs::Create(loop.dispatcher(), std::move(device), options, zx::resource(), &blobfs);
if (status != ZX_OK) {
- FX_LOGS(ERROR) << "Cannot create filesystem for checking";
+ FX_LOGS(ERROR) << "Cannot create filesystem for checking" << zx_status_get_string(status);
return status;
}
BlobfsChecker::Options checker_options;
if (blobfs->writability() == Writability::ReadOnlyDisk) {
checker_options.repair = false;
}
- return BlobfsChecker(std::move(blobfs), checker_options).Check();
+ return BlobfsChecker(std::move(blobfs), checker_options).Check()
+ ? ZX_OK
+ : ZX_ERR_IO_DATA_INTEGRITY;
}
} // namespace blobfs
diff --git a/src/storage/blobfs/test/host/host-test.cc b/src/storage/blobfs/test/host/host-test.cc
index aaf2c90..c62315c 100644
--- a/src/storage/blobfs/test/host/host-test.cc
+++ b/src/storage/blobfs/test/host/host-test.cc
@@ -206,7 +206,7 @@
// Check that the blob can be read back and verified.
BlobfsChecker checker(std::move(blobfs), {.repair = false});
- EXPECT_EQ(checker.Check(), ZX_OK);
+ EXPECT_TRUE(checker.Check());
}
TEST(BlobfsHostTest, WriteBlobWithCompactFormatAndSharedBlockIsCorrect) {
@@ -223,7 +223,7 @@
// Check that the blob can be read back and verified.
BlobfsChecker checker(std::move(blobfs), {.repair = false});
- EXPECT_EQ(checker.Check(), ZX_OK);
+ EXPECT_TRUE(checker.Check());
}
TEST(BlobfsHostTest, WriteBlobWithCompactFormatAndBlockIsNotSharedIsCorrect) {
@@ -239,7 +239,7 @@
// Check that the blob can be read back and verified.
BlobfsChecker checker(std::move(blobfs), {.repair = false});
- EXPECT_EQ(checker.Check(), ZX_OK);
+ EXPECT_TRUE(checker.Check());
}
TEST(BlobfsHostTest, WriteCompressedBlobWithCompactFormatAndSharedBlockIsCorrect) {
@@ -254,7 +254,7 @@
// Check that the blob can be read back and verified.
BlobfsChecker checker(std::move(blobfs), {.repair = false});
- EXPECT_EQ(checker.Check(), ZX_OK);
+ EXPECT_TRUE(checker.Check());
}
TEST(BlobfsHostTest, WriteCompressedBlobWithPaddedFormatIsCorrect) {
@@ -269,7 +269,7 @@
// Check that the blob can be read back and verified.
BlobfsChecker checker(std::move(blobfs), {.repair = false});
- EXPECT_EQ(checker.Check(), ZX_OK);
+ EXPECT_TRUE(checker.Check());
}
TEST(BlobfsHostTest, WriteEmptyBlobWithCompactFormatIsCorrect) {
@@ -282,7 +282,7 @@
// Check that the blob can be read back and verified.
BlobfsChecker checker(std::move(blobfs), {.repair = false});
- EXPECT_EQ(checker.Check(), ZX_OK);
+ EXPECT_TRUE(checker.Check());
}
void CheckBlobContents(File& blob, fbl::Span<const uint8_t> contents) {
@@ -350,7 +350,7 @@
// Check that the blob can be read back and verified.
BlobfsChecker checker(std::move(blobfs), {.repair = false});
- EXPECT_EQ(checker.Check(), ZX_OK);
+ EXPECT_TRUE(checker.Check());
}
TEST(BlobfsHostTest, VisitBlobsForwardsVisitorErrors) {
@@ -457,7 +457,7 @@
ASSERT_TRUE(blobfs);
AddUncompressedBlob(/*data_size=*/0, *blobfs);
BlobfsChecker checker(std::move(blobfs));
- ASSERT_EQ(checker.Check(), ZX_OK);
+ EXPECT_TRUE(checker.Check());
}
} // namespace
diff --git a/src/storage/blobfs/test/unit/blobfs-checker-test.cc b/src/storage/blobfs/test/unit/blobfs-checker-test.cc
index 4b30645..f46d9ba 100644
--- a/src/storage/blobfs/test/unit/blobfs-checker-test.cc
+++ b/src/storage/blobfs/test/unit/blobfs-checker-test.cc
@@ -146,7 +146,7 @@
void RunTestEmpty(BlobfsCheckerTest* t) {
BlobfsChecker checker(t->get_fs_unique());
- ASSERT_EQ(checker.Check(), ZX_OK);
+ EXPECT_TRUE(checker.Check());
}
TEST_F(BlobfsCheckerTest, TestEmpty) { RunTestEmpty(this); }
@@ -163,7 +163,7 @@
EXPECT_EQ(t->Sync(), ZX_OK);
BlobfsChecker checker(t->get_fs_unique());
- ASSERT_EQ(checker.Check(), ZX_OK);
+ EXPECT_TRUE(checker.Check());
}
TEST_F(BlobfsCheckerTest, TestNonEmpty) { RunTestNonEmpty(this); }
@@ -183,7 +183,7 @@
t->get_fs()->GetAllocator()->FreeBlocks(e);
BlobfsChecker checker(t->get_fs_unique());
- ASSERT_EQ(checker.Check(), ZX_ERR_BAD_STATE);
+ EXPECT_FALSE(checker.Check());
}
TEST_F(BlobfsCheckerTest, TestInodeWithUnallocatedBlock) { RunTestInodeWithUnallocatedBlock(this); }
@@ -205,7 +205,7 @@
ASSERT_EQ(t->UpdateSuperblock(superblock), ZX_OK);
BlobfsChecker checker(t->get_fs_unique());
- ASSERT_EQ(checker.Check(), ZX_ERR_BAD_STATE);
+ EXPECT_FALSE(checker.Check());
}
TEST_F(BlobfsCheckerTest, TestAllocatedBlockCountTooHigh) {
@@ -230,7 +230,7 @@
t->UpdateSuperblock(superblock);
BlobfsChecker checker(t->get_fs_unique());
- ASSERT_EQ(checker.Check(), ZX_ERR_BAD_STATE);
+ EXPECT_FALSE(checker.Check());
}
TEST_F(BlobfsCheckerTest, TestAllocatedBlockCountTooLow) { RunTestAllocatedBlockCountTooLow(this); }
@@ -243,7 +243,7 @@
Extent e(0, 1);
t->get_fs()->GetAllocator()->FreeBlocks(e);
BlobfsChecker checker(t->get_fs_unique());
- ASSERT_EQ(checker.Check(), ZX_ERR_BAD_STATE);
+ EXPECT_FALSE(checker.Check());
}
TEST_F(BlobfsCheckerTest, TestFewerThanMinimumBlocksAllocated) {
@@ -265,7 +265,7 @@
t->UpdateSuperblock(superblock);
BlobfsChecker checker(t->get_fs_unique());
- ASSERT_EQ(checker.Check(), ZX_ERR_BAD_STATE);
+ EXPECT_FALSE(checker.Check());
}
TEST_F(BlobfsCheckerTest, TestAllocatedInodeCountTooHigh) {
@@ -290,7 +290,7 @@
t->UpdateSuperblock(superblock);
BlobfsChecker checker(t->get_fs_unique());
- ASSERT_EQ(checker.Check(), ZX_ERR_BAD_STATE);
+ EXPECT_FALSE(checker.Check());
}
TEST_F(BlobfsCheckerTest, TestAllocatedInodeCountTooLow) { RunTestAllocatedInodeCountTooLow(this); }
@@ -316,7 +316,7 @@
EXPECT_EQ(t->Sync(), ZX_OK);
BlobfsChecker checker(t->get_fs_unique());
- ASSERT_EQ(checker.Check(), ZX_ERR_BAD_STATE);
+ EXPECT_FALSE(checker.Check());
}
TEST_F(BlobfsCheckerTest, TestCorruptBlobs) { RunTestCorruptBlobs(this); }
diff --git a/src/storage/blobfs/test/unit/fsck-test.cc b/src/storage/blobfs/test/unit/fsck-test.cc
index 3e61fff..152e9e5 100644
--- a/src/storage/blobfs/test/unit/fsck-test.cc
+++ b/src/storage/blobfs/test/unit/fsck-test.cc
@@ -72,10 +72,10 @@
memset(block, 0xaf, sizeof(block));
DeviceBlockWrite(device.get(), block, sizeof(block), kBlobfsBlockSize);
- ASSERT_EQ(Fsck(std::move(device), MountOptions()), ZX_ERR_INVALID_ARGS);
+ ASSERT_NE(Fsck(std::move(device), MountOptions()), ZX_OK);
}
-TEST(FsckTest, TestNoBackupSuperblockOnOldRevsiionPassesFsck) {
+TEST(FsckTest, TestNoBackupSuperblockOnOldRevisionPassesFsck) {
auto device = std::make_unique<block_client::FakeFVMBlockDevice>(
400, kBlobfsBlockSize, /*slice_size=*/32768, /*slice_capacity=*/500);
ASSERT_TRUE(device);
