ceph, cifs, nfs, fuse: boolean and / or confusion

ceph, cifs, nfs, fuse: boolean and / or confusion

[roel]

The test not SEEK_CUR or not SEEK_SET always evaluates to true

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
 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);

Re: ceph, cifs, nfs, fuse: boolean and / or confusion

[Andrew Morton]

On Tue, 13 Dec 2011 00:06:36 +0100
roel <roel.kluin@gmail.com> wrote:

> The test not SEEK_CUR or not SEEK_SET always evaluates to true
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
>  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 ;)

Re: ceph, cifs, nfs, fuse: boolean and / or confusion

[Trond Myklebust]

On Mon, 2011-12-12 at 15:28 -0800, Andrew Morton wrote: 
> On Tue, 13 Dec 2011 00:06:36 +0100
> roel <roel.kluin@gmail.com> wrote:
> 
> > The test not SEEK_CUR or not SEEK_SET always evaluates to true
> > 
> > Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> > ---
> >  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
Trond.Myklebust@netapp.com
www.netapp.com

Re: ceph, cifs, nfs, fuse: boolean and / or confusion

[Trond Myklebust]

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
Trond.Myklebust@netapp.com
www.netapp.com

Re: ceph, cifs, nfs, fuse: boolean and / or confusion

[Andrew Morton]

On Mon, 12 Dec 2011 18:45:11 -0500
Trond Myklebust <Trond.Myklebust@netapp.com> 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?

Re: ceph, cifs, nfs, fuse: boolean and / or confusion

[Sage Weil]

On Mon, 12 Dec 2011, Andrew Morton wrote:
> On Mon, 12 Dec 2011 18:45:11 -0500
> Trond Myklebust <Trond.Myklebust@netapp.com> 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

Re: ceph, cifs, nfs, fuse: boolean and / or confusion

[Miklos Szeredi]

Sage Weil <sage@newdream.net> 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 <roel.kluin@gmail.com>
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 <mszeredi@suse.cz>
CC: stable@vger.kernel.org
CC: Josef Bacik <josef@redhat.com>
CC: Al Viro <viro@zeniv.linux.org.uk>
---
 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

Re: ceph, cifs, nfs, fuse: boolean and / or confusion

[Jeff Layton]

On Mon, 12 Dec 2011 18:39:13 -0500
Trond Myklebust <Trond.Myklebust@netapp.com> wrote:

> On Mon, 2011-12-12 at 15:28 -0800, Andrew Morton wrote: 
> > On Tue, 13 Dec 2011 00:06:36 +0100
> > roel <roel.kluin@gmail.com> wrote:
> > 
> > > The test not SEEK_CUR or not SEEK_SET always evaluates to true
> > > 
> > > Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> > > ---
> > >  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 <jlayton@redhat.com>

drivers/crypto/picoxcell_crypto.c: boolean and / or confusion

[Joe Perches]

On Tue, 2011-12-13 at 00:06 +0100, roel wrote:
> The test not [val1] or not [val2] always evaluates to true

Hey Jamie and Roel

Looking at drivers with:

$ grep -rP --include=*.[ch] "(\b[\w\[\]\>\._\-]+)\s*\!\=\s*[\w\[\]\>\._\-]+\s*\|\|\s*\1\s*\!\=" drivers

drivers/crypto/picoxcell_crypto.c:	if ((len != AES_KEYSIZE_128 || len != AES_KEYSIZE_256) &&
drivers/crypto/picoxcell_crypto.c:	} else if ((len != AES_KEYSIZE_128 || len != AES_KEYSIZE_256) &&

Most likely these should be && not ||.

drivers/media/video/s5p-fimc/fimc-capture.c: boolean and / or confusion

[Joe Perches]

On Tue, 2011-12-13 at 00:06 +0100, roel wrote:
> The test not [val1] or not [val2] always evaluates to true

Hello

Looking at drivers with:

$ grep -rP --include=*.[ch] "(\b[\w\[\]\>\._\-]+)\s*\!\=\s*[\w\[\]\>\._\-]+\s*\|\|\s*\1\s*\!\=" drivers

drivers/media/video/s5p-fimc/fimc-capture.c:		if (mf->width != tfmt->width || mf->width != tfmt->width) {
drivers/media/video/s5p-fimc/fimc-capture.c:			if (mf->width != tfmt->width || mf->width != tfmt->width)

Most likely these tests should be:
	if (mf->height != tfmt->height || mf->width != tfmt->width)

Re: drivers/media/video/s5p-fimc/fimc-capture.c: boolean and / or confusion

[Sylwester Nawrocki]

On 12/13/2011 12:55 AM, Joe Perches wrote:
> On Tue, 2011-12-13 at 00:06 +0100, roel wrote:
>> The test not [val1] or not [val2] always evaluates to true
> 
> Hello
> 
> Looking at drivers with:
> 
> $ grep -rP --include=*.[ch] "(\b[\w\[\]\>\._\-]+)\s*\!\=\s*[\w\[\]\>\._\-]+\s*\|\|\s*\1\s*\!\=" drivers
> 
> drivers/media/video/s5p-fimc/fimc-capture.c:		if (mf->width != tfmt->width || mf->width != tfmt->width) {
> drivers/media/video/s5p-fimc/fimc-capture.c:			if (mf->width != tfmt->width || mf->width != tfmt->width)
> 
> Most likely these tests should be:
> 	if (mf->height != tfmt->height || mf->width != tfmt->width)

Indeed. I'll prepare a patch fixing this.

-- 
Thank you,
Sylwester

Re: drivers/crypto/picoxcell_crypto.c: boolean and / or confusion

[Jamie Iles]

Hi Joe,

On Mon, Dec 12, 2011 at 03:44:53PM -0800, Joe Perches wrote:
> On Tue, 2011-12-13 at 00:06 +0100, roel wrote:
> > The test not [val1] or not [val2] always evaluates to true
> 
> Hey Jamie and Roel
> 
> Looking at drivers with:
> 
> $ grep -rP --include=*.[ch] "(\b[\w\[\]\>\._\-]+)\s*\!\=\s*[\w\[\]\>\._\-]+\s*\|\|\s*\1\s*\!\=" drivers
> 
> drivers/crypto/picoxcell_crypto.c:	if ((len != AES_KEYSIZE_128 || len != AES_KEYSIZE_256) &&
> drivers/crypto/picoxcell_crypto.c:	} else if ((len != AES_KEYSIZE_128 || len != AES_KEYSIZE_256) &&
> 
> Most likely these should be && not ||.

Yup, the original code was incorrect.  Patch to follow.  Thanks Joe!

Jamie

drivers/scsi/bfa/bfa_fcpim.c: boolean and / or confusion

[Joe Perches]

On Tue, 2011-12-13 at 00:06 +0100, roel wrote:
> The test not [val1] or not [val2] always evaluates to true

Looking at drivers with:

$ grep -rP --include=*.[ch] "(\b[\w\[\]\>\._\-]+)\s*\!\=\s*[\w\[\]\>\._\-]+\s*\)\s*\|\|\s*\(\s*\1\s*\!\=" drivers

drivers/scsi/bfa/bfa_fcpim.c:			    ((cdb->scsi_cdb[0] != INQUIRY) ||
			    (cdb->scsi_cdb[0] != REPORT_LUNS))) {

Likely the || should be &&

RE: drivers/scsi/bfa/bfa_fcpim.c: boolean and / or confusion

[Jing Huang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 635 bytes --]

>On Tue, 2011-12-13 at 00:06 +0100, roel wrote:
>> The test not [val1] or not [val2] always evaluates to true
>
>Looking at drivers with:
>
>$ grep -rP --include=*.[ch] "(\b[\w\[\]\>\._\-]+)\s*\!\=\s*[\w\[\]\>\._\-
>]+\s*\)\s*\|\|\s*\(\s*\1\s*\!\=" drivers
>
>drivers/scsi/bfa/bfa_fcpim.c:			    ((cdb->scsi_cdb[0] != INQUIRY) ||
>			    (cdb->scsi_cdb[0] != REPORT_LUNS))) {
>
>Likely the || should be &&

This is a bug and it should be &&. Will provide a fix.

Thanks
Jing
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

drivers/dma/sirf-dma.c: boolean and / or confusion

[Joe Perches]

On Tue, 2011-12-13 at 00:06 +0100, roel wrote:
> The test not [val1] or not [val2] always evaluates to true

Looking at drivers with:

$ grep -rP --include=*.[ch] "(\b[\w\[\]\>\._\-]+)\s*\!\=\s*[\w\[\]\>\._\-]+\s*\)\s*\|\|\s*\(\s*\1\s*\!\=" drivers

drivers/dma/sirf-dma.c:	if ((xt->dir != DMA_MEM_TO_DEV) || (xt->dir != DMA_DEV_TO_MEM)) {

Likely the || should be &&