Skip to contentSkip to conversation

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

PATCH roel 10 participants 15 messages Dec 12, 2011 – Dec 13, 2011

Subsystems (8)
SCSI NFS client NFS server Crypto API +4 more subsystems
lore
Share & tools
lore

roel

author
#0 lore raw

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(-)
fs/ceph/file.c +1 -1
index ce549d3..4d61a66 100644
@@ -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;
fs/cifs/cifsfs.c +1 -1
index 8f1fe32..f8351c2 100644
@@ -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;
fs/fuse/file.c +1 -1
index 594f07a..19029e9 100644
@@ -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;
fs/nfs/file.c +1 -1
index eca56d4..606ef0f 100644
@@ -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);
#1 lore raw
Quoted 54 lines · On Tue, 13 Dec 2011 00:06:36 +0100 roel <roel.kluin-Re5JQEeQqe8AvxtiuMwx3w@publ…

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(-)
fs/ceph/file.c +1 -1
index ce549d3..4d61a66 100644
@@ -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;
fs/cifs/cifsfs.c +1 -1
index 8f1fe32..f8351c2 100644
@@ -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;
fs/fuse/file.c +1 -1
index 594f07a..19029e9 100644
@@ -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;
fs/nfs/file.c +0 -0
index eca56d4..606ef0f 100644

fs/nfs/file.c static loff_t nfs_file_llseek(struct file *filp, loff_t offset, int origin) · line 153

Quoted 8 lines
fs/nfs/file.c +1 -1
@@ -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 ;)

#2 lore raw
Quoted 13 lines · Andrew Morton
Quoted 10 lines · On Tue, 13 Dec 2011 00:06:36 +0100 roel <roel.kluin-Re5JQEeQqe8AvxtiuMwx3w@publ…

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>

Quoted 12 lines
Quoted 12 lines
fs/nfs/file.c +1 -1
index eca56d4..606ef0f 100644
@@ -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;
Quoted 4 lines

fs/nfs/file.c static loff_t nfs_file_llseek(struct file *filp, loff_t offset, int origin) · line 153

Quoted 1 line
fs/nfs/file.c +0 -0
 		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

Signature · 6 lines
Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
www.netapp.com
#3 lore raw
Quoted 5 lines · Trond Myklebust

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...

Signature · 11 lines
Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
www.netapp.com

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#4 lore raw
Quoted 11 lines · On Mon, 12 Dec 2011 18:45:11 -0500 Trond Myklebust…
Quoted 5 lines · Trond Myklebust

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? --
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#5 lore raw
Quoted 15 lines · Andrew Morton
Quoted 11 lines · On Mon, 12 Dec 2011 18:45:11 -0500 Trond Myklebust…
Quoted 5 lines · Trond Myklebust

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...

Quoted 6 lines

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

maintainer
#6 lore raw
Quoted 3 lines · Sage Weil
Quoted 2 lines · Andrew Morton

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

----

Quoted 1 line

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 <viro-RmSDqhL/[email protected]>
---
 fs/fuse/file.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
fs/fuse/file.c +1 -1
index 594f07a..19029e9 100644
@@ -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;
Signature · 6 lines
1.7.7

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#7 lore raw
In reply to Trond Myklebust
Quoted 32 lines · On Mon, 12 Dec 2011 18:39:13 -0500 Trond Myklebust…
Quoted 13 lines · Andrew Morton
Quoted 10 lines · On Tue, 13 Dec 2011 00:06:36 +0100 roel <roel.kluin-Re5JQEeQqe8AvxtiuMwx3w@publ…

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>

Quoted 12 lines
Quoted 12 lines
fs/nfs/file.c +1 -1
index eca56d4..606ef0f 100644
@@ -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;
Quoted 4 lines

fs/nfs/file.c static loff_t nfs_file_llseek(struct file *filp, loff_t offset, int origin) · line 153

Quoted 1 line
fs/nfs/file.c +0 -0
 		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.

Quoted 8 lines

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.

Signature · 1 line
Jeff Layton <[email protected]>
#8 lore raw

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

In reply to roel
Quoted 1 line · roel

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 ||.

#9 lore raw

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

Quoted 1 line · roel

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)
#10 lore raw

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

Quoted 7 lines · Joe Perches
Quoted 1 line · roel

The test not [val1] or not [val2] always evaluates to true

Hello

Looking at drivers with:

Quoted 7 lines
$ 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.

Signature · 2 lines
Thank you,
Sylwester
#11 lore raw

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

In reply to Joe Perches

Hi Joe,

Quoted 5 lines · Joe Perches
Quoted 1 line · roel

The test not [val1] or not [val2] always evaluates to true

Hey Jamie and Roel

Quoted 8 lines

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

#12 lore raw

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

In reply to roel
Quoted 1 line · roel

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 &&

#13 lore raw

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

Quoted 5 lines · roel
Quoted 1 line

The test not [val1] or not [val2] always evaluates to true

Looking at drivers with:

Quoted 7 lines
$ 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

#14 lore raw

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

In reply to roel
Quoted 1 line · roel

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 &&