Skip to content
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: Some more plumbing for zone cleaning (WIP) #38641

Merged
merged 14 commits into from Apr 24, 2021

Conversation

agayev
Copy link
Contributor

@agayev agayev commented Dec 17, 2020

Checklist

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug

Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@agayev
Copy link
Contributor Author

agayev commented Dec 17, 2020

@ifed01 Would appreciate if you could take a look. This is not complete yet, but does this make sense so far? I'm not sure about calling db->submit_transaction from ZonedFreelistManager::zoned_mark_zone...

@agayev
Copy link
Contributor Author

agayev commented Jan 15, 2021

@ifed01 Would appreciate if you could take a look. This is not complete yet, but does this make sense so far? I'm not sure about calling db->submit_transaction from ZonedFreelistManager::zoned_mark_zone...

Hi @ifed01 -- I hope you had a great winter break. Would appreciate if you could take a look at this when you get a chance. Thanks!

Copy link
Contributor

@ifed01 ifed01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides other comments I have a strong desire to simplify the logic of zones cleaning. Can zoned_cleaner_thread access the allocator directly and retrieve a list of zones to clean by itself? Hence avoiding all the tricks to pass the list from do_allocate_write to the thread?

src/os/bluestore/ZonedAllocator.cc Outdated Show resolved Hide resolved
src/os/bluestore/ZonedAllocator.cc Outdated Show resolved Hide resolved
src/os/bluestore/ZonedAllocator.cc Outdated Show resolved Hide resolved
src/os/bluestore/ZonedAllocator.cc Show resolved Hide resolved
src/os/bluestore/ZonedAllocator.cc Show resolved Hide resolved
src/os/bluestore/ZonedFreelistManager.cc Outdated Show resolved Hide resolved
src/os/bluestore/ZonedAllocator.cc Outdated Show resolved Hide resolved
src/os/bluestore/ZonedAllocator.cc Outdated Show resolved Hide resolved
@agayev
Copy link
Contributor Author

agayev commented Jan 28, 2021

Besides other comments I have a strong desire to simplify the logic of zones cleaning. Can zoned_cleaner_thread access the allocator directly and retrieve a list of zones to clean by itself? Hence avoiding all the tricks to pass the list from do_allocate_write to the thread?

Writes happen in do_allocate_write. That's the only place to detect when to start cleaning. The alternative is for the cleaner thread to poll the allocator every second and start cleaning if there is a need. This can be simpler if you want to go that route.

@agayev
Copy link
Contributor Author

agayev commented Feb 24, 2021

@ifed01 Would appreciate if you could take a look. Thanks!

@agayev
Copy link
Contributor Author

agayev commented Mar 3, 2021

Hi @ifed01 are you planning to look at this?

@ifed01
Copy link
Contributor

ifed01 commented Mar 5, 2021

Hi @ifed01 are you planning to look at this?

yeah, will do this week.

Sorry for delays....

src/os/bluestore/BlueStore.cc Outdated Show resolved Hide resolved
src/os/bluestore/ZonedAllocator.cc Outdated Show resolved Hide resolved
@ifed01
Copy link
Contributor

ifed01 commented Mar 9, 2021

Besides other comments I have a strong desire to simplify the logic of zones cleaning. Can zoned_cleaner_thread access the allocator directly and retrieve a list of zones to clean by itself? Hence avoiding all the tricks to pass the list from do_allocate_write to the thread?

Writes happen in do_allocate_write. That's the only place to detect when to start cleaning. The alternative is for the cleaner thread to poll the allocator every second and start cleaning if there is a need. This can be simpler if you want to go that route.

May be something like this:
Zoned allocator is provided with the conditional it notifies on when there are zones to clean. It's up to allocator when to trigger the notification. It can be on extent allocation, release, whatever...
zoned_cleaner_thread wakes on this conditional and retrieves list of zones to clean directly from the allocator.
Hence do_allocate_write is completely out of the path.

Moreover it looks like ZonedAllocator and zoned_cleaner_thread are doing cleanup in-sync and hence ZonedAllocator::cleaning_in_progress_zones container is a sole and static set of zones to clean at a given point of time. Hence there is no need to make any copies of it - the thread can directly use it and pass back the completion indication in a single call to the allocator.

@agayev
Copy link
Contributor Author

agayev commented Mar 23, 2021

Besides other comments I have a strong desire to simplify the logic of zones cleaning. Can zoned_cleaner_thread access the allocator directly and retrieve a list of zones to clean by itself? Hence avoiding all the tricks to pass the list from do_allocate_write to the thread?

Writes happen in do_allocate_write. That's the only place to detect when to start cleaning. The alternative is for the cleaner thread to poll the allocator every second and start cleaning if there is a need. This can be simpler if you want to go that route.

May be something like this:
Zoned allocator is provided with the conditional it notifies on when there are zones to clean. It's up to allocator when to trigger the notification. It can be on extent allocation, release, whatever...
zoned_cleaner_thread wakes on this conditional and retrieves list of zones to clean directly from the allocator.
Hence do_allocate_write is completely out of the path.

Moreover it looks like ZonedAllocator and zoned_cleaner_thread are doing cleanup in-sync and hence ZonedAllocator::cleaning_in_progress_zones container is a sole and static set of zones to clean at a given point of time. Hence there is no need to make any copies of it - the thread can directly use it and pass back the completion indication in a single call to the allocator.

@ifed01 Please take a look. I tried to simplify it based on your description. Thanks.

src/os/bluestore/Allocator.h Outdated Show resolved Hide resolved
src/os/bluestore/FreelistManager.h Outdated Show resolved Hide resolved
src/os/bluestore/BlueStore.cc Outdated Show resolved Hide resolved
@agayev
Copy link
Contributor Author

agayev commented Apr 1, 2021

jenkins make check

@agayev
Copy link
Contributor Author

agayev commented Apr 1, 2021

jenkins test make check

@agayev
Copy link
Contributor Author

agayev commented Apr 1, 2021

@ifed01 @tchaikov I don't see the following error when building on my system with gcc. Do test machines use a different compiler?

/home/jenkins-build/build/workspace/ceph-api/build/../src/os/bluestore/BlueStore.cc:5442: undefined reference to typeinfo for ZonedAllocator' /usr/bin/ld: /home/jenkins-build/build/workspace/ceph-api/build/../src/os/bluestore/BlueStore.cc:5443: undefined reference to typeinfo for ZonedFreelistManager'
/usr/bin/ld: /home/jenkins-build/build/workspace/ceph-api/build/../src/os/bluestore/BlueStore.cc:5444: undefined reference to ZonedFreelistManager::zoned_get_zone_states(KeyValueDB*) const' /usr/bin/ld: /home/jenkins-build/build/workspace/ceph-api/build/../src/os/bluestore/BlueStore.cc:5444: undefined reference to ZonedAllocator::zoned_init_alloc(std::vector<zone_state_t, std::allocator<zone_state_t> >&&, std::mutex*, std::condition_variable*)'

@ifed01
Copy link
Contributor

ifed01 commented Apr 1, 2021

@ifed01 @tchaikov I don't see the following error when building on my system with gcc. Do test machines use a different compiler?

/home/jenkins-build/build/workspace/ceph-api/build/../src/os/bluestore/BlueStore.cc:5442: undefined reference to typeinfo for ZonedAllocator' /usr/bin/ld: /home/jenkins-build/build/workspace/ceph-api/build/../src/os/bluestore/BlueStore.cc:5443: undefined reference to typeinfo for ZonedFreelistManager'
/usr/bin/ld: /home/jenkins-build/build/workspace/ceph-api/build/../src/os/bluestore/BlueStore.cc:5444: undefined reference to ZonedFreelistManager::zoned_get_zone_states(KeyValueDB*) const' /usr/bin/ld: /home/jenkins-build/build/workspace/ceph-api/build/../src/os/bluestore/BlueStore.cc:5444: undefined reference to ZonedAllocator::zoned_init_alloc(std::vector<zone_state_t, std::allocator<zone_state_t> >&&, std::mutex*, std::condition_variable*)'

In my lab this failed to build due to undefined WITH_ZBD.

@agayev
Copy link
Contributor Author

agayev commented Apr 1, 2021

@ifed01 @tchaikov I don't see the following error when building on my system with gcc. Do test machines use a different compiler?
/home/jenkins-build/build/workspace/ceph-api/build/../src/os/bluestore/BlueStore.cc:5442: undefined reference to typeinfo for ZonedAllocator' /usr/bin/ld: /home/jenkins-build/build/workspace/ceph-api/build/../src/os/bluestore/BlueStore.cc:5443: undefined reference to typeinfo for ZonedFreelistManager'
/usr/bin/ld: /home/jenkins-build/build/workspace/ceph-api/build/../src/os/bluestore/BlueStore.cc:5444: undefined reference to ZonedFreelistManager::zoned_get_zone_states(KeyValueDB*) const' /usr/bin/ld: /home/jenkins-build/build/workspace/ceph-api/build/../src/os/bluestore/BlueStore.cc:5444: undefined reference to ZonedAllocator::zoned_init_alloc(std::vector<zone_state_t, std::allocator<zone_state_t> >&&, std::mutex*, std::condition_variable*)'

In my lab this failed to build due to undefined WITH_ZBD.

Oh, that's because you don't have libzbd installed (https://github.com/westerndigitalcorporation/libzbd). It is installed in the build servers.

src/os/bluestore/BlueStore.cc Show resolved Hide resolved
src/os/bluestore/BlueStore.cc Outdated Show resolved Hide resolved
src/os/bluestore/Allocator.h Outdated Show resolved Hide resolved
Copy link
Contributor

@ifed01 ifed01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good except the assertion in ZonedAllocator.

Could you please squash the commits in a more sensible way prior to the merge?

src/os/bluestore/ZonedAllocator.cc Outdated Show resolved Hide resolved
ZonedFreelistManager to mark a zone free.

Signed-off-by: Abutalib Aghayev <agayev@psu.edu>
… the number of dead bytes of a zone on which the released extents are located.

Signed-off-by: Abutalib Aghayev <agayev@psu.edu>
Signed-off-by: Abutalib Aghayev <agayev@psu.edu>
Signed-off-by: Abutalib Aghayev <agayev@psu.edu>
…on parameter.

Signed-off-by: Abutalib Aghayev <agayev@psu.edu>
Signed-off-by: Abutalib Aghayev <agayev@psu.edu>
…at are

currently being cleaned; also decrement free space after an allocation.

Signed-off-by: Abutalib Aghayev <agayev@psu.edu>
Signed-off-by: Abutalib Aghayev <agayev@psu.edu>
@agayev
Copy link
Contributor Author

agayev commented Apr 14, 2021

@ifed01 Please take a look. It had a quite a bit of history so I couldn't squash them nicely, but gradually reapplied the whole diff in small commits that indvidually make sense.

@agayev
Copy link
Contributor Author

agayev commented Apr 15, 2021

@tchaikov @ifed01 ceph API tests fail because the script that runs it does not specify -DWITH_ZBD=ON. I've created a PR to fix that: ceph/ceph-build#1802

@tchaikov
Copy link
Contributor

jenkins test api

@tchaikov
Copy link
Contributor

@tchaikov @ifed01 ceph API tests fail because the script that runs it does not specify -DWITH_ZBD=ON. I've created a PR to fix that: ceph/ceph-build#1802

should have been fixed by #40873

@agayev
Copy link
Contributor Author

agayev commented Apr 15, 2021

@tchaikov not sure that it will but let's see.

@agayev
Copy link
Contributor Author

agayev commented Apr 15, 2021

@tchaikov it didn't. This should fix it, I verified locally.

Signed-off-by: Abutalib Aghayev <agayev@psu.edu>
Copy link
Contributor

@ifed01 ifed01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, just a few minor suggestions to cleanup logging

src/os/bluestore/ZonedAllocator.cc Outdated Show resolved Hide resolved
src/os/bluestore/ZonedAllocator.cc Outdated Show resolved Hide resolved
src/os/bluestore/ZonedAllocator.cc Outdated Show resolved Hide resolved
src/os/bluestore/ZonedAllocator.cc Outdated Show resolved Hide resolved
src/os/bluestore/BlueStore.cc Show resolved Hide resolved
Signed-off-by: Abutalib Aghayev <agayev@psu.edu>
@agayev
Copy link
Contributor Author

agayev commented Apr 23, 2021

jenkins test docs

@agayev
Copy link
Contributor Author

agayev commented Apr 23, 2021

@tchaikov can you please merge it? Thanks!

@tchaikov
Copy link
Contributor

@agayev i'd prefer running this change against the rados suite before merging it.

@agayev
Copy link
Contributor Author

agayev commented Apr 23, 2021

@agayev i'd prefer running this change against the rados suite before merging it.

Sounds good. Thanks!

@tchaikov tchaikov merged commit 9827ee5 into ceph:master Apr 24, 2021
@agayev agayev deleted the zoned-cleaner branch May 27, 2021 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants