All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v5 1/2] fsstress: add mwrite/mread into test operation list
@ 2017-03-21  9:18 Zorro Lang
  2017-03-21  9:18 ` [PATCH v5 2/2] xfs/068: skip new fsstress operations mwrite and mread Zorro Lang
  2017-03-22  6:58 ` [PATCH v5 1/2] fsstress: add mwrite/mread into test operation list Eryu Guan
  0 siblings, 2 replies; 7+ messages in thread
From: Zorro Lang @ 2017-03-21  9:18 UTC (permalink / raw)
  To: fstests

mmap as a popular and basic operation, most of softwares use it to
access files. More and more customers report bugs related with
mmap/munmap and other stress conditions.

So add mmap read/write test into fsstress to increase mmap related
stress to reproduce or find more bugs easily.

Signed-off-by: Zorro Lang <zlang@redhat.com>
---

Nothing changed from V4. Send V5 for fixing xfs/068 failure together.

Thanks,
Zorro

 ltp/fsstress.c | 187 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/global.h   |   4 ++
 2 files changed, 191 insertions(+)

diff --git a/ltp/fsstress.c b/ltp/fsstress.c
index 7e7cf60..e341aa1 100644
--- a/ltp/fsstress.c
+++ b/ltp/fsstress.c
@@ -69,6 +69,8 @@ typedef enum {
 	OP_LINK,
 	OP_MKDIR,
 	OP_MKNOD,
+	OP_MREAD,
+	OP_MWRITE,
 	OP_PUNCH,
 	OP_ZERO,
 	OP_COLLAPSE,
@@ -168,6 +170,8 @@ void	getdents_f(int, long);
 void	link_f(int, long);
 void	mkdir_f(int, long);
 void	mknod_f(int, long);
+void	mread_f(int, long);
+void	mwrite_f(int, long);
 void	punch_f(int, long);
 void	zero_f(int, long);
 void	collapse_f(int, long);
@@ -208,6 +212,8 @@ opdesc_t	ops[] = {
 	{ OP_LINK, "link", link_f, 1, 1 },
 	{ OP_MKDIR, "mkdir", mkdir_f, 2, 1 },
 	{ OP_MKNOD, "mknod", mknod_f, 2, 1 },
+	{ OP_MREAD, "mread", mread_f, 4, 0 },
+	{ OP_MWRITE, "mwrite", mwrite_f, 4, 1 },
 	{ OP_PUNCH, "punch", punch_f, 1, 1 },
 	{ OP_ZERO, "zero", zero_f, 1, 1 },
 	{ OP_COLLAPSE, "collapse", collapse_f, 1, 1 },
@@ -2656,6 +2662,187 @@ mknod_f(int opno, long r)
 }
 
 void
+mread_f(int opno, long r)
+{
+	char		*addr;
+	char		*buf;
+	int		e;
+	pathname_t	f;
+	int		fd;
+	size_t		len;
+	__int64_t	lr;
+	off64_t		off;
+	int		flags;
+	struct stat64	stb;
+	int		v;
+	char		st[1024];
+
+	init_pathname(&f);
+	if (!get_fname(FT_REGFILE, r, &f, NULL, NULL, &v)) {
+		if (v)
+			printf("%d/%d: mread - no filename\n", procid, opno);
+		free_pathname(&f);
+		return;
+	}
+	fd = open_path(&f, O_RDONLY);
+	e = fd < 0 ? errno : 0;
+	check_cwd();
+	if (fd < 0) {
+		if (v)
+			printf("%d/%d: mread - open %s failed %d\n",
+			       procid, opno, f.path, e);
+		free_pathname(&f);
+		return;
+	}
+	if (fstat64(fd, &stb) < 0) {
+		if (v)
+			printf("%d/%d: mread - fstat64 %s failed %d\n",
+			       procid, opno, f.path, errno);
+		free_pathname(&f);
+		close(fd);
+		return;
+	}
+	if (stb.st_size == 0) {
+		if (v)
+			printf("%d/%d: mread - %s%s zero size\n", procid, opno,
+			       f.path, st);
+		free_pathname(&f);
+		close(fd);
+		return;
+	}
+
+	inode_info(st, sizeof(st), &stb, v);
+	lr = ((__int64_t)random() << 32) + random();
+	off = (off64_t)(lr % stb.st_size);
+	off &= (off64_t)(~(sysconf(_SC_PAGE_SIZE) - 1));
+	len = (size_t)(random() % MIN(stb.st_size - off, FILELEN_MAX)) + 1;
+
+	/* try private file mappings with 20% rate */
+	flags = (random() % 20) ? MAP_SHARED : MAP_PRIVATE;
+	do {
+		addr = mmap(NULL, len, PROT_READ, flags, fd, off);
+		e = (addr == MAP_FAILED) ? errno : 0;
+		if (errno == ENOMEM && flags & MAP_PRIVATE) {
+		/* turn to shared mapping if memeory is not enough for private mapping */
+			flags = MAP_SHARED;
+		} else if (errno == ENOMEM && len > sysconf(_SC_PAGE_SIZE)) {
+		/* reduce mapping length, if memeory is not enough for shared mapping */
+			len /= 2;
+		}
+	} while (errno == ENOMEM && len > sysconf(_SC_PAGE_SIZE));
+	if (e && v)
+		printf("%d/%d: mread - mmap failed %s%s [%lld,%d,%s] %d\n",
+		       procid, opno, f.path, st, (long long)off, (int)len,
+		       (flags & MAP_PRIVATE) ? "MAP_PRIVATE" : "MAP_SHARED", e);
+
+	if (addr != MAP_FAILED) {
+		if ((buf = malloc(len)) != NULL) {
+			memcpy(buf, addr, len);
+			free(buf);
+		}
+		e = munmap(addr, len) < 0 ? errno : 0;
+		if (e && v)
+			printf("%d/%d: mread - munmap failed %s%s [%lld,%d] %d\n",
+			       procid, opno, f.path, st, (long long)off,
+			       (int)len, e);
+	}
+	if (v)
+		printf("%d/%d: mread %s%s [%lld,%d,%s] %d\n",
+		       procid, opno, f.path, st, (long long)off, (int)len,
+		       (flags & MAP_PRIVATE) ? "MAP_PRIVATE" : "MAP_SHARED", e);
+
+	free_pathname(&f);
+	close(fd);
+}
+
+void
+mwrite_f(int opno, long r)
+{
+	char		*addr;
+	int		e;
+	pathname_t	f;
+	int		fd;
+	size_t		len;
+	__int64_t	lr;
+	off64_t		off;
+	int		flags;
+	struct stat64	stb;
+	int		v;
+	char		st[1024];
+
+	init_pathname(&f);
+	if (!get_fname(FT_REGFILE, r, &f, NULL, NULL, &v)) {
+		if (v)
+			printf("%d/%d: mwrite - no filename\n", procid, opno);
+		free_pathname(&f);
+		return;
+	}
+	fd = open_path(&f, O_WRONLY);
+	e = fd < 0 ? errno : 0;
+	check_cwd();
+	if (fd < 0) {
+		if (v)
+			printf("%d/%d: mwrite - open %s failed %d\n",
+			       procid, opno, f.path, e);
+		free_pathname(&f);
+		return;
+	}
+	if (fstat64(fd, &stb) < 0) {
+		if (v)
+			printf("%d/%d: mwrite - fstat64 %s failed %d\n",
+			       procid, opno, f.path, errno);
+		free_pathname(&f);
+		close(fd);
+		return;
+	}
+	inode_info(st, sizeof(st), &stb, v);
+	lr = ((__int64_t)random() << 32) + random();
+	off = (off64_t)(lr % MIN(stb.st_size + (1024 * 1024), MAXFSIZE));
+	off %= maxfsize;
+	off &= (off64_t)(~(sysconf(_SC_PAGE_SIZE) - 1));
+	len = (size_t)(random() % MIN(maxfsize - off, FILELEN_MAX)) + 1;
+
+	/*
+	 * truncate file to the size we need to map and access,
+	 * keep away SIGBUS / SIGSEGV killing this process
+	 */
+	e = truncate64_path(&f, off + len) < 0 ? errno : 0;
+	/* try private file mappings with 20% rate */
+	flags = (random() % 20) ? MAP_SHARED : MAP_PRIVATE;
+	do {
+		addr = mmap(NULL, len, PROT_WRITE, flags, fd, off);
+		e = (addr == MAP_FAILED) ? errno : 0;
+		if (errno == ENOMEM && flags & MAP_PRIVATE) {
+		/* turn to shared mapping if memeory is not enough for private mapping */
+			flags = MAP_SHARED;
+		} else if (errno == ENOMEM && len > sysconf(_SC_PAGE_SIZE)) {
+		/* reduce mapping length, if memeory is not enough for shared mapping */
+			len /= 2;
+		}
+	} while (errno == ENOMEM && len > sysconf(_SC_PAGE_SIZE));
+	if (e && v)
+		printf("%d/%d: mwrite - mmap failed %s%s [%lld,%d,%s] %d\n",
+		       procid, opno, f.path, st, (long long)off, (int)len,
+		       (flags & MAP_PRIVATE) ? "MAP_PRIVATE" : "MAP_SHARED", e);
+
+	if (addr != MAP_FAILED) {
+		memset(addr, nameseq & 0xff, len);
+		e = munmap(addr, len) < 0 ? errno : 0;
+		if (e && v)
+			printf("%d/%d: mwrite - munmap failed %s%s [%lld,%d] %d\n",
+			       procid, opno, f.path, st, (long long)off,
+			       (int)len, e);
+	}
+	if (v)
+		printf("%d/%d: mwrite %s%s [%lld,%d,%s] %d\n",
+		       procid, opno, f.path, st, (long long)off, (int)len,
+		       (flags & MAP_PRIVATE) ? "MAP_PRIVATE" : "MAP_SHARED", e);
+
+	free_pathname(&f);
+	close(fd);
+}
+
+void
 punch_f(int opno, long r)
 {
 #ifdef HAVE_LINUX_FALLOC_H
diff --git a/src/global.h b/src/global.h
index f63246b..51d1e94 100644
--- a/src/global.h
+++ b/src/global.h
@@ -178,4 +178,8 @@
 
 #endif /* HAVE_LINUX_FALLOC_H */
 
+#ifndef HAVE_SYS_MMAN_H
+#include <sys/mman.h>
+#endif
+
 #endif /* GLOBAL_H */
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PQcxBKCX5YTpkKY057SbK10&r=-yMrTV4jriXR7ieyzzjV-QgHBD0UDw8ixoR77aMeAHE&m=IGSaKRUSY4SQQ2IYAZRqk7ZYnn2imvSu4PJZwbgMmpM&s=iHqVs2zcmOV2--PaUzjsuoaP8F0piIiqcfmNqRevqsA&e= 

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v5 2/2] xfs/068: skip new fsstress operations mwrite and mread
  2017-03-21  9:18 [PATCH v5 1/2] fsstress: add mwrite/mread into test operation list Zorro Lang
@ 2017-03-21  9:18 ` Zorro Lang
  2017-03-21 10:55   ` Amir Goldstein
  2017-03-22  6:58 ` [PATCH v5 1/2] fsstress: add mwrite/mread into test operation list Eryu Guan
  1 sibling, 1 reply; 7+ messages in thread
From: Zorro Lang @ 2017-03-21  9:18 UTC (permalink / raw)
  To: fstests

The output of x/068 will be perturbed by new fsstress operations.
For keep away breaking x/068's golden image, skip mread and
mwrite which are brought in by a new fsstress patch.

Reported-by: Eryu Guan <eguan@redhat.com>
Signed-off-by: Zorro Lang <zlang@redhat.com>
---
 tests/xfs/068 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/xfs/068 b/tests/xfs/068
index 4dac95e..568ba60 100755
--- a/tests/xfs/068
+++ b/tests/xfs/068
@@ -44,7 +44,7 @@ _supported_fs xfs
 _supported_os Linux
 
 # need to ensure new fsstress operations don't perturb expected output
-FSSTRESS_AVOID="-f insert=0 $FSSTRESS_AVOID"
+FSSTRESS_AVOID="-f insert=0 -f mwrite=0 -f mread=0 $FSSTRESS_AVOID"
 _create_dumpdir_stress_num 4096
 _do_dump_restore
 
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PQcxBKCX5YTpkKY057SbK10&r=-yMrTV4jriXR7ieyzzjV-QgHBD0UDw8ixoR77aMeAHE&m=96dJ5wHk8dO6CXdf0pO5mLBQS3qWZv67us9m4CvqANI&s=YADzqekwOHb89zRff6FLwVTTaMuOYo19VrQRxUAm1ms&e= 

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v5 2/2] xfs/068: skip new fsstress operations mwrite and mread
  2017-03-21  9:18 ` [PATCH v5 2/2] xfs/068: skip new fsstress operations mwrite and mread Zorro Lang
@ 2017-03-21 10:55   ` Amir Goldstein
  2017-03-21 21:34     ` Dave Chinner
  0 siblings, 1 reply; 7+ messages in thread
From: Amir Goldstein @ 2017-03-21 10:55 UTC (permalink / raw)
  To: Zorro Lang, Eric Sandeen; +Cc: fstests, Eryu Guan, Dave Chinner

On Tue, Mar 21, 2017 at 5:18 AM, Zorro Lang <zlang@redhat.com> wrote:
> The output of x/068 will be perturbed by new fsstress operations.
> For keep away breaking x/068's golden image, skip mread and
> mwrite which are brought in by a new fsstress patch.
>
> Reported-by: Eryu Guan <eguan@redhat.com>
> Signed-off-by: Zorro Lang <zlang@redhat.com>
> ---
>  tests/xfs/068 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/xfs/068 b/tests/xfs/068
> index 4dac95e..568ba60 100755
> --- a/tests/xfs/068
> +++ b/tests/xfs/068
> @@ -44,7 +44,7 @@ _supported_fs xfs
>  _supported_os Linux
>
>  # need to ensure new fsstress operations don't perturb expected output
> -FSSTRESS_AVOID="-f insert=0 $FSSTRESS_AVOID"
> +FSSTRESS_AVOID="-f insert=0 -f mwrite=0 -f mread=0 $FSSTRESS_AVOID"

What?? This is so fragile!

If the test wanted to restrict fsstress to a pre-defined set of operations, then
_create_dumpdir_stress_num() should have prefixed '_param' with -z and
mention all pre-defined ops (and not only the more frequent ones).

I am very surprised that Dave was the one that made this bandaid.

Eric,

Can you say if adding -z to _create_dumpdir_stress_num() params
would be fine with the 2 tests that use it - xfs/022 and xfs/068?

068 output would have to be fixed to accommodate this change,
but then we can remove the bandaid above and never think of
this ever again.




>  _create_dumpdir_stress_num 4096
>  _do_dump_restore
>
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIBaQ&c=RoP1YumCXCgaWHvlZYR8PQcxBKCX5YTpkKY057SbK10&r=-yMrTV4jriXR7ieyzzjV-QgHBD0UDw8ixoR77aMeAHE&m=7B_Fox-9vCw4xAJ5cW1GVVeARkWiBAFy-Bmpxg6ZBuE&s=h38K-AlKoB7pMZoCp7s84E-4pClXFBq9pkXXWn0u-bo&e= 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v5 2/2] xfs/068: skip new fsstress operations mwrite and mread
  2017-03-21 10:55   ` Amir Goldstein
@ 2017-03-21 21:34     ` Dave Chinner
  2017-03-22  1:42       ` Amir Goldstein
  0 siblings, 1 reply; 7+ messages in thread
From: Dave Chinner @ 2017-03-21 21:34 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Zorro Lang, Eric Sandeen, fstests, Eryu Guan

On Tue, Mar 21, 2017 at 06:55:31AM -0400, Amir Goldstein wrote:
> On Tue, Mar 21, 2017 at 5:18 AM, Zorro Lang <zlang@redhat.com> wrote:
> > The output of x/068 will be perturbed by new fsstress operations.
> > For keep away breaking x/068's golden image, skip mread and
> > mwrite which are brought in by a new fsstress patch.
> >
> > Reported-by: Eryu Guan <eguan@redhat.com>
> > Signed-off-by: Zorro Lang <zlang@redhat.com>
> > ---
> >  tests/xfs/068 | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tests/xfs/068 b/tests/xfs/068
> > index 4dac95e..568ba60 100755
> > --- a/tests/xfs/068
> > +++ b/tests/xfs/068
> > @@ -44,7 +44,7 @@ _supported_fs xfs
> >  _supported_os Linux
> >
> >  # need to ensure new fsstress operations don't perturb expected output
> > -FSSTRESS_AVOID="-f insert=0 $FSSTRESS_AVOID"
> > +FSSTRESS_AVOID="-f insert=0 -f mwrite=0 -f mread=0 $FSSTRESS_AVOID"
> 
> What?? This is so fragile!

Yet, it is is the existing method for telling fsstress not to run
certain optionsi on an "as needed" basis:

$ git grep ^FSSTRESS_AVOID
tests/ext4/307:FSSTRESS_AVOID="$FSSTRESS_AVOID -ffsync=0 -fsync=0 -ffdatasync=0"
tests/generic/019:FSSTRESS_AVOID="$FSSTRESS_AVOID -ffsync=0 -fsync=0 -ffdatasync=0 -f setattr=1"
tests/generic/269:FSSTRESS_AVOID="$FSSTRESS_AVOID -ffsync=0 -fsync=0 -ffdatasync=0"
tests/generic/270:FSSTRESS_AVOID="$FSSTRESS_AVOID -ffsync=0 -fsync=0 -ffdatasync=0"
tests/xfs/068:FSSTRESS_AVOID="-f insert=0 $FSSTRESS_AVOID"

> If the test wanted to restrict fsstress to a pre-defined set of operations, then
> _create_dumpdir_stress_num() should have prefixed '_param' with -z and
> mention all pre-defined ops (and not only the more frequent ones).

Reality: _create_dumpdir_stress_num(), along with all the other dump
tests and infrastructure, was ported from the Irix version of
xfstests way back in:

commit 27fba05e66981c239c3be7a7e5a3aa0d8dc59247
Author: Nathan Scott <nathans@sgi.com>
Date:   Mon Jan 15 05:01:19 2001 +0000

    cmd/xfs/stress/001 1.6 Renamed to cmd/xfstests/001

The dump infrastructure is muddy and obtuse, and rather than risk
breaking other dump tests by changing infrastructure, I simply
applied the known, accepted method for making fsstress avoid certain
operations.

> I am very surprised that Dave was the one that made this bandaid.

Reality: Maintainers consider more than just the code when it comes
to merging things. Encouraging community participation and making
forwards progress is rather more important than having any single
change be perfect. IOWs, rather than have things drag on endlessly
by sending the patchset back to the authors for yet another round of
review, I simply said "fuck it, no more bikeshedding, we need to
merge this" and made the obvious, one line change that fixed the
only regression my testing uncovered.


> Eric,
> 
> Can you say if adding -z to _create_dumpdir_stress_num() params
> would be fine with the 2 tests that use it - xfs/022 and xfs/068?

That means dump tests will /never/ exercise new fsstress operations
which, IMO, is much worse than having to keep a test up to date with
new operations.

i.e. The whole point of adding new operations to fsstress and have
tests run them is that we automatically get coverage of new
functionality. Neutering this for all xfsdump tests by using "-z" is
- philosophically speaking - the wrong thing to do.

> 068 output would have to be fixed to accommodate this change,
> but then we can remove the bandaid above and never think of
> this ever again.

And so never get coverage of new functionality fsstress exposes that
may affect xfsdump behaviour, and hence we may very well miss
regressions those new operations cause....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PQcxBKCX5YTpkKY057SbK10&r=-yMrTV4jriXR7ieyzzjV-QgHBD0UDw8ixoR77aMeAHE&m=s-j946QcZgePt_3VNOjzsGT__vmiWVJef1uioYoIFnc&s=y4gWloi2adUFJL0DbFQVduLgSbUYkM0QBpM-DdrEeao&e= 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v5 2/2] xfs/068: skip new fsstress operations mwrite and mread
  2017-03-21 21:34     ` Dave Chinner
@ 2017-03-22  1:42       ` Amir Goldstein
  0 siblings, 0 replies; 7+ messages in thread
From: Amir Goldstein @ 2017-03-22  1:42 UTC (permalink / raw)
  To: Dave Chinner; +Cc: Zorro Lang, Eric Sandeen, fstests, Eryu Guan

On Tue, Mar 21, 2017 at 5:34 PM, Dave Chinner <david@fromorbit.com> wrote:
> On Tue, Mar 21, 2017 at 06:55:31AM -0400, Amir Goldstein wrote:
>> On Tue, Mar 21, 2017 at 5:18 AM, Zorro Lang <zlang@redhat.com> wrote:
>> > The output of x/068 will be perturbed by new fsstress operations.
>> > For keep away breaking x/068's golden image, skip mread and
>> > mwrite which are brought in by a new fsstress patch.
>> >
>> > Reported-by: Eryu Guan <eguan@redhat.com>
>> > Signed-off-by: Zorro Lang <zlang@redhat.com>
>> > ---
>> >  tests/xfs/068 | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/tests/xfs/068 b/tests/xfs/068
>> > index 4dac95e..568ba60 100755
>> > --- a/tests/xfs/068
>> > +++ b/tests/xfs/068
>> > @@ -44,7 +44,7 @@ _supported_fs xfs
>> >  _supported_os Linux
>> >
>> >  # need to ensure new fsstress operations don't perturb expected output
>> > -FSSTRESS_AVOID="-f insert=0 $FSSTRESS_AVOID"
>> > +FSSTRESS_AVOID="-f insert=0 -f mwrite=0 -f mread=0 $FSSTRESS_AVOID"
>>
>> What?? This is so fragile!
>
> Yet, it is is the existing method for telling fsstress not to run
> certain optionsi on an "as needed" basis:
>
> $ git grep ^FSSTRESS_AVOID
> tests/ext4/307:FSSTRESS_AVOID="$FSSTRESS_AVOID -ffsync=0 -fsync=0 -ffdatasync=0"
> tests/generic/019:FSSTRESS_AVOID="$FSSTRESS_AVOID -ffsync=0 -fsync=0 -ffdatasync=0 -f setattr=1"
> tests/generic/269:FSSTRESS_AVOID="$FSSTRESS_AVOID -ffsync=0 -fsync=0 -ffdatasync=0"
> tests/generic/270:FSSTRESS_AVOID="$FSSTRESS_AVOID -ffsync=0 -fsync=0 -ffdatasync=0"
> tests/xfs/068:FSSTRESS_AVOID="-f insert=0 $FSSTRESS_AVOID"
>

$ git grep -B 1 ^FSSTRESS_AVOID
tests/ext4/307-# Disable all sync operations to get higher load
tests/ext4/307:FSSTRESS_AVOID="$FSSTRESS_AVOID -ffsync=0 -fsync=0 -ffdatasync=0"
--
tests/generic/019-# Disable all sync operations to get higher load
tests/generic/019:FSSTRESS_AVOID="$FSSTRESS_AVOID -ffsync=0 -fsync=0
-ffdatasync=0 -f setattr=1"
--
tests/generic/269-# Disable all sync operations to get higher load
tests/generic/269:FSSTRESS_AVOID="$FSSTRESS_AVOID -ffsync=0 -fsync=0
-ffdatasync=0"
--
tests/generic/270-# Disable all sync operations to get higher load
tests/generic/270:FSSTRESS_AVOID="$FSSTRESS_AVOID -ffsync=0 -fsync=0
-ffdatasync=0"
--
tests/xfs/068-# need to ensure new fsstress operations don't perturb
expected output
tests/xfs/068:FSSTRESS_AVOID="-f insert=0 $FSSTRESS_AVOID"

There is a pattern here and xfs/086 doesn't match it.
It is perfectly understandable to want to exclude certain ops that
don't mix well with the test,
but to avoid an op instead of update the output? I am must be missing
the difficulty
of the alternative solution.

>> If the test wanted to restrict fsstress to a pre-defined set of operations, then
>> _create_dumpdir_stress_num() should have prefixed '_param' with -z and
>> mention all pre-defined ops (and not only the more frequent ones).
>
> Reality: _create_dumpdir_stress_num(), along with all the other dump
> tests and infrastructure, was ported from the Irix version of
> xfstests way back in:
>
> commit 27fba05e66981c239c3be7a7e5a3aa0d8dc59247
> Author: Nathan Scott <nathans@sgi.com>
> Date:   Mon Jan 15 05:01:19 2001 +0000
>
>     cmd/xfs/stress/001 1.6 Renamed to cmd/xfstests/001
>
> The dump infrastructure is muddy and obtuse, and rather than risk
> breaking other dump tests by changing infrastructure, I simply
> applied the known, accepted method for making fsstress avoid certain
> operations.
>
>> I am very surprised that Dave was the one that made this bandaid.
>
> Reality: Maintainers consider more than just the code when it comes
> to merging things. Encouraging community participation and making
> forwards progress is rather more important than having any single
> change be perfect. IOWs, rather than have things drag on endlessly
> by sending the patchset back to the authors for yet another round of
> review, I simply said "fuck it, no more bikeshedding, we need to
> merge this" and made the obvious, one line change that fixed the
> only regression my testing uncovered.
>

Music to my ears :-)

>
>> Eric,
>>
>> Can you say if adding -z to _create_dumpdir_stress_num() params
>> would be fine with the 2 tests that use it - xfs/022 and xfs/068?
>
> That means dump tests will /never/ exercise new fsstress operations
> which, IMO, is much worse than having to keep a test up to date with
> new operations.
>
> i.e. The whole point of adding new operations to fsstress and have
> tests run them is that we automatically get coverage of new
> functionality. Neutering this for all xfsdump tests by using "-z" is
> - philosophically speaking - the wrong thing to do.
>

Fair enough.
So why exclude the new fsstress operation instead of accomodating 086.out?
philosophically speaking - that would be the right thing to do. No?

>> 068 output would have to be fixed to accommodate this change,
>> but then we can remove the bandaid above and never think of
>> this ever again.
>
> And so never get coverage of new functionality fsstress exposes that
> may affect xfsdump behaviour, and hence we may very well miss
> regressions those new operations cause....

Operations like insert and mmap write....

I think we both agree that Zorro should try to remove the existing bandaid
and fix the expected output. Right?
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIBaQ&c=RoP1YumCXCgaWHvlZYR8PQcxBKCX5YTpkKY057SbK10&r=-yMrTV4jriXR7ieyzzjV-QgHBD0UDw8ixoR77aMeAHE&m=f7fmQEDFpnUZsNLHavw-MwAgqk9wIOJIAy8rwuvPvG8&s=D3hzcipuvInwqg6NvynTbbnNd3cPfaDoEPOWEmTj5wU&e= 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v5 1/2] fsstress: add mwrite/mread into test operation list
  2017-03-21  9:18 [PATCH v5 1/2] fsstress: add mwrite/mread into test operation list Zorro Lang
  2017-03-21  9:18 ` [PATCH v5 2/2] xfs/068: skip new fsstress operations mwrite and mread Zorro Lang
@ 2017-03-22  6:58 ` Eryu Guan
  2017-03-22 14:59   ` Zorro Lang
  1 sibling, 1 reply; 7+ messages in thread
From: Eryu Guan @ 2017-03-22  6:58 UTC (permalink / raw)
  To: Zorro Lang; +Cc: fstests

On Tue, Mar 21, 2017 at 05:18:02PM +0800, Zorro Lang wrote:
> mmap as a popular and basic operation, most of softwares use it to
> access files. More and more customers report bugs related with
> mmap/munmap and other stress conditions.
> 
> So add mmap read/write test into fsstress to increase mmap related
> stress to reproduce or find more bugs easily.
> 
> Signed-off-by: Zorro Lang <zlang@redhat.com>
> ---
> 
> Nothing changed from V4. Send V5 for fixing xfs/068 failure together.
> 
> Thanks,
> Zorro
> 
>  ltp/fsstress.c | 187 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/global.h   |   4 ++
>  2 files changed, 191 insertions(+)
> 
> diff --git a/ltp/fsstress.c b/ltp/fsstress.c
> index 7e7cf60..e341aa1 100644
> --- a/ltp/fsstress.c
> +++ b/ltp/fsstress.c
> @@ -69,6 +69,8 @@ typedef enum {
>  	OP_LINK,
>  	OP_MKDIR,
>  	OP_MKNOD,
> +	OP_MREAD,
> +	OP_MWRITE,
>  	OP_PUNCH,
>  	OP_ZERO,
>  	OP_COLLAPSE,
> @@ -168,6 +170,8 @@ void	getdents_f(int, long);
>  void	link_f(int, long);
>  void	mkdir_f(int, long);
>  void	mknod_f(int, long);
> +void	mread_f(int, long);
> +void	mwrite_f(int, long);
>  void	punch_f(int, long);
>  void	zero_f(int, long);
>  void	collapse_f(int, long);
> @@ -208,6 +212,8 @@ opdesc_t	ops[] = {
>  	{ OP_LINK, "link", link_f, 1, 1 },
>  	{ OP_MKDIR, "mkdir", mkdir_f, 2, 1 },
>  	{ OP_MKNOD, "mknod", mknod_f, 2, 1 },
> +	{ OP_MREAD, "mread", mread_f, 4, 0 },
> +	{ OP_MWRITE, "mwrite", mwrite_f, 4, 1 },

I don't think mread/mwrite are widely used as read/write, so freq 4
seems a bit high, freq 2 should be OK.

>  	{ OP_PUNCH, "punch", punch_f, 1, 1 },
>  	{ OP_ZERO, "zero", zero_f, 1, 1 },
>  	{ OP_COLLAPSE, "collapse", collapse_f, 1, 1 },
> @@ -2656,6 +2662,187 @@ mknod_f(int opno, long r)
>  }
>  
>  void
> +mread_f(int opno, long r)
> +{
> +	char		*addr;
> +	char		*buf;
> +	int		e;
> +	pathname_t	f;
> +	int		fd;
> +	size_t		len;
> +	__int64_t	lr;
> +	off64_t		off;
> +	int		flags;
> +	struct stat64	stb;
> +	int		v;
> +	char		st[1024];
> +
> +	init_pathname(&f);
> +	if (!get_fname(FT_REGFILE, r, &f, NULL, NULL, &v)) {
> +		if (v)
> +			printf("%d/%d: mread - no filename\n", procid, opno);
> +		free_pathname(&f);
> +		return;
> +	}
> +	fd = open_path(&f, O_RDONLY);
> +	e = fd < 0 ? errno : 0;
> +	check_cwd();
> +	if (fd < 0) {
> +		if (v)
> +			printf("%d/%d: mread - open %s failed %d\n",
> +			       procid, opno, f.path, e);
> +		free_pathname(&f);
> +		return;
> +	}
> +	if (fstat64(fd, &stb) < 0) {
> +		if (v)
> +			printf("%d/%d: mread - fstat64 %s failed %d\n",
> +			       procid, opno, f.path, errno);
> +		free_pathname(&f);
> +		close(fd);
> +		return;
> +	}
> +	if (stb.st_size == 0) {
> +		if (v)
> +			printf("%d/%d: mread - %s%s zero size\n", procid, opno,
> +			       f.path, st);
> +		free_pathname(&f);
> +		close(fd);
> +		return;
> +	}
> +
> +	inode_info(st, sizeof(st), &stb, v);
> +	lr = ((__int64_t)random() << 32) + random();
> +	off = (off64_t)(lr % stb.st_size);
> +	off &= (off64_t)(~(sysconf(_SC_PAGE_SIZE) - 1));
> +	len = (size_t)(random() % MIN(stb.st_size - off, FILELEN_MAX)) + 1;
> +
> +	/* try private file mappings with 20% rate */
> +	flags = (random() % 20) ? MAP_SHARED : MAP_PRIVATE;

This is not 20%, it's 5%. But I think 50%:50% should be OK?

> +	do {
> +		addr = mmap(NULL, len, PROT_READ, flags, fd, off);
> +		e = (addr == MAP_FAILED) ? errno : 0;
> +		if (errno == ENOMEM && flags & MAP_PRIVATE) {
> +		/* turn to shared mapping if memeory is not enough for private mapping */
> +			flags = MAP_SHARED;
> +		} else if (errno == ENOMEM && len > sysconf(_SC_PAGE_SIZE)) {
> +		/* reduce mapping length, if memeory is not enough for shared mapping */
> +			len /= 2;
> +		}
> +	} while (errno == ENOMEM && len > sysconf(_SC_PAGE_SIZE));

No need to retry on mmap, just let it fail and print out failure
message.

> +	if (e && v)
> +		printf("%d/%d: mread - mmap failed %s%s [%lld,%d,%s] %d\n",
> +		       procid, opno, f.path, st, (long long)off, (int)len,
> +		       (flags & MAP_PRIVATE) ? "MAP_PRIVATE" : "MAP_SHARED", e);
> +
> +	if (addr != MAP_FAILED) {
> +		if ((buf = malloc(len)) != NULL) {
> +			memcpy(buf, addr, len);
> +			free(buf);
> +		}
> +		e = munmap(addr, len) < 0 ? errno : 0;
> +		if (e && v)
> +			printf("%d/%d: mread - munmap failed %s%s [%lld,%d] %d\n",
> +			       procid, opno, f.path, st, (long long)off,
> +			       (int)len, e);
> +	}
> +	if (v)
> +		printf("%d/%d: mread %s%s [%lld,%d,%s] %d\n",
> +		       procid, opno, f.path, st, (long long)off, (int)len,
> +		       (flags & MAP_PRIVATE) ? "MAP_PRIVATE" : "MAP_SHARED", e);
> +
> +	free_pathname(&f);
> +	close(fd);
> +}
> +
> +void
> +mwrite_f(int opno, long r)
> +{
> +	char		*addr;
> +	int		e;
> +	pathname_t	f;
> +	int		fd;
> +	size_t		len;
> +	__int64_t	lr;
> +	off64_t		off;
> +	int		flags;
> +	struct stat64	stb;
> +	int		v;
> +	char		st[1024];
> +
> +	init_pathname(&f);
> +	if (!get_fname(FT_REGFILE, r, &f, NULL, NULL, &v)) {
> +		if (v)
> +			printf("%d/%d: mwrite - no filename\n", procid, opno);
> +		free_pathname(&f);
> +		return;
> +	}
> +	fd = open_path(&f, O_WRONLY);
> +	e = fd < 0 ? errno : 0;
> +	check_cwd();
> +	if (fd < 0) {
> +		if (v)
> +			printf("%d/%d: mwrite - open %s failed %d\n",
> +			       procid, opno, f.path, e);
> +		free_pathname(&f);
> +		return;
> +	}
> +	if (fstat64(fd, &stb) < 0) {
> +		if (v)
> +			printf("%d/%d: mwrite - fstat64 %s failed %d\n",
> +			       procid, opno, f.path, errno);
> +		free_pathname(&f);
> +		close(fd);
> +		return;
> +	}
> +	inode_info(st, sizeof(st), &stb, v);
> +	lr = ((__int64_t)random() << 32) + random();
> +	off = (off64_t)(lr % MIN(stb.st_size + (1024 * 1024), MAXFSIZE));
> +	off %= maxfsize;
> +	off &= (off64_t)(~(sysconf(_SC_PAGE_SIZE) - 1));
> +	len = (size_t)(random() % MIN(maxfsize - off, FILELEN_MAX)) + 1;
> +
> +	/*
> +	 * truncate file to the size we need to map and access,
> +	 * keep away SIGBUS / SIGSEGV killing this process
> +	 */
> +	e = truncate64_path(&f, off + len) < 0 ? errno : 0;

This implies a truncate(2) operation in mwrite. I don't think we need
this. Just mwrite within file size, let other operations like truncate
and write to extend file size.

> +	/* try private file mappings with 20% rate */
> +	flags = (random() % 20) ? MAP_SHARED : MAP_PRIVATE;

Again, this is 5%.

> +	do {
> +		addr = mmap(NULL, len, PROT_WRITE, flags, fd, off);
> +		e = (addr == MAP_FAILED) ? errno : 0;
> +		if (errno == ENOMEM && flags & MAP_PRIVATE) {
> +		/* turn to shared mapping if memeory is not enough for private mapping */
> +			flags = MAP_SHARED;
> +		} else if (errno == ENOMEM && len > sysconf(_SC_PAGE_SIZE)) {
> +		/* reduce mapping length, if memeory is not enough for shared mapping */
> +			len /= 2;
> +		}
> +	} while (errno == ENOMEM && len > sysconf(_SC_PAGE_SIZE));

And again, no need to retry on mmap failure, IMO, just let it fail.

> +	if (e && v)
> +		printf("%d/%d: mwrite - mmap failed %s%s [%lld,%d,%s] %d\n",
> +		       procid, opno, f.path, st, (long long)off, (int)len,
> +		       (flags & MAP_PRIVATE) ? "MAP_PRIVATE" : "MAP_SHARED", e);

Consider putting this flag translation into a "struct print_flags"? See
struct print_flags falloc_flags [].

Thanks,
Eryu

> +
> +	if (addr != MAP_FAILED) {
> +		memset(addr, nameseq & 0xff, len);
> +		e = munmap(addr, len) < 0 ? errno : 0;
> +		if (e && v)
> +			printf("%d/%d: mwrite - munmap failed %s%s [%lld,%d] %d\n",
> +			       procid, opno, f.path, st, (long long)off,
> +			       (int)len, e);
> +	}
> +	if (v)
> +		printf("%d/%d: mwrite %s%s [%lld,%d,%s] %d\n",
> +		       procid, opno, f.path, st, (long long)off, (int)len,
> +		       (flags & MAP_PRIVATE) ? "MAP_PRIVATE" : "MAP_SHARED", e);
> +
> +	free_pathname(&f);
> +	close(fd);
> +}
> +
> +void
>  punch_f(int opno, long r)
>  {
>  #ifdef HAVE_LINUX_FALLOC_H
> diff --git a/src/global.h b/src/global.h
> index f63246b..51d1e94 100644
> --- a/src/global.h
> +++ b/src/global.h
> @@ -178,4 +178,8 @@
>  
>  #endif /* HAVE_LINUX_FALLOC_H */
>  
> +#ifndef HAVE_SYS_MMAN_H
> +#include <sys/mman.h>
> +#endif
> +
>  #endif /* GLOBAL_H */
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PQcxBKCX5YTpkKY057SbK10&r=-yMrTV4jriXR7ieyzzjV-QgHBD0UDw8ixoR77aMeAHE&m=sRjm768aU_r6WAYkhCVs84Q0mbTl9Ls2luPcA_M2yfg&s=pLoANwaLvp5MiCf2M6V3CqwCOor767PphRpNErnuz9E&e= 
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PQcxBKCX5YTpkKY057SbK10&r=-yMrTV4jriXR7ieyzzjV-QgHBD0UDw8ixoR77aMeAHE&m=sRjm768aU_r6WAYkhCVs84Q0mbTl9Ls2luPcA_M2yfg&s=pLoANwaLvp5MiCf2M6V3CqwCOor767PphRpNErnuz9E&e= 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v5 1/2] fsstress: add mwrite/mread into test operation list
  2017-03-22  6:58 ` [PATCH v5 1/2] fsstress: add mwrite/mread into test operation list Eryu Guan
@ 2017-03-22 14:59   ` Zorro Lang
  0 siblings, 0 replies; 7+ messages in thread
From: Zorro Lang @ 2017-03-22 14:59 UTC (permalink / raw)
  To: Eryu Guan; +Cc: fstests

On Wed, Mar 22, 2017 at 02:58:44PM +0800, Eryu Guan wrote:
> On Tue, Mar 21, 2017 at 05:18:02PM +0800, Zorro Lang wrote:
> > mmap as a popular and basic operation, most of softwares use it to
> > access files. More and more customers report bugs related with
> > mmap/munmap and other stress conditions.
> > 
> > So add mmap read/write test into fsstress to increase mmap related
> > stress to reproduce or find more bugs easily.
> > 
> > Signed-off-by: Zorro Lang <zlang@redhat.com>
> > ---
> > 
> > Nothing changed from V4. Send V5 for fixing xfs/068 failure together.
> > 
> > Thanks,
> > Zorro
> > 
> >  ltp/fsstress.c | 187 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  src/global.h   |   4 ++
> >  2 files changed, 191 insertions(+)
> > 
> > diff --git a/ltp/fsstress.c b/ltp/fsstress.c
> > index 7e7cf60..e341aa1 100644
> > --- a/ltp/fsstress.c
> > +++ b/ltp/fsstress.c
> > @@ -69,6 +69,8 @@ typedef enum {
> >  	OP_LINK,
> >  	OP_MKDIR,
> >  	OP_MKNOD,
> > +	OP_MREAD,
> > +	OP_MWRITE,
> >  	OP_PUNCH,
> >  	OP_ZERO,
> >  	OP_COLLAPSE,
> > @@ -168,6 +170,8 @@ void	getdents_f(int, long);
> >  void	link_f(int, long);
> >  void	mkdir_f(int, long);
> >  void	mknod_f(int, long);
> > +void	mread_f(int, long);
> > +void	mwrite_f(int, long);
> >  void	punch_f(int, long);
> >  void	zero_f(int, long);
> >  void	collapse_f(int, long);
> > @@ -208,6 +212,8 @@ opdesc_t	ops[] = {
> >  	{ OP_LINK, "link", link_f, 1, 1 },
> >  	{ OP_MKDIR, "mkdir", mkdir_f, 2, 1 },
> >  	{ OP_MKNOD, "mknod", mknod_f, 2, 1 },
> > +	{ OP_MREAD, "mread", mread_f, 4, 0 },
> > +	{ OP_MWRITE, "mwrite", mwrite_f, 4, 1 },
> 
> I don't think mread/mwrite are widely used as read/write, so freq 4
> seems a bit high, freq 2 should be OK.

OK

> 
> >  	{ OP_PUNCH, "punch", punch_f, 1, 1 },
> >  	{ OP_ZERO, "zero", zero_f, 1, 1 },
> >  	{ OP_COLLAPSE, "collapse", collapse_f, 1, 1 },
> > @@ -2656,6 +2662,187 @@ mknod_f(int opno, long r)
> >  }
> >  
> >  void
> > +mread_f(int opno, long r)
> > +{
> > +	char		*addr;
> > +	char		*buf;
> > +	int		e;
> > +	pathname_t	f;
> > +	int		fd;
> > +	size_t		len;
> > +	__int64_t	lr;
> > +	off64_t		off;
> > +	int		flags;
> > +	struct stat64	stb;
> > +	int		v;
> > +	char		st[1024];
> > +
> > +	init_pathname(&f);
> > +	if (!get_fname(FT_REGFILE, r, &f, NULL, NULL, &v)) {
> > +		if (v)
> > +			printf("%d/%d: mread - no filename\n", procid, opno);
> > +		free_pathname(&f);
> > +		return;
> > +	}
> > +	fd = open_path(&f, O_RDONLY);
> > +	e = fd < 0 ? errno : 0;
> > +	check_cwd();
> > +	if (fd < 0) {
> > +		if (v)
> > +			printf("%d/%d: mread - open %s failed %d\n",
> > +			       procid, opno, f.path, e);
> > +		free_pathname(&f);
> > +		return;
> > +	}
> > +	if (fstat64(fd, &stb) < 0) {
> > +		if (v)
> > +			printf("%d/%d: mread - fstat64 %s failed %d\n",
> > +			       procid, opno, f.path, errno);
> > +		free_pathname(&f);
> > +		close(fd);
> > +		return;
> > +	}
> > +	if (stb.st_size == 0) {
> > +		if (v)
> > +			printf("%d/%d: mread - %s%s zero size\n", procid, opno,
> > +			       f.path, st);
> > +		free_pathname(&f);
> > +		close(fd);
> > +		return;
> > +	}
> > +
> > +	inode_info(st, sizeof(st), &stb, v);
> > +	lr = ((__int64_t)random() << 32) + random();
> > +	off = (off64_t)(lr % stb.st_size);
> > +	off &= (off64_t)(~(sysconf(_SC_PAGE_SIZE) - 1));
> > +	len = (size_t)(random() % MIN(stb.st_size - off, FILELEN_MAX)) + 1;
> > +
> > +	/* try private file mappings with 20% rate */
> > +	flags = (random() % 20) ? MAP_SHARED : MAP_PRIVATE;
> 
> This is not 20%, it's 5%. But I think 50%:50% should be OK?

Hah, sorry, my mistake. OK, I don't mind change it to 50%:50%.
I was thinking MAP_SHARED is used more popular?

> 
> > +	do {
> > +		addr = mmap(NULL, len, PROT_READ, flags, fd, off);
> > +		e = (addr == MAP_FAILED) ? errno : 0;
> > +		if (errno == ENOMEM && flags & MAP_PRIVATE) {
> > +		/* turn to shared mapping if memeory is not enough for private mapping */
> > +			flags = MAP_SHARED;
> > +		} else if (errno == ENOMEM && len > sysconf(_SC_PAGE_SIZE)) {
> > +		/* reduce mapping length, if memeory is not enough for shared mapping */
> > +			len /= 2;
> > +		}
> > +	} while (errno == ENOMEM && len > sysconf(_SC_PAGE_SIZE));
> 
> No need to retry on mmap, just let it fail and print out failure
> message.

Because file size can be very large, but we have limited memory size.
So I was afraid mmap can't be run mostly. Especially for private mapping.
But OK, that's easier if return directly.

> 
> > +	if (e && v)
> > +		printf("%d/%d: mread - mmap failed %s%s [%lld,%d,%s] %d\n",
> > +		       procid, opno, f.path, st, (long long)off, (int)len,
> > +		       (flags & MAP_PRIVATE) ? "MAP_PRIVATE" : "MAP_SHARED", e);
> > +
> > +	if (addr != MAP_FAILED) {
> > +		if ((buf = malloc(len)) != NULL) {
> > +			memcpy(buf, addr, len);
> > +			free(buf);
> > +		}
> > +		e = munmap(addr, len) < 0 ? errno : 0;
> > +		if (e && v)
> > +			printf("%d/%d: mread - munmap failed %s%s [%lld,%d] %d\n",
> > +			       procid, opno, f.path, st, (long long)off,
> > +			       (int)len, e);
> > +	}
> > +	if (v)
> > +		printf("%d/%d: mread %s%s [%lld,%d,%s] %d\n",
> > +		       procid, opno, f.path, st, (long long)off, (int)len,
> > +		       (flags & MAP_PRIVATE) ? "MAP_PRIVATE" : "MAP_SHARED", e);
> > +
> > +	free_pathname(&f);
> > +	close(fd);
> > +}
> > +
> > +void
> > +mwrite_f(int opno, long r)
> > +{
> > +	char		*addr;
> > +	int		e;
> > +	pathname_t	f;
> > +	int		fd;
> > +	size_t		len;
> > +	__int64_t	lr;
> > +	off64_t		off;
> > +	int		flags;
> > +	struct stat64	stb;
> > +	int		v;
> > +	char		st[1024];
> > +
> > +	init_pathname(&f);
> > +	if (!get_fname(FT_REGFILE, r, &f, NULL, NULL, &v)) {
> > +		if (v)
> > +			printf("%d/%d: mwrite - no filename\n", procid, opno);
> > +		free_pathname(&f);
> > +		return;
> > +	}
> > +	fd = open_path(&f, O_WRONLY);
> > +	e = fd < 0 ? errno : 0;
> > +	check_cwd();
> > +	if (fd < 0) {
> > +		if (v)
> > +			printf("%d/%d: mwrite - open %s failed %d\n",
> > +			       procid, opno, f.path, e);
> > +		free_pathname(&f);
> > +		return;
> > +	}
> > +	if (fstat64(fd, &stb) < 0) {
> > +		if (v)
> > +			printf("%d/%d: mwrite - fstat64 %s failed %d\n",
> > +			       procid, opno, f.path, errno);
> > +		free_pathname(&f);
> > +		close(fd);
> > +		return;
> > +	}
> > +	inode_info(st, sizeof(st), &stb, v);
> > +	lr = ((__int64_t)random() << 32) + random();
> > +	off = (off64_t)(lr % MIN(stb.st_size + (1024 * 1024), MAXFSIZE));
> > +	off %= maxfsize;
> > +	off &= (off64_t)(~(sysconf(_SC_PAGE_SIZE) - 1));
> > +	len = (size_t)(random() % MIN(maxfsize - off, FILELEN_MAX)) + 1;
> > +
> > +	/*
> > +	 * truncate file to the size we need to map and access,
> > +	 * keep away SIGBUS / SIGSEGV killing this process
> > +	 */
> > +	e = truncate64_path(&f, off + len) < 0 ? errno : 0;
> 
> This implies a truncate(2) operation in mwrite. I don't think we need
> this. Just mwrite within file size, let other operations like truncate
> and write to extend file size.

OK.

> 
> > +	/* try private file mappings with 20% rate */
> > +	flags = (random() % 20) ? MAP_SHARED : MAP_PRIVATE;
> 
> Again, this is 5%.
> 
> > +	do {
> > +		addr = mmap(NULL, len, PROT_WRITE, flags, fd, off);
> > +		e = (addr == MAP_FAILED) ? errno : 0;
> > +		if (errno == ENOMEM && flags & MAP_PRIVATE) {
> > +		/* turn to shared mapping if memeory is not enough for private mapping */
> > +			flags = MAP_SHARED;
> > +		} else if (errno == ENOMEM && len > sysconf(_SC_PAGE_SIZE)) {
> > +		/* reduce mapping length, if memeory is not enough for shared mapping */
> > +			len /= 2;
> > +		}
> > +	} while (errno == ENOMEM && len > sysconf(_SC_PAGE_SIZE));
> 
> And again, no need to retry on mmap failure, IMO, just let it fail.

OK.

> 
> > +	if (e && v)
> > +		printf("%d/%d: mwrite - mmap failed %s%s [%lld,%d,%s] %d\n",
> > +		       procid, opno, f.path, st, (long long)off, (int)len,
> > +		       (flags & MAP_PRIVATE) ? "MAP_PRIVATE" : "MAP_SHARED", e);
> 
> Consider putting this flag translation into a "struct print_flags"? See
> struct print_flags falloc_flags [].

It looks make things more difficult. But OK, I'll try it.

Thanks,
Zorro

> 
> Thanks,
> Eryu
> 
> > +
> > +	if (addr != MAP_FAILED) {
> > +		memset(addr, nameseq & 0xff, len);
> > +		e = munmap(addr, len) < 0 ? errno : 0;
> > +		if (e && v)
> > +			printf("%d/%d: mwrite - munmap failed %s%s [%lld,%d] %d\n",
> > +			       procid, opno, f.path, st, (long long)off,
> > +			       (int)len, e);
> > +	}
> > +	if (v)
> > +		printf("%d/%d: mwrite %s%s [%lld,%d,%s] %d\n",
> > +		       procid, opno, f.path, st, (long long)off, (int)len,
> > +		       (flags & MAP_PRIVATE) ? "MAP_PRIVATE" : "MAP_SHARED", e);
> > +
> > +	free_pathname(&f);
> > +	close(fd);
> > +}
> > +
> > +void
> >  punch_f(int opno, long r)
> >  {
> >  #ifdef HAVE_LINUX_FALLOC_H
> > diff --git a/src/global.h b/src/global.h
> > index f63246b..51d1e94 100644
> > --- a/src/global.h
> > +++ b/src/global.h
> > @@ -178,4 +178,8 @@
> >  
> >  #endif /* HAVE_LINUX_FALLOC_H */
> >  
> > +#ifndef HAVE_SYS_MMAN_H
> > +#include <sys/mman.h>
> > +#endif
> > +
> >  #endif /* GLOBAL_H */
> > -- 
> > 2.7.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe fstests" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PQcxBKCX5YTpkKY057SbK10&r=-yMrTV4jriXR7ieyzzjV-QgHBD0UDw8ixoR77aMeAHE&m=lkITlTdZ5GLMs6hTeBruHcJo4KVTiiRB53y4qWFkduY&s=unrG1l-1WFHTIEW3Qltb9bmYSEvIo7ms-Xtvl7HQukA&e= 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PQcxBKCX5YTpkKY057SbK10&r=-yMrTV4jriXR7ieyzzjV-QgHBD0UDw8ixoR77aMeAHE&m=lkITlTdZ5GLMs6hTeBruHcJo4KVTiiRB53y4qWFkduY&s=unrG1l-1WFHTIEW3Qltb9bmYSEvIo7ms-Xtvl7HQukA&e= 
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PQcxBKCX5YTpkKY057SbK10&r=-yMrTV4jriXR7ieyzzjV-QgHBD0UDw8ixoR77aMeAHE&m=lkITlTdZ5GLMs6hTeBruHcJo4KVTiiRB53y4qWFkduY&s=unrG1l-1WFHTIEW3Qltb9bmYSEvIo7ms-Xtvl7HQukA&e= 

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2017-03-22 15:00 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-21  9:18 [PATCH v5 1/2] fsstress: add mwrite/mread into test operation list Zorro Lang
2017-03-21  9:18 ` [PATCH v5 2/2] xfs/068: skip new fsstress operations mwrite and mread Zorro Lang
2017-03-21 10:55   ` Amir Goldstein
2017-03-21 21:34     ` Dave Chinner
2017-03-22  1:42       ` Amir Goldstein
2017-03-22  6:58 ` [PATCH v5 1/2] fsstress: add mwrite/mread into test operation list Eryu Guan
2017-03-22 14:59   ` Zorro Lang

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.