[linux-cifs] Re: [PATCH v2] CIFS: Fix mkdir/rmdir bug for the non-POSIX case


On Mon, Feb 27, 2012 at 2:22 PM, Jeff Layton  wrote:
> On Sat, 25 Feb 2012 13:11:26 +0400
> Pavel Shilovsky  wrote:
>
>> 2012/2/21 Jeff Layton :
>> > On Fri, 17 Feb 2012 16:13:30 +0300
>> > Pavel Shilovsky  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 
>> >> ---
>> >> 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 
>>
>> 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

This message from: http://www.mailbrowse.com/linux-cifs/5536.html
Previous message: Re: [linux-cifs-client] [PATCH] cifs: hard mount option behaviour implementation
Next message:Re: [PATCH 01/11] CIFS: Simplify inFlight logic