[PATCH] io_uring: Add IO_U_F_DEVICE_ERROR to identify error types

[PATCH] io_uring: Add IO_U_F_DEVICE_ERROR to identify error types

[Minwoo Im]

__io_u_log_error expects a positive value of io_u->error parsing it with
strerror() expecting it to be an errno.  io_uring_cmd (cmd_type=nvme),
for example, has returned errno value as a positive value and
device-specific CQE status type and code as well.

Commit 78831c6b35c5 ("io_uring: Fix the flip to negative of CQE status")
has put the abs() to the cqe->res, and it caused confusions between the
actual CQE stauts and the system error value (errno).  Now we have
Commit 2a13699a89dc ("io_uring: Add .errdetails to parse CQ status"),
meaning that io_uring_cmd ioengines will parse the actual value of
io_u->error value as CQE status value, so we should know if the value is
for CQE status or errno.

This patch added a flag IO_U_F_DEVICE_ERROR to io_u to represent if
io_u->error has device-specific error value, otherwise it's errno.

Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
---
 engines/io_uring.c | 22 ++++++++++++++++------
 io_u.c             |  9 ++++++---
 io_u.h             |  1 +
 3 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/engines/io_uring.c b/engines/io_uring.c
index 96a042a88820..1ab9a1765e38 100644
--- a/engines/io_uring.c
+++ b/engines/io_uring.c
@@ -528,12 +528,9 @@ static struct io_u *fio_ioring_cmd_event(struct thread_data *td, int event)
 	cqe = &ld->cq_ring.cqes[index];
 	io_u = (struct io_u *) (uintptr_t) cqe->user_data;
 
-	if (cqe->res != 0) {
-		io_u->error = abs(cqe->res);
-		return io_u;
-	} else {
-		io_u->error = 0;
-	}
+	io_u->error = cqe->res;
+	if (io_u->error != 0)
+		goto ret;
 
 	if (o->cmd_type == FIO_URING_CMD_NVME) {
 		data = FILE_ENG_DATA(io_u->file);
@@ -544,6 +541,16 @@ static struct io_u *fio_ioring_cmd_event(struct thread_data *td, int event)
 		}
 	}
 
+ret:
+	/*
+	 * If IO_U_F_DEVICE_ERROR is not set, io_u->error will be parsed as an
+	 * errno, otherwise device-specific error value (status value in CQE).
+	 */
+	if ((int)io_u->error > 0)
+		io_u_set(td, io_u, IO_U_F_DEVICE_ERROR);
+	else
+		io_u_clear(td, io_u, IO_U_F_DEVICE_ERROR);
+	io_u->error = abs(io_u->error);
 	return io_u;
 }
 
@@ -557,6 +564,9 @@ static char *fio_ioring_cmd_errdetails(struct thread_data *td,
 #define MAXMSGCHUNK 128
 	char *msg, msgchunk[MAXMSGCHUNK];
 
+	if (!(io_u->flags & IO_U_F_DEVICE_ERROR))
+		return NULL;
+
 	msg = calloc(1, MAXERRDETAIL);
 	strcpy(msg, "io_uring_cmd: ");
 
diff --git a/io_u.c b/io_u.c
index c49cd4df0237..b699169d79b5 100644
--- a/io_u.c
+++ b/io_u.c
@@ -1956,7 +1956,8 @@ static void __io_u_log_error(struct thread_data *td, struct io_u *io_u)
 	log_err("fio: io_u error%s%s: %s: %s offset=%llu, buflen=%llu\n",
 		io_u->file ? " on file " : "",
 		io_u->file ? io_u->file->file_name : "",
-		strerror(io_u->error),
+		(io_u->flags & IO_U_F_DEVICE_ERROR) ?
+			"Device-specific error" : strerror(io_u->error),
 		io_ddir_name(io_u->ddir),
 		io_u->offset, io_u->xfer_buflen);
 
@@ -1965,8 +1966,10 @@ static void __io_u_log_error(struct thread_data *td, struct io_u *io_u)
 	if (td->io_ops->errdetails) {
 		char *err = td->io_ops->errdetails(td, io_u);
 
-		log_err("fio: %s\n", err);
-		free(err);
+		if (err) {
+			log_err("fio: %s\n", err);
+			free(err);
+		}
 	}
 
 	if (!td->error)
diff --git a/io_u.h b/io_u.h
index ab93d50f967e..20afad667ee1 100644
--- a/io_u.h
+++ b/io_u.h
@@ -22,6 +22,7 @@ enum {
 	IO_U_F_BARRIER		= 1 << 6,
 	IO_U_F_VER_LIST		= 1 << 7,
 	IO_U_F_PATTERN_DONE	= 1 << 8,
+	IO_U_F_DEVICE_ERROR	= 1 << 9,
 };
 
 /*
-- 
2.34.1

Re: [PATCH] io_uring: Add IO_U_F_DEVICE_ERROR to identify error types

[Jens Axboe]

On Thu, 26 Sep 2024 09:41:34 +0900, Minwoo Im wrote:
> __io_u_log_error expects a positive value of io_u->error parsing it with
> strerror() expecting it to be an errno.  io_uring_cmd (cmd_type=nvme),
> for example, has returned errno value as a positive value and
> device-specific CQE status type and code as well.
> 
> Commit 78831c6b35c5 ("io_uring: Fix the flip to negative of CQE status")
> has put the abs() to the cqe->res, and it caused confusions between the
> actual CQE stauts and the system error value (errno).  Now we have
> Commit 2a13699a89dc ("io_uring: Add .errdetails to parse CQ status"),
> meaning that io_uring_cmd ioengines will parse the actual value of
> io_u->error value as CQE status value, so we should know if the value is
> for CQE status or errno.
> 
> [...]

Applied, thanks!

[1/1] io_uring: Add IO_U_F_DEVICE_ERROR to identify error types
      commit: ebe67b667f25694ead4caa0198598318f891b345

Best regards,
-- 
Jens Axboe

Re: [PATCH] io_uring: Add IO_U_F_DEVICE_ERROR to identify error types

[Jens Axboe]

On 9/25/24 6:41 PM, Minwoo Im wrote:
> diff --git a/io_u.h b/io_u.h
> index ab93d50f967e..20afad667ee1 100644
> --- a/io_u.h
> +++ b/io_u.h
> @@ -22,6 +22,7 @@ enum {
>  	IO_U_F_BARRIER		= 1 << 6,
>  	IO_U_F_VER_LIST		= 1 << 7,
>  	IO_U_F_PATTERN_DONE	= 1 << 8,
> +	IO_U_F_DEVICE_ERROR	= 1 << 9,
>  };
>  
>  /*

The patches you sent should've been a series, how are they supposed to
both apply when you add an item here for each of them as if the other
one doesn't exist?

I'll fix it up, but for the future, if patches depend on each other, it
should be a series. Please check if everything works when it's pushed
out, which should be shortly.

And since I'm on a plane and this doesn't appear to want to send, when
you do see it, please also add HOWTO additions similar to the fio.1
additions you made.

-- 
Jens Axboe

Re: [PATCH] io_uring: Add IO_U_F_DEVICE_ERROR to identify error types

[Vincent Fu]

On 9/26/24 13:48, Jens Axboe wrote:
> On 9/25/24 6:41 PM, Minwoo Im wrote:
>> diff --git a/io_u.h b/io_u.h
>> index ab93d50f967e..20afad667ee1 100644
>> --- a/io_u.h
>> +++ b/io_u.h
>> @@ -22,6 +22,7 @@ enum {
>>   	IO_U_F_BARRIER		= 1 << 6,
>>   	IO_U_F_VER_LIST		= 1 << 7,
>>   	IO_U_F_PATTERN_DONE	= 1 << 8,
>> +	IO_U_F_DEVICE_ERROR	= 1 << 9,
>>   };
>>   
>>   /*
> 
> The patches you sent should've been a series, how are they supposed to
> both apply when you add an item here for each of them as if the other
> one doesn't exist?
> 
> I'll fix it up, but for the future, if patches depend on each other, it
> should be a series. Please check if everything works when it's pushed
> out, which should be shortly.
> 
> And since I'm on a plane and this doesn't appear to want to send, when
> you do see it, please also add HOWTO additions similar to the fio.1
> additions you made.
> 

Also, Minwoo, we are seeing some build failures with your patches with 
compilers rejecting abs(io_u->error) since error is unsigned. Please fix 
that up as well.

I do have a bot that automatically runs mailing list patches through our 
CI, but it does not report the results to the list and I am not always 
quick enough to manually report CI failures:

https://github.com/fiotestbot/fio/actions

Vincent

Re: [PATCH] io_uring: Add IO_U_F_DEVICE_ERROR to identify error types

[Jens Axboe]

On 9/26/24 12:08 PM, Vincent Fu wrote:
> On 9/26/24 13:48, Jens Axboe wrote:
>> On 9/25/24 6:41 PM, Minwoo Im wrote:
>>> diff --git a/io_u.h b/io_u.h
>>> index ab93d50f967e..20afad667ee1 100644
>>> --- a/io_u.h
>>> +++ b/io_u.h
>>> @@ -22,6 +22,7 @@ enum {
>>>       IO_U_F_BARRIER        = 1 << 6,
>>>       IO_U_F_VER_LIST        = 1 << 7,
>>>       IO_U_F_PATTERN_DONE    = 1 << 8,
>>> +    IO_U_F_DEVICE_ERROR    = 1 << 9,
>>>   };
>>>     /*
>>
>> The patches you sent should've been a series, how are they supposed to
>> both apply when you add an item here for each of them as if the other
>> one doesn't exist?
>>
>> I'll fix it up, but for the future, if patches depend on each other, it
>> should be a series. Please check if everything works when it's pushed
>> out, which should be shortly.
>>
>> And since I'm on a plane and this doesn't appear to want to send, when
>> you do see it, please also add HOWTO additions similar to the fio.1
>> additions you made.
>>
> 
> Also, Minwoo, we are seeing some build failures with your patches with
> compilers rejecting abs(io_u->error) since error is unsigned. Please
> fix that up as well.

Doh yes, I fixed that up now.

> I do have a bot that automatically runs mailing list patches through
> our CI, but it does not report the results to the list and I am not
> always quick enough to manually report CI failures:
> 
> https://github.com/fiotestbot/fio/actions

Send them to the list! It's not like it's a high traffic list, and
that's super useful. Mostly because it gets the same coverage as a
github pr then, but also because it'll inform the submitter that there's
an issue without either you or me letting them now. Hence it saves time
and cycles, which is a big win in my book.

-- 
Jens Axboe

Re: [PATCH] io_uring: Add IO_U_F_DEVICE_ERROR to identify error types

[Minwoo Im]

On 24-09-26 11:48:22, Jens Axboe wrote:
> On 9/25/24 6:41 PM, Minwoo Im wrote:
> > diff --git a/io_u.h b/io_u.h
> > index ab93d50f967e..20afad667ee1 100644
> > --- a/io_u.h
> > +++ b/io_u.h
> > @@ -22,6 +22,7 @@ enum {
> >  	IO_U_F_BARRIER		= 1 << 6,
> >  	IO_U_F_VER_LIST		= 1 << 7,
> >  	IO_U_F_PATTERN_DONE	= 1 << 8,
> > +	IO_U_F_DEVICE_ERROR	= 1 << 9,
> >  };
> >  
> >  /*
> 
> The patches you sent should've been a series, how are they supposed to
> both apply when you add an item here for each of them as if the other
> one doesn't exist?

My bad.  Sorry for making mess here.  Based on the feature, I think it
should not be a series, but, yes, adding new flags with the same value
is totally a mess.

> 
> I'll fix it up, but for the future, if patches depend on each other, it
> should be a series. Please check if everything works when it's pushed
> out, which should be shortly.

Okay.

> 
> And since I'm on a plane and this doesn't appear to want to send, when
> you do see it, please also add HOWTO additions similar to the fio.1
> additions you made.

Will do it.  Thanks!

> 
> -- 
> Jens Axboe

Re: [PATCH] io_uring: Add IO_U_F_DEVICE_ERROR to identify error types

[Minwoo Im]

On 24-09-26 15:06:49, Jens Axboe wrote:
> On 9/26/24 12:08 PM, Vincent Fu wrote:
> > On 9/26/24 13:48, Jens Axboe wrote:
> >> On 9/25/24 6:41 PM, Minwoo Im wrote:
> >>> diff --git a/io_u.h b/io_u.h
> >>> index ab93d50f967e..20afad667ee1 100644
> >>> --- a/io_u.h
> >>> +++ b/io_u.h
> >>> @@ -22,6 +22,7 @@ enum {
> >>>       IO_U_F_BARRIER        = 1 << 6,
> >>>       IO_U_F_VER_LIST        = 1 << 7,
> >>>       IO_U_F_PATTERN_DONE    = 1 << 8,
> >>> +    IO_U_F_DEVICE_ERROR    = 1 << 9,
> >>>   };
> >>>     /*
> >>
> >> The patches you sent should've been a series, how are they supposed to
> >> both apply when you add an item here for each of them as if the other
> >> one doesn't exist?
> >>
> >> I'll fix it up, but for the future, if patches depend on each other, it
> >> should be a series. Please check if everything works when it's pushed
> >> out, which should be shortly.
> >>
> >> And since I'm on a plane and this doesn't appear to want to send, when
> >> you do see it, please also add HOWTO additions similar to the fio.1
> >> additions you made.
> >>
> > 
> > Also, Minwoo, we are seeing some build failures with your patches with
> > compilers rejecting abs(io_u->error) since error is unsigned. Please
> > fix that up as well.
> 
> Doh yes, I fixed that up now.

I appreciate for your fix!

> 
> > I do have a bot that automatically runs mailing list patches through
> > our CI, but it does not report the results to the list and I am not
> > always quick enough to manually report CI failures:
> > 
> > https://github.com/fiotestbot/fio/actions

Vincent,

I will have a look into the CI when I posted patches to the mailing list.
It's super cool to have it.

> 
> Send them to the list! It's not like it's a high traffic list, and
> that's super useful. Mostly because it gets the same coverage as a
> github pr then, but also because it'll inform the submitter that there's
> an issue without either you or me letting them now. Hence it saves time
> and cycles, which is a big win in my book.
> 
> -- 
> Jens Axboe

Re: [PATCH] io_uring: Add IO_U_F_DEVICE_ERROR to identify error types

[Minwoo Im]

On 24-09-26 15:06:49, Jens Axboe wrote:
> On 9/26/24 12:08 PM, Vincent Fu wrote:
> > On 9/26/24 13:48, Jens Axboe wrote:
> >> On 9/25/24 6:41 PM, Minwoo Im wrote:
> >>> diff --git a/io_u.h b/io_u.h
> >>> index ab93d50f967e..20afad667ee1 100644
> >>> --- a/io_u.h
> >>> +++ b/io_u.h
> >>> @@ -22,6 +22,7 @@ enum {
> >>>       IO_U_F_BARRIER        = 1 << 6,
> >>>       IO_U_F_VER_LIST        = 1 << 7,
> >>>       IO_U_F_PATTERN_DONE    = 1 << 8,
> >>> +    IO_U_F_DEVICE_ERROR    = 1 << 9,
> >>>   };
> >>>     /*
> >>
> >> The patches you sent should've been a series, how are they supposed to
> >> both apply when you add an item here for each of them as if the other
> >> one doesn't exist?
> >>
> >> I'll fix it up, but for the future, if patches depend on each other, it
> >> should be a series. Please check if everything works when it's pushed
> >> out, which should be shortly.
> >>
> >> And since I'm on a plane and this doesn't appear to want to send, when
> >> you do see it, please also add HOWTO additions similar to the fio.1
> >> additions you made.
> >>
> > 
> > Also, Minwoo, we are seeing some build failures with your patches with
> > compilers rejecting abs(io_u->error) since error is unsigned. Please
> > fix that up as well.
> 
> Doh yes, I fixed that up now.

Jens,

How about the following one for the fix?  (checked with clang)

diff --git a/engines/io_uring.c b/engines/io_uring.c
index 6c07c1011c40..85cebf8371cf 100644
--- a/engines/io_uring.c
+++ b/engines/io_uring.c
@@ -589,7 +589,7 @@ ret:
                io_u_set(td, io_u, IO_U_F_DEVICE_ERROR);
        else
                io_u_clear(td, io_u, IO_U_F_DEVICE_ERROR);
-       io_u->error = io_u->error;
+       io_u->error = abs((int)io_u->error);
        return io_u;
 }

io_u->error is unsigned, but it can have negative value as an errno.
io_u->error should be converted to a positive one especially when it's
an errno.  If we don't, it will be like Unknown error:

fio: io_u error on file /dev/ng0n1: Unknown error -22: write offset=429916160, buflen=1048576

> 
> > I do have a bot that automatically runs mailing list patches through
> > our CI, but it does not report the results to the list and I am not
> > always quick enough to manually report CI failures:
> > 
> > https://github.com/fiotestbot/fio/actions
> 
> Send them to the list! It's not like it's a high traffic list, and
> that's super useful. Mostly because it gets the same coverage as a
> github pr then, but also because it'll inform the submitter that there's
> an issue without either you or me letting them now. Hence it saves time
> and cycles, which is a big win in my book.
> 
> -- 
> Jens Axboe

Re: [PATCH] io_uring: Add IO_U_F_DEVICE_ERROR to identify error types

[Jens Axboe]

On 9/26/24 3:33 PM, Minwoo Im wrote:
> On 24-09-26 15:06:49, Jens Axboe wrote:
>> On 9/26/24 12:08 PM, Vincent Fu wrote:
>>> On 9/26/24 13:48, Jens Axboe wrote:
>>>> On 9/25/24 6:41 PM, Minwoo Im wrote:
>>>>> diff --git a/io_u.h b/io_u.h
>>>>> index ab93d50f967e..20afad667ee1 100644
>>>>> --- a/io_u.h
>>>>> +++ b/io_u.h
>>>>> @@ -22,6 +22,7 @@ enum {
>>>>>       IO_U_F_BARRIER        = 1 << 6,
>>>>>       IO_U_F_VER_LIST        = 1 << 7,
>>>>>       IO_U_F_PATTERN_DONE    = 1 << 8,
>>>>> +    IO_U_F_DEVICE_ERROR    = 1 << 9,
>>>>>   };
>>>>>     /*
>>>>
>>>> The patches you sent should've been a series, how are they supposed to
>>>> both apply when you add an item here for each of them as if the other
>>>> one doesn't exist?
>>>>
>>>> I'll fix it up, but for the future, if patches depend on each other, it
>>>> should be a series. Please check if everything works when it's pushed
>>>> out, which should be shortly.
>>>>
>>>> And since I'm on a plane and this doesn't appear to want to send, when
>>>> you do see it, please also add HOWTO additions similar to the fio.1
>>>> additions you made.
>>>>
>>>
>>> Also, Minwoo, we are seeing some build failures with your patches with
>>> compilers rejecting abs(io_u->error) since error is unsigned. Please
>>> fix that up as well.
>>
>> Doh yes, I fixed that up now.
> 
> Jens,
> 
> How about the following one for the fix?  (checked with clang)
> 
> diff --git a/engines/io_uring.c b/engines/io_uring.c
> index 6c07c1011c40..85cebf8371cf 100644
> --- a/engines/io_uring.c
> +++ b/engines/io_uring.c
> @@ -589,7 +589,7 @@ ret:
>                 io_u_set(td, io_u, IO_U_F_DEVICE_ERROR);
>         else
>                 io_u_clear(td, io_u, IO_U_F_DEVICE_ERROR);
> -       io_u->error = io_u->error;
> +       io_u->error = abs((int)io_u->error);
>         return io_u;
>  }
> 
> io_u->error is unsigned, but it can have negative value as an errno.
> io_u->error should be converted to a positive one especially when it's
> an errno.  If we don't, it will be like Unknown error:
> 
> fio: io_u error on file /dev/ng0n1: Unknown error -22: write offset=429916160, buflen=1048576

Yeah I think that looks good. Can you send this as a patch against the
current tree? I did do a fixup, but way too jet lagged and it's
obviously garbage.

-- 
Jens Axboe

Re: [PATCH] io_uring: Add IO_U_F_DEVICE_ERROR to identify error types

[Minwoo Im]

On 24-09-26 18:54:35, Jens Axboe wrote:
> On 9/26/24 3:33 PM, Minwoo Im wrote:
> > On 24-09-26 15:06:49, Jens Axboe wrote:
> >> On 9/26/24 12:08 PM, Vincent Fu wrote:
> >>> On 9/26/24 13:48, Jens Axboe wrote:
> >>>> On 9/25/24 6:41 PM, Minwoo Im wrote:
> >>>>> diff --git a/io_u.h b/io_u.h
> >>>>> index ab93d50f967e..20afad667ee1 100644
> >>>>> --- a/io_u.h
> >>>>> +++ b/io_u.h
> >>>>> @@ -22,6 +22,7 @@ enum {
> >>>>>       IO_U_F_BARRIER        = 1 << 6,
> >>>>>       IO_U_F_VER_LIST        = 1 << 7,
> >>>>>       IO_U_F_PATTERN_DONE    = 1 << 8,
> >>>>> +    IO_U_F_DEVICE_ERROR    = 1 << 9,
> >>>>>   };
> >>>>>     /*
> >>>>
> >>>> The patches you sent should've been a series, how are they supposed to
> >>>> both apply when you add an item here for each of them as if the other
> >>>> one doesn't exist?
> >>>>
> >>>> I'll fix it up, but for the future, if patches depend on each other, it
> >>>> should be a series. Please check if everything works when it's pushed
> >>>> out, which should be shortly.
> >>>>
> >>>> And since I'm on a plane and this doesn't appear to want to send, when
> >>>> you do see it, please also add HOWTO additions similar to the fio.1
> >>>> additions you made.
> >>>>
> >>>
> >>> Also, Minwoo, we are seeing some build failures with your patches with
> >>> compilers rejecting abs(io_u->error) since error is unsigned. Please
> >>> fix that up as well.
> >>
> >> Doh yes, I fixed that up now.
> > 
> > Jens,
> > 
> > How about the following one for the fix?  (checked with clang)
> > 
> > diff --git a/engines/io_uring.c b/engines/io_uring.c
> > index 6c07c1011c40..85cebf8371cf 100644
> > --- a/engines/io_uring.c
> > +++ b/engines/io_uring.c
> > @@ -589,7 +589,7 @@ ret:
> >                 io_u_set(td, io_u, IO_U_F_DEVICE_ERROR);
> >         else
> >                 io_u_clear(td, io_u, IO_U_F_DEVICE_ERROR);
> > -       io_u->error = io_u->error;
> > +       io_u->error = abs((int)io_u->error);
> >         return io_u;
> >  }
> > 
> > io_u->error is unsigned, but it can have negative value as an errno.
> > io_u->error should be converted to a positive one especially when it's
> > an errno.  If we don't, it will be like Unknown error:
> > 
> > fio: io_u error on file /dev/ng0n1: Unknown error -22: write offset=429916160, buflen=1048576
> 
> Yeah I think that looks good. Can you send this as a patch against the
> current tree? I did do a fixup, but way too jet lagged and it's
> obviously garbage.

Sure, I will, Thanks!