All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] CIFS: Fix mkdir/rmdir bug for the non-POSIX case
@ 2012-02-17 13:13 Pavel Shilovsky
       [not found] ` <1329484410-30540-1-git-send-email-piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
  0 siblings, 1 reply; 9+ messages in thread
From: Pavel Shilovsky @ 2012-02-17 13:13 UTC (permalink / raw)
  To: linux-cifs-u79uwXL29TY76Z2rM5mHXA

Currently we do inc/drop_nlink for a parent directory for every
mkdir/rmdir calls. That's wrong when Unix extensions are disabled
because in this case a server doesn't follow the same semantic and
returns the old value on the next QueryInfo request. As the result,
we update our value with the server one and then decrement it on
every rmdir call - go to negative nlink values.

Fix this by removing inc/drop_nlink for the parent directory from
mkdir/rmdir, setting it for a revalidation and ignoring NumberOfLinks
for directories when Unix extensions are disabled.

Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
---
 fs/cifs/inode.c |   28 +++++++++++++++++++---------
 1 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index a5f54b7..745da3d 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -534,6 +534,11 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
 	if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
 		fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode;
 		fattr->cf_dtype = DT_DIR;
+		/*
+		 * Server can return wrong NumberOfLinks value for directories
+		 * when Unix extensions are disabled - fake it.
+		 */
+		fattr->cf_nlink = 2;
 	} else {
 		fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode;
 		fattr->cf_dtype = DT_REG;
@@ -541,9 +546,9 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
 		/* clear write bits if ATTR_READONLY is set */
 		if (fattr->cf_cifsattrs & ATTR_READONLY)
 			fattr->cf_mode &= ~(S_IWUGO);
-	}
 
-	fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
+		fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
+	}
 
 	fattr->cf_uid = cifs_sb->mnt_uid;
 	fattr->cf_gid = cifs_sb->mnt_gid;
@@ -1322,7 +1327,6 @@ int cifs_mkdir(struct inode *inode, struct dentry *direntry, umode_t mode)
 			}
 /*BB check (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SET_UID ) to see if need
 	to set uid/gid */
-			inc_nlink(inode);
 
 			cifs_unix_basic_to_fattr(&fattr, pInfo, cifs_sb);
 			cifs_fill_uniqueid(inode->i_sb, &fattr);
@@ -1355,7 +1359,6 @@ mkdir_retry_old:
 		d_drop(direntry);
 	} else {
 mkdir_get_info:
-		inc_nlink(inode);
 		if (pTcon->unix_ext)
 			rc = cifs_get_inode_info_unix(&newinode, full_path,
 						      inode->i_sb, xid);
@@ -1436,6 +1439,11 @@ mkdir_get_info:
 		}
 	}
 mkdir_out:
+	/*
+	 * Force revalidate to get parent dir info when needed since cached
+	 * attributes are invalid now.
+	 */
+	CIFS_I(inode)->time = 0;
 	kfree(full_path);
 	FreeXid(xid);
 	cifs_put_tlink(tlink);
@@ -1475,7 +1483,6 @@ int cifs_rmdir(struct inode *inode, struct dentry *direntry)
 	cifs_put_tlink(tlink);
 
 	if (!rc) {
-		drop_nlink(inode);
 		spin_lock(&direntry->d_inode->i_lock);
 		i_size_write(direntry->d_inode, 0);
 		clear_nlink(direntry->d_inode);
@@ -1483,12 +1490,15 @@ int cifs_rmdir(struct inode *inode, struct dentry *direntry)
 	}
 
 	cifsInode = CIFS_I(direntry->d_inode);
-	cifsInode->time = 0;	/* force revalidate to go get info when
-				   needed */
+	/* force revalidate to go get info when needed */
+	cifsInode->time = 0;
 
 	cifsInode = CIFS_I(inode);
-	cifsInode->time = 0;	/* force revalidate to get parent dir info
-				   since cached search results now invalid */
+	/*
+	 * Force revalidate to get parent dir info when needed since cached
+	 * attributes are invalid now.
+	 */
+	cifsInode->time = 0;
 
 	direntry->d_inode->i_ctime = inode->i_ctime = inode->i_mtime =
 		current_fs_time(inode->i_sb);
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] CIFS: Fix mkdir/rmdir bug for the non-POSIX case
       [not found] ` <1329484410-30540-1-git-send-email-piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
@ 2012-02-21 13:26   ` Pavel Shilovsky
  2012-02-21 15:00   ` Jeff Layton
  1 sibling, 0 replies; 9+ messages in thread
From: Pavel Shilovsky @ 2012-02-21 13:26 UTC (permalink / raw)
  To: linux-cifs-u79uwXL29TY76Z2rM5mHXA

2012/2/17 Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>:
> Currently we do inc/drop_nlink for a parent directory for every
> mkdir/rmdir calls. That's wrong when Unix extensions are disabled
> because in this case a server doesn't follow the same semantic and
> returns the old value on the next QueryInfo request. As the result,
> we update our value with the server one and then decrement it on
> every rmdir call - go to negative nlink values.
>
> Fix this by removing inc/drop_nlink for the parent directory from
> mkdir/rmdir, setting it for a revalidation and ignoring NumberOfLinks
> for directories when Unix extensions are disabled.
>
> Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> ---
>  fs/cifs/inode.c |   28 +++++++++++++++++++---------
>  1 files changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index a5f54b7..745da3d 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -534,6 +534,11 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
>        if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
>                fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode;
>                fattr->cf_dtype = DT_DIR;
> +               /*
> +                * Server can return wrong NumberOfLinks value for directories
> +                * when Unix extensions are disabled - fake it.
> +                */
> +               fattr->cf_nlink = 2;
>        } else {
>                fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode;
>                fattr->cf_dtype = DT_REG;
> @@ -541,9 +546,9 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
>                /* clear write bits if ATTR_READONLY is set */
>                if (fattr->cf_cifsattrs & ATTR_READONLY)
>                        fattr->cf_mode &= ~(S_IWUGO);
> -       }
>
> -       fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
> +               fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
> +       }
>
>        fattr->cf_uid = cifs_sb->mnt_uid;
>        fattr->cf_gid = cifs_sb->mnt_gid;
> @@ -1322,7 +1327,6 @@ int cifs_mkdir(struct inode *inode, struct dentry *direntry, umode_t mode)
>                        }
>  /*BB check (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SET_UID ) to see if need
>        to set uid/gid */
> -                       inc_nlink(inode);
>
>                        cifs_unix_basic_to_fattr(&fattr, pInfo, cifs_sb);
>                        cifs_fill_uniqueid(inode->i_sb, &fattr);
> @@ -1355,7 +1359,6 @@ mkdir_retry_old:
>                d_drop(direntry);
>        } else {
>  mkdir_get_info:
> -               inc_nlink(inode);
>                if (pTcon->unix_ext)
>                        rc = cifs_get_inode_info_unix(&newinode, full_path,
>                                                      inode->i_sb, xid);
> @@ -1436,6 +1439,11 @@ mkdir_get_info:
>                }
>        }
>  mkdir_out:
> +       /*
> +        * Force revalidate to get parent dir info when needed since cached
> +        * attributes are invalid now.
> +        */
> +       CIFS_I(inode)->time = 0;
>        kfree(full_path);
>        FreeXid(xid);
>        cifs_put_tlink(tlink);
> @@ -1475,7 +1483,6 @@ int cifs_rmdir(struct inode *inode, struct dentry *direntry)
>        cifs_put_tlink(tlink);
>
>        if (!rc) {
> -               drop_nlink(inode);
>                spin_lock(&direntry->d_inode->i_lock);
>                i_size_write(direntry->d_inode, 0);
>                clear_nlink(direntry->d_inode);
> @@ -1483,12 +1490,15 @@ int cifs_rmdir(struct inode *inode, struct dentry *direntry)
>        }
>
>        cifsInode = CIFS_I(direntry->d_inode);
> -       cifsInode->time = 0;    /* force revalidate to go get info when
> -                                  needed */
> +       /* force revalidate to go get info when needed */
> +       cifsInode->time = 0;
>
>        cifsInode = CIFS_I(inode);
> -       cifsInode->time = 0;    /* force revalidate to get parent dir info
> -                                  since cached search results now invalid */
> +       /*
> +        * Force revalidate to get parent dir info when needed since cached
> +        * attributes are invalid now.
> +        */
> +       cifsInode->time = 0;
>
>        direntry->d_inode->i_ctime = inode->i_ctime = inode->i_mtime =
>                current_fs_time(inode->i_sb);
> --
> 1.7.1
>

What's about this? Should it go to stable too?

-- 
Best regards,
Pavel Shilovsky.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] CIFS: Fix mkdir/rmdir bug for the non-POSIX case
       [not found] ` <1329484410-30540-1-git-send-email-piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
  2012-02-21 13:26   ` Pavel Shilovsky
@ 2012-02-21 15:00   ` Jeff Layton
       [not found]     ` <20120221100006.7e43e8b9-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
  1 sibling, 1 reply; 9+ messages in thread
From: Jeff Layton @ 2012-02-21 15:00 UTC (permalink / raw)
  To: Pavel Shilovsky; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA

On Fri, 17 Feb 2012 16:13:30 +0300
Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:

> Currently we do inc/drop_nlink for a parent directory for every
> mkdir/rmdir calls. That's wrong when Unix extensions are disabled
> because in this case a server doesn't follow the same semantic and
> returns the old value on the next QueryInfo request. As the result,
> we update our value with the server one and then decrement it on
> every rmdir call - go to negative nlink values.
> 
> Fix this by removing inc/drop_nlink for the parent directory from
> mkdir/rmdir, setting it for a revalidation and ignoring NumberOfLinks
> for directories when Unix extensions are disabled.
> 
> Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> ---
>  fs/cifs/inode.c |   28 +++++++++++++++++++---------
>  1 files changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index a5f54b7..745da3d 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -534,6 +534,11 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
>  	if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
>  		fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode;
>  		fattr->cf_dtype = DT_DIR;
> +		/*
> +		 * Server can return wrong NumberOfLinks value for directories
> +		 * when Unix extensions are disabled - fake it.
> +		 */
> +		fattr->cf_nlink = 2;
>  	} else {
>  		fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode;
>  		fattr->cf_dtype = DT_REG;
> @@ -541,9 +546,9 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
>  		/* clear write bits if ATTR_READONLY is set */
>  		if (fattr->cf_cifsattrs & ATTR_READONLY)
>  			fattr->cf_mode &= ~(S_IWUGO);
> -	}
>  
> -	fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
> +		fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
> +	}
>  
>  	fattr->cf_uid = cifs_sb->mnt_uid;
>  	fattr->cf_gid = cifs_sb->mnt_gid;
> @@ -1322,7 +1327,6 @@ int cifs_mkdir(struct inode *inode, struct dentry *direntry, umode_t mode)
>  			}
>  /*BB check (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SET_UID ) to see if need
>  	to set uid/gid */
> -			inc_nlink(inode);
>  
>  			cifs_unix_basic_to_fattr(&fattr, pInfo, cifs_sb);
>  			cifs_fill_uniqueid(inode->i_sb, &fattr);
> @@ -1355,7 +1359,6 @@ mkdir_retry_old:
>  		d_drop(direntry);
>  	} else {
>  mkdir_get_info:
> -		inc_nlink(inode);
>  		if (pTcon->unix_ext)
>  			rc = cifs_get_inode_info_unix(&newinode, full_path,
>  						      inode->i_sb, xid);
> @@ -1436,6 +1439,11 @@ mkdir_get_info:
>  		}
>  	}
>  mkdir_out:
> +	/*
> +	 * Force revalidate to get parent dir info when needed since cached
> +	 * attributes are invalid now.
> +	 */
> +	CIFS_I(inode)->time = 0;
>  	kfree(full_path);
>  	FreeXid(xid);
>  	cifs_put_tlink(tlink);
> @@ -1475,7 +1483,6 @@ int cifs_rmdir(struct inode *inode, struct dentry *direntry)
>  	cifs_put_tlink(tlink);
>  
>  	if (!rc) {
> -		drop_nlink(inode);
>  		spin_lock(&direntry->d_inode->i_lock);
>  		i_size_write(direntry->d_inode, 0);
>  		clear_nlink(direntry->d_inode);
> @@ -1483,12 +1490,15 @@ int cifs_rmdir(struct inode *inode, struct dentry *direntry)
>  	}
>  
>  	cifsInode = CIFS_I(direntry->d_inode);
> -	cifsInode->time = 0;	/* force revalidate to go get info when
> -				   needed */
> +	/* force revalidate to go get info when needed */
> +	cifsInode->time = 0;
>  
>  	cifsInode = CIFS_I(inode);
> -	cifsInode->time = 0;	/* force revalidate to get parent dir info
> -				   since cached search results now invalid */
> +	/*
> +	 * Force revalidate to get parent dir info when needed since cached
> +	 * attributes are invalid now.
> +	 */
> +	cifsInode->time = 0;
>  
>  	direntry->d_inode->i_ctime = inode->i_ctime = inode->i_mtime =
>  		current_fs_time(inode->i_sb);

Looks good.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] CIFS: Fix mkdir/rmdir bug for the non-POSIX case
       [not found]     ` <20120221100006.7e43e8b9-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
@ 2012-02-25  9:11       ` Pavel Shilovsky
       [not found]         ` <CAH2r5mv-W=Y40rnAck3uQiR8BuZULv0Nsj1P=OE8o5fPFWM-zw@mail.gmail.com>
       [not found]         ` <CAKywueS0XKjuX_sLfb=Lhp=hLgrYpq7ndAtNGDjq1pFg16A7dA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 2 replies; 9+ messages in thread
From: Pavel Shilovsky @ 2012-02-25  9:11 UTC (permalink / raw)
  To: Steve French; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA, Jeff Layton

2012/2/21 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> On Fri, 17 Feb 2012 16:13:30 +0300
> Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
>
>> Currently we do inc/drop_nlink for a parent directory for every
>> mkdir/rmdir calls. That's wrong when Unix extensions are disabled
>> because in this case a server doesn't follow the same semantic and
>> returns the old value on the next QueryInfo request. As the result,
>> we update our value with the server one and then decrement it on
>> every rmdir call - go to negative nlink values.
>>
>> Fix this by removing inc/drop_nlink for the parent directory from
>> mkdir/rmdir, setting it for a revalidation and ignoring NumberOfLinks
>> for directories when Unix extensions are disabled.
>>
>> Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
>> ---
>>  fs/cifs/inode.c |   28 +++++++++++++++++++---------
>>  1 files changed, 19 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
>> index a5f54b7..745da3d 100644
>> --- a/fs/cifs/inode.c
>> +++ b/fs/cifs/inode.c
>> @@ -534,6 +534,11 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
>>       if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
>>               fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode;
>>               fattr->cf_dtype = DT_DIR;
>> +             /*
>> +              * Server can return wrong NumberOfLinks value for directories
>> +              * when Unix extensions are disabled - fake it.
>> +              */
>> +             fattr->cf_nlink = 2;
>>       } else {
>>               fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode;
>>               fattr->cf_dtype = DT_REG;
>> @@ -541,9 +546,9 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
>>               /* clear write bits if ATTR_READONLY is set */
>>               if (fattr->cf_cifsattrs & ATTR_READONLY)
>>                       fattr->cf_mode &= ~(S_IWUGO);
>> -     }
>>
>> -     fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
>> +             fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
>> +     }
>>
>>       fattr->cf_uid = cifs_sb->mnt_uid;
>>       fattr->cf_gid = cifs_sb->mnt_gid;
>> @@ -1322,7 +1327,6 @@ int cifs_mkdir(struct inode *inode, struct dentry *direntry, umode_t mode)
>>                       }
>>  /*BB check (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SET_UID ) to see if need
>>       to set uid/gid */
>> -                     inc_nlink(inode);
>>
>>                       cifs_unix_basic_to_fattr(&fattr, pInfo, cifs_sb);
>>                       cifs_fill_uniqueid(inode->i_sb, &fattr);
>> @@ -1355,7 +1359,6 @@ mkdir_retry_old:
>>               d_drop(direntry);
>>       } else {
>>  mkdir_get_info:
>> -             inc_nlink(inode);
>>               if (pTcon->unix_ext)
>>                       rc = cifs_get_inode_info_unix(&newinode, full_path,
>>                                                     inode->i_sb, xid);
>> @@ -1436,6 +1439,11 @@ mkdir_get_info:
>>               }
>>       }
>>  mkdir_out:
>> +     /*
>> +      * Force revalidate to get parent dir info when needed since cached
>> +      * attributes are invalid now.
>> +      */
>> +     CIFS_I(inode)->time = 0;
>>       kfree(full_path);
>>       FreeXid(xid);
>>       cifs_put_tlink(tlink);
>> @@ -1475,7 +1483,6 @@ int cifs_rmdir(struct inode *inode, struct dentry *direntry)
>>       cifs_put_tlink(tlink);
>>
>>       if (!rc) {
>> -             drop_nlink(inode);
>>               spin_lock(&direntry->d_inode->i_lock);
>>               i_size_write(direntry->d_inode, 0);
>>               clear_nlink(direntry->d_inode);
>> @@ -1483,12 +1490,15 @@ int cifs_rmdir(struct inode *inode, struct dentry *direntry)
>>       }
>>
>>       cifsInode = CIFS_I(direntry->d_inode);
>> -     cifsInode->time = 0;    /* force revalidate to go get info when
>> -                                needed */
>> +     /* force revalidate to go get info when needed */
>> +     cifsInode->time = 0;
>>
>>       cifsInode = CIFS_I(inode);
>> -     cifsInode->time = 0;    /* force revalidate to get parent dir info
>> -                                since cached search results now invalid */
>> +     /*
>> +      * Force revalidate to get parent dir info when needed since cached
>> +      * attributes are invalid now.
>> +      */
>> +     cifsInode->time = 0;
>>
>>       direntry->d_inode->i_ctime = inode->i_ctime = inode->i_mtime =
>>               current_fs_time(inode->i_sb);
>
> Looks good.
>
> Reviewed-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Steve, what's about this patch? I think it should go to stable as well.

-- 
Best regards,
Pavel Shilovsky.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] CIFS: Fix mkdir/rmdir bug for the non-POSIX case
       [not found]         ` <CAKywueS0XKjuX_sLfb=Lhp=hLgrYpq7ndAtNGDjq1pFg16A7dA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2012-02-25 18:42           ` Steve French
  2012-02-27 20:22           ` Jeff Layton
  1 sibling, 0 replies; 9+ messages in thread
From: Steve French @ 2012-02-25 18:42 UTC (permalink / raw)
  To: linux-cifs-u79uwXL29TY76Z2rM5mHXA

I have not rerun my normal functional tests with this in tree, but
will this weekend (before the upcoming test event).

It also would be helpful to know the state of your current smb2 test
branch so I can make sure I am working from a copy that is reasonably
uptodate when I make changes during testing next week.

On Sat, Feb 25, 2012 at 3:11 AM, Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
>
> 2012/2/21 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> > On Fri, 17 Feb 2012 16:13:30 +0300
> > Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
> >
> >> Currently we do inc/drop_nlink for a parent directory for every
> >> mkdir/rmdir calls. That's wrong when Unix extensions are disabled
> >> because in this case a server doesn't follow the same semantic and
> >> returns the old value on the next QueryInfo request. As the result,
> >> we update our value with the server one and then decrement it on
> >> every rmdir call - go to negative nlink values.
> >>
> >> Fix this by removing inc/drop_nlink for the parent directory from
> >> mkdir/rmdir, setting it for a revalidation and ignoring NumberOfLinks
> >> for directories when Unix extensions are disabled.
> >>
> >> Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> >> ---
> >>  fs/cifs/inode.c |   28 +++++++++++++++++++---------
> >>  1 files changed, 19 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> >> index a5f54b7..745da3d 100644
> >> --- a/fs/cifs/inode.c
> >> +++ b/fs/cifs/inode.c
> >> @@ -534,6 +534,11 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr,
> >> FILE_ALL_INFO *info,
> >>       if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
> >>               fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode;
> >>               fattr->cf_dtype = DT_DIR;
> >> +             /*
> >> +              * Server can return wrong NumberOfLinks value for
> >> directories
> >> +              * when Unix extensions are disabled - fake it.
> >> +              */
> >> +             fattr->cf_nlink = 2;
> >>       } else {
> >>               fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode;
> >>               fattr->cf_dtype = DT_REG;
> >> @@ -541,9 +546,9 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr,
> >> FILE_ALL_INFO *info,
> >>               /* clear write bits if ATTR_READONLY is set */
> >>               if (fattr->cf_cifsattrs & ATTR_READONLY)
> >>                       fattr->cf_mode &= ~(S_IWUGO);
> >> -     }
> >>
> >> -     fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
> >> +             fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
> >> +     }
> >>
> >>       fattr->cf_uid = cifs_sb->mnt_uid;
> >>       fattr->cf_gid = cifs_sb->mnt_gid;
> >> @@ -1322,7 +1327,6 @@ int cifs_mkdir(struct inode *inode, struct dentry
> >> *direntry, umode_t mode)
> >>                       }
> >>  /*BB check (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SET_UID ) to see if
> >> need
> >>       to set uid/gid */
> >> -                     inc_nlink(inode);
> >>
> >>                       cifs_unix_basic_to_fattr(&fattr, pInfo, cifs_sb);
> >>                       cifs_fill_uniqueid(inode->i_sb, &fattr);
> >> @@ -1355,7 +1359,6 @@ mkdir_retry_old:
> >>               d_drop(direntry);
> >>       } else {
> >>  mkdir_get_info:
> >> -             inc_nlink(inode);
> >>               if (pTcon->unix_ext)
> >>                       rc = cifs_get_inode_info_unix(&newinode,
> >> full_path,
> >>                                                     inode->i_sb, xid);
> >> @@ -1436,6 +1439,11 @@ mkdir_get_info:
> >>               }
> >>       }
> >>  mkdir_out:
> >> +     /*
> >> +      * Force revalidate to get parent dir info when needed since
> >> cached
> >> +      * attributes are invalid now.
> >> +      */
> >> +     CIFS_I(inode)->time = 0;
> >>       kfree(full_path);
> >>       FreeXid(xid);
> >>       cifs_put_tlink(tlink);
> >> @@ -1475,7 +1483,6 @@ int cifs_rmdir(struct inode *inode, struct dentry
> >> *direntry)
> >>       cifs_put_tlink(tlink);
> >>
> >>       if (!rc) {
> >> -             drop_nlink(inode);
> >>               spin_lock(&direntry->d_inode->i_lock);
> >>               i_size_write(direntry->d_inode, 0);
> >>               clear_nlink(direntry->d_inode);
> >> @@ -1483,12 +1490,15 @@ int cifs_rmdir(struct inode *inode, struct
> >> dentry *direntry)
> >>       }
> >>
> >>       cifsInode = CIFS_I(direntry->d_inode);
> >> -     cifsInode->time = 0;    /* force revalidate to go get info when
> >> -                                needed */
> >> +     /* force revalidate to go get info when needed */
> >> +     cifsInode->time = 0;
> >>
> >>       cifsInode = CIFS_I(inode);
> >> -     cifsInode->time = 0;    /* force revalidate to get parent dir
> >> info
> >> -                                since cached search results now
> >> invalid */
> >> +     /*
> >> +      * Force revalidate to get parent dir info when needed since
> >> cached
> >> +      * attributes are invalid now.
> >> +      */
> >> +     cifsInode->time = 0;
> >>
> >>       direntry->d_inode->i_ctime = inode->i_ctime = inode->i_mtime =
> >>               current_fs_time(inode->i_sb);
> >
> > Looks good.
> >
> > Reviewed-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
>
> Steve, what's about this patch? I think it should go to stable as well.
>
> --
> Best regards,
> Pavel Shilovsky.




--
Thanks,

Steve

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] CIFS: Fix mkdir/rmdir bug for the non-POSIX case
       [not found]           ` <CAH2r5mv-W=Y40rnAck3uQiR8BuZULv0Nsj1P=OE8o5fPFWM-zw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2012-02-25 21:25             ` Pavel Shilovsky
  2012-02-27  5:17             ` Steve French
  1 sibling, 0 replies; 9+ messages in thread
From: Pavel Shilovsky @ 2012-02-25 21:25 UTC (permalink / raw)
  To: Steve French; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA, Jeff Layton

2012/2/25 Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> I have not rerun my normal functional tests with this in tree, but will this
> weekend (before the upcoming test event).
>
> It also would be helpful to know the state of your current smb2 test branch
> so I can make sure I am working from a copy that is reasonably uptodate when
> I make changes during testing next week.
>
>

http://git.etersoft.ru/people/piastry/packages/?p=cifs-2.6.git;a=shortlog;h=refs/heads/smb2-dev2
branch is the latest
update of smb2 patches.

I have been working with cifs related part of the patches (preparing
transport code to handle new protocol as well) since that time (this
branch: http://git.etersoft.ru/people/piastry/packages?p=cifs-2.6.git;a=shortlog;h=refs/heads/cifs-patches)
and haven't rebased other smb2 specific patches yet. I am going to do
that soon - will let you know when it's done.

-- 
Best regards,
Pavel Shilovsky.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] CIFS: Fix mkdir/rmdir bug for the non-POSIX case
       [not found]           ` <CAH2r5mv-W=Y40rnAck3uQiR8BuZULv0Nsj1P=OE8o5fPFWM-zw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  2012-02-25 21:25             ` Pavel Shilovsky
@ 2012-02-27  5:17             ` Steve French
  1 sibling, 0 replies; 9+ messages in thread
From: Steve French @ 2012-02-27  5:17 UTC (permalink / raw)
  To: Pavel Shilovsky; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA, Jeff Layton

merged

On Sat, Feb 25, 2012 at 12:41 PM, Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> I have not rerun my normal functional tests with this in tree, but will this
> weekend (before the upcoming test event).
>
> It also would be helpful to know the state of your current smb2 test branch
> so I can make sure I am working from a copy that is reasonably uptodate when
> I make changes during testing next week.
>
>
> On Sat, Feb 25, 2012 at 3:11 AM, Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> wrote:
>>
>> 2012/2/21 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
>> > On Fri, 17 Feb 2012 16:13:30 +0300
>> > Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
>> >
>> >> Currently we do inc/drop_nlink for a parent directory for every
>> >> mkdir/rmdir calls. That's wrong when Unix extensions are disabled
>> >> because in this case a server doesn't follow the same semantic and
>> >> returns the old value on the next QueryInfo request. As the result,
>> >> we update our value with the server one and then decrement it on
>> >> every rmdir call - go to negative nlink values.
>> >>
>> >> Fix this by removing inc/drop_nlink for the parent directory from
>> >> mkdir/rmdir, setting it for a revalidation and ignoring NumberOfLinks
>> >> for directories when Unix extensions are disabled.
>> >>
>> >> Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
>> >> ---
>> >>  fs/cifs/inode.c |   28 +++++++++++++++++++---------
>> >>  1 files changed, 19 insertions(+), 9 deletions(-)
>> >>
>> >> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
>> >> index a5f54b7..745da3d 100644
>> >> --- a/fs/cifs/inode.c
>> >> +++ b/fs/cifs/inode.c
>> >> @@ -534,6 +534,11 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr,
>> >> FILE_ALL_INFO *info,
>> >>       if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
>> >>               fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode;
>> >>               fattr->cf_dtype = DT_DIR;
>> >> +             /*
>> >> +              * Server can return wrong NumberOfLinks value for
>> >> directories
>> >> +              * when Unix extensions are disabled - fake it.
>> >> +              */
>> >> +             fattr->cf_nlink = 2;
>> >>       } else {
>> >>               fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode;
>> >>               fattr->cf_dtype = DT_REG;
>> >> @@ -541,9 +546,9 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr,
>> >> FILE_ALL_INFO *info,
>> >>               /* clear write bits if ATTR_READONLY is set */
>> >>               if (fattr->cf_cifsattrs & ATTR_READONLY)
>> >>                       fattr->cf_mode &= ~(S_IWUGO);
>> >> -     }
>> >>
>> >> -     fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
>> >> +             fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
>> >> +     }
>> >>
>> >>       fattr->cf_uid = cifs_sb->mnt_uid;
>> >>       fattr->cf_gid = cifs_sb->mnt_gid;
>> >> @@ -1322,7 +1327,6 @@ int cifs_mkdir(struct inode *inode, struct dentry
>> >> *direntry, umode_t mode)
>> >>                       }
>> >>  /*BB check (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SET_UID ) to see if
>> >> need
>> >>       to set uid/gid */
>> >> -                     inc_nlink(inode);
>> >>
>> >>                       cifs_unix_basic_to_fattr(&fattr, pInfo, cifs_sb);
>> >>                       cifs_fill_uniqueid(inode->i_sb, &fattr);
>> >> @@ -1355,7 +1359,6 @@ mkdir_retry_old:
>> >>               d_drop(direntry);
>> >>       } else {
>> >>  mkdir_get_info:
>> >> -             inc_nlink(inode);
>> >>               if (pTcon->unix_ext)
>> >>                       rc = cifs_get_inode_info_unix(&newinode,
>> >> full_path,
>> >>                                                     inode->i_sb, xid);
>> >> @@ -1436,6 +1439,11 @@ mkdir_get_info:
>> >>               }
>> >>       }
>> >>  mkdir_out:
>> >> +     /*
>> >> +      * Force revalidate to get parent dir info when needed since
>> >> cached
>> >> +      * attributes are invalid now.
>> >> +      */
>> >> +     CIFS_I(inode)->time = 0;
>> >>       kfree(full_path);
>> >>       FreeXid(xid);
>> >>       cifs_put_tlink(tlink);
>> >> @@ -1475,7 +1483,6 @@ int cifs_rmdir(struct inode *inode, struct dentry
>> >> *direntry)
>> >>       cifs_put_tlink(tlink);
>> >>
>> >>       if (!rc) {
>> >> -             drop_nlink(inode);
>> >>               spin_lock(&direntry->d_inode->i_lock);
>> >>               i_size_write(direntry->d_inode, 0);
>> >>               clear_nlink(direntry->d_inode);
>> >> @@ -1483,12 +1490,15 @@ int cifs_rmdir(struct inode *inode, struct
>> >> dentry *direntry)
>> >>       }
>> >>
>> >>       cifsInode = CIFS_I(direntry->d_inode);
>> >> -     cifsInode->time = 0;    /* force revalidate to go get info when
>> >> -                                needed */
>> >> +     /* force revalidate to go get info when needed */
>> >> +     cifsInode->time = 0;
>> >>
>> >>       cifsInode = CIFS_I(inode);
>> >> -     cifsInode->time = 0;    /* force revalidate to get parent dir
>> >> info
>> >> -                                since cached search results now
>> >> invalid */
>> >> +     /*
>> >> +      * Force revalidate to get parent dir info when needed since
>> >> cached
>> >> +      * attributes are invalid now.
>> >> +      */
>> >> +     cifsInode->time = 0;
>> >>
>> >>       direntry->d_inode->i_ctime = inode->i_ctime = inode->i_mtime =
>> >>               current_fs_time(inode->i_sb);
>> >
>> > Looks good.
>> >
>> > Reviewed-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
>>
>> Steve, what's about this patch? I think it should go to stable as well.
>>
>> --
>> Best regards,
>> Pavel Shilovsky.
>
>
>
>
> --
> Thanks,
>
> Steve



-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] CIFS: Fix mkdir/rmdir bug for the non-POSIX case
       [not found]         ` <CAKywueS0XKjuX_sLfb=Lhp=hLgrYpq7ndAtNGDjq1pFg16A7dA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  2012-02-25 18:42           ` Steve French
@ 2012-02-27 20:22           ` Jeff Layton
       [not found]             ` <20120227152242.2945558a-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
  1 sibling, 1 reply; 9+ messages in thread
From: Jeff Layton @ 2012-02-27 20:22 UTC (permalink / raw)
  To: Pavel Shilovsky; +Cc: Steve French, linux-cifs-u79uwXL29TY76Z2rM5mHXA

On Sat, 25 Feb 2012 13:11:26 +0400
Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:

> 2012/2/21 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> > On Fri, 17 Feb 2012 16:13:30 +0300
> > Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
> >
> >> Currently we do inc/drop_nlink for a parent directory for every
> >> mkdir/rmdir calls. That's wrong when Unix extensions are disabled
> >> because in this case a server doesn't follow the same semantic and
> >> returns the old value on the next QueryInfo request. As the result,
> >> we update our value with the server one and then decrement it on
> >> every rmdir call - go to negative nlink values.
> >>
> >> Fix this by removing inc/drop_nlink for the parent directory from
> >> mkdir/rmdir, setting it for a revalidation and ignoring NumberOfLinks
> >> for directories when Unix extensions are disabled.
> >>
> >> Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> >> ---
> >>  fs/cifs/inode.c |   28 +++++++++++++++++++---------
> >>  1 files changed, 19 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> >> index a5f54b7..745da3d 100644
> >> --- a/fs/cifs/inode.c
> >> +++ b/fs/cifs/inode.c
> >> @@ -534,6 +534,11 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
> >>       if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
> >>               fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode;
> >>               fattr->cf_dtype = DT_DIR;
> >> +             /*
> >> +              * Server can return wrong NumberOfLinks value for directories
> >> +              * when Unix extensions are disabled - fake it.
> >> +              */
> >> +             fattr->cf_nlink = 2;
> >>       } else {
> >>               fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode;
> >>               fattr->cf_dtype = DT_REG;
> >> @@ -541,9 +546,9 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
> >>               /* clear write bits if ATTR_READONLY is set */
> >>               if (fattr->cf_cifsattrs & ATTR_READONLY)
> >>                       fattr->cf_mode &= ~(S_IWUGO);
> >> -     }
> >>
> >> -     fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
> >> +             fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
> >> +     }
> >>
> >>       fattr->cf_uid = cifs_sb->mnt_uid;
> >>       fattr->cf_gid = cifs_sb->mnt_gid;
> >> @@ -1322,7 +1327,6 @@ int cifs_mkdir(struct inode *inode, struct dentry *direntry, umode_t mode)
> >>                       }
> >>  /*BB check (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SET_UID ) to see if need
> >>       to set uid/gid */
> >> -                     inc_nlink(inode);
> >>
> >>                       cifs_unix_basic_to_fattr(&fattr, pInfo, cifs_sb);
> >>                       cifs_fill_uniqueid(inode->i_sb, &fattr);
> >> @@ -1355,7 +1359,6 @@ mkdir_retry_old:
> >>               d_drop(direntry);
> >>       } else {
> >>  mkdir_get_info:
> >> -             inc_nlink(inode);
> >>               if (pTcon->unix_ext)
> >>                       rc = cifs_get_inode_info_unix(&newinode, full_path,
> >>                                                     inode->i_sb, xid);
> >> @@ -1436,6 +1439,11 @@ mkdir_get_info:
> >>               }
> >>       }
> >>  mkdir_out:
> >> +     /*
> >> +      * Force revalidate to get parent dir info when needed since cached
> >> +      * attributes are invalid now.
> >> +      */
> >> +     CIFS_I(inode)->time = 0;
> >>       kfree(full_path);
> >>       FreeXid(xid);
> >>       cifs_put_tlink(tlink);
> >> @@ -1475,7 +1483,6 @@ int cifs_rmdir(struct inode *inode, struct dentry *direntry)
> >>       cifs_put_tlink(tlink);
> >>
> >>       if (!rc) {
> >> -             drop_nlink(inode);
> >>               spin_lock(&direntry->d_inode->i_lock);
> >>               i_size_write(direntry->d_inode, 0);
> >>               clear_nlink(direntry->d_inode);
> >> @@ -1483,12 +1490,15 @@ int cifs_rmdir(struct inode *inode, struct dentry *direntry)
> >>       }
> >>
> >>       cifsInode = CIFS_I(direntry->d_inode);
> >> -     cifsInode->time = 0;    /* force revalidate to go get info when
> >> -                                needed */
> >> +     /* force revalidate to go get info when needed */
> >> +     cifsInode->time = 0;
> >>
> >>       cifsInode = CIFS_I(inode);
> >> -     cifsInode->time = 0;    /* force revalidate to get parent dir info
> >> -                                since cached search results now invalid */
> >> +     /*
> >> +      * Force revalidate to get parent dir info when needed since cached
> >> +      * attributes are invalid now.
> >> +      */
> >> +     cifsInode->time = 0;
> >>
> >>       direntry->d_inode->i_ctime = inode->i_ctime = inode->i_mtime =
> >>               current_fs_time(inode->i_sb);
> >
> > Looks good.
> >
> > Reviewed-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> 
> Steve, what's about this patch? I think it should go to stable as well.
> 

I think we probably ought to wait on stable. There is some (small)
potential for regression from this change. AFAIK, this is just fixing a
WARN_ON, so I think that there's no real urgency to stick it into
stable just yet.

We can always reconsider that later too if it turns out that there's
need to do so.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] CIFS: Fix mkdir/rmdir bug for the non-POSIX case
       [not found]             ` <20120227152242.2945558a-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
@ 2012-02-27 22:21               ` Steve French
  0 siblings, 0 replies; 9+ messages in thread
From: Steve French @ 2012-02-27 22:21 UTC (permalink / raw)
  To: Jeff Layton; +Cc: Pavel Shilovsky, linux-cifs-u79uwXL29TY76Z2rM5mHXA

On Mon, Feb 27, 2012 at 2:22 PM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> On Sat, 25 Feb 2012 13:11:26 +0400
> Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
>
>> 2012/2/21 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
>> > On Fri, 17 Feb 2012 16:13:30 +0300
>> > Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
>> >
>> >> Currently we do inc/drop_nlink for a parent directory for every
>> >> mkdir/rmdir calls. That's wrong when Unix extensions are disabled
>> >> because in this case a server doesn't follow the same semantic and
>> >> returns the old value on the next QueryInfo request. As the result,
>> >> we update our value with the server one and then decrement it on
>> >> every rmdir call - go to negative nlink values.
>> >>
>> >> Fix this by removing inc/drop_nlink for the parent directory from
>> >> mkdir/rmdir, setting it for a revalidation and ignoring NumberOfLinks
>> >> for directories when Unix extensions are disabled.
>> >>
>> >> Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
>> >> ---
>> >>  fs/cifs/inode.c |   28 +++++++++++++++++++---------
>> >>  1 files changed, 19 insertions(+), 9 deletions(-)
>> >>
>> >> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
>> >> index a5f54b7..745da3d 100644
>> >> --- a/fs/cifs/inode.c
>> >> +++ b/fs/cifs/inode.c
>> >> @@ -534,6 +534,11 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
>> >>       if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
>> >>               fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode;
>> >>               fattr->cf_dtype = DT_DIR;
>> >> +             /*
>> >> +              * Server can return wrong NumberOfLinks value for directories
>> >> +              * when Unix extensions are disabled - fake it.
>> >> +              */
>> >> +             fattr->cf_nlink = 2;
>> >>       } else {
>> >>               fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode;
>> >>               fattr->cf_dtype = DT_REG;
>> >> @@ -541,9 +546,9 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
>> >>               /* clear write bits if ATTR_READONLY is set */
>> >>               if (fattr->cf_cifsattrs & ATTR_READONLY)
>> >>                       fattr->cf_mode &= ~(S_IWUGO);
>> >> -     }
>> >>
>> >> -     fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
>> >> +             fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
>> >> +     }
>> >>
>> >>       fattr->cf_uid = cifs_sb->mnt_uid;
>> >>       fattr->cf_gid = cifs_sb->mnt_gid;
>> >> @@ -1322,7 +1327,6 @@ int cifs_mkdir(struct inode *inode, struct dentry *direntry, umode_t mode)
>> >>                       }
>> >>  /*BB check (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SET_UID ) to see if need
>> >>       to set uid/gid */
>> >> -                     inc_nlink(inode);
>> >>
>> >>                       cifs_unix_basic_to_fattr(&fattr, pInfo, cifs_sb);
>> >>                       cifs_fill_uniqueid(inode->i_sb, &fattr);
>> >> @@ -1355,7 +1359,6 @@ mkdir_retry_old:
>> >>               d_drop(direntry);
>> >>       } else {
>> >>  mkdir_get_info:
>> >> -             inc_nlink(inode);
>> >>               if (pTcon->unix_ext)
>> >>                       rc = cifs_get_inode_info_unix(&newinode, full_path,
>> >>                                                     inode->i_sb, xid);
>> >> @@ -1436,6 +1439,11 @@ mkdir_get_info:
>> >>               }
>> >>       }
>> >>  mkdir_out:
>> >> +     /*
>> >> +      * Force revalidate to get parent dir info when needed since cached
>> >> +      * attributes are invalid now.
>> >> +      */
>> >> +     CIFS_I(inode)->time = 0;
>> >>       kfree(full_path);
>> >>       FreeXid(xid);
>> >>       cifs_put_tlink(tlink);
>> >> @@ -1475,7 +1483,6 @@ int cifs_rmdir(struct inode *inode, struct dentry *direntry)
>> >>       cifs_put_tlink(tlink);
>> >>
>> >>       if (!rc) {
>> >> -             drop_nlink(inode);
>> >>               spin_lock(&direntry->d_inode->i_lock);
>> >>               i_size_write(direntry->d_inode, 0);
>> >>               clear_nlink(direntry->d_inode);
>> >> @@ -1483,12 +1490,15 @@ int cifs_rmdir(struct inode *inode, struct dentry *direntry)
>> >>       }
>> >>
>> >>       cifsInode = CIFS_I(direntry->d_inode);
>> >> -     cifsInode->time = 0;    /* force revalidate to go get info when
>> >> -                                needed */
>> >> +     /* force revalidate to go get info when needed */
>> >> +     cifsInode->time = 0;
>> >>
>> >>       cifsInode = CIFS_I(inode);
>> >> -     cifsInode->time = 0;    /* force revalidate to get parent dir info
>> >> -                                since cached search results now invalid */
>> >> +     /*
>> >> +      * Force revalidate to get parent dir info when needed since cached
>> >> +      * attributes are invalid now.
>> >> +      */
>> >> +     cifsInode->time = 0;
>> >>
>> >>       direntry->d_inode->i_ctime = inode->i_ctime = inode->i_mtime =
>> >>               current_fs_time(inode->i_sb);
>> >
>> > Looks good.
>> >
>> > Reviewed-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
>>
>> Steve, what's about this patch? I think it should go to stable as well.
>>
>
> I think we probably ought to wait on stable. There is some (small)
> potential for regression from this change. AFAIK, this is just fixing a
> WARN_ON, so I think that there's no real urgency to stick it into
> stable just yet.
>
> We can always reconsider that later too if it turns out that there's
> need to do so.

Makes sense.  Can wait on stable.

-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2012-02-27 22:21 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-02-17 13:13 [PATCH v2] CIFS: Fix mkdir/rmdir bug for the non-POSIX case Pavel Shilovsky
     [not found] ` <1329484410-30540-1-git-send-email-piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
2012-02-21 13:26   ` Pavel Shilovsky
2012-02-21 15:00   ` Jeff Layton
     [not found]     ` <20120221100006.7e43e8b9-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
2012-02-25  9:11       ` Pavel Shilovsky
     [not found]         ` <CAH2r5mv-W=Y40rnAck3uQiR8BuZULv0Nsj1P=OE8o5fPFWM-zw@mail.gmail.com>
     [not found]           ` <CAH2r5mv-W=Y40rnAck3uQiR8BuZULv0Nsj1P=OE8o5fPFWM-zw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2012-02-25 21:25             ` Pavel Shilovsky
2012-02-27  5:17             ` Steve French
     [not found]         ` <CAKywueS0XKjuX_sLfb=Lhp=hLgrYpq7ndAtNGDjq1pFg16A7dA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2012-02-25 18:42           ` Steve French
2012-02-27 20:22           ` Jeff Layton
     [not found]             ` <20120227152242.2945558a-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
2012-02-27 22:21               ` Steve French

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.