[PATCH 0/3] cifs: tighten up cifs_iget matching criteria

[PATCH 0/3] cifs: tighten up cifs_iget matching criteria

[Jeff Layton]

These patches are intended to help reduce the number of false matches
that cifs_iget does. They simply try to make the code do some extra
checking of the cached inodes against the new file's attributes when a
uniqueid matches one that's already in cache.

I've tested these against samba and windows. They seem to work and help
reduce the false positives that can cause server inode numbers to be
disabled.

This patchset is intended for 2.6.36. The patches are in the cifs-2.6.36
branch of my kernel.org tree:

http://git.kernel.org/?p=linux/kernel/git/jlayton/linux.git;a=shortlog;h=refs/heads/cifs-2.6.36

Jeff Layton (3):
  cifs: don't allow cifs_iget to match inodes of the wrong type
  cifs: use CreationTime like an i_generation field
  cifs: reduce false positives with inode aliasing serverino
    autodisable

 fs/cifs/cifsfs.c   |    2 +
 fs/cifs/cifsglob.h |    2 +
 fs/cifs/inode.c    |   54 ++++++++++++++++++++++++++++++++++++++++-----------
 fs/cifs/readdir.c  |    1 +
 4 files changed, 47 insertions(+), 12 deletions(-)

[PATCH 1/3] cifs: don't allow cifs_iget to match inodes of the wrong type

[Jeff Layton]

If the type is different from what we think it should be, then don't
match the existing inode.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/cifs/inode.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 6f0683c..f64b95f 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -723,9 +723,14 @@ cifs_find_inode(struct inode *inode, void *opaque)
 {
 	struct cifs_fattr *fattr = (struct cifs_fattr *) opaque;
 
+	/* don't match inode with different uniqueid */
 	if (CIFS_I(inode)->uniqueid != fattr->cf_uniqueid)
 		return 0;
 
+	/* don't match inode of different type */
+	if ((inode->i_mode & S_IFMT) != (fattr->cf_mode & S_IFMT))
+		return 0;
+
 	/*
 	 * uh oh -- it's a directory. We can't use it since hardlinked dirs are
 	 * verboten. Disable serverino and return it as if it were found, the
-- 
1.6.6.1

[PATCH 2/3] cifs: use CreationTime like an i_generation field

[Jeff Layton]

Reduce false inode collisions by using the CreationTime like an
i_generation field. This way, even if the server ends up reusing
a uniqueid after a delete/create cycle, we can avoid matching
the inode incorrectly.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifsfs.c   |    2 ++
 fs/cifs/cifsglob.h |    2 ++
 fs/cifs/inode.c    |    6 ++++++
 fs/cifs/readdir.c  |    1 +
 4 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 484e52b..e409216 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -314,6 +314,8 @@ cifs_alloc_inode(struct super_block *sb)
 	cifs_inode->invalid_mapping = false;
 	cifs_inode->vfs_inode.i_blkbits = 14;  /* 2**14 = CIFS_MAX_MSGSIZE */
 	cifs_inode->server_eof = 0;
+	cifs_inode->uniqueid = 0;
+	cifs_inode->createtime = 0;
 
 	/* Can not set i_flags here - they get immediately overwritten
 	   to zero by the VFS */
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index e15d7a5..a0fd2c2 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -382,6 +382,7 @@ struct cifsInodeInfo {
 	bool invalid_mapping:1;		/* pagecache is invalid */
 	u64  server_eof;		/* current file size on server */
 	u64  uniqueid;			/* server inode number */
+	u64  createtime;		/* creation time on server */
 	struct inode vfs_inode;
 };
 
@@ -499,6 +500,7 @@ struct cifs_fattr {
 	u64		cf_uniqueid;
 	u64		cf_eof;
 	u64		cf_bytes;
+	u64		cf_createtime;
 	uid_t		cf_uid;
 	gid_t		cf_gid;
 	umode_t		cf_mode;
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index f64b95f..35e2466 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -487,6 +487,7 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
 
 	fattr->cf_eof = le64_to_cpu(info->EndOfFile);
 	fattr->cf_bytes = le64_to_cpu(info->AllocationSize);
+	fattr->cf_createtime = le64_to_cpu(info->CreationTime);
 
 	if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
 		fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode;
@@ -727,6 +728,10 @@ cifs_find_inode(struct inode *inode, void *opaque)
 	if (CIFS_I(inode)->uniqueid != fattr->cf_uniqueid)
 		return 0;
 
+	/* use createtime like an i_generation field */
+	if (CIFS_I(inode)->createtime != fattr->cf_createtime)
+		return 0;
+
 	/* don't match inode of different type */
 	if ((inode->i_mode & S_IFMT) != (fattr->cf_mode & S_IFMT))
 		return 0;
@@ -750,6 +755,7 @@ cifs_init_inode(struct inode *inode, void *opaque)
 	struct cifs_fattr *fattr = (struct cifs_fattr *) opaque;
 
 	CIFS_I(inode)->uniqueid = fattr->cf_uniqueid;
+	CIFS_I(inode)->createtime = fattr->cf_createtime;
 	return 0;
 }
 
diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
index daf1753..8bd18be 100644
--- a/fs/cifs/readdir.c
+++ b/fs/cifs/readdir.c
@@ -160,6 +160,7 @@ cifs_dir_info_to_fattr(struct cifs_fattr *fattr, FILE_DIRECTORY_INFO *info,
 	fattr->cf_cifsattrs = le32_to_cpu(info->ExtFileAttributes);
 	fattr->cf_eof = le64_to_cpu(info->EndOfFile);
 	fattr->cf_bytes = le64_to_cpu(info->AllocationSize);
+	fattr->cf_createtime = le64_to_cpu(info->CreationTime);
 	fattr->cf_atime = cifs_NTtimeToUnix(info->LastAccessTime);
 	fattr->cf_ctime = cifs_NTtimeToUnix(info->ChangeTime);
 	fattr->cf_mtime = cifs_NTtimeToUnix(info->LastWriteTime);
-- 
1.6.6.1

Re: [PATCH 2/3] cifs: use CreationTime like an i_generation field

[Suresh Jayaraman]

On 06/28/2010 04:40 PM, Jeff Layton wrote:
> Reduce false inode collisions by using the CreationTime like an
> i_generation field. This way, even if the server ends up reusing
> a uniqueid after a delete/create cycle, we can avoid matching
> the inode incorrectly.
> 
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/cifsfs.c   |    2 ++
>  fs/cifs/cifsglob.h |    2 ++
>  fs/cifs/inode.c    |    6 ++++++
>  fs/cifs/readdir.c  |    1 +
>  4 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 484e52b..e409216 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -314,6 +314,8 @@ cifs_alloc_inode(struct super_block *sb)
>  	cifs_inode->invalid_mapping = false;
>  	cifs_inode->vfs_inode.i_blkbits = 14;  /* 2**14 = CIFS_MAX_MSGSIZE */
>  	cifs_inode->server_eof = 0;
> +	cifs_inode->uniqueid = 0;
> +	cifs_inode->createtime = 0;
>  
>  	/* Can not set i_flags here - they get immediately overwritten
>  	   to zero by the VFS */
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index e15d7a5..a0fd2c2 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -382,6 +382,7 @@ struct cifsInodeInfo {
>  	bool invalid_mapping:1;		/* pagecache is invalid */
>  	u64  server_eof;		/* current file size on server */
>  	u64  uniqueid;			/* server inode number */
> +	u64  createtime;		/* creation time on server */
>  	struct inode vfs_inode;
>  };
>  
> @@ -499,6 +500,7 @@ struct cifs_fattr {
>  	u64		cf_uniqueid;
>  	u64		cf_eof;
>  	u64		cf_bytes;
> +	u64		cf_createtime;
>  	uid_t		cf_uid;
>  	gid_t		cf_gid;
>  	umode_t		cf_mode;
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index f64b95f..35e2466 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -487,6 +487,7 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
>  
>  	fattr->cf_eof = le64_to_cpu(info->EndOfFile);
>  	fattr->cf_bytes = le64_to_cpu(info->AllocationSize);
> +	fattr->cf_createtime = le64_to_cpu(info->CreationTime);
>  
>  	if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
>  		fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode;
> @@ -727,6 +728,10 @@ cifs_find_inode(struct inode *inode, void *opaque)
>  	if (CIFS_I(inode)->uniqueid != fattr->cf_uniqueid)
>  		return 0;
>  
> +	/* use createtime like an i_generation field */
> +	if (CIFS_I(inode)->createtime != fattr->cf_createtime)
> +		return 0;
> +
>  	/* don't match inode of different type */
>  	if ((inode->i_mode & S_IFMT) != (fattr->cf_mode & S_IFMT))
>  		return 0;
> @@ -750,6 +755,7 @@ cifs_init_inode(struct inode *inode, void *opaque)
>  	struct cifs_fattr *fattr = (struct cifs_fattr *) opaque;
>  
>  	CIFS_I(inode)->uniqueid = fattr->cf_uniqueid;
> +	CIFS_I(inode)->createtime = fattr->cf_createtime;
>  	return 0;
>  }
>  
> diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
> index daf1753..8bd18be 100644
> --- a/fs/cifs/readdir.c
> +++ b/fs/cifs/readdir.c
> @@ -160,6 +160,7 @@ cifs_dir_info_to_fattr(struct cifs_fattr *fattr, FILE_DIRECTORY_INFO *info,
>  	fattr->cf_cifsattrs = le32_to_cpu(info->ExtFileAttributes);
>  	fattr->cf_eof = le64_to_cpu(info->EndOfFile);
>  	fattr->cf_bytes = le64_to_cpu(info->AllocationSize);
> +	fattr->cf_createtime = le64_to_cpu(info->CreationTime);

cifs_std_info_to_fattr() needs this too?

>  	fattr->cf_atime = cifs_NTtimeToUnix(info->LastAccessTime);
>  	fattr->cf_ctime = cifs_NTtimeToUnix(info->ChangeTime);
>  	fattr->cf_mtime = cifs_NTtimeToUnix(info->LastWriteTime);


-- 
Suresh Jayaraman

Re: [PATCH 2/3] cifs: use CreationTime like an i_generation field

[Jeff Layton]

On Mon, 28 Jun 2010 18:56:09 +0530
Suresh Jayaraman <sjayaraman@suse.de> wrote:

> On 06/28/2010 04:40 PM, Jeff Layton wrote:
> > Reduce false inode collisions by using the CreationTime like an
> > i_generation field. This way, even if the server ends up reusing
> > a uniqueid after a delete/create cycle, we can avoid matching
> > the inode incorrectly.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  fs/cifs/cifsfs.c   |    2 ++
> >  fs/cifs/cifsglob.h |    2 ++
> >  fs/cifs/inode.c    |    6 ++++++
> >  fs/cifs/readdir.c  |    1 +
> >  4 files changed, 11 insertions(+), 0 deletions(-)
> > 
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index 484e52b..e409216 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -314,6 +314,8 @@ cifs_alloc_inode(struct super_block *sb)
> >  	cifs_inode->invalid_mapping = false;
> >  	cifs_inode->vfs_inode.i_blkbits = 14;  /* 2**14 = CIFS_MAX_MSGSIZE */
> >  	cifs_inode->server_eof = 0;
> > +	cifs_inode->uniqueid = 0;
> > +	cifs_inode->createtime = 0;
> >  
> >  	/* Can not set i_flags here - they get immediately overwritten
> >  	   to zero by the VFS */
> > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > index e15d7a5..a0fd2c2 100644
> > --- a/fs/cifs/cifsglob.h
> > +++ b/fs/cifs/cifsglob.h
> > @@ -382,6 +382,7 @@ struct cifsInodeInfo {
> >  	bool invalid_mapping:1;		/* pagecache is invalid */
> >  	u64  server_eof;		/* current file size on server */
> >  	u64  uniqueid;			/* server inode number */
> > +	u64  createtime;		/* creation time on server */
> >  	struct inode vfs_inode;
> >  };
> >  
> > @@ -499,6 +500,7 @@ struct cifs_fattr {
> >  	u64		cf_uniqueid;
> >  	u64		cf_eof;
> >  	u64		cf_bytes;
> > +	u64		cf_createtime;
> >  	uid_t		cf_uid;
> >  	gid_t		cf_gid;
> >  	umode_t		cf_mode;
> > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> > index f64b95f..35e2466 100644
> > --- a/fs/cifs/inode.c
> > +++ b/fs/cifs/inode.c
> > @@ -487,6 +487,7 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
> >  
> >  	fattr->cf_eof = le64_to_cpu(info->EndOfFile);
> >  	fattr->cf_bytes = le64_to_cpu(info->AllocationSize);
> > +	fattr->cf_createtime = le64_to_cpu(info->CreationTime);
> >  
> >  	if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
> >  		fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode;
> > @@ -727,6 +728,10 @@ cifs_find_inode(struct inode *inode, void *opaque)
> >  	if (CIFS_I(inode)->uniqueid != fattr->cf_uniqueid)
> >  		return 0;
> >  
> > +	/* use createtime like an i_generation field */
> > +	if (CIFS_I(inode)->createtime != fattr->cf_createtime)
> > +		return 0;
> > +
> >  	/* don't match inode of different type */
> >  	if ((inode->i_mode & S_IFMT) != (fattr->cf_mode & S_IFMT))
> >  		return 0;
> > @@ -750,6 +755,7 @@ cifs_init_inode(struct inode *inode, void *opaque)
> >  	struct cifs_fattr *fattr = (struct cifs_fattr *) opaque;
> >  
> >  	CIFS_I(inode)->uniqueid = fattr->cf_uniqueid;
> > +	CIFS_I(inode)->createtime = fattr->cf_createtime;
> >  	return 0;
> >  }
> >  
> > diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
> > index daf1753..8bd18be 100644
> > --- a/fs/cifs/readdir.c
> > +++ b/fs/cifs/readdir.c
> > @@ -160,6 +160,7 @@ cifs_dir_info_to_fattr(struct cifs_fattr *fattr, FILE_DIRECTORY_INFO *info,
> >  	fattr->cf_cifsattrs = le32_to_cpu(info->ExtFileAttributes);
> >  	fattr->cf_eof = le64_to_cpu(info->EndOfFile);
> >  	fattr->cf_bytes = le64_to_cpu(info->AllocationSize);
> > +	fattr->cf_createtime = le64_to_cpu(info->CreationTime);
> 
> cifs_std_info_to_fattr() needs this too?
> 


I don't think so. If we're using that function then we're probably
going to be using the legacy SMBQueryInformation calls too. Those do
not return a CreateTime and it ends up being set to 0. This *should*
do the right thing I think...

It would be nice to do this in a more definite way somehow though. The
legacy QPathInfo codepath is a bit of a mess...

> >  	fattr->cf_atime = cifs_NTtimeToUnix(info->LastAccessTime);
> >  	fattr->cf_ctime = cifs_NTtimeToUnix(info->ChangeTime);
> >  	fattr->cf_mtime = cifs_NTtimeToUnix(info->LastWriteTime);
> 
> 


-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 2/3] cifs: use CreationTime like an i_generation field

[Jeff Layton]

On Mon, 28 Jun 2010 07:10:12 -0400
Jeff Layton <jlayton@redhat.com> wrote:

> Reduce false inode collisions by using the CreationTime like an
> i_generation field. This way, even if the server ends up reusing
> a uniqueid after a delete/create cycle, we can avoid matching
> the inode incorrectly.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/cifs/cifsfs.c   |    2 ++
>  fs/cifs/cifsglob.h |    2 ++
>  fs/cifs/inode.c    |    6 ++++++
>  fs/cifs/readdir.c  |    1 +
>  4 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 484e52b..e409216 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -314,6 +314,8 @@ cifs_alloc_inode(struct super_block *sb)
>  	cifs_inode->invalid_mapping = false;
>  	cifs_inode->vfs_inode.i_blkbits = 14;  /* 2**14 = CIFS_MAX_MSGSIZE */
>  	cifs_inode->server_eof = 0;
> +	cifs_inode->uniqueid = 0;
> +	cifs_inode->createtime = 0;
>  
>  	/* Can not set i_flags here - they get immediately overwritten
>  	   to zero by the VFS */
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index e15d7a5..a0fd2c2 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -382,6 +382,7 @@ struct cifsInodeInfo {
>  	bool invalid_mapping:1;		/* pagecache is invalid */
>  	u64  server_eof;		/* current file size on server */
>  	u64  uniqueid;			/* server inode number */
> +	u64  createtime;		/* creation time on server */
>  	struct inode vfs_inode;
>  };
>  
> @@ -499,6 +500,7 @@ struct cifs_fattr {
>  	u64		cf_uniqueid;
>  	u64		cf_eof;
>  	u64		cf_bytes;
> +	u64		cf_createtime;
>  	uid_t		cf_uid;
>  	gid_t		cf_gid;
>  	umode_t		cf_mode;
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index f64b95f..35e2466 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -487,6 +487,7 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
>  
>  	fattr->cf_eof = le64_to_cpu(info->EndOfFile);
>  	fattr->cf_bytes = le64_to_cpu(info->AllocationSize);
> +	fattr->cf_createtime = le64_to_cpu(info->CreationTime);
>  
>  	if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
>  		fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode;
> @@ -727,6 +728,10 @@ cifs_find_inode(struct inode *inode, void *opaque)
>  	if (CIFS_I(inode)->uniqueid != fattr->cf_uniqueid)
>  		return 0;
>  
> +	/* use createtime like an i_generation field */
> +	if (CIFS_I(inode)->createtime != fattr->cf_createtime)
> +		return 0;
> +
>  	/* don't match inode of different type */
>  	if ((inode->i_mode & S_IFMT) != (fattr->cf_mode & S_IFMT))
>  		return 0;
> @@ -750,6 +755,7 @@ cifs_init_inode(struct inode *inode, void *opaque)
>  	struct cifs_fattr *fattr = (struct cifs_fattr *) opaque;
>  
>  	CIFS_I(inode)->uniqueid = fattr->cf_uniqueid;
> +	CIFS_I(inode)->createtime = fattr->cf_createtime;
>  	return 0;
>  }
>  
> diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
> index daf1753..8bd18be 100644
> --- a/fs/cifs/readdir.c
> +++ b/fs/cifs/readdir.c
> @@ -160,6 +160,7 @@ cifs_dir_info_to_fattr(struct cifs_fattr *fattr, FILE_DIRECTORY_INFO *info,
>  	fattr->cf_cifsattrs = le32_to_cpu(info->ExtFileAttributes);
>  	fattr->cf_eof = le64_to_cpu(info->EndOfFile);
>  	fattr->cf_bytes = le64_to_cpu(info->AllocationSize);
> +	fattr->cf_createtime = le64_to_cpu(info->CreationTime);
>  	fattr->cf_atime = cifs_NTtimeToUnix(info->LastAccessTime);
>  	fattr->cf_ctime = cifs_NTtimeToUnix(info->ChangeTime);
>  	fattr->cf_mtime = cifs_NTtimeToUnix(info->LastWriteTime);

Steve, can you clarify where we are with this patch? I originally sent
this back in June. You and I discussed this at the CIFS plugfest and I
had thought you had decided to merge it. It's still not in your tree
however. Can you give a verdict on whether you're going to merge it and
let me know when I can expect to see it in your tree if so?

Thanks,
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 2/3] cifs: use CreationTime like an i_generation field

[Jeff Layton]

On Mon, 22 Nov 2010 14:22:50 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Mon, Nov 22, 2010 at 1:55 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> 
> > On Mon, 28 Jun 2010 07:10:12 -0400
> > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >
> >
> > Steve, can you clarify where we are with this patch? I originally sent
> > this back in June. You and I discussed this at the CIFS plugfest and I
> > had thought you had decided to merge it. It's still not in your tree
> > however. Can you give a verdict on whether you're going to merge it and
> > let me know when I can expect to see it in your tree if so?
> >
> >
> Thanks for reminding me.  I had forgotten to ask JRA about a question
> on legacy Samba behavior.   I remembered that older Samba
> had problems with creation time - and was trying to remember what
> the implication of using this patch with older Samba was.
> 

All samba versions have issues with create times (at least on Linux
anyway). They fake them, by using the ctime (which seems like an odd
choice to me) in most cases.

Either way, let me know when you decide or if you have a question on
this.

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

Re: [PATCH 2/3] cifs: use CreationTime like an i_generation field

[Jeremy Allison]

On Mon, Nov 22, 2010 at 03:31:25PM -0500, Jeff Layton wrote:
> On Mon, 22 Nov 2010 14:22:50 -0600
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > On Mon, Nov 22, 2010 at 1:55 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > 
> > > On Mon, 28 Jun 2010 07:10:12 -0400
> > > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > >
> > >
> > > Steve, can you clarify where we are with this patch? I originally sent
> > > this back in June. You and I discussed this at the CIFS plugfest and I
> > > had thought you had decided to merge it. It's still not in your tree
> > > however. Can you give a verdict on whether you're going to merge it and
> > > let me know when I can expect to see it in your tree if so?
> > >
> > >
> > Thanks for reminding me.  I had forgotten to ask JRA about a question
> > on legacy Samba behavior.   I remembered that older Samba
> > had problems with creation time - and was trying to remember what
> > the implication of using this patch with older Samba was.
> > 
> 
> All samba versions have issues with create times (at least on Linux
> anyway). They fake them, by using the ctime (which seems like an odd
> choice to me) in most cases.

New Samba's store create time in an xattr, so they can
return a reasonably valid create time value.

Jeremy.

Re: [PATCH 2/3] cifs: use CreationTime like an i_generation field

[Jeff Layton]

On Mon, 22 Nov 2010 12:59:17 -0800
Jeremy Allison <jra-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:

> On Mon, Nov 22, 2010 at 03:31:25PM -0500, Jeff Layton wrote:
> > On Mon, 22 Nov 2010 14:22:50 -0600
> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > 
> > > On Mon, Nov 22, 2010 at 1:55 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > > 
> > > > On Mon, 28 Jun 2010 07:10:12 -0400
> > > > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > > >
> > > >
> > > > Steve, can you clarify where we are with this patch? I originally sent
> > > > this back in June. You and I discussed this at the CIFS plugfest and I
> > > > had thought you had decided to merge it. It's still not in your tree
> > > > however. Can you give a verdict on whether you're going to merge it and
> > > > let me know when I can expect to see it in your tree if so?
> > > >
> > > >
> > > Thanks for reminding me.  I had forgotten to ask JRA about a question
> > > on legacy Samba behavior.   I remembered that older Samba
> > > had problems with creation time - and was trying to remember what
> > > the implication of using this patch with older Samba was.
> > > 
> > 
> > All samba versions have issues with create times (at least on Linux
> > anyway). They fake them, by using the ctime (which seems like an odd
> > choice to me) in most cases.
> 
> New Samba's store create time in an xattr, so they can
> return a reasonably valid create time value.
> 

Steve, we're approaching the 6 month mark since I originally sent this
patch. Could you make a decision on it soon so that I know what should
be done with it?

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

[PATCH 3/3] cifs: reduce false positives with inode aliasing serverino autodisable

[Jeff Layton]

It turns out that not all directory inodes with dentries on the
i_dentry list are unusable here. We only consider them unusable if they
are still hashed or if they have a root dentry attached.

Full disclosure -- this check is inherently racy. There's nothing that
stops someone from slapping a new dentry onto this inode just after
this check, or hashing an existing one that's already attached. So,
this is really a "best effort" thing to work around misbehaving servers.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/inode.c |   43 +++++++++++++++++++++++++++++++------------
 1 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 35e2466..1bfb8ec 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -736,15 +736,9 @@ cifs_find_inode(struct inode *inode, void *opaque)
 	if ((inode->i_mode & S_IFMT) != (fattr->cf_mode & S_IFMT))
 		return 0;
 
-	/*
-	 * uh oh -- it's a directory. We can't use it since hardlinked dirs are
-	 * verboten. Disable serverino and return it as if it were found, the
-	 * caller can discard it, generate a uniqueid and retry the find
-	 */
-	if (S_ISDIR(inode->i_mode) && !list_empty(&inode->i_dentry)) {
+	/* if it's not a directory or has no dentries, then flag it */
+	if (S_ISDIR(inode->i_mode) && !list_empty(&inode->i_dentry))
 		fattr->cf_flags |= CIFS_FATTR_INO_COLLISION;
-		cifs_autodisable_serverino(CIFS_SB(inode->i_sb));
-	}
 
 	return 1;
 }
@@ -759,6 +753,27 @@ cifs_init_inode(struct inode *inode, void *opaque)
 	return 0;
 }
 
+/*
+ * walk dentry list for an inode and report whether it has aliases that
+ * are hashed. We use this to determine if a directory inode can actually
+ * be used.
+ */
+static bool
+inode_has_hashed_dentries(struct inode *inode)
+{
+	struct dentry *dentry;
+
+	spin_lock(&dcache_lock);
+	list_for_each_entry(dentry, &inode->i_dentry, d_alias) {
+		if (!d_unhashed(dentry) || IS_ROOT(dentry)) {
+			spin_unlock(&dcache_lock);
+			return true;
+		}
+	}
+	spin_unlock(&dcache_lock);
+	return false;
+}
+
 /* Given fattrs, get a corresponding inode */
 struct inode *
 cifs_iget(struct super_block *sb, struct cifs_fattr *fattr)
@@ -774,12 +789,16 @@ retry_iget5_locked:
 
 	inode = iget5_locked(sb, hash, cifs_find_inode, cifs_init_inode, fattr);
 	if (inode) {
-		/* was there a problematic inode number collision? */
+		/* was there a potentially problematic inode collision? */
 		if (fattr->cf_flags & CIFS_FATTR_INO_COLLISION) {
-			iput(inode);
-			fattr->cf_uniqueid = iunique(sb, ROOT_I);
 			fattr->cf_flags &= ~CIFS_FATTR_INO_COLLISION;
-			goto retry_iget5_locked;
+
+			if (inode_has_hashed_dentries(inode)) {
+				cifs_autodisable_serverino(CIFS_SB(sb));
+				iput(inode);
+				fattr->cf_uniqueid = iunique(sb, ROOT_I);
+				goto retry_iget5_locked;
+			}
 		}
 
 		cifs_fattr_to_inode(inode, fattr);
-- 
1.6.6.1