[PATCH] xfs_io: report io error for pwrite -W and -w

[PATCH] xfs_io: report io error for pwrite -W and -w

[Liu Bo]

When IO error occurs, xfs_io -c "pwrite -W/w" doesn't report errors
while xfs_io -c "pwrite" -c "fsync" does.

This changes "pwrite -W/w" to report errors when it should.

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 io/pwrite.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/io/pwrite.c b/io/pwrite.c
index 1c5dfca..71bcccc 100644
--- a/io/pwrite.c
+++ b/io/pwrite.c
@@ -379,11 +379,18 @@ pwrite_f(
 	}
 	if (c < 0)
 		goto done;
-	if (Wflag)
-		fsync(file->fd);
-	if (wflag)
-		fdatasync(file->fd);
+	if (Wflag) {
+		if (fsync(file->fd) < 0) {
+			perror("fsync");
+			goto done;
+		}
+	}
+	if (wflag) {
+		if (fdatasync(file->fd) < 0) {
+			perror("fdatasync");
+			goto done;
+		}
+	}
 	if (qflag)
 		goto done;
 	gettimeofday(&t2, NULL);
-- 
2.9.4

@@ -390,0 +397,0 @@

Re: [PATCH] xfs_io: report io error for pwrite -W and -w

[Darrick J. Wong]

On Thu, Oct 12, 2017 at 02:49:49PM -0600, Liu Bo wrote:
> When IO error occurs, xfs_io -c "pwrite -W/w" doesn't report errors
> while xfs_io -c "pwrite" -c "fsync" does.
> 
> This changes "pwrite -W/w" to report errors when it should.
> 
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
>  io/pwrite.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/io/pwrite.c b/io/pwrite.c
> index 1c5dfca..71bcccc 100644
> --- a/io/pwrite.c
> +++ b/io/pwrite.c
> @@ -379,11 +379,18 @@ pwrite_f(
>  	}
>  	if (c < 0)
>  		goto done;
> -	if (Wflag)
> -		fsync(file->fd);
> -	if (wflag)
> -		fdatasync(file->fd);
> +	if (Wflag) {
> +		if (fsync(file->fd) < 0) {
> +			perror("fsync");
> +			goto done;
> +		}
> +	}
> +	if (wflag) {
> +		if (fdatasync(file->fd) < 0) {
> +			perror("fdatasync");

Ok.

> +			goto done;

So this is a new behavior -- previously we'd print the timing info even
if the f{data,}sync call fails.  I don't know that the timing matters if
we fail to guarantee the data is on stable storage, but does anyone else
have opinions?

--D

> +		}
> +	}
>  	if (qflag)
>  		goto done;
>  	gettimeofday(&t2, NULL);
> -- 
> 2.9.4
> 
> @@ -390,0 +397,0 @@
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfs_io: report io error for pwrite -W and -w

[Brian Foster]

On Thu, Oct 12, 2017 at 03:02:45PM -0700, Darrick J. Wong wrote:
> On Thu, Oct 12, 2017 at 02:49:49PM -0600, Liu Bo wrote:
> > When IO error occurs, xfs_io -c "pwrite -W/w" doesn't report errors
> > while xfs_io -c "pwrite" -c "fsync" does.
> > 
> > This changes "pwrite -W/w" to report errors when it should.
> > 
> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > ---
> >  io/pwrite.c | 16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> > 
> > diff --git a/io/pwrite.c b/io/pwrite.c
> > index 1c5dfca..71bcccc 100644
> > --- a/io/pwrite.c
> > +++ b/io/pwrite.c
> > @@ -379,11 +379,18 @@ pwrite_f(
> >  	}
> >  	if (c < 0)
> >  		goto done;
> > -	if (Wflag)
> > -		fsync(file->fd);
> > -	if (wflag)
> > -		fdatasync(file->fd);
> > +	if (Wflag) {
> > +		if (fsync(file->fd) < 0) {
> > +			perror("fsync");
> > +			goto done;
> > +		}
> > +	}
> > +	if (wflag) {
> > +		if (fdatasync(file->fd) < 0) {
> > +			perror("fdatasync");
> 
> Ok.
> 
> > +			goto done;
> 
> So this is a new behavior -- previously we'd print the timing info even
> if the f{data,}sync call fails.  I don't know that the timing matters if
> we fail to guarantee the data is on stable storage, but does anyone else
> have opinions?
> 

Seems like an Ok change to me. The manpage says that the f[data]sync()
call is included in the timing results, which implies that you'd
probably want to know if it outright failed. If you didn't want to time
the sync, then I'd think you wouldn't use -w.

Brian

> --D
> 
> > +		}
> > +	}
> >  	if (qflag)
> >  		goto done;
> >  	gettimeofday(&t2, NULL);
> > -- 
> > 2.9.4
> > 
> > @@ -390,0 +397,0 @@
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfs_io: report io error for pwrite -W and -w

[Eric Sandeen]

On 10/12/17 3:49 PM, Liu Bo wrote:
> When IO error occurs, xfs_io -c "pwrite -W/w" doesn't report errors
> while xfs_io -c "pwrite" -c "fsync" does.
> 
> This changes "pwrite -W/w" to report errors when it should.
> 
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>

<after looking at djwong & bfoster's comments>

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  io/pwrite.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/io/pwrite.c b/io/pwrite.c
> index 1c5dfca..71bcccc 100644
> --- a/io/pwrite.c
> +++ b/io/pwrite.c
> @@ -379,11 +379,18 @@ pwrite_f(
>  	}
>  	if (c < 0)
>  		goto done;
> -	if (Wflag)
> -		fsync(file->fd);
> -	if (wflag)
> -		fdatasync(file->fd);
> +	if (Wflag) {
> +		if (fsync(file->fd) < 0) {
> +			perror("fsync");
> +			goto done;
> +		}
> +	}
> +	if (wflag) {
> +		if (fdatasync(file->fd) < 0) {
> +			perror("fdatasync");
> +			goto done;
> +		}
> +	}
>  	if (qflag)
>  		goto done;
>  	gettimeofday(&t2, NULL);
>