ltp-aiodio/dio_sparse: Fix write in dio_sparse()

PATCH Superseded Guangwen Feng 2 participants 15 messages Jan 19, 2017 – Mar 22, 2017

Share & tools

Guangwen Feng

author

2017-01-19 09:32 UTC #0 lore raw

PATCH 1/2 ltp-aiodio/dio_sparse: Fix write in dio_sparse()

Current code looks buggy because when the offset is greater than or equal to the filesize, it will never happen to do the write in the loop, as a result, ADSP080..ADSP087 do not work actually. Fix it by making sure that we do write writesize bytes.

Signed-off-by Guangwen Feng <[email protected]>

---
 testcases/kernel/io/ltp-aiodio/dio_sparse.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

testcases/kernel/io/ltp-aiodio/dio_sparse.c +1 -1

index 41b9929..3828ed7 100644
@@ -78,7 +78,7 @@ int dio_sparse(char *filename, int align, int writesize, int filesize, int offse
 
 	memset(bufptr, 0, writesize);
 	lseek(fd, offset, SEEK_SET);
-	for (i = offset; i < filesize;) {
+	for (i = 0; i < filesize;) {
 		if ((w = write(fd, bufptr, writesize)) != writesize) {
 			tst_resm(TBROK | TERRNO, "write() returned %d", w);
 			close(fd);

Signature · 1 line

1.8.4.2

Guangwen Feng

author

2017-01-19 09:32 UTC #1 lore raw

PATCH 2/2 ltp-aiodio/dio_sparse: Fix usleep in read_sparse()

usleep(100000) sometimes leads to child process being too late to do the read before being killed by parent process, tune it to usleep(10) to make sure we do the real test in time.

Signed-off-by Guangwen Feng <[email protected]>

---
 testcases/kernel/io/ltp-aiodio/common_sparse.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

testcases/kernel/io/ltp-aiodio/common_sparse.h +1 -1

index f7f4ef4..a7f5035 100644
@@ -123,7 +123,7 @@ static void read_sparse(char *filename, int filesize)
 			fprintf(stderr, "Child %i waits for '%s' to appear\n",
 			        getpid(), filename);
 
-		usleep(100000);
+		usleep(10);
 	}
 
 	if (fd == -1) {

Signature · 1 line

1.8.4.2

Cyril Hrubis

2017-01-24 13:43 UTC #2 lore raw

PATCH 2/2 ltp-aiodio/dio_sparse: Fix usleep in read_sparse()

Hi!

testcases/kernel/io/ltp-aiodio/common_sparse.h static void read_sparse(char *filename, int filesize) · line 126

Quoted 10 lines

testcases/kernel/io/ltp-aiodio/common_sparse.h +1 -1

index f7f4ef4..a7f5035 100644
@@ -123,7 +123,7 @@ static void read_sparse(char *filename, int filesize)
 			fprintf(stderr, "Child %i waits for '%s' to appear\n",
 			        getpid(), filename);
 
-		usleep(100000);
+		usleep(10);

Isn't 10us too small? Also we really should increase the number of the loop iterations if we reduce the sleep as much.

Quoted 11 lines

testcases/kernel/io/ltp-aiodio/common_sparse.h +0 -0
 	}
 
 	if (fd == -1) {
Signature · 7 lines
1.8.4.2




-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Signature · 2 lines

Cyril Hrubis
[email protected]

Guangwen Feng

author

2017-01-25 04:02 UTC #3 lore raw

PATCH 2/2 ltp-aiodio/dio_sparse: Fix usleep in read_sparse()

Hi Cyril,

Thanks for your review.

Quoted 9 lines · Cyril Hrubis

Hi!

Quoted 8 lines

testcases/kernel/io/ltp-aiodio/common_sparse.h +0 -0

index f7f4ef4..a7f5035 100644
@@ -123,7 +123,7 @@ static void read_sparse(char *filename, int filesize)
 			fprintf(stderr, "Child %i waits for '%s' to appear\n",
 			        getpid(), filename);

Quoted 6 lines

testcases/kernel/io/ltp-aiodio/common_sparse.h static void read_sparse(char *filename, int filesize) · line 126
Quoted 2 lines
testcases/kernel/io/ltp-aiodio/common_sparse.h +1 -1
-		usleep(100000);
+		usleep(10);
Isn't 10us too small? Also we really should increase the number of the loop iterations if we reduce the sleep as much.

Most of the time child works well by usleep(100), but there is still a small chance that it misses the test on my system...

OK, I will also increase the loop number and send a V2. Thanks.

Best Regards,
Guangwen Feng

Quoted 12 lines

Quoted 11 lines
testcases/kernel/io/ltp-aiodio/common_sparse.h +0 -0
 	}
 
 	if (fd == -1) {
Signature · 7 lines
1.8.4.2




-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Guangwen Feng

author

2017-02-09 07:23 UTC #4 lore raw

PATCH v2 1/1 ltp-aiodio/dio_sparse: Fix usleep in read_sparse()

usleep(100000) sometimes leads to child process being too late to do the read before being killed by parent process, tune it to usleep(100) to make sure we do the real test in time.

Signed-off-by Guangwen Feng <[email protected]>

---
 testcases/kernel/io/ltp-aiodio/common_sparse.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

testcases/kernel/io/ltp-aiodio/common_sparse.h +2 -2

index f7f4ef4..7297319 100644
@@ -113,7 +113,7 @@ static void read_sparse(char *filename, int filesize)
 	/*
 	 * Wait for the file to appear.
 	 */
-	for (i = 0; i < 10000; i++) {
+	for (i = 0; i < 10000000; i++) {
 		fd = open(filename, O_RDONLY);
 
 		if (fd != -1)

@@ -123,7 +123,7 @@ static void read_sparse(char *filename, int filesize)
 			fprintf(stderr, "Child %i waits for '%s' to appear\n",
 			        getpid(), filename);
 
-		usleep(100000);
+		usleep(100);
 	}
 
 	if (fd == -1) {

Signature · 1 line

1.8.4.2

Cyril Hrubis

2017-02-09 09:35 UTC #5 lore raw

PATCH v2 1/1 ltp-aiodio/dio_sparse: Fix usleep in read_sparse()

Hi!

Quoted 23 lines

usleep(100000) sometimes leads to child process being too late to do the read before being killed by parent process, tune it to usleep(100) to make sure we do the real test in time.
Signed-off-by Guangwen Feng <[email protected]>
---
 testcases/kernel/io/ltp-aiodio/common_sparse.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
testcases/kernel/io/ltp-aiodio/common_sparse.h +1 -1
index f7f4ef4..7297319 100644
@@ -113,7 +113,7 @@ static void read_sparse(char *filename, int filesize)
 	/*
 	 * Wait for the file to appear.
 	 */
-	for (i = 0; i < 10000; i++) {
+	for (i = 0; i < 10000000; i++) {
 		fd = open(filename, O_RDONLY);
 
 		if (fd != -1)

testcases/kernel/io/ltp-aiodio/common_sparse.h static void read_sparse(char *filename, int filesize) · line 127

Quoted 6 lines

testcases/kernel/io/ltp-aiodio/common_sparse.h +1 -1

@@ -123,7 +123,7 @@ static void read_sparse(char *filename, int filesize)
 			fprintf(stderr, "Child %i waits for '%s' to appear\n",
 			        getpid(), filename);
 
-		usleep(100000);
+		usleep(100);
 	}

Looking at the code again, I see no reason why we can't open and truncate the file before we start the read_sparse() children, then we could drop this loop and just pass file descriptor to this function and to the dio_sparse() and aiodio_sparse() as well. Or did I miss something?

Signature · 2 lines

Cyril Hrubis
[email protected]

Guangwen Feng

author

2017-02-14 03:20 UTC #6 lore raw

PATCH v2 1/1 ltp-aiodio/dio_sparse: Fix usleep in read_sparse()

Hi!

Quoted 27 lines · Cyril Hrubis

Hi!
Quoted 26 lines
usleep(100000) sometimes leads to child process being too late to do the read before being killed by parent process, tune it to usleep(100) to make sure we do the real test in time.
Signed-off-by Guangwen Feng <[email protected]>
---
 testcases/kernel/io/ltp-aiodio/common_sparse.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
testcases/kernel/io/ltp-aiodio/common_sparse.h +1 -1
index f7f4ef4..7297319 100644
@@ -113,7 +113,7 @@ static void read_sparse(char *filename, int filesize)
 	/*
 	 * Wait for the file to appear.
 	 */
-	for (i = 0; i < 10000; i++) {
+	for (i = 0; i < 10000000; i++) {
 		fd = open(filename, O_RDONLY);
 
 		if (fd != -1)
@@ -123,7 +123,7 @@ static void read_sparse(char *filename, int filesize)
 			fprintf(stderr, "Child %i waits for '%s' to appear\n",
 			        getpid(), filename);
 

Quoted 9 lines

testcases/kernel/io/ltp-aiodio/common_sparse.h static void read_sparse(char *filename, int filesize) · line 127
Quoted 3 lines
testcases/kernel/io/ltp-aiodio/common_sparse.h +1 -1
-		usleep(100000);
+		usleep(100);
 	}
Looking at the code again, I see no reason why we can't open and truncate the file before we start the read_sparse() children, then we could drop this loop and just pass file descriptor to this function and to the dio_sparse() and aiodio_sparse() as well. Or did I miss something?

Yes, you are right, it is a thoroghly solution to this issue, I will rewrite this according to your suggestion, thanks.

Best Regards,
Guangwen Feng

Guangwen Feng

author

2017-03-21 08:08 UTC #7 lore raw

PATCH 1/1 ltp-aiodio: Create the file before fork

Currently, child's waiting for the file to appear in a loop with usleep(100000) sometimes is too late to do the read before child process is killed by parent process, which lead to child misses the whole test. Create and truncate the file before fork, then drop the loop of waiting in read_sparse().

Signed-off-by Guangwen Feng <[email protected]>

---
 testcases/kernel/io/ltp-aiodio/aiodio_sparse.c | 32 +++++++++-----------------
 testcases/kernel/io/ltp-aiodio/common_sparse.h | 17 +-------------
 testcases/kernel/io/ltp-aiodio/dio_sparse.c    | 28 +++++++++-------------
 3 files changed, 23 insertions(+), 54 deletions(-)

testcases/kernel/io/ltp-aiodio/aiodio_sparse.c +11 ~~-21~~

index 49a0915..d40e45b 100644
@@ -43,6 +43,7 @@
 #define NUM_CHILDREN 1000
 
 int debug;
+int fd;
 
 static void setup(void);
 static void cleanup(void);

@@ -56,10 +57,8 @@ int TST_TOTAL = 1;
 /*
  * do async DIO writes to a sparse file
  */
-int aiodio_sparse(char *filename, int align, int writesize, int filesize,
-		  int num_aio)
+int aiodio_sparse(int fd, int align, int writesize, int filesize, int num_aio)
 {
-	int fd;
 	int i, w;
 	struct iocb **iocbs;
 	off_t offset;

@@ -81,15 +80,6 @@ int aiodio_sparse(char *filename, int align, int writesize, int filesize,
 		}
 	}
 
-	fd = open(filename, O_DIRECT | O_WRONLY | O_CREAT | O_EXCL, 0600);
-
-	if (fd < 0) {
-		tst_resm(TBROK | TERRNO, "open()");
-		return 1;
-	}
-
-	SAFE_FTRUNCATE(cleanup, fd, filesize);
-
 	/*
 	 * allocate the iocbs array and iocbs with buffers
 	 */

@@ -100,8 +90,6 @@ int aiodio_sparse(char *filename, int align, int writesize, int filesize,
 		TEST(posix_memalign(&bufptr, align, writesize));
 		if (TEST_RETURN) {
 			tst_resm(TBROK | TRERRNO, "cannot allocate aligned memory");
-			close(fd);
-			unlink(filename);
 			return 1;
 		}
 		memset(bufptr, 0, writesize);

@@ -114,8 +102,6 @@ int aiodio_sparse(char *filename, int align, int writesize, int filesize,
 	 */
 	if ((w = io_submit(myctx, num_aio, iocbs)) < 0) {
 		tst_resm(TBROK, "io_submit() returned %i", w);
-		close(fd);
-		unlink(filename);
 		return 1;
 	}

@@ -205,9 +191,6 @@ int aiodio_sparse(char *filename, int align, int writesize, int filesize,
 		}
 	}
 
-	close(fd);
-	unlink(filename);
-
 	return 0;
 }

@@ -275,11 +258,16 @@ int main(int argc, char **argv)
 	tst_resm(TINFO, "Dirtying free blocks");
 	dirty_freeblocks(filesize);
 
+	fd = SAFE_OPEN(cleanup, filename,
+		O_DIRECT | O_WRONLY | O_CREAT | O_EXCL, 0600);
+	SAFE_FTRUNCATE(cleanup, fd, filesize);
+
 	tst_resm(TINFO, "Starting I/O tests");
 	signal(SIGTERM, SIG_DFL);
 	for (i = 0; i < num_children; i++) {
 		switch (pid[i] = fork()) {
 		case 0:
+			SAFE_CLOSE(NULL, fd);
 			read_sparse(filename, filesize);
 			break;
 		case -1:

@@ -293,7 +281,7 @@ int main(int argc, char **argv)
 	}
 	tst_sig(FORK, DEF_HANDLER, cleanup);
 
-	ret = aiodio_sparse(filename, alignment, writesize, filesize, num_aio);
+	ret = aiodio_sparse(fd, alignment, writesize, filesize, num_aio);
 
 	tst_resm(TINFO, "Killing childrens(s)");

@@ -332,6 +320,8 @@ static void setup(void)
 
 static void cleanup(void)
 {
+	if (fd > 0 && close(fd))
+		tst_resm(TWARN | TERRNO, "Failed to close file");
+
 	tst_rmdir();
-	tst_exit();
 }

testcases/kernel/io/ltp-aiodio/common_sparse.h +1 ~~-16~~

index f7f4ef4..e8b906e 100644
@@ -110,22 +110,7 @@ static void read_sparse(char *filename, int filesize)
 	int  i, j, r;
 	char buf[4096];
 
-	/*
-	 * Wait for the file to appear.
-	 */
-	for (i = 0; i < 10000; i++) {
-		fd = open(filename, O_RDONLY);
-
-		if (fd != -1)
-			break;
-
-		if (debug)
-			fprintf(stderr, "Child %i waits for '%s' to appear\n",
-			        getpid(), filename);
-
-		usleep(100000);
-	}
-
+	fd = open(filename, O_RDONLY);
 	if (fd == -1) {
 		if (debug)
 			fprintf(stderr, "Child %i failed to open '%s'\n",

testcases/kernel/io/ltp-aiodio/dio_sparse.c +11 ~~-17~~

index 41b9929..67b338b 100644
@@ -45,6 +45,7 @@ static void setup(void);
 static void cleanup(void);
 static void usage(void);
 static int debug = 0;
+static int fd;
 
 char *TCID = "dio_sparse";
 int TST_TOTAL = 1;

@@ -54,25 +55,14 @@ int TST_TOTAL = 1;
 /*
  * Write zeroes using O_DIRECT into sparse file.
  */
-int dio_sparse(char *filename, int align, int writesize, int filesize, int offset)
+int dio_sparse(int fd, int align, int writesize, int filesize, int offset)
 {
-	int fd;
 	void *bufptr;
 	int i, w;
 
-	fd = open(filename, O_DIRECT | O_WRONLY | O_CREAT | O_EXCL, 0600);
-
-	if (fd < 0) {
-		tst_resm(TBROK | TERRNO, "open()");
-		return 1;
-	}
-
-	SAFE_FTRUNCATE(cleanup, fd, filesize);
-
 	TEST(posix_memalign(&bufptr, align, writesize));
 	if (TEST_RETURN) {
 		tst_resm(TBROK | TRERRNO, "cannot allocate aligned memory");
-		close(fd);
 		return 1;
 	}

@@ -81,16 +71,12 @@ int dio_sparse(char *filename, int align, int writesize, int filesize, int offse
 	for (i = offset; i < filesize;) {
 		if ((w = write(fd, bufptr, writesize)) != writesize) {
 			tst_resm(TBROK | TERRNO, "write() returned %d", w);
-			close(fd);
 			return 1;
 		}
 
 		i += w;
 	}
 
-	close(fd);
-	unlink(filename);
-
 	return 0;
 }

@@ -156,11 +142,16 @@ int main(int argc, char **argv)
 	tst_resm(TINFO, "Dirtying free blocks");
 	dirty_freeblocks(filesize);
 
+	fd = SAFE_OPEN(cleanup, filename,
+		O_DIRECT | O_WRONLY | O_CREAT | O_EXCL, 0600);
+	SAFE_FTRUNCATE(cleanup, fd, filesize);
+
 	tst_resm(TINFO, "Starting I/O tests");
 	signal(SIGTERM, SIG_DFL);
 	for (i = 0; i < num_children; i++) {
 		switch (pid[i] = fork()) {
 		case 0:
+			SAFE_CLOSE(NULL, fd);
 			read_sparse(filename, filesize);
 			break;
 		case -1:

@@ -174,7 +165,7 @@ int main(int argc, char **argv)
 	}
 	tst_sig(FORK, DEF_HANDLER, cleanup);
 
-	ret = dio_sparse(filename, alignment, writesize, filesize, offset);
+	ret = dio_sparse(fd, alignment, writesize, filesize, offset);
 
 	tst_resm(TINFO, "Killing childrens(s)");

@@ -213,5 +204,8 @@ static void setup(void)
 
 static void cleanup(void)
 {
+	if (fd > 0 && close(fd))
+		tst_resm(TWARN | TERRNO, "Failed to close file");
+
 	tst_rmdir();
 }

Signature · 1 line

1.8.4.2

Cyril Hrubis

2017-03-22 15:33 UTC #8 lore raw

PATCH 1/1 ltp-aiodio: Create the file before fork

Hi!
Pushed, thanks.

Signature · 2 lines

Cyril Hrubis
[email protected]

Cyril Hrubis

2017-01-24 13:40 UTC #9 lore raw

PATCH 1/2 ltp-aiodio/dio_sparse: Fix write in dio_sparse()

In reply to Guangwen Feng

Hi!

Quoted 15 lines

Current code looks buggy because when the offset is greater than or equal to the filesize, it will never happen to do the write in the loop, as a result, ADSP080..ADSP087 do not work actually. Fix it by making sure that we do write writesize bytes.
Signed-off-by Guangwen Feng <[email protected]>
---
 testcases/kernel/io/ltp-aiodio/dio_sparse.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
testcases/kernel/io/ltp-aiodio/dio_sparse.c +0 -0
index 41b9929..3828ed7 100644

testcases/kernel/io/ltp-aiodio/dio_sparse.c int dio_sparse(char *filename, int align, int writesize, int filesize, int offse · line 84

Quoted 8 lines

testcases/kernel/io/ltp-aiodio/dio_sparse.c +1 -1

@@ -78,7 +78,7 @@ int dio_sparse(char *filename, int align, int writesize, int filesize, int offse
 
 	memset(bufptr, 0, writesize);
 	lseek(fd, offset, SEEK_SET);
-	for (i = offset; i < filesize;) {
+	for (i = 0; i < filesize;) {
 		if ((w = write(fd, bufptr, writesize)) != writesize) {
 			tst_resm(TBROK | TERRNO, "write() returned %d", w);
 			close(fd);

Hmm, it looks to me like the code actually does what it should have. Since we pass a filesize and offset so the test should actually write filesize - offset bytes. As far as I can tell the bug here is that the test is not checking that filesize > offset before it starts.

Signature · 2 lines

Cyril Hrubis
[email protected]

Guangwen Feng

author

2017-01-25 04:33 UTC #10 lore raw

PATCH 1/2 ltp-aiodio/dio_sparse: Fix write in dio_sparse()

Hi Cyril,

Thanks for your review.

Quoted 17 lines · Cyril Hrubis

Hi!
Quoted 16 lines
Current code looks buggy because when the offset is greater than or equal to the filesize, it will never happen to do the write in the loop, as a result, ADSP080..ADSP087 do not work actually. Fix it by making sure that we do write writesize bytes.
Signed-off-by Guangwen Feng <[email protected]>
---
 testcases/kernel/io/ltp-aiodio/dio_sparse.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
testcases/kernel/io/ltp-aiodio/dio_sparse.c +0 -0
index 41b9929..3828ed7 100644
@@ -78,7 +78,7 @@ int dio_sparse(char *filename, int align, int writesize, int filesize, int offse
 

Quoted 12 lines

testcases/kernel/io/ltp-aiodio/dio_sparse.c int dio_sparse(char *filename, int align, int writesize, int filesize, int offse · line 84
Quoted 7 lines
testcases/kernel/io/ltp-aiodio/dio_sparse.c +1 -1
 	memset(bufptr, 0, writesize);
 	lseek(fd, offset, SEEK_SET);
-	for (i = offset; i < filesize;) {
+	for (i = 0; i < filesize;) {
 		if ((w = write(fd, bufptr, writesize)) != writesize) {
 			tst_resm(TBROK | TERRNO, "write() returned %d", w);
 			close(fd);
Hmm, it looks to me like the code actually does what it should have. Since we pass a filesize and offset so the test should actually write filesize - offset bytes. As far as I can tell the bug here is that the test is not checking that filesize > offset before it starts.

Sorry, but shouldn't we write the whole "writesize" from the "offset"? ADSP080 ADSP081 ADSP082 ADSP083 did reproduce the BUG via above modification...

Best Regards,
Guangwen Feng

Guangwen Feng

author

2017-02-09 08:56 UTC #11 lore raw

PATCH 1/2 ltp-aiodio/dio_sparse: Fix write in dio_sparse()

Hi!

Quoted 35 lines · Guangwen Feng

Hi Cyril,
Thanks for your review.
Quoted 17 lines · Cyril Hrubis
Hi!
Quoted 16 lines
Current code looks buggy because when the offset is greater than or equal to the filesize, it will never happen to do the write in the loop, as a result, ADSP080..ADSP087 do not work actually. Fix it by making sure that we do write writesize bytes.
Signed-off-by Guangwen Feng <[email protected]>
---
 testcases/kernel/io/ltp-aiodio/dio_sparse.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
testcases/kernel/io/ltp-aiodio/dio_sparse.c +0 -0
index 41b9929..3828ed7 100644
@@ -78,7 +78,7 @@ int dio_sparse(char *filename, int align, int writesize, int filesize, int offse
 
Quoted 12 lines
testcases/kernel/io/ltp-aiodio/dio_sparse.c int dio_sparse(char *filename, int align, int writesize, int filesize, int offse · line 84
Quoted 7 lines
testcases/kernel/io/ltp-aiodio/dio_sparse.c +1 -1
 	memset(bufptr, 0, writesize);
 	lseek(fd, offset, SEEK_SET);
-	for (i = offset; i < filesize;) {
+	for (i = 0; i < filesize;) {
 		if ((w = write(fd, bufptr, writesize)) != writesize) {
 			tst_resm(TBROK | TERRNO, "write() returned %d", w);
 			close(fd);
Hmm, it looks to me like the code actually does what it should have. Since we pass a filesize and offset so the test should actually write filesize - offset bytes. As far as I can tell the bug here is that the test is not checking that filesize > offset before it starts.

Quoted 2 lines

Sorry, but shouldn't we write the whole "writesize" from the "offset"? ADSP080 ADSP081 ADSP082 ADSP083 did reproduce the BUG via above modification...

I think we pass an "offset" is to make sure writing from the file offset, but the actual write size should be just the "writesize" argument.

As the second case of kernel commit 9ecd10b says:
2. Direct writes starting from or beyong i_size (not inside i_size) also could trigger block allocation and expose stale data. For example, consider a sparse file with i_size of 2k, and a write to offset 2k or 3k into the file, with a filesystem block size of 4k. (Thanks to Jeff Moyer for pointing this case out in his review.)

Best Regards,
Guangwen Feng

Quoted 3 lines

Best Regards,
Guangwen Feng

Cyril Hrubis

2017-02-09 09:31 UTC #12 lore raw

PATCH 1/2 ltp-aiodio/dio_sparse: Fix write in dio_sparse()

Hi!

Quoted 19 lines

Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
testcases/kernel/io/ltp-aiodio/dio_sparse.c +1 -1
 	memset(bufptr, 0, writesize);
 	lseek(fd, offset, SEEK_SET);
-	for (i = offset; i < filesize;) {
+	for (i = 0; i < filesize;) {
Quoted 8 lines
testcases/kernel/io/ltp-aiodio/dio_sparse.c int dio_sparse(char *filename, int align, int writesize, int filesize, int offse · line 84
Quoted 3 lines
testcases/kernel/io/ltp-aiodio/dio_sparse.c +0 -0
 		if ((w = write(fd, bufptr, writesize)) != writesize) {
 			tst_resm(TBROK | TERRNO, "write() returned %d", w);
 			close(fd);
Hmm, it looks to me like the code actually does what it should have. Since we pass a filesize and offset so the test should actually write filesize - offset bytes. As far as I can tell the bug here is that the test is not checking that filesize > offset before it starts.
Quoted 2 lines
Sorry, but shouldn't we write the whole "writesize" from the "offset"? ADSP080 ADSP081 ADSP082 ADSP083 did reproduce the BUG via above modification...
I think we pass an "offset" is to make sure writing from the file offset, but the actual write size should be just the "writesize" argument.

Quoted 6 lines

As the second case of kernel commit 9ecd10b says:
2. Direct writes starting from or beyong i_size (not inside i_size) also could trigger block allocation and expose stale data. For example, consider a sparse file with i_size of 2k, and a write to offset 2k or 3k into the file, with a filesystem block size of 4k. (Thanks to Jeff Moyer for pointing this case out in his review.)

Ah, looking at the git log, the offset was just added recently, that's why the code is confusing, previously the filesize was size of the file...

So what about we rename the writesize to blocksize and filesize to writesize so that it's clear what the function dio_sparse does.

int dio_sparse(char *filename, int align, int blocksize, int writesize, int offset)

Also shouldn't we truncate the file to offset + filesize rather than just filesize? And then pass the size to the children as offset + filesize as well?

Signature · 2 lines

Cyril Hrubis
[email protected]

Guangwen Feng

author

2017-02-14 08:53 UTC #13 lore raw

PATCH 1/2 ltp-aiodio/dio_sparse: Fix write in dio_sparse()

Hi!

Quoted 27 lines · Cyril Hrubis

Hi!
Quoted 19 lines
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
testcases/kernel/io/ltp-aiodio/dio_sparse.c +1 -1
 	memset(bufptr, 0, writesize);
 	lseek(fd, offset, SEEK_SET);
-	for (i = offset; i < filesize;) {
+	for (i = 0; i < filesize;) {
Quoted 8 lines
testcases/kernel/io/ltp-aiodio/dio_sparse.c int dio_sparse(char *filename, int align, int writesize, int filesize, int offse · line 84
Quoted 3 lines
testcases/kernel/io/ltp-aiodio/dio_sparse.c +0 -0
 		if ((w = write(fd, bufptr, writesize)) != writesize) {
 			tst_resm(TBROK | TERRNO, "write() returned %d", w);
 			close(fd);
Hmm, it looks to me like the code actually does what it should have. Since we pass a filesize and offset so the test should actually write filesize - offset bytes. As far as I can tell the bug here is that the test is not checking that filesize > offset before it starts.
Quoted 2 lines
Sorry, but shouldn't we write the whole "writesize" from the "offset"? ADSP080 ADSP081 ADSP082 ADSP083 did reproduce the BUG via above modification...
I think we pass an "offset" is to make sure writing from the file offset, but the actual write size should be just the "writesize" argument.
Quoted 6 lines
As the second case of kernel commit 9ecd10b says:
2. Direct writes starting from or beyong i_size (not inside i_size) also could trigger block allocation and expose stale data. For example, consider a sparse file with i_size of 2k, and a write to offset 2k or 3k into the file, with a filesystem block size of 4k. (Thanks to Jeff Moyer for pointing this case out in his review.)

Quoted 3 lines

Ah, looking at the git log, the offset was just added recently, that's why the code is confusing, previously the filesize was size of the file...

Yes, it seems that the code get confusing after the offset was added, and yes, previously the filesize was the size of the file, but sorry, according to my understanding it still represents the size of the file now.

Quoted 9 lines

So what about we rename the writesize to blocksize and filesize to writesize so that it's clear what the function dio_sparse does.
int dio_sparse(char *filename, int align, int blocksize, int writesize, int offset)
Also shouldn't we truncate the file to offset + filesize rather than just filesize? And then pass the size to the children as offset + filesize as well?

In my humble opinion, there was no problem at all in the code before the offset was added, and look at the commit message of adding offset, the aim is to achieve the second case of kernel commit 9ecd10b above, make it to direct write from or beyond the end of the file.

So, I think the writesize, filesize and offset should be independent of each other, the "offset" here is not filesize's offset (i.e. filesize does not calculate with offset), but only to decide where we write start from.

Best Regards,
Guangwen Feng

Cyril Hrubis

2017-02-16 13:12 UTC #14 lore raw

PATCH 1/2 ltp-aiodio/dio_sparse: Fix write in dio_sparse()

Hi!

Quoted 7 lines

Quoted 7 lines
Quoted 6 lines
As the second case of kernel commit 9ecd10b says:
2. Direct writes starting from or beyong i_size (not inside i_size) also could trigger block allocation and expose stale data. For example, consider a sparse file with i_size of 2k, and a write to offset 2k or 3k into the file, with a filesystem block size of 4k. (Thanks to Jeff Moyer for pointing this case out in his review.)

Quoted 7 lines

Quoted 3 lines
Ah, looking at the git log, the offset was just added recently, that's why the code is confusing, previously the filesize was size of the file...
Yes, it seems that the code get confusing after the offset was added, and yes, previously the filesize was the size of the file, but sorry, according to my understanding it still represents the size of the file now.

Ok we are mixing two things together. One is how much size the file actually takes on a filesystem and second one is the file length.

Quoted 6 lines

Quoted 6 lines
So what about we rename the writesize to blocksize and filesize to writesize so that it's clear what the function dio_sparse does.
int dio_sparse(char *filename, int align, int blocksize, int writesize, int offset)

Quoted 8 lines

Quoted 3 lines
Also shouldn't we truncate the file to offset + filesize rather than just filesize? And then pass the size to the children as offset + filesize as well?
In my humble opinion, there was no problem at all in the code before the offset was added, and look at the commit message of adding offset, the aim is to achieve the second case of kernel commit 9ecd10b above, make it to direct write from or beyond the end of the file.

The patch is correct, there is no doubt about that.

Quoted 3 lines

So, I think the writesize, filesize and offset should be independent of each other, the "offset" here is not filesize's offset (i.e. filesize does not calculate with offset), but only to decide where we write start from.

Okay, but still the writesize should be rename to something as block_size.

Signature · 2 lines

Cyril Hrubis
[email protected]