On Tue, 1 Nov 2011 22:56:18 +0300
Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
> 2011/11/1 Pavel Shilovsky <piastry@xxxxxxxxxxx>:
> > Sorry for so long delay on this. Here is the fixed version of the patch:
> >
> > [PATCH] CIFS: Fix NT_STATUS_ACCESS_DENIED for mounts with prefixpath option
> >
> > but I found out one use case where it doesn't work right:
> >
> > 1) mount //server/share/a/b/ test
> > 2) mount //server/share/ test2
> > 3) stat test2/a/b - return wrong inode number (autodisable server inode number happens)
> >
> > In the same time if I add "ls test2" before step #3 it works! May be VFS can't
> > handle negative dentry with children in this case, but I am not sure.
> >
> > 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@xxxxxxxxxxx>
> > ---
> > fs/cifs/cifsfs.c | 138 +++++++++++++++++++++++++++++++--------------------
> > fs/cifs/cifsfs.h | 3 +-
> > fs/cifs/cifsglob.h | 6 ++
> > fs/cifs/dir.c | 3 +-
> > fs/cifs/inode.c | 7 ++-
> > fs/cifs/readdir.c | 43 ++++++++++------
> > 6 files changed, 124 insertions(+), 76 deletions(-)
> >
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index 8f1fe32..a188be1 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -90,14 +90,13 @@ extern mempool_t *cifs_sm_req_poolp;
> > extern mempool_t *cifs_req_poolp;
> > extern mempool_t *cifs_mid_poolp;
> >
> > -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;
> > @@ -115,26 +114,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 CONFIG_CIFS_NFSD_EXPORT
> > if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM) {
> > @@ -143,14 +122,7 @@ cifs_read_super(struct super_block *sb)
> > }
> > #endif /* CONFIG_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)
> > @@ -528,6 +500,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.
> > @@ -535,8 +518,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;
> > @@ -548,20 +533,30 @@ 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;
> > + }
> > +
> > 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;
> >
> > - if (!dir) {
> > - dput(dentry);
> > - dentry = ERR_PTR(-ENOENT);
> > - break;
> > - }
> > -
> > /* skip separators */
> > while (*s == sep)
> > s++;
> > @@ -572,12 +567,55 @@ 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;
> > } while (!IS_ERR(dentry));
> > +
> > + if (IS_ERR(dentry))
> > + goto out;
> > +
> > + mutex_lock(&cifs_root_mutex);
> > +
> > + if (dentry->d_parent->d_inode)
> > + mutex_lock(&dentry->d_parent->d_inode->i_mutex);
> > +
> > + if (!dentry->d_inode) {
> > + inode = cifs_mntroot_iget(sb, full_path);
> > + if (IS_ERR(inode)) {
> > + dput(dentry);
> > + dentry = ERR_CAST(inode);
> > + goto out;
> > + }
> > +
> > + found = d_instantiate_unique(dentry, inode);
> > + if (found) {
> > + dput(dentry);
> > + dentry = found;
> > + }
> > + }
> > +
> > + if (dentry->d_parent->d_inode)
> > + mutex_unlock(&dentry->d_parent->d_inode->i_mutex);
> > +
> > + mutex_unlock(&cifs_root_mutex);
> > +out:
> > kfree(full_path);
> > return dentry;
> > }
> > @@ -644,16 +682,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);
> > @@ -1112,6 +1141,7 @@ init_cifs(void)
> > spin_lock_init(&cifs_tcp_ses_lock);
> > spin_lock_init(&cifs_file_list_lock);
> > spin_lock_init(&GlobalMid_Lock);
> > + mutex_init(&cifs_root_mutex);
> >
> > if (cifs_max_pending < 2) {
> > cifs_max_pending = 2;
> > diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
> > index 30ff560..cf28150 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/cifsglob.h b/fs/cifs/cifsglob.h
> > index 8238aa1..67f6852 100644
> > --- a/fs/cifs/cifsglob.h
> > +++ b/fs/cifs/cifsglob.h
> > @@ -958,6 +958,12 @@ GLOBAL_EXTERN spinlock_t cifs_tcp_ses_lock;
> > */
> > GLOBAL_EXTERN spinlock_t cifs_file_list_lock;
> >
> > +/*
> > + * This lock protects root dentry in cifs_get_root from being instantiated
> > + * twice.
> > + */
> > +GLOBAL_EXTERN struct mutex cifs_root_mutex;
> > +
> > #ifdef CONFIG_CIFS_DNOTIFY_EXPERIMENTAL /* unused temporarily */
> > /* Outstanding dir notify requests */
> > GLOBAL_EXTERN struct list_head GlobalDnotifyReqList;
> > diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> > index d7eeb9d..d6b3319 100644
> > --- a/fs/cifs/dir.c
> > +++ b/fs/cifs/dir.c
> > @@ -553,6 +553,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
> >
> > if (direntry->d_inode != NULL) {
> > cFYI(1, "non-NULL inode in lookup");
> > + newInode = direntry->d_inode;
>
> this and ...
>
> > } else {
> > cFYI(1, "NULL inode in lookup");
> > }
> > @@ -596,7 +597,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
> > rc = cifs_get_inode_info(&newInode, full_path, NULL,
> > parent_dir_inode->i_sb, xid, NULL);
> >
> > - if ((rc == 0) && (newInode != NULL)) {
> > + if ((rc == 0) && (newInode != NULL) && !direntry->d_inode) {
>
> ... this doesn't make sense - I made it for testing purpose only and
> forgot to remove. Since this patch isn't going to be a candidate to
> merge, seems no need to respin it.
>
> > d_add(direntry, newInode);
> > if (posix_open) {
> > filp = lookup_instantiate_filp(nd, direntry,
> > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> > index 2c50bd2..7b7fccd 100644
> > --- a/fs/cifs/inode.c
> > +++ b/fs/cifs/inode.c
> > @@ -878,7 +878,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);
> > @@ -888,9 +888,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 5de03ec..b2f8851 100644
> > --- a/fs/cifs/readdir.c
> > +++ b/fs/cifs/readdir.c
> > @@ -65,10 +65,8 @@ static inline void dump_cifs_file_struct(struct file *file, char *label)
> > }
> > #endif /* DEBUG2 */
> >
> > -/*
> > - * Find the dentry that matches "name". If there isn't one, create one. If it's
> > - * a negative dentry or the uniqueid changed, then drop it and recreate it.
> > - */
> > +
> > +/* Find the dentry that matches "name". If there isn't one, create one. */
> > static struct dentry *
> > cifs_readdir_lookup(struct dentry *parent, struct qstr *name,
> > struct cifs_fattr *fattr)
> > @@ -84,32 +82,38 @@ cifs_readdir_lookup(struct dentry *parent, struct qstr *name,
> > else
> > name->hash = full_name_hash(name->name, name->len);
> >
> > + mutex_lock(&cifs_root_mutex);
> > +
> > dentry = d_lookup(parent, name);
> > if (dentry) {
> > /* FIXME: check for inode number changes? */
> > if (dentry->d_inode != NULL)
> > - return dentry;
> > - d_drop(dentry);
> > - dput(dentry);
> > + goto out;
> > + } else {
> > + dentry = d_alloc(parent, name);
> > + if (dentry == NULL)
> > + goto out;
> > + d_rehash(dentry);
> > }
> >
> > - dentry = d_alloc(parent, name);
> > - if (dentry == NULL)
> > - return NULL;
> > -
> > inode = cifs_iget(sb, fattr);
> > if (!inode) {
> > dput(dentry);
> > - return NULL;
> > + dentry = NULL;
> > + goto out;
> > }
> >
> > - alias = d_materialise_unique(dentry, inode);
> > + alias = d_instantiate_unique(dentry, inode);
> > if (alias != NULL) {
> > dput(dentry);
> > - if (IS_ERR(alias))
> > - return NULL;
> > + if (IS_ERR(alias)) {
> > + dentry = NULL;
> > + goto out;
> > + }
> > dentry = alias;
> > }
> > +out:
> > + mutex_unlock(&cifs_root_mutex);
> >
> > return dentry;
> > }
> > @@ -708,6 +712,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 +737,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
> >
> >
>
>
>
(cc'ing fsdevel since I think we could use more eyes on this scheme)
I'm not a big fan of this patch. It seems to me that this assumes that
it's ok to have a dentry with a negative dentry as a parent. That makes
no sense though. Anegative dentry is the result of a lookup that
returned the equivalent of -ENOENT. I'm very reticent to play that sort
of game with the dcache code...
What may be better is to do something like this:
Go ahead and instantiate positive dentries from the root of the share
to the root of the vfsmount. However, we would mark those inodes as
"placeholders" and give them a bogus inode number of some sort (maybe
even 0?).
If we end up coming across a dentry with a placeholder d_inode later,
the we'd fetch the inode info and, we could remove and reinsert the
inode into the inode hashtable with the proper inode number.
Another idea might be to keep the root dentry of the mount as a
disconnected root dentry, and then try to graft it into a "parent
mount's" tree later if you end up coming across it from another
vfsmount that contains it. That seems a little trickier to handle
though.
--
Jeff Layton <jlayton@xxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
