Re: [PATCH] ext4: Remove repeated test in ext4_file_read_iter.

[Sean Fu]

On Wed, Jan 10, 2018 at 03:02:55PM -0500, Theodore Ts'o wrote:
> On Wed, Jan 10, 2018 at 10:01:49PM +0800, Sean Fu wrote:
> >
> > Correct, IOCB_NOWAIT read with zero count can return -EAGAIN, But I
> > think that it is reasonable. while it got lock, zero would be returned
> > in this case.
> 
> Returning -EAGAIN and 0 are not the same thing.  Specifically
> returning 0 means "end of file".  Hence, it is NOT reasonable.
> 
I know that two returnning value mean different things.
once the inode is not under lock contention, read again with zero count
would return zero.
> See the man page for read(2) or the relevant Single Unix Specification
> standard:
> 
> RETURN VALUE
>  On success, the number of bytes read is returned (zero indicates
>  end of file)
> 
> Changing the behavior of the system in which will break userspace is
> never acceptable, and even more so given that this is just a "cleanup
> patch".
>
I agree with you at this point.
Thanks for your reply.

>   - Ted
> 

Re: [PATCH] ext4: Remove repeated test in ext4_file_read_iter.

[Sean Fu]

On Wed, Jan 10, 2018 at 03:02:55PM -0500, Theodore Ts'o wrote:
> On Wed, Jan 10, 2018 at 10:01:49PM +0800, Sean Fu wrote:
> >
> > Correct, IOCB_NOWAIT read with zero count can return -EAGAIN, But I
> > think that it is reasonable. while it got lock, zero would be returned
> > in this case.
> 
> Returning -EAGAIN and 0 are not the same thing.  Specifically
> returning 0 means "end of file".  Hence, it is NOT reasonable.
> 
I know that two returnning value mean different things.
once the inode is not under lock contention, read again with zero count
would return zero.
> See the man page for read(2) or the relevant Single Unix Specification
> standard:
> 
> RETURN VALUE
>  On success, the number of bytes read is returned (zero indicates
>  end of file)
> 
> Changing the behavior of the system in which will break userspace is
> never acceptable, and even more so given that this is just a "cleanup
> patch".
>
I agree with you at this point.
Thanks for your reply.

>   - Ted
> 

Re: [PATCH] ext4: Remove repeated test in ext4_file_read_iter.

[Theodore Ts'o]

On Wed, Jan 10, 2018 at 10:01:49PM +0800, Sean Fu wrote:
>
> Correct, IOCB_NOWAIT read with zero count can return -EAGAIN, But I
> think that it is reasonable. while it got lock, zero would be returned
> in this case.

Returning -EAGAIN and 0 are not the same thing.  Specifically
returning 0 means "end of file".  Hence, it is NOT reasonable.

See the man page for read(2) or the relevant Single Unix Specification
standard:

RETURN VALUE
 On success, the number of bytes read is returned (zero indicates
 end of file)

Changing the behavior of the system in which will break userspace is
never acceptable, and even more so given that this is just a "cleanup
patch".

- Ted

Re: [PATCH] ext4: Remove repeated test in ext4_file_read_iter.

[Theodore Ts'o]

On Wed, Jan 10, 2018 at 10:01:49PM +0800, Sean Fu wrote:
>
> Correct, IOCB_NOWAIT read with zero count can return -EAGAIN, But I
> think that it is reasonable. while it got lock, zero would be returned
> in this case.

Returning -EAGAIN and 0 are not the same thing.  Specifically
returning 0 means "end of file".  Hence, it is NOT reasonable.

See the man page for read(2) or the relevant Single Unix Specification
standard:

RETURN VALUE
 On success, the number of bytes read is returned (zero indicates
 end of file)

Changing the behavior of the system in which will break userspace is
never acceptable, and even more so given that this is just a "cleanup
patch".

- Ted

Re: [PATCH] ext4: Remove repeated test in ext4_file_read_iter.

[Sean Fu]

On Wed, Jan 03, 2018 at 02:08:05AM +, Al Viro wrote:
> On Wed, Dec 27, 2017 at 04:19:58PM +0800, Sean Fu wrote:
> > generic_file_read_iter has done the count test.
> > So ext4_file_read_iter don't need to test the count repeatedly.
> 
> Huh?  You do realize that generic_file_read_iter() is not the
> only variant possible there, right?
>
Yes, I know that generic_file_read_iter() is not the only variant possible.
The other possible dax_iomap_rw() with zero count would return zero.
> static ssize_t ext4_dax_read_iter(struct kiocb *iocb, struct iov_iter *to)
> {
> struct inode *inode = file_inode(iocb->ki_filp);
> ssize_t ret;
> 
> if (!inode_trylock_shared(inode)) {
> if (iocb->ki_flags & IOCB_NOWAIT)
> return -EAGAIN;
> inode_lock_shared(inode);
> }
> 
> ... and now IOCB_NOWAIT read with zero count can fail with -EAGAIN.
>
Correct, IOCB_NOWAIT read with zero count can return -EAGAIN, But I
think that it is reasonable. while it got lock, zero would be returned
in this case.

Re: [PATCH] ext4: Remove repeated test in ext4_file_read_iter.

[Sean Fu]

On Wed, Jan 03, 2018 at 02:08:05AM +, Al Viro wrote:
> On Wed, Dec 27, 2017 at 04:19:58PM +0800, Sean Fu wrote:
> > generic_file_read_iter has done the count test.
> > So ext4_file_read_iter don't need to test the count repeatedly.
> 
> Huh?  You do realize that generic_file_read_iter() is not the
> only variant possible there, right?
>
Yes, I know that generic_file_read_iter() is not the only variant possible.
The other possible dax_iomap_rw() with zero count would return zero.
> static ssize_t ext4_dax_read_iter(struct kiocb *iocb, struct iov_iter *to)
> {
> struct inode *inode = file_inode(iocb->ki_filp);
> ssize_t ret;
> 
> if (!inode_trylock_shared(inode)) {
> if (iocb->ki_flags & IOCB_NOWAIT)
> return -EAGAIN;
> inode_lock_shared(inode);
> }
> 
> ... and now IOCB_NOWAIT read with zero count can fail with -EAGAIN.
>
Correct, IOCB_NOWAIT read with zero count can return -EAGAIN, But I
think that it is reasonable. while it got lock, zero would be returned
in this case.

Re: [PATCH] ext4: Remove repeated test in ext4_file_read_iter.

[Al Viro]

On Wed, Dec 27, 2017 at 04:19:58PM +0800, Sean Fu wrote:
> generic_file_read_iter has done the count test.
> So ext4_file_read_iter don't need to test the count repeatedly.

Huh?  You do realize that generic_file_read_iter() is not the
only variant possible there, right?

static ssize_t ext4_dax_read_iter(struct kiocb *iocb, struct iov_iter *to)
{
struct inode *inode = file_inode(iocb->ki_filp);
ssize_t ret;

if (!inode_trylock_shared(inode)) {
if (iocb->ki_flags & IOCB_NOWAIT)
return -EAGAIN;
inode_lock_shared(inode);
}

... and now IOCB_NOWAIT read with zero count can fail with -EAGAIN.

Re: [PATCH] ext4: Remove repeated test in ext4_file_read_iter.

[Al Viro]

On Wed, Dec 27, 2017 at 04:19:58PM +0800, Sean Fu wrote:
> generic_file_read_iter has done the count test.
> So ext4_file_read_iter don't need to test the count repeatedly.

Huh?  You do realize that generic_file_read_iter() is not the
only variant possible there, right?

static ssize_t ext4_dax_read_iter(struct kiocb *iocb, struct iov_iter *to)
{
struct inode *inode = file_inode(iocb->ki_filp);
ssize_t ret;

if (!inode_trylock_shared(inode)) {
if (iocb->ki_flags & IOCB_NOWAIT)
return -EAGAIN;
inode_lock_shared(inode);
}

... and now IOCB_NOWAIT read with zero count can fail with -EAGAIN.

[PATCH] ext4: Remove repeated test in ext4_file_read_iter.

[Sean Fu]

generic_file_read_iter has done the count test.
So ext4_file_read_iter don't need to test the count repeatedly.

Signed-off-by: Sean Fu 
---
 fs/ext4/file.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index a0ae27b..87ca13e 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -67,9 +67,6 @@ static ssize_t ext4_file_read_iter(struct kiocb *iocb, struct 
iov_iter *to)
if 
(unlikely(ext4_forced_shutdown(EXT4_SB(file_inode(iocb->ki_filp)->i_sb
return -EIO;
 
-   if (!iov_iter_count(to))
-   return 0; /* skip atime */
-
 #ifdef CONFIG_FS_DAX
if (IS_DAX(file_inode(iocb->ki_filp)))
return ext4_dax_read_iter(iocb, to);
-- 
2.6.2

[PATCH] ext4: Remove repeated test in ext4_file_read_iter.

[Sean Fu]

generic_file_read_iter has done the count test.
So ext4_file_read_iter don't need to test the count repeatedly.

Signed-off-by: Sean Fu 
---
 fs/ext4/file.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index a0ae27b..87ca13e 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -67,9 +67,6 @@ static ssize_t ext4_file_read_iter(struct kiocb *iocb, struct 
iov_iter *to)
if 
(unlikely(ext4_forced_shutdown(EXT4_SB(file_inode(iocb->ki_filp)->i_sb
return -EIO;
 
-   if (!iov_iter_count(to))
-   return 0; /* skip atime */
-
 #ifdef CONFIG_FS_DAX
if (IS_DAX(file_inode(iocb->ki_filp)))
return ext4_dax_read_iter(iocb, to);
-- 
2.6.2