On Sat, 7 Sep 2013 14:49:24 -0400
Jim McDonough <jmcd@xxxxxxxxx> wrote:
> Here's my second try. I hope there aren't missed ops which update the
> inode without having nlink updated. At worst, they should stop
> clobbering valid values, which was definitely happening before.
>
> Signed-off-by: Jim McDonough <jmcdonough@xxxxxxx>
>
> ---
> fs/cifs/cifsglob.h | 1 +
> fs/cifs/inode.c | 21 ++++++++++++++++++++-
> fs/cifs/readdir.c | 3 +++
> 3 files changed, 24 insertions(+), 1 deletion(-)
>
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -712,6 +712,7 @@ struct dfs_info3_param {
> #define CIFS_FATTR_DELETE_PENDING 0x2
> #define CIFS_FATTR_NEED_REVAL 0x4
> #define CIFS_FATTR_INO_COLLISION 0x8
> +#define CIFS_FATTR_MISSING_NLINK 0x10
>
> struct cifs_fattr {
> u32 cf_flags;
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -118,6 +118,25 @@ cifs_revalidate_cache(struct inode *inod
> cifs_i->invalid_mapping = true;
> }
>
> +/* copy nlink to the inode, unless it wasn't provided. Provide
> + sane values if we don't have an existing one and none was provided */
> +static void
> +cifs_nlink_fattr_to_inode(struct inode *inode, struct cifs_fattr *fattr)
> +{
> + if (fattr->cf_nlink) {
> + inode->i_nlink = fattr->cf_nlink;
> + return;
> + }
> +
I think it would be best to get rid of the above if block and add an
else block to the end of the following if clause that just sets
inode->i_nlink = fattr->cf_nlink unconditionally.
The problem with the logic here is that you're ignoring the
(legitimate) case where cf_nlink might be 0 as reported by the server.
> + if ((fattr->cf_flags & CIFS_FATTR_MISSING_NLINK) &&
> + (inode->i_state & I_NEW)) {
> + if (fattr->cf_cifsattrs & ATTR_DIRECTORY)
> + inode->i_nlink = 2;
> + else
> + inode->i_nlink = 1;
> + }
> +}
> +
> /* populate an inode with info from a cifs_fattr struct */
> void
> cifs_fattr_to_inode(struct inode *inode, struct cifs_fattr *fattr)
> @@ -132,7 +151,7 @@ cifs_fattr_to_inode(struct inode *inode,
> inode->i_mtime = fattr->cf_mtime;
> inode->i_ctime = fattr->cf_ctime;
> inode->i_rdev = fattr->cf_rdev;
> - inode->i_nlink = fattr->cf_nlink;
> + cifs_nlink_fattr_to_inode(inode, fattr);
> inode->i_uid = fattr->cf_uid;
> inode->i_gid = fattr->cf_gid;
>
> --- a/fs/cifs/readdir.c
> +++ b/fs/cifs/readdir.c
> @@ -130,6 +130,9 @@ cifs_fill_common_info(struct cifs_fattr
> fattr->cf_dtype = DT_REG;
> }
>
> + /* non-unix readdir doesn't provide nlink */
> + fattr->cf_flags &= CIFS_FATTR_MISSING_NLINK;
> +
Shouldn't that be "|=" ?
> if (fattr->cf_cifsattrs & ATTR_READONLY)
> fattr->cf_mode &= ~S_IWUGO;
>
>
>
>
The only other thing I see is that the code in cifs_all_info_to_fattr
ought to be changed to use this flag and not try to fake up the
cf_nlink value.
--
Jeff Layton <jlayton@xxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
