[PATCH] cifs: Add information about noserverino

[PATCH] cifs: Add information about noserverino

[Bernhard Walle]

In my case I had the problem that 32 bit userspace applications in an
amd64 environment was not able to list the directories of a CIFS-mounted
share. The simple userspace code

    int main(int argc, char *argv[])
    {
        DIR *dir;
        struct dirent *dirent;

        if (!argv[1]) {
            fprintf(stderr, "Usage: %s <directory>\n", argv[0]);
            return -1;
        }

        dir = opendir(argv[1]);
        if (!dir) {
            perror("Unable to open directory");
            return -1;
        }

        while ((dirent = readdir(dir)) != NULL)
            puts(dirent->d_name);

        closedir(dir);

        return 0;
    }

was sufficient to trigger the problem.

I discovered that the problem was that the inodes were too large to fit
in a 32 bit (unsigned long) integer, so the compat_filldir() function
returned -EOVERFLOW.

While that is okay it would have saved me a some hours of debugging if
the message below would have appeared in my kernel log.

The target was a Samba server (I guess) of a Buffalo LinkStation Duo
with the unmodified vendor firmware 1.34.

Signed-off-by: Bernhard Walle <bernhard-X9USDgGjgfuzQB+pC5nmwQ@public.gmane.org>
---
 fs/cifs/readdir.c |   23 +++++++++++++++++++----
 1 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
index d5e591f..d979826 100644
--- a/fs/cifs/readdir.c
+++ b/fs/cifs/readdir.c
@@ -773,6 +773,7 @@ int cifs_readdir(struct file *file, void *direntry, filldir_t filldir)
 	char *tmp_buf = NULL;
 	char *end_of_smb;
 	unsigned int max_len;
+	int err;
 
 	xid = GetXid();
 
@@ -783,17 +784,31 @@ int cifs_readdir(struct file *file, void *direntry, filldir_t filldir)
 
 	switch ((int) file->f_pos) {
 	case 0:
-		if (filldir(direntry, ".", 1, file->f_pos,
-		     file->f_path.dentry->d_inode->i_ino, DT_DIR) < 0) {
+		err = filldir(direntry, ".", 1, file->f_pos,
+		     file->f_path.dentry->d_inode->i_ino, DT_DIR);
+		if (err < 0) {
 			cERROR(1, "Filldir for current dir failed");
+			if (err == -EOVERFLOW) {
+				cERROR(1, "Server inodes are too large for 32 "
+						"bit userspace. You might "
+						"consider using 'noserverino' "
+						"mount option for this mount.");
+			}
 			rc = -ENOMEM;
 			break;
 		}
 		file->f_pos++;
 	case 1:
-		if (filldir(direntry, "..", 2, file->f_pos,
-		     file->f_path.dentry->d_parent->d_inode->i_ino, DT_DIR) < 0) {
+		err = filldir(direntry, "..", 2, file->f_pos,
+		     file->f_path.dentry->d_parent->d_inode->i_ino, DT_DIR);
+		if (err < 0) {
 			cERROR(1, "Filldir for parent dir failed");
+			if (err == -EOVERFLOW) {
+				cERROR(1, "Server inodes are too large for 32 "
+						"bit userspace. You might "
+						"consider using 'noserverino' "
+						"mount option for this mount.");
+			}
 			rc = -ENOMEM;
 			break;
 		}
-- 
1.7.3.2

[PATCH] cifs: Add information about noserverino

[Bernhard Walle]

In my case I had the problem that 32 bit userspace applications in an
amd64 environment was not able to list the directories of a CIFS-mounted
share. The simple userspace code

    int main(int argc, char *argv[])
    {
        DIR *dir;
        struct dirent *dirent;

        if (!argv[1]) {
            fprintf(stderr, "Usage: %s <directory>\n", argv[0]);
            return -1;
        }

        dir = opendir(argv[1]);
        if (!dir) {
            perror("Unable to open directory");
            return -1;
        }

        while ((dirent = readdir(dir)) != NULL)
            puts(dirent->d_name);

        closedir(dir);

        return 0;
    }

was sufficient to trigger the problem.

I discovered that the problem was that the inodes were too large to fit
in a 32 bit (unsigned long) integer, so the compat_filldir() function
returned -EOVERFLOW.

While that is okay it would have saved me a some hours of debugging if
the message below would have appeared in my kernel log.

The target was a Samba server (I guess) of a Buffalo LinkStation Duo
with the unmodified vendor firmware 1.34.

Signed-off-by: Bernhard Walle <bernhard@bwalle.de>
---
 fs/cifs/readdir.c |   23 +++++++++++++++++++----
 1 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
index d5e591f..d979826 100644
--- a/fs/cifs/readdir.c
+++ b/fs/cifs/readdir.c
@@ -773,6 +773,7 @@ int cifs_readdir(struct file *file, void *direntry, filldir_t filldir)
 	char *tmp_buf = NULL;
 	char *end_of_smb;
 	unsigned int max_len;
+	int err;
 
 	xid = GetXid();
 
@@ -783,17 +784,31 @@ int cifs_readdir(struct file *file, void *direntry, filldir_t filldir)
 
 	switch ((int) file->f_pos) {
 	case 0:
-		if (filldir(direntry, ".", 1, file->f_pos,
-		     file->f_path.dentry->d_inode->i_ino, DT_DIR) < 0) {
+		err = filldir(direntry, ".", 1, file->f_pos,
+		     file->f_path.dentry->d_inode->i_ino, DT_DIR);
+		if (err < 0) {
 			cERROR(1, "Filldir for current dir failed");
+			if (err == -EOVERFLOW) {
+				cERROR(1, "Server inodes are too large for 32 "
+						"bit userspace. You might "
+						"consider using 'noserverino' "
+						"mount option for this mount.");
+			}
 			rc = -ENOMEM;
 			break;
 		}
 		file->f_pos++;
 	case 1:
-		if (filldir(direntry, "..", 2, file->f_pos,
-		     file->f_path.dentry->d_parent->d_inode->i_ino, DT_DIR) < 0) {
+		err = filldir(direntry, "..", 2, file->f_pos,
+		     file->f_path.dentry->d_parent->d_inode->i_ino, DT_DIR);
+		if (err < 0) {
 			cERROR(1, "Filldir for parent dir failed");
+			if (err == -EOVERFLOW) {
+				cERROR(1, "Server inodes are too large for 32 "
+						"bit userspace. You might "
+						"consider using 'noserverino' "
+						"mount option for this mount.");
+			}
 			rc = -ENOMEM;
 			break;
 		}
-- 
1.7.3.2

Re: [PATCH] cifs: Add information about noserverino

[Suresh Jayaraman]

On 12/05/2010 10:37 PM, Bernhard Walle wrote:
> In my case I had the problem that 32 bit userspace applications in an
> amd64 environment was not able to list the directories of a CIFS-mounted
> share. The simple userspace code
> 
>     int main(int argc, char *argv[])
>     {
>         DIR *dir;
>         struct dirent *dirent;
> 
>         if (!argv[1]) {
>             fprintf(stderr, "Usage: %s <directory>\n", argv[0]);
>             return -1;
>         }
> 
>         dir = opendir(argv[1]);
>         if (!dir) {
>             perror("Unable to open directory");
>             return -1;
>         }
> 
>         while ((dirent = readdir(dir)) != NULL)
>             puts(dirent->d_name);
> 
>         closedir(dir);
> 
>         return 0;
>     }
> 
> was sufficient to trigger the problem.
> 
> I discovered that the problem was that the inodes were too large to fit
> in a 32 bit (unsigned long) integer, so the compat_filldir() function
> returned -EOVERFLOW.
> 
> While that is okay it would have saved me a some hours of debugging if
> the message below would have appeared in my kernel log.
> 
> The target was a Samba server (I guess) of a Buffalo LinkStation Duo
> with the unmodified vendor firmware 1.34.
> 
> Signed-off-by: Bernhard Walle <bernhard-X9USDgGjgfuzQB+pC5nmwQ@public.gmane.org>
> ---
>  fs/cifs/readdir.c |   23 +++++++++++++++++++----
>  1 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
> index d5e591f..d979826 100644
> --- a/fs/cifs/readdir.c
> +++ b/fs/cifs/readdir.c
> @@ -773,6 +773,7 @@ int cifs_readdir(struct file *file, void *direntry, filldir_t filldir)
>  	char *tmp_buf = NULL;
>  	char *end_of_smb;
>  	unsigned int max_len;
> +	int err;
>  
>  	xid = GetXid();
>  
> @@ -783,17 +784,31 @@ int cifs_readdir(struct file *file, void *direntry, filldir_t filldir)
>  
>  	switch ((int) file->f_pos) {
>  	case 0:
> -		if (filldir(direntry, ".", 1, file->f_pos,
> -		     file->f_path.dentry->d_inode->i_ino, DT_DIR) < 0) {
> +		err = filldir(direntry, ".", 1, file->f_pos,
> +		     file->f_path.dentry->d_inode->i_ino, DT_DIR);
> +		if (err < 0) {
>  			cERROR(1, "Filldir for current dir failed");
> +			if (err == -EOVERFLOW) {
> +				cERROR(1, "Server inodes are too large for 32 "
> +						"bit userspace. You might "
> +						"consider using 'noserverino' "
> +						"mount option for this mount.");
> +			}

The patch looks good, however I don't see any reason why we need to
override the returned error with -ENOMEM below. I think we could
possibly use the variable 'rc' itself and set the error code as is.

>  			rc = -ENOMEM;
>  			break;
>  		}
>  		file->f_pos++;
>  	case 1:
> -		if (filldir(direntry, "..", 2, file->f_pos,
> -		     file->f_path.dentry->d_parent->d_inode->i_ino, DT_DIR) < 0) {
> +		err = filldir(direntry, "..", 2, file->f_pos,
> +		     file->f_path.dentry->d_parent->d_inode->i_ino, DT_DIR);
> +		if (err < 0) {
>  			cERROR(1, "Filldir for parent dir failed");
> +			if (err == -EOVERFLOW) {
> +				cERROR(1, "Server inodes are too large for 32 "
> +						"bit userspace. You might "
> +						"consider using 'noserverino' "
> +						"mount option for this mount.");
> +			}
>  			rc = -ENOMEM;
>  			break;
>  		}


-- 
Suresh Jayaraman

Re: [PATCH] cifs: Add information about noserverino

[Suresh Jayaraman]

On 12/05/2010 10:37 PM, Bernhard Walle wrote:
> In my case I had the problem that 32 bit userspace applications in an
> amd64 environment was not able to list the directories of a CIFS-mounted
> share. The simple userspace code
> 
>     int main(int argc, char *argv[])
>     {
>         DIR *dir;
>         struct dirent *dirent;
> 
>         if (!argv[1]) {
>             fprintf(stderr, "Usage: %s <directory>\n", argv[0]);
>             return -1;
>         }
> 
>         dir = opendir(argv[1]);
>         if (!dir) {
>             perror("Unable to open directory");
>             return -1;
>         }
> 
>         while ((dirent = readdir(dir)) != NULL)
>             puts(dirent->d_name);
> 
>         closedir(dir);
> 
>         return 0;
>     }
> 
> was sufficient to trigger the problem.
> 
> I discovered that the problem was that the inodes were too large to fit
> in a 32 bit (unsigned long) integer, so the compat_filldir() function
> returned -EOVERFLOW.
> 
> While that is okay it would have saved me a some hours of debugging if
> the message below would have appeared in my kernel log.
> 
> The target was a Samba server (I guess) of a Buffalo LinkStation Duo
> with the unmodified vendor firmware 1.34.
> 
> Signed-off-by: Bernhard Walle <bernhard@bwalle.de>
> ---
>  fs/cifs/readdir.c |   23 +++++++++++++++++++----
>  1 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
> index d5e591f..d979826 100644
> --- a/fs/cifs/readdir.c
> +++ b/fs/cifs/readdir.c
> @@ -773,6 +773,7 @@ int cifs_readdir(struct file *file, void *direntry, filldir_t filldir)
>  	char *tmp_buf = NULL;
>  	char *end_of_smb;
>  	unsigned int max_len;
> +	int err;
>  
>  	xid = GetXid();
>  
> @@ -783,17 +784,31 @@ int cifs_readdir(struct file *file, void *direntry, filldir_t filldir)
>  
>  	switch ((int) file->f_pos) {
>  	case 0:
> -		if (filldir(direntry, ".", 1, file->f_pos,
> -		     file->f_path.dentry->d_inode->i_ino, DT_DIR) < 0) {
> +		err = filldir(direntry, ".", 1, file->f_pos,
> +		     file->f_path.dentry->d_inode->i_ino, DT_DIR);
> +		if (err < 0) {
>  			cERROR(1, "Filldir for current dir failed");
> +			if (err == -EOVERFLOW) {
> +				cERROR(1, "Server inodes are too large for 32 "
> +						"bit userspace. You might "
> +						"consider using 'noserverino' "
> +						"mount option for this mount.");
> +			}

The patch looks good, however I don't see any reason why we need to
override the returned error with -ENOMEM below. I think we could
possibly use the variable 'rc' itself and set the error code as is.

>  			rc = -ENOMEM;
>  			break;
>  		}
>  		file->f_pos++;
>  	case 1:
> -		if (filldir(direntry, "..", 2, file->f_pos,
> -		     file->f_path.dentry->d_parent->d_inode->i_ino, DT_DIR) < 0) {
> +		err = filldir(direntry, "..", 2, file->f_pos,
> +		     file->f_path.dentry->d_parent->d_inode->i_ino, DT_DIR);
> +		if (err < 0) {
>  			cERROR(1, "Filldir for parent dir failed");
> +			if (err == -EOVERFLOW) {
> +				cERROR(1, "Server inodes are too large for 32 "
> +						"bit userspace. You might "
> +						"consider using 'noserverino' "
> +						"mount option for this mount.");
> +			}
>  			rc = -ENOMEM;
>  			break;
>  		}


-- 
Suresh Jayaraman

Re: [PATCH] cifs: Add information about noserverino

[Jeff Layton]

On Sun,  5 Dec 2010 18:07:35 +0100
Bernhard Walle <bernhard-X9USDgGjgfuzQB+pC5nmwQ@public.gmane.org> wrote:

> In my case I had the problem that 32 bit userspace applications in an
> amd64 environment was not able to list the directories of a CIFS-mounted
> share. The simple userspace code
> 
>     int main(int argc, char *argv[])
>     {
>         DIR *dir;
>         struct dirent *dirent;
> 
>         if (!argv[1]) {
>             fprintf(stderr, "Usage: %s <directory>\n", argv[0]);
>             return -1;
>         }
> 
>         dir = opendir(argv[1]);
>         if (!dir) {
>             perror("Unable to open directory");
>             return -1;
>         }
> 
>         while ((dirent = readdir(dir)) != NULL)
>             puts(dirent->d_name);
> 
>         closedir(dir);
> 
>         return 0;
>     }
> 
> was sufficient to trigger the problem.
> 
> I discovered that the problem was that the inodes were too large to fit
> in a 32 bit (unsigned long) integer, so the compat_filldir() function
> returned -EOVERFLOW.
> 
> While that is okay it would have saved me a some hours of debugging if
> the message below would have appeared in my kernel log.
> 
> The target was a Samba server (I guess) of a Buffalo LinkStation Duo
> with the unmodified vendor firmware 1.34.
> 
> Signed-off-by: Bernhard Walle <bernhard-X9USDgGjgfuzQB+pC5nmwQ@public.gmane.org>
> ---
>  fs/cifs/readdir.c |   23 +++++++++++++++++++----
>  1 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
> index d5e591f..d979826 100644
> --- a/fs/cifs/readdir.c
> +++ b/fs/cifs/readdir.c
> @@ -773,6 +773,7 @@ int cifs_readdir(struct file *file, void *direntry, filldir_t filldir)
>  	char *tmp_buf = NULL;
>  	char *end_of_smb;
>  	unsigned int max_len;
> +	int err;
>  
>  	xid = GetXid();
>  
> @@ -783,17 +784,31 @@ int cifs_readdir(struct file *file, void *direntry, filldir_t filldir)
>  
>  	switch ((int) file->f_pos) {
>  	case 0:
> -		if (filldir(direntry, ".", 1, file->f_pos,
> -		     file->f_path.dentry->d_inode->i_ino, DT_DIR) < 0) {
> +		err = filldir(direntry, ".", 1, file->f_pos,
> +		     file->f_path.dentry->d_inode->i_ino, DT_DIR);
> +		if (err < 0) {
>  			cERROR(1, "Filldir for current dir failed");
> +			if (err == -EOVERFLOW) {
> +				cERROR(1, "Server inodes are too large for 32 "
> +						"bit userspace. You might "
> +						"consider using 'noserverino' "
> +						"mount option for this mount.");
> +			}
>  			rc = -ENOMEM;
>  			break;
>  		}
>  		file->f_pos++;
>  	case 1:
> -		if (filldir(direntry, "..", 2, file->f_pos,
> -		     file->f_path.dentry->d_parent->d_inode->i_ino, DT_DIR) < 0) {
> +		err = filldir(direntry, "..", 2, file->f_pos,
> +		     file->f_path.dentry->d_parent->d_inode->i_ino, DT_DIR);
> +		if (err < 0) {
>  			cERROR(1, "Filldir for parent dir failed");
> +			if (err == -EOVERFLOW) {
> +				cERROR(1, "Server inodes are too large for 32 "
> +						"bit userspace. You might "
> +						"consider using 'noserverino' "
> +						"mount option for this mount.");
> +			}
>  			rc = -ENOMEM;
>  			break;
>  		}

This doesn't look right to me...

i_ino is an unsigned long. The code in filldir() is this:

        unsigned long d_ino;

[...]

        d_ino = ino;
        if (sizeof(d_ino) < sizeof(ino) && d_ino != ino) {
                buf->error = -EOVERFLOW;
                return -EOVERFLOW;
        }

...so the first condition will return true on a 32-bit arch, but it's
not clear to me how you'd get the second one to return true in this
situation.

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH] cifs: Add information about noserverino

[Jeff Layton]

On Sun,  5 Dec 2010 18:07:35 +0100
Bernhard Walle <bernhard@bwalle.de> wrote:

> In my case I had the problem that 32 bit userspace applications in an
> amd64 environment was not able to list the directories of a CIFS-mounted
> share. The simple userspace code
> 
>     int main(int argc, char *argv[])
>     {
>         DIR *dir;
>         struct dirent *dirent;
> 
>         if (!argv[1]) {
>             fprintf(stderr, "Usage: %s <directory>\n", argv[0]);
>             return -1;
>         }
> 
>         dir = opendir(argv[1]);
>         if (!dir) {
>             perror("Unable to open directory");
>             return -1;
>         }
> 
>         while ((dirent = readdir(dir)) != NULL)
>             puts(dirent->d_name);
> 
>         closedir(dir);
> 
>         return 0;
>     }
> 
> was sufficient to trigger the problem.
> 
> I discovered that the problem was that the inodes were too large to fit
> in a 32 bit (unsigned long) integer, so the compat_filldir() function
> returned -EOVERFLOW.
> 
> While that is okay it would have saved me a some hours of debugging if
> the message below would have appeared in my kernel log.
> 
> The target was a Samba server (I guess) of a Buffalo LinkStation Duo
> with the unmodified vendor firmware 1.34.
> 
> Signed-off-by: Bernhard Walle <bernhard@bwalle.de>
> ---
>  fs/cifs/readdir.c |   23 +++++++++++++++++++----
>  1 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
> index d5e591f..d979826 100644
> --- a/fs/cifs/readdir.c
> +++ b/fs/cifs/readdir.c
> @@ -773,6 +773,7 @@ int cifs_readdir(struct file *file, void *direntry, filldir_t filldir)
>  	char *tmp_buf = NULL;
>  	char *end_of_smb;
>  	unsigned int max_len;
> +	int err;
>  
>  	xid = GetXid();
>  
> @@ -783,17 +784,31 @@ int cifs_readdir(struct file *file, void *direntry, filldir_t filldir)
>  
>  	switch ((int) file->f_pos) {
>  	case 0:
> -		if (filldir(direntry, ".", 1, file->f_pos,
> -		     file->f_path.dentry->d_inode->i_ino, DT_DIR) < 0) {
> +		err = filldir(direntry, ".", 1, file->f_pos,
> +		     file->f_path.dentry->d_inode->i_ino, DT_DIR);
> +		if (err < 0) {
>  			cERROR(1, "Filldir for current dir failed");
> +			if (err == -EOVERFLOW) {
> +				cERROR(1, "Server inodes are too large for 32 "
> +						"bit userspace. You might "
> +						"consider using 'noserverino' "
> +						"mount option for this mount.");
> +			}
>  			rc = -ENOMEM;
>  			break;
>  		}
>  		file->f_pos++;
>  	case 1:
> -		if (filldir(direntry, "..", 2, file->f_pos,
> -		     file->f_path.dentry->d_parent->d_inode->i_ino, DT_DIR) < 0) {
> +		err = filldir(direntry, "..", 2, file->f_pos,
> +		     file->f_path.dentry->d_parent->d_inode->i_ino, DT_DIR);
> +		if (err < 0) {
>  			cERROR(1, "Filldir for parent dir failed");
> +			if (err == -EOVERFLOW) {
> +				cERROR(1, "Server inodes are too large for 32 "
> +						"bit userspace. You might "
> +						"consider using 'noserverino' "
> +						"mount option for this mount.");
> +			}
>  			rc = -ENOMEM;
>  			break;
>  		}

This doesn't look right to me...

i_ino is an unsigned long. The code in filldir() is this:

        unsigned long d_ino;

[...]

        d_ino = ino;
        if (sizeof(d_ino) < sizeof(ino) && d_ino != ino) {
                buf->error = -EOVERFLOW;
                return -EOVERFLOW;
        }

...so the first condition will return true on a 32-bit arch, but it's
not clear to me how you'd get the second one to return true in this
situation.

-- 
Jeff Layton <jlayton@samba.org>

Re: [PATCH] cifs: Add information about noserverino

[Bernhard Walle]

Hello,

Zitat von Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
>
> This doesn't look right to me...
>
> i_ino is an unsigned long. The code in filldir() is this:
>
>         unsigned long d_ino;

You need to look at compat_filldir():

         compat_ulong_t d_ino;


Regards,
Bernhard

Re: [PATCH] cifs: Add information about noserverino

[Bernhard Walle]

Hello,

Zitat von Jeff Layton <jlayton@samba.org>:
>
> This doesn't look right to me...
>
> i_ino is an unsigned long. The code in filldir() is this:
>
>         unsigned long d_ino;

You need to look at compat_filldir():

         compat_ulong_t d_ino;


Regards,
Bernhard

Re: [PATCH] cifs: Add information about noserverino

[Jeff Layton]

On Mon, 6 Dec 2010 09:57:25 -0500
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:

> On Sun,  5 Dec 2010 18:07:35 +0100
> Bernhard Walle <bernhard-X9USDgGjgfuzQB+pC5nmwQ@public.gmane.org> wrote:
> 
> > In my case I had the problem that 32 bit userspace applications in an
> > amd64 environment was not able to list the directories of a CIFS-mounted
> > share. The simple userspace code
> > 
> >     int main(int argc, char *argv[])
> >     {
> >         DIR *dir;
> >         struct dirent *dirent;
> > 
> >         if (!argv[1]) {
> >             fprintf(stderr, "Usage: %s <directory>\n", argv[0]);
> >             return -1;
> >         }
> > 
> >         dir = opendir(argv[1]);
> >         if (!dir) {
> >             perror("Unable to open directory");
> >             return -1;
> >         }
> > 
> >         while ((dirent = readdir(dir)) != NULL)
> >             puts(dirent->d_name);
> > 
> >         closedir(dir);
> > 
> >         return 0;
> >     }
> > 
> > was sufficient to trigger the problem.
> > 
> > I discovered that the problem was that the inodes were too large to fit
> > in a 32 bit (unsigned long) integer, so the compat_filldir() function
> > returned -EOVERFLOW.
> > 
> > While that is okay it would have saved me a some hours of debugging if
> > the message below would have appeared in my kernel log.
> > 
> > The target was a Samba server (I guess) of a Buffalo LinkStation Duo
> > with the unmodified vendor firmware 1.34.
> > 
> > Signed-off-by: Bernhard Walle <bernhard-X9USDgGjgfuzQB+pC5nmwQ@public.gmane.org>
> > ---
> >  fs/cifs/readdir.c |   23 +++++++++++++++++++----
> >  1 files changed, 19 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
> > index d5e591f..d979826 100644
> > --- a/fs/cifs/readdir.c
> > +++ b/fs/cifs/readdir.c
> > @@ -773,6 +773,7 @@ int cifs_readdir(struct file *file, void *direntry, filldir_t filldir)
> >  	char *tmp_buf = NULL;
> >  	char *end_of_smb;
> >  	unsigned int max_len;
> > +	int err;
> >  
> >  	xid = GetXid();
> >  
> > @@ -783,17 +784,31 @@ int cifs_readdir(struct file *file, void *direntry, filldir_t filldir)
> >  
> >  	switch ((int) file->f_pos) {
> >  	case 0:
> > -		if (filldir(direntry, ".", 1, file->f_pos,
> > -		     file->f_path.dentry->d_inode->i_ino, DT_DIR) < 0) {
> > +		err = filldir(direntry, ".", 1, file->f_pos,
> > +		     file->f_path.dentry->d_inode->i_ino, DT_DIR);
> > +		if (err < 0) {
> >  			cERROR(1, "Filldir for current dir failed");
> > +			if (err == -EOVERFLOW) {
> > +				cERROR(1, "Server inodes are too large for 32 "
> > +						"bit userspace. You might "
> > +						"consider using 'noserverino' "
> > +						"mount option for this mount.");
> > +			}
> >  			rc = -ENOMEM;
> >  			break;
> >  		}
> >  		file->f_pos++;
> >  	case 1:
> > -		if (filldir(direntry, "..", 2, file->f_pos,
> > -		     file->f_path.dentry->d_parent->d_inode->i_ino, DT_DIR) < 0) {
> > +		err = filldir(direntry, "..", 2, file->f_pos,
> > +		     file->f_path.dentry->d_parent->d_inode->i_ino, DT_DIR);
> > +		if (err < 0) {
> >  			cERROR(1, "Filldir for parent dir failed");
> > +			if (err == -EOVERFLOW) {
> > +				cERROR(1, "Server inodes are too large for 32 "
> > +						"bit userspace. You might "
> > +						"consider using 'noserverino' "
> > +						"mount option for this mount.");
> > +			}
> >  			rc = -ENOMEM;
> >  			break;
> >  		}
> 
> This doesn't look right to me...
> 
> i_ino is an unsigned long. The code in filldir() is this:
> 
>         unsigned long d_ino;
> 
> [...]
> 
>         d_ino = ino;
>         if (sizeof(d_ino) < sizeof(ino) && d_ino != ino) {
>                 buf->error = -EOVERFLOW;
>                 return -EOVERFLOW;
>         }
> 
> ...so the first condition will return true on a 32-bit arch, but it's
> not clear to me how you'd get the second one to return true in this
> situation.
> 

Oh.... *compat*_filldir. Ok, that makes more sense...

I'm still not sure I like this patch however. It potentially means a
lot of printk spam since these things have no ratelimiting. It also
doesn't tell me anything about which server might be giving me grief.

Maybe this should be turned into a cFYI?

The bottom line though is that running 32-bit applications that were
built without -D_FILE_OFFSET_BITS=64 on a 64-bit kernel is a very bad
idea. It would be nice to be able to alert users that things aren't
working the way they expect, but I'm not sure this is the right place
to do that.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH] cifs: Add information about noserverino

[Jeff Layton]

On Mon, 6 Dec 2010 09:57:25 -0500
Jeff Layton <jlayton@samba.org> wrote:

> On Sun,  5 Dec 2010 18:07:35 +0100
> Bernhard Walle <bernhard@bwalle.de> wrote:
> 
> > In my case I had the problem that 32 bit userspace applications in an
> > amd64 environment was not able to list the directories of a CIFS-mounted
> > share. The simple userspace code
> > 
> >     int main(int argc, char *argv[])
> >     {
> >         DIR *dir;
> >         struct dirent *dirent;
> > 
> >         if (!argv[1]) {
> >             fprintf(stderr, "Usage: %s <directory>\n", argv[0]);
> >             return -1;
> >         }
> > 
> >         dir = opendir(argv[1]);
> >         if (!dir) {
> >             perror("Unable to open directory");
> >             return -1;
> >         }
> > 
> >         while ((dirent = readdir(dir)) != NULL)
> >             puts(dirent->d_name);
> > 
> >         closedir(dir);
> > 
> >         return 0;
> >     }
> > 
> > was sufficient to trigger the problem.
> > 
> > I discovered that the problem was that the inodes were too large to fit
> > in a 32 bit (unsigned long) integer, so the compat_filldir() function
> > returned -EOVERFLOW.
> > 
> > While that is okay it would have saved me a some hours of debugging if
> > the message below would have appeared in my kernel log.
> > 
> > The target was a Samba server (I guess) of a Buffalo LinkStation Duo
> > with the unmodified vendor firmware 1.34.
> > 
> > Signed-off-by: Bernhard Walle <bernhard@bwalle.de>
> > ---
> >  fs/cifs/readdir.c |   23 +++++++++++++++++++----
> >  1 files changed, 19 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
> > index d5e591f..d979826 100644
> > --- a/fs/cifs/readdir.c
> > +++ b/fs/cifs/readdir.c
> > @@ -773,6 +773,7 @@ int cifs_readdir(struct file *file, void *direntry, filldir_t filldir)
> >  	char *tmp_buf = NULL;
> >  	char *end_of_smb;
> >  	unsigned int max_len;
> > +	int err;
> >  
> >  	xid = GetXid();
> >  
> > @@ -783,17 +784,31 @@ int cifs_readdir(struct file *file, void *direntry, filldir_t filldir)
> >  
> >  	switch ((int) file->f_pos) {
> >  	case 0:
> > -		if (filldir(direntry, ".", 1, file->f_pos,
> > -		     file->f_path.dentry->d_inode->i_ino, DT_DIR) < 0) {
> > +		err = filldir(direntry, ".", 1, file->f_pos,
> > +		     file->f_path.dentry->d_inode->i_ino, DT_DIR);
> > +		if (err < 0) {
> >  			cERROR(1, "Filldir for current dir failed");
> > +			if (err == -EOVERFLOW) {
> > +				cERROR(1, "Server inodes are too large for 32 "
> > +						"bit userspace. You might "
> > +						"consider using 'noserverino' "
> > +						"mount option for this mount.");
> > +			}
> >  			rc = -ENOMEM;
> >  			break;
> >  		}
> >  		file->f_pos++;
> >  	case 1:
> > -		if (filldir(direntry, "..", 2, file->f_pos,
> > -		     file->f_path.dentry->d_parent->d_inode->i_ino, DT_DIR) < 0) {
> > +		err = filldir(direntry, "..", 2, file->f_pos,
> > +		     file->f_path.dentry->d_parent->d_inode->i_ino, DT_DIR);
> > +		if (err < 0) {
> >  			cERROR(1, "Filldir for parent dir failed");
> > +			if (err == -EOVERFLOW) {
> > +				cERROR(1, "Server inodes are too large for 32 "
> > +						"bit userspace. You might "
> > +						"consider using 'noserverino' "
> > +						"mount option for this mount.");
> > +			}
> >  			rc = -ENOMEM;
> >  			break;
> >  		}
> 
> This doesn't look right to me...
> 
> i_ino is an unsigned long. The code in filldir() is this:
> 
>         unsigned long d_ino;
> 
> [...]
> 
>         d_ino = ino;
>         if (sizeof(d_ino) < sizeof(ino) && d_ino != ino) {
>                 buf->error = -EOVERFLOW;
>                 return -EOVERFLOW;
>         }
> 
> ...so the first condition will return true on a 32-bit arch, but it's
> not clear to me how you'd get the second one to return true in this
> situation.
> 

Oh.... *compat*_filldir. Ok, that makes more sense...

I'm still not sure I like this patch however. It potentially means a
lot of printk spam since these things have no ratelimiting. It also
doesn't tell me anything about which server might be giving me grief.

Maybe this should be turned into a cFYI?

The bottom line though is that running 32-bit applications that were
built without -D_FILE_OFFSET_BITS=64 on a 64-bit kernel is a very bad
idea. It would be nice to be able to alert users that things aren't
working the way they expect, but I'm not sure this is the right place
to do that.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] cifs: Add information about noserverino

[Bernhard Walle]

Zitat von Jeff Layton <jlayton@redhat.com>:

>
> I'm still not sure I like this patch however. It potentially means a
> lot of printk spam since these things have no ratelimiting. It also
> doesn't tell me anything about which server might be giving me grief.
>
> Maybe this should be turned into a cFYI?

Well, if I see it in the kernel log, it doesn't matter if it's info or
something else.

> The bottom line though is that running 32-bit applications that were
> built without -D_FILE_OFFSET_BITS=64 on a 64-bit kernel is a very bad
> idea. It would be nice to be able to alert users that things aren't
> working the way they expect, but I'm not sure this is the right place
> to do that.

Well, but there *are* such application (in my case it was Softmaker Office
which is a proprietary word processor) and it's quite nice if you know
how you can workaround it when you encounter such a problem. That's all.


Regards,
Bernhard

Re: [PATCH] cifs: Add information about noserverino

[Bernhard Walle]

Zitat von Jeff Layton <jlayton@redhat.com>:

>
> I'm still not sure I like this patch however. It potentially means a
> lot of printk spam since these things have no ratelimiting. It also
> doesn't tell me anything about which server might be giving me grief.
>
> Maybe this should be turned into a cFYI?

Well, if I see it in the kernel log, it doesn't matter if it's info or
something else.

> The bottom line though is that running 32-bit applications that were
> built without -D_FILE_OFFSET_BITS=64 on a 64-bit kernel is a very bad
> idea. It would be nice to be able to alert users that things aren't
> working the way they expect, but I'm not sure this is the right place
> to do that.

Well, but there *are* such application (in my case it was Softmaker Office
which is a proprietary word processor) and it's quite nice if you know
how you can workaround it when you encounter such a problem. That's all.


Regards,
Bernhard

Re: [PATCH] cifs: Add information about noserverino

[Jeff Layton]

On Mon, 06 Dec 2010 16:35:06 +0100
Bernhard Walle <bernhard-X9USDgGjgfuzQB+pC5nmwQ@public.gmane.org> wrote:

> 
> Zitat von Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> 
> >
> > I'm still not sure I like this patch however. It potentially means a
> > lot of printk spam since these things have no ratelimiting. It also
> > doesn't tell me anything about which server might be giving me grief.
> >
> > Maybe this should be turned into a cFYI?
> 
> Well, if I see it in the kernel log, it doesn't matter if it's info or
> something else.
> 
> > The bottom line though is that running 32-bit applications that were
> > built without -D_FILE_OFFSET_BITS=64 on a 64-bit kernel is a very bad
> > idea. It would be nice to be able to alert users that things aren't
> > working the way they expect, but I'm not sure this is the right place
> > to do that.
> 
> Well, but there *are* such application (in my case it was Softmaker Office
> which is a proprietary word processor) and it's quite nice if you know
> how you can workaround it when you encounter such a problem. That's all.
> 

Sure...but this problem is not limited to CIFS. Many modern filesystems
use 64-bit inodes. Running this application on XFS or NFS for instance
is likely to give you the same trouble. You just hit it on CIFS because
the server happened to give you a very large inode number.

If we're going to add printk's for this situation, it probably ought to
be in a more generic place.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH] cifs: Add information about noserverino

[Jeff Layton]

On Mon, 06 Dec 2010 16:35:06 +0100
Bernhard Walle <bernhard@bwalle.de> wrote:

> 
> Zitat von Jeff Layton <jlayton@redhat.com>:
> 
> >
> > I'm still not sure I like this patch however. It potentially means a
> > lot of printk spam since these things have no ratelimiting. It also
> > doesn't tell me anything about which server might be giving me grief.
> >
> > Maybe this should be turned into a cFYI?
> 
> Well, if I see it in the kernel log, it doesn't matter if it's info or
> something else.
> 
> > The bottom line though is that running 32-bit applications that were
> > built without -D_FILE_OFFSET_BITS=64 on a 64-bit kernel is a very bad
> > idea. It would be nice to be able to alert users that things aren't
> > working the way they expect, but I'm not sure this is the right place
> > to do that.
> 
> Well, but there *are* such application (in my case it was Softmaker Office
> which is a proprietary word processor) and it's quite nice if you know
> how you can workaround it when you encounter such a problem. That's all.
> 

Sure...but this problem is not limited to CIFS. Many modern filesystems
use 64-bit inodes. Running this application on XFS or NFS for instance
is likely to give you the same trouble. You just hit it on CIFS because
the server happened to give you a very large inode number.

If we're going to add printk's for this situation, it probably ought to
be in a more generic place.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] cifs: Add information about noserverino

[Suresh Jayaraman]

On 12/06/2010 09:08 PM, Jeff Layton wrote:
> On Mon, 06 Dec 2010 16:35:06 +0100
> Bernhard Walle <bernhard-X9USDgGjgfuzQB+pC5nmwQ@public.gmane.org> wrote:
> 
>>
>> Zitat von Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>>
>>>
>>> I'm still not sure I like this patch however. It potentially means a
>>> lot of printk spam since these things have no ratelimiting. It also
>>> doesn't tell me anything about which server might be giving me grief.
>>>
>>> Maybe this should be turned into a cFYI?
>>
>> Well, if I see it in the kernel log, it doesn't matter if it's info or
>> something else.
>>
>>> The bottom line though is that running 32-bit applications that were
>>> built without -D_FILE_OFFSET_BITS=64 on a 64-bit kernel is a very bad
>>> idea. It would be nice to be able to alert users that things aren't
>>> working the way they expect, but I'm not sure this is the right place
>>> to do that.
>>
>> Well, but there *are* such application (in my case it was Softmaker Office
>> which is a proprietary word processor) and it's quite nice if you know
>> how you can workaround it when you encounter such a problem. That's all.
>>
> 
> Sure...but this problem is not limited to CIFS. Many modern filesystems
> use 64-bit inodes. Running this application on XFS or NFS for instance
> is likely to give you the same trouble. You just hit it on CIFS because
> the server happened to give you a very large inode number.
> 
> If we're going to add printk's for this situation, it probably ought to
> be in a more generic place.
> 

By generic place, did you mean at the VFS level? I think at VFS level,
there is little information about the Server or underlying fs and this
information doesn't seem too critical that VFS should warn/care much about.

May be sticking to a cFYI along with Server detail is a good idea?


-- 
Suresh Jayaraman

Re: [PATCH] cifs: Add information about noserverino

[Suresh Jayaraman]

On 12/06/2010 09:08 PM, Jeff Layton wrote:
> On Mon, 06 Dec 2010 16:35:06 +0100
> Bernhard Walle <bernhard@bwalle.de> wrote:
> 
>>
>> Zitat von Jeff Layton <jlayton@redhat.com>:
>>
>>>
>>> I'm still not sure I like this patch however. It potentially means a
>>> lot of printk spam since these things have no ratelimiting. It also
>>> doesn't tell me anything about which server might be giving me grief.
>>>
>>> Maybe this should be turned into a cFYI?
>>
>> Well, if I see it in the kernel log, it doesn't matter if it's info or
>> something else.
>>
>>> The bottom line though is that running 32-bit applications that were
>>> built without -D_FILE_OFFSET_BITS=64 on a 64-bit kernel is a very bad
>>> idea. It would be nice to be able to alert users that things aren't
>>> working the way they expect, but I'm not sure this is the right place
>>> to do that.
>>
>> Well, but there *are* such application (in my case it was Softmaker Office
>> which is a proprietary word processor) and it's quite nice if you know
>> how you can workaround it when you encounter such a problem. That's all.
>>
> 
> Sure...but this problem is not limited to CIFS. Many modern filesystems
> use 64-bit inodes. Running this application on XFS or NFS for instance
> is likely to give you the same trouble. You just hit it on CIFS because
> the server happened to give you a very large inode number.
> 
> If we're going to add printk's for this situation, it probably ought to
> be in a more generic place.
> 

By generic place, did you mean at the VFS level? I think at VFS level,
there is little information about the Server or underlying fs and this
information doesn't seem too critical that VFS should warn/care much about.

May be sticking to a cFYI along with Server detail is a good idea?


-- 
Suresh Jayaraman

Re: [PATCH] cifs: Add information about noserverino

[Jeff Layton]

On Thu, 09 Dec 2010 17:10:28 +0530
Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:

> On 12/06/2010 09:08 PM, Jeff Layton wrote:
> > On Mon, 06 Dec 2010 16:35:06 +0100
> > Bernhard Walle <bernhard-X9USDgGjgfuzQB+pC5nmwQ@public.gmane.org> wrote:
> > 
> >>
> >> Zitat von Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> >>
> >>>
> >>> I'm still not sure I like this patch however. It potentially means a
> >>> lot of printk spam since these things have no ratelimiting. It also
> >>> doesn't tell me anything about which server might be giving me grief.
> >>>
> >>> Maybe this should be turned into a cFYI?
> >>
> >> Well, if I see it in the kernel log, it doesn't matter if it's info or
> >> something else.
> >>
> >>> The bottom line though is that running 32-bit applications that were
> >>> built without -D_FILE_OFFSET_BITS=64 on a 64-bit kernel is a very bad
> >>> idea. It would be nice to be able to alert users that things aren't
> >>> working the way they expect, but I'm not sure this is the right place
> >>> to do that.
> >>
> >> Well, but there *are* such application (in my case it was Softmaker Office
> >> which is a proprietary word processor) and it's quite nice if you know
> >> how you can workaround it when you encounter such a problem. That's all.
> >>
> > 
> > Sure...but this problem is not limited to CIFS. Many modern filesystems
> > use 64-bit inodes. Running this application on XFS or NFS for instance
> > is likely to give you the same trouble. You just hit it on CIFS because
> > the server happened to give you a very large inode number.
> > 
> > If we're going to add printk's for this situation, it probably ought to
> > be in a more generic place.
> > 
> 
> By generic place, did you mean at the VFS level? I think at VFS level,
> there is little information about the Server or underlying fs and this
> information doesn't seem too critical that VFS should warn/care much about.
> 
> May be sticking to a cFYI along with Server detail is a good idea?
> 
My poing was mainly that there's nothing special about CIFS in this
regard, other than the fact that servers regularly send us inodes that
are larger than 2^32. Why should we do this for cifs but not for nfs,
xfs, ext4, etc?

The filldir function gets a dentry as an argument, so it could
reasonably generate a printk for this. I'm also not keen on
the printk recommending noserverino for this. That has its own
drawbacks.

A cFYI for this sort of thing seems reasonable however.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH] cifs: Add information about noserverino

[Jeff Layton]

On Thu, 09 Dec 2010 17:10:28 +0530
Suresh Jayaraman <sjayaraman@suse.de> wrote:

> On 12/06/2010 09:08 PM, Jeff Layton wrote:
> > On Mon, 06 Dec 2010 16:35:06 +0100
> > Bernhard Walle <bernhard@bwalle.de> wrote:
> > 
> >>
> >> Zitat von Jeff Layton <jlayton@redhat.com>:
> >>
> >>>
> >>> I'm still not sure I like this patch however. It potentially means a
> >>> lot of printk spam since these things have no ratelimiting. It also
> >>> doesn't tell me anything about which server might be giving me grief.
> >>>
> >>> Maybe this should be turned into a cFYI?
> >>
> >> Well, if I see it in the kernel log, it doesn't matter if it's info or
> >> something else.
> >>
> >>> The bottom line though is that running 32-bit applications that were
> >>> built without -D_FILE_OFFSET_BITS=64 on a 64-bit kernel is a very bad
> >>> idea. It would be nice to be able to alert users that things aren't
> >>> working the way they expect, but I'm not sure this is the right place
> >>> to do that.
> >>
> >> Well, but there *are* such application (in my case it was Softmaker Office
> >> which is a proprietary word processor) and it's quite nice if you know
> >> how you can workaround it when you encounter such a problem. That's all.
> >>
> > 
> > Sure...but this problem is not limited to CIFS. Many modern filesystems
> > use 64-bit inodes. Running this application on XFS or NFS for instance
> > is likely to give you the same trouble. You just hit it on CIFS because
> > the server happened to give you a very large inode number.
> > 
> > If we're going to add printk's for this situation, it probably ought to
> > be in a more generic place.
> > 
> 
> By generic place, did you mean at the VFS level? I think at VFS level,
> there is little information about the Server or underlying fs and this
> information doesn't seem too critical that VFS should warn/care much about.
> 
> May be sticking to a cFYI along with Server detail is a good idea?
> 
My poing was mainly that there's nothing special about CIFS in this
regard, other than the fact that servers regularly send us inodes that
are larger than 2^32. Why should we do this for cifs but not for nfs,
xfs, ext4, etc?

The filldir function gets a dentry as an argument, so it could
reasonably generate a printk for this. I'm also not keen on
the printk recommending noserverino for this. That has its own
drawbacks.

A cFYI for this sort of thing seems reasonable however.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] cifs: Add information about noserverino

[Steve French]

On Thu, Dec 9, 2010 at 6:09 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Thu, 09 Dec 2010 17:10:28 +0530
> Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
>
>> On 12/06/2010 09:08 PM, Jeff Layton wrote:
>> > On Mon, 06 Dec 2010 16:35:06 +0100
>> > Bernhard Walle <bernhard-X9USDgGjgfuzQB+pC5nmwQ@public.gmane.org> wrote:
>> >
>> >>
>> >> Zitat von Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>> >>
>> >>>
>> >>> I'm still not sure I like this patch however. It potentially means a
>> >>> lot of printk spam since these things have no ratelimiting. It also
>> >>> doesn't tell me anything about which server might be giving me grief.
>> >>>
>> >>> Maybe this should be turned into a cFYI?
>> >>
>> >> Well, if I see it in the kernel log, it doesn't matter if it's info or
>> >> something else.
>> >>
>> >>> The bottom line though is that running 32-bit applications that were
>> >>> built without -D_FILE_OFFSET_BITS=64 on a 64-bit kernel is a very bad
>> >>> idea. It would be nice to be able to alert users that things aren't
>> >>> working the way they expect, but I'm not sure this is the right place
>> >>> to do that.
>> >>
>> >> Well, but there *are* such application (in my case it was Softmaker Office
>> >> which is a proprietary word processor) and it's quite nice if you know
>> >> how you can workaround it when you encounter such a problem. That's all.
>> >>
>> >
>> > Sure...but this problem is not limited to CIFS. Many modern filesystems
>> > use 64-bit inodes. Running this application on XFS or NFS for instance
>> > is likely to give you the same trouble. You just hit it on CIFS because
>> > the server happened to give you a very large inode number.
>> >
>> > If we're going to add printk's for this situation, it probably ought to
>> > be in a more generic place.
>> >
>>
>> By generic place, did you mean at the VFS level? I think at VFS level,
>> there is little information about the Server or underlying fs and this
>> information doesn't seem too critical that VFS should warn/care much about.
>>
>> May be sticking to a cFYI along with Server detail is a good idea?
>>
> My poing was mainly that there's nothing special about CIFS in this
> regard, other than the fact that servers regularly send us inodes that
> are larger than 2^32. Why should we do this for cifs but not for nfs,
> xfs, ext4, etc?
>
> The filldir function gets a dentry as an argument, so it could
> reasonably generate a printk for this. I'm also not keen on
> the printk recommending noserverino for this. That has its own
> drawbacks.
>
> A cFYI for this sort of thing seems reasonable however.

I agree that a cFYI is reasonable.  The next obvious question is: do
we need to add code to generate unique 32 bit inode numbers
that don't collide (as IIRC Samba does by xor the high and low 32
bits of the inode number) when the app can't support ino64
I would prefer not to go back to noserverino since that has worse
drawbacks.



-- 
Thanks,

Steve

Re: [PATCH] cifs: Add information about noserverino

[Steve French]

On Thu, Dec 9, 2010 at 6:09 AM, Jeff Layton <jlayton@redhat.com> wrote:
> On Thu, 09 Dec 2010 17:10:28 +0530
> Suresh Jayaraman <sjayaraman@suse.de> wrote:
>
>> On 12/06/2010 09:08 PM, Jeff Layton wrote:
>> > On Mon, 06 Dec 2010 16:35:06 +0100
>> > Bernhard Walle <bernhard@bwalle.de> wrote:
>> >
>> >>
>> >> Zitat von Jeff Layton <jlayton@redhat.com>:
>> >>
>> >>>
>> >>> I'm still not sure I like this patch however. It potentially means a
>> >>> lot of printk spam since these things have no ratelimiting. It also
>> >>> doesn't tell me anything about which server might be giving me grief.
>> >>>
>> >>> Maybe this should be turned into a cFYI?
>> >>
>> >> Well, if I see it in the kernel log, it doesn't matter if it's info or
>> >> something else.
>> >>
>> >>> The bottom line though is that running 32-bit applications that were
>> >>> built without -D_FILE_OFFSET_BITS=64 on a 64-bit kernel is a very bad
>> >>> idea. It would be nice to be able to alert users that things aren't
>> >>> working the way they expect, but I'm not sure this is the right place
>> >>> to do that.
>> >>
>> >> Well, but there *are* such application (in my case it was Softmaker Office
>> >> which is a proprietary word processor) and it's quite nice if you know
>> >> how you can workaround it when you encounter such a problem. That's all.
>> >>
>> >
>> > Sure...but this problem is not limited to CIFS. Many modern filesystems
>> > use 64-bit inodes. Running this application on XFS or NFS for instance
>> > is likely to give you the same trouble. You just hit it on CIFS because
>> > the server happened to give you a very large inode number.
>> >
>> > If we're going to add printk's for this situation, it probably ought to
>> > be in a more generic place.
>> >
>>
>> By generic place, did you mean at the VFS level? I think at VFS level,
>> there is little information about the Server or underlying fs and this
>> information doesn't seem too critical that VFS should warn/care much about.
>>
>> May be sticking to a cFYI along with Server detail is a good idea?
>>
> My poing was mainly that there's nothing special about CIFS in this
> regard, other than the fact that servers regularly send us inodes that
> are larger than 2^32. Why should we do this for cifs but not for nfs,
> xfs, ext4, etc?
>
> The filldir function gets a dentry as an argument, so it could
> reasonably generate a printk for this. I'm also not keen on
> the printk recommending noserverino for this. That has its own
> drawbacks.
>
> A cFYI for this sort of thing seems reasonable however.

I agree that a cFYI is reasonable.  The next obvious question is: do
we need to add code to generate unique 32 bit inode numbers
that don't collide (as IIRC Samba does by xor the high and low 32
bits of the inode number) when the app can't support ino64
I would prefer not to go back to noserverino since that has worse
drawbacks.



-- 
Thanks,

Steve

Re: [PATCH] cifs: Add information about noserverino

[Jeff Layton]

On Thu, 9 Dec 2010 12:26:39 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Thu, Dec 9, 2010 at 6:09 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Thu, 09 Dec 2010 17:10:28 +0530
> > Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
> >
> >> On 12/06/2010 09:08 PM, Jeff Layton wrote:
> >> > On Mon, 06 Dec 2010 16:35:06 +0100
> >> > Bernhard Walle <bernhard-X9USDgGjgfuzQB+pC5nmwQ@public.gmane.org> wrote:
> >> >
> >> >>
> >> >> Zitat von Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> >> >>
> >> >>>
> >> >>> I'm still not sure I like this patch however. It potentially means a
> >> >>> lot of printk spam since these things have no ratelimiting. It also
> >> >>> doesn't tell me anything about which server might be giving me grief.
> >> >>>
> >> >>> Maybe this should be turned into a cFYI?
> >> >>
> >> >> Well, if I see it in the kernel log, it doesn't matter if it's info or
> >> >> something else.
> >> >>
> >> >>> The bottom line though is that running 32-bit applications that were
> >> >>> built without -D_FILE_OFFSET_BITS=64 on a 64-bit kernel is a very bad
> >> >>> idea. It would be nice to be able to alert users that things aren't
> >> >>> working the way they expect, but I'm not sure this is the right place
> >> >>> to do that.
> >> >>
> >> >> Well, but there *are* such application (in my case it was Softmaker Office
> >> >> which is a proprietary word processor) and it's quite nice if you know
> >> >> how you can workaround it when you encounter such a problem. That's all.
> >> >>
> >> >
> >> > Sure...but this problem is not limited to CIFS. Many modern filesystems
> >> > use 64-bit inodes. Running this application on XFS or NFS for instance
> >> > is likely to give you the same trouble. You just hit it on CIFS because
> >> > the server happened to give you a very large inode number.
> >> >
> >> > If we're going to add printk's for this situation, it probably ought to
> >> > be in a more generic place.
> >> >
> >>
> >> By generic place, did you mean at the VFS level? I think at VFS level,
> >> there is little information about the Server or underlying fs and this
> >> information doesn't seem too critical that VFS should warn/care much about.
> >>
> >> May be sticking to a cFYI along with Server detail is a good idea?
> >>
> > My poing was mainly that there's nothing special about CIFS in this
> > regard, other than the fact that servers regularly send us inodes that
> > are larger than 2^32. Why should we do this for cifs but not for nfs,
> > xfs, ext4, etc?
> >
> > The filldir function gets a dentry as an argument, so it could
> > reasonably generate a printk for this. I'm also not keen on
> > the printk recommending noserverino for this. That has its own
> > drawbacks.
> >
> > A cFYI for this sort of thing seems reasonable however.
> 
> I agree that a cFYI is reasonable.  The next obvious question is: do
> we need to add code to generate unique 32 bit inode numbers
> that don't collide (as IIRC Samba does by xor the high and low 32
> bits of the inode number) when the app can't support ino64
> I would prefer not to go back to noserverino since that has worse
> drawbacks.
> 

Right, the fact that noserverino works around this is really just due
to an implementation detail of iunique(). That should probably be
discouraged as a solution since it's not guaranteed to be a workaround
in the future.

If we did add such a switch, I'd suggest that we pattern it after what
NFS did for this. They added an "enable_ino64" module parameter a
couple of years ago that defaults to "true".

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH] cifs: Add information about noserverino

[Jeff Layton]

On Thu, 9 Dec 2010 12:26:39 -0600
Steve French <smfrench@gmail.com> wrote:

> On Thu, Dec 9, 2010 at 6:09 AM, Jeff Layton <jlayton@redhat.com> wrote:
> > On Thu, 09 Dec 2010 17:10:28 +0530
> > Suresh Jayaraman <sjayaraman@suse.de> wrote:
> >
> >> On 12/06/2010 09:08 PM, Jeff Layton wrote:
> >> > On Mon, 06 Dec 2010 16:35:06 +0100
> >> > Bernhard Walle <bernhard@bwalle.de> wrote:
> >> >
> >> >>
> >> >> Zitat von Jeff Layton <jlayton@redhat.com>:
> >> >>
> >> >>>
> >> >>> I'm still not sure I like this patch however. It potentially means a
> >> >>> lot of printk spam since these things have no ratelimiting. It also
> >> >>> doesn't tell me anything about which server might be giving me grief.
> >> >>>
> >> >>> Maybe this should be turned into a cFYI?
> >> >>
> >> >> Well, if I see it in the kernel log, it doesn't matter if it's info or
> >> >> something else.
> >> >>
> >> >>> The bottom line though is that running 32-bit applications that were
> >> >>> built without -D_FILE_OFFSET_BITS=64 on a 64-bit kernel is a very bad
> >> >>> idea. It would be nice to be able to alert users that things aren't
> >> >>> working the way they expect, but I'm not sure this is the right place
> >> >>> to do that.
> >> >>
> >> >> Well, but there *are* such application (in my case it was Softmaker Office
> >> >> which is a proprietary word processor) and it's quite nice if you know
> >> >> how you can workaround it when you encounter such a problem. That's all.
> >> >>
> >> >
> >> > Sure...but this problem is not limited to CIFS. Many modern filesystems
> >> > use 64-bit inodes. Running this application on XFS or NFS for instance
> >> > is likely to give you the same trouble. You just hit it on CIFS because
> >> > the server happened to give you a very large inode number.
> >> >
> >> > If we're going to add printk's for this situation, it probably ought to
> >> > be in a more generic place.
> >> >
> >>
> >> By generic place, did you mean at the VFS level? I think at VFS level,
> >> there is little information about the Server or underlying fs and this
> >> information doesn't seem too critical that VFS should warn/care much about.
> >>
> >> May be sticking to a cFYI along with Server detail is a good idea?
> >>
> > My poing was mainly that there's nothing special about CIFS in this
> > regard, other than the fact that servers regularly send us inodes that
> > are larger than 2^32. Why should we do this for cifs but not for nfs,
> > xfs, ext4, etc?
> >
> > The filldir function gets a dentry as an argument, so it could
> > reasonably generate a printk for this. I'm also not keen on
> > the printk recommending noserverino for this. That has its own
> > drawbacks.
> >
> > A cFYI for this sort of thing seems reasonable however.
> 
> I agree that a cFYI is reasonable.  The next obvious question is: do
> we need to add code to generate unique 32 bit inode numbers
> that don't collide (as IIRC Samba does by xor the high and low 32
> bits of the inode number) when the app can't support ino64
> I would prefer not to go back to noserverino since that has worse
> drawbacks.
> 

Right, the fact that noserverino works around this is really just due
to an implementation detail of iunique(). That should probably be
discouraged as a solution since it's not guaranteed to be a workaround
in the future.

If we did add such a switch, I'd suggest that we pattern it after what
NFS did for this. They added an "enable_ino64" module parameter a
couple of years ago that defaults to "true".

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] cifs: Add information about noserverino

[Steve French]

On Thu, Dec 9, 2010 at 1:34 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Thu, 9 Dec 2010 12:26:39 -0600
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> On Thu, Dec 9, 2010 at 6:09 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> > On Thu, 09 Dec 2010 17:10:28 +0530
>> > Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
>> >
>> >> On 12/06/2010 09:08 PM, Jeff Layton wrote:
>> >> > On Mon, 06 Dec 2010 16:35:06 +0100
>> >> > Bernhard Walle <bernhard-X9USDgGjgfuzQB+pC5nmwQ@public.gmane.org> wrote:
>> >> >
>> >> >>
>> >> >> Zitat von Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>> >> >>
>> >> >>>
>> >> >>> I'm still not sure I like this patch however. It potentially means a
>> >> >>> lot of printk spam since these things have no ratelimiting. It also
>> >> >>> doesn't tell me anything about which server might be giving me grief.
>> >> >>>
>> >> >>> Maybe this should be turned into a cFYI?
>> >> >>
>> >> >> Well, if I see it in the kernel log, it doesn't matter if it's info or
>> >> >> something else.
>> >> >>
>> >> >>> The bottom line though is that running 32-bit applications that were
>> >> >>> built without -D_FILE_OFFSET_BITS=64 on a 64-bit kernel is a very bad
>> >> >>> idea. It would be nice to be able to alert users that things aren't
>> >> >>> working the way they expect, but I'm not sure this is the right place
>> >> >>> to do that.
>> >> >>
>> >> >> Well, but there *are* such application (in my case it was Softmaker Office
>> >> >> which is a proprietary word processor) and it's quite nice if you know
>> >> >> how you can workaround it when you encounter such a problem. That's all.
>> >> >>
>> >> >
>> >> > Sure...but this problem is not limited to CIFS. Many modern filesystems
>> >> > use 64-bit inodes. Running this application on XFS or NFS for instance
>> >> > is likely to give you the same trouble. You just hit it on CIFS because
>> >> > the server happened to give you a very large inode number.
>> >> >
>> >> > If we're going to add printk's for this situation, it probably ought to
>> >> > be in a more generic place.
>> >> >
>> >>
>> >> By generic place, did you mean at the VFS level? I think at VFS level,
>> >> there is little information about the Server or underlying fs and this
>> >> information doesn't seem too critical that VFS should warn/care much about.
>> >>
>> >> May be sticking to a cFYI along with Server detail is a good idea?
>> >>
>> > My poing was mainly that there's nothing special about CIFS in this
>> > regard, other than the fact that servers regularly send us inodes that
>> > are larger than 2^32. Why should we do this for cifs but not for nfs,
>> > xfs, ext4, etc?
>> >
>> > The filldir function gets a dentry as an argument, so it could
>> > reasonably generate a printk for this. I'm also not keen on
>> > the printk recommending noserverino for this. That has its own
>> > drawbacks.
>> >
>> > A cFYI for this sort of thing seems reasonable however.
>>
>> I agree that a cFYI is reasonable.  The next obvious question is: do
>> we need to add code to generate unique 32 bit inode numbers
>> that don't collide (as IIRC Samba does by xor the high and low 32
>> bits of the inode number) when the app can't support ino64
>> I would prefer not to go back to noserverino since that has worse
>> drawbacks.
>>
>
> Right, the fact that noserverino works around this is really just due
> to an implementation detail of iunique(). That should probably be
> discouraged as a solution since it's not guaranteed to be a workaround
> in the future.
>
> If we did add such a switch, I'd suggest that we pattern it after what
> NFS did for this. They added an "enable_ino64" module parameter a
> couple of years ago that defaults to "true".

makes me uncomfortable to break ino64 for all mounts - when we
may have one application on one mount that needs it (might be
better to make a mount related)



-- 
Thanks,

Steve

Re: [PATCH] cifs: Add information about noserverino

[Steve French]

On Thu, Dec 9, 2010 at 1:34 PM, Jeff Layton <jlayton@redhat.com> wrote:
> On Thu, 9 Dec 2010 12:26:39 -0600
> Steve French <smfrench@gmail.com> wrote:
>
>> On Thu, Dec 9, 2010 at 6:09 AM, Jeff Layton <jlayton@redhat.com> wrote:
>> > On Thu, 09 Dec 2010 17:10:28 +0530
>> > Suresh Jayaraman <sjayaraman@suse.de> wrote:
>> >
>> >> On 12/06/2010 09:08 PM, Jeff Layton wrote:
>> >> > On Mon, 06 Dec 2010 16:35:06 +0100
>> >> > Bernhard Walle <bernhard@bwalle.de> wrote:
>> >> >
>> >> >>
>> >> >> Zitat von Jeff Layton <jlayton@redhat.com>:
>> >> >>
>> >> >>>
>> >> >>> I'm still not sure I like this patch however. It potentially means a
>> >> >>> lot of printk spam since these things have no ratelimiting. It also
>> >> >>> doesn't tell me anything about which server might be giving me grief.
>> >> >>>
>> >> >>> Maybe this should be turned into a cFYI?
>> >> >>
>> >> >> Well, if I see it in the kernel log, it doesn't matter if it's info or
>> >> >> something else.
>> >> >>
>> >> >>> The bottom line though is that running 32-bit applications that were
>> >> >>> built without -D_FILE_OFFSET_BITS=64 on a 64-bit kernel is a very bad
>> >> >>> idea. It would be nice to be able to alert users that things aren't
>> >> >>> working the way they expect, but I'm not sure this is the right place
>> >> >>> to do that.
>> >> >>
>> >> >> Well, but there *are* such application (in my case it was Softmaker Office
>> >> >> which is a proprietary word processor) and it's quite nice if you know
>> >> >> how you can workaround it when you encounter such a problem. That's all.
>> >> >>
>> >> >
>> >> > Sure...but this problem is not limited to CIFS. Many modern filesystems
>> >> > use 64-bit inodes. Running this application on XFS or NFS for instance
>> >> > is likely to give you the same trouble. You just hit it on CIFS because
>> >> > the server happened to give you a very large inode number.
>> >> >
>> >> > If we're going to add printk's for this situation, it probably ought to
>> >> > be in a more generic place.
>> >> >
>> >>
>> >> By generic place, did you mean at the VFS level? I think at VFS level,
>> >> there is little information about the Server or underlying fs and this
>> >> information doesn't seem too critical that VFS should warn/care much about.
>> >>
>> >> May be sticking to a cFYI along with Server detail is a good idea?
>> >>
>> > My poing was mainly that there's nothing special about CIFS in this
>> > regard, other than the fact that servers regularly send us inodes that
>> > are larger than 2^32. Why should we do this for cifs but not for nfs,
>> > xfs, ext4, etc?
>> >
>> > The filldir function gets a dentry as an argument, so it could
>> > reasonably generate a printk for this. I'm also not keen on
>> > the printk recommending noserverino for this. That has its own
>> > drawbacks.
>> >
>> > A cFYI for this sort of thing seems reasonable however.
>>
>> I agree that a cFYI is reasonable.  The next obvious question is: do
>> we need to add code to generate unique 32 bit inode numbers
>> that don't collide (as IIRC Samba does by xor the high and low 32
>> bits of the inode number) when the app can't support ino64
>> I would prefer not to go back to noserverino since that has worse
>> drawbacks.
>>
>
> Right, the fact that noserverino works around this is really just due
> to an implementation detail of iunique(). That should probably be
> discouraged as a solution since it's not guaranteed to be a workaround
> in the future.
>
> If we did add such a switch, I'd suggest that we pattern it after what
> NFS did for this. They added an "enable_ino64" module parameter a
> couple of years ago that defaults to "true".

makes me uncomfortable to break ino64 for all mounts - when we
may have one application on one mount that needs it (might be
better to make a mount related)



-- 
Thanks,

Steve

Re: [PATCH] cifs: Add information about noserverino

[Jeff Layton]

On Thu, 9 Dec 2010 14:44:27 -0600
Steve French <smfrench@gmail.com> wrote:

> On Thu, Dec 9, 2010 at 1:34 PM, Jeff Layton <jlayton@redhat.com> wrote:
> > On Thu, 9 Dec 2010 12:26:39 -0600
> > Steve French <smfrench@gmail.com> wrote:
> >
> >> On Thu, Dec 9, 2010 at 6:09 AM, Jeff Layton <jlayton@redhat.com> wrote:
> >> > On Thu, 09 Dec 2010 17:10:28 +0530
> >> > Suresh Jayaraman <sjayaraman@suse.de> wrote:
> >> >
> >> >> On 12/06/2010 09:08 PM, Jeff Layton wrote:
> >> >> > On Mon, 06 Dec 2010 16:35:06 +0100
> >> >> > Bernhard Walle <bernhard@bwalle.de> wrote:
> >> >> >
> >> >> >>
> >> >> >> Zitat von Jeff Layton <jlayton@redhat.com>:
> >> >> >>
> >> >> >>>
> >> >> >>> I'm still not sure I like this patch however. It potentially means a
> >> >> >>> lot of printk spam since these things have no ratelimiting. It also
> >> >> >>> doesn't tell me anything about which server might be giving me grief.
> >> >> >>>
> >> >> >>> Maybe this should be turned into a cFYI?
> >> >> >>
> >> >> >> Well, if I see it in the kernel log, it doesn't matter if it's info or
> >> >> >> something else.
> >> >> >>
> >> >> >>> The bottom line though is that running 32-bit applications that were
> >> >> >>> built without -D_FILE_OFFSET_BITS=64 on a 64-bit kernel is a very bad
> >> >> >>> idea. It would be nice to be able to alert users that things aren't
> >> >> >>> working the way they expect, but I'm not sure this is the right place
> >> >> >>> to do that.
> >> >> >>
> >> >> >> Well, but there *are* such application (in my case it was Softmaker Office
> >> >> >> which is a proprietary word processor) and it's quite nice if you know
> >> >> >> how you can workaround it when you encounter such a problem. That's all.
> >> >> >>
> >> >> >
> >> >> > Sure...but this problem is not limited to CIFS. Many modern filesystems
> >> >> > use 64-bit inodes. Running this application on XFS or NFS for instance
> >> >> > is likely to give you the same trouble. You just hit it on CIFS because
> >> >> > the server happened to give you a very large inode number.
> >> >> >
> >> >> > If we're going to add printk's for this situation, it probably ought to
> >> >> > be in a more generic place.
> >> >> >
> >> >>
> >> >> By generic place, did you mean at the VFS level? I think at VFS level,
> >> >> there is little information about the Server or underlying fs and this
> >> >> information doesn't seem too critical that VFS should warn/care much about.
> >> >>
> >> >> May be sticking to a cFYI along with Server detail is a good idea?
> >> >>
> >> > My poing was mainly that there's nothing special about CIFS in this
> >> > regard, other than the fact that servers regularly send us inodes that
> >> > are larger than 2^32. Why should we do this for cifs but not for nfs,
> >> > xfs, ext4, etc?
> >> >
> >> > The filldir function gets a dentry as an argument, so it could
> >> > reasonably generate a printk for this. I'm also not keen on
> >> > the printk recommending noserverino for this. That has its own
> >> > drawbacks.
> >> >
> >> > A cFYI for this sort of thing seems reasonable however.
> >>
> >> I agree that a cFYI is reasonable.  The next obvious question is: do
> >> we need to add code to generate unique 32 bit inode numbers
> >> that don't collide (as IIRC Samba does by xor the high and low 32
> >> bits of the inode number) when the app can't support ino64
> >> I would prefer not to go back to noserverino since that has worse
> >> drawbacks.
> >>
> >
> > Right, the fact that noserverino works around this is really just due
> > to an implementation detail of iunique(). That should probably be
> > discouraged as a solution since it's not guaranteed to be a workaround
> > in the future.
> >
> > If we did add such a switch, I'd suggest that we pattern it after what
> > NFS did for this. They added an "enable_ino64" module parameter a
> > couple of years ago that defaults to "true".
> 
> makes me uncomfortable to break ino64 for all mounts - when we
> may have one application on one mount that needs it (might be
> better to make a mount related)
> 

It's a workaround for broken applications. Some discomfort is
reasonable, as long as the default is that ino64 is enabled.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] cifs: Add information about noserverino

[Jeff Layton]

On Thu, 9 Dec 2010 14:44:27 -0600
Steve French <smfrench@gmail.com> wrote:

> On Thu, Dec 9, 2010 at 1:34 PM, Jeff Layton <jlayton@redhat.com> wrote:
> > On Thu, 9 Dec 2010 12:26:39 -0600
> > Steve French <smfrench@gmail.com> wrote:
> >
> >> On Thu, Dec 9, 2010 at 6:09 AM, Jeff Layton <jlayton@redhat.com> wrote:
> >> > On Thu, 09 Dec 2010 17:10:28 +0530
> >> > Suresh Jayaraman <sjayaraman@suse.de> wrote:
> >> >
> >> >> On 12/06/2010 09:08 PM, Jeff Layton wrote:
> >> >> > On Mon, 06 Dec 2010 16:35:06 +0100
> >> >> > Bernhard Walle <bernhard@bwalle.de> wrote:
> >> >> >
> >> >> >>
> >> >> >> Zitat von Jeff Layton <jlayton@redhat.com>:
> >> >> >>
> >> >> >>>
> >> >> >>> I'm still not sure I like this patch however. It potentially means a
> >> >> >>> lot of printk spam since these things have no ratelimiting. It also
> >> >> >>> doesn't tell me anything about which server might be giving me grief.
> >> >> >>>
> >> >> >>> Maybe this should be turned into a cFYI?
> >> >> >>
> >> >> >> Well, if I see it in the kernel log, it doesn't matter if it's info or
> >> >> >> something else.
> >> >> >>
> >> >> >>> The bottom line though is that running 32-bit applications that were
> >> >> >>> built without -D_FILE_OFFSET_BITS=64 on a 64-bit kernel is a very bad
> >> >> >>> idea. It would be nice to be able to alert users that things aren't
> >> >> >>> working the way they expect, but I'm not sure this is the right place
> >> >> >>> to do that.
> >> >> >>
> >> >> >> Well, but there *are* such application (in my case it was Softmaker Office
> >> >> >> which is a proprietary word processor) and it's quite nice if you know
> >> >> >> how you can workaround it when you encounter such a problem. That's all.
> >> >> >>
> >> >> >
> >> >> > Sure...but this problem is not limited to CIFS. Many modern filesystems
> >> >> > use 64-bit inodes. Running this application on XFS or NFS for instance
> >> >> > is likely to give you the same trouble. You just hit it on CIFS because
> >> >> > the server happened to give you a very large inode number.
> >> >> >
> >> >> > If we're going to add printk's for this situation, it probably ought to
> >> >> > be in a more generic place.
> >> >> >
> >> >>
> >> >> By generic place, did you mean at the VFS level? I think at VFS level,
> >> >> there is little information about the Server or underlying fs and this
> >> >> information doesn't seem too critical that VFS should warn/care much about.
> >> >>
> >> >> May be sticking to a cFYI along with Server detail is a good idea?
> >> >>
> >> > My poing was mainly that there's nothing special about CIFS in this
> >> > regard, other than the fact that servers regularly send us inodes that
> >> > are larger than 2^32. Why should we do this for cifs but not for nfs,
> >> > xfs, ext4, etc?
> >> >
> >> > The filldir function gets a dentry as an argument, so it could
> >> > reasonably generate a printk for this. I'm also not keen on
> >> > the printk recommending noserverino for this. That has its own
> >> > drawbacks.
> >> >
> >> > A cFYI for this sort of thing seems reasonable however.
> >>
> >> I agree that a cFYI is reasonable.  The next obvious question is: do
> >> we need to add code to generate unique 32 bit inode numbers
> >> that don't collide (as IIRC Samba does by xor the high and low 32
> >> bits of the inode number) when the app can't support ino64
> >> I would prefer not to go back to noserverino since that has worse
> >> drawbacks.
> >>
> >
> > Right, the fact that noserverino works around this is really just due
> > to an implementation detail of iunique(). That should probably be
> > discouraged as a solution since it's not guaranteed to be a workaround
> > in the future.
> >
> > If we did add such a switch, I'd suggest that we pattern it after what
> > NFS did for this. They added an "enable_ino64" module parameter a
> > couple of years ago that defaults to "true".
> 
> makes me uncomfortable to break ino64 for all mounts - when we
> may have one application on one mount that needs it (might be
> better to make a mount related)
> 

It's a workaround for broken applications. Some discomfort is
reasonable, as long as the default is that ino64 is enabled.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] cifs: Add information about noserverino

[Suresh Jayaraman]

On 12/10/2010 02:14 AM, Steve French wrote:
> On Thu, Dec 9, 2010 at 1:34 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> On Thu, 9 Dec 2010 12:26:39 -0600
>> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>>> On Thu, Dec 9, 2010 at 6:09 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>>> On Thu, 09 Dec 2010 17:10:28 +0530
>>>> Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
>>>>
>>>>> On 12/06/2010 09:08 PM, Jeff Layton wrote:
>>>>>> On Mon, 06 Dec 2010 16:35:06 +0100
>>>>>> Bernhard Walle <bernhard-X9USDgGjgfuzQB+pC5nmwQ@public.gmane.org> wrote:
>>>>>>
>>>>>>>
>>>>>>> Zitat von Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>>>>>>>
>>>>>>>>
>>>>>>>> I'm still not sure I like this patch however. It potentially means a
>>>>>>>> lot of printk spam since these things have no ratelimiting. It also
>>>>>>>> doesn't tell me anything about which server might be giving me grief.
>>>>>>>>
>>>>>>>> Maybe this should be turned into a cFYI?
>>>>>>>
>>>>>>> Well, if I see it in the kernel log, it doesn't matter if it's info or
>>>>>>> something else.
>>>>>>>
>>>>>>>> The bottom line though is that running 32-bit applications that were
>>>>>>>> built without -D_FILE_OFFSET_BITS=64 on a 64-bit kernel is a very bad
>>>>>>>> idea. It would be nice to be able to alert users that things aren't
>>>>>>>> working the way they expect, but I'm not sure this is the right place
>>>>>>>> to do that.
>>>>>>>
>>>>>>> Well, but there *are* such application (in my case it was Softmaker Office
>>>>>>> which is a proprietary word processor) and it's quite nice if you know
>>>>>>> how you can workaround it when you encounter such a problem. That's all.
>>>>>>>
>>>>>>
>>>>>> Sure...but this problem is not limited to CIFS. Many modern filesystems
>>>>>> use 64-bit inodes. Running this application on XFS or NFS for instance
>>>>>> is likely to give you the same trouble. You just hit it on CIFS because
>>>>>> the server happened to give you a very large inode number.
>>>>>>
>>>>>> If we're going to add printk's for this situation, it probably ought to
>>>>>> be in a more generic place.
>>>>>>
>>>>>
>>>>> By generic place, did you mean at the VFS level? I think at VFS level,
>>>>> there is little information about the Server or underlying fs and this
>>>>> information doesn't seem too critical that VFS should warn/care much about.
>>>>>
>>>>> May be sticking to a cFYI along with Server detail is a good idea?
>>>>>
>>>> My poing was mainly that there's nothing special about CIFS in this
>>>> regard, other than the fact that servers regularly send us inodes that
>>>> are larger than 2^32. Why should we do this for cifs but not for nfs,
>>>> xfs, ext4, etc?
>>>>
>>>> The filldir function gets a dentry as an argument, so it could
>>>> reasonably generate a printk for this. I'm also not keen on
>>>> the printk recommending noserverino for this. That has its own
>>>> drawbacks.
>>>>
>>>> A cFYI for this sort of thing seems reasonable however.
>>>
>>> I agree that a cFYI is reasonable. �The next obvious question is: do
>>> we need to add code to generate unique 32 bit inode numbers
>>> that don't collide (as IIRC Samba does by xor the high and low 32
>>> bits of the inode number) when the app can't support ino64
>>> I would prefer not to go back to noserverino since that has worse
>>> drawbacks.
>>>
>>
>> Right, the fact that noserverino works around this is really just due
>> to an implementation detail of iunique(). That should probably be
>> discouraged as a solution since it's not guaranteed to be a workaround
>> in the future.
>>
>> If we did add such a switch, I'd suggest that we pattern it after what
>> NFS did for this. They added an "enable_ino64" module parameter a
>> couple of years ago that defaults to "true".

What are the advantages we have by making it a module parameter as
opposed to an mount option? XFS seems to have "inode64" mount option for
quite sometime now, without much issues..

> makes me uncomfortable to break ino64 for all mounts - when we
> may have one application on one mount that needs it (might be
> better to make a mount related)
> 
> 


-- 
Suresh Jayaraman

Re: [PATCH] cifs: Add information about noserverino

[Suresh Jayaraman]

On 12/10/2010 02:14 AM, Steve French wrote:
> On Thu, Dec 9, 2010 at 1:34 PM, Jeff Layton <jlayton@redhat.com> wrote:
>> On Thu, 9 Dec 2010 12:26:39 -0600
>> Steve French <smfrench@gmail.com> wrote:
>>
>>> On Thu, Dec 9, 2010 at 6:09 AM, Jeff Layton <jlayton@redhat.com> wrote:
>>>> On Thu, 09 Dec 2010 17:10:28 +0530
>>>> Suresh Jayaraman <sjayaraman@suse.de> wrote:
>>>>
>>>>> On 12/06/2010 09:08 PM, Jeff Layton wrote:
>>>>>> On Mon, 06 Dec 2010 16:35:06 +0100
>>>>>> Bernhard Walle <bernhard@bwalle.de> wrote:
>>>>>>
>>>>>>>
>>>>>>> Zitat von Jeff Layton <jlayton@redhat.com>:
>>>>>>>
>>>>>>>>
>>>>>>>> I'm still not sure I like this patch however. It potentially means a
>>>>>>>> lot of printk spam since these things have no ratelimiting. It also
>>>>>>>> doesn't tell me anything about which server might be giving me grief.
>>>>>>>>
>>>>>>>> Maybe this should be turned into a cFYI?
>>>>>>>
>>>>>>> Well, if I see it in the kernel log, it doesn't matter if it's info or
>>>>>>> something else.
>>>>>>>
>>>>>>>> The bottom line though is that running 32-bit applications that were
>>>>>>>> built without -D_FILE_OFFSET_BITS=64 on a 64-bit kernel is a very bad
>>>>>>>> idea. It would be nice to be able to alert users that things aren't
>>>>>>>> working the way they expect, but I'm not sure this is the right place
>>>>>>>> to do that.
>>>>>>>
>>>>>>> Well, but there *are* such application (in my case it was Softmaker Office
>>>>>>> which is a proprietary word processor) and it's quite nice if you know
>>>>>>> how you can workaround it when you encounter such a problem. That's all.
>>>>>>>
>>>>>>
>>>>>> Sure...but this problem is not limited to CIFS. Many modern filesystems
>>>>>> use 64-bit inodes. Running this application on XFS or NFS for instance
>>>>>> is likely to give you the same trouble. You just hit it on CIFS because
>>>>>> the server happened to give you a very large inode number.
>>>>>>
>>>>>> If we're going to add printk's for this situation, it probably ought to
>>>>>> be in a more generic place.
>>>>>>
>>>>>
>>>>> By generic place, did you mean at the VFS level? I think at VFS level,
>>>>> there is little information about the Server or underlying fs and this
>>>>> information doesn't seem too critical that VFS should warn/care much about.
>>>>>
>>>>> May be sticking to a cFYI along with Server detail is a good idea?
>>>>>
>>>> My poing was mainly that there's nothing special about CIFS in this
>>>> regard, other than the fact that servers regularly send us inodes that
>>>> are larger than 2^32. Why should we do this for cifs but not for nfs,
>>>> xfs, ext4, etc?
>>>>
>>>> The filldir function gets a dentry as an argument, so it could
>>>> reasonably generate a printk for this. I'm also not keen on
>>>> the printk recommending noserverino for this. That has its own
>>>> drawbacks.
>>>>
>>>> A cFYI for this sort of thing seems reasonable however.
>>>
>>> I agree that a cFYI is reasonable. �The next obvious question is: do
>>> we need to add code to generate unique 32 bit inode numbers
>>> that don't collide (as IIRC Samba does by xor the high and low 32
>>> bits of the inode number) when the app can't support ino64
>>> I would prefer not to go back to noserverino since that has worse
>>> drawbacks.
>>>
>>
>> Right, the fact that noserverino works around this is really just due
>> to an implementation detail of iunique(). That should probably be
>> discouraged as a solution since it's not guaranteed to be a workaround
>> in the future.
>>
>> If we did add such a switch, I'd suggest that we pattern it after what
>> NFS did for this. They added an "enable_ino64" module parameter a
>> couple of years ago that defaults to "true".

What are the advantages we have by making it a module parameter as
opposed to an mount option? XFS seems to have "inode64" mount option for
quite sometime now, without much issues..

> makes me uncomfortable to break ino64 for all mounts - when we
> may have one application on one mount that needs it (might be
> better to make a mount related)
> 
> 


-- 
Suresh Jayaraman

Re: [PATCH] cifs: Add information about noserverino

[Steve French]

On Thu, Dec 9, 2010 at 9:09 PM, Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
> On 12/10/2010 02:14 AM, Steve French wrote:
>> On Thu, Dec 9, 2010 at 1:34 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>> On Thu, 9 Dec 2010 12:26:39 -0600
>>> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>
>>>> On Thu, Dec 9, 2010 at 6:09 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>>>> On Thu, 09 Dec 2010 17:10:28 +0530
>>>>> Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
>>>>>
>>>>>> On 12/06/2010 09:08 PM, Jeff Layton wrote:
>>>>>>> On Mon, 06 Dec 2010 16:35:06 +0100
>>>>>>> Bernhard Walle <bernhard-X9USDgGjgfuzQB+pC5nmwQ@public.gmane.org> wrote:
>>>>>>>
>>>>>>>>
>>>>>>>> Zitat von Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>>>>>>>>
>>>>>>>>>
>>>>>>>>> I'm still not sure I like this patch however. It potentially means a
>>>>>>>>> lot of printk spam since these things have no ratelimiting. It also
>>>>>>>>> doesn't tell me anything about which server might be giving me grief.
>>>>>>>>>
>>>>>>>>> Maybe this should be turned into a cFYI?
>>>>>>>>
>>>>>>>> Well, if I see it in the kernel log, it doesn't matter if it's info or
>>>>>>>> something else.
>>>>>>>>
>>>>>>>>> The bottom line though is that running 32-bit applications that were
>>>>>>>>> built without -D_FILE_OFFSET_BITS=64 on a 64-bit kernel is a very bad
>>>>>>>>> idea. It would be nice to be able to alert users that things aren't
>>>>>>>>> working the way they expect, but I'm not sure this is the right place
>>>>>>>>> to do that.
>>>>>>>>
>>>>>>>> Well, but there *are* such application (in my case it was Softmaker Office
>>>>>>>> which is a proprietary word processor) and it's quite nice if you know
>>>>>>>> how you can workaround it when you encounter such a problem. That's all.
>>>>>>>>
>>>>>>>
>>>>>>> Sure...but this problem is not limited to CIFS. Many modern filesystems
>>>>>>> use 64-bit inodes. Running this application on XFS or NFS for instance
>>>>>>> is likely to give you the same trouble. You just hit it on CIFS because
>>>>>>> the server happened to give you a very large inode number.
>>>>>>>
>>>>>>> If we're going to add printk's for this situation, it probably ought to
>>>>>>> be in a more generic place.
>>>>>>>
>>>>>>
>>>>>> By generic place, did you mean at the VFS level? I think at VFS level,
>>>>>> there is little information about the Server or underlying fs and this
>>>>>> information doesn't seem too critical that VFS should warn/care much about.
>>>>>>
>>>>>> May be sticking to a cFYI along with Server detail is a good idea?
>>>>>>
>>>>> My poing was mainly that there's nothing special about CIFS in this
>>>>> regard, other than the fact that servers regularly send us inodes that
>>>>> are larger than 2^32. Why should we do this for cifs but not for nfs,
>>>>> xfs, ext4, etc?
>>>>>
>>>>> The filldir function gets a dentry as an argument, so it could
>>>>> reasonably generate a printk for this. I'm also not keen on
>>>>> the printk recommending noserverino for this. That has its own
>>>>> drawbacks.
>>>>>
>>>>> A cFYI for this sort of thing seems reasonable however.
>>>>
>>>> I agree that a cFYI is reasonable. �The next obvious question is: do
>>>> we need to add code to generate unique 32 bit inode numbers
>>>> that don't collide (as IIRC Samba does by xor the high and low 32
>>>> bits of the inode number) when the app can't support ino64
>>>> I would prefer not to go back to noserverino since that has worse
>>>> drawbacks.
>>>>
>>>
>>> Right, the fact that noserverino works around this is really just due
>>> to an implementation detail of iunique(). That should probably be
>>> discouraged as a solution since it's not guaranteed to be a workaround
>>> in the future.
>>>
>>> If we did add such a switch, I'd suggest that we pattern it after what
>>> NFS did for this. They added an "enable_ino64" module parameter a
>>> couple of years ago that defaults to "true".
>
> What are the advantages we have by making it a module parameter as
> opposed to an mount option? XFS seems to have "inode64" mount option for
> quite sometime now, without much issues..

I prefer mount option, but with the default to support 64 bit inode numbers.

>> makes me uncomfortable to break ino64 for all mounts - when we
>> may have one application on one mount that needs it (might be
>> better to make a mount related)
>>
>>
>
>
> --
> Suresh Jayaraman
>



-- 
Thanks,

Steve

Re: [PATCH] cifs: Add information about noserverino

[Steve French]

On Thu, Dec 9, 2010 at 9:09 PM, Suresh Jayaraman <sjayaraman@suse.de> wrote:
> On 12/10/2010 02:14 AM, Steve French wrote:
>> On Thu, Dec 9, 2010 at 1:34 PM, Jeff Layton <jlayton@redhat.com> wrote:
>>> On Thu, 9 Dec 2010 12:26:39 -0600
>>> Steve French <smfrench@gmail.com> wrote:
>>>
>>>> On Thu, Dec 9, 2010 at 6:09 AM, Jeff Layton <jlayton@redhat.com> wrote:
>>>>> On Thu, 09 Dec 2010 17:10:28 +0530
>>>>> Suresh Jayaraman <sjayaraman@suse.de> wrote:
>>>>>
>>>>>> On 12/06/2010 09:08 PM, Jeff Layton wrote:
>>>>>>> On Mon, 06 Dec 2010 16:35:06 +0100
>>>>>>> Bernhard Walle <bernhard@bwalle.de> wrote:
>>>>>>>
>>>>>>>>
>>>>>>>> Zitat von Jeff Layton <jlayton@redhat.com>:
>>>>>>>>
>>>>>>>>>
>>>>>>>>> I'm still not sure I like this patch however. It potentially means a
>>>>>>>>> lot of printk spam since these things have no ratelimiting. It also
>>>>>>>>> doesn't tell me anything about which server might be giving me grief.
>>>>>>>>>
>>>>>>>>> Maybe this should be turned into a cFYI?
>>>>>>>>
>>>>>>>> Well, if I see it in the kernel log, it doesn't matter if it's info or
>>>>>>>> something else.
>>>>>>>>
>>>>>>>>> The bottom line though is that running 32-bit applications that were
>>>>>>>>> built without -D_FILE_OFFSET_BITS=64 on a 64-bit kernel is a very bad
>>>>>>>>> idea. It would be nice to be able to alert users that things aren't
>>>>>>>>> working the way they expect, but I'm not sure this is the right place
>>>>>>>>> to do that.
>>>>>>>>
>>>>>>>> Well, but there *are* such application (in my case it was Softmaker Office
>>>>>>>> which is a proprietary word processor) and it's quite nice if you know
>>>>>>>> how you can workaround it when you encounter such a problem. That's all.
>>>>>>>>
>>>>>>>
>>>>>>> Sure...but this problem is not limited to CIFS. Many modern filesystems
>>>>>>> use 64-bit inodes. Running this application on XFS or NFS for instance
>>>>>>> is likely to give you the same trouble. You just hit it on CIFS because
>>>>>>> the server happened to give you a very large inode number.
>>>>>>>
>>>>>>> If we're going to add printk's for this situation, it probably ought to
>>>>>>> be in a more generic place.
>>>>>>>
>>>>>>
>>>>>> By generic place, did you mean at the VFS level? I think at VFS level,
>>>>>> there is little information about the Server or underlying fs and this
>>>>>> information doesn't seem too critical that VFS should warn/care much about.
>>>>>>
>>>>>> May be sticking to a cFYI along with Server detail is a good idea?
>>>>>>
>>>>> My poing was mainly that there's nothing special about CIFS in this
>>>>> regard, other than the fact that servers regularly send us inodes that
>>>>> are larger than 2^32. Why should we do this for cifs but not for nfs,
>>>>> xfs, ext4, etc?
>>>>>
>>>>> The filldir function gets a dentry as an argument, so it could
>>>>> reasonably generate a printk for this. I'm also not keen on
>>>>> the printk recommending noserverino for this. That has its own
>>>>> drawbacks.
>>>>>
>>>>> A cFYI for this sort of thing seems reasonable however.
>>>>
>>>> I agree that a cFYI is reasonable. �The next obvious question is: do
>>>> we need to add code to generate unique 32 bit inode numbers
>>>> that don't collide (as IIRC Samba does by xor the high and low 32
>>>> bits of the inode number) when the app can't support ino64
>>>> I would prefer not to go back to noserverino since that has worse
>>>> drawbacks.
>>>>
>>>
>>> Right, the fact that noserverino works around this is really just due
>>> to an implementation detail of iunique(). That should probably be
>>> discouraged as a solution since it's not guaranteed to be a workaround
>>> in the future.
>>>
>>> If we did add such a switch, I'd suggest that we pattern it after what
>>> NFS did for this. They added an "enable_ino64" module parameter a
>>> couple of years ago that defaults to "true".
>
> What are the advantages we have by making it a module parameter as
> opposed to an mount option? XFS seems to have "inode64" mount option for
> quite sometime now, without much issues..

I prefer mount option, but with the default to support 64 bit inode numbers.

>> makes me uncomfortable to break ino64 for all mounts - when we
>> may have one application on one mount that needs it (might be
>> better to make a mount related)
>>
>>
>
>
> --
> Suresh Jayaraman
>



-- 
Thanks,

Steve

Re: [PATCH] cifs: Add information about noserverino

[Jeff Layton]

On Thu, 9 Dec 2010 22:58:20 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Thu, Dec 9, 2010 at 9:09 PM, Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
> > On 12/10/2010 02:14 AM, Steve French wrote:
> >> On Thu, Dec 9, 2010 at 1:34 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >>> On Thu, 9 Dec 2010 12:26:39 -0600
> >>> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >>>
> >>>> On Thu, Dec 9, 2010 at 6:09 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >>>>> On Thu, 09 Dec 2010 17:10:28 +0530
> >>>>> Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
> >>>>>
> >>>>>> On 12/06/2010 09:08 PM, Jeff Layton wrote:
> >>>>>>> On Mon, 06 Dec 2010 16:35:06 +0100
> >>>>>>> Bernhard Walle <bernhard-X9USDgGjgfuzQB+pC5nmwQ@public.gmane.org> wrote:
> >>>>>>>
> >>>>>>>>
> >>>>>>>> Zitat von Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> >>>>>>>>
> >>>>>>>>>
> >>>>>>>>> I'm still not sure I like this patch however. It potentially means a
> >>>>>>>>> lot of printk spam since these things have no ratelimiting. It also
> >>>>>>>>> doesn't tell me anything about which server might be giving me grief.
> >>>>>>>>>
> >>>>>>>>> Maybe this should be turned into a cFYI?
> >>>>>>>>
> >>>>>>>> Well, if I see it in the kernel log, it doesn't matter if it's info or
> >>>>>>>> something else.
> >>>>>>>>
> >>>>>>>>> The bottom line though is that running 32-bit applications that were
> >>>>>>>>> built without -D_FILE_OFFSET_BITS=64 on a 64-bit kernel is a very bad
> >>>>>>>>> idea. It would be nice to be able to alert users that things aren't
> >>>>>>>>> working the way they expect, but I'm not sure this is the right place
> >>>>>>>>> to do that.
> >>>>>>>>
> >>>>>>>> Well, but there *are* such application (in my case it was Softmaker Office
> >>>>>>>> which is a proprietary word processor) and it's quite nice if you know
> >>>>>>>> how you can workaround it when you encounter such a problem. That's all.
> >>>>>>>>
> >>>>>>>
> >>>>>>> Sure...but this problem is not limited to CIFS. Many modern filesystems
> >>>>>>> use 64-bit inodes. Running this application on XFS or NFS for instance
> >>>>>>> is likely to give you the same trouble. You just hit it on CIFS because
> >>>>>>> the server happened to give you a very large inode number.
> >>>>>>>
> >>>>>>> If we're going to add printk's for this situation, it probably ought to
> >>>>>>> be in a more generic place.
> >>>>>>>
> >>>>>>
> >>>>>> By generic place, did you mean at the VFS level? I think at VFS level,
> >>>>>> there is little information about the Server or underlying fs and this
> >>>>>> information doesn't seem too critical that VFS should warn/care much about.
> >>>>>>
> >>>>>> May be sticking to a cFYI along with Server detail is a good idea?
> >>>>>>
> >>>>> My poing was mainly that there's nothing special about CIFS in this
> >>>>> regard, other than the fact that servers regularly send us inodes that
> >>>>> are larger than 2^32. Why should we do this for cifs but not for nfs,
> >>>>> xfs, ext4, etc?
> >>>>>
> >>>>> The filldir function gets a dentry as an argument, so it could
> >>>>> reasonably generate a printk for this. I'm also not keen on
> >>>>> the printk recommending noserverino for this. That has its own
> >>>>> drawbacks.
> >>>>>
> >>>>> A cFYI for this sort of thing seems reasonable however.
> >>>>
> >>>> I agree that a cFYI is reasonable. �The next obvious question is: do
> >>>> we need to add code to generate unique 32 bit inode numbers
> >>>> that don't collide (as IIRC Samba does by xor the high and low 32
> >>>> bits of the inode number) when the app can't support ino64
> >>>> I would prefer not to go back to noserverino since that has worse
> >>>> drawbacks.
> >>>>
> >>>
> >>> Right, the fact that noserverino works around this is really just due
> >>> to an implementation detail of iunique(). That should probably be
> >>> discouraged as a solution since it's not guaranteed to be a workaround
> >>> in the future.
> >>>
> >>> If we did add such a switch, I'd suggest that we pattern it after what
> >>> NFS did for this. They added an "enable_ino64" module parameter a
> >>> couple of years ago that defaults to "true".
> >
> > What are the advantages we have by making it a module parameter as
> > opposed to an mount option? XFS seems to have "inode64" mount option for
> > quite sometime now, without much issues..
> 
> I prefer mount option, but with the default to support 64 bit inode numbers.
> 
> >> makes me uncomfortable to break ino64 for all mounts - when we
> >> may have one application on one mount that needs it (might be
> >> better to make a mount related)
> >>
> >>

I think that NFS did it that way because of the way that superblocks
are shared between vfsmounts. It would be impossible to share a sb
between two vfsmounts with two different settings.

I won't object to a new mount option for it if that's the concensus.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH] cifs: Add information about noserverino

[Jeff Layton]

On Thu, 9 Dec 2010 22:58:20 -0600
Steve French <smfrench@gmail.com> wrote:

> On Thu, Dec 9, 2010 at 9:09 PM, Suresh Jayaraman <sjayaraman@suse.de> wrote:
> > On 12/10/2010 02:14 AM, Steve French wrote:
> >> On Thu, Dec 9, 2010 at 1:34 PM, Jeff Layton <jlayton@redhat.com> wrote:
> >>> On Thu, 9 Dec 2010 12:26:39 -0600
> >>> Steve French <smfrench@gmail.com> wrote:
> >>>
> >>>> On Thu, Dec 9, 2010 at 6:09 AM, Jeff Layton <jlayton@redhat.com> wrote:
> >>>>> On Thu, 09 Dec 2010 17:10:28 +0530
> >>>>> Suresh Jayaraman <sjayaraman@suse.de> wrote:
> >>>>>
> >>>>>> On 12/06/2010 09:08 PM, Jeff Layton wrote:
> >>>>>>> On Mon, 06 Dec 2010 16:35:06 +0100
> >>>>>>> Bernhard Walle <bernhard@bwalle.de> wrote:
> >>>>>>>
> >>>>>>>>
> >>>>>>>> Zitat von Jeff Layton <jlayton@redhat.com>:
> >>>>>>>>
> >>>>>>>>>
> >>>>>>>>> I'm still not sure I like this patch however. It potentially means a
> >>>>>>>>> lot of printk spam since these things have no ratelimiting. It also
> >>>>>>>>> doesn't tell me anything about which server might be giving me grief.
> >>>>>>>>>
> >>>>>>>>> Maybe this should be turned into a cFYI?
> >>>>>>>>
> >>>>>>>> Well, if I see it in the kernel log, it doesn't matter if it's info or
> >>>>>>>> something else.
> >>>>>>>>
> >>>>>>>>> The bottom line though is that running 32-bit applications that were
> >>>>>>>>> built without -D_FILE_OFFSET_BITS=64 on a 64-bit kernel is a very bad
> >>>>>>>>> idea. It would be nice to be able to alert users that things aren't
> >>>>>>>>> working the way they expect, but I'm not sure this is the right place
> >>>>>>>>> to do that.
> >>>>>>>>
> >>>>>>>> Well, but there *are* such application (in my case it was Softmaker Office
> >>>>>>>> which is a proprietary word processor) and it's quite nice if you know
> >>>>>>>> how you can workaround it when you encounter such a problem. That's all.
> >>>>>>>>
> >>>>>>>
> >>>>>>> Sure...but this problem is not limited to CIFS. Many modern filesystems
> >>>>>>> use 64-bit inodes. Running this application on XFS or NFS for instance
> >>>>>>> is likely to give you the same trouble. You just hit it on CIFS because
> >>>>>>> the server happened to give you a very large inode number.
> >>>>>>>
> >>>>>>> If we're going to add printk's for this situation, it probably ought to
> >>>>>>> be in a more generic place.
> >>>>>>>
> >>>>>>
> >>>>>> By generic place, did you mean at the VFS level? I think at VFS level,
> >>>>>> there is little information about the Server or underlying fs and this
> >>>>>> information doesn't seem too critical that VFS should warn/care much about.
> >>>>>>
> >>>>>> May be sticking to a cFYI along with Server detail is a good idea?
> >>>>>>
> >>>>> My poing was mainly that there's nothing special about CIFS in this
> >>>>> regard, other than the fact that servers regularly send us inodes that
> >>>>> are larger than 2^32. Why should we do this for cifs but not for nfs,
> >>>>> xfs, ext4, etc?
> >>>>>
> >>>>> The filldir function gets a dentry as an argument, so it could
> >>>>> reasonably generate a printk for this. I'm also not keen on
> >>>>> the printk recommending noserverino for this. That has its own
> >>>>> drawbacks.
> >>>>>
> >>>>> A cFYI for this sort of thing seems reasonable however.
> >>>>
> >>>> I agree that a cFYI is reasonable. �The next obvious question is: do
> >>>> we need to add code to generate unique 32 bit inode numbers
> >>>> that don't collide (as IIRC Samba does by xor the high and low 32
> >>>> bits of the inode number) when the app can't support ino64
> >>>> I would prefer not to go back to noserverino since that has worse
> >>>> drawbacks.
> >>>>
> >>>
> >>> Right, the fact that noserverino works around this is really just due
> >>> to an implementation detail of iunique(). That should probably be
> >>> discouraged as a solution since it's not guaranteed to be a workaround
> >>> in the future.
> >>>
> >>> If we did add such a switch, I'd suggest that we pattern it after what
> >>> NFS did for this. They added an "enable_ino64" module parameter a
> >>> couple of years ago that defaults to "true".
> >
> > What are the advantages we have by making it a module parameter as
> > opposed to an mount option? XFS seems to have "inode64" mount option for
> > quite sometime now, without much issues..
> 
> I prefer mount option, but with the default to support 64 bit inode numbers.
> 
> >> makes me uncomfortable to break ino64 for all mounts - when we
> >> may have one application on one mount that needs it (might be
> >> better to make a mount related)
> >>
> >>

I think that NFS did it that way because of the way that superblocks
are shared between vfsmounts. It would be impossible to share a sb
between two vfsmounts with two different settings.

I won't object to a new mount option for it if that's the concensus.

-- 
Jeff Layton <jlayton@redhat.com>