[2.6.18-rc* PATCH RFC]: Correct ambiguous errno of aio

[2.6.18-rc* PATCH RFC]: Correct ambiguous errno of aio

[Yi Yang]

In the current implementation of AIO, for the operation IOCB_CMD_FDSYNC
and IOCB_CMD_FSYNC, the returned errno is -EINVAL although the kernel
does know them, I think the correct errno should be -EOPNOTSUPP which
means they aren't be implemented or supported.

>From the kernel source code, we can see they can be supported without
any code modification if a specific filesystem implements aio_fsync.

Another obvious problem is the function aio_fsync is as same as the
function aio_fdsync, so they are duplicate, only one is enough.

Here this patch is.



--- a/fs/aio.c.orig 2006-08-28 15:15:18.000000000 +0800
+++ b/fs/aio.c 2006-08-28 15:33:59.000000000 +0800
@@ -1363,20 +1363,10 @@ static ssize_t aio_pwrite(struct kiocb *
return ret;
}

-static ssize_t aio_fdsync(struct kiocb *iocb)
-{
- struct file *file = iocb->ki_filp;
- ssize_t ret = -EINVAL;
-
- if (file->f_op->aio_fsync)
- ret = file->f_op->aio_fsync(iocb, 1);
- return ret;
-}
-
static ssize_t aio_fsync(struct kiocb *iocb)
{
struct file *file = iocb->ki_filp;
- ssize_t ret = -EINVAL;
+ ssize_t ret = -EOPNOTSUPP;

if (file->f_op->aio_fsync)
ret = file->f_op->aio_fsync(iocb, 0);
@@ -1425,12 +1415,12 @@ static ssize_t aio_setup_iocb(struct kio
kiocb->ki_retry = aio_pwrite;
break;
case IOCB_CMD_FDSYNC:
- ret = -EINVAL;
+ ret = -EOPNOTSUPP;
if (file->f_op->aio_fsync)
- kiocb->ki_retry = aio_fdsync;
+ kiocb->ki_retry = aio_fsync;
break;
case IOCB_CMD_FSYNC:
- ret = -EINVAL;
+ ret = -EOPNOTSUPP;
if (file->f_op->aio_fsync)
kiocb->ki_retry = aio_fsync;
break;

Re: [2.6.18-rc* PATCH RFC]: Correct ambiguous errno of aio

[Yi Yang]

Frederik Deweerdt 写道:
> On Mon, Aug 28, 2006 at 09:28:48PM +0800, Yi Yang wrote:
>   
>> In the current implementation of AIO, for the operation IOCB_CMD_FDSYNC
>> and IOCB_CMD_FSYNC, the returned errno is -EINVAL although the kernel
>> does know them, I think the correct errno should be -EOPNOTSUPP which
>> means they aren't be implemented or supported.
>>     
> Hi, 
>
> If I'm not mistaken, returning EINVAL conforms to POSIX, isn't it?
>   
But POSIX also defined ENOTSUP which is equal to EOPNOTSUPP for linux.
> http://www.opengroup.org/onlinepubs/009695399/functions/fsync.html
> Regards,
> Frederik
>  
>
>   

Re: [2.6.18-rc* PATCH RFC]: Correct ambiguous errno of aio

[Randy.Dunlap]

On Mon, 28 Aug 2006 21:28:48 +0800 Yi Yang wrote:

> In the current implementation of AIO, for the operation IOCB_CMD_FDSYNC
> and IOCB_CMD_FSYNC, the returned errno is -EINVAL although the kernel
> does know them, I think the correct errno should be -EOPNOTSUPP which
> means they aren't be implemented or supported.
> 
> >From the kernel source code, we can see they can be supported without
> any code modification if a specific filesystem implements aio_fsync.
> 
> Another obvious problem is the function aio_fsync is as same as the
> function aio_fdsync, so they are duplicate, only one is enough.
> 
> Here this patch is.
> 
gmail ate all of the tabs, maybe other whitespace problems.

> 
> --- a/fs/aio.c.orig 2006-08-28 15:15:18.000000000 +0800
> +++ b/fs/aio.c 2006-08-28 15:33:59.000000000 +0800
> @@ -1363,20 +1363,10 @@ static ssize_t aio_pwrite(struct kiocb *
> return ret;
> }
> 
> -static ssize_t aio_fdsync(struct kiocb *iocb)
> -{
> - struct file *file = iocb->ki_filp;
> - ssize_t ret = -EINVAL;
> -
> - if (file->f_op->aio_fsync)
> - ret = file->f_op->aio_fsync(iocb, 1);
> - return ret;
> -}
> -
> static ssize_t aio_fsync(struct kiocb *iocb)
> {
> struct file *file = iocb->ki_filp;
> - ssize_t ret = -EINVAL;
> + ssize_t ret = -EOPNOTSUPP;
> 
> if (file->f_op->aio_fsync)
> ret = file->f_op->aio_fsync(iocb, 0);
> @@ -1425,12 +1415,12 @@ static ssize_t aio_setup_iocb(struct kio
> kiocb->ki_retry = aio_pwrite;
> break;
> case IOCB_CMD_FDSYNC:
> - ret = -EINVAL;
> + ret = -EOPNOTSUPP;
> if (file->f_op->aio_fsync)
> - kiocb->ki_retry = aio_fdsync;
> + kiocb->ki_retry = aio_fsync;
> break;
> case IOCB_CMD_FSYNC:
> - ret = -EINVAL;
> + ret = -EOPNOTSUPP;
> if (file->f_op->aio_fsync)
> kiocb->ki_retry = aio_fsync;
> break;
> -

---
~Randy

Re: [2.6.18-rc* PATCH RFC]: Correct ambiguous errno of aio

[Frederik Deweerdt]

On Mon, Aug 28, 2006 at 09:28:48PM +0800, Yi Yang wrote:
> In the current implementation of AIO, for the operation IOCB_CMD_FDSYNC
> and IOCB_CMD_FSYNC, the returned errno is -EINVAL although the kernel
> does know them, I think the correct errno should be -EOPNOTSUPP which
> means they aren't be implemented or supported.
Hi, 

If I'm not mistaken, returning EINVAL conforms to POSIX, isn't it?
http://www.opengroup.org/onlinepubs/009695399/functions/fsync.html
Regards,
Frederik
 

[2.6.18-rc* PATCH RFC]: Correct ambiguous errno of aio

[Yi Yang]

Sorry, the last post is corrupted by email client, this is a repost.

In the current implementation of AIO, for the operation IOCB_CMD_FDSYNC

and IOCB_CMD_FSYNC, the returned errno is -EINVAL although the kernel

does know them, I think the correct errno should be -EOPNOTSUPP which

means they aren't be implemented or supported.

--- a/fs/aio.c.orig	2006-08-28 15:15:18.000000000 +0800
+++ b/fs/aio.c	2006-08-28 15:33:59.000000000 +0800
@@ -1363,20 +1363,10 @@ static ssize_t aio_pwrite(struct kiocb *
 	return ret;
 }
 
-static ssize_t aio_fdsync(struct kiocb *iocb)
-{
-	struct file *file = iocb->ki_filp;
-	ssize_t ret = -EINVAL;
-
-	if (file->f_op->aio_fsync)
-		ret = file->f_op->aio_fsync(iocb, 1);
-	return ret;
-}
-
 static ssize_t aio_fsync(struct kiocb *iocb)
 {
 	struct file *file = iocb->ki_filp;
-	ssize_t ret = -EINVAL;
+	ssize_t ret = -EOPNOTSUPP;
 
 	if (file->f_op->aio_fsync)
 		ret = file->f_op->aio_fsync(iocb, 0);
@@ -1425,12 +1415,12 @@ static ssize_t aio_setup_iocb(struct kio
 			kiocb->ki_retry = aio_pwrite;
 		break;
 	case IOCB_CMD_FDSYNC:
-		ret = -EINVAL;
+		ret = -EOPNOTSUPP;
 		if (file->f_op->aio_fsync)
-			kiocb->ki_retry = aio_fdsync;
+			kiocb->ki_retry = aio_fsync;
 		break;
 	case IOCB_CMD_FSYNC:
-		ret = -EINVAL;
+		ret = -EOPNOTSUPP;
 		if (file->f_op->aio_fsync)
 			kiocb->ki_retry = aio_fsync;
 		break;

Re: [2.6.18-rc* PATCH RFC]: Correct ambiguous errno of aio

[Zach Brown]

Sorry, we shouldn't merge this patch in its current form.

> In the current implementation of AIO, for the operation IOCB_CMD_FDSYNC
> and IOCB_CMD_FSYNC, the returned errno is -EINVAL although the kernel
> does know them, I think the correct errno should be -EOPNOTSUPP which
> means they aren't be implemented or supported.

Like it or not, the sys_io_submit() interface returns -EINVAL when the
file descriptor doesn't support the requested command.  Changing the
binary interface is a big deal and should not be done lightly.  What is
the motivation for making this change?

Even if we decided to, we'd want to do it for all the commands.  This
patch only addresses F{D,}SYNC.  All the other commands would still
return -EINVAL if the descriptor doesn't have the corresponding ->aio_
method, leaving userspace do deal with more complexity.

> -static ssize_t aio_fdsync(struct kiocb *iocb)
> -{
> -	struct file *file = iocb->ki_filp;
> -	ssize_t ret = -EINVAL;
> -
> -	if (file->f_op->aio_fsync)
> -		ret = file->f_op->aio_fsync(iocb, 1);
> -	return ret;
> -}
> -
>  static ssize_t aio_fsync(struct kiocb *iocb)
>  {
>  	struct file *file = iocb->ki_filp;
> -	ssize_t ret = -EINVAL;
> +	ssize_t ret = -EOPNOTSUPP;
>  
>  	if (file->f_op->aio_fsync)
>  		ret = file->f_op->aio_fsync(iocb, 0);

>  	case IOCB_CMD_FDSYNC:
> -		ret = -EINVAL;
> +		ret = -EOPNOTSUPP;
>  		if (file->f_op->aio_fsync)
> -			kiocb->ki_retry = aio_fdsync;
> +			kiocb->ki_retry = aio_fsync;

Hmm, your most recent patch didn't mention this aio_f{d,}sync() change
though the earlier one did.  Please make sure all patch submissions have
complete descriptions.

These calls are not the same, notice that they differ in the second
argument to their ->aio_fsync() calls.  Cleaning up the ->aio_fsync()
interface might well be reasonable.  Missing that subtle difference
suggests that it should be more clear and there are precisely zero
merged ->aio_fsync() users.  But that kind of cleanup belongs in a
separate patch with its own justification.

Are you working with an ->aio_fsync() user that might be merged?

- z

Re: [2.6.18-rc* PATCH RFC]: Correct ambiguous errno of aio

[Benjamin LaHaise]

On Tue, Aug 29, 2006 at 11:32:05AM -0700, Zach Brown wrote:
> Like it or not, the sys_io_submit() interface returns -EINVAL when the
> file descriptor doesn't support the requested command.  Changing the
> binary interface is a big deal and should not be done lightly.  What is
> the motivation for making this change?

-EOPNOTSUPP also gives the wrong error message, as it is a networking 
error.  Any program which knows that it is submitting a correctly filled 
in set of parameters can deduce the reason for the -EINVAL.  Changing it 
otherwise would result in behaviour outside of that specified in the man 
page (which lists reasons for the -EINVAL result).

		-ben
-- 
"Time is of no importance, Mr. President, only life is important."
Don't Email: <dont@kvack.org>.

Re: [2.6.18-rc* PATCH RFC]: Correct ambiguous errno of aio

[Yi Yang]

On 8/30/06, *Zach Brown* <zab@zabbo.net <mailto:zab@zabbo.net>> wrote:


    Sorry, we shouldn't merge this patch in its current form.

    > In the current implementation of AIO, for the operation IOCB_CMD_FDSYNC
    > and IOCB_CMD_FSYNC, the returned errno is -EINVAL although the kernel
    > does know them, I think the correct errno should be -EOPNOTSUPP which
    > means they aren't be implemented or supported.

    Like it or not, the sys_io_submit() interface returns -EINVAL when the
    file descriptor doesn't support the requested command. Changing the
    binary interface is a big deal and should not be done lightly. What is
    the motivation for making this change?


When I run ltp aio test case, for the operation OCB_CMD_FDSYNC and
IOCB_CMD_FSYNC, io_submit returns -1 and errno is EINVAL, perror's
output is "Invalid arguments", from the user's perspective, the
arguments are valid
and the kernel also know it and progress the process to file operation
of the
filesystem actually, so I think ENOTSUP is more appropriate. Note
ENOTSUP in
the user space corresponds to EOPNOTSUPP in the kernel mode. For ENOTSUP,
perror's output is "Function isn't implemented", obviously, it is a
reasonable explanation
about the execution error and not ambiguous.

    Even if we decided to, we'd want to do it for all the commands. This
    patch only addresses F{D,}SYNC. All the other commands would still
    return -EINVAL if the descriptor doesn't have the corresponding ->aio_
    method, leaving userspace do deal with more complexity.


What you said is true, but I want to know if my idea is right.
- Show quoted text -

    > -static ssize_t aio_fdsync(struct kiocb *iocb)
    > -{
    > - struct file *file = iocb->ki_filp;
    > - ssize_t ret = -EINVAL;
    > -
    > - if (file->f_op->aio_fsync)
    > - ret = file->f_op->aio_fsync(iocb, 1);
    > - return ret;
    > -}
    > -
    > static ssize_t aio_fsync(struct kiocb *iocb)
    > {
    > struct file *file = iocb->ki_filp;
    > - ssize_t ret = -EINVAL;
    > + ssize_t ret = -EOPNOTSUPP;
    >
    > if (file->f_op->aio_fsync)
    > ret = file->f_op->aio_fsync(iocb, 0);

    > case IOCB_CMD_FDSYNC:
    > - ret = -EINVAL;
    > + ret = -EOPNOTSUPP;
    > if (file->f_op->aio_fsync)
    > - kiocb->ki_retry = aio_fdsync;
    > + kiocb->ki_retry = aio_fsync;

    Hmm, your most recent patch didn't mention this aio_f{d,}sync() change
    though the earlier one did. Please make sure all patch submissions have
    complete descriptions.

    These calls are not the same, notice that they differ in the second
    argument to their ->aio_fsync() calls. Cleaning up the ->aio_fsync()
    interface might well be reasonable. Missing that subtle difference
    suggests that it should be more clear and there are precisely zero
    merged ->aio_fsync() users. But that kind of cleanup belongs in a
    separate patch with its own justification.


Thank your care, it is my mistake and I'm so sorry for this, Your suggest
is good, I'll send a small claenup patch for this.

    Are you working with an ->aio_fsync() user that might be merged? 


No, It just is noticed by me while I trace io_submit for FSYNC/FDSYNC.

- z

[2.6.18-rc5 PATCH]: aio cleanup

[Yi Yang]

As Zach Brown said, a cleanup patch is reasonable. Here it is.

This patch extracts the common part from aio_fsync and aio_fdsync
 and define a new inlined function aio_xsync, then aio_fsync and 
aio_fdsync just call aio_xsunc in the almost same way except second
 argument is different, one is 1 and another 0.

--- a/fs/aio.c.orig	2006-08-30 22:00:00.000000000 +0800
+++ b/fs/aio.c	2006-08-30 22:08:35.000000000 +0800
@@ -1363,24 +1363,24 @@ static ssize_t aio_pwrite(struct kiocb *
 	return ret;
 }
 
-static ssize_t aio_fdsync(struct kiocb *iocb)
+static inline ssize_t aio_xsync(struct kiocb *iocb, int flags)
 {
 	struct file *file = iocb->ki_filp;
 	ssize_t ret = -EINVAL;
 
 	if (file->f_op->aio_fsync)
-		ret = file->f_op->aio_fsync(iocb, 1);
+		ret = file->f_op->aio_fsync(iocb, flags);
 	return ret;
 }
 
-static ssize_t aio_fsync(struct kiocb *iocb)
+static ssize_t aio_fdsync(struct kiocb *iocb)
 {
-	struct file *file = iocb->ki_filp;
-	ssize_t ret = -EINVAL;
+	return aio_xsync(iocb, 1);
+}
 
-	if (file->f_op->aio_fsync)
-		ret = file->f_op->aio_fsync(iocb, 0);
-	return ret;
+static ssize_t aio_fsync(struct kiocb *iocb)
+{
+	return aio_xsync(iocb, 0);
 }
 
 /*

Re: [2.6.18-rc5 PATCH]: aio cleanup

[Zach Brown]

Yi Yang wrote:
> As Zach Brown said, a cleanup patch is reasonable. Here it is.

I said a cleanup patch could be reasonable :)

> This patch extracts the common part from aio_fsync and aio_fdsync
> and define a new inlined function aio_xsync, then aio_fsync and
> aio_fdsync just call aio_xsunc in the almost same way except second
> argument is different, one is 1 and another 0.

I don't think we need to change this, to be honest.  They're tiny
functions without users.  It doesn't seem worth the trouble, minimal
though it is.

Maybe if we had ->aio_fsync() users it would be more clear if there was
an opportunity to really clarify the interface.

- z

Re: [2.6.18-rc* PATCH RFC]: Correct ambiguous errno of aio

[Zach Brown]

> When I run ltp aio test case, for the operation OCB_CMD_FDSYNC and 
> IOCB_CMD_FSYNC, io_submit returns -1 and errno is EINVAL, perror's 
> output is "Invalid arguments", from the user's perspective, the 
> arguments are valid and the kernel also know it and progress the
> process to file operation of the filesystem actually, so I think
> ENOTSUP is more appropriate. Note ENOTSUP in the user space
> corresponds to EOPNOTSUPP in the kernel mode. For ENOTSUP, perror's
> output is "Function isn't implemented", obviously, it is a reasonable
> explanation about the execution error and not ambiguous.

This might have been a convincing argument when the interface was first
being written, but now we have to take into account that changing the
interface can break existing users.

That you prefer EOPNOTSUPP is not a compelling reason to break existing
setups, sorry.

- z