[LTP] [PATCH] fs_fill: switch the SAFE MACROs back to ordinary syscall

[LTP] [PATCH] fs_fill: switch the SAFE MACROs back to ordinary syscall

[Li Wang]

To fix those brokens:
  safe_macros.c:225: BROK: tst_fill_fs.c:43: open(mntpoint/thread48/file0,65,0700) failed: ENOSPC
  safe_macros.c:169: BROK: fs_fill.c:49: mkdir(mntpoint/thread21,0700) failed: ENOSPC

Signed-off-by: Li Wang <liwang@redhat.com>
---
 lib/tst_fill_fs.c                     |  9 ++++++++-
 testcases/kernel/fs/fs_fill/fs_fill.c | 10 +++++++++-
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/lib/tst_fill_fs.c b/lib/tst_fill_fs.c
index 6b2bbbc..2775a2c 100644
--- a/lib/tst_fill_fs.c
+++ b/lib/tst_fill_fs.c
@@ -40,7 +40,14 @@ void tst_fill_fs(const char *path, int verbose)
 		if (verbose)
 			tst_res(TINFO, "Creating file %s size %zu", file, len);
 
-		fd = SAFE_OPEN(file, O_WRONLY | O_CREAT, 0700);
+		fd = open(file, O_WRONLY | O_CREAT, 0700);
+		if (fd == -1) {
+			if (errno != ENOSPC)
+				tst_brk(TBROK | TERRNO, "open()");
+
+			tst_res(TINFO | TERRNO, "open()");
+			return;
+		}
 
 		while (len) {
 			ret = write(fd, buf, MIN(len, sizeof(buf)));
diff --git a/testcases/kernel/fs/fs_fill/fs_fill.c b/testcases/kernel/fs/fs_fill/fs_fill.c
index a50a22f..f592cd5 100644
--- a/testcases/kernel/fs/fs_fill/fs_fill.c
+++ b/testcases/kernel/fs/fs_fill/fs_fill.c
@@ -41,12 +41,20 @@ struct worker {
 
 static void *worker(void *p)
 {
+	int ret;
 	struct worker *w = p;
 	DIR *d;
 	struct dirent *ent;
 	char file[PATH_MAX];
 
-	SAFE_MKDIR(w->dir, 0700);
+	ret = mkdir(w->dir, 0700);
+	if (ret == -1) {
+		if (errno != ENOSPC)
+			tst_brk(TBROK | TERRNO, "mkdir()");
+
+		tst_res(TINFO | TERRNO, "mkdir()");
+		return NULL;
+	}
 
 	while (run) {
 		tst_fill_fs(w->dir, 0);
-- 
2.9.3

[LTP] [PATCH] fs_fill: switch the SAFE MACROs back to ordinary syscall

[Jan Stancek]

----- Original Message -----
> To fix those brokens:
>   safe_macros.c:225: BROK: tst_fill_fs.c:43:
>   open(mntpoint/thread48/file0,65,0700) failed: ENOSPC
>   safe_macros.c:169: BROK: fs_fill.c:49: mkdir(mntpoint/thread21,0700)
>   failed: ENOSPC
> 
> Signed-off-by: Li Wang <liwang@redhat.com>

Pushed.

Thanks,
Jan

[LTP] [PATCH] fs_fill: switch the SAFE MACROs back to ordinary syscall

[Cyril Hrubis]

Hi!
> >   safe_macros.c:225: BROK: tst_fill_fs.c:43:
> >   open(mntpoint/thread48/file0,65,0700) failed: ENOSPC
> >   safe_macros.c:169: BROK: fs_fill.c:49: mkdir(mntpoint/thread21,0700)
> >   failed: ENOSPC
> > 
> > Signed-off-by: Li Wang <liwang@redhat.com>
> 
> Pushed.

I do not like that the worker exits prematurely here if the mkdir fails,
that's why I suggested to mkdir() all the directories before we attempt
to run the the worker threads.

But I can always change that in a follow up patch...

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] fs_fill: switch the SAFE MACROs back to ordinary syscall

[Li Wang]

On Mon, Nov 20, 2017 at 9:19 PM, Cyril Hrubis <chrubis@suse.cz> wrote:
> Hi!
>> >   safe_macros.c:225: BROK: tst_fill_fs.c:43:
>> >   open(mntpoint/thread48/file0,65,0700) failed: ENOSPC
>> >   safe_macros.c:169: BROK: fs_fill.c:49: mkdir(mntpoint/thread21,0700)
>> >   failed: ENOSPC
>> >
>> > Signed-off-by: Li Wang <liwang@redhat.com>
>>
>> Pushed.
>
> I do not like that the worker exits prematurely here if the mkdir fails,
> that's why I suggested to mkdir() all the directories before we attempt
> to run the the worker threads.

My bad. Seems I did not fully get your point before.

>
> But I can always change that in a follow up patch...

How about this following change:

--- a/testcases/kernel/fs/fs_fill/fs_fill.c
+++ b/testcases/kernel/fs/fs_fill/fs_fill.c
@@ -41,21 +41,11 @@ struct worker {

 static void *worker(void *p)
 {
-       int ret;
        struct worker *w = p;
        DIR *d;
        struct dirent *ent;
        char file[PATH_MAX];

-       ret = mkdir(w->dir, 0700);
-       if (ret == -1) {
-               if (errno != ENOSPC)
-                       tst_brk(TBROK | TERRNO, "mkdir()");
-
-               tst_res(TINFO | TERRNO, "mkdir()");
-               return NULL;
-       }
-
        while (run) {
                tst_fill_fs(w->dir, 0);

@@ -92,6 +82,10 @@ static void testrun(void)
        for (i = 0; i < nthreads; i++) {
                snprintf(workers[i].dir, sizeof(workers[i].dir),
                         MNTPOINT "/thread%i", i + 1);
+               SAFE_MKDIR(workers[i].dir, 0700);
+       }
+
+       for (i = 0; i < nthreads; i++) {
                SAFE_PTHREAD_CREATE(&threads[i], NULL, worker, &workers[i]);
        }


>
> --
> Cyril Hrubis
> chrubis@suse.cz



-- 
Li Wang
liwang@redhat.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-fs_fill-create-all-the-directories-befere-run-worker.patch
Type: text/x-patch
Size: 1409 bytes
Desc: not available
URL: <http://lists.linux.it/pipermail/ltp/attachments/20171121/d78e751e/attachment.bin>

[LTP] [PATCH] fs_fill: switch the SAFE MACROs back to ordinary syscall

[Cyril Hrubis]

Hi!
> > But I can always change that in a follow up patch...
> 
> How about this following change:
> 
> --- a/testcases/kernel/fs/fs_fill/fs_fill.c
> +++ b/testcases/kernel/fs/fs_fill/fs_fill.c
> @@ -41,21 +41,11 @@ struct worker {
> 
>  static void *worker(void *p)
>  {
> -       int ret;
>         struct worker *w = p;
>         DIR *d;
>         struct dirent *ent;
>         char file[PATH_MAX];
> 
> -       ret = mkdir(w->dir, 0700);
> -       if (ret == -1) {
> -               if (errno != ENOSPC)
> -                       tst_brk(TBROK | TERRNO, "mkdir()");
> -
> -               tst_res(TINFO | TERRNO, "mkdir()");
> -               return NULL;
> -       }
> -
>         while (run) {
>                 tst_fill_fs(w->dir, 0);
> 
> @@ -92,6 +82,10 @@ static void testrun(void)
>         for (i = 0; i < nthreads; i++) {
>                 snprintf(workers[i].dir, sizeof(workers[i].dir),
>                          MNTPOINT "/thread%i", i + 1);
> +               SAFE_MKDIR(workers[i].dir, 0700);
> +       }
> +
> +       for (i = 0; i < nthreads; i++) {
>                 SAFE_PTHREAD_CREATE(&threads[i], NULL, worker, &workers[i]);
>         }

This one looks cleaner to me.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] fs_fill: switch the SAFE MACROs back to ordinary syscall

[Jan Stancek]

----- Original Message -----
> Hi!
> > > But I can always change that in a follow up patch...
> > 
> > How about this following change:
> > 
> > --- a/testcases/kernel/fs/fs_fill/fs_fill.c
> > +++ b/testcases/kernel/fs/fs_fill/fs_fill.c
> > @@ -41,21 +41,11 @@ struct worker {
> > 
> >  static void *worker(void *p)
> >  {
> > -       int ret;
> >         struct worker *w = p;
> >         DIR *d;
> >         struct dirent *ent;
> >         char file[PATH_MAX];
> > 
> > -       ret = mkdir(w->dir, 0700);
> > -       if (ret == -1) {
> > -               if (errno != ENOSPC)
> > -                       tst_brk(TBROK | TERRNO, "mkdir()");
> > -
> > -               tst_res(TINFO | TERRNO, "mkdir()");
> > -               return NULL;
> > -       }
> > -
> >         while (run) {
> >                 tst_fill_fs(w->dir, 0);
> > 
> > @@ -92,6 +82,10 @@ static void testrun(void)
> >         for (i = 0; i < nthreads; i++) {
> >                 snprintf(workers[i].dir, sizeof(workers[i].dir),
> >                          MNTPOINT "/thread%i", i + 1);
> > +               SAFE_MKDIR(workers[i].dir, 0700);
> > +       }
> > +
> > +       for (i = 0; i < nthreads; i++) {
> >                 SAFE_PTHREAD_CREATE(&threads[i], NULL, worker,
> >                 &workers[i]);
> >         }
> 
> This one looks cleaner to me.

But perhaps this should go to setup(), otherwise with "-i 2"
it hits TBROK on mkdir.

I tried adding RMDIR after join, but unlink doesn't seem to remove
everything:
safe_macros.c:184: BROK: fs_fill.c:108: rmdir(mntpoint/thread4) failed: ENOTEMPTY

Regards,
Jan

[LTP] [PATCH] fs_fill: switch the SAFE MACROs back to ordinary syscall

[Jan Stancek]

----- Original Message -----
> 
> ----- Original Message -----
> > Hi!
> > > > But I can always change that in a follow up patch...
> > > 
> > > How about this following change:
> > > 
> > > --- a/testcases/kernel/fs/fs_fill/fs_fill.c
> > > +++ b/testcases/kernel/fs/fs_fill/fs_fill.c
> > > @@ -41,21 +41,11 @@ struct worker {
> > > 
> > >  static void *worker(void *p)
> > >  {
> > > -       int ret;
> > >         struct worker *w = p;
> > >         DIR *d;
> > >         struct dirent *ent;
> > >         char file[PATH_MAX];
> > > 
> > > -       ret = mkdir(w->dir, 0700);
> > > -       if (ret == -1) {
> > > -               if (errno != ENOSPC)
> > > -                       tst_brk(TBROK | TERRNO, "mkdir()");
> > > -
> > > -               tst_res(TINFO | TERRNO, "mkdir()");
> > > -               return NULL;
> > > -       }
> > > -
> > >         while (run) {
> > >                 tst_fill_fs(w->dir, 0);
> > > 
> > > @@ -92,6 +82,10 @@ static void testrun(void)
> > >         for (i = 0; i < nthreads; i++) {
> > >                 snprintf(workers[i].dir, sizeof(workers[i].dir),
> > >                          MNTPOINT "/thread%i", i + 1);
> > > +               SAFE_MKDIR(workers[i].dir, 0700);
> > > +       }
> > > +
> > > +       for (i = 0; i < nthreads; i++) {
> > >                 SAFE_PTHREAD_CREATE(&threads[i], NULL, worker,
> > >                 &workers[i]);
> > >         }
> > 
> > This one looks cleaner to me.
> 
> But perhaps this should go to setup(), otherwise with "-i 2"
> it hits TBROK on mkdir.
> 
> I tried adding RMDIR after join, but unlink doesn't seem to remove
> everything:
> safe_macros.c:184: BROK: fs_fill.c:108: rmdir(mntpoint/thread4) failed:
> ENOTEMPTY

I see, there's "break" in loop that does unlink. Which is likely by design,
so we hit ENOSPC sooner.

> 
> Regards,
> Jan
> 
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
> 

[LTP] [PATCH] fs_fill: switch the SAFE MACROs back to ordinary syscall

[Cyril Hrubis]

Hi!
> > But perhaps this should go to setup(), otherwise with "-i 2"
> > it hits TBROK on mkdir.
> > 
> > I tried adding RMDIR after join, but unlink doesn't seem to remove
> > everything:
> > safe_macros.c:184: BROK: fs_fill.c:108: rmdir(mntpoint/thread4) failed:
> > ENOTEMPTY
> 
> I see, there's "break" in loop that does unlink. Which is likely by design,
> so we hit ENOSPC sooner.

That was the intention. The initial idea is that many threads race on
writing into nearly full filesystem. I guess I should have written
better description in the top level comment.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] fs_fill: switch the SAFE MACROs back to ordinary syscall

[Cyril Hrubis]

Hi!
> But perhaps this should go to setup(), otherwise with "-i 2"
> it hits TBROK on mkdir.

You are right, the whole worker initialization including the MKDIR()
belongs to setup() function.

-- 
Cyril Hrubis
chrubis@suse.cz