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;
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
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: <[email protected]>.
On 8/30/06, *Zach Brown* <[email protected] <mailto:[email protected]>> 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
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);
}
/*
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
> 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