| Message ID | 1314341541-2476-1-git-send-email-piastry@etersoft.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2011/8/26 Jeff Layton <jlayton@redhat.com>: ... >> +static struct dentry * >> +cifs_alloc_root(struct super_block *sb) >> +{ >> + struct qstr q; >> + >> + q.name = "/"; > > I don't think you want a separator in the name. That seems > like it should be "". Ok, I will try. > >> + q.len = 1; >> + q.hash = full_name_hash(q.name, q.len); >> + return d_alloc_pseudo(sb, &q); >> +} >> + ... >> - if (filldir(direntry, "..", 2, file->f_pos, >> - parent_ino(file->f_path.dentry), DT_DIR) < 0) { >> + if (!file->f_path.dentry->d_parent->d_inode) { >> + cFYI(1, "parent dir is negative, filling as current"); >> + ino = file->f_path.dentry->d_inode->i_ino; >> + } else >> + ino = parent_ino(file->f_path.dentry); >> + if (filldir(direntry, "..", 2, file->f_pos, ino, DT_DIR) < 0) { >> cERROR(1, "Filldir for parent dir failed"); >> rc = -ENOMEM; >> break; >> > (cc'ing David Howells since he did the sb sharing code for NFS) > > This doesn't seem quite right to me... > > This implies that the parent in this situation would be the parent > dentry on the server, but that shouldn't be the case right? Shouldn't > this be the mountpoint's parent? > > For instance, suppose you have //server/share/d1/d2/d3 mounted > on /mnt/cifs. During the mount, d1 and d2 are instantiated as negative > dentries. Here, you try to fill in the i_ino for d2 as the inode number > for "..", but that doesn't seem correct -- shouldn't it be the inode > number for /mnt? I think that doesn't seem correct too, but I looked through another file systems code and found the same things. > > I don't have a great feel for this sort of dcache trickery, but I tend > to think we probably ought to follow NFS' example here. It has sort of > an "opportunistic" mechanism for filling in the dcache for a particular > superblock. This is detailed in the comments on commit 54ceac45. As I understand, NFS does the following: 1) requests fattr from the server error = server->nfs_client->rpc_ops->getroot(server, mntfh, &fsinfo); 2) lookup an inode with ino got from the server inode = nfs_fhget(sb, mntfh, fsinfo.fattr); 3) set dummy root error = nfs_superblock_set_dummy_root(sb, inode); 4) get dentry for the inode ret = d_obtain_alias(inode); So, it lookup an existing dentry according to the ino got from the server. As for CIFS, we can't do it because we may end up using a different inode numbers for the same path (noserverino cases). Thoughts?
2011/8/26 Jeff Layton <jlayton@redhat.com>: > On Fri, 26 Aug 2011 10:52:21 +0400 > Pavel Shilovsky <piastry@etersoft.ru> wrote: > >> Reorganize code and make it send qpath info request only for a full >> path (//server/share/prefixpath) rather than request for every path >> compoment. In this case we end up with negative dentries for all >> components except full one and we will instantiate them later if >> such a mount is requested. >> >> Signed-off-by: Pavel Shilovsky <piastry@etersoft.ru> >> --- >> fs/cifs/cifsfs.c | 123 +++++++++++++++++++++++++++++++---------------------- >> fs/cifs/cifsfs.h | 3 +- >> fs/cifs/inode.c | 7 ++- >> fs/cifs/readdir.c | 9 +++- >> 4 files changed, 85 insertions(+), 57 deletions(-) >> >> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c >> index 0435bb9..33a2e1e 100644 >> --- a/fs/cifs/cifsfs.c >> +++ b/fs/cifs/cifsfs.c >> @@ -95,14 +95,13 @@ mempool_t *smb2_mid_poolp; >> static struct kmem_cache *smb2_mid_cachep; >> #endif /* CONFIG_CIFS_SMB2 */ >> >> -static int >> +static void >> cifs_read_super(struct super_block *sb) >> { >> - struct inode *inode; >> - struct cifs_sb_info *cifs_sb; >> - int rc = 0; >> + struct cifs_sb_info *cifs_sb = CIFS_SB(sb); >> >> - cifs_sb = CIFS_SB(sb); >> + /* BB should we make this contingent on mount parm? */ >> + sb->s_flags |= MS_NODIRATIME | MS_NOATIME; >> >> if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIXACL) >> sb->s_flags |= MS_POSIXACL; >> @@ -120,26 +119,6 @@ cifs_read_super(struct super_block *sb) >> sb->s_bdi = &cifs_sb->bdi; >> sb->s_blocksize = CIFS_MAX_MSGSIZE; >> sb->s_blocksize_bits = 14; /* default 2**14 = CIFS_MAX_MSGSIZE */ >> - inode = cifs_root_iget(sb); >> - >> - if (IS_ERR(inode)) { >> - rc = PTR_ERR(inode); >> - inode = NULL; >> - goto out_no_root; >> - } >> - >> - sb->s_root = d_alloc_root(inode); >> - >> - if (!sb->s_root) { >> - rc = -ENOMEM; >> - goto out_no_root; >> - } >> - >> - /* do that *after* d_alloc_root() - we want NULL ->d_op for root here */ >> - if (cifs_sb_master_tcon(cifs_sb)->nocase) >> - sb->s_d_op = &cifs_ci_dentry_ops; >> - else >> - sb->s_d_op = &cifs_dentry_ops; >> >> #ifdef CIFS_NFSD_EXPORT >> if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM) { >> @@ -148,14 +127,7 @@ cifs_read_super(struct super_block *sb) >> } >> #endif /* CIFS_NFSD_EXPORT */ >> >> - return 0; >> - >> -out_no_root: >> - cERROR(1, "cifs_read_super: get root inode failed"); >> - if (inode) >> - iput(inode); >> - >> - return rc; >> + sb->s_flags |= MS_ACTIVE; >> } >> >> static void cifs_kill_sb(struct super_block *sb) >> @@ -529,6 +501,17 @@ static const struct super_operations cifs_super_ops = { >> #endif >> }; >> >> +static struct dentry * >> +cifs_alloc_root(struct super_block *sb) >> +{ >> + struct qstr q; >> + >> + q.name = "/"; > > I don't think you want a separator in the name. That seems > like it should be "". > In this case I based it on d_alloc_root() function that fills name as "/". Why this should be ""?
On Sat, 27 Aug 2011 00:11:57 +0400 Pavel Shilovsky <piastry@etersoft.ru> wrote: > 2011/8/26 Jeff Layton <jlayton@redhat.com>: > > On Fri, 26 Aug 2011 10:52:21 +0400 > > Pavel Shilovsky <piastry@etersoft.ru> wrote: > > > >> Reorganize code and make it send qpath info request only for a full > >> path (//server/share/prefixpath) rather than request for every path > >> compoment. In this case we end up with negative dentries for all > >> components except full one and we will instantiate them later if > >> such a mount is requested. > >> > >> Signed-off-by: Pavel Shilovsky <piastry@etersoft.ru> > >> --- > >> fs/cifs/cifsfs.c | 123 +++++++++++++++++++++++++++++++---------------------- > >> fs/cifs/cifsfs.h | 3 +- > >> fs/cifs/inode.c | 7 ++- > >> fs/cifs/readdir.c | 9 +++- > >> 4 files changed, 85 insertions(+), 57 deletions(-) > >> > >> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c > >> index 0435bb9..33a2e1e 100644 > >> --- a/fs/cifs/cifsfs.c > >> +++ b/fs/cifs/cifsfs.c > >> @@ -95,14 +95,13 @@ mempool_t *smb2_mid_poolp; > >> static struct kmem_cache *smb2_mid_cachep; > >> #endif /* CONFIG_CIFS_SMB2 */ > >> > >> -static int > >> +static void > >> cifs_read_super(struct super_block *sb) > >> { > >> - struct inode *inode; > >> - struct cifs_sb_info *cifs_sb; > >> - int rc = 0; > >> + struct cifs_sb_info *cifs_sb = CIFS_SB(sb); > >> > >> - cifs_sb = CIFS_SB(sb); > >> + /* BB should we make this contingent on mount parm? */ > >> + sb->s_flags |= MS_NODIRATIME | MS_NOATIME; > >> > >> if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIXACL) > >> sb->s_flags |= MS_POSIXACL; > >> @@ -120,26 +119,6 @@ cifs_read_super(struct super_block *sb) > >> sb->s_bdi = &cifs_sb->bdi; > >> sb->s_blocksize = CIFS_MAX_MSGSIZE; > >> sb->s_blocksize_bits = 14; /* default 2**14 = CIFS_MAX_MSGSIZE */ > >> - inode = cifs_root_iget(sb); > >> - > >> - if (IS_ERR(inode)) { > >> - rc = PTR_ERR(inode); > >> - inode = NULL; > >> - goto out_no_root; > >> - } > >> - > >> - sb->s_root = d_alloc_root(inode); > >> - > >> - if (!sb->s_root) { > >> - rc = -ENOMEM; > >> - goto out_no_root; > >> - } > >> - > >> - /* do that *after* d_alloc_root() - we want NULL ->d_op for root here */ > >> - if (cifs_sb_master_tcon(cifs_sb)->nocase) > >> - sb->s_d_op = &cifs_ci_dentry_ops; > >> - else > >> - sb->s_d_op = &cifs_dentry_ops; > >> > >> #ifdef CIFS_NFSD_EXPORT > >> if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM) { > >> @@ -148,14 +127,7 @@ cifs_read_super(struct super_block *sb) > >> } > >> #endif /* CIFS_NFSD_EXPORT */ > >> > >> - return 0; > >> - > >> -out_no_root: > >> - cERROR(1, "cifs_read_super: get root inode failed"); > >> - if (inode) > >> - iput(inode); > >> - > >> - return rc; > >> + sb->s_flags |= MS_ACTIVE; > >> } > >> > >> static void cifs_kill_sb(struct super_block *sb) > >> @@ -529,6 +501,17 @@ static const struct super_operations cifs_super_ops = { > >> #endif > >> }; > >> > >> +static struct dentry * > >> +cifs_alloc_root(struct super_block *sb) > >> +{ > >> + struct qstr q; > >> + > >> + q.name = "/"; > > > > I don't think you want a separator in the name. That seems > > like it should be "". > > > > In this case I based it on d_alloc_root() function that fills name as > "/". Why this should be ""? > You may be right. It just looked odd to me because '/' is typically the delimiter between path components and the q.name should be the name of the path component itself. If d_alloc_root does it though, you're probably OK.
2011/8/26 Pavel Shilovsky <piastry@etersoft.ru>: > Reorganize code and make it send qpath info request only for a full > path (//server/share/prefixpath) rather than request for every path > compoment. In this case we end up with negative dentries for all > components except full one and we will instantiate them later if > such a mount is requested. > > Signed-off-by: Pavel Shilovsky <piastry@etersoft.ru> > --- > fs/cifs/cifsfs.c | 123 +++++++++++++++++++++++++++++++---------------------- > fs/cifs/cifsfs.h | 3 +- > fs/cifs/inode.c | 7 ++- > fs/cifs/readdir.c | 9 +++- > 4 files changed, 85 insertions(+), 57 deletions(-) > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c > index 0435bb9..33a2e1e 100644 > --- a/fs/cifs/cifsfs.c > +++ b/fs/cifs/cifsfs.c > @@ -95,14 +95,13 @@ mempool_t *smb2_mid_poolp; > static struct kmem_cache *smb2_mid_cachep; > #endif /* CONFIG_CIFS_SMB2 */ > > -static int > +static void > cifs_read_super(struct super_block *sb) > { > - struct inode *inode; > - struct cifs_sb_info *cifs_sb; > - int rc = 0; > + struct cifs_sb_info *cifs_sb = CIFS_SB(sb); > > - cifs_sb = CIFS_SB(sb); > + /* BB should we make this contingent on mount parm? */ > + sb->s_flags |= MS_NODIRATIME | MS_NOATIME; > > if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIXACL) > sb->s_flags |= MS_POSIXACL; > @@ -120,26 +119,6 @@ cifs_read_super(struct super_block *sb) > sb->s_bdi = &cifs_sb->bdi; > sb->s_blocksize = CIFS_MAX_MSGSIZE; > sb->s_blocksize_bits = 14; /* default 2**14 = CIFS_MAX_MSGSIZE */ > - inode = cifs_root_iget(sb); > - > - if (IS_ERR(inode)) { > - rc = PTR_ERR(inode); > - inode = NULL; > - goto out_no_root; > - } > - > - sb->s_root = d_alloc_root(inode); > - > - if (!sb->s_root) { > - rc = -ENOMEM; > - goto out_no_root; > - } > - > - /* do that *after* d_alloc_root() - we want NULL ->d_op for root here */ > - if (cifs_sb_master_tcon(cifs_sb)->nocase) > - sb->s_d_op = &cifs_ci_dentry_ops; > - else > - sb->s_d_op = &cifs_dentry_ops; > > #ifdef CIFS_NFSD_EXPORT > if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM) { > @@ -148,14 +127,7 @@ cifs_read_super(struct super_block *sb) > } > #endif /* CIFS_NFSD_EXPORT */ > > - return 0; > - > -out_no_root: > - cERROR(1, "cifs_read_super: get root inode failed"); > - if (inode) > - iput(inode); > - > - return rc; > + sb->s_flags |= MS_ACTIVE; > } > > static void cifs_kill_sb(struct super_block *sb) > @@ -529,6 +501,17 @@ static const struct super_operations cifs_super_ops = { > #endif > }; > > +static struct dentry * > +cifs_alloc_root(struct super_block *sb) > +{ > + struct qstr q; > + > + q.name = "/"; > + q.len = 1; > + q.hash = full_name_hash(q.name, q.len); > + return d_alloc_pseudo(sb, &q); > +} > + > /* > * Get root dentry from superblock according to prefix path mount option. > * Return dentry with refcount + 1 on success and NULL otherwise. > @@ -536,8 +519,10 @@ static const struct super_operations cifs_super_ops = { > static struct dentry * > cifs_get_root(struct smb_vol *vol, struct super_block *sb) > { > - struct dentry *dentry; > + struct dentry *dentry, *found; > + struct inode *inode; > struct cifs_sb_info *cifs_sb = CIFS_SB(sb); > + struct qstr q; > char *full_path = NULL; > char *s, *p; > char sep; > @@ -550,13 +535,29 @@ cifs_get_root(struct smb_vol *vol, struct super_block *sb) > > cFYI(1, "Get root dentry for %s", full_path); > > + if (!sb->s_root) { > + sb->s_root = cifs_alloc_root(sb); > + if (IS_ERR(sb->s_root)) { > + dentry = ERR_PTR(-ENOMEM); > + goto out; > + } > + > + /* > + * do that *after* cifs_alloc_root() - we want NULL ->d_op for > + * root here > + */ > + if (cifs_sb_master_tcon(cifs_sb)->nocase) > + sb->s_d_op = &cifs_ci_dentry_ops; > + else > + sb->s_d_op = &cifs_dentry_ops; > + } > + > xid = GetXid(); > 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; > > /* skip separators */ > @@ -569,16 +570,45 @@ cifs_get_root(struct smb_vol *vol, struct super_block *sb) > while (*s && *s != sep) > s++; > > - mutex_lock(&dir->i_mutex); > - child = lookup_one_len(p, dentry, s - p); > - mutex_unlock(&dir->i_mutex); > + q.name = p; > + q.len = s - p; > + if (dentry->d_flags & DCACHE_OP_HASH) > + dentry->d_op->d_hash(dentry, dentry->d_inode, &q); > + else > + q.hash = full_name_hash(q.name, q.len); > + > + child = d_lookup(dentry, &q); > + if (!child) { > + child = d_alloc(dentry, &q); > + if (IS_ERR(child)) { > + dput(dentry); > + dentry = ERR_CAST(child); > + break; > + } > + d_rehash(child); > + } > dput(dentry); > dentry = child; > - if (!dentry->d_inode) { > + } while (!IS_ERR(dentry)); > + > + if (IS_ERR(dentry)) > + goto out; > + > + if (!dentry->d_inode) { > + inode = cifs_mntroot_iget(sb, full_path); > + if (IS_ERR(inode)) { > dput(dentry); > - dentry = ERR_PTR(-ENOENT); > + dentry = ERR_CAST(inode); > + goto out; > } > - } while (!IS_ERR(dentry)); > + > + found = d_instantiate_unique(dentry, inode); > + if (found) { > + dput(dentry); > + dentry = dget(found); it seems like found dentry has been already got by d_instantiate_unique - we don't need to call dget again here > + } > + } also, I think that if (!dentry->d_inode) {} statement should be protected by a lock (mutex) - I am not sure should we create new lock for this or use existing one. What d you think about it? > +out: > _FreeXid(xid); > kfree(full_path); > return dentry; > @@ -646,16 +676,7 @@ cifs_do_mount(struct file_system_type *fs_type, > cifs_umount(cifs_sb); > } else { > sb->s_flags = flags; > - /* BB should we make this contingent on mount parm? */ > - sb->s_flags |= MS_NODIRATIME | MS_NOATIME; > - > - rc = cifs_read_super(sb); > - if (rc) { > - root = ERR_PTR(rc); > - goto out_super; > - } > - > - sb->s_flags |= MS_ACTIVE; > + cifs_read_super(sb); > } > > root = cifs_get_root(volume_info, sb); > diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h > index 3145c18..47d9ec9 100644 > --- a/fs/cifs/cifsfs.h > +++ b/fs/cifs/cifsfs.h > @@ -43,7 +43,8 @@ extern const struct address_space_operations cifs_addr_ops_smallbuf; > > /* Functions related to inodes */ > extern const struct inode_operations cifs_dir_inode_ops; > -extern struct inode *cifs_root_iget(struct super_block *); > +extern struct inode *cifs_mntroot_iget(struct super_block *sb, > + const char *full_path); > extern int cifs_create(struct inode *, struct dentry *, int, > struct nameidata *); > extern struct dentry *cifs_lookup(struct inode *, struct dentry *, > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c > index aee0c0b..dedfed3 100644 > --- a/fs/cifs/inode.c > +++ b/fs/cifs/inode.c > @@ -931,7 +931,7 @@ retry_iget5_locked: > } > > /* gets root inode */ > -struct inode *cifs_root_iget(struct super_block *sb) > +struct inode *cifs_mntroot_iget(struct super_block *sb, const char *full_path) > { > int xid; > struct cifs_sb_info *cifs_sb = CIFS_SB(sb); > @@ -941,9 +941,10 @@ struct inode *cifs_root_iget(struct super_block *sb) > > xid = GetXid(); > if (tcon->unix_ext) > - rc = cifs_get_inode_info_unix(&inode, "", sb, xid); > + rc = cifs_get_inode_info_unix(&inode, full_path, sb, xid); > else > - rc = cifs_get_inode_info(&inode, "", NULL, sb, xid, NULL); > + rc = cifs_get_inode_info(&inode, full_path, NULL, sb, xid, > + NULL); > > if (!inode) { > inode = ERR_PTR(rc); > diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c > index 75e8b5c..7475cba 100644 > --- a/fs/cifs/readdir.c > +++ b/fs/cifs/readdir.c > @@ -708,6 +708,7 @@ int cifs_readdir(struct file *file, void *direntry, filldir_t filldir) > char *tmp_buf = NULL; > char *end_of_smb; > unsigned int max_len; > + ino_t ino; > > xid = GetXid(); > > @@ -732,8 +733,12 @@ int cifs_readdir(struct file *file, void *direntry, filldir_t filldir) > } > file->f_pos++; > case 1: > - if (filldir(direntry, "..", 2, file->f_pos, > - parent_ino(file->f_path.dentry), DT_DIR) < 0) { > + if (!file->f_path.dentry->d_parent->d_inode) { > + cFYI(1, "parent dir is negative, filling as current"); > + ino = file->f_path.dentry->d_inode->i_ino; > + } else > + ino = parent_ino(file->f_path.dentry); > + if (filldir(direntry, "..", 2, file->f_pos, ino, DT_DIR) < 0) { > cERROR(1, "Filldir for parent dir failed"); > rc = -ENOMEM; > break; > -- > 1.7.1 > >
On Tue, 30 Aug 2011 16:31:52 +0400 Pavel Shilovsky <piastry@etersoft.ru> wrote: > 2011/8/26 Pavel Shilovsky <piastry@etersoft.ru>: > > Reorganize code and make it send qpath info request only for a full > > path (//server/share/prefixpath) rather than request for every path > > compoment. In this case we end up with negative dentries for all > > components except full one and we will instantiate them later if > > such a mount is requested. > > > > Signed-off-by: Pavel Shilovsky <piastry@etersoft.ru> > > --- > > fs/cifs/cifsfs.c | 123 +++++++++++++++++++++++++++++++---------------------- > > fs/cifs/cifsfs.h | 3 +- > > fs/cifs/inode.c | 7 ++- > > fs/cifs/readdir.c | 9 +++- > > 4 files changed, 85 insertions(+), 57 deletions(-) > > > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c > > index 0435bb9..33a2e1e 100644 > > --- a/fs/cifs/cifsfs.c > > +++ b/fs/cifs/cifsfs.c > > @@ -95,14 +95,13 @@ mempool_t *smb2_mid_poolp; > > static struct kmem_cache *smb2_mid_cachep; > > #endif /* CONFIG_CIFS_SMB2 */ > > > > -static int > > +static void > > cifs_read_super(struct super_block *sb) > > { > > - struct inode *inode; > > - struct cifs_sb_info *cifs_sb; > > - int rc = 0; > > + struct cifs_sb_info *cifs_sb = CIFS_SB(sb); > > > > - cifs_sb = CIFS_SB(sb); > > + /* BB should we make this contingent on mount parm? */ > > + sb->s_flags |= MS_NODIRATIME | MS_NOATIME; > > > > if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIXACL) > > sb->s_flags |= MS_POSIXACL; > > @@ -120,26 +119,6 @@ cifs_read_super(struct super_block *sb) > > sb->s_bdi = &cifs_sb->bdi; > > sb->s_blocksize = CIFS_MAX_MSGSIZE; > > sb->s_blocksize_bits = 14; /* default 2**14 = CIFS_MAX_MSGSIZE */ > > - inode = cifs_root_iget(sb); > > - > > - if (IS_ERR(inode)) { > > - rc = PTR_ERR(inode); > > - inode = NULL; > > - goto out_no_root; > > - } > > - > > - sb->s_root = d_alloc_root(inode); > > - > > - if (!sb->s_root) { > > - rc = -ENOMEM; > > - goto out_no_root; > > - } > > - > > - /* do that *after* d_alloc_root() - we want NULL ->d_op for root here */ > > - if (cifs_sb_master_tcon(cifs_sb)->nocase) > > - sb->s_d_op = &cifs_ci_dentry_ops; > > - else > > - sb->s_d_op = &cifs_dentry_ops; > > > > #ifdef CIFS_NFSD_EXPORT > > if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM) { > > @@ -148,14 +127,7 @@ cifs_read_super(struct super_block *sb) > > } > > #endif /* CIFS_NFSD_EXPORT */ > > > > - return 0; > > - > > -out_no_root: > > - cERROR(1, "cifs_read_super: get root inode failed"); > > - if (inode) > > - iput(inode); > > - > > - return rc; > > + sb->s_flags |= MS_ACTIVE; > > } > > > > static void cifs_kill_sb(struct super_block *sb) > > @@ -529,6 +501,17 @@ static const struct super_operations cifs_super_ops = { > > #endif > > }; > > > > +static struct dentry * > > +cifs_alloc_root(struct super_block *sb) > > +{ > > + struct qstr q; > > + > > + q.name = "/"; > > + q.len = 1; > > + q.hash = full_name_hash(q.name, q.len); > > + return d_alloc_pseudo(sb, &q); > > +} > > + > > /* > > * Get root dentry from superblock according to prefix path mount option. > > * Return dentry with refcount + 1 on success and NULL otherwise. > > @@ -536,8 +519,10 @@ static const struct super_operations cifs_super_ops = { > > static struct dentry * > > cifs_get_root(struct smb_vol *vol, struct super_block *sb) > > { > > - struct dentry *dentry; > > + struct dentry *dentry, *found; > > + struct inode *inode; > > struct cifs_sb_info *cifs_sb = CIFS_SB(sb); > > + struct qstr q; > > char *full_path = NULL; > > char *s, *p; > > char sep; > > @@ -550,13 +535,29 @@ cifs_get_root(struct smb_vol *vol, struct super_block *sb) > > > > cFYI(1, "Get root dentry for %s", full_path); > > > > + if (!sb->s_root) { > > + sb->s_root = cifs_alloc_root(sb); > > + if (IS_ERR(sb->s_root)) { > > + dentry = ERR_PTR(-ENOMEM); > > + goto out; > > + } > > + > > + /* > > + * do that *after* cifs_alloc_root() - we want NULL ->d_op for > > + * root here > > + */ > > + if (cifs_sb_master_tcon(cifs_sb)->nocase) > > + sb->s_d_op = &cifs_ci_dentry_ops; > > + else > > + sb->s_d_op = &cifs_dentry_ops; > > + } > > + > > xid = GetXid(); > > 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; > > > > /* skip separators */ > > @@ -569,16 +570,45 @@ cifs_get_root(struct smb_vol *vol, struct super_block *sb) > > while (*s && *s != sep) > > s++; > > > > - mutex_lock(&dir->i_mutex); > > - child = lookup_one_len(p, dentry, s - p); > > - mutex_unlock(&dir->i_mutex); > > + q.name = p; > > + q.len = s - p; > > + if (dentry->d_flags & DCACHE_OP_HASH) > > + dentry->d_op->d_hash(dentry, dentry->d_inode, &q); > > + else > > + q.hash = full_name_hash(q.name, q.len); > > + > > + child = d_lookup(dentry, &q); > > + if (!child) { > > + child = d_alloc(dentry, &q); > > + if (IS_ERR(child)) { > > + dput(dentry); > > + dentry = ERR_CAST(child); > > + break; > > + } > > + d_rehash(child); > > + } > > dput(dentry); > > dentry = child; > > - if (!dentry->d_inode) { > > + } while (!IS_ERR(dentry)); > > + > > + if (IS_ERR(dentry)) > > + goto out; > > + > > + if (!dentry->d_inode) { > > + inode = cifs_mntroot_iget(sb, full_path); > > + if (IS_ERR(inode)) { > > dput(dentry); > > - dentry = ERR_PTR(-ENOENT); > > + dentry = ERR_CAST(inode); > > + goto out; > > } > > - } while (!IS_ERR(dentry)); > > + > > + found = d_instantiate_unique(dentry, inode); > > + if (found) { > > + dput(dentry); > > + dentry = dget(found); > > it seems like found dentry has been already got by > d_instantiate_unique - we don't need to call dget again here > That sounds right. > > + } > > + } > > also, I think that if (!dentry->d_inode) {} statement should be > protected by a lock (mutex) - I am not sure should we create new lock > for this or use existing one. What d you think about it? > I guess you're worried about the dentry suddenly becoming negative after checking for it? If you have an active reference to it, then that shouldn't occur. See d_delete(). > > +out: > > _FreeXid(xid); > > kfree(full_path); > > return dentry; > > @@ -646,16 +676,7 @@ cifs_do_mount(struct file_system_type *fs_type, > > cifs_umount(cifs_sb); > > } else { > > sb->s_flags = flags; > > - /* BB should we make this contingent on mount parm? */ > > - sb->s_flags |= MS_NODIRATIME | MS_NOATIME; > > - > > - rc = cifs_read_super(sb); > > - if (rc) { > > - root = ERR_PTR(rc); > > - goto out_super; > > - } > > - > > - sb->s_flags |= MS_ACTIVE; > > + cifs_read_super(sb); > > } > > > > root = cifs_get_root(volume_info, sb); > > diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h > > index 3145c18..47d9ec9 100644 > > --- a/fs/cifs/cifsfs.h > > +++ b/fs/cifs/cifsfs.h > > @@ -43,7 +43,8 @@ extern const struct address_space_operations cifs_addr_ops_smallbuf; > > > > /* Functions related to inodes */ > > extern const struct inode_operations cifs_dir_inode_ops; > > -extern struct inode *cifs_root_iget(struct super_block *); > > +extern struct inode *cifs_mntroot_iget(struct super_block *sb, > > + const char *full_path); > > extern int cifs_create(struct inode *, struct dentry *, int, > > struct nameidata *); > > extern struct dentry *cifs_lookup(struct inode *, struct dentry *, > > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c > > index aee0c0b..dedfed3 100644 > > --- a/fs/cifs/inode.c > > +++ b/fs/cifs/inode.c > > @@ -931,7 +931,7 @@ retry_iget5_locked: > > } > > > > /* gets root inode */ > > -struct inode *cifs_root_iget(struct super_block *sb) > > +struct inode *cifs_mntroot_iget(struct super_block *sb, const char *full_path) > > { > > int xid; > > struct cifs_sb_info *cifs_sb = CIFS_SB(sb); > > @@ -941,9 +941,10 @@ struct inode *cifs_root_iget(struct super_block *sb) > > > > xid = GetXid(); > > if (tcon->unix_ext) > > - rc = cifs_get_inode_info_unix(&inode, "", sb, xid); > > + rc = cifs_get_inode_info_unix(&inode, full_path, sb, xid); > > else > > - rc = cifs_get_inode_info(&inode, "", NULL, sb, xid, NULL); > > + rc = cifs_get_inode_info(&inode, full_path, NULL, sb, xid, > > + NULL); > > > > if (!inode) { > > inode = ERR_PTR(rc); > > diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c > > index 75e8b5c..7475cba 100644 > > --- a/fs/cifs/readdir.c > > +++ b/fs/cifs/readdir.c > > @@ -708,6 +708,7 @@ int cifs_readdir(struct file *file, void *direntry, filldir_t filldir) > > char *tmp_buf = NULL; > > char *end_of_smb; > > unsigned int max_len; > > + ino_t ino; > > > > xid = GetXid(); > > > > @@ -732,8 +733,12 @@ int cifs_readdir(struct file *file, void *direntry, filldir_t filldir) > > } > > file->f_pos++; > > case 1: > > - if (filldir(direntry, "..", 2, file->f_pos, > > - parent_ino(file->f_path.dentry), DT_DIR) < 0) { > > + if (!file->f_path.dentry->d_parent->d_inode) { > > + cFYI(1, "parent dir is negative, filling as current"); > > + ino = file->f_path.dentry->d_inode->i_ino; > > + } else > > + ino = parent_ino(file->f_path.dentry); > > + if (filldir(direntry, "..", 2, file->f_pos, ino, DT_DIR) < 0) { > > cERROR(1, "Filldir for parent dir failed"); > > rc = -ENOMEM; > > break; > > -- > > 1.7.1 > > > > > > >
2011/8/30 Jeff Layton <jlayton@redhat.com>: > On Tue, 30 Aug 2011 16:31:52 +0400 > Pavel Shilovsky <piastry@etersoft.ru> wrote: > >> 2011/8/26 Pavel Shilovsky <piastry@etersoft.ru>: >> > Reorganize code and make it send qpath info request only for a full >> > path (//server/share/prefixpath) rather than request for every path >> > compoment. In this case we end up with negative dentries for all >> > components except full one and we will instantiate them later if >> > such a mount is requested. >> > >> > Signed-off-by: Pavel Shilovsky <piastry@etersoft.ru> >> > --- >> > fs/cifs/cifsfs.c | 123 +++++++++++++++++++++++++++++++---------------------- >> > fs/cifs/cifsfs.h | 3 +- >> > fs/cifs/inode.c | 7 ++- >> > fs/cifs/readdir.c | 9 +++- >> > 4 files changed, 85 insertions(+), 57 deletions(-) >> > >> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c >> > index 0435bb9..33a2e1e 100644 >> > --- a/fs/cifs/cifsfs.c >> > +++ b/fs/cifs/cifsfs.c >> > @@ -95,14 +95,13 @@ mempool_t *smb2_mid_poolp; >> > static struct kmem_cache *smb2_mid_cachep; >> > #endif /* CONFIG_CIFS_SMB2 */ >> > >> > -static int >> > +static void >> > cifs_read_super(struct super_block *sb) >> > { >> > - struct inode *inode; >> > - struct cifs_sb_info *cifs_sb; >> > - int rc = 0; >> > + struct cifs_sb_info *cifs_sb = CIFS_SB(sb); >> > >> > - cifs_sb = CIFS_SB(sb); >> > + /* BB should we make this contingent on mount parm? */ >> > + sb->s_flags |= MS_NODIRATIME | MS_NOATIME; >> > >> > if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIXACL) >> > sb->s_flags |= MS_POSIXACL; >> > @@ -120,26 +119,6 @@ cifs_read_super(struct super_block *sb) >> > sb->s_bdi = &cifs_sb->bdi; >> > sb->s_blocksize = CIFS_MAX_MSGSIZE; >> > sb->s_blocksize_bits = 14; /* default 2**14 = CIFS_MAX_MSGSIZE */ >> > - inode = cifs_root_iget(sb); >> > - >> > - if (IS_ERR(inode)) { >> > - rc = PTR_ERR(inode); >> > - inode = NULL; >> > - goto out_no_root; >> > - } >> > - >> > - sb->s_root = d_alloc_root(inode); >> > - >> > - if (!sb->s_root) { >> > - rc = -ENOMEM; >> > - goto out_no_root; >> > - } >> > - >> > - /* do that *after* d_alloc_root() - we want NULL ->d_op for root here */ >> > - if (cifs_sb_master_tcon(cifs_sb)->nocase) >> > - sb->s_d_op = &cifs_ci_dentry_ops; >> > - else >> > - sb->s_d_op = &cifs_dentry_ops; >> > >> > #ifdef CIFS_NFSD_EXPORT >> > if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM) { >> > @@ -148,14 +127,7 @@ cifs_read_super(struct super_block *sb) >> > } >> > #endif /* CIFS_NFSD_EXPORT */ >> > >> > - return 0; >> > - >> > -out_no_root: >> > - cERROR(1, "cifs_read_super: get root inode failed"); >> > - if (inode) >> > - iput(inode); >> > - >> > - return rc; >> > + sb->s_flags |= MS_ACTIVE; >> > } >> > >> > static void cifs_kill_sb(struct super_block *sb) >> > @@ -529,6 +501,17 @@ static const struct super_operations cifs_super_ops = { >> > #endif >> > }; >> > >> > +static struct dentry * >> > +cifs_alloc_root(struct super_block *sb) >> > +{ >> > + struct qstr q; >> > + >> > + q.name = "/"; >> > + q.len = 1; >> > + q.hash = full_name_hash(q.name, q.len); >> > + return d_alloc_pseudo(sb, &q); >> > +} >> > + >> > /* >> > * Get root dentry from superblock according to prefix path mount option. >> > * Return dentry with refcount + 1 on success and NULL otherwise. >> > @@ -536,8 +519,10 @@ static const struct super_operations cifs_super_ops = { >> > static struct dentry * >> > cifs_get_root(struct smb_vol *vol, struct super_block *sb) >> > { >> > - struct dentry *dentry; >> > + struct dentry *dentry, *found; >> > + struct inode *inode; >> > struct cifs_sb_info *cifs_sb = CIFS_SB(sb); >> > + struct qstr q; >> > char *full_path = NULL; >> > char *s, *p; >> > char sep; >> > @@ -550,13 +535,29 @@ cifs_get_root(struct smb_vol *vol, struct super_block *sb) >> > >> > cFYI(1, "Get root dentry for %s", full_path); >> > >> > + if (!sb->s_root) { >> > + sb->s_root = cifs_alloc_root(sb); >> > + if (IS_ERR(sb->s_root)) { >> > + dentry = ERR_PTR(-ENOMEM); >> > + goto out; >> > + } >> > + >> > + /* >> > + * do that *after* cifs_alloc_root() - we want NULL ->d_op for >> > + * root here >> > + */ >> > + if (cifs_sb_master_tcon(cifs_sb)->nocase) >> > + sb->s_d_op = &cifs_ci_dentry_ops; >> > + else >> > + sb->s_d_op = &cifs_dentry_ops; >> > + } >> > + >> > xid = GetXid(); >> > 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; >> > >> > /* skip separators */ >> > @@ -569,16 +570,45 @@ cifs_get_root(struct smb_vol *vol, struct super_block *sb) >> > while (*s && *s != sep) >> > s++; >> > >> > - mutex_lock(&dir->i_mutex); >> > - child = lookup_one_len(p, dentry, s - p); >> > - mutex_unlock(&dir->i_mutex); >> > + q.name = p; >> > + q.len = s - p; >> > + if (dentry->d_flags & DCACHE_OP_HASH) >> > + dentry->d_op->d_hash(dentry, dentry->d_inode, &q); >> > + else >> > + q.hash = full_name_hash(q.name, q.len); >> > + >> > + child = d_lookup(dentry, &q); >> > + if (!child) { >> > + child = d_alloc(dentry, &q); >> > + if (IS_ERR(child)) { >> > + dput(dentry); >> > + dentry = ERR_CAST(child); >> > + break; >> > + } >> > + d_rehash(child); >> > + } >> > dput(dentry); >> > dentry = child; >> > - if (!dentry->d_inode) { >> > + } while (!IS_ERR(dentry)); >> > + >> > + if (IS_ERR(dentry)) >> > + goto out; >> > + >> > + if (!dentry->d_inode) { >> > + inode = cifs_mntroot_iget(sb, full_path); >> > + if (IS_ERR(inode)) { >> > dput(dentry); >> > - dentry = ERR_PTR(-ENOENT); >> > + dentry = ERR_CAST(inode); >> > + goto out; >> > } >> > - } while (!IS_ERR(dentry)); >> > + >> > + found = d_instantiate_unique(dentry, inode); >> > + if (found) { >> > + dput(dentry); >> > + dentry = dget(found); >> >> it seems like found dentry has been already got by >> d_instantiate_unique - we don't need to call dget again here >> > > That sounds right. > >> > + } >> > + } >> >> also, I think that if (!dentry->d_inode) {} statement should be >> protected by a lock (mutex) - I am not sure should we create new lock >> for this or use existing one. What d you think about it? >> > > I guess you're worried about the dentry suddenly becoming negative > after checking for it? If you have an active reference to it, then > that shouldn't occur. See d_delete(). > No, I mean the following situation: 1) process1 gets a dentry and comes into if (!dentry->d_inode) {} 2) process2 gets the same dentry and comes into if {} too (because dentry is still negative). 3) process1 gets a new inode 4) process2 gets a new inode (e.g. it doesn't get an inode created by process1 in noserverino case) 5) denty is instantiated twice - wrong! if we protect if statement with a mutex - it won't happen. Should we create extra one for this? >> > +out: >> > _FreeXid(xid); >> > kfree(full_path); >> > return dentry; >> > @@ -646,16 +676,7 @@ cifs_do_mount(struct file_system_type *fs_type, >> > cifs_umount(cifs_sb); >> > } else { >> > sb->s_flags = flags; >> > - /* BB should we make this contingent on mount parm? */ >> > - sb->s_flags |= MS_NODIRATIME | MS_NOATIME; >> > - >> > - rc = cifs_read_super(sb); >> > - if (rc) { >> > - root = ERR_PTR(rc); >> > - goto out_super; >> > - } >> > - >> > - sb->s_flags |= MS_ACTIVE; >> > + cifs_read_super(sb); >> > } >> > >> > root = cifs_get_root(volume_info, sb); >> > diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h >> > index 3145c18..47d9ec9 100644 >> > --- a/fs/cifs/cifsfs.h >> > +++ b/fs/cifs/cifsfs.h >> > @@ -43,7 +43,8 @@ extern const struct address_space_operations cifs_addr_ops_smallbuf; >> > >> > /* Functions related to inodes */ >> > extern const struct inode_operations cifs_dir_inode_ops; >> > -extern struct inode *cifs_root_iget(struct super_block *); >> > +extern struct inode *cifs_mntroot_iget(struct super_block *sb, >> > + const char *full_path); >> > extern int cifs_create(struct inode *, struct dentry *, int, >> > struct nameidata *); >> > extern struct dentry *cifs_lookup(struct inode *, struct dentry *, >> > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c >> > index aee0c0b..dedfed3 100644 >> > --- a/fs/cifs/inode.c >> > +++ b/fs/cifs/inode.c >> > @@ -931,7 +931,7 @@ retry_iget5_locked: >> > } >> > >> > /* gets root inode */ >> > -struct inode *cifs_root_iget(struct super_block *sb) >> > +struct inode *cifs_mntroot_iget(struct super_block *sb, const char *full_path) >> > { >> > int xid; >> > struct cifs_sb_info *cifs_sb = CIFS_SB(sb); >> > @@ -941,9 +941,10 @@ struct inode *cifs_root_iget(struct super_block *sb) >> > >> > xid = GetXid(); >> > if (tcon->unix_ext) >> > - rc = cifs_get_inode_info_unix(&inode, "", sb, xid); >> > + rc = cifs_get_inode_info_unix(&inode, full_path, sb, xid); >> > else >> > - rc = cifs_get_inode_info(&inode, "", NULL, sb, xid, NULL); >> > + rc = cifs_get_inode_info(&inode, full_path, NULL, sb, xid, >> > + NULL); >> > >> > if (!inode) { >> > inode = ERR_PTR(rc); >> > diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c >> > index 75e8b5c..7475cba 100644 >> > --- a/fs/cifs/readdir.c >> > +++ b/fs/cifs/readdir.c >> > @@ -708,6 +708,7 @@ int cifs_readdir(struct file *file, void *direntry, filldir_t filldir) >> > char *tmp_buf = NULL; >> > char *end_of_smb; >> > unsigned int max_len; >> > + ino_t ino; >> > >> > xid = GetXid(); >> > >> > @@ -732,8 +733,12 @@ int cifs_readdir(struct file *file, void *direntry, filldir_t filldir) >> > } >> > file->f_pos++; >> > case 1: >> > - if (filldir(direntry, "..", 2, file->f_pos, >> > - parent_ino(file->f_path.dentry), DT_DIR) < 0) { >> > + if (!file->f_path.dentry->d_parent->d_inode) { >> > + cFYI(1, "parent dir is negative, filling as current"); >> > + ino = file->f_path.dentry->d_inode->i_ino; >> > + } else >> > + ino = parent_ino(file->f_path.dentry); >> > + if (filldir(direntry, "..", 2, file->f_pos, ino, DT_DIR) < 0) { >> > cERROR(1, "Filldir for parent dir failed"); >> > rc = -ENOMEM; >> > break; >> > -- >> > 1.7.1 >> > >> > >> >> >> > > > -- > Jeff Layton <jlayton@redhat.com> >
On Wed, 31 Aug 2011 21:35:39 +0400 Pavel Shilovsky <piastry@etersoft.ru> wrote: > 2011/8/30 Jeff Layton <jlayton@redhat.com>: > > On Tue, 30 Aug 2011 16:31:52 +0400 > > Pavel Shilovsky <piastry@etersoft.ru> wrote: > > > >> 2011/8/26 Pavel Shilovsky <piastry@etersoft.ru>: > >> > Reorganize code and make it send qpath info request only for a full > >> > path (//server/share/prefixpath) rather than request for every path > >> > compoment. In this case we end up with negative dentries for all > >> > components except full one and we will instantiate them later if > >> > such a mount is requested. > >> > > >> > Signed-off-by: Pavel Shilovsky <piastry@etersoft.ru> > >> > --- > >> > fs/cifs/cifsfs.c | 123 +++++++++++++++++++++++++++++++---------------------- > >> > fs/cifs/cifsfs.h | 3 +- > >> > fs/cifs/inode.c | 7 ++- > >> > fs/cifs/readdir.c | 9 +++- > >> > 4 files changed, 85 insertions(+), 57 deletions(-) > >> > > >> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c > >> > index 0435bb9..33a2e1e 100644 > >> > --- a/fs/cifs/cifsfs.c > >> > +++ b/fs/cifs/cifsfs.c > >> > @@ -95,14 +95,13 @@ mempool_t *smb2_mid_poolp; > >> > static struct kmem_cache *smb2_mid_cachep; > >> > #endif /* CONFIG_CIFS_SMB2 */ > >> > > >> > -static int > >> > +static void > >> > cifs_read_super(struct super_block *sb) > >> > { > >> > - struct inode *inode; > >> > - struct cifs_sb_info *cifs_sb; > >> > - int rc = 0; > >> > + struct cifs_sb_info *cifs_sb = CIFS_SB(sb); > >> > > >> > - cifs_sb = CIFS_SB(sb); > >> > + /* BB should we make this contingent on mount parm? */ > >> > + sb->s_flags |= MS_NODIRATIME | MS_NOATIME; > >> > > >> > if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIXACL) > >> > sb->s_flags |= MS_POSIXACL; > >> > @@ -120,26 +119,6 @@ cifs_read_super(struct super_block *sb) > >> > sb->s_bdi = &cifs_sb->bdi; > >> > sb->s_blocksize = CIFS_MAX_MSGSIZE; > >> > sb->s_blocksize_bits = 14; /* default 2**14 = CIFS_MAX_MSGSIZE */ > >> > - inode = cifs_root_iget(sb); > >> > - > >> > - if (IS_ERR(inode)) { > >> > - rc = PTR_ERR(inode); > >> > - inode = NULL; > >> > - goto out_no_root; > >> > - } > >> > - > >> > - sb->s_root = d_alloc_root(inode); > >> > - > >> > - if (!sb->s_root) { > >> > - rc = -ENOMEM; > >> > - goto out_no_root; > >> > - } > >> > - > >> > - /* do that *after* d_alloc_root() - we want NULL ->d_op for root here */ > >> > - if (cifs_sb_master_tcon(cifs_sb)->nocase) > >> > - sb->s_d_op = &cifs_ci_dentry_ops; > >> > - else > >> > - sb->s_d_op = &cifs_dentry_ops; > >> > > >> > #ifdef CIFS_NFSD_EXPORT > >> > if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM) { > >> > @@ -148,14 +127,7 @@ cifs_read_super(struct super_block *sb) > >> > } > >> > #endif /* CIFS_NFSD_EXPORT */ > >> > > >> > - return 0; > >> > - > >> > -out_no_root: > >> > - cERROR(1, "cifs_read_super: get root inode failed"); > >> > - if (inode) > >> > - iput(inode); > >> > - > >> > - return rc; > >> > + sb->s_flags |= MS_ACTIVE; > >> > } > >> > > >> > static void cifs_kill_sb(struct super_block *sb) > >> > @@ -529,6 +501,17 @@ static const struct super_operations cifs_super_ops = { > >> > #endif > >> > }; > >> > > >> > +static struct dentry * > >> > +cifs_alloc_root(struct super_block *sb) > >> > +{ > >> > + struct qstr q; > >> > + > >> > + q.name = "/"; > >> > + q.len = 1; > >> > + q.hash = full_name_hash(q.name, q.len); > >> > + return d_alloc_pseudo(sb, &q); > >> > +} > >> > + > >> > /* > >> > * Get root dentry from superblock according to prefix path mount option. > >> > * Return dentry with refcount + 1 on success and NULL otherwise. > >> > @@ -536,8 +519,10 @@ static const struct super_operations cifs_super_ops = { > >> > static struct dentry * > >> > cifs_get_root(struct smb_vol *vol, struct super_block *sb) > >> > { > >> > - struct dentry *dentry; > >> > + struct dentry *dentry, *found; > >> > + struct inode *inode; > >> > struct cifs_sb_info *cifs_sb = CIFS_SB(sb); > >> > + struct qstr q; > >> > char *full_path = NULL; > >> > char *s, *p; > >> > char sep; > >> > @@ -550,13 +535,29 @@ cifs_get_root(struct smb_vol *vol, struct super_block *sb) > >> > > >> > cFYI(1, "Get root dentry for %s", full_path); > >> > > >> > + if (!sb->s_root) { > >> > + sb->s_root = cifs_alloc_root(sb); > >> > + if (IS_ERR(sb->s_root)) { > >> > + dentry = ERR_PTR(-ENOMEM); > >> > + goto out; > >> > + } > >> > + > >> > + /* > >> > + * do that *after* cifs_alloc_root() - we want NULL ->d_op for > >> > + * root here > >> > + */ > >> > + if (cifs_sb_master_tcon(cifs_sb)->nocase) > >> > + sb->s_d_op = &cifs_ci_dentry_ops; > >> > + else > >> > + sb->s_d_op = &cifs_dentry_ops; > >> > + } > >> > + > >> > xid = GetXid(); > >> > 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; > >> > > >> > /* skip separators */ > >> > @@ -569,16 +570,45 @@ cifs_get_root(struct smb_vol *vol, struct super_block *sb) > >> > while (*s && *s != sep) > >> > s++; > >> > > >> > - mutex_lock(&dir->i_mutex); > >> > - child = lookup_one_len(p, dentry, s - p); > >> > - mutex_unlock(&dir->i_mutex); > >> > + q.name = p; > >> > + q.len = s - p; > >> > + if (dentry->d_flags & DCACHE_OP_HASH) > >> > + dentry->d_op->d_hash(dentry, dentry->d_inode, &q); > >> > + else > >> > + q.hash = full_name_hash(q.name, q.len); > >> > + > >> > + child = d_lookup(dentry, &q); > >> > + if (!child) { > >> > + child = d_alloc(dentry, &q); > >> > + if (IS_ERR(child)) { > >> > + dput(dentry); > >> > + dentry = ERR_CAST(child); > >> > + break; > >> > + } > >> > + d_rehash(child); > >> > + } > >> > dput(dentry); > >> > dentry = child; > >> > - if (!dentry->d_inode) { > >> > + } while (!IS_ERR(dentry)); > >> > + > >> > + if (IS_ERR(dentry)) > >> > + goto out; > >> > + > >> > + if (!dentry->d_inode) { > >> > + inode = cifs_mntroot_iget(sb, full_path); > >> > + if (IS_ERR(inode)) { > >> > dput(dentry); > >> > - dentry = ERR_PTR(-ENOENT); > >> > + dentry = ERR_CAST(inode); > >> > + goto out; > >> > } > >> > - } while (!IS_ERR(dentry)); > >> > + > >> > + found = d_instantiate_unique(dentry, inode); > >> > + if (found) { > >> > + dput(dentry); > >> > + dentry = dget(found); > >> > >> it seems like found dentry has been already got by > >> d_instantiate_unique - we don't need to call dget again here > >> > > > > That sounds right. > > > >> > + } > >> > + } > >> > >> also, I think that if (!dentry->d_inode) {} statement should be > >> protected by a lock (mutex) - I am not sure should we create new lock > >> for this or use existing one. What d you think about it? > >> > > > > I guess you're worried about the dentry suddenly becoming negative > > after checking for it? If you have an active reference to it, then > > that shouldn't occur. See d_delete(). > > > > No, I mean the following situation: > 1) process1 gets a dentry and comes into if (!dentry->d_inode) {} > 2) process2 gets the same dentry and comes into if {} too (because > dentry is still negative). > 3) process1 gets a new inode > 4) process2 gets a new inode (e.g. it doesn't get an inode created by > process1 in noserverino case) > 5) denty is instantiated twice - wrong! > > if we protect if statement with a mutex - it won't happen. Should we > create extra one for this? > I think you need to make sure you're holding the i_mutex of the parent for this. Most of these sorts of dentry operations require it.
2011/8/31 Jeff Layton <jlayton@redhat.com>: > I think you need to make sure you're holding the i_mutex of the parent > for this. Most of these sorts of dentry operations require it. > In the case of this patch, we can have a negative parent - so, can't hold i_mutex here. It seems to me that we need extra cifs mutex for dealing this it.
On Wed, 31 Aug 2011 22:27:25 +0400 Pavel Shilovsky <piastry@etersoft.ru> wrote: > 2011/8/31 Jeff Layton <jlayton@redhat.com>: > > I think you need to make sure you're holding the i_mutex of the parent > > for this. Most of these sorts of dentry operations require it. > > > > In the case of this patch, we can have a negative parent - so, can't > hold i_mutex here. It seems to me that we need extra cifs mutex for > dealing this it. > In the event of a positive parent, how do you intend to ensure that the dentry is not instantiated via other codepaths (lookup or readdir?) It's not clear to me though that this is really a problem though in any case. In the event that you "re-instantiate" the dentry, the old inode will just end up being put (and likely eventually freed). Sure, the inode number might change, but that's life with noserverino.
2011/8/31 Jeff Layton <jlayton@redhat.com>: > On Wed, 31 Aug 2011 22:27:25 +0400 > Pavel Shilovsky <piastry@etersoft.ru> wrote: > >> 2011/8/31 Jeff Layton <jlayton@redhat.com>: >> > I think you need to make sure you're holding the i_mutex of the parent >> > for this. Most of these sorts of dentry operations require it. >> > >> >> In the case of this patch, we can have a negative parent - so, can't >> hold i_mutex here. It seems to me that we need extra cifs mutex for >> dealing this it. >> > > In the event of a positive parent, how do you intend to ensure that the > dentry is not instantiated via other codepaths (lookup or readdir?) If dentry is negative we don't have it reachable from userspace (cifs mount point always starts with a positive one) - no lookups and readdirs from here. If I am mistaken, point me to it, please. > It's not clear to me though that this is really a problem though in any > case. In the event that you "re-instantiate" the dentry, the old inode > will just end up being put (and likely eventually freed). Sure, the > inode number might change, but that's life with noserverino. > > -- > Jeff Layton <jlayton@redhat.com> >
On Wed, 31 Aug 2011 23:00:45 +0400 Pavel Shilovsky <piastry@etersoft.ru> wrote: > 2011/8/31 Jeff Layton <jlayton@redhat.com>: > > On Wed, 31 Aug 2011 22:27:25 +0400 > > Pavel Shilovsky <piastry@etersoft.ru> wrote: > > > >> 2011/8/31 Jeff Layton <jlayton@redhat.com>: > >> > I think you need to make sure you're holding the i_mutex of the parent > >> > for this. Most of these sorts of dentry operations require it. > >> > > >> > >> In the case of this patch, we can have a negative parent - so, can't > >> hold i_mutex here. It seems to me that we need extra cifs mutex for > >> dealing this it. > >> > > > > In the event of a positive parent, how do you intend to ensure that the > > dentry is not instantiated via other codepaths (lookup or readdir?) > > If dentry is negative we don't have it reachable from userspace (cifs > mount point always starts with a positive one) - no lookups and > readdirs from here. If I am mistaken, point me to it, please. > If the parent dentry is negative, you mean. Ok, Good point... Ok, so you're proposing to wrap this check in a big mutex to prevent this race when the parent dentry is negative. In the event that it's positive though, you'll need to make sure that you take the parent's i_mutex. I don't have a great feel for this sort of dcache trickery, but this whole scheme sounds like it's going to break a lot of assumptions about how the dcache works. In particular having negative dentries with children sounds very problematic. Would it be better to simply make the initial dentry in this case be a disconnected or root dentry (see DCACHE_DISCONNECTED), and then later "graft" it into a larger tree if the parent comes to life? > > It's not clear to me though that this is really a problem though in any > > case. In the event that you "re-instantiate" the dentry, the old inode > > will just end up being put (and likely eventually freed). Sure, the > > inode number might change, but that's life with noserverino. > > > > -- > > Jeff Layton <jlayton@redhat.com> > > >
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c index 0435bb9..33a2e1e 100644 --- a/fs/cifs/cifsfs.c +++ b/fs/cifs/cifsfs.c @@ -95,14 +95,13 @@ mempool_t *smb2_mid_poolp; static struct kmem_cache *smb2_mid_cachep; #endif /* CONFIG_CIFS_SMB2 */ -static int +static void cifs_read_super(struct super_block *sb) { - struct inode *inode; - struct cifs_sb_info *cifs_sb; - int rc = 0; + struct cifs_sb_info *cifs_sb = CIFS_SB(sb); - cifs_sb = CIFS_SB(sb); + /* BB should we make this contingent on mount parm? */ + sb->s_flags |= MS_NODIRATIME | MS_NOATIME; if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIXACL) sb->s_flags |= MS_POSIXACL; @@ -120,26 +119,6 @@ cifs_read_super(struct super_block *sb) sb->s_bdi = &cifs_sb->bdi; sb->s_blocksize = CIFS_MAX_MSGSIZE; sb->s_blocksize_bits = 14; /* default 2**14 = CIFS_MAX_MSGSIZE */ - inode = cifs_root_iget(sb); - - if (IS_ERR(inode)) { - rc = PTR_ERR(inode); - inode = NULL; - goto out_no_root; - } - - sb->s_root = d_alloc_root(inode); - - if (!sb->s_root) { - rc = -ENOMEM; - goto out_no_root; - } - - /* do that *after* d_alloc_root() - we want NULL ->d_op for root here */ - if (cifs_sb_master_tcon(cifs_sb)->nocase) - sb->s_d_op = &cifs_ci_dentry_ops; - else - sb->s_d_op = &cifs_dentry_ops; #ifdef CIFS_NFSD_EXPORT if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM) { @@ -148,14 +127,7 @@ cifs_read_super(struct super_block *sb) } #endif /* CIFS_NFSD_EXPORT */ - return 0; - -out_no_root: - cERROR(1, "cifs_read_super: get root inode failed"); - if (inode) - iput(inode); - - return rc; + sb->s_flags |= MS_ACTIVE; } static void cifs_kill_sb(struct super_block *sb) @@ -529,6 +501,17 @@ static const struct super_operations cifs_super_ops = { #endif }; +static struct dentry * +cifs_alloc_root(struct super_block *sb) +{ + struct qstr q; + + q.name = "/"; + q.len = 1; + q.hash = full_name_hash(q.name, q.len); + return d_alloc_pseudo(sb, &q); +} + /* * Get root dentry from superblock according to prefix path mount option. * Return dentry with refcount + 1 on success and NULL otherwise. @@ -536,8 +519,10 @@ static const struct super_operations cifs_super_ops = { static struct dentry * cifs_get_root(struct smb_vol *vol, struct super_block *sb) { - struct dentry *dentry; + struct dentry *dentry, *found; + struct inode *inode; struct cifs_sb_info *cifs_sb = CIFS_SB(sb); + struct qstr q; char *full_path = NULL; char *s, *p; char sep; @@ -550,13 +535,29 @@ cifs_get_root(struct smb_vol *vol, struct super_block *sb) cFYI(1, "Get root dentry for %s", full_path); + if (!sb->s_root) { + sb->s_root = cifs_alloc_root(sb); + if (IS_ERR(sb->s_root)) { + dentry = ERR_PTR(-ENOMEM); + goto out; + } + + /* + * do that *after* cifs_alloc_root() - we want NULL ->d_op for + * root here + */ + if (cifs_sb_master_tcon(cifs_sb)->nocase) + sb->s_d_op = &cifs_ci_dentry_ops; + else + sb->s_d_op = &cifs_dentry_ops; + } + xid = GetXid(); 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; /* skip separators */ @@ -569,16 +570,45 @@ cifs_get_root(struct smb_vol *vol, struct super_block *sb) while (*s && *s != sep) s++; - mutex_lock(&dir->i_mutex); - child = lookup_one_len(p, dentry, s - p); - mutex_unlock(&dir->i_mutex); + q.name = p; + q.len = s - p; + if (dentry->d_flags & DCACHE_OP_HASH) + dentry->d_op->d_hash(dentry, dentry->d_inode, &q); + else + q.hash = full_name_hash(q.name, q.len); + + child = d_lookup(dentry, &q); + if (!child) { + child = d_alloc(dentry, &q); + if (IS_ERR(child)) { + dput(dentry); + dentry = ERR_CAST(child); + break; + } + d_rehash(child); + } dput(dentry); dentry = child; - if (!dentry->d_inode) { + } while (!IS_ERR(dentry)); + + if (IS_ERR(dentry)) + goto out; + + if (!dentry->d_inode) { + inode = cifs_mntroot_iget(sb, full_path); + if (IS_ERR(inode)) { dput(dentry); - dentry = ERR_PTR(-ENOENT); + dentry = ERR_CAST(inode); + goto out; } - } while (!IS_ERR(dentry)); + + found = d_instantiate_unique(dentry, inode); + if (found) { + dput(dentry); + dentry = dget(found); + } + } +out: _FreeXid(xid); kfree(full_path); return dentry; @@ -646,16 +676,7 @@ cifs_do_mount(struct file_system_type *fs_type, cifs_umount(cifs_sb); } else { sb->s_flags = flags; - /* BB should we make this contingent on mount parm? */ - sb->s_flags |= MS_NODIRATIME | MS_NOATIME; - - rc = cifs_read_super(sb); - if (rc) { - root = ERR_PTR(rc); - goto out_super; - } - - sb->s_flags |= MS_ACTIVE; + cifs_read_super(sb); } root = cifs_get_root(volume_info, sb); diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h index 3145c18..47d9ec9 100644 --- a/fs/cifs/cifsfs.h +++ b/fs/cifs/cifsfs.h @@ -43,7 +43,8 @@ extern const struct address_space_operations cifs_addr_ops_smallbuf; /* Functions related to inodes */ extern const struct inode_operations cifs_dir_inode_ops; -extern struct inode *cifs_root_iget(struct super_block *); +extern struct inode *cifs_mntroot_iget(struct super_block *sb, + const char *full_path); extern int cifs_create(struct inode *, struct dentry *, int, struct nameidata *); extern struct dentry *cifs_lookup(struct inode *, struct dentry *, diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c index aee0c0b..dedfed3 100644 --- a/fs/cifs/inode.c +++ b/fs/cifs/inode.c @@ -931,7 +931,7 @@ retry_iget5_locked: } /* gets root inode */ -struct inode *cifs_root_iget(struct super_block *sb) +struct inode *cifs_mntroot_iget(struct super_block *sb, const char *full_path) { int xid; struct cifs_sb_info *cifs_sb = CIFS_SB(sb); @@ -941,9 +941,10 @@ struct inode *cifs_root_iget(struct super_block *sb) xid = GetXid(); if (tcon->unix_ext) - rc = cifs_get_inode_info_unix(&inode, "", sb, xid); + rc = cifs_get_inode_info_unix(&inode, full_path, sb, xid); else - rc = cifs_get_inode_info(&inode, "", NULL, sb, xid, NULL); + rc = cifs_get_inode_info(&inode, full_path, NULL, sb, xid, + NULL); if (!inode) { inode = ERR_PTR(rc); diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c index 75e8b5c..7475cba 100644 --- a/fs/cifs/readdir.c +++ b/fs/cifs/readdir.c @@ -708,6 +708,7 @@ int cifs_readdir(struct file *file, void *direntry, filldir_t filldir) char *tmp_buf = NULL; char *end_of_smb; unsigned int max_len; + ino_t ino; xid = GetXid(); @@ -732,8 +733,12 @@ int cifs_readdir(struct file *file, void *direntry, filldir_t filldir) } file->f_pos++; case 1: - if (filldir(direntry, "..", 2, file->f_pos, - parent_ino(file->f_path.dentry), DT_DIR) < 0) { + if (!file->f_path.dentry->d_parent->d_inode) { + cFYI(1, "parent dir is negative, filling as current"); + ino = file->f_path.dentry->d_inode->i_ino; + } else + ino = parent_ino(file->f_path.dentry); + if (filldir(direntry, "..", 2, file->f_pos, ino, DT_DIR) < 0) { cERROR(1, "Filldir for parent dir failed"); rc = -ENOMEM; break;
Reorganize code and make it send qpath info request only for a full path (//server/share/prefixpath) rather than request for every path compoment. In this case we end up with negative dentries for all components except full one and we will instantiate them later if such a mount is requested. Signed-off-by: Pavel Shilovsky <piastry@etersoft.ru> --- fs/cifs/cifsfs.c | 123 +++++++++++++++++++++++++++++++---------------------- fs/cifs/cifsfs.h | 3 +- fs/cifs/inode.c | 7 ++- fs/cifs/readdir.c | 9 +++- 4 files changed, 85 insertions(+), 57 deletions(-)