On Tue, Oct 20, 2015 at 12:22 PM, Steve French <smfre...@gmail.com> wrote: > On Tue, Oct 20, 2015 at 3:55 AM, Ross Lagerwall > <ross.lagerw...@citrix.com> wrote: >> On 10/19/2015 05:21 PM, Steve French wrote: >>> >>> Could you verify this over SMB2 (vers=3.0) as well? It looks like it >>> should fail because cifs_all_info_to_fattr doesn't see the inode >>> attribute change and fill in the new inode number (IndexNuber). I am >>> suspicious that your patch is overkill (sends an extra request on >>> revalidate, doubling the traffic) in the SMB2/SMB3 case since we are >>> already doing a query file with FILE_ALL_INFO requested which already >>> returned IndexNumber (so should have already gotten the inode number) >>> - probably more important to use the IndexNumber we got back rather >>> than request it twice. >> >> >> It was verified with vers=3.0. It works due to the following code, where >> -ESTALE is returned if uniqueid has changed (this is independent of >> cifs_all_info_to_fattr). >> >> >> >>>> @@ -814,8 +814,26 @@ cifs_get_inode_info(struct inode **inode, const char >>>> *full_path, >>>> } >>>> } else >>>> fattr.cf_uniqueid = iunique(sb, ROOT_I); >>>> - } else >>>> - fattr.cf_uniqueid = CIFS_I(*inode)->uniqueid; >>>> + } else { >>>> + if (check_inode_no && >>>> + (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM) && >>>> + server->ops->get_srv_inum) { >>>> + tmprc = server->ops->get_srv_inum(xid, >>>> + tcon, cifs_sb, full_path, >>>> + &fattr.cf_uniqueid, data); >>>> + if (tmprc) { >>>> + cifs_dbg(FYI, "GetSrvInodeNum rc %d\n", >>>> + tmprc); >>>> + fattr.cf_uniqueid = iunique(sb, ROOT_I); >>>> + cifs_autodisable_serverino(cifs_sb); >>>> + } else if (CIFS_I(*inode)->uniqueid != >>>> + fattr.cf_uniqueid) { >>>> + rc = -ESTALE; >>>> + goto cgii_exit; >>>> + } >>>> + } else >>>> + fattr.cf_uniqueid = CIFS_I(*inode)->uniqueid; >>>> + } >>>> >>>> /* query for SFU type info if supported and needed */ >>>> if (fattr.cf_cifsattrs & ATTR_SYSTEM && >> >> >> Maybe it is possible for cifs_all_info_to_fattr to update the inode number >> in place? > > Could you run some experiments (cifs and smb3) with different use > cases (rename of file of inode with same name but of different type, > symlink, directory etc.) and see if cifs_all_info_fattr catches these?
(Adding some logging code which catches unexpected inode differences there and perhaps invalidates the dentry and/or updates the inode) >> I'm a bit concerned with the approach above that it only applies when the >> dentry is revalidated. There are many other instances where >> cifs_get_inode_info is called and the inode number may have changed which >> this does not take care of. >> >> I'm certainly not an expert in this area, so looking for ideas of how to >> solve the problem. >> >> Thanks, >> -- >> Ross Lagerwall > > > > -- > Thanks, > > Steve -- Thanks, Steve -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html