On Tue, Oct 20, 2015 at 12:22 PM, Steve French <smfrench@xxxxxxxxx> wrote:
> On Tue, Oct 20, 2015 at 3:55 AM, Ross Lagerwall
> <ross.lagerwall@xxxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
