Patch to re-use already filled up pattern in io buffers

Patch to re-use already filled up pattern in io buffers

[Radha Ramachandran]

[-- Attachment #1: Type: text/plain, Size: 1783 bytes --]

Hi Jens,
I made changes to fio so we wld re-use the already populated io_u
buffer (when there is a non-random pattern) during writes. That way
only the header will be re-calculated for every I/O. This way the
buffer wld get populated in the beginning and as long as the
subsequent ios using the same io_u structure are writes and have same
or less block size, it wld get re-used. If any of the subsequent i/o
is a read or has a block size greater than the pre-filled one, then
the buffer is invalidated and will be re-filled at the next write.

Reason for this risky change: (Performance)
I tested this change on a tmpfs(with no swap backing), with the
following config file:
[sscan_write]
filename=/mytmpfs/datafile.tmp
rw=write
bs=64k
size=3G
ioengine=libaio
iodepth=1024
iodepth_low=512
runtime=10800
bwavgtime=5000
thread=1
do_verify=0
verify=meta
verify_pattern=0x55aaa55a
verify_interval=4k
continue_on_error=1

fio-1-41-6 gave 306MB/s and the new change had a performance of 1546MB/s

Side effects/Risks:
There is a risk with this fix, that if the buffer gets corrupted then
the subsequent writes will also be corrupt. I think for both
sequential writes and random writes (with verify, where the I/O log is
replayed) we shld be able to find the first I/O that started with the
corruption and if the buffer is getting corrupted, there are other
issues here.

Testing:
I have tested this fix with sequential write(verify)/random read write
mix combination(with verify).

I think I have taken care of most of the case, but please let me know
if there is anything I have missed. I have attached the patch along
with this email. I think the performance improvement outweighs the
risk associated with the fix. But I will let you decide if you wld
like to pick it up.

thanks
-radha

[-- Attachment #2: fill_buff.patch --]
[-- Type: text/x-diff, Size: 3716 bytes --]

diff --git a/fio.c b/fio.c
index 2dab64e..896f797 100644
--- a/fio.c
+++ b/fio.c
@@ -831,6 +831,13 @@ static int init_io_u(struct thread_data *td)
 
 			if (td_write(td) && !td->o.refill_buffers)
 				io_u_fill_buffer(td, io_u, max_bs);
+			else if (td_write(td) && td->o.verify_pattern_bytes) {
+				/*
+				 * Fill the buffer with the pattern if we are
+				 * going to be doing writes.
+				 */
+				fill_pattern(td, io_u->buf, max_bs, io_u);
+			}
 		}
 
 		io_u->index = i;
diff --git a/io_u.c b/io_u.c
index b2b7230..dc4473b 100644
--- a/io_u.c
+++ b/io_u.c
@@ -983,6 +983,14 @@ struct io_u *get_io_u(struct thread_data *td)
 			populate_verify_io_u(td, io_u);
 		else if (td->o.refill_buffers && io_u->ddir == DDIR_WRITE)
 			io_u_fill_buffer(td, io_u, io_u->xfer_buflen);
+		else if (io_u->ddir == DDIR_READ) {
+			/*
+			 * Reset the buf_filled parameters so next time if the
+			 * buffer is used for writes it is refilled.
+			 */
+			io_u->buf_filled = 0;
+			io_u->buf_filled_len = 0;
+		}
 	}
 
 	/*
diff --git a/ioengine.h b/ioengine.h
index 91dd429..b599b61 100644
--- a/ioengine.h
+++ b/ioengine.h
@@ -43,6 +43,13 @@ struct io_u {
 	unsigned long long offset;
 
 	/*
+	 * Parameters related to pre-filled buffers and
+	 * their size to handle variable block sizes.
+	 */
+	int buf_filled;
+	unsigned long buf_filled_len;
+
+	/*
 	 * IO engine state, may be different from above when we get
 	 * partial transfers / residual data counts
 	 */
diff --git a/verify.c b/verify.c
index 265bd55..73c1262 100644
--- a/verify.c
+++ b/verify.c
@@ -22,7 +22,7 @@
 #include "crc/sha512.h"
 #include "crc/sha1.h"
 
-static void fill_pattern(struct thread_data *td, void *p, unsigned int len)
+void fill_pattern(struct thread_data *td, void *p, unsigned int len, struct io_u *io_u)
 {
 	switch (td->o.verify_pattern_bytes) {
 	case 0:
@@ -30,13 +30,24 @@ static void fill_pattern(struct thread_data *td, void *p, unsigned int len)
 		fill_random_buf(p, len);
 		break;
 	case 1:
+		if (io_u->buf_filled && io_u->buf_filled_len >= len) {
+			dprint(FD_VERIFY, "using already filled verify pattern b=0 len=%u\n", len);
+			return;
+		}
 		dprint(FD_VERIFY, "fill verify pattern b=0 len=%u\n", len);
 		memset(p, td->o.verify_pattern[0], len);
+		io_u->buf_filled = 1;
+		io_u->buf_filled_len = len;
 		break;
 	default: {
 		unsigned int i = 0, size = 0;
 		unsigned char *b = p;
 
+		if (io_u->buf_filled && io_u->buf_filled_len >= len) {
+			dprint(FD_VERIFY, "using already filled verify pattern b=%d len=%u\n",
+					td->o.verify_pattern_bytes, len);
+			return;
+		}
 		dprint(FD_VERIFY, "fill verify pattern b=%d len=%u\n",
 					td->o.verify_pattern_bytes, len);
 
@@ -47,6 +58,8 @@ static void fill_pattern(struct thread_data *td, void *p, unsigned int len)
 			memcpy(b+i, td->o.verify_pattern, size);
 			i += size;
 		}
+		io_u->buf_filled = 1;
+		io_u->buf_filled_len = len;
 		break;
 		}
 	}
@@ -675,7 +688,7 @@ void populate_verify_io_u(struct thread_data *td, struct io_u *io_u)
 	if (td->o.verify == VERIFY_NULL)
 		return;
 
-	fill_pattern(td, p, io_u->buflen);
+	fill_pattern(td, p, io_u->buflen, io_u);
 
 	hdr_inc = io_u->buflen;
 	if (td->o.verify_interval)
diff --git a/verify.h b/verify.h
index a4a8cfe..29c4b46 100644
--- a/verify.h
+++ b/verify.h
@@ -69,6 +69,7 @@ extern void populate_verify_io_u(struct thread_data *, struct io_u *);
 extern int __must_check get_next_verify(struct thread_data *td, struct io_u *);
 extern int __must_check verify_io_u(struct thread_data *, struct io_u *);
 extern int verify_io_u_async(struct thread_data *, struct io_u *);
+extern void fill_pattern(struct thread_data *td, void *p, unsigned int len, struct io_u *io_u);
 
 /*
  * Async verify offload

Re: Patch to re-use already filled up pattern in io buffers

[Jens Axboe]

On 07/14/2010 02:13 AM, Radha Ramachandran wrote:
> Hi Jens,
> I made changes to fio so we wld re-use the already populated io_u
> buffer (when there is a non-random pattern) during writes. That way
> only the header will be re-calculated for every I/O. This way the
> buffer wld get populated in the beginning and as long as the
> subsequent ios using the same io_u structure are writes and have same
> or less block size, it wld get re-used. If any of the subsequent i/o
> is a read or has a block size greater than the pre-filled one, then
> the buffer is invalidated and will be re-filled at the next write.
> 
> Reason for this risky change: (Performance)
> I tested this change on a tmpfs(with no swap backing), with the
> following config file:
> [sscan_write]
> filename=/mytmpfs/datafile.tmp
> rw=write
> bs=64k
> size=3G
> ioengine=libaio
> iodepth=1024
> iodepth_low=512
> runtime=10800
> bwavgtime=5000
> thread=1
> do_verify=0
> verify=meta
> verify_pattern=0x55aaa55a
> verify_interval=4k
> continue_on_error=1
> 
> fio-1-41-6 gave 306MB/s and the new change had a performance of 1546MB/s
> 
> Side effects/Risks:
> There is a risk with this fix, that if the buffer gets corrupted then
> the subsequent writes will also be corrupt. I think for both
> sequential writes and random writes (with verify, where the I/O log is
> replayed) we shld be able to find the first I/O that started with the
> corruption and if the buffer is getting corrupted, there are other
> issues here.
> 
> Testing:
> I have tested this fix with sequential write(verify)/random read write
> mix combination(with verify).
> 
> I think I have taken care of most of the case, but please let me know
> if there is anything I have missed. I have attached the patch along
> with this email. I think the performance improvement outweighs the
> risk associated with the fix. But I will let you decide if you wld
> like to pick it up.

I will pick this up, the fill time is the reason for some of the
other hoops we jump through to try and avoid that when possible.
I don't think the risk of memory corruption is something we need
to consider. That could just as easily happen after the data
has been read anyway, both cases would result in a verify error.

-- 
Jens Axboe

Re: Patch to re-use already filled up pattern in io buffers

[Jens Axboe]

On 07/14/2010 08:33 AM, Jens Axboe wrote:
> On 07/14/2010 02:13 AM, Radha Ramachandran wrote:
>> Hi Jens,
>> I made changes to fio so we wld re-use the already populated io_u
>> buffer (when there is a non-random pattern) during writes. That way
>> only the header will be re-calculated for every I/O. This way the
>> buffer wld get populated in the beginning and as long as the
>> subsequent ios using the same io_u structure are writes and have same
>> or less block size, it wld get re-used. If any of the subsequent i/o
>> is a read or has a block size greater than the pre-filled one, then
>> the buffer is invalidated and will be re-filled at the next write.
>>
>> Reason for this risky change: (Performance)
>> I tested this change on a tmpfs(with no swap backing), with the
>> following config file:
>> [sscan_write]
>> filename=/mytmpfs/datafile.tmp
>> rw=write
>> bs=64k
>> size=3G
>> ioengine=libaio
>> iodepth=1024
>> iodepth_low=512
>> runtime=10800
>> bwavgtime=5000
>> thread=1
>> do_verify=0
>> verify=meta
>> verify_pattern=0x55aaa55a
>> verify_interval=4k
>> continue_on_error=1
>>
>> fio-1-41-6 gave 306MB/s and the new change had a performance of 1546MB/s
>>
>> Side effects/Risks:
>> There is a risk with this fix, that if the buffer gets corrupted then
>> the subsequent writes will also be corrupt. I think for both
>> sequential writes and random writes (with verify, where the I/O log is
>> replayed) we shld be able to find the first I/O that started with the
>> corruption and if the buffer is getting corrupted, there are other
>> issues here.
>>
>> Testing:
>> I have tested this fix with sequential write(verify)/random read write
>> mix combination(with verify).
>>
>> I think I have taken care of most of the case, but please let me know
>> if there is anything I have missed. I have attached the patch along
>> with this email. I think the performance improvement outweighs the
>> risk associated with the fix. But I will let you decide if you wld
>> like to pick it up.
> 
> I will pick this up, the fill time is the reason for some of the
> other hoops we jump through to try and avoid that when possible.
> I don't think the risk of memory corruption is something we need
> to consider. That could just as easily happen after the data
> has been read anyway, both cases would result in a verify error.
> 

I made a small change to turn ->buf_filled into an io_u
flag. It would be nice if you could retest the current
-git state and ensure that it works properly. I'm on the
road the next ~10 days, so wont have a lot of testing time
on my hands.

-- 
Jens Axboe

Re: Patch to re-use already filled up pattern in io buffers

[Radha Ramachandran]

[-- Attachment #1: Type: text/plain, Size: 3159 bytes --]

Hi Jens,
As I was verifying the latest code from head, I realized that we
actually did not need the IO_U_F_FILLED flag, we can just
use(overload) the buf_filled_len to check if the buffer is actually
filled (as this is reset to 0 on a read). This also gave me slight
performance bump. Attached is the patch for it.
thanks
-radha


On Tue, Jul 13, 2010 at 11:44 PM, Jens Axboe <jaxboe@fusionio.com> wrote:
> On 07/14/2010 08:33 AM, Jens Axboe wrote:
>> On 07/14/2010 02:13 AM, Radha Ramachandran wrote:
>>> Hi Jens,
>>> I made changes to fio so we wld re-use the already populated io_u
>>> buffer (when there is a non-random pattern) during writes. That way
>>> only the header will be re-calculated for every I/O. This way the
>>> buffer wld get populated in the beginning and as long as the
>>> subsequent ios using the same io_u structure are writes and have same
>>> or less block size, it wld get re-used. If any of the subsequent i/o
>>> is a read or has a block size greater than the pre-filled one, then
>>> the buffer is invalidated and will be re-filled at the next write.
>>>
>>> Reason for this risky change: (Performance)
>>> I tested this change on a tmpfs(with no swap backing), with the
>>> following config file:
>>> [sscan_write]
>>> filename=/mytmpfs/datafile.tmp
>>> rw=write
>>> bs=64k
>>> size=3G
>>> ioengine=libaio
>>> iodepth=1024
>>> iodepth_low=512
>>> runtime=10800
>>> bwavgtime=5000
>>> thread=1
>>> do_verify=0
>>> verify=meta
>>> verify_pattern=0x55aaa55a
>>> verify_interval=4k
>>> continue_on_error=1
>>>
>>> fio-1-41-6 gave 306MB/s and the new change had a performance of 1546MB/s
>>>
>>> Side effects/Risks:
>>> There is a risk with this fix, that if the buffer gets corrupted then
>>> the subsequent writes will also be corrupt. I think for both
>>> sequential writes and random writes (with verify, where the I/O log is
>>> replayed) we shld be able to find the first I/O that started with the
>>> corruption and if the buffer is getting corrupted, there are other
>>> issues here.
>>>
>>> Testing:
>>> I have tested this fix with sequential write(verify)/random read write
>>> mix combination(with verify).
>>>
>>> I think I have taken care of most of the case, but please let me know
>>> if there is anything I have missed. I have attached the patch along
>>> with this email. I think the performance improvement outweighs the
>>> risk associated with the fix. But I will let you decide if you wld
>>> like to pick it up.
>>
>> I will pick this up, the fill time is the reason for some of the
>> other hoops we jump through to try and avoid that when possible.
>> I don't think the risk of memory corruption is something we need
>> to consider. That could just as easily happen after the data
>> has been read anyway, both cases would result in a verify error.
>>
>
> I made a small change to turn ->buf_filled into an io_u
> flag. It would be nice if you could retest the current
> -git state and ensure that it works properly. I'm on the
> road the next ~10 days, so wont have a lot of testing time
> on my hands.
>
> --
> Jens Axboe
>
>

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: IO_U_F_FILLED.patch --]
[-- Type: text/x-diff; name="IO_U_F_FILLED.patch", Size: 1787 bytes --]

diff --git a/io_u.c b/io_u.c
index 88b2b9f..f27cdda 100644
--- a/io_u.c
+++ b/io_u.c
@@ -988,7 +988,6 @@ struct io_u *get_io_u(struct thread_data *td)
 			 * Reset the buf_filled parameters so next time if the
 			 * buffer is used for writes it is refilled.
 			 */
-			io_u->flags &= ~IO_U_F_FILLED;
 			io_u->buf_filled_len = 0;
 		}
 	}
diff --git a/ioengine.h b/ioengine.h
index 343b06f..e9f5d92 100644
--- a/ioengine.h
+++ b/ioengine.h
@@ -8,7 +8,6 @@ enum {
 	IO_U_F_FLIGHT		= 1 << 1,
 	IO_U_F_FREE_DEF		= 1 << 2,
 	IO_U_F_IN_CUR_DEPTH	= 1 << 3,
-	IO_U_F_FILLED		= 1 << 4,
 };
 
 /*
diff --git a/verify.c b/verify.c
index 098c19b..42ea462 100644
--- a/verify.c
+++ b/verify.c
@@ -30,22 +30,19 @@ void fill_pattern(struct thread_data *td, void *p, unsigned int len, struct io_u
 		fill_random_buf(p, len);
 		break;
 	case 1:
-		if ((io_u->flags & IO_U_F_FILLED) &&
-		     io_u->buf_filled_len >= len) {
+		if (io_u->buf_filled_len >= len) {
 			dprint(FD_VERIFY, "using already filled verify pattern b=0 len=%u\n", len);
 			return;
 		}
 		dprint(FD_VERIFY, "fill verify pattern b=0 len=%u\n", len);
 		memset(p, td->o.verify_pattern[0], len);
-		io_u->flags |= IO_U_F_FILLED;
 		io_u->buf_filled_len = len;
 		break;
 	default: {
 		unsigned int i = 0, size = 0;
 		unsigned char *b = p;
 
-		if ((io_u->flags & IO_U_F_FILLED) &&
-		     io_u->buf_filled_len >= len) {
+		if (io_u->buf_filled_len >= len) {
 			dprint(FD_VERIFY, "using already filled verify pattern b=%d len=%u\n",
 					td->o.verify_pattern_bytes, len);
 			return;
@@ -60,7 +57,6 @@ void fill_pattern(struct thread_data *td, void *p, unsigned int len, struct io_u
 			memcpy(b+i, td->o.verify_pattern, size);
 			i += size;
 		}
-		io_u->flags |= IO_U_F_FILLED;
 		io_u->buf_filled_len = len;
 		break;
 		}

Re: Patch to re-use already filled up pattern in io buffers

[Jens Axboe]

On 07/14/2010 12:10 PM, Radha Ramachandran wrote:
> Hi Jens,
> As I was verifying the latest code from head, I realized that we
> actually did not need the IO_U_F_FILLED flag, we can just
> use(overload) the buf_filled_len to check if the buffer is actually
> filled (as this is reset to 0 on a read). This also gave me slight
> performance bump. Attached is the patch for it.

Good point, there's no need for that flag/field at all. Will
apply your patch, thanks.

-- 
Jens Axboe