[patch] cifs: fix revalidation test in cifs_llseek()

[patch] cifs: fix revalidation test in cifs_llseek()

[Dan Carpenter]

This test is always true so it means we revalidate the length every
time, which generates more network traffic.  This was introduced in
06222e491e "fs: handle SEEK_HOLE/SEEK_DATA properly in all fs's that
define their own llseek".

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Josef, there were three other places that had this same problem but I
think they've all been fixed now.  Except that I had a question about
nfs_file_llseek().  Isn't that reversed?  It seems like it only
revalidates when it's not supposed to.  I chose to copy what
fuse_file_llseek() does instead.

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index d342128..97d26c7 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -695,7 +695,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;
 

Re: [patch] cifs: fix revalidation test in cifs_llseek()

[Pavel Shilovsky]

20 ÁĞÒÅÌÑ 2012šÇ. 1:06 ĞÏÌØÚÏ×ÁÔÅÌØ Dan Carpenter
<dan.carpenter@oracle.com> ÎÁĞÉÓÁÌ:
> This test is always true so it means we revalidate the length every
> time, which generates more network traffic. šThis was introduced in
> 06222e491e "fs: handle SEEK_HOLE/SEEK_DATA properly in all fs's that
> define their own llseek".
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Josef, there were three other places that had this same problem but I
> think they've all been fixed now. šExcept that I had a question about
> nfs_file_llseek(). šIsn't that reversed? šIt seems like it only
> revalidates when it's not supposed to. šI chose to copy what
> fuse_file_llseek() does instead.
>
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index d342128..97d26c7 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -695,7 +695,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;
>

In this case the semantic contradict the comment above. May be it
should be "if (origin != SEEK_SET && origin != SEEK_CUR)"?

-- 
Best regards,
Pavel Shilovsky.
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [patch] cifs: fix revalidation test in cifs_llseek()

[Pankaj Baranwal]

Could you please unscribe me from this list

-----Original Message-----
From: samba-technical-bounces@lists.samba.org [mailto:samba-technical-bounces@lists.samba.org] On Behalf Of Pavel Shilovsky
Sent: Friday, April 27, 2012 6:29 PM
To: Dan Carpenter
Cc: Steve French; linux-cifs@vger.kernel.org; kernel-janitors@vger.kernel.org; samba-technical@lists.samba.org; Josef Bacik
Subject: Re: [patch] cifs: fix revalidation test in cifs_llseek()

20 апреля 2012 г. 1:06 пользователь Dan Carpenter
<dan.carpenter@oracle.com> написал:
> This test is always true so it means we revalidate the length every
> time, which generates more network traffic.  This was introduced in
> 06222e491e "fs: handle SEEK_HOLE/SEEK_DATA properly in all fs's that
> define their own llseek".
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Josef, there were three other places that had this same problem but I
> think they've all been fixed now.  Except that I had a question about
> nfs_file_llseek().  Isn't that reversed?  It seems like it only
> revalidates when it's not supposed to.  I chose to copy what
> fuse_file_llseek() does instead.
>
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index d342128..97d26c7 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -695,7 +695,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;
>

In this case the semantic contradict the comment above. May be it
should be "if (origin != SEEK_SET && origin != SEEK_CUR)"?

--
Best regards,
Pavel Shilovsky.



::DISCLAIMER::
-----------------------------------------------------------------------------------------------------------------------

The contents of this e-mail and any attachment(s) are confidential and intended for the named recipient(s) only.
E-mail transmission cannot be guaranteed to be secure or error-free as information could be intercepted, corrupted,
lost, destroyed, arrive late or incomplete, or contain viruses.The e mail and its contents (with or without referred
errors) shall therefore not attach any liability on the originator or HCL or its affiliates. Any views or opinions
presented in this email are solely those of the author and may not necessarily reflect the opinions of HCL or its
affiliates. Any form of reproduction, dissemination, copying, disclosure, Modification, distribution and/or publication
of this message without the prior written consent of the author of this e-mail is strictly prohibited. If you have
received this email in error please delete it and notify the sender immediately. Before opening any mail and attachments
please check them for viruses and defect.

-----------------------------------------------------------------------------------------------------------------------
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] cifs: fix revalidation test in cifs_llseek()

[Jeff Layton]

On Fri, 27 Apr 2012 16:59:07 +0400
Pavel Shilovsky <piastryyy@gmail.com> wrote:

> 20 апреля 2012 г. 1:06 пользователь Dan Carpenter
> <dan.carpenter@oracle.com> написал:
> > This test is always true so it means we revalidate the length every
> > time, which generates more network traffic.  This was introduced in
> > 06222e491e "fs: handle SEEK_HOLE/SEEK_DATA properly in all fs's that
> > define their own llseek".
> >
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > Josef, there were three other places that had this same problem but I
> > think they've all been fixed now.  Except that I had a question about
> > nfs_file_llseek().  Isn't that reversed?  It seems like it only
> > revalidates when it's not supposed to.  I chose to copy what
> > fuse_file_llseek() does instead.
> >
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index d342128..97d26c7 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -695,7 +695,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;
> >
> 
> In this case the semantic contradict the comment above. May be it
> should be "if (origin != SEEK_SET && origin != SEEK_CUR)"?
> 

Agreed, I think Pavel is correct here. The stuff inside the if block
revalidates the file size, and we only need to do that if whence is not
SEEK_SET or SEEK_CUR.

-- 
Jeff Layton <jlayton@samba.org>

[patch v2] cifs: fix revalidation test in cifs_llseek()

[Dan Carpenter]

This test is always true so it means we revalidate the length every
time, which generates more network traffic.  When it is SEEK_SET or
SEEK_CUR, then we don't need to revalidate.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2:  I had the test reversed in the first version.

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 811245b..4a6ad20 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -699,7 +699,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;
 

Re: [patch v2] cifs: fix revalidation test in cifs_llseek()

[Jeff Layton]

On Mon, 30 Apr 2012 17:36:21 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:

> This test is always true so it means we revalidate the length every
> time, which generates more network traffic.  When it is SEEK_SET or
> SEEK_CUR, then we don't need to revalidate.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2:  I had the test reversed in the first version.
> 
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 811245b..4a6ad20 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -699,7 +699,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;
>  

Looks good.

Reviewed-by: Jeff Layton <jlayton@redhat.com>