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

login
register
mail settings
Submitter Jeff Layton
Date May 17, 2010, 11:18 a.m.
Message ID <1274095138-3386-2-git-send-email-jlayton@redhat.com>
Download mbox | patch
Permalink /patch/52785/
State New
Headers show

Comments

Jeff Layton - May 17, 2010, 11:18 a.m.
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 <jlayton@redhat.com>
---
 fs/cifs/inode.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
Steve French - May 17, 2010, 3:34 p.m.
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 <jlayton@redhat.com> 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 <jlayton@redhat.com>
> ---
>  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
>
>
Jeff Layton - May 17, 2010, 4:36 p.m.
On Mon, 17 May 2010 10:34:03 -0500
Steve French <smfrench@gmail.com> 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.

Patch

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;
 }