: Correct ambiguous errno of aio

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.

@@ -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;

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

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.

-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: <[email protected]>.

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.

@@ -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);
}

/*

I said a cleanup patch could be reasonable :)

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

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