Re: cifs and win2k8

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



2011/11/10 Jeff Layton <jlayton@xxxxxxxxx>:
> 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...
>

Jeff, now I agree with you that keeping negative dentries with
childrens is not a good idea. That really breaks some VFS assumptions.

> 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?).
>

Yes, I was thinking about this approach too but didn't have enough
time to prototype it. It seems easier to implement than the second one
but now I don't imagine what problems we can faced with going this
way.

-- 
Best regards,
Pavel Shilovsky.
--
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


[Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux