Q. NFS, return value of close(2)

Q. NFS, return value of close(2)

[J. R. Okajima]

By the commit af7fa16 "NFS: Fix up the fsync code", close(2) seems to
return the non-zero value even if it goes well, eg. the number of page via

nfs_file_flush()
vfs_fsync()
nfs_file_fsync()
nfs_commit_inode()
nfs_scan_commit()
nfs_scan_list()

Should nfs_file_fsync() return 0 when "status" is positive?

{
	status = nfs_commit_inode(inode, FLUSH_SYNC);
		;;;
-	if (!ret)
+	if (!ret && status < 0)
		ret = status;
}


J. R. Okajima

Re: Q. NFS, return value of close(2)

[Trond Myklebust]

On Thu, 2010-08-12 at 00:36 +0900, J. R. Okajima wrote:
> By the commit af7fa16 "NFS: Fix up the fsync code", close(2) seems to
> return the non-zero value even if it goes well, eg. the number of page via
> 
> nfs_file_flush()
> vfs_fsync()
> nfs_file_fsync()
> nfs_commit_inode()
> nfs_scan_commit()
> nfs_scan_list()
> 
> Should nfs_file_fsync() return 0 when "status" is positive?
> 
> {
> 	status = nfs_commit_inode(inode, FLUSH_SYNC);
> 		;;;
> -	if (!ret)
> +	if (!ret && status < 0)
> 		ret = status;
> }
> 
> 
> J. R. Okajima

Yes. That looks correct. Can you resend with an appropriate signed-off
line, etc.?

Cheers
  Trond

Re: Q. NFS, return value of close(2)

[J. R. Okajima]

Trond Myklebust:
> Yes. That looks correct. Can you resend with an appropriate signed-off
> line, etc.?

Sure.

----------------------------------------------------------------------
commit e56f956a08a6227004cd9b7747df6a089dbb3594
Author: J. R. Okajima <hooanon05@yahoo.co.jp>
Date:   Thu Aug 12 02:00:02 2010 +0900

    NFS: fix the return value of nfs_file_fsync()
    
    By the commit
    	af7fa16 2010-08-03 NFS: Fix up the fsync code
    close(2) became returning the non-zero value even if it went well.
    nfs_file_fsync() should return 0 when "status" is positive.
    
    Signed-off-by: J. R. Okajima <hooanon05@yahoo.co.jp>

diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 2d141a7..eb51bd6 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -323,7 +323,7 @@ nfs_file_fsync(struct file *file, int datasync)
 	have_error |= test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
 	if (have_error)
 		ret = xchg(&ctx->error, 0);
-	if (!ret)
+	if (!ret && status < 0)
 		ret = status;
 	return ret;
 }

Re: Q. NFS, return value of close(2)

[Trond Myklebust]

On Thu, 2010-08-12 at 02:04 +0900, J. R. Okajima wrote:
> Trond Myklebust:
> > Yes. That looks correct. Can you resend with an appropriate signed-off
> > line, etc.?
> 
> Sure.

Thank you! Applied...

  Trond

> ----------------------------------------------------------------------
> commit e56f956a08a6227004cd9b7747df6a089dbb3594
> Author: J. R. Okajima <hooanon05@yahoo.co.jp>
> Date:   Thu Aug 12 02:00:02 2010 +0900
> 
>     NFS: fix the return value of nfs_file_fsync()
>     
>     By the commit
>     	af7fa16 2010-08-03 NFS: Fix up the fsync code
>     close(2) became returning the non-zero value even if it went well.
>     nfs_file_fsync() should return 0 when "status" is positive.
>     
>     Signed-off-by: J. R. Okajima <hooanon05@yahoo.co.jp>
> 
> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> index 2d141a7..eb51bd6 100644
> --- a/fs/nfs/file.c
> +++ b/fs/nfs/file.c
> @@ -323,7 +323,7 @@ nfs_file_fsync(struct file *file, int datasync)
>  	have_error |= test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
>  	if (have_error)
>  		ret = xchg(&ctx->error, 0);
> -	if (!ret)
> +	if (!ret && status < 0)
>  		ret = status;
>  	return ret;
>  }

Re: Q. NFS, return value of close(2)

[Milan Broz]

On 08/11/2010 07:06 PM, Trond Myklebust wrote:
>> commit e56f956a08a6227004cd9b7747df6a089dbb3594
>> Author: J. R. Okajima <hooanon05@yahoo.co.jp>
>> Date:   Thu Aug 12 02:00:02 2010 +0900
>>
>>     NFS: fix the return value of nfs_file_fsync()
>>     
>>     By the commit
>>     	af7fa16 2010-08-03 NFS: Fix up the fsync code
>>     close(2) became returning the non-zero value even if it went well.
>>     nfs_file_fsync() should return 0 when "status" is positive.
>>     
>>     Signed-off-by: J. R. Okajima <hooanon05@yahoo.co.jp>

Hi,

please can you send this patch to Linus' tree?
NFS is currently broken, so it will save time for people trying
to bisect the problem.

It breaks even elementary operation on nfs mounts
# cat a > b
cat: write error

Patch above from nfs-git fixed the problem for me
http://git.linux-nfs.org/?p=trondmy/nfs-2.6.git;a=commitdiff;h=0702099bd86c33c2dcdbd3963433a61f3f503901

Thanks,
Milan
--
mbroz@redhat.com