| Message ID | 20110211212916.AAFBBF89F8@sepang.rtg.net |
|---|---|
| State | Accepted |
| Headers | show |
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; > }
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; >> } > >
applied and pushed
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
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(-)