On Tue, Nov 19, 2013 at 2:59 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> On Tue, 19 Nov 2013 10:33:33 -0600
> shirishpargaonkar@xxxxxxxxx wrote:
>
>> From: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
>>
>>
>> Allow cifs mounts for a prefixpath with intermediate paths without access,
>> so as to continue with the shared superblock model.
>>
>> For the intermediate path entries that do not allow access, create "placeholder"
>> inodes and instantiate dentries with those inodes.
>> If and when such a path entry becomes accessible it is filled with correct
>> info.
>>
>
> I'm unclear on this last bit...
>
> How exactly do they end up being filled with the correct info once that
> happens?
I was thinking when the access permissions for this directory change
on the server.
For example, if we had two mounts with shared superblocks
/mnt_m/a/b
/mnt_n
where a was inaccessible. If the permissions change on a such that
it becomes accessible, stat /mnt_n/a will fetch correct info.
>
>> Reference: Samba bugzilla 6950
>>
>> Signed-off-by: Shirish Pargaonkar <spargaonkar@xxxxxxxx>
>>
>> ---
>> fs/cifs/cifsfs.c | 17 +++++++++++++++--
>> fs/cifs/cifsproto.h | 3 +++
>> fs/cifs/inode.c | 16 ++++++++++++++++
>> 3 files changed, 34 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
>> index 849f613..bce185c 100644
>> --- a/fs/cifs/cifsfs.c
>> +++ b/fs/cifs/cifsfs.c
>> @@ -584,6 +584,9 @@ cifs_get_root(struct smb_vol *vol, struct super_block *sb)
>> char *full_path = NULL;
>> char *s, *p;
>> char sep;
>> + struct inode *dir, *phinode;
>> + struct dentry *child;
>> + struct cifs_fattr phfattr;
>>
>> full_path = cifs_build_path_to_root(vol, cifs_sb,
>> cifs_sb_master_tcon(cifs_sb));
>> @@ -592,13 +595,13 @@ cifs_get_root(struct smb_vol *vol, struct super_block *sb)
>>
>> cifs_dbg(FYI, "Get root dentry for %s\n", full_path);
>>
>> + fill_phfattr(&phfattr, sb);
>
> Hmmm...so this function sets up a cifs_fattr struct, including the
> cf_uniqueid...which won't be very unique when you reuse that struct
> several times in the do...while loop below. Don't you need to call it
> again after you use it once?
>
That was silly. Will fix that.
>> sep = CIFS_DIR_SEP(cifs_sb);
>> dentry = dget(sb->s_root);
>> p = s = full_path;
>>
>> do {
>> - struct inode *dir = dentry->d_inode;
>> - struct dentry *child;
>> + dir = dentry->d_inode;
>>
>> if (!dir) {
>> dput(dentry);
>> @@ -626,6 +629,16 @@ cifs_get_root(struct smb_vol *vol, struct super_block *sb)
>> mutex_unlock(&dir->i_mutex);
>> dput(dentry);
>> dentry = child;
>> + if (!IS_ERR(dentry)) {
>> + if (*s) {
>> + /* EACCESS on an intermediate dir */
>> + if (!dentry->d_inode) {
>> + phinode = cifs_iget(sb, &phfattr);
>> + if (phinode)
>> + d_instantiate(dentry, phinode);
>> + }
>> + }
>> + }
>
> This doesn't make any sense to me.
>
> The do...while loop here is basically doing lookups and instantiating
> dentries and inodes down from the root of the mount to the root of the
> share. The lookup_one_len call is what does the single-level lookup in
> that directory.
>
> Why are you adding special handling when "dentry" is not an error?
> Would it not make more sense to do so when lookup_one_len does return
> an error?
If server returns ENOENT, lookup_one_len will return with a negative
dentry. This is the case I was handling. Need to access EACCESS.
Not sure why/when server returns ENOENT (object not found) even when
the (directory) entry exists.
>
>> } while (!IS_ERR(dentry));
>> kfree(full_path);
>> return dentry;
>> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
>> index aa33976..9a84e5c 100644
>> --- a/fs/cifs/cifsproto.h
>> +++ b/fs/cifs/cifsproto.h
>> @@ -498,4 +498,7 @@ void cifs_writedata_release(struct kref *refcount);
>> int open_query_close_cifs_symlink(const unsigned char *path, char *pbuf,
>> unsigned int *pbytes_read, struct cifs_sb_info *cifs_sb,
>> unsigned int xid);
>> +
>> +extern void fill_phfattr(struct cifs_fattr *, struct super_block *);
>> +
>> #endif /* _CIFSPROTO_H */
>> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
>> index 36f9ebb..4f5a09a 100644
>> --- a/fs/cifs/inode.c
>> +++ b/fs/cifs/inode.c
>> @@ -322,6 +322,22 @@ cifs_create_dfs_fattr(struct cifs_fattr *fattr, struct super_block *sb)
>> fattr->cf_flags |= CIFS_FATTR_DFS_REFERRAL;
>> }
>>
>> +void
>> +fill_phfattr(struct cifs_fattr *cf, struct super_block *sb)
>> +{
>> + struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
>> +
>> + memset(cf, 0, sizeof(*cf));
>> + cf->cf_uniqueid = iunique(sb, ROOT_I);
>
>
> Also, iunique is only guaranteed to return a number that hasn't yet
> been used. So suppose we're going to instantiate one of these bogus
> inodes. We give it the uniqueid of '3'. Then later, once the mount is
> done we find another (legitimate) inode on the server and it also has a
> uniqueid of '3'. Now you have a collision. Worse yet, if it's a
> directory now it looks like we have a hardlinked directory...
>
> Once you start using iunique, you can't really go back to using server
> inode numbers since you can't guarantee that there won't be collisions.
>
> One possibility to deal with that would be to tag these entries as
> bogus and ensure that cifs_iget will never match them. Another
> possibility might be just to never hash these inodes, but I'm not clear
> on how kosher that is from a vfs standpoint.
Looking into this.
>
>> + cf->cf_nlink = 1;
>> + cf->cf_atime = CURRENT_TIME;
>> + cf->cf_ctime = CURRENT_TIME;
>> + cf->cf_mtime = CURRENT_TIME;
>> + cf->cf_mode = S_IFDIR | S_IXUGO | S_IRWXU;
>> + cf->cf_uid = cifs_sb->mnt_uid;
>> + cf->cf_gid = cifs_sb->mnt_gid;
>> +}
>> +
>
>
>> static int
>> cifs_get_file_info_unix(struct file *filp)
>> {
>
>
> I think the thing to do is to step back and describe (in English) how
> you envision this working. Then once that's done, we can start
> discussing what the code should look like...
>
> --
> Jeff Layton <jlayton@xxxxxxxxxx>
--
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
