The test not SEEK_CUR or not SEEK_SET always evaluates to true
Signed-off-by: Roel Kluin <[email protected]>
---
fs/ceph/file.c | 2 +-
fs/cifs/cifsfs.c | 2 +-
fs/fuse/file.c | 2 +-
fs/nfs/file.c | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index ce549d3..4d61a66 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -797,7 +797,7 @@ static loff_t ceph_llseek(struct file *file, loff_t offset, int origin)
mutex_lock(&inode->i_mutex);
__ceph_do_pending_vmtruncate(inode);
- if (origin != SEEK_CUR || origin != SEEK_SET) {
+ if (origin != SEEK_CUR && origin != SEEK_SET) {
ret = ceph_do_getattr(inode, CEPH_STAT_CAP_SIZE);
if (ret < 0) {
offset = ret;
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 8f1fe32..f8351c2 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -703,7 +703,7 @@ static loff_t cifs_llseek(struct file *file, loff_t offset, int origin)
* origin == SEEK_END || SEEK_DATA || SEEK_HOLE => we must revalidate
* the cached file length
*/
- if (origin != SEEK_SET || origin != SEEK_CUR) {
+ if (origin != SEEK_SET && origin != SEEK_CUR) {
int rc;
struct inode *inode = file->f_path.dentry->d_inode;
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 594f07a..19029e9 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1556,7 +1556,7 @@ static loff_t fuse_file_llseek(struct file *file, loff_t offset, int origin)
struct inode *inode = file->f_path.dentry->d_inode;
mutex_lock(&inode->i_mutex);
- if (origin != SEEK_CUR || origin != SEEK_SET) {
+ if (origin != SEEK_CUR && origin != SEEK_SET) {
retval = fuse_update_attributes(inode, NULL, file, NULL);
if (retval)
goto exit;
diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index eca56d4..606ef0f 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -147,7 +147,7 @@ static loff_t nfs_file_llseek(struct file *filp, loff_t offset, int origin)
* origin == SEEK_END || SEEK_DATA || SEEK_HOLE => we must revalidate
* the cached file length
*/
- if (origin != SEEK_SET || origin != SEEK_CUR) {
+ if (origin != SEEK_SET && origin != SEEK_CUR) {
struct inode *inode = filp->f_mapping->host;
int retval = nfs_revalidate_file_size(inode, filp);
On Mon, 2011-12-12 at 18:39 -0500, Trond Myklebust wrote:
> As far as NFS is concerned, it reverts a regression. NFS only used to
> run revalidate_file_size() for SEEK_END prior to commit
> 06222e491e663dac939f04b125c9dc52126a75c4. I accept that we now also need
> to run it for SEEK_HOLE and SEEK_DATA, but we've never had to do so for
> SEEK_SET and SEEK_CUR.
BTW: The regression exists in 3.1, so this is probably a stable
candidate...
--
Trond Myklebust
Linux NFS client maintainer
NetApp
[email protected]
http://www.netapp.com
On Mon, 12 Dec 2011, Andrew Morton wrote:
> On Mon, 12 Dec 2011 18:45:11 -0500
> Trond Myklebust <[email protected]> wrote:
>
> > On Mon, 2011-12-12 at 18:39 -0500, Trond Myklebust wrote:
> >
> > > As far as NFS is concerned, it reverts a regression. NFS only used to
> > > run revalidate_file_size() for SEEK_END prior to commit
> > > 06222e491e663dac939f04b125c9dc52126a75c4. I accept that we now also need
> > > to run it for SEEK_HOLE and SEEK_DATA, but we've never had to do so for
> > > SEEK_SET and SEEK_CUR.
> >
> > BTW: The regression exists in 3.1, so this is probably a stable
> > candidate...
> >
>
> I'd prefer not to merge this one unless all four fs maintainers have
> reviewed and preferably tested it. Also, in view of your earlier email
> the changelog for this patch needs updating for NFS and probably other
> filesystems.
>
> So please prepare and send the NFS patch, with a Reported-by:roel?
I'll push a fix for the ceph bit through my tree. Thanks, roel!
sage
Sage Weil <[email protected]> writes:
> On Mon, 12 Dec 2011, Andrew Morton wrote:
>>
>> So please prepare and send the NFS patch, with a Reported-by:roel?
Fuse patch follows. Also pushed to git tree:
git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git for-linus
Thanks Roel.
Miklos
----
>From b48c6af2086ab2ba8a9c9b6ce9ecb34592ce500c Mon Sep 17 00:00:00 2001
From: Roel Kluin <[email protected]>
Date: Tue, 13 Dec 2011 10:37:00 +0100
Subject: [PATCH] fuse: fix llseek bug
The test in fuse_file_llseek() "not SEEK_CUR or not SEEK_SET" always evaluates
to true.
This was introduced in 3.1 by commit 06222e49 (fs: handle SEEK_HOLE/SEEK_DATA
properly in all fs's that define their own llseek) and changed the behavior of
SEEK_CUR and SEEK_SET to always retrieve the file attributes. This is a
performance regression.
Fix the test so that it makes sense.
Signed-off-by: Miklos Szeredi <[email protected]>
CC: [email protected]
CC: Josef Bacik <[email protected]>
CC: Al Viro <[email protected]>
---
fs/fuse/file.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 594f07a..19029e9 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1556,7 +1556,7 @@ static loff_t fuse_file_llseek(struct file *file, loff_t offset, int origin)
struct inode *inode = file->f_path.dentry->d_inode;
mutex_lock(&inode->i_mutex);
- if (origin != SEEK_CUR || origin != SEEK_SET) {
+ if (origin != SEEK_CUR && origin != SEEK_SET) {
retval = fuse_update_attributes(inode, NULL, file, NULL);
if (retval)
goto exit;
--
1.7.7
On Mon, 2011-12-12 at 15:28 -0800, Andrew Morton wrote:
> On Tue, 13 Dec 2011 00:06:36 +0100
> roel <[email protected]> wrote:
>
> > The test not SEEK_CUR or not SEEK_SET always evaluates to true
> >
> > Signed-off-by: Roel Kluin <[email protected]>
> > ---
> > fs/ceph/file.c | 2 +-
> > fs/cifs/cifsfs.c | 2 +-
> > fs/fuse/file.c | 2 +-
> > fs/nfs/file.c | 2 +-
> > 4 files changed, 4 insertions(+), 4 deletions(-)
> >
<snip>
> > diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> > index eca56d4..606ef0f 100644
> > --- a/fs/nfs/file.c
> > +++ b/fs/nfs/file.c
> > @@ -147,7 +147,7 @@ static loff_t nfs_file_llseek(struct file *filp, loff_t offset, int origin)
> > * origin == SEEK_END || SEEK_DATA || SEEK_HOLE => we must revalidate
> > * the cached file length
> > */
> > - if (origin != SEEK_SET || origin != SEEK_CUR) {
> > + if (origin != SEEK_SET && origin != SEEK_CUR) {
> > struct inode *inode = filp->f_mapping->host;
> >
> > int retval = nfs_revalidate_file_size(inode, filp);
>
> This fix will cause changed runtime behaviour, such as NFS no longer
> running nfs_revalidate_file_size() for all seek modes.
As far as NFS is concerned, it reverts a regression. NFS only used to
run revalidate_file_size() for SEEK_END prior to commit
06222e491e663dac939f04b125c9dc52126a75c4. I accept that we now also need
to run it for SEEK_HOLE and SEEK_DATA, but we've never had to do so for
SEEK_SET and SEEK_CUR.
I suspect the same is true of ceph, cifs etc...
Cheers
Trond
--
Trond Myklebust
Linux NFS client maintainer
NetApp
[email protected]
http://www.netapp.com
On Mon, 12 Dec 2011 18:39:13 -0500
Trond Myklebust <[email protected]> wrote:
> On Mon, 2011-12-12 at 15:28 -0800, Andrew Morton wrote:
> > On Tue, 13 Dec 2011 00:06:36 +0100
> > roel <[email protected]> wrote:
> >
> > > The test not SEEK_CUR or not SEEK_SET always evaluates to true
> > >
> > > Signed-off-by: Roel Kluin <[email protected]>
> > > ---
> > > fs/ceph/file.c | 2 +-
> > > fs/cifs/cifsfs.c | 2 +-
> > > fs/fuse/file.c | 2 +-
> > > fs/nfs/file.c | 2 +-
> > > 4 files changed, 4 insertions(+), 4 deletions(-)
> > >
> <snip>
> > > diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> > > index eca56d4..606ef0f 100644
> > > --- a/fs/nfs/file.c
> > > +++ b/fs/nfs/file.c
> > > @@ -147,7 +147,7 @@ static loff_t nfs_file_llseek(struct file *filp, loff_t offset, int origin)
> > > * origin == SEEK_END || SEEK_DATA || SEEK_HOLE => we must revalidate
> > > * the cached file length
> > > */
> > > - if (origin != SEEK_SET || origin != SEEK_CUR) {
> > > + if (origin != SEEK_SET && origin != SEEK_CUR) {
> > > struct inode *inode = filp->f_mapping->host;
> > >
> > > int retval = nfs_revalidate_file_size(inode, filp);
> >
> > This fix will cause changed runtime behaviour, such as NFS no longer
> > running nfs_revalidate_file_size() for all seek modes.
>
> As far as NFS is concerned, it reverts a regression. NFS only used to
> run revalidate_file_size() for SEEK_END prior to commit
> 06222e491e663dac939f04b125c9dc52126a75c4. I accept that we now also need
> to run it for SEEK_HOLE and SEEK_DATA, but we've never had to do so for
> SEEK_SET and SEEK_CUR.
>
> I suspect the same is true of ceph, cifs etc...
>
(cc'ing Josef Bacik on this too, since he made this change originally)
Agreed. The patch looks correct to me for NFS and CIFS at at least.
--
Jeff Layton <[email protected]>
On Mon, 12 Dec 2011 18:45:11 -0500
Trond Myklebust <[email protected]> wrote:
> On Mon, 2011-12-12 at 18:39 -0500, Trond Myklebust wrote:
>
> > As far as NFS is concerned, it reverts a regression. NFS only used to
> > run revalidate_file_size() for SEEK_END prior to commit
> > 06222e491e663dac939f04b125c9dc52126a75c4. I accept that we now also need
> > to run it for SEEK_HOLE and SEEK_DATA, but we've never had to do so for
> > SEEK_SET and SEEK_CUR.
>
> BTW: The regression exists in 3.1, so this is probably a stable
> candidate...
>
I'd prefer not to merge this one unless all four fs maintainers have
reviewed and preferably tested it. Also, in view of your earlier email
the changelog for this patch needs updating for NFS and probably other
filesystems.
So please prepare and send the NFS patch, with a Reported-by:roel?
On Tue, 13 Dec 2011 00:06:36 +0100
roel <[email protected]> wrote:
> The test not SEEK_CUR or not SEEK_SET always evaluates to true
>
> Signed-off-by: Roel Kluin <[email protected]>
> ---
> fs/ceph/file.c | 2 +-
> fs/cifs/cifsfs.c | 2 +-
> fs/fuse/file.c | 2 +-
> fs/nfs/file.c | 2 +-
> 4 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> index ce549d3..4d61a66 100644
> --- a/fs/ceph/file.c
> +++ b/fs/ceph/file.c
> @@ -797,7 +797,7 @@ static loff_t ceph_llseek(struct file *file, loff_t offset, int origin)
>
> mutex_lock(&inode->i_mutex);
> __ceph_do_pending_vmtruncate(inode);
> - if (origin != SEEK_CUR || origin != SEEK_SET) {
> + if (origin != SEEK_CUR && origin != SEEK_SET) {
> ret = ceph_do_getattr(inode, CEPH_STAT_CAP_SIZE);
> if (ret < 0) {
> offset = ret;
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 8f1fe32..f8351c2 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -703,7 +703,7 @@ static loff_t cifs_llseek(struct file *file, loff_t offset, int origin)
> * origin == SEEK_END || SEEK_DATA || SEEK_HOLE => we must revalidate
> * the cached file length
> */
> - if (origin != SEEK_SET || origin != SEEK_CUR) {
> + if (origin != SEEK_SET && origin != SEEK_CUR) {
> int rc;
> struct inode *inode = file->f_path.dentry->d_inode;
>
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 594f07a..19029e9 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1556,7 +1556,7 @@ static loff_t fuse_file_llseek(struct file *file, loff_t offset, int origin)
> struct inode *inode = file->f_path.dentry->d_inode;
>
> mutex_lock(&inode->i_mutex);
> - if (origin != SEEK_CUR || origin != SEEK_SET) {
> + if (origin != SEEK_CUR && origin != SEEK_SET) {
> retval = fuse_update_attributes(inode, NULL, file, NULL);
> if (retval)
> goto exit;
> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> index eca56d4..606ef0f 100644
> --- a/fs/nfs/file.c
> +++ b/fs/nfs/file.c
> @@ -147,7 +147,7 @@ static loff_t nfs_file_llseek(struct file *filp, loff_t offset, int origin)
> * origin == SEEK_END || SEEK_DATA || SEEK_HOLE => we must revalidate
> * the cached file length
> */
> - if (origin != SEEK_SET || origin != SEEK_CUR) {
> + if (origin != SEEK_SET && origin != SEEK_CUR) {
> struct inode *inode = filp->f_mapping->host;
>
> int retval = nfs_revalidate_file_size(inode, filp);
This fix will cause changed runtime behaviour, such as NFS no longer
running nfs_revalidate_file_size() for all seek modes.
So I think it should be reviewed, tested and merged by the various fs
maintainers. Splitting it into four patches would help that process,
but they could independently treat it as a bug report, too.
Meanwhile, I'll put this in my tree so I can keep an eye on people ;)