[1/2] fsstress: add splice support
diff mbox series

Message ID 20181225085632.9875-1-zlang@redhat.com
State New
Headers show
Series
  • [1/2] fsstress: add splice support
Related show

Commit Message

Zorro Lang Dec. 25, 2018, 8:56 a.m. UTC
Support the splice syscall in fsstress.

Signed-off-by: Zorro Lang <zlang@redhat.com>
---
 ltp/fsstress.c | 144 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 144 insertions(+)

Comments

Eryu Guan Dec. 29, 2018, 7:48 a.m. UTC | #1
On Tue, Dec 25, 2018 at 04:56:31PM +0800, Zorro Lang wrote:
> Support the splice syscall in fsstress.
> 
> Signed-off-by: Zorro Lang <zlang@redhat.com>

Thanks for adding the new syscall! Some comments inline.

> ---
>  ltp/fsstress.c | 144 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 144 insertions(+)
> 
> diff --git a/ltp/fsstress.c b/ltp/fsstress.c
> index 99a1d733..cdf73026 100644
> --- a/ltp/fsstress.c
> +++ b/ltp/fsstress.c
> @@ -85,6 +85,7 @@ typedef enum {
>  	OP_RMDIR,
>  	OP_SETATTR,
>  	OP_SETXATTR,
> +	OP_SPLICE,
>  	OP_STAT,
>  	OP_SYMLINK,
>  	OP_SYNC,
> @@ -194,6 +195,7 @@ void	resvsp_f(int, long);
>  void	rmdir_f(int, long);
>  void	setattr_f(int, long);
>  void	setxattr_f(int, long);
> +void	splice_f(int, long);
>  void	stat_f(int, long);
>  void	symlink_f(int, long);
>  void	sync_f(int, long);
> @@ -244,6 +246,7 @@ opdesc_t	ops[] = {
>  	{ OP_RMDIR, "rmdir", rmdir_f, 1, 1 },
>  	{ OP_SETATTR, "setattr", setattr_f, 0, 1 },
>  	{ OP_SETXATTR, "setxattr", setxattr_f, 1, 1 },
> +	{ OP_SPLICE, "splice", splice_f, 1, 1 },
>  	{ OP_STAT, "stat", stat_f, 1, 0 },
>  	{ OP_SYMLINK, "symlink", symlink_f, 2, 1 },
>  	{ OP_SYNC, "sync", sync_f, 1, 1 },
> @@ -2764,6 +2767,147 @@ setxattr_f(int opno, long r)
>  #endif
>  }
>  
> +void
> +splice_f(int opno, long r)
> +{
> +	struct pathname		fpath1;
> +	struct pathname		fpath2;
> +	struct stat64		stat1;
> +	struct stat64		stat2;
> +	char			inoinfo1[1024];
> +	char			inoinfo2[1024];
> +	loff_t			lr;
> +	loff_t			off1;
> +	loff_t			off2;
> +	size_t			len;
> +	size_t			total;
> +	int			v1;
> +	int			v2;
> +	int			fd1;
> +	int			fd2;
> +	size_t			ret;
> +	int			e;
> +	int			filedes[2];
> +
> +	/* Load paths */
> +	init_pathname(&fpath1);
> +	if (!get_fname(FT_REGm, r, &fpath1, NULL, NULL, &v1)) {
> +		if (v1)
> +			printf("%d/%d: splice read - no filename\n",
> +				procid, opno);
> +		goto out_fpath1;
> +	}
> +
> +	init_pathname(&fpath2);
> +	if (!get_fname(FT_REGm, random(), &fpath2, NULL, NULL, &v2)) {
> +		if (v2)
> +			printf("%d/%d: splice write - no filename\n",
> +				procid, opno);
> +		goto out_fpath2;
> +	}
> +
> +	/* Open files */
> +	fd1 = open_path(&fpath1, O_RDONLY);
> +	e = fd1 < 0 ? errno : 0;
> +	check_cwd();
> +	if (fd1 < 0) {
> +		if (v1)
> +			printf("%d/%d: splice read - open %s failed %d\n",
> +				procid, opno, fpath1.path, e);
> +		goto out_fpath2;
> +	}
> +
> +	fd2 = open_path(&fpath2, O_WRONLY);
> +	e = fd2 < 0 ? errno : 0;
> +	check_cwd();
> +	if (fd2 < 0) {
> +		if (v2)
> +			printf("%d/%d: splice write - open %s failed %d\n",
> +				procid, opno, fpath2.path, e);
> +		goto out_fd1;
> +	}
> +
> +	/* Get file stats */
> +	if (fstat64(fd1, &stat1) < 0) {
> +		if (v1)
> +			printf("%d/%d: splice read - fstat64 %s failed %d\n",
> +				procid, opno, fpath1.path, errno);
> +		goto out_fd2;
> +	}
> +	inode_info(inoinfo1, sizeof(inoinfo1), &stat1, v1);
> +
> +	if (fstat64(fd2, &stat2) < 0) {
> +		if (v2)
> +			printf("%d/%d: splice write - fstat64 %s failed %d\n",
> +				procid, opno, fpath2.path, errno);
> +		goto out_fd2;
> +	}
> +	inode_info(inoinfo2, sizeof(inoinfo2), &stat2, v2);
> +
> +	/* Calculate offsets */
> +	len = (random() % FILELEN_MAX) + 1;
> +	if (len == 0)
> +		len = stat1.st_blksize;
> +	if (len > stat1.st_size)
> +		len = stat1.st_size;
> +
> +	lr = ((int64_t)random() << 32) + random();
> +	if (stat1.st_size == len)
> +		off1 = 0;
> +	else
> +		off1 = (off64_t)(lr % MIN(stat1.st_size - len, MAXFSIZE));
> +	off1 %= maxfsize;
> +
> +	if (pipe(filedes) < 0) {
> +		if (v1 || v2) {
> +			printf("%d/%d: splice - pipe failed %d\n",
> +			       procid, opno, errno);
> +			goto out_fd2;
> +		}
> +	}
> +
> +	while (len > 0) {
> +		/* move to pipe buffer */
> +		ret = splice(fd1, &off1, filedes[1], NULL, len, 0);

ret is defined as size_t, but splice(2) returns ssize_t, and the return
value is casted to size_t and can't be negative.

> +		if (ret < 0) {
> +			break;
> +		}

So this will never be true.

> +		/* move from pipe buffer to dst file */
> +		ret = splice(filedes[0], NULL, fd2, &off2, len, 0);

We should splice 'ret' bytes as the length not 'len' here, otherwise we
may request to splice more data to fd2 than the pipe actually holds and
block on reading the pipe. I can hit fsstress 'hang' occasionally.

Also, off2 is used without being initialized.

> +		if (ret < 0) {
> +			break;
> +		}
> +                len -= ret;
> +                total += ret;
> +	}
> +
> +	e = ret < 0 ? errno : 0;
> +	if (v1 || v2) {
> +		printf("%d/%d: splice %s%s [%lld,%lld] -> %s%s [%lld,%lld]",
> +			procid, opno,
> +			fpath1.path, inoinfo1, (long long)off1, (long long)len,
> +			fpath2.path, inoinfo2, (long long)off2, (long long)len);

The values of off1 and off2 have been updated by splice(2) calls, and
are not the initial value we requested. And 'len' is always '0' here as
it's been adjusted in above while loop as well.

> +
> +		if (ret < 0)
> +			printf(" error %d", e);

Always print out errno even if it's 0 (and 'error' can be omitted), as
other operations do.

> +		else if (len && ret > len)

Same here, 'len' is 0 and the if can never be true.

Thanks,
Eryu

> +			printf(" asked for %lld, copied %lld??\n",
> +				(long long)len, (long long)total);
> +		printf("\n");
> +	}
> +
> +	close(filedes[0]);
> +	close(filedes[1]);
> +out_fd2:
> +	close(fd2);
> +out_fd1:
> +	close(fd1);
> +out_fpath2:
> +	free_pathname(&fpath2);
> +out_fpath1:
> +	free_pathname(&fpath1);
> +}
> +
>  void
>  creat_f(int opno, long r)
>  {
> -- 
> 2.17.2
>
Zorro Lang Dec. 29, 2018, 8:03 a.m. UTC | #2
On Sat, Dec 29, 2018 at 03:48:07PM +0800, Eryu Guan wrote:
> On Tue, Dec 25, 2018 at 04:56:31PM +0800, Zorro Lang wrote:
> > Support the splice syscall in fsstress.
> > 
> > Signed-off-by: Zorro Lang <zlang@redhat.com>
> 
> Thanks for adding the new syscall! Some comments inline.
> 
> > ---
> >  ltp/fsstress.c | 144 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 144 insertions(+)
> > 
> > diff --git a/ltp/fsstress.c b/ltp/fsstress.c
> > index 99a1d733..cdf73026 100644
> > --- a/ltp/fsstress.c
> > +++ b/ltp/fsstress.c
> > @@ -85,6 +85,7 @@ typedef enum {
> >  	OP_RMDIR,
> >  	OP_SETATTR,
> >  	OP_SETXATTR,
> > +	OP_SPLICE,
> >  	OP_STAT,
> >  	OP_SYMLINK,
> >  	OP_SYNC,
> > @@ -194,6 +195,7 @@ void	resvsp_f(int, long);
> >  void	rmdir_f(int, long);
> >  void	setattr_f(int, long);
> >  void	setxattr_f(int, long);
> > +void	splice_f(int, long);
> >  void	stat_f(int, long);
> >  void	symlink_f(int, long);
> >  void	sync_f(int, long);
> > @@ -244,6 +246,7 @@ opdesc_t	ops[] = {
> >  	{ OP_RMDIR, "rmdir", rmdir_f, 1, 1 },
> >  	{ OP_SETATTR, "setattr", setattr_f, 0, 1 },
> >  	{ OP_SETXATTR, "setxattr", setxattr_f, 1, 1 },
> > +	{ OP_SPLICE, "splice", splice_f, 1, 1 },
> >  	{ OP_STAT, "stat", stat_f, 1, 0 },
> >  	{ OP_SYMLINK, "symlink", symlink_f, 2, 1 },
> >  	{ OP_SYNC, "sync", sync_f, 1, 1 },
> > @@ -2764,6 +2767,147 @@ setxattr_f(int opno, long r)
> >  #endif
> >  }
> >  
> > +void
> > +splice_f(int opno, long r)
> > +{
> > +	struct pathname		fpath1;
> > +	struct pathname		fpath2;
> > +	struct stat64		stat1;
> > +	struct stat64		stat2;
> > +	char			inoinfo1[1024];
> > +	char			inoinfo2[1024];
> > +	loff_t			lr;
> > +	loff_t			off1;
> > +	loff_t			off2;
> > +	size_t			len;
> > +	size_t			total;
> > +	int			v1;
> > +	int			v2;
> > +	int			fd1;
> > +	int			fd2;
> > +	size_t			ret;
> > +	int			e;
> > +	int			filedes[2];
> > +
> > +	/* Load paths */
> > +	init_pathname(&fpath1);
> > +	if (!get_fname(FT_REGm, r, &fpath1, NULL, NULL, &v1)) {
> > +		if (v1)
> > +			printf("%d/%d: splice read - no filename\n",
> > +				procid, opno);
> > +		goto out_fpath1;
> > +	}
> > +
> > +	init_pathname(&fpath2);
> > +	if (!get_fname(FT_REGm, random(), &fpath2, NULL, NULL, &v2)) {
> > +		if (v2)
> > +			printf("%d/%d: splice write - no filename\n",
> > +				procid, opno);
> > +		goto out_fpath2;
> > +	}
> > +
> > +	/* Open files */
> > +	fd1 = open_path(&fpath1, O_RDONLY);
> > +	e = fd1 < 0 ? errno : 0;
> > +	check_cwd();
> > +	if (fd1 < 0) {
> > +		if (v1)
> > +			printf("%d/%d: splice read - open %s failed %d\n",
> > +				procid, opno, fpath1.path, e);
> > +		goto out_fpath2;
> > +	}
> > +
> > +	fd2 = open_path(&fpath2, O_WRONLY);
> > +	e = fd2 < 0 ? errno : 0;
> > +	check_cwd();
> > +	if (fd2 < 0) {
> > +		if (v2)
> > +			printf("%d/%d: splice write - open %s failed %d\n",
> > +				procid, opno, fpath2.path, e);
> > +		goto out_fd1;
> > +	}
> > +
> > +	/* Get file stats */
> > +	if (fstat64(fd1, &stat1) < 0) {
> > +		if (v1)
> > +			printf("%d/%d: splice read - fstat64 %s failed %d\n",
> > +				procid, opno, fpath1.path, errno);
> > +		goto out_fd2;
> > +	}
> > +	inode_info(inoinfo1, sizeof(inoinfo1), &stat1, v1);
> > +
> > +	if (fstat64(fd2, &stat2) < 0) {
> > +		if (v2)
> > +			printf("%d/%d: splice write - fstat64 %s failed %d\n",
> > +				procid, opno, fpath2.path, errno);
> > +		goto out_fd2;
> > +	}
> > +	inode_info(inoinfo2, sizeof(inoinfo2), &stat2, v2);
> > +
> > +	/* Calculate offsets */
> > +	len = (random() % FILELEN_MAX) + 1;
> > +	if (len == 0)
> > +		len = stat1.st_blksize;
> > +	if (len > stat1.st_size)
> > +		len = stat1.st_size;
> > +
> > +	lr = ((int64_t)random() << 32) + random();
> > +	if (stat1.st_size == len)
> > +		off1 = 0;
> > +	else
> > +		off1 = (off64_t)(lr % MIN(stat1.st_size - len, MAXFSIZE));
> > +	off1 %= maxfsize;
> > +
> > +	if (pipe(filedes) < 0) {
> > +		if (v1 || v2) {
> > +			printf("%d/%d: splice - pipe failed %d\n",
> > +			       procid, opno, errno);
> > +			goto out_fd2;
> > +		}
> > +	}
> > +
> > +	while (len > 0) {
> > +		/* move to pipe buffer */
> > +		ret = splice(fd1, &off1, filedes[1], NULL, len, 0);
> 
> ret is defined as size_t, but splice(2) returns ssize_t, and the return
> value is casted to size_t and can't be negative.
> 
> > +		if (ret < 0) {
> > +			break;
> > +		}
> 
> So this will never be true.
> 
> > +		/* move from pipe buffer to dst file */
> > +		ret = splice(filedes[0], NULL, fd2, &off2, len, 0);
> 
> We should splice 'ret' bytes as the length not 'len' here, otherwise we
> may request to splice more data to fd2 than the pipe actually holds and
> block on reading the pipe. I can hit fsstress 'hang' occasionally.
> 
> Also, off2 is used without being initialized.
> 
> > +		if (ret < 0) {
> > +			break;
> > +		}
> > +                len -= ret;
> > +                total += ret;
> > +	}
> > +
> > +	e = ret < 0 ? errno : 0;
> > +	if (v1 || v2) {
> > +		printf("%d/%d: splice %s%s [%lld,%lld] -> %s%s [%lld,%lld]",
> > +			procid, opno,
> > +			fpath1.path, inoinfo1, (long long)off1, (long long)len,
> > +			fpath2.path, inoinfo2, (long long)off2, (long long)len);
> 
> The values of off1 and off2 have been updated by splice(2) calls, and
> are not the initial value we requested. And 'len' is always '0' here as
> it's been adjusted in above while loop as well.
> 
> > +
> > +		if (ret < 0)
> > +			printf(" error %d", e);
> 
> Always print out errno even if it's 0 (and 'error' can be omitted), as
> other operations do.
> 
> > +		else if (len && ret > len)
> 
> Same here, 'len' is 0 and the if can never be true.

Thanks for your reviewing, looks like I was too hurry to send this patch out:)
I should do more self-check. I'm going to fix these problems and send V2 later.

Thanks,
Zorro

> 
> Thanks,
> Eryu
> 
> > +			printf(" asked for %lld, copied %lld??\n",
> > +				(long long)len, (long long)total);
> > +		printf("\n");
> > +	}
> > +
> > +	close(filedes[0]);
> > +	close(filedes[1]);
> > +out_fd2:
> > +	close(fd2);
> > +out_fd1:
> > +	close(fd1);
> > +out_fpath2:
> > +	free_pathname(&fpath2);
> > +out_fpath1:
> > +	free_pathname(&fpath1);
> > +}
> > +
> >  void
> >  creat_f(int opno, long r)
> >  {
> > -- 
> > 2.17.2
> >

Patch
diff mbox series

diff --git a/ltp/fsstress.c b/ltp/fsstress.c
index 99a1d733..cdf73026 100644
--- a/ltp/fsstress.c
+++ b/ltp/fsstress.c
@@ -85,6 +85,7 @@  typedef enum {
 	OP_RMDIR,
 	OP_SETATTR,
 	OP_SETXATTR,
+	OP_SPLICE,
 	OP_STAT,
 	OP_SYMLINK,
 	OP_SYNC,
@@ -194,6 +195,7 @@  void	resvsp_f(int, long);
 void	rmdir_f(int, long);
 void	setattr_f(int, long);
 void	setxattr_f(int, long);
+void	splice_f(int, long);
 void	stat_f(int, long);
 void	symlink_f(int, long);
 void	sync_f(int, long);
@@ -244,6 +246,7 @@  opdesc_t	ops[] = {
 	{ OP_RMDIR, "rmdir", rmdir_f, 1, 1 },
 	{ OP_SETATTR, "setattr", setattr_f, 0, 1 },
 	{ OP_SETXATTR, "setxattr", setxattr_f, 1, 1 },
+	{ OP_SPLICE, "splice", splice_f, 1, 1 },
 	{ OP_STAT, "stat", stat_f, 1, 0 },
 	{ OP_SYMLINK, "symlink", symlink_f, 2, 1 },
 	{ OP_SYNC, "sync", sync_f, 1, 1 },
@@ -2764,6 +2767,147 @@  setxattr_f(int opno, long r)
 #endif
 }
 
+void
+splice_f(int opno, long r)
+{
+	struct pathname		fpath1;
+	struct pathname		fpath2;
+	struct stat64		stat1;
+	struct stat64		stat2;
+	char			inoinfo1[1024];
+	char			inoinfo2[1024];
+	loff_t			lr;
+	loff_t			off1;
+	loff_t			off2;
+	size_t			len;
+	size_t			total;
+	int			v1;
+	int			v2;
+	int			fd1;
+	int			fd2;
+	size_t			ret;
+	int			e;
+	int			filedes[2];
+
+	/* Load paths */
+	init_pathname(&fpath1);
+	if (!get_fname(FT_REGm, r, &fpath1, NULL, NULL, &v1)) {
+		if (v1)
+			printf("%d/%d: splice read - no filename\n",
+				procid, opno);
+		goto out_fpath1;
+	}
+
+	init_pathname(&fpath2);
+	if (!get_fname(FT_REGm, random(), &fpath2, NULL, NULL, &v2)) {
+		if (v2)
+			printf("%d/%d: splice write - no filename\n",
+				procid, opno);
+		goto out_fpath2;
+	}
+
+	/* Open files */
+	fd1 = open_path(&fpath1, O_RDONLY);
+	e = fd1 < 0 ? errno : 0;
+	check_cwd();
+	if (fd1 < 0) {
+		if (v1)
+			printf("%d/%d: splice read - open %s failed %d\n",
+				procid, opno, fpath1.path, e);
+		goto out_fpath2;
+	}
+
+	fd2 = open_path(&fpath2, O_WRONLY);
+	e = fd2 < 0 ? errno : 0;
+	check_cwd();
+	if (fd2 < 0) {
+		if (v2)
+			printf("%d/%d: splice write - open %s failed %d\n",
+				procid, opno, fpath2.path, e);
+		goto out_fd1;
+	}
+
+	/* Get file stats */
+	if (fstat64(fd1, &stat1) < 0) {
+		if (v1)
+			printf("%d/%d: splice read - fstat64 %s failed %d\n",
+				procid, opno, fpath1.path, errno);
+		goto out_fd2;
+	}
+	inode_info(inoinfo1, sizeof(inoinfo1), &stat1, v1);
+
+	if (fstat64(fd2, &stat2) < 0) {
+		if (v2)
+			printf("%d/%d: splice write - fstat64 %s failed %d\n",
+				procid, opno, fpath2.path, errno);
+		goto out_fd2;
+	}
+	inode_info(inoinfo2, sizeof(inoinfo2), &stat2, v2);
+
+	/* Calculate offsets */
+	len = (random() % FILELEN_MAX) + 1;
+	if (len == 0)
+		len = stat1.st_blksize;
+	if (len > stat1.st_size)
+		len = stat1.st_size;
+
+	lr = ((int64_t)random() << 32) + random();
+	if (stat1.st_size == len)
+		off1 = 0;
+	else
+		off1 = (off64_t)(lr % MIN(stat1.st_size - len, MAXFSIZE));
+	off1 %= maxfsize;
+
+	if (pipe(filedes) < 0) {
+		if (v1 || v2) {
+			printf("%d/%d: splice - pipe failed %d\n",
+			       procid, opno, errno);
+			goto out_fd2;
+		}
+	}
+
+	while (len > 0) {
+		/* move to pipe buffer */
+		ret = splice(fd1, &off1, filedes[1], NULL, len, 0);
+		if (ret < 0) {
+			break;
+		}
+		/* move from pipe buffer to dst file */
+		ret = splice(filedes[0], NULL, fd2, &off2, len, 0);
+		if (ret < 0) {
+			break;
+		}
+                len -= ret;
+                total += ret;
+	}
+
+	e = ret < 0 ? errno : 0;
+	if (v1 || v2) {
+		printf("%d/%d: splice %s%s [%lld,%lld] -> %s%s [%lld,%lld]",
+			procid, opno,
+			fpath1.path, inoinfo1, (long long)off1, (long long)len,
+			fpath2.path, inoinfo2, (long long)off2, (long long)len);
+
+		if (ret < 0)
+			printf(" error %d", e);
+		else if (len && ret > len)
+			printf(" asked for %lld, copied %lld??\n",
+				(long long)len, (long long)total);
+		printf("\n");
+	}
+
+	close(filedes[0]);
+	close(filedes[1]);
+out_fd2:
+	close(fd2);
+out_fd1:
+	close(fd1);
+out_fpath2:
+	free_pathname(&fpath2);
+out_fpath1:
+	free_pathname(&fpath1);
+}
+
 void
 creat_f(int opno, long r)
 {