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

[experimental]Set cleaning policy #27

Open
wants to merge 1 commit into
base: experimental
Choose a base branch
from

Conversation

rafalste
Copy link

Add RPC methods for changing cleaning policy and parameters:

  • bdev_ocf_set_cleaning_alru
  • bdev_ocf_set_cleaning_acp
  • bdev_ocf_set_cleaning_nop

Signed-off-by: Rafal Stefanowski [email protected]

Add RPC methods for changing cleaning policy and parameters:
- bdev_ocf_set_cleaning_alru
- bdev_ocf_set_cleaning_acp
- bdev_ocf_set_cleaning_nop

Signed-off-by: Rafal Stefanowski <[email protected]>
@pep8speaks
Copy link

Hello @rafalste! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 348:80: E501 line too long (90 > 79 characters)
Line 349:80: E501 line too long (96 > 79 characters)
Line 350:80: E501 line too long (99 > 79 characters)
Line 352:80: E501 line too long (98 > 79 characters)
Line 355:80: E501 line too long (107 > 79 characters)
Line 357:80: E501 line too long (105 > 79 characters)
Line 358:80: E501 line too long (86 > 79 characters)
Line 360:80: E501 line too long (116 > 79 characters)
Line 362:80: E501 line too long (111 > 79 characters)
Line 369:80: E501 line too long (96 > 79 characters)
Line 371:80: E501 line too long (97 > 79 characters)
Line 374:80: E501 line too long (101 > 79 characters)
Line 376:80: E501 line too long (121 > 79 characters)
Line 383:80: E501 line too long (81 > 79 characters)

Line 238:80: E501 line too long (81 > 79 characters)
Line 239:80: E501 line too long (84 > 79 characters)
Line 240:80: E501 line too long (84 > 79 characters)

@mmkayPL mmkayPL changed the title Set cleaning policy [experimental]Set cleaning policy Jun 14, 2021
p = subparsers.add_parser('bdev_ocf_set_cleaning_alru',
help='Set ALRU cleaning policy with parameters on OCF cache device')
p.add_argument('name', help='Name of OCF bdev')
p.add_argument('-w', '--wake-up', type=int, default=-1,
Copy link

Choose a reason for hiding this comment

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

I think it might be better to set the default implicitly to 20 here (and other default values too) and not base it on defaults in OCF? In that case changing the type to uint32 would also be useful, I think.

Copy link
Author

Choose a reason for hiding this comment

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

I think there are two problems with your proposed approach:

  1. SPDK maintainers prefer to be as independent as possible from any hard-coded values. This way, when we change OCF defaults in the future, there will be no need to update them also in SPDK adapter.
  2. Some of those parameters takes a range from 0 to some value, so the default=-1 informs of no change to that particular parameter. I think it will be much more obfuscated to handle this situation using different type of variable here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants