On Wed, 31 Aug 2011 21:35:39 +0400
Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
> 2011/8/30 Jeff Layton <jlayton@xxxxxxxxxx>:
> > On Tue, 30 Aug 2011 16:31:52 +0400
> > Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
> >
> >> 2011/8/26 Pavel Shilovsky <piastry@xxxxxxxxxxx>:
> >> > 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 | 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@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
