I am not sure if ESTALE or ENOENT would have an effect on a dcache entry. A dcache entry and dentry are two different things, as I understand. In this case, dcache entry has not changed, what has changed is the dentry, specifically the inode it points to, so there is really no reason to purge and reinstate a dcache entry. On Fri, Apr 10, 2015 at 8:16 AM, Jeff Layton <jlayton@xxxxxxxxx> wrote: > On Thu, 9 Apr 2015 17:07:56 +0900 > Nakajima Akira <nakajima.akira@xxxxxxxxxxxx> wrote: > >> On 2015/04/07 23:39, Steve French wrote: >> > On Tue, Apr 7, 2015 at 5:45 AM, Jeff Layton <jlayton@xxxxxxxxx> wrote: >> >> On Wed, 24 Dec 2014 11:27:38 +0900 >> >> Nakajima Akira <nakajima.akira@xxxxxxxxxxxx> wrote: >> >> >> >>> When refer file "directly" (e.g. ls -li <filename>), >> >>> if file is same name, old inode cache is used. >> >>> This causes that client shows wrong(old) inode number. >> >>> So this patch is that if uniqueid is different, return error. >> >>> >> >>> ## But this patch is applicable to when Server is UNIX. >> >>> ## When Server is Windows, we need another new patch. >> >>> >> >>> >> >>> Reproducible sample : >> >>> 1. create file 'a' at cifs client. >> >>> 2. rm 'a' and touch 'b a' at server. >> >>> 3. ls -li 'a' at client, then client shows wrong(old) inode number. >> >>> >> >>> Bug link: >> >>> https://bugzilla.kernel.org/show_bug.cgi?id=90021 >> >>> >> >>> >> >>> >> >>> Signed-off-by: Nakajima Akira <nakajima.akira@xxxxxxxxxxxx> >> >>> diff -uprN -X linux-3.18-vanilla/Documentation/dontdiff linux-3.18-vanilla/fs/cifs/inode.c linux-3.18/fs/cifs/inode.c >> >>> --- linux-3.18-vanilla/fs/cifs/inode.c 2014-12-08 07:21:05.000000000 +0900 >> >>> +++ linux-3.18/fs/cifs/inode.c 2014-12-19 11:07:59.127000000 +0900 >> >>> @@ -402,9 +402,18 @@ int cifs_get_inode_info_unix(struct inod >> >>> rc = -ENOMEM; >> >>> } else { >> >>> /* we already have inode, update it */ >> >>> + >> >>> + /* if uniqueid is different, return error */ >> >>> + if (unlikely(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM && >> >>> + CIFS_I(*pinode)->uniqueid != fattr.cf_uniqueid)) { >> >>> + rc = -ENOENT; >> >>> + goto cgiiu_exit; >> >>> + } >> >>> + >> >>> cifs_fattr_to_inode(*pinode, &fattr); >> >>> } >> >>> >> >>> +cgiiu_exit: >> >>> return rc; >> >>> } >> >>> >> >> >> >> Returning ENOENT here seems like the wrong error to me. That path does >> >> exist, it just no longer refers to the same file as before. >> >> >> >> Maybe ESTALE would be better as it would allow the VFS layer >> >> to revalidate the dcache and invalidate the old dentry? >> >> >> >> -- >> >> Jeff Layton <jlayton@xxxxxxxxx> >> > >> > Similar to what Jeff mentioned, isn't the nfs_relavidate_inode path >> > roughly equivalent to what we want here (where nfs.ko returns ESTALE >> > on various cases where we detect an inode that doesn't match what we >> > expect)? >> >> If uniqueid is different, return -ESTALE. >> If filetype is different, return -ENOENT. >> That's right? >> >> + /* if filetype is different, return error */ >> + if (unlikely(((*pinode)->i_mode & S_IFMT) != >> + (fattr.cf_mode & S_IFMT))) { >> + rc = -ENOENT; >> + goto cgiiu_exit; >> + } >> > > No, I don't think so. In both cases, the dcache is wrong and the dentry > should be dropped and reinstantiated to point to a new inode. An ESTALE > return is the trigger for that to occur. An ENOENT return is going to > mean a stat() failure in your testcase, I think. > > So I think you want to return ESTALE in both cases. That said, please > do test it and ensure that it does the right thing. > > -- > 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 -- 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
