* [PATCH] CIFS: Fix NT_STATUS_ACCESS_DENIED for mounts with prefixpath option
@ 2011-08-26 6:52 Pavel Shilovsky
[not found] ` <1314341541-2476-1-git-send-email-piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
0 siblings, 1 reply; 15+ messages in thread
From: Pavel Shilovsky @ 2011-08-26 6:52 UTC (permalink / raw)
To: Jeff Layton, Al Viro; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA
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-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
---
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);
+ }
+ }
+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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] CIFS: Fix NT_STATUS_ACCESS_DENIED for mounts with prefixpath option
[not found] ` <1314341541-2476-1-git-send-email-piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
@ 2011-08-26 11:34 ` Jeff Layton
[not found] ` <20110826073437.743c06a3-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
2011-08-30 12:31 ` Pavel Shilovsky
1 sibling, 1 reply; 15+ messages in thread
From: Jeff Layton @ 2011-08-26 11:34 UTC (permalink / raw)
To: Pavel Shilovsky
Cc: Al Viro, linux-cifs-u79uwXL29TY76Z2rM5mHXA,
dhowells-H+wXaHxf7aLQT0dZR+AlfA
On Fri, 26 Aug 2011 10:52:21 +0400
Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> 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-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> ---
> 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 "".
> + 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;
(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 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.
David, any thoughts here?
--
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] CIFS: Fix NT_STATUS_ACCESS_DENIED for mounts with prefixpath option
[not found] ` <20110826073437.743c06a3-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
@ 2011-08-26 12:36 ` Pavel Shilovsky
2011-08-26 20:11 ` Pavel Shilovsky
1 sibling, 0 replies; 15+ messages in thread
From: Pavel Shilovsky @ 2011-08-26 12:36 UTC (permalink / raw)
To: Jeff Layton
Cc: Al Viro, linux-cifs-u79uwXL29TY76Z2rM5mHXA,
dhowells-H+wXaHxf7aLQT0dZR+AlfA
2011/8/26 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
...
>> +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?
--
Best regards,
Pavel Shilovsky.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] CIFS: Fix NT_STATUS_ACCESS_DENIED for mounts with prefixpath option
[not found] ` <20110826073437.743c06a3-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
2011-08-26 12:36 ` Pavel Shilovsky
@ 2011-08-26 20:11 ` Pavel Shilovsky
[not found] ` <CAKywueQQMuTA=xszQYr6sf5L+cZ_s56qrCiL8MqthWS-Xbxw5A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
1 sibling, 1 reply; 15+ messages in thread
From: Pavel Shilovsky @ 2011-08-26 20:11 UTC (permalink / raw)
To: Jeff Layton
Cc: Al Viro, linux-cifs-u79uwXL29TY76Z2rM5mHXA,
dhowells-H+wXaHxf7aLQT0dZR+AlfA
2011/8/26 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> On Fri, 26 Aug 2011 10:52:21 +0400
> Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> 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-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
>> ---
>> 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 ""?
--
Best regards,
Pavel Shilovsky.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] CIFS: Fix NT_STATUS_ACCESS_DENIED for mounts with prefixpath option
[not found] ` <CAKywueQQMuTA=xszQYr6sf5L+cZ_s56qrCiL8MqthWS-Xbxw5A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2011-08-26 21:23 ` Jeff Layton
0 siblings, 0 replies; 15+ messages in thread
From: Jeff Layton @ 2011-08-26 21:23 UTC (permalink / raw)
To: Pavel Shilovsky
Cc: Al Viro, linux-cifs-u79uwXL29TY76Z2rM5mHXA,
dhowells-H+wXaHxf7aLQT0dZR+AlfA
On Sat, 27 Aug 2011 00:11:57 +0400
Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
> 2011/8/26 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> > On Fri, 26 Aug 2011 10:52:21 +0400
> > Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> 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-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> >> ---
> >> 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.
--
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] CIFS: Fix NT_STATUS_ACCESS_DENIED for mounts with prefixpath option
[not found] ` <1314341541-2476-1-git-send-email-piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
2011-08-26 11:34 ` Jeff Layton
@ 2011-08-30 12:31 ` Pavel Shilovsky
[not found] ` <CAKywueRzwQPROzWTNwhbSOeEanV+Pumn=vFDmYL0uuEApfsX9A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
1 sibling, 1 reply; 15+ messages in thread
From: Pavel Shilovsky @ 2011-08-30 12:31 UTC (permalink / raw)
To: Jeff Layton, Al Viro; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA
2011/8/26 Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>:
> 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-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> ---
> 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
>
>
--
Best regards,
Pavel Shilovsky.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] CIFS: Fix NT_STATUS_ACCESS_DENIED for mounts with prefixpath option
[not found] ` <CAKywueRzwQPROzWTNwhbSOeEanV+Pumn=vFDmYL0uuEApfsX9A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2011-08-30 12:59 ` Jeff Layton
[not found] ` <20110830085935.71e671e4-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
0 siblings, 1 reply; 15+ messages in thread
From: Jeff Layton @ 2011-08-30 12:59 UTC (permalink / raw)
To: Pavel Shilovsky; +Cc: Al Viro, linux-cifs-u79uwXL29TY76Z2rM5mHXA
On Tue, 30 Aug 2011 16:31:52 +0400
Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
> 2011/8/26 Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>:
> > 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-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> > ---
> > 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
> >
> >
>
>
>
--
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] CIFS: Fix NT_STATUS_ACCESS_DENIED for mounts with prefixpath option
[not found] ` <20110830085935.71e671e4-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
@ 2011-08-31 17:35 ` Pavel Shilovsky
[not found] ` <CAKywueSy2tFQC4mxOBZYjiY=PyLjUZ44v+qvtNNg+zKoxJmeCw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 15+ messages in thread
From: Pavel Shilovsky @ 2011-08-31 17:35 UTC (permalink / raw)
To: Jeff Layton; +Cc: Al Viro, linux-cifs-u79uwXL29TY76Z2rM5mHXA
2011/8/30 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> On Tue, 30 Aug 2011 16:31:52 +0400
> Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
>
>> 2011/8/26 Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>:
>> > 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-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
>> > ---
>> > 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>
--
Best regards,
Pavel Shilovsky.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] CIFS: Fix NT_STATUS_ACCESS_DENIED for mounts with prefixpath option
[not found] ` <CAKywueSy2tFQC4mxOBZYjiY=PyLjUZ44v+qvtNNg+zKoxJmeCw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2011-08-31 18:00 ` Jeff Layton
[not found] ` <20110831140044.5ac4a691-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
0 siblings, 1 reply; 15+ messages in thread
From: Jeff Layton @ 2011-08-31 18:00 UTC (permalink / raw)
To: Pavel Shilovsky; +Cc: Al Viro, linux-cifs-u79uwXL29TY76Z2rM5mHXA
On Wed, 31 Aug 2011 21:35:39 +0400
Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
> 2011/8/30 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> > On Tue, 30 Aug 2011 16:31:52 +0400
> > Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
> >
> >> 2011/8/26 Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>:
> >> > 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-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> >> > ---
> >> > 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.
--
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] CIFS: Fix NT_STATUS_ACCESS_DENIED for mounts with prefixpath option
[not found] ` <20110831140044.5ac4a691-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
@ 2011-08-31 18:27 ` Pavel Shilovsky
2011-08-31 18:41 ` Jeff Layton
0 siblings, 1 reply; 15+ messages in thread
From: Pavel Shilovsky @ 2011-08-31 18:27 UTC (permalink / raw)
To: Jeff Layton; +Cc: Al Viro, linux-cifs-u79uwXL29TY76Z2rM5mHXA
2011/8/31 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> 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.
--
Best regards,
Pavel Shilovsky.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] CIFS: Fix NT_STATUS_ACCESS_DENIED for mounts with prefixpath option
2011-08-31 18:27 ` Pavel Shilovsky
@ 2011-08-31 18:41 ` Jeff Layton
[not found] ` <20110831144152.788c2543-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
0 siblings, 1 reply; 15+ messages in thread
From: Jeff Layton @ 2011-08-31 18:41 UTC (permalink / raw)
To: Pavel Shilovsky; +Cc: Al Viro, linux-cifs-u79uwXL29TY76Z2rM5mHXA
On Wed, 31 Aug 2011 22:27:25 +0400
Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
> 2011/8/31 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> > 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.
--
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] CIFS: Fix NT_STATUS_ACCESS_DENIED for mounts with prefixpath option
[not found] ` <20110831144152.788c2543-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
@ 2011-08-31 19:00 ` Pavel Shilovsky
[not found] ` <CAKywueQB=xM1Uv=HBpBwHSqbDcq43aBm4gYD_XbUahP9fyy9vA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 15+ messages in thread
From: Pavel Shilovsky @ 2011-08-31 19:00 UTC (permalink / raw)
To: Jeff Layton; +Cc: Al Viro, linux-cifs-u79uwXL29TY76Z2rM5mHXA
2011/8/31 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> On Wed, 31 Aug 2011 22:27:25 +0400
> Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
>
>> 2011/8/31 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>> > 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>
--
Best regards,
Pavel Shilovsky.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] CIFS: Fix NT_STATUS_ACCESS_DENIED for mounts with prefixpath option
[not found] ` <CAKywueQB=xM1Uv=HBpBwHSqbDcq43aBm4gYD_XbUahP9fyy9vA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2011-08-31 19:22 ` Jeff Layton
[not found] ` <20110831152247.2f2cfcbd-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
0 siblings, 1 reply; 15+ messages in thread
From: Jeff Layton @ 2011-08-31 19:22 UTC (permalink / raw)
To: Pavel Shilovsky; +Cc: Al Viro, linux-cifs-u79uwXL29TY76Z2rM5mHXA
On Wed, 31 Aug 2011 23:00:45 +0400
Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
> 2011/8/31 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> > On Wed, 31 Aug 2011 22:27:25 +0400
> > Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
> >
> >> 2011/8/31 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> >> > 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >
>
--
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] CIFS: Fix NT_STATUS_ACCESS_DENIED for mounts with prefixpath option
[not found] ` <20110831152247.2f2cfcbd-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
@ 2011-10-04 9:01 ` Pavel Shilovsky
[not found] ` <CAKywueQ1shXsy1rSBJx1H7aD80wg-3=GbvKhLXHeH_nJRz3b2g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 15+ messages in thread
From: Pavel Shilovsky @ 2011-10-04 9:01 UTC (permalink / raw)
To: Jeff Layton; +Cc: Al Viro, linux-cifs-u79uwXL29TY76Z2rM5mHXA
Sorry for the long delay on this.
2011/8/31 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> 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.
Yes, that's what I meant.
>
> 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?
>
I am not sure that I understand you right. As I see what you suggest:
Let's predict we have mounted //server/share/a/ so
1. Dummy superblock root is set to //server/share/a/.
2. Get an inode and a dentry for //server/share/a/.
Then, we mount //server/share/, so
3. Get an inode and a dentry for //server/share/
Then, we reach //server/share/a from the second share, so we need to
find the existing dentry from the first share. We can store all
dentries that pass through cifs_lookup and cifs_get_root with a full
path hash and use them in future when such a path is requested.
Makes sense?
--
Best regards,
Pavel Shilovsky.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] CIFS: Fix NT_STATUS_ACCESS_DENIED for mounts with prefixpath option
[not found] ` <CAKywueQ1shXsy1rSBJx1H7aD80wg-3=GbvKhLXHeH_nJRz3b2g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2011-10-04 11:23 ` Jeff Layton
0 siblings, 0 replies; 15+ messages in thread
From: Jeff Layton @ 2011-10-04 11:23 UTC (permalink / raw)
To: Pavel Shilovsky
Cc: Al Viro, linux-cifs-u79uwXL29TY76Z2rM5mHXA,
dhowells-H+wXaHxf7aLQT0dZR+AlfA
On Tue, 4 Oct 2011 13:01:35 +0400
Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
> Sorry for the long delay on this.
>
> 2011/8/31 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> > 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.
>
> Yes, that's what I meant.
>
> >
> > 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?
> >
>
> I am not sure that I understand you right. As I see what you suggest:
>
> Let's predict we have mounted //server/share/a/ so
> 1. Dummy superblock root is set to //server/share/a/.
> 2. Get an inode and a dentry for //server/share/a/.
>
> Then, we mount //server/share/, so
> 3. Get an inode and a dentry for //server/share/
>
> Then, we reach //server/share/a from the second share, so we need to
> find the existing dentry from the first share. We can store all
> dentries that pass through cifs_lookup and cifs_get_root with a full
> path hash and use them in future when such a path is requested.
>
> Makes sense?
>
(cc'ing David Howells since he did the original sb-sharing code for NFS
and may have an opinion here)
That sounds basically like what I'm saying. The tricky part is
fixing up the parent for the original '/a' when you reach it. To do
that, I think you'll also need to fix up cifs_lookup to call
d_materialise_unique instead of d_add.
This situation however is a bit different from NFS. With NFS, the
filehandle (and hence the inode) will be the same no matter how you
reach /a. It's possible though that cifs.ko will be manufacturing
inode numbers out of thin air.
So you may need some other mechanism to determine when you've reached a
"connection" to an existing mount like this, or come up with some way
to ensure that you always get the right inode when you do a lookup on
the same pathname.
--
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2011-10-04 11:23 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-08-26 6:52 [PATCH] CIFS: Fix NT_STATUS_ACCESS_DENIED for mounts with prefixpath option Pavel Shilovsky
[not found] ` <1314341541-2476-1-git-send-email-piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
2011-08-26 11:34 ` Jeff Layton
[not found] ` <20110826073437.743c06a3-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
2011-08-26 12:36 ` Pavel Shilovsky
2011-08-26 20:11 ` Pavel Shilovsky
[not found] ` <CAKywueQQMuTA=xszQYr6sf5L+cZ_s56qrCiL8MqthWS-Xbxw5A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2011-08-26 21:23 ` Jeff Layton
2011-08-30 12:31 ` Pavel Shilovsky
[not found] ` <CAKywueRzwQPROzWTNwhbSOeEanV+Pumn=vFDmYL0uuEApfsX9A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2011-08-30 12:59 ` Jeff Layton
[not found] ` <20110830085935.71e671e4-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
2011-08-31 17:35 ` Pavel Shilovsky
[not found] ` <CAKywueSy2tFQC4mxOBZYjiY=PyLjUZ44v+qvtNNg+zKoxJmeCw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2011-08-31 18:00 ` Jeff Layton
[not found] ` <20110831140044.5ac4a691-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
2011-08-31 18:27 ` Pavel Shilovsky
2011-08-31 18:41 ` Jeff Layton
[not found] ` <20110831144152.788c2543-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
2011-08-31 19:00 ` Pavel Shilovsky
[not found] ` <CAKywueQB=xM1Uv=HBpBwHSqbDcq43aBm4gYD_XbUahP9fyy9vA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2011-08-31 19:22 ` Jeff Layton
[not found] ` <20110831152247.2f2cfcbd-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
2011-10-04 9:01 ` Pavel Shilovsky
[not found] ` <CAKywueQ1shXsy1rSBJx1H7aD80wg-3=GbvKhLXHeH_nJRz3b2g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2011-10-04 11:23 ` Jeff Layton
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.