[three fio patches 0/3]

[three fio patches 0/3]

[vincentfu]

From: Vincent Fu <vincent.fu@wdc.com>

Jens, please consider this series of patches. They do the following:

- fix a smalloc synchronization bug
- allow offset_increment to understand percentages
- fix minor documentation issues

Vincent


Vincent Fu (3):
  docs: small HOWTO fixes
  options: allow offset_increment to understand percentages
  smalloc: allocate pool-> members from shared memory

 HOWTO            | 12 ++++++++++--
 cconv.c          |  2 ++
 filesetup.c      | 32 ++++++++++++++++++++++----------
 fio.1            |  4 +++-
 options.c        | 17 +++++++++++++++++
 server.h         |  2 +-
 smalloc.c        | 36 ++++++++++++++++++++----------------
 thread_options.h |  3 +++
 8 files changed, 78 insertions(+), 30 deletions(-)

-- 
2.17.1

[three fio patches 1/3] docs: small HOWTO fixes

[vincentfu]

From: Vincent Fu <vincent.fu@wdc.com>

Document three debug options and fix a formatting error.
---
 HOWTO | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/HOWTO b/HOWTO
index 41b959d1..1c9b2c10 100644
--- a/HOWTO
+++ b/HOWTO
@@ -93,6 +93,12 @@ Command line options
 			Dump info related to I/O rate switching.
 	*compress*
 			Dump info related to log compress/decompress.
+	*steadystate*
+			Dump info related to steadystate detection.
+	*helperthread*
+			Dump info related to the helper thread.
+	*zbd*
+			Dump info related to support for zoned block devices.
 	*?* or *help*
 			Show available debug options.
 
@@ -2388,7 +2394,7 @@ I/O depth
 	this option can reduce both performance and the :option:`iodepth` achieved.
 
 	This option only applies to I/Os issued for a single job except when it is
-	enabled along with :option:`io_submit_mode`=offload. In offload mode, fio
+	enabled along with :option:`io_submit_mode`\=offload. In offload mode, fio
 	will check for overlap among all I/Os submitted by offload jobs with :option:`serialize_overlap`
 	enabled.
 
-- 
2.17.1

[three fio patches 2/3] options: allow offset_increment to understand percentages

[vincentfu]

From: Vincent Fu <vincent.fu@wdc.com>

Add the ability for the offset_increment option to understand
percentages. With this patch offset_increment=10% will, for example,
increase the start offset by 10% of the file size for additional jobs
created by numjobs.
---
 HOWTO            |  4 +++-
 cconv.c          |  2 ++
 filesetup.c      | 32 ++++++++++++++++++++++----------
 fio.1            |  4 +++-
 options.c        | 17 +++++++++++++++++
 server.h         |  2 +-
 thread_options.h |  3 +++
 7 files changed, 51 insertions(+), 13 deletions(-)

diff --git a/HOWTO b/HOWTO
index 1c9b2c10..4201e2e9 100644
--- a/HOWTO
+++ b/HOWTO
@@ -1252,7 +1252,9 @@ I/O type
 	is incremented for each sub-job (i.e. when :option:`numjobs` option is
 	specified). This option is useful if there are several jobs which are
 	intended to operate on a file in parallel disjoint segments, with even
-	spacing between the starting points.
+	spacing between the starting points. Percentages can be used for this option.
+	If a percentage is given, the generated offset will be aligned to the minimum
+	``blocksize`` or to the value of ``offset_align`` if provided.
 
 .. option:: number_ios=int
 
diff --git a/cconv.c b/cconv.c
index 0e657246..bff5e34f 100644
--- a/cconv.c
+++ b/cconv.c
@@ -226,6 +226,7 @@ void convert_thread_options_to_cpu(struct thread_options *o,
 	o->zone_skip = le64_to_cpu(top->zone_skip);
 	o->zone_mode = le32_to_cpu(top->zone_mode);
 	o->lockmem = le64_to_cpu(top->lockmem);
+	o->offset_increment_percent = le32_to_cpu(top->offset_increment_percent);
 	o->offset_increment = le64_to_cpu(top->offset_increment);
 	o->number_ios = le64_to_cpu(top->number_ios);
 
@@ -566,6 +567,7 @@ void convert_thread_options_to_net(struct thread_options_pack *top,
 	top->start_offset_align = __cpu_to_le64(o->start_offset_align);
 	top->start_offset_percent = __cpu_to_le32(o->start_offset_percent);
 	top->trim_backlog = __cpu_to_le64(o->trim_backlog);
+	top->offset_increment_percent = __cpu_to_le32(o->offset_increment_percent);
 	top->offset_increment = __cpu_to_le64(o->offset_increment);
 	top->number_ios = __cpu_to_le64(o->number_ios);
 	top->rate_process = cpu_to_le32(o->rate_process);
diff --git a/filesetup.c b/filesetup.c
index 57eca1bf..7904d187 100644
--- a/filesetup.c
+++ b/filesetup.c
@@ -852,16 +852,37 @@ static unsigned long long get_fs_free_counts(struct thread_data *td)
 
 uint64_t get_start_offset(struct thread_data *td, struct fio_file *f)
 {
+	bool align = false;
 	struct thread_options *o = &td->o;
 	unsigned long long align_bs;
 	unsigned long long offset;
+	unsigned long long increment;
 
 	if (o->file_append && f->filetype == FIO_TYPE_FILE)
 		return f->real_file_size;
 
+	if (o->offset_increment_percent) {
+		assert(!o->offset_increment);
+		increment = o->offset_increment_percent * f->real_file_size / 100;
+		align = true;
+	} else
+		increment = o->offset_increment;
+
 	if (o->start_offset_percent > 0) {
+		/* calculate the raw offset */
+		offset = (f->real_file_size * o->start_offset_percent / 100) +
+			(td->subjob_number * increment);
+
+		align = true;
+	} else {
+		/* start_offset_percent not set */
+		offset = o->start_offset +
+				td->subjob_number * increment;
+	}
+
+	if (align) {
 		/*
-		 * if offset_align is provided, set initial offset
+		 * if offset_align is provided, use it
 		 */
 		if (fio_option_is_set(o, start_offset_align)) {
 			align_bs = o->start_offset_align;
@@ -870,20 +891,11 @@ uint64_t get_start_offset(struct thread_data *td, struct fio_file *f)
 			align_bs = td_min_bs(td);
 		}
 
-		/* calculate the raw offset */
-		offset = (f->real_file_size * o->start_offset_percent / 100) +
-			(td->subjob_number * o->offset_increment);
-
 		/*
 		 * block align the offset at the next available boundary at
 		 * ceiling(offset / align_bs) * align_bs
 		 */
 		offset = (offset / align_bs + (offset % align_bs != 0)) * align_bs;
-
-	} else {
-		/* start_offset_percent not set */
-		offset = o->start_offset +
-				td->subjob_number * o->offset_increment;
 	}
 
 	return offset;
diff --git a/fio.1 b/fio.1
index 97371d77..a06a12da 100644
--- a/fio.1
+++ b/fio.1
@@ -1015,7 +1015,9 @@ If this is provided, then the real offset becomes `\fBoffset\fR + \fBoffset_incr
 is incremented for each sub\-job (i.e. when \fBnumjobs\fR option is
 specified). This option is useful if there are several jobs which are
 intended to operate on a file in parallel disjoint segments, with even
-spacing between the starting points.
+spacing between the starting points. Percentages can be used for this option.
+If a percentage is given, the generated offset will be aligned to the minimum
+\fBblocksize\fR or to the value of \fBoffset_align\fR if provided.
 .TP
 .BI number_ios \fR=\fPint
 Fio will normally perform I/Os until it has exhausted the size of the region
diff --git a/options.c b/options.c
index 447f231e..2c5bf5e0 100644
--- a/options.c
+++ b/options.c
@@ -1434,6 +1434,22 @@ static int str_offset_cb(void *data, unsigned long long *__val)
 	return 0;
 }
 
+static int str_offset_increment_cb(void *data, unsigned long long *__val)
+{
+	struct thread_data *td = cb_data_to_td(data);
+	unsigned long long v = *__val;
+
+	if (parse_is_percent(v)) {
+		td->o.offset_increment = 0;
+		td->o.offset_increment_percent = -1ULL - v;
+		dprint(FD_PARSE, "SET offset_increment_percent %d\n",
+					td->o.offset_increment_percent);
+	} else
+		td->o.offset_increment = v;
+
+	return 0;
+}
+
 static int str_size_cb(void *data, unsigned long long *__val)
 {
 	struct thread_data *td = cb_data_to_td(data);
@@ -2084,6 +2100,7 @@ struct fio_option fio_options[FIO_MAX_OPTS] = {
 		.name	= "offset_increment",
 		.lname	= "IO offset increment",
 		.type	= FIO_OPT_STR_VAL,
+		.cb	= str_offset_increment_cb,
 		.off1	= offsetof(struct thread_options, offset_increment),
 		.help	= "What is the increment from one offset to the next",
 		.parent = "offset",
diff --git a/server.h b/server.h
index ac713252..de1d7f9b 100644
--- a/server.h
+++ b/server.h
@@ -48,7 +48,7 @@ struct fio_net_cmd_reply {
 };
 
 enum {
-	FIO_SERVER_VER			= 79,
+	FIO_SERVER_VER			= 80,
 
 	FIO_SERVER_MAX_FRAGMENT_PDU	= 1024,
 	FIO_SERVER_MAX_CMD_MB		= 2048,
diff --git a/thread_options.h b/thread_options.h
index 14c6969f..ee6e4d6d 100644
--- a/thread_options.h
+++ b/thread_options.h
@@ -313,6 +313,7 @@ struct thread_options {
 	int flow_watermark;
 	unsigned int flow_sleep;
 
+	unsigned int offset_increment_percent;
 	unsigned long long offset_increment;
 	unsigned long long number_ios;
 
@@ -599,6 +600,8 @@ struct thread_options_pack {
 	int32_t flow_watermark;
 	uint32_t flow_sleep;
 
+	uint32_t offset_increment_percent;
+	uint32_t pad4;
 	uint64_t offset_increment;
 	uint64_t number_ios;
 
-- 
2.17.1

[three fio patches 3/3] smalloc: allocate pool-> members from shared memory

[vincentfu]

From: Vincent Fu <vincent.fu@wdc.com>

If one process is making smalloc calls and another process is making
sfree calls, pool->free_blocks and pool->next_non_full will not be
synchronized because the two processes each have independent, local
copies of the variables.

This patch allocates space for the two variables from shared storage so
that separate processes will be modifying quantities stored at the same
locations.

This issue was discovered on the server side running a client/server job
with --status-interval=1. Such a job encountered an OOM error when only
~50 objects were allocated from the smalloc pool.

Also change the calculation of free_blocks in add_pool() to use
SMALLOC_BPI instead of SMALLOC_BPB. These two constants are
coincidentally the same on Linux and Windows but SMALLOC_BPI is the
correct one to use. free_blocks is the number of available blocks of
size SMALLOC_BPB. It is the product of the number of unsigned integers
in the bitmap (bitmap_blocks) and the number of bits per unsigned
integer (SMALLOC_BPI).
---
 smalloc.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/smalloc.c b/smalloc.c
index 125e07bf..ccd73122 100644
--- a/smalloc.c
+++ b/smalloc.c
@@ -35,9 +35,9 @@ struct pool {
 	struct fio_sem *lock;			/* protects this pool */
 	void *map;				/* map of blocks */
 	unsigned int *bitmap;			/* blocks free/busy map */
-	size_t free_blocks;		/* free blocks */
+	size_t *free_blocks;			/* free blocks */
 	size_t nr_blocks;			/* total blocks */
-	size_t next_non_full;
+	size_t *next_non_full;
 	size_t mmap_size;
 };
 
@@ -170,10 +170,9 @@ static bool add_pool(struct pool *pool, unsigned int alloc_size)
 	alloc_size = (alloc_size + SMALLOC_BPL - 1) & ~(SMALLOC_BPL - 1);
 	bitmap_blocks = alloc_size / SMALLOC_BPL;
 	alloc_size += bitmap_blocks * sizeof(unsigned int);
+	alloc_size += 2 * sizeof(size_t);
 	pool->mmap_size = alloc_size;
-
 	pool->nr_blocks = bitmap_blocks;
-	pool->free_blocks = bitmap_blocks * SMALLOC_BPB;
 
 	mmap_flags = OS_MAP_ANON;
 #ifdef CONFIG_ESX
@@ -189,6 +188,11 @@ static bool add_pool(struct pool *pool, unsigned int alloc_size)
 	pool->map = ptr;
 	pool->bitmap = (unsigned int *)((char *) ptr + (pool->nr_blocks * SMALLOC_BPL));
 	memset(pool->bitmap, 0, bitmap_blocks * sizeof(unsigned int));
+	pool->free_blocks = (size_t *) (pool->bitmap + bitmap_blocks);
+	pool->next_non_full = pool->free_blocks + 1;
+
+	*(pool->free_blocks) = bitmap_blocks * SMALLOC_BPI;
+	*(pool->next_non_full) = 0;
 
 	pool->lock = fio_sem_init(FIO_SEM_UNLOCKED);
 	if (!pool->lock)
@@ -309,9 +313,9 @@ static void sfree_pool(struct pool *pool, void *ptr)
 
 	fio_sem_down(pool->lock);
 	clear_blocks(pool, i, idx, size_to_blocks(hdr->size));
-	if (i < pool->next_non_full)
-		pool->next_non_full = i;
-	pool->free_blocks += size_to_blocks(hdr->size);
+	if (i < *(pool->next_non_full))
+		*(pool->next_non_full) = i;
+	*(pool->free_blocks) += size_to_blocks(hdr->size);
 	fio_sem_up(pool->lock);
 }
 
@@ -342,9 +346,9 @@ static unsigned int find_best_index(struct pool *pool)
 {
 	unsigned int i;
 
-	assert(pool->free_blocks);
+	assert(*(pool->free_blocks));
 
-	for (i = pool->next_non_full; pool->bitmap[i] == -1U; i++) {
+	for (i = *(pool->next_non_full); pool->bitmap[i] == -1U; i++) {
 		if (i == pool->nr_blocks - 1) {
 			unsigned int j;
 
@@ -368,14 +372,14 @@ static void *__smalloc_pool(struct pool *pool, size_t size)
 	fio_sem_down(pool->lock);
 
 	nr_blocks = size_to_blocks(size);
-	if (nr_blocks > pool->free_blocks)
+	if (nr_blocks > *(pool->free_blocks))
 		goto fail;
 
-	pool->next_non_full = find_best_index(pool);
+	*(pool->next_non_full) = find_best_index(pool);
 
 	last_idx = 0;
 	offset = -1U;
-	i = pool->next_non_full;
+	i = *(pool->next_non_full);
 	while (i < pool->nr_blocks) {
 		unsigned int idx;
 
@@ -405,7 +409,7 @@ static void *__smalloc_pool(struct pool *pool, size_t size)
 	}
 
 	if (i < pool->nr_blocks) {
-		pool->free_blocks -= nr_blocks;
+		*(pool->free_blocks) -= nr_blocks;
 		ret = pool->map + offset;
 	}
 fail:
@@ -496,9 +500,9 @@ void smalloc_debug(size_t size)
 			(unsigned long) alloc_blocks);
 	for (i = 0; i < nr_pools; i++) {
 		log_err("smalloc: pool %u, free/total blocks %u/%u\n", i,
-			(unsigned int) (mp[i].free_blocks),
+			(unsigned int) *(mp[i].free_blocks),
 			(unsigned int) (mp[i].nr_blocks*sizeof(unsigned int)*8));
-		if (size && mp[i].free_blocks >= alloc_blocks) {
+		if (size && *(mp[i].free_blocks) >= alloc_blocks) {
 			void *ptr = smalloc_pool(&mp[i], size);
 			if (ptr) {
 				sfree(ptr);
@@ -507,7 +511,7 @@ void smalloc_debug(size_t size)
 			} else {
 				log_err("smalloc: smalloc_pool %u failed\n", i);
 				log_err("smalloc: next_non_full=%u, nr_blocks=%u\n",
-					(unsigned int) mp[i].next_non_full, (unsigned int) mp[i].nr_blocks);
+					(unsigned int) *(mp[i].next_non_full), (unsigned int) mp[i].nr_blocks);
 				smalloc_print_bitmap(&mp[i]);
 			}
 		}
-- 
2.17.1

Re: [three fio patches 3/3] smalloc: allocate pool-> members from shared memory

[Jens Axboe]

On 8/28/19 11:48 AM, vincentfu@gmail.com wrote:
> From: Vincent Fu <vincent.fu@wdc.com>
> 
> If one process is making smalloc calls and another process is making
> sfree calls, pool->free_blocks and pool->next_non_full will not be
> synchronized because the two processes each have independent, local
> copies of the variables.
> 
> This patch allocates space for the two variables from shared storage so
> that separate processes will be modifying quantities stored at the same
> locations.
> 
> This issue was discovered on the server side running a client/server job
> with --status-interval=1. Such a job encountered an OOM error when only
> ~50 objects were allocated from the smalloc pool.
> 
> Also change the calculation of free_blocks in add_pool() to use
> SMALLOC_BPI instead of SMALLOC_BPB. These two constants are
> coincidentally the same on Linux and Windows but SMALLOC_BPI is the
> correct one to use. free_blocks is the number of available blocks of
> size SMALLOC_BPB. It is the product of the number of unsigned integers
> in the bitmap (bitmap_blocks) and the number of bits per unsigned
> integer (SMALLOC_BPI).

Would it make more sense to just have the pool[] come out of shared
memory?

-- 
Jens Axboe

Re: [three fio patches 3/3] smalloc: allocate pool-> members from shared memory

[Vincent Fu]

On 8/28/19 3:13 PM, Jens Axboe wrote:
> On 8/28/19 11:48 AM, vincentfu@gmail.com wrote:
>> From: Vincent Fu <vincent.fu@wdc.com>
>>
>> If one process is making smalloc calls and another process is making
>> sfree calls, pool->free_blocks and pool->next_non_full will not be
>> synchronized because the two processes each have independent, local
>> copies of the variables.
>>
>> This patch allocates space for the two variables from shared storage so
>> that separate processes will be modifying quantities stored at the same
>> locations.
>>
>> This issue was discovered on the server side running a client/server job
>> with --status-interval=1. Such a job encountered an OOM error when only
>> ~50 objects were allocated from the smalloc pool.
>>
>> Also change the calculation of free_blocks in add_pool() to use
>> SMALLOC_BPI instead of SMALLOC_BPB. These two constants are
>> coincidentally the same on Linux and Windows but SMALLOC_BPI is the
>> correct one to use. free_blocks is the number of available blocks of
>> size SMALLOC_BPB. It is the product of the number of unsigned integers
>> in the bitmap (bitmap_blocks) and the number of bits per unsigned
>> integer (SMALLOC_BPI).
> 
> Would it make more sense to just have the pool[] come out of shared
> memory?
> 
>

Yeah, that would avoid the ugly *(pool->free_blocks) expressions and 
make things easier if anything else that needs to be shared is ever 
added to struct pool.

Would something like this in sinit() work?

if (nr_pools == 0)

     mp = mmap(NULL, MAX_POOLS * sizeof(struct pool), ...)

I'm on vacation through Labor Day and will work on this next week.

Vincent

Re: [three fio patches 3/3] smalloc: allocate pool-> members from shared memory

[Jens Axboe]

On 8/28/19 1:44 PM, Vincent Fu wrote:
> On 8/28/19 3:13 PM, Jens Axboe wrote:
>> On 8/28/19 11:48 AM, vincentfu@gmail.com wrote:
>>> From: Vincent Fu <vincent.fu@wdc.com>
>>>
>>> If one process is making smalloc calls and another process is making
>>> sfree calls, pool->free_blocks and pool->next_non_full will not be
>>> synchronized because the two processes each have independent, local
>>> copies of the variables.
>>>
>>> This patch allocates space for the two variables from shared storage so
>>> that separate processes will be modifying quantities stored at the same
>>> locations.
>>>
>>> This issue was discovered on the server side running a client/server job
>>> with --status-interval=1. Such a job encountered an OOM error when only
>>> ~50 objects were allocated from the smalloc pool.
>>>
>>> Also change the calculation of free_blocks in add_pool() to use
>>> SMALLOC_BPI instead of SMALLOC_BPB. These two constants are
>>> coincidentally the same on Linux and Windows but SMALLOC_BPI is the
>>> correct one to use. free_blocks is the number of available blocks of
>>> size SMALLOC_BPB. It is the product of the number of unsigned integers
>>> in the bitmap (bitmap_blocks) and the number of bits per unsigned
>>> integer (SMALLOC_BPI).
>>
>> Would it make more sense to just have the pool[] come out of shared
>> memory?
>>
>>
> 
> Yeah, that would avoid the ugly *(pool->free_blocks) expressions and
> make things easier if anything else that needs to be shared is ever
> added to struct pool.
> 
> Would something like this in sinit() work?
> 
> if (nr_pools == 0)
> 
>       mp = mmap(NULL, MAX_POOLS * sizeof(struct pool), ...)

It should only be called once, so you should not need that nr_pools
check. But yeah, something like that.

> I'm on vacation through Labor Day and will work on this next week.

Sounds good. I have applied 1-2 for now, thanks Vincent.

-- 
Jens Axboe