Patchwork Lucid SRU, NFS: fix the return value of nfs_file_fsync()

login
register
mail settings
Submitter Tim Gardner
Date Feb. 11, 2011, 9:29 p.m.
Message ID <20110211212916.AAFBBF89F8@sepang.rtg.net>
Download mbox
Permalink /patch/82836/
State Accepted
Headers show

Pull-request

git://kernel.ubuntu.com/rtg/ubuntu-lucid.git nfs-file-fsync-lp585657

Comments

Tim Gardner - Feb. 11, 2011, 9:29 p.m.
The following changes since commit a1be943be789b54ed829eef43a13037a44558350:
  Bruce Rogers (1):
        virtio_net: Add schedule check to napi_enable call

are available in the git repository at:

  git://kernel.ubuntu.com/rtg/ubuntu-lucid.git nfs-file-fsync-lp585657

Tim Gardner (1):
      NFS: fix the return value of nfs_file_fsync()

 fs/nfs/file.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

From 1a20d7bade407d3fbb3f682c9879826971cdfdbb Mon Sep 17 00:00:00 2001
From: Tim Gardner <tim.gardner@canonical.com>
Date: Fri, 11 Feb 2011 11:04:37 -0700
Subject: [PATCH] NFS: fix the return value of nfs_file_fsync()

BugLink: http://bugs.launchpad.net/bugs/585657

Backport of 0702099bd86c33c2dcdbd3963433a61f3f503901

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>
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 fs/nfs/file.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Stefan Bader - Feb. 14, 2011, 10:47 a.m.
On 02/11/2011 10:29 PM, Tim Gardner wrote:
> 
> From 1a20d7bade407d3fbb3f682c9879826971cdfdbb Mon Sep 17 00:00:00 2001
> From: Tim Gardner <tim.gardner@canonical.com>
> Date: Fri, 11 Feb 2011 11:04:37 -0700
> Subject: [PATCH] NFS: fix the return value of nfs_file_fsync()
> 
> BugLink: http://bugs.launchpad.net/bugs/585657
> 
> Backport of 0702099bd86c33c2dcdbd3963433a61f3f503901
> 
> 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.
>

So if I understand this correctly, then it is a valid fix which has been
attributed to the wrong commit and by that may have missed the right upstream
stable branches. It looks to me that the offending commit was

commit 7b159fc18d417980f57aef64cab3417ee6af70f8
Author: Trond Myklebust <Trond.Myklebust@netapp.com>
Date:   Wed Jul 25 14:09:54 2007 -0400

    NFS: Fall back to synchronous writes when a background write errors...

and this happened around 2.6.24-rc1 which would in turn looks like the fix is
needed for all stable/longterm trees back to 2.6.24 (Hardy in our case).

It is not that simple to know all the effects. NFS internal calls seem to test
only for nfs_do_fsync <0 which would work in both cases. But the function is
used for the flush and fsync vfs operations and that could cause all sorts of
problems.

> Signed-off-by: J. R. Okajima <hooanon05@yahoo.co.jp>
> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>  fs/nfs/file.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> index 6fed6cc..99545e2 100644
> --- a/fs/nfs/file.c
> +++ b/fs/nfs/file.c
> @@ -220,7 +220,7 @@ static int nfs_do_fsync(struct nfs_open_context *ctx, struct inode *inode)
>  	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;
>  }
John Johansen - Feb. 14, 2011, 8:15 p.m.
On 02/14/2011 02:47 AM, Stefan Bader wrote:
> On 02/11/2011 10:29 PM, Tim Gardner wrote:
>>
>> From 1a20d7bade407d3fbb3f682c9879826971cdfdbb Mon Sep 17 00:00:00 2001
>> From: Tim Gardner <tim.gardner@canonical.com>
>> Date: Fri, 11 Feb 2011 11:04:37 -0700
>> Subject: [PATCH] NFS: fix the return value of nfs_file_fsync()
>>
>> BugLink: http://bugs.launchpad.net/bugs/585657
>>
>> Backport of 0702099bd86c33c2dcdbd3963433a61f3f503901
>>
>> 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.
>>
> 
> So if I understand this correctly, then it is a valid fix which has been
> attributed to the wrong commit and by that may have missed the right upstream
> stable branches. It looks to me that the offending commit was
> 
> commit 7b159fc18d417980f57aef64cab3417ee6af70f8
> Author: Trond Myklebust <Trond.Myklebust@netapp.com>
> Date:   Wed Jul 25 14:09:54 2007 -0400
> 
>     NFS: Fall back to synchronous writes when a background write errors...
> 
> and this happened around 2.6.24-rc1 which would in turn looks like the fix is
> needed for all stable/longterm trees back to 2.6.24 (Hardy in our case).
> 
> It is not that simple to know all the effects. NFS internal calls seem to test
> only for nfs_do_fsync <0 which would work in both cases. But the function is
> used for the flush and fsync vfs operations and that could cause all sorts of
> problems.

Hrmmm I have to agree it looks like we need this on hardy as well, but I want to
spend a little more time tracing all the paths

> 
>> Signed-off-by: J. R. Okajima <hooanon05@yahoo.co.jp>
>> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
>> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
> Acked-by: Stefan Bader <stefan.bader@canonical.com>
Acked-by: John Johansen <john.johansen@canonical.com>


>> ---
>>  fs/nfs/file.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
>> index 6fed6cc..99545e2 100644
>> --- a/fs/nfs/file.c
>> +++ b/fs/nfs/file.c
>> @@ -220,7 +220,7 @@ static int nfs_do_fsync(struct nfs_open_context *ctx, struct inode *inode)
>>  	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;
>>  }
> 
>
Tim Gardner - Feb. 14, 2011, 8:25 p.m.
applied and pushed
Tim Gardner - Feb. 14, 2011, 8:41 p.m.
On 02/14/2011 01:15 PM, John Johansen wrote:
> On 02/14/2011 02:47 AM, Stefan Bader wrote:
>> On 02/11/2011 10:29 PM, Tim Gardner wrote:
>>>
>>>  From 1a20d7bade407d3fbb3f682c9879826971cdfdbb Mon Sep 17 00:00:00 2001
>>> From: Tim Gardner<tim.gardner@canonical.com>
>>> Date: Fri, 11 Feb 2011 11:04:37 -0700
>>> Subject: [PATCH] NFS: fix the return value of nfs_file_fsync()
>>>
>>> BugLink: http://bugs.launchpad.net/bugs/585657
>>>
>>> Backport of 0702099bd86c33c2dcdbd3963433a61f3f503901
>>>
>>> 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.
>>>
>>
>> So if I understand this correctly, then it is a valid fix which has been
>> attributed to the wrong commit and by that may have missed the right upstream
>> stable branches. It looks to me that the offending commit was
>>
>> commit 7b159fc18d417980f57aef64cab3417ee6af70f8
>> Author: Trond Myklebust<Trond.Myklebust@netapp.com>
>> Date:   Wed Jul 25 14:09:54 2007 -0400
>>
>>      NFS: Fall back to synchronous writes when a background write errors...
>>
>> and this happened around 2.6.24-rc1 which would in turn looks like the fix is
>> needed for all stable/longterm trees back to 2.6.24 (Hardy in our case).
>>
>> It is not that simple to know all the effects. NFS internal calls seem to test
>> only for nfs_do_fsync<0 which would work in both cases. But the function is
>> used for the flush and fsync vfs operations and that could cause all sorts of
>> problems.
>
> Hrmmm I have to agree it looks like we need this on hardy as well, but I want to
> spend a little more time tracing all the paths
>

I've submitted a patch for Hardy. The semantics and call sites that 
exist in 2.6.24 appear to be identical to 2.6.32.

rtg