On 2021/11/17 18:26, Niklas Cassel wrote: > On Wed, Nov 17, 2021 at 03:52:13PM +0900, Damien Le Moal wrote: >> When iops, latency, or bw logging options are used, fio will by default >> log information for any I/O that completes. The initial number of I/O >> log entries is 1024, as defined by (DEF_LOG_ENTRIES). When all log >> entries are used, new log entries are dynamically allocated by >> get_new_log(). This dynamic log entry allocation can negatively impact >> time-related statistics such as the I/O tail latencies (e.g. 99.9 >> percentile completion latency) as growing the logs causes a temporary >> I/O stall (IO quiesce), which disturbs the workload steady state. The >> effect of this is especially noticeable with workloads using IO >> priorities: the tail latencies of high priority I/Os increase if the >> IO log needs to be grown. >> >> For example, running the following fio command on a SATA disk >> supporting NCQ priority: >> >> fio --name=prio-randread --filename=/dev/sdg \ >> --random_generator=tausworthe64 --ioscheduler=none \ >> --write_lat_log=randread.log --log_prio=1 --rw=randread --bs=128k \ >> --ioengine=libaio --iodepth=32 --direct=1 --cmdprio_class=1 ¥ > > Nit: s/¥/\ > > Republic credits? > Republic credits are no good out here. I need something more real. > (For the Star Wars fans out there..) Japanese keyboard: ¥ (Japanese Yen) and \ are the same key/character on many keyboards. [...] >> All completion percentiles clearly now show shorter latencies for high >> priority commands, as expected. The 99.99th percentile for low prirotiy > > Nit: s/prirotiy/priority Will fix. >> commands is also improved compared to the previous case as the >> measurements are not impacted by the log dynamic allocation. >> >> Suggested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@xxxxxxx> >> Signed-off-by: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx> > > Side note: > Would it perhaps be wise, in addition to this patch, > to also increase the DEF_LOG_ENTRIES value slightly? > The default value is 1024, and sizeof(struct io_sample_offset) == 40, > regrow_logs() seems (at maximum) regrow 6 different logs (slat, clat, > clat_hist_log, lat_log, bw_log, iops_log). > The default space used for a single log == 40 * 1024 = 40 KB, > if someone has enabled all 6 different logs/log types = 40 KB * 6 = 240 KB > per thread_data. Increasing DEF_LOG_ENTRIES can be done independently of this patch. the option log_entries defaults to DEF_LOG_ENTRIES and use this value as the min value too. So changing DEF_LOG_ENTRIES will also have log_entries follow the change. > While it would be hard to figure out a new default, e.g. bumping it to 32768 > would mean 40 * 32768 = 1.26 MB, if all 6 logs are enabled = 7.5 MB per > struct thread_data. Sure, someone can have thousands of threads, but I doubt > that you would have logging enabled for all those threads. > If you have logging enabled, the most common case is probably to have slat, > clat and lat logs, which means 3 different logs: > 1.26 MB * 3 = 3.76 MB per struct thread_data, however, this dynamic memory > allocation will only be done if you have explicitly enabled any of the log > files in the first place. > > TL;DR: I think that we can merge this patch AND increase the default number > of log entries to e.g. 32768. (Or if we want to be quite conservative, > perhaps only bump it to something like 16384, which would only be less than > 2 MB per struct thread_data if 3 different log files are enabled.) Not sure. On embedded systems with slow storage (e.g. emmc SD card), increasing the default log size may not be desired. DEF_LOG_ENTRIES as is seems to not be an issue for most cases. So I think we should just leave it as is for now. The new log_entries option now allows larger logs if one needs/want one. > > > > For this patch, except for two small nits in the commit message: > > Reviewed-by: Niklas Cassel <niklas.cassel@xxxxxxx> > -- Damien Le Moal Western Digital Research
