* [LTP] [PATCH 1/2] ltp-aiodio/dio_sparse: Fix write in dio_sparse() @ 2017-01-19 9:32 Guangwen Feng 2017-01-19 9:32 ` [LTP] [PATCH 2/2] ltp-aiodio/dio_sparse: Fix usleep in read_sparse() Guangwen Feng 2017-01-24 13:40 ` [LTP] [PATCH 1/2] ltp-aiodio/dio_sparse: Fix write in dio_sparse() Cyril Hrubis 0 siblings, 2 replies; 15+ messages in thread From: Guangwen Feng @ 2017-01-19 9:32 UTC (permalink / raw) To: ltp 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 <fenggw-fnst@cn.fujitsu.com> --- testcases/kernel/io/ltp-aiodio/dio_sparse.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testcases/kernel/io/ltp-aiodio/dio_sparse.c b/testcases/kernel/io/ltp-aiodio/dio_sparse.c index 41b9929..3828ed7 100644 --- a/testcases/kernel/io/ltp-aiodio/dio_sparse.c +++ b/testcases/kernel/io/ltp-aiodio/dio_sparse.c @@ -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); -- 1.8.4.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [LTP] [PATCH 2/2] ltp-aiodio/dio_sparse: Fix usleep in read_sparse() 2017-01-19 9:32 [LTP] [PATCH 1/2] ltp-aiodio/dio_sparse: Fix write in dio_sparse() Guangwen Feng @ 2017-01-19 9:32 ` Guangwen Feng 2017-01-24 13:43 ` Cyril Hrubis 2017-01-24 13:40 ` [LTP] [PATCH 1/2] ltp-aiodio/dio_sparse: Fix write in dio_sparse() Cyril Hrubis 1 sibling, 1 reply; 15+ messages in thread From: Guangwen Feng @ 2017-01-19 9:32 UTC (permalink / raw) To: ltp 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 <fenggw-fnst@cn.fujitsu.com> --- testcases/kernel/io/ltp-aiodio/common_sparse.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testcases/kernel/io/ltp-aiodio/common_sparse.h b/testcases/kernel/io/ltp-aiodio/common_sparse.h index f7f4ef4..a7f5035 100644 --- a/testcases/kernel/io/ltp-aiodio/common_sparse.h +++ b/testcases/kernel/io/ltp-aiodio/common_sparse.h @@ -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) { -- 1.8.4.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [LTP] [PATCH 2/2] ltp-aiodio/dio_sparse: Fix usleep in read_sparse() 2017-01-19 9:32 ` [LTP] [PATCH 2/2] ltp-aiodio/dio_sparse: Fix usleep in read_sparse() Guangwen Feng @ 2017-01-24 13:43 ` Cyril Hrubis 2017-01-25 4:02 ` Guangwen Feng 0 siblings, 1 reply; 15+ messages in thread From: Cyril Hrubis @ 2017-01-24 13:43 UTC (permalink / raw) To: ltp Hi! > diff --git a/testcases/kernel/io/ltp-aiodio/common_sparse.h b/testcases/kernel/io/ltp-aiodio/common_sparse.h > index f7f4ef4..a7f5035 100644 > --- a/testcases/kernel/io/ltp-aiodio/common_sparse.h > +++ b/testcases/kernel/io/ltp-aiodio/common_sparse.h > @@ -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. > } > > if (fd == -1) { > -- > 1.8.4.2 > > > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp -- Cyril Hrubis chrubis@suse.cz ^ permalink raw reply [flat|nested] 15+ messages in thread
* [LTP] [PATCH 2/2] ltp-aiodio/dio_sparse: Fix usleep in read_sparse() 2017-01-24 13:43 ` Cyril Hrubis @ 2017-01-25 4:02 ` Guangwen Feng 2017-02-09 7:23 ` [LTP] [PATCH v2] " Guangwen Feng 0 siblings, 1 reply; 15+ messages in thread From: Guangwen Feng @ 2017-01-25 4:02 UTC (permalink / raw) To: ltp Hi Cyril, Thanks for your review. On 01/24/2017 09:43 PM, Cyril Hrubis wrote: > Hi! >> diff --git a/testcases/kernel/io/ltp-aiodio/common_sparse.h b/testcases/kernel/io/ltp-aiodio/common_sparse.h >> index f7f4ef4..a7f5035 100644 >> --- a/testcases/kernel/io/ltp-aiodio/common_sparse.h >> +++ b/testcases/kernel/io/ltp-aiodio/common_sparse.h >> @@ -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. > 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 >> } >> >> if (fd == -1) { >> -- >> 1.8.4.2 >> >> >> >> >> -- >> Mailing list info: https://lists.linux.it/listinfo/ltp > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [LTP] [PATCH v2] ltp-aiodio/dio_sparse: Fix usleep in read_sparse() 2017-01-25 4:02 ` Guangwen Feng @ 2017-02-09 7:23 ` Guangwen Feng 2017-02-09 9:35 ` Cyril Hrubis 0 siblings, 1 reply; 15+ messages in thread From: Guangwen Feng @ 2017-02-09 7:23 UTC (permalink / raw) To: ltp 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 <fenggw-fnst@cn.fujitsu.com> --- testcases/kernel/io/ltp-aiodio/common_sparse.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/testcases/kernel/io/ltp-aiodio/common_sparse.h b/testcases/kernel/io/ltp-aiodio/common_sparse.h index f7f4ef4..7297319 100644 --- a/testcases/kernel/io/ltp-aiodio/common_sparse.h +++ b/testcases/kernel/io/ltp-aiodio/common_sparse.h @@ -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) { -- 1.8.4.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [LTP] [PATCH v2] ltp-aiodio/dio_sparse: Fix usleep in read_sparse() 2017-02-09 7:23 ` [LTP] [PATCH v2] " Guangwen Feng @ 2017-02-09 9:35 ` Cyril Hrubis 2017-02-14 3:20 ` Guangwen Feng 0 siblings, 1 reply; 15+ messages in thread From: Cyril Hrubis @ 2017-02-09 9:35 UTC (permalink / raw) To: ltp Hi! > 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 <fenggw-fnst@cn.fujitsu.com> > --- > testcases/kernel/io/ltp-aiodio/common_sparse.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/testcases/kernel/io/ltp-aiodio/common_sparse.h b/testcases/kernel/io/ltp-aiodio/common_sparse.h > index f7f4ef4..7297319 100644 > --- a/testcases/kernel/io/ltp-aiodio/common_sparse.h > +++ b/testcases/kernel/io/ltp-aiodio/common_sparse.h > @@ -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); > } 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? -- Cyril Hrubis chrubis@suse.cz ^ permalink raw reply [flat|nested] 15+ messages in thread
* [LTP] [PATCH v2] ltp-aiodio/dio_sparse: Fix usleep in read_sparse() 2017-02-09 9:35 ` Cyril Hrubis @ 2017-02-14 3:20 ` Guangwen Feng 2017-03-21 8:08 ` [LTP] [PATCH] ltp-aiodio: Create the file before fork Guangwen Feng 0 siblings, 1 reply; 15+ messages in thread From: Guangwen Feng @ 2017-02-14 3:20 UTC (permalink / raw) To: ltp Hi! On 02/09/2017 05:35 PM, Cyril Hrubis wrote: > Hi! >> 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 <fenggw-fnst@cn.fujitsu.com> >> --- >> testcases/kernel/io/ltp-aiodio/common_sparse.h | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/testcases/kernel/io/ltp-aiodio/common_sparse.h b/testcases/kernel/io/ltp-aiodio/common_sparse.h >> index f7f4ef4..7297319 100644 >> --- a/testcases/kernel/io/ltp-aiodio/common_sparse.h >> +++ b/testcases/kernel/io/ltp-aiodio/common_sparse.h >> @@ -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); >> } > > 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [LTP] [PATCH] ltp-aiodio: Create the file before fork 2017-02-14 3:20 ` Guangwen Feng @ 2017-03-21 8:08 ` Guangwen Feng 2017-03-22 15:33 ` Cyril Hrubis 0 siblings, 1 reply; 15+ messages in thread From: Guangwen Feng @ 2017-03-21 8:08 UTC (permalink / raw) To: ltp 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 <fenggw-fnst@cn.fujitsu.com> --- 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(-) diff --git a/testcases/kernel/io/ltp-aiodio/aiodio_sparse.c b/testcases/kernel/io/ltp-aiodio/aiodio_sparse.c index 49a0915..d40e45b 100644 --- a/testcases/kernel/io/ltp-aiodio/aiodio_sparse.c +++ b/testcases/kernel/io/ltp-aiodio/aiodio_sparse.c @@ -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(); } diff --git a/testcases/kernel/io/ltp-aiodio/common_sparse.h b/testcases/kernel/io/ltp-aiodio/common_sparse.h index f7f4ef4..e8b906e 100644 --- a/testcases/kernel/io/ltp-aiodio/common_sparse.h +++ b/testcases/kernel/io/ltp-aiodio/common_sparse.h @@ -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", diff --git a/testcases/kernel/io/ltp-aiodio/dio_sparse.c b/testcases/kernel/io/ltp-aiodio/dio_sparse.c index 41b9929..67b338b 100644 --- a/testcases/kernel/io/ltp-aiodio/dio_sparse.c +++ b/testcases/kernel/io/ltp-aiodio/dio_sparse.c @@ -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(); } -- 1.8.4.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [LTP] [PATCH] ltp-aiodio: Create the file before fork 2017-03-21 8:08 ` [LTP] [PATCH] ltp-aiodio: Create the file before fork Guangwen Feng @ 2017-03-22 15:33 ` Cyril Hrubis 0 siblings, 0 replies; 15+ messages in thread From: Cyril Hrubis @ 2017-03-22 15:33 UTC (permalink / raw) To: ltp Hi! Pushed, thanks. -- Cyril Hrubis chrubis@suse.cz ^ permalink raw reply [flat|nested] 15+ messages in thread
* [LTP] [PATCH 1/2] ltp-aiodio/dio_sparse: Fix write in dio_sparse() 2017-01-19 9:32 [LTP] [PATCH 1/2] ltp-aiodio/dio_sparse: Fix write in dio_sparse() Guangwen Feng 2017-01-19 9:32 ` [LTP] [PATCH 2/2] ltp-aiodio/dio_sparse: Fix usleep in read_sparse() Guangwen Feng @ 2017-01-24 13:40 ` Cyril Hrubis 2017-01-25 4:33 ` Guangwen Feng 1 sibling, 1 reply; 15+ messages in thread From: Cyril Hrubis @ 2017-01-24 13:40 UTC (permalink / raw) To: ltp Hi! > 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 <fenggw-fnst@cn.fujitsu.com> > --- > testcases/kernel/io/ltp-aiodio/dio_sparse.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/testcases/kernel/io/ltp-aiodio/dio_sparse.c b/testcases/kernel/io/ltp-aiodio/dio_sparse.c > index 41b9929..3828ed7 100644 > --- a/testcases/kernel/io/ltp-aiodio/dio_sparse.c > +++ b/testcases/kernel/io/ltp-aiodio/dio_sparse.c > @@ -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. -- Cyril Hrubis chrubis@suse.cz ^ permalink raw reply [flat|nested] 15+ messages in thread
* [LTP] [PATCH 1/2] ltp-aiodio/dio_sparse: Fix write in dio_sparse() 2017-01-24 13:40 ` [LTP] [PATCH 1/2] ltp-aiodio/dio_sparse: Fix write in dio_sparse() Cyril Hrubis @ 2017-01-25 4:33 ` Guangwen Feng 2017-02-09 8:56 ` Guangwen Feng 0 siblings, 1 reply; 15+ messages in thread From: Guangwen Feng @ 2017-01-25 4:33 UTC (permalink / raw) To: ltp Hi Cyril, Thanks for your review. On 01/24/2017 09:40 PM, Cyril Hrubis wrote: > Hi! >> 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 <fenggw-fnst@cn.fujitsu.com> >> --- >> testcases/kernel/io/ltp-aiodio/dio_sparse.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/testcases/kernel/io/ltp-aiodio/dio_sparse.c b/testcases/kernel/io/ltp-aiodio/dio_sparse.c >> index 41b9929..3828ed7 100644 >> --- a/testcases/kernel/io/ltp-aiodio/dio_sparse.c >> +++ b/testcases/kernel/io/ltp-aiodio/dio_sparse.c >> @@ -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. 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [LTP] [PATCH 1/2] ltp-aiodio/dio_sparse: Fix write in dio_sparse() 2017-01-25 4:33 ` Guangwen Feng @ 2017-02-09 8:56 ` Guangwen Feng 2017-02-09 9:31 ` Cyril Hrubis 0 siblings, 1 reply; 15+ messages in thread From: Guangwen Feng @ 2017-02-09 8:56 UTC (permalink / raw) To: ltp Hi! On 01/25/2017 12:33 PM, Guangwen Feng wrote: > Hi Cyril, > > Thanks for your review. > > On 01/24/2017 09:40 PM, Cyril Hrubis wrote: >> Hi! >>> 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 <fenggw-fnst@cn.fujitsu.com> >>> --- >>> testcases/kernel/io/ltp-aiodio/dio_sparse.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/testcases/kernel/io/ltp-aiodio/dio_sparse.c b/testcases/kernel/io/ltp-aiodio/dio_sparse.c >>> index 41b9929..3828ed7 100644 >>> --- a/testcases/kernel/io/ltp-aiodio/dio_sparse.c >>> +++ b/testcases/kernel/io/ltp-aiodio/dio_sparse.c >>> @@ -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. > > 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 > > Best Regards, > Guangwen Feng ^ permalink raw reply [flat|nested] 15+ messages in thread
* [LTP] [PATCH 1/2] ltp-aiodio/dio_sparse: Fix write in dio_sparse() 2017-02-09 8:56 ` Guangwen Feng @ 2017-02-09 9:31 ` Cyril Hrubis 2017-02-14 8:53 ` Guangwen Feng 0 siblings, 1 reply; 15+ messages in thread From: Cyril Hrubis @ 2017-02-09 9:31 UTC (permalink / raw) To: ltp Hi! > >>> 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... > > 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.) 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? -- Cyril Hrubis chrubis@suse.cz ^ permalink raw reply [flat|nested] 15+ messages in thread
* [LTP] [PATCH 1/2] ltp-aiodio/dio_sparse: Fix write in dio_sparse() 2017-02-09 9:31 ` Cyril Hrubis @ 2017-02-14 8:53 ` Guangwen Feng 2017-02-16 13:12 ` Cyril Hrubis 0 siblings, 1 reply; 15+ messages in thread From: Guangwen Feng @ 2017-02-14 8:53 UTC (permalink / raw) To: ltp Hi! On 02/09/2017 05:31 PM, Cyril Hrubis wrote: > Hi! >>>>> 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... >> >> 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.) > > 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. > > 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [LTP] [PATCH 1/2] ltp-aiodio/dio_sparse: Fix write in dio_sparse() 2017-02-14 8:53 ` Guangwen Feng @ 2017-02-16 13:12 ` Cyril Hrubis 0 siblings, 0 replies; 15+ messages in thread From: Cyril Hrubis @ 2017-02-16 13:12 UTC (permalink / raw) To: ltp Hi! > >> 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... > > 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. > > > > 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. The patch is correct, there is no doubt about that. > 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. -- Cyril Hrubis chrubis@suse.cz ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-03-22 15:33 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-01-19 9:32 [LTP] [PATCH 1/2] ltp-aiodio/dio_sparse: Fix write in dio_sparse() Guangwen Feng 2017-01-19 9:32 ` [LTP] [PATCH 2/2] ltp-aiodio/dio_sparse: Fix usleep in read_sparse() Guangwen Feng 2017-01-24 13:43 ` Cyril Hrubis 2017-01-25 4:02 ` Guangwen Feng 2017-02-09 7:23 ` [LTP] [PATCH v2] " Guangwen Feng 2017-02-09 9:35 ` Cyril Hrubis 2017-02-14 3:20 ` Guangwen Feng 2017-03-21 8:08 ` [LTP] [PATCH] ltp-aiodio: Create the file before fork Guangwen Feng 2017-03-22 15:33 ` Cyril Hrubis 2017-01-24 13:40 ` [LTP] [PATCH 1/2] ltp-aiodio/dio_sparse: Fix write in dio_sparse() Cyril Hrubis 2017-01-25 4:33 ` Guangwen Feng 2017-02-09 8:56 ` Guangwen Feng 2017-02-09 9:31 ` Cyril Hrubis 2017-02-14 8:53 ` Guangwen Feng 2017-02-16 13:12 ` Cyril Hrubis
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.