New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os/bluestore: fsck: verify blob.unused field #14316

Merged
merged 6 commits into from Apr 5, 2017

Conversation

Projects
None yet
2 participants
@liewegas
Member

liewegas commented Apr 4, 2017

Two checks:

  • verify that no logical extents reference the portion of the
    blob marked unused, and
  • verify that the csum (if present) is zero for any unused
    region.

And a few fixes to the blob reuse code that the checks turn up!

@liewegas liewegas requested a review from ifed01 Apr 4, 2017

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Apr 4, 2017

Member

@ifed01 can you take a look?

Member

liewegas commented Apr 4, 2017

@ifed01 can you take a look?

liewegas added some commits Apr 4, 2017

os/bluestore: prefix fsck errors with 'fsck error:'
Greppable!

Signed-off-by: Sage Weil <sage@redhat.com>
os/bluestore: mark_used for wctx->writes
The refactored blob reuse code needs to mark blobs used when
processing wctx->writes.

Signed-off-by: Sage Weil <sage@redhat.com>
os/bluestore: some whitespace
Signed-off-by: Sage Weil <sage@redhat.com>
os/bluestore: do not extend blobs with unused bitmap
If we resize the blob we need to adjust the resolution of the
unused bitmap, and that is only possible for some bit patterns.
For now just ignore blobs with unused blocks.

Add an assert in add_tail() so that we don't forget that
add_tail is (probably) where we'd would do that adjustment.

Signed-off-by: Sage Weil <sage@redhat.com>

@liewegas liewegas changed the title from DNM os/bluestore: fsck: verify blob.unused field to os/bluestore: fsck: verify blob.unused field Apr 4, 2017

@ifed01

This comment has been minimized.

Show comment
Hide comment
@ifed01

ifed01 Apr 4, 2017

Contributor

@liewegas - will do

Contributor

ifed01 commented Apr 4, 2017

@liewegas - will do

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Apr 5, 2017

Member

@ifed01 look ok? passes ceph_test_objectstore and rados run

Member

liewegas commented Apr 5, 2017

@ifed01 look ok? passes ceph_test_objectstore and rados run

@@ -1784,6 +1784,11 @@ bool BlueStore::Blob::try_reuse_blob(uint32_t min_alloc_size,
return false;
}
// FIXME: in some cases we could reduce unused resolution

This comment has been minimized.

@ifed01

ifed01 Apr 5, 2017

Contributor

Suggest to move on top of the func, close to is_mutable check...

@ifed01

ifed01 Apr 5, 2017

Contributor

Suggest to move on top of the func, close to is_mutable check...

This comment has been minimized.

@ifed01

ifed01 Apr 5, 2017

Contributor

Never mind, that's for extend only...

@ifed01

ifed01 Apr 5, 2017

Contributor

Never mind, that's for extend only...

Show outdated Hide outdated src/os/bluestore/BlueStore.cc
@@ -5206,6 +5207,65 @@ int BlueStore::fsck(bool deep)
l.blob_offset,
l.length);
++num_extents;
if (blob.has_unused()) {
auto p = referenced.find(l.blob);
uint16_t *pu;

This comment has been minimized.

@ifed01

ifed01 Apr 5, 2017

Contributor

make a typedef for unused type to ease subsequent evolution if any? Or even use decltype...

@ifed01

ifed01 Apr 5, 2017

Contributor

make a typedef for unused type to ease subsequent evolution if any? Or even use decltype...

Show outdated Hide outdated src/os/bluestore/BlueStore.cc
if (i.second & blob.unused) {
derr << __func__ << " error: " << oid << " blob claims unused 0x"
<< std::hex << blob.unused
<< " but extents referece 0x" << i.second

This comment has been minimized.

@ifed01

ifed01 Apr 5, 2017

Contributor

typo

@ifed01

ifed01 Apr 5, 2017

Contributor

typo

liewegas added some commits Apr 4, 2017

os/bluestore: fsck: verify blob.unused field
Two checks:

- verify that no logical extents reference the portion of the
  blob marked unused, and
- verify that the csum (if present) is zero for any unused
  region.

Signed-off-by: Sage Weil <sage@redhat.com>
os/bluestore: use typedef for blob unused (uint16_t)
Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Apr 5, 2017

Member

pushed fixes

Member

liewegas commented Apr 5, 2017

pushed fixes

@ifed01

ifed01 approved these changes Apr 5, 2017

@liewegas liewegas merged commit 5f2db7f into ceph:master Apr 5, 2017

2 of 3 checks passed

default Build started sha1 is merged.
Details
Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details

@liewegas liewegas deleted the liewegas:wip-bluestore-fsck-unused branch Apr 5, 2017

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Apr 5, 2017

Member

thanks!

Member

liewegas commented Apr 5, 2017

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment