[PATCH 0/3] cifs: fix some problems with noserverino + unix extensions

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/3] cifs: fix some problems with noserverino + unix extensions

Jeff Layton-2
This patchset fixes a number of issues with unix extensions when
"noserverino" is specified. The second patch in the set is one I've
posted before. Steve had previously mentioned that he had concerns
about it, but I think it's the right patch for now.

It would be good to eventually check for and handle the situation
where the uniqueid on a path-based operation changes, but I'm not
sure of the right way to handle that at this point and the current
code is clearly broken -- the server-provided uniqueid's are reported
in stat() calls even when noserverino is specified.

Given that, I think that patch #2 should be taken at this time even
if we don't currently handle inode number changes as well as we
should.

Jeff Layton (3):
  cifs: always revalidate hardlinked inodes
  cifs: don't update uniqueid in cifs_fattr_to_inode
  cifs: fix noserverino handling when unix extensions are enabled

 fs/cifs/cifsproto.h |    1 +
 fs/cifs/dir.c       |    1 +
 fs/cifs/inode.c     |   18 +++++++++++++++++-
 3 files changed, 19 insertions(+), 1 deletions(-)

_______________________________________________
linux-cifs-client mailing list
[hidden email]
https://lists.samba.org/mailman/listinfo/linux-cifs-client
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/3] cifs: always revalidate hardlinked inodes

Jeff Layton-2
The old cifs_revalidate logic always revalidated hardlinked inodes.
This hack allowed CIFS to pass some connectathon tests when server inode
numbers aren't used (basic test7, in particular).

Signed-off-by: Jeff Layton <[hidden email]>
---
 fs/cifs/inode.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index b35cb03..f52161a 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1511,6 +1511,10 @@ cifs_inode_needs_reval(struct inode *inode)
  if (time_after_eq(jiffies, cifs_i->time + HZ))
  return true;
 
+ /* hardlinked files get "special" treatment */
+ if (S_ISREG(inode->i_mode) && inode->i_nlink != 1)
+ return true;
+
  return false;
 }
 
--
1.6.6.1

_______________________________________________
linux-cifs-client mailing list
[hidden email]
https://lists.samba.org/mailman/listinfo/linux-cifs-client
Reply | Threaded
Open this post in threaded view
|

[PATCH 2/3] cifs: don't update uniqueid in cifs_fattr_to_inode

Jeff Layton-2
In reply to this post by Jeff Layton-2
We use this value to find an inode within the hash bucket, so we can't
change this without re-hashing the inode. For now, treat this value
as immutable.

Eventually, we should probably use an inode number change on a path
based operation to indicate that the lookup cache is invalid, but that's
a bit more code to deal with.

Signed-off-by: Jeff Layton <[hidden email]>
---
 fs/cifs/inode.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index f52161a..ae9a4cf 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -137,7 +137,6 @@ cifs_fattr_to_inode(struct inode *inode, struct cifs_fattr *fattr)
  inode->i_mode = fattr->cf_mode;
 
  cifs_i->cifsAttrs = fattr->cf_cifsattrs;
- cifs_i->uniqueid = fattr->cf_uniqueid;
 
  if (fattr->cf_flags & CIFS_FATTR_NEED_REVAL)
  cifs_i->time = 0;
--
1.6.6.1

_______________________________________________
linux-cifs-client mailing list
[hidden email]
https://lists.samba.org/mailman/listinfo/linux-cifs-client
Reply | Threaded
Open this post in threaded view
|

[PATCH 3/3] cifs: fix noserverino handling when unix extensions are enabled

Jeff Layton-2
In reply to this post by Jeff Layton-2
The uniqueid field sent by the server when unix extensions are enabled
is currently used sometimes when it shouldn't be. The readdir codepath
is correct, but most others are not. Fix it.

Signed-off-by: Jeff Layton <[hidden email]>
---
 fs/cifs/cifsproto.h |    1 +
 fs/cifs/dir.c       |    1 +
 fs/cifs/inode.c     |   13 +++++++++++++
 3 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index 2e07da9..fb1657e 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -110,6 +110,7 @@ extern int cifs_posix_open(char *full_path, struct inode **pinode,
  struct super_block *sb,
  int mode, int oflags,
  __u32 *poplock, __u16 *pnetfid, int xid);
+void cifs_fill_uniqueid(struct super_block *sb, struct cifs_fattr *fattr);
 extern void cifs_unix_basic_to_fattr(struct cifs_fattr *fattr,
      FILE_UNIX_BASIC_INFO *info,
      struct cifs_sb_info *cifs_sb);
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 86d3c0c..391816b 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -248,6 +248,7 @@ int cifs_posix_open(char *full_path, struct inode **pinode,
 
  /* get new inode and set it up */
  if (*pinode == NULL) {
+ cifs_fill_uniqueid(sb, &fattr);
  *pinode = cifs_iget(sb, &fattr);
  if (!*pinode) {
  rc = -ENOMEM;
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index ae9a4cf..800f894 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -169,6 +169,17 @@ cifs_fattr_to_inode(struct inode *inode, struct cifs_fattr *fattr)
  cifs_set_ops(inode, fattr->cf_flags & CIFS_FATTR_DFS_REFERRAL);
 }
 
+void
+cifs_fill_uniqueid(struct super_block *sb, struct cifs_fattr *fattr)
+{
+ struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
+
+ if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM)
+ return;
+
+ fattr->cf_uniqueid = iunique(sb, ROOT_I);
+}
+
 /* Fill a cifs_fattr struct with info from FILE_UNIX_BASIC_INFO. */
 void
 cifs_unix_basic_to_fattr(struct cifs_fattr *fattr, FILE_UNIX_BASIC_INFO *info,
@@ -322,6 +333,7 @@ int cifs_get_inode_info_unix(struct inode **pinode,
 
  if (*pinode == NULL) {
  /* get new inode */
+ cifs_fill_uniqueid(sb, &fattr);
  *pinode = cifs_iget(sb, &fattr);
  if (!*pinode)
  rc = -ENOMEM;
@@ -1180,6 +1192,7 @@ int cifs_mkdir(struct inode *inode, struct dentry *direntry, int mode)
  direntry->d_op = &cifs_dentry_ops;
 
  cifs_unix_basic_to_fattr(&fattr, pInfo, cifs_sb);
+ cifs_fill_uniqueid(inode->i_sb, &fattr);
  newinode = cifs_iget(inode->i_sb, &fattr);
  if (!newinode) {
  kfree(pInfo);
--
1.6.6.1

_______________________________________________
linux-cifs-client mailing list
[hidden email]
https://lists.samba.org/mailman/listinfo/linux-cifs-client
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/3] cifs: always revalidate hardlinked inodes

Steve French-2
In reply to this post by Jeff Layton-2
Seems odd to turn off metadata caching completely for hardlinked
files.  Wouldn't it be easier, and less invasive, to simply set
cifs_i->time to zero on source and destination inodes when the client
does the hardlink (so we revalidate the first time after the link is
done, but otherwise treat hardlinked files the same, and continue to
allow limited metadata caching on them)?

On Mon, May 17, 2010 at 6:18 AM, Jeff Layton <[hidden email]> wrote:

> The old cifs_revalidate logic always revalidated hardlinked inodes.
> This hack allowed CIFS to pass some connectathon tests when server inode
> numbers aren't used (basic test7, in particular).
>
> Signed-off-by: Jeff Layton <[hidden email]>
> ---
>  fs/cifs/inode.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index b35cb03..f52161a 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -1511,6 +1511,10 @@ cifs_inode_needs_reval(struct inode *inode)
>        if (time_after_eq(jiffies, cifs_i->time + HZ))
>                return true;
>
> +       /* hardlinked files get "special" treatment */
> +       if (S_ISREG(inode->i_mode) && inode->i_nlink != 1)
> +               return true;
> +
>        return false;
>  }
>
> --
> 1.6.6.1
>
>



--
Thanks,

Steve
_______________________________________________
linux-cifs-client mailing list
[hidden email]
https://lists.samba.org/mailman/listinfo/linux-cifs-client
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/3] cifs: always revalidate hardlinked inodes

Jeff Layton-2
On Mon, 17 May 2010 10:34:03 -0500
Steve French <[hidden email]> wrote:

> Seems odd to turn off metadata caching completely for hardlinked
> files.  Wouldn't it be easier, and less invasive, to simply set
> cifs_i->time to zero on source and destination inodes when the client
> does the hardlink (so we revalidate the first time after the link is
> done, but otherwise treat hardlinked files the same, and continue to
> allow limited metadata caching on them)?
>

I do agree that this makes little sense, but what you suggest won't
solve the problem. cifs_hardlink already does this:

        d_drop(direntry);       /* force new lookup from server of target */

...and:
                cifsInode = CIFS_I(old_file->d_inode);
                ...
                /* if not oplocked will force revalidate to get info
                   on source file from srv */
                cifsInode->time = 0;

...the reason the test doesn't pass is that the link count doesn't drop
on the inode attached to one of the dentries when you unlink the other.

What probably makes the most sense is to refuse to allow hardlinking of
inodes when noserverino is set, but that might be considered a
regression since older kernels allowed it.

In short, it's a mess but this gets the behavior back to what it was
originally.

--
Jeff Layton <[hidden email]>
_______________________________________________
linux-cifs-client mailing list
[hidden email]
https://lists.samba.org/mailman/listinfo/linux-cifs-client
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/3] cifs: always revalidate hardlinked inodes

Jeff Layton-4
In reply to this post by Jeff Layton-2
On Mon, 17 May 2010 07:18:56 -0400
Jeff Layton <[hidden email]> wrote:

> The old cifs_revalidate logic always revalidated hardlinked inodes.
> This hack allowed CIFS to pass some connectathon tests when server inode
> numbers aren't used (basic test7, in particular).
>
> Signed-off-by: Jeff Layton <[hidden email]>
> ---
>  fs/cifs/inode.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index b35cb03..f52161a 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -1511,6 +1511,10 @@ cifs_inode_needs_reval(struct inode *inode)
>   if (time_after_eq(jiffies, cifs_i->time + HZ))
>   return true;
>  
> + /* hardlinked files get "special" treatment */
> + if (S_ISREG(inode->i_mode) && inode->i_nlink != 1)
> + return true;
> +
>   return false;
>  }
>  
Steve mentioned on IRC that we should limit this behavior to the
noserverino case. Replacement patch attached.

--
Jeff Layton <[hidden email]>

_______________________________________________
linux-cifs-client mailing list
[hidden email]
https://lists.samba.org/mailman/listinfo/linux-cifs-client

0001-cifs-always-revalidate-hardlinked-inodes-when-using-.patch (1K) Download Attachment