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

[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>.

[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