Single workload rawfio benchmark module support#355
Conversation
4c356f4 to
be3543d
Compare
Signed-off-by: Lee Sanders <ljsanders@uk.ibm.com>
be3543d to
8b47f48
Compare
| # the set for permutation | ||
| if param == "acceptable": | ||
| default[param] = value | ||
| elif param == "block_devices": |
There was a problem hiding this comment.
This looks like an extra bit of code that is required due to another change. The block_devices parameter actually takes a comma separated string of devices, or was designed to. The only reason this is needed is because it's been given a list, and the code in rawfio has been changed to handle a list.
I think that the fewer special cases we need to call out here the cleaner the code will be. Is there is a definitive reason why we require the block_devices parameter to take a list?
| pre_cmd = 'sudo %s --rw=write -ioengine=%s --bs=%s ' % (self.fio_cmd, self.ioengine, self.op_size) | ||
| pre_cmd = '%s --size %dM --name=%s --output-format=%s> /dev/null' % ( | ||
| pre_cmd, self.vol_size, fiopath, self.fio_out_format) | ||
| pre_cmd = 'sudo %s --rw=write -ioengine=%s --numjobs=1 --bs=65536 ' % (self.fio_cmd, self.ioengine) |
There was a problem hiding this comment.
Hardcoded --bs=65536 while the benchmark uses --bs=%dB % self.op_size (line 148).
| pre_cmd = 'sudo %s --rw=write -ioengine=%s --numjobs=1 --bs=65536 ' % (self.fio_cmd, self.ioengine) | |
| pre_cmd = 'sudo %s --rw=write -ioengine=%s --numjobs=1 --bs=%dB ' % (self.fio_cmd, self.ioengine, self.op_size) |
There was a problem hiding this comment.
Thank you @baum this is a good point. I think we really need to have this as a separate attribute to the op_size that is being used for the performance benchmark. So the prefill size is configurable.
I think ideally this should be similar to what we do for librbdfio.py the YAML format should be something like this:
prefill:
blocksize: '4M'
numjobs: 1
| self.block_device_list = config.get('block_devices', '/dev/vdb') | ||
| self.block_devices = [d.strip() for d in self.block_device_list.split(',')] | ||
| _block_devices = config.get('block_devices', '/dev/vdb') | ||
| if isinstance(_block_devices, list): |
There was a problem hiding this comment.
From reading the original code this is supposed to be a comma-separated string of devices. I am worried that we are adding complexity here (and in other related classes) by switching it to take a list or a string.
I think the example file shows a list, but I think that was done in error, and we should also change the example to take a comma-separated string
|
|
||
| def __init__(self, archive_dir, cluster, config): | ||
| super(RawFio, self).__init__(archive_dir, cluster, config) | ||
| cbt_logger = logging.getLogger("cbt") |
There was a problem hiding this comment.
The logger should be set up outside of the init method, generally. See librbdfio.py
It looks like here you're just trying to get rid of some messages - if you don't think they are relevant then we should do the right thing and get rid of them in the base class, not hack around them here
| self.total_procs = self.concurrent_procs * len(settings.getnodes('clients').split(',')) | ||
| self.fio_out_format = "json" | ||
| # Use json,normal so the output format matches librbdfio and parse() can extract | ||
| # the JSON block correctly from the mixed output. |
There was a problem hiding this comment.
The parse method should already cope with the JSON format, or the parse method in librbdfio.py does so I'm not sure we need this. Maybe we just need to copy the method from librbdfio for now.
Eventually we should re-factor the Benchmarks, or at least the multitude of FIO ones, so hopefully we can get rid of any copied code at that point
|
Might be an issue in my environment, but report generation on rawfio archives fails: Suggested fix: --- a/post_processing/run_results/run_result_factory.py
+++ b/post_processing/run_results/run_result_factory.py
@@ -64,6 +64,11 @@ def get_run_result_from_directory_name(directory: Path, file_name_root: str) ->
log.debug("Creating %s result processor for directory %s", result_class.__name__, directory)
return result_class(directory=directory, file_name_root=file_name_root)
+ # rawfio archives: .../randwrite/iodepth-NNN/ (no rawfio subdirectory)
+ if any(p.is_dir() and p.name.startswith("iodepth-") for p in directory.iterdir()):
+ log.debug("Inferring rawfio from iodepth-* layout under %s", directory)
+ return BENCHMARK_TYPE_MAP["rawfio"](directory=directory, file_name_root=file_name_root)
+
raise NotImplementedError(f"Could not determine benchmark type from directory {directory}, ")
Tested on m1 NVMe-oF smoke archive (CBT @ 8b47f48). Full patch: cbt_artifacts/lee_handoff/rawfio-run-result-factory.patch. |
3 participants
This PR adds support to the rawfio benchmark module to be able to run a single performance benchmark to multiple raw devices (such as a KRBD or NVME raw block device).
There were multiple issues in the original implementation that have now been addressed.
You will be able to use the generate_performance_report.py and generate_comparison_performance_report.py tool to post process the data.
There is an example YAML supplied in the PR.