Re: [PATCH v5 1/2] fsstress: add mwrite/mread into test operation list

[Zorro Lang <zlang@redhat.com>]

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=