-
Notifications
You must be signed in to change notification settings - Fork 141
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
Adding a total_iodepth option for librbdfio #330
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Chris Harris([email protected])
python unit tests: ============================= slowest 5 durations ============================== I haven't run black, ruff or mypy against the file to check for pep-8 compliance as the unchanged file gives too many errors currently |
Testing with workloads:precondition: iodepth=[2]13:45:31 - INFO - cbt - Running rbd fio precondition test, mode randwrite 13:45:31 - INFO - cbt - CHDEBUG: fio command for iodepth 2 and vol 0 is 13:45:31 - INFO - cbt - CHDEBUG: fio command for iodepth 2 and vol 1 is 13:45:31 - INFO - cbt - CHDEBUG: fio command for iodepth 2 and vol 7 is The procondition runs on all 8 vlumes with an iodepth of 2, as expected. total_iodepth=16, volumes=813:45:31 - INFO - cbt - Running rbd fio seq32kwrite test, mode write 13:45:31 - INFO - cbt - CHDEBUG: fio command for iodepth 16 and vol 0 is 13:45:31 - INFO - cbt - CHDEBUG: fio command for iodepth 16 and vol 1 is total_iodepth=7, volumes=813:45:32 - WARNING - cbt - The total iodepth requested: 7 is less than 1 per volume (8) 13:45:32 - INFO - cbt - CHDEBUG: fio command for iodepth 7 and vol 0 is |
Farther testing: Using the following yaml (workload section below:
gives (extra debug added to show values used:
Some farther testing to make sure that the iodepth value used in the results directory structure is the total iodepth value or iodepth value, depending on which one is set.
which is as expected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestions added to improve readability and maintanability
@@ -50,6 +49,13 @@ def __init__(self, archive_dir, cluster, config): | |||
self.rate_iops = config.get('rate_iops', None) | |||
self.fio_out_format = config.get('fio_out_format', 'json,normal') | |||
self.data_pool = None | |||
|
|||
iodepth_key: str = self._get_iodepth_key(config.keys()) # type: ignore[arg-type] | |||
self.iodepth: str = config.get(iodepth_key, "16") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you prefer to get a string rather than a numeric value? How do you validate that is a valid iodepth?
iodepth_key: str = self._get_iodepth_key(config.keys()) # type: ignore[arg-type] | ||
self.iodepth: str = config.get(iodepth_key, "16") | ||
self._iodepth_per_volume: dict[int, int] = self._calculate_iodepth_per_volume( | ||
self.volumes_per_client, int(self.iodepth), iodepth_key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if self.iodepth is not a valid numeric string? Do we catch an exception somewhere? Notice how we use the config.get() method from the .yaml, with an alternative initial numeric value if it does not convert to a valid value.
|
||
return iodepth_key | ||
|
||
def _calculate_iodepth_per_volume(self, number_of_volumes: int, iodepth: int, iodepth_key: str) -> dict[int, int]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably best to update the member attribute within the method, rather than returning a dict that is only consumed by such an attribute. Furthermore, such method can be simplified to expect only the two arguments that are not in self already.
""" | ||
iodepth_key: str = "iodepth" | ||
if "total_iodepth" in configuration_keys: | ||
iodepth_key = "total_iodepth" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use a dictionary to give that key instead
@@ -234,7 +245,8 @@ def run(self): | |||
monitoring.start(self.run_dir) | |||
logger.info('Running rbd fio %s test.', self.mode) | |||
ps = [] | |||
for i in range(self.volumes_per_client): | |||
number_of_volumes: int = len(self._iodepth_per_volume.keys()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why to have the iodepth as string, then convert them back to numeric? Not see the value
return self._calculate_iodepth_per_volume_from_total_iodepth(number_of_volumes, iodepth) | ||
else: | ||
return self._set_iodepth_for_every_volume(number_of_volumes, iodepth) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can also be further simplified (and probably generalised) using a simple dictionary: if you have a single volume, you can have a single iodepth (which is the same as total_iodepth), otherwise calculate the iodepth per volume (which can either be an arryay, or a dict: vol->iodepth).
And that's it, no need to several cases that obscure the logic.
if remainder > 0: | ||
iodepth += 1 | ||
remainder -= 1 | ||
queue_depths[volume_id] = iodepth |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's what I mean in the previous comment. queue_depths[] can by an attribute of this object instance (eg. self.vol_queue_depths). This idea can be generalised further: you can have a dict (or any collection object that allows iterators) to have several options per volume, besides iodepth, controlled by a single global flag. In this way you can have all volumes with the same iodepth, or distributed as per your requirement, etc.
You might also need to run update the unit test generator to pick up this new attribute though.
|
||
return queue_depths |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be inclined to this be an attribute (eg. self.vol_queue_depths) as suggested above, so this method is no longer a function but an update member method.
This is a continuation of PR 324
Description
Add an option to the CBT configuration yaml file called total_iodepth. The total_iodepth is then split evenly among the number of volumes used for the test. If the total_iodepth for a run does not divide evenly into the number of volumes, then any remainder will be assigned 1iodepth at a time to the volumes, starting at 0.
For (a simple) example:
For an total_iodepth of 18 and volumes_per_client of 5, the following iodepth allocations would occur:
If the number of volumes specified is such that there is not enough iodepth for 1 per volume, then the number of volumes for that test will be reduced so that an iodepth of 1 per volume can be achieved.
Example:
For volumes_per_client = 5 and total_iodepth=4, the benchmark would be run with 4 volumes, each of iodepth 1
Testing
Manual testing was done for a number of scenarios, with and without using workloads The below are the output from debug statements added for the purposes of testing.
Regression testing: iodepth=32, volumes_per_client=3
CHDEBUG: fio_cmd for volume 0 is /usr/local/bin/fio --ioengine=rbd --clientname=admin --pool=cbt-librbdfio --rbdname=cbt-librbdfio-
hostname -f
-0 --invalidate=0 --rw=randwrite --output-format=json,normal --runtime=300 --numjobs=1 --direct=1 --bs=4096B --iodepth=32 --end_fsync=0 --norandommap --write_iops_log=/tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-003/iodepth-032/randwrite/output.0 --write_bw_log=/tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-003/iodepth-032/randwrite/output.0 --write_lat_log=/tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-003/iodepth-032/randwrite/output.0 --log_avg_msec=101 --name=cbt-librbdfio-hostname -f
-file-0 > /tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-003/iodepth-032/randwrite/output.0CHDEBUG: fio_cmd for volume 1 is /usr/local/bin/fio --ioengine=rbd --clientname=admin --pool=cbt-librbdfio --rbdname=cbt-librbdfio-
hostname -f
-1 --invalidate=0 --rw=randwrite --output-format=json,normal --runtime=300 --numjobs=1 --direct=1 --bs=4096B --iodepth=32 --end_fsync=0 --norandommap --write_iops_log=/tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-003/iodepth-032/randwrite/output.1 --write_bw_log=/tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-003/iodepth-032/randwrite/output.1 --write_lat_log=/tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-003/iodepth-032/randwrite/output.1 --log_avg_msec=101 --name=cbt-librbdfio-hostname -f
-file-0 > /tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-003/iodepth-032/randwrite/output.1CHDEBUG: fio_cmd for volume 2 is /usr/local/bin/fio --ioengine=rbd --clientname=admin --pool=cbt-librbdfio --rbdname=cbt-librbdfio-
hostname -f
-2 --invalidate=0 --rw=randwrite --output-format=json,normal --runtime=300 --numjobs=1 --direct=1 --bs=4096B --iodepth=32 --end_fsync=0 --norandommap --write_iops_log=/tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-003/iodepth-032/randwrite/output.2 --write_bw_log=/tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-003/iodepth-032/randwrite/output.2 --write_lat_log=/tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-003/iodepth-032/randwrite/output.2 --log_avg_msec=101 --name=cbt-librbdfio-hostname -f
-file-0 > /tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-003/iodepth-032/randwrite/output.2total_iodepth=32, volumes_per_client=3
CHDEBUG: fio_cmd for volume 0 is /usr/local/bin/fio --ioengine=rbd --clientname=admin --pool=cbt-librbdfio --rbdname=cbt-librbdfio-
hostname -f
-0 --invalidate=0 --rw=randwrite --output-format=json,normal --runtime=300 --numjobs=1 --direct=1 --bs=4096B --iodepth=11 --end_fsync=0 --norandommap --write_iops_log=/tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-003/iodepth-016/randwrite/output.0 --write_bw_log=/tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-003/iodepth-016/randwrite/output.0 --write_lat_log=/tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-003/iodepth-016/randwrite/output.0 --log_avg_msec=101 --name=cbt-librbdfio-hostname -f
-file-0 > /tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-003/iodepth-016/randwrite/output.0CHDEBUG: fio_cmd for volume 1 is /usr/local/bin/fio --ioengine=rbd --clientname=admin --pool=cbt-librbdfio --rbdname=cbt-librbdfio-
hostname -f
-1 --invalidate=0 --rw=randwrite --output-format=json,normal --runtime=300 --numjobs=1 --direct=1 --bs=4096B --iodepth=11 --end_fsync=0 --norandommap --write_iops_log=/tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-003/iodepth-016/randwrite/output.1 --write_bw_log=/tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-003/iodepth-016/randwrite/output.1 --write_lat_log=/tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-003/iodepth-016/randwrite/output.1 --log_avg_msec=101 --name=cbt-librbdfio-hostname -f
-file-0 > /tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-003/iodepth-016/randwrite/output.1CHDEBUG: fio_cmd for volume 2 is /usr/local/bin/fio --ioengine=rbd --clientname=admin --pool=cbt-librbdfio --rbdname=cbt-librbdfio-
hostname -f
-2 --invalidate=0 --rw=randwrite --output-format=json,normal --runtime=300 --numjobs=1 --direct=1 --bs=4096B --iodepth=10 --end_fsync=0 --norandommap --write_iops_log=/tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-003/iodepth-016/randwrite/output.2 --write_bw_log=/tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-003/iodepth-016/randwrite/output.2 --write_lat_log=/tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-003/iodepth-016/randwrite/output.2 --log_avg_msec=101 --name=cbt-librbdfio-hostname -f
-file-0 > /tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-003/iodepth-016/randwrite/output.2total_iodepth=2, volumes_per_client=3
11:13:28 - WARNING - cbt - The total iodepth requested: 2 is less than 1 per volume.
11:13:28 - WARNING - cbt - Number of volumes per client will be reduced from 3 to 2
CHDEBUG: fio_cmd for volume 0 is /usr/local/bin/fio --ioengine=rbd --clientname=admin --pool=cbt-librbdfio --rbdname=cbt-librbdfio-
hostname -f
-0 --invalidate=0 --rw=randwrite --output-format=json,normal --runtime=300 --numjobs=1 --direct=1 --bs=4096B --iodepth=1 --end_fsync=0 --norandommap --write_iops_log=/tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-002/iodepth-016/randwrite/output.0 --write_bw_log=/tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-002/iodepth-016/randwrite/output.0 --write_lat_log=/tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-002/iodepth-016/randwrite/output.0 --log_avg_msec=101 --name=cbt-librbdfio-hostname -f
-file-0 > /tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-002/iodepth-016/randwrite/output.0CHDEBUG: fio_cmd for volume 1 is /usr/local/bin/fio --ioengine=rbd --clientname=admin --pool=cbt-librbdfio --rbdname=cbt-librbdfio-
hostname -f
-1 --invalidate=0 --rw=randwrite --output-format=json,normal --runtime=300 --numjobs=1 --direct=1 --bs=4096B --iodepth=1 --end_fsync=0 --norandommap --write_iops_log=/tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-002/iodepth-016/randwrite/output.1 --write_bw_log=/tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-002/iodepth-016/randwrite/output.1 --write_lat_log=/tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-002/iodepth-016/randwrite/output.1 --log_avg_msec=101 --name=cbt-librbdfio-hostname -f
-file-0 > /tmp/cbt/00000000/LibrbdFio/osd_ra-00004096/op_size-00004096/concurrent_procs-002/iodepth-016/randwrite/output.1I'll update with teuthology results once they have run