[PATCH] xfstests: fix async io error handling in fsx

[PATCH] xfstests: fix async io error handling in fsx

[Felix Blyakher]

The result of async io returned in the event.res in addition to
the number of bytes read/written provides negated error
number. The broken libaio defines event.res as unsigned
while the same structure in ithe kernel defines it as signed.
The kernel indeed treat it as signed, and returns the
negated error number. Till libaio is fixed we provide
the signed long temp var.
Also set errno for each error condition in aio_rw, as the
caller is not aio aware and expects ret(-1)+errno by the
traditional libc convention.

Signed-off-by: Felix Blyakher <felixb@sgi.com>
---
 ltp/fsx.c |   34 ++++++++++++++++++++++++++++------
 1 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/ltp/fsx.c b/ltp/fsx.c
index ebe5324..81402be 100644
--- a/ltp/fsx.c
+++ b/ltp/fsx.c
@@ -976,19 +976,41 @@ __aio_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
 		fprintf(stderr, "errcode=%d\n", ret);
 		fprintf(stderr, "aio_rw: io_submit failed: %s\n",
 				strerror(ret));
-		return(-1);
+		errno = -ret;
+		return -1;
 	}
 
 	ret = io_getevents(io_ctx, 1, 1, &event, &ts);
 	if (ret != 1) {
-		fprintf(stderr, "errcode=%d\n", ret);
-		fprintf(stderr, "aio_rw: io_getevents failed: %s\n",
-				 strerror(ret));
+		if (ret == 0)
+			fprintf(stderr, "aio_rw: no events available\n");
+		else {
+			fprintf(stderr, "errcode=%d\n", -ret);
+			fprintf(stderr, "aio_rw: io_getevents failed: %s\n",
+				 	strerror(-ret));
+		}
+		errno = -ret;
 		return -1;
 	}
 	if (len != event.res) {
-		fprintf(stderr, "bad read length: %lu instead of %u\n",
-				event.res, len);
+		/*
+		 * The b0rked libaio defines event.res as signed.
+		 * However the kernel strucuture has it unsigned,
+		 * and it's used to pass negated error value.
+		 * Till the library is fixed use the temp var.
+		 */
+		long res = (long)event.res;
+		if (res >= 0)
+			fprintf(stderr, "bad io length: %lu instead of %u\n",
+					res, len);
+		else {
+			fprintf(stderr, "errcode=%d\n", -res);
+			fprintf(stderr, "aio_rw: async io failed: %s\n",
+					strerror(-res));
+			errno = -res;
+			return -1;
+		}
+
 	}
 	return event.res;
 }
-- 
1.5.4.rc3

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfstests: fix async io error handling in fsx

[Christoph Hellwig]

On Mon, Mar 30, 2009 at 12:30:50AM -0500, Felix Blyakher wrote:
> --- a/ltp/fsx.c
> +++ b/ltp/fsx.c
> @@ -976,19 +976,41 @@ __aio_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
>  		fprintf(stderr, "errcode=%d\n", ret);
>  		fprintf(stderr, "aio_rw: io_submit failed: %s\n",
>  				strerror(ret));
> -		return(-1);
> +		errno = -ret;
> +		return -1;

I think we'd be better off having all these places that set errno in one
place and use goto labels.  That way you can also add a small comment
about it.

> +		/*
> +		 * The b0rked libaio defines event.res as signed.
> +		 * However the kernel strucuture has it unsigned,
> +		 * and it's used to pass negated error value.
> +		 * Till the library is fixed use the temp var.
> +		 */

This comment seems backwards to the patch description and the actual
code.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfstests: fix async io error handling in fsx

[Felix Blyakher]

On Mar 30, 2009, at 12:54 PM, Christoph Hellwig wrote:

> On Mon, Mar 30, 2009 at 12:30:50AM -0500, Felix Blyakher wrote:
>> --- a/ltp/fsx.c
>> +++ b/ltp/fsx.c
>> @@ -976,19 +976,41 @@ __aio_rw(int rw, int fd, char *buf, unsigned  
>> len, unsigned offset)
>> 		fprintf(stderr, "errcode=%d\n", ret);
>> 		fprintf(stderr, "aio_rw: io_submit failed: %s\n",
>> 				strerror(ret));
>> -		return(-1);
>> +		errno = -ret;
>> +		return -1;
>
> I think we'd be better off having all these places that set errno in  
> one
> place and use goto labels.  That way you can also add a small comment
> about it.

Makes sense, will do.

>
>
>> +		/*
>> +		 * The b0rked libaio defines event.res as signed.
>> +		 * However the kernel strucuture has it unsigned,
>> +		 * and it's used to pass negated error value.
>> +		 * Till the library is fixed use the temp var.
>> +		 */
>
> This comment seems backwards to the patch description and the actual
> code.

Hmm, not really.
If the libaio library would be correct (do you know whom to talk
to and where to send a patch), the code would look like this:

                 if (event.res >= 0)
                         fprintf(stderr, "bad io length: %lu instead  
of %u\n",
                                         event.res, len);
                 else {
                         fprintf(stderr, "errcode=%d\n", -event.res);
                         fprintf(stderr, "aio_rw: async io failed: %s 
\n",
                                         strerror(-event.res));
                         errno = -event.res;
                         return -1;
                 }


and no need for the temp var of right type, as in a patch now.
The comments just explains the need for that "long res" var.
It does not really describe the need to check the error value,
that's just the part of interface, I assume.
Though, I think, it would be clearer if I state the need to
check the error first, and then note why it could not be
done as above.

I'll repost.

Thanks,
Felix


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfstests: fix async io error handling in fsx

[Christoph Hellwig]

On Mon, Mar 30, 2009 at 01:53:21PM -0500, Felix Blyakher wrote:
>>> +		/*
>>> +		 * The b0rked libaio defines event.res as signed.
>>> +		 * However the kernel strucuture has it unsigned,
>>> +		 * and it's used to pass negated error value.
>>> +		 * Till the library is fixed use the temp var.
>>> +		 */
>>
>> This comment seems backwards to the patch description and the actual
>> code.
>
> Hmm, not really.
> If the libaio library would be correct (do you know whom to talk
> to and where to send a patch), the code would look like this:

Yeah, but shouldn't the comment read:

	/*
	 * The b0rked libaio defines event.res as *unsigned*.
	 * However the kernel strucuture has it *signed*,
	 * and it's used to pass negated error value.
	 * Till the library is fixed use the temp var.
	 */

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfstests: fix async io error handling in fsx

[Felix Blyakher]

On Mar 30, 2009, at 1:56 PM, Christoph Hellwig wrote:

> On Mon, Mar 30, 2009 at 01:53:21PM -0500, Felix Blyakher wrote:
>>>> +		/*
>>>> +		 * The b0rked libaio defines event.res as signed.
>>>> +		 * However the kernel strucuture has it unsigned,
>>>> +		 * and it's used to pass negated error value.
>>>> +		 * Till the library is fixed use the temp var.
>>>> +		 */
>>>
>>> This comment seems backwards to the patch description and the actual
>>> code.
>>
>> Hmm, not really.
>> If the libaio library would be correct (do you know whom to talk
>> to and where to send a patch), the code would look like this:
>
> Yeah, but shouldn't the comment read:
>
> 	/*
> 	 * The b0rked libaio defines event.res as *unsigned*.
> 	 * However the kernel strucuture has it *signed*,

Ohh, I was blind twice.
Sure, you're right. Thanks for pointing it out.

Felix

>
> 	 * and it's used to pass negated error value.
> 	 * Till the library is fixed use the temp var.
> 	 */

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH] xfstests: fix async io error handling in fsx

[Felix Blyakher]

The result of async io returned in the event.res in addition
to the number of bytes read/written provides negated error
number. The broken libaio defines event.res as unsigned
while the same structure in the kernel defines it as signed.
The kernel indeed treat it as signed, and returns the
negated error number. Till libaio is fixed we provide
the signed long temp var.
Also set errno for each error condition in aio_rw, as the
caller is not aio aware and expects ret(-1)+errno by the
traditional libc convention.

Signed-off-by: Felix Blyakher <felixb@sgi.com>
---
 ltp/fsx.c |   42 +++++++++++++++++++++++++++++++++++-------
 1 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/ltp/fsx.c b/ltp/fsx.c
index ebe5324..739d56c 100644
--- a/ltp/fsx.c
+++ b/ltp/fsx.c
@@ -962,6 +962,7 @@ __aio_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
 	static struct timespec ts;
 	struct iocb *iocbs[] = { &iocb };
 	int ret;
+	long res;
 
 	if (rw == READ) {
 		io_prep_pread(&iocb, fd, buf, len, offset);
@@ -976,21 +977,48 @@ __aio_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
 		fprintf(stderr, "errcode=%d\n", ret);
 		fprintf(stderr, "aio_rw: io_submit failed: %s\n",
 				strerror(ret));
-		return(-1);
+		goto out_error;
 	}
 
 	ret = io_getevents(io_ctx, 1, 1, &event, &ts);
 	if (ret != 1) {
-		fprintf(stderr, "errcode=%d\n", ret);
-		fprintf(stderr, "aio_rw: io_getevents failed: %s\n",
-				 strerror(ret));
-		return -1;
+		if (ret == 0)
+			fprintf(stderr, "aio_rw: no events available\n");
+		else {
+			fprintf(stderr, "errcode=%d\n", -ret);
+			fprintf(stderr, "aio_rw: io_getevents failed: %s\n",
+				 	strerror(-ret));
+		}
+		goto out_error;
 	}
 	if (len != event.res) {
-		fprintf(stderr, "bad read length: %lu instead of %u\n",
-				event.res, len);
+		/*
+		 * The b0rked libaio defines event.res as unsigned.
+		 * However the kernel strucuture has it signed,
+		 * and it's used to pass negated error value.
+		 * Till the library is fixed use the temp var.
+		 */
+		res = (long)event.res;
+		if (res >= 0)
+			fprintf(stderr, "bad io length: %lu instead of %u\n",
+					res, len);
+		else {
+			fprintf(stderr, "errcode=%d\n", -res);
+			fprintf(stderr, "aio_rw: async io failed: %s\n",
+					strerror(-res));
+			goto out_error;
+		}
+
 	}
 	return event.res;
+
+out_error:
+	/*
+	 * The caller expects error return in traditional libc
+	 * convention, i.e. -1 and the errno set to error.
+	 */
+	errno = ret <= 0 ? -ret : -res;
+	return -1;
 }
 
 int aio_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
-- 
1.5.4.rc3

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfstests: fix async io error handling in fsx

[Christoph Hellwig]

On Mon, Mar 30, 2009 at 02:41:41PM -0500, Felix Blyakher wrote:
> The result of async io returned in the event.res in addition
> to the number of bytes read/written provides negated error
> number. The broken libaio defines event.res as unsigned
> while the same structure in the kernel defines it as signed.
> The kernel indeed treat it as signed, and returns the
> negated error number. Till libaio is fixed we provide
> the signed long temp var.
> Also set errno for each error condition in aio_rw, as the
> caller is not aio aware and expects ret(-1)+errno by the
> traditional libc convention.
> 
> Signed-off-by: Felix Blyakher <felixb@sgi.com>
> ---
>  ltp/fsx.c |   42 +++++++++++++++++++++++++++++++++++-------
>  1 files changed, 35 insertions(+), 7 deletions(-)

>  	if (len != event.res) {
> -		fprintf(stderr, "bad read length: %lu instead of %u\n",
> -				event.res, len);
> +		/*
> +		 * The b0rked libaio defines event.res as unsigned.
> +		 * However the kernel strucuture has it signed,
> +		 * and it's used to pass negated error value.
> +		 * Till the library is fixed use the temp var.
> +		 */
> +		res = (long)event.res;
> +		if (res >= 0)
> +			fprintf(stderr, "bad io length: %lu instead of %u\n",
> +					res, len);
> +		else {
> +			fprintf(stderr, "errcode=%d\n", -res);
> +			fprintf(stderr, "aio_rw: async io failed: %s\n",
> +					strerror(-res));
> +			goto out_error;
> +		}
> +
>  	}
>  	return event.res;
> +
> +out_error:
> +	/*
> +	 * The caller expects error return in traditional libc
> +	 * convention, i.e. -1 and the errno set to error.
> +	 */
> +	errno = ret <= 0 ? -ret : -res;
> +	return -1;

I wonder why this doesn't give a compiler warning.  res is only
initialized in that last branch above.  Wouldn't it be better to set
ret to res inside that branch and only use ret down here?

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH] xfstests: fix async io error handling in fsx

[Felix Blyakher]

The result of async io returned in the event.res in addition
to the number of bytes read/written provides negated error
number. The broken libaio defines event.res as unsigned
while the same structure in the kernel defines it as signed.
The kernel indeed treats it as signed, and returns the
negated error number. Till libaio is fixed we provide
the signed long temp var.
Also set errno for each error condition in aio_rw, as the
caller is not aio aware and expects ret(-1)+errno by the
traditional libc convention.

Signed-off-by: Felix Blyakher <felixb@sgi.com>
---
 ltp/fsx.c |   43 ++++++++++++++++++++++++++++++++++++-------
 1 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/ltp/fsx.c b/ltp/fsx.c
index ebe5324..210afd5 100644
--- a/ltp/fsx.c
+++ b/ltp/fsx.c
@@ -962,6 +962,7 @@ __aio_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
 	static struct timespec ts;
 	struct iocb *iocbs[] = { &iocb };
 	int ret;
+	long res;
 
 	if (rw == READ) {
 		io_prep_pread(&iocb, fd, buf, len, offset);
@@ -976,21 +977,49 @@ __aio_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
 		fprintf(stderr, "errcode=%d\n", ret);
 		fprintf(stderr, "aio_rw: io_submit failed: %s\n",
 				strerror(ret));
-		return(-1);
+		goto out_error;
 	}
 
 	ret = io_getevents(io_ctx, 1, 1, &event, &ts);
 	if (ret != 1) {
-		fprintf(stderr, "errcode=%d\n", ret);
-		fprintf(stderr, "aio_rw: io_getevents failed: %s\n",
-				 strerror(ret));
-		return -1;
+		if (ret == 0)
+			fprintf(stderr, "aio_rw: no events available\n");
+		else {
+			fprintf(stderr, "errcode=%d\n", -ret);
+			fprintf(stderr, "aio_rw: io_getevents failed: %s\n",
+				 	strerror(-ret));
+		}
+		goto out_error;
 	}
 	if (len != event.res) {
-		fprintf(stderr, "bad read length: %lu instead of %u\n",
-				event.res, len);
+		/*
+		 * The b0rked libaio defines event.res as unsigned.
+		 * However the kernel strucuture has it signed,
+		 * and it's used to pass negated error value.
+		 * Till the library is fixed use the temp var.
+		 */
+		res = (long)event.res;
+		if (res >= 0)
+			fprintf(stderr, "bad io length: %lu instead of %u\n",
+					res, len);
+		else {
+			fprintf(stderr, "errcode=%d\n", -res);
+			fprintf(stderr, "aio_rw: async io failed: %s\n",
+					strerror(-res));
+			ret = res;
+			goto out_error;
+		}
+
 	}
 	return event.res;
+
+out_error:
+	/*
+	 * The caller expects error return in traditional libc
+	 * convention, i.e. -1 and the errno set to error.
+	 */
+	errno = -ret;
+	return -1;
 }
 
 int aio_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
-- 
1.5.4.rc3

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfstests: fix async io error handling in fsx

[Christoph Hellwig]

On Mon, Apr 06, 2009 at 10:26:16AM -0500, Felix Blyakher wrote:
> The result of async io returned in the event.res in addition
> to the number of bytes read/written provides negated error
> number. The broken libaio defines event.res as unsigned
> while the same structure in the kernel defines it as signed.
> The kernel indeed treats it as signed, and returns the
> negated error number. Till libaio is fixed we provide
> the signed long temp var.
> Also set errno for each error condition in aio_rw, as the
> caller is not aio aware and expects ret(-1)+errno by the
> traditional libc convention.

Looks good.

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfstests: fix async io error handling in fsx

[Christoph Hellwig]

On Mon, Apr 06, 2009 at 12:33:31PM -0400, Christoph Hellwig wrote:
> On Mon, Apr 06, 2009 at 10:26:16AM -0500, Felix Blyakher wrote:
> > The result of async io returned in the event.res in addition
> > to the number of bytes read/written provides negated error
> > number. The broken libaio defines event.res as unsigned
> > while the same structure in the kernel defines it as signed.
> > The kernel indeed treats it as signed, and returns the
> > negated error number. Till libaio is fixed we provide
> > the signed long temp var.
> > Also set errno for each error condition in aio_rw, as the
> > caller is not aio aware and expects ret(-1)+errno by the
> > traditional libc convention.
> 
> Looks good.
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Do you plan to commit this?  Otherwise I'll just suck it into the
xfstests-dev tree.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs