Re: [PATCH 1/2] cifs: store pointer to master tlink in superblock

[Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

On Thu, 28 Oct 2010 12:23:25 -0400
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> On Thu, 28 Oct 2010 10:06:05 -0500
> Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > On Thu, Oct 28, 2010 at 6:33 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > > Instead of keeping a tag on the master tlink in the tree, just keep a
> > > pointer to the master in the superblock. That eliminates the need for
> > > using the radix tree to look up a tagged entry.
> > >
> > > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > > ---
> > >  fs/cifs/cifs_fs_sb.h |    2 +-
> > >  fs/cifs/connect.c    |   18 +++---------------
> > >  2 files changed, 4 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/fs/cifs/cifs_fs_sb.h b/fs/cifs/cifs_fs_sb.h
> > > index 525ba59..79576da 100644
> > > --- a/fs/cifs/cifs_fs_sb.h
> > > +++ b/fs/cifs/cifs_fs_sb.h
> > > @@ -43,8 +43,8 @@
> > >
> > >  struct cifs_sb_info {
> > >        struct radix_tree_root tlink_tree;
> > > -#define CIFS_TLINK_MASTER_TAG          0       /* is "master" (mount) tcon */
> > >        spinlock_t tlink_tree_lock;
> > > +       struct tcon_link *master_tlink;
> > >        struct nls_table *local_nls;
> > >        unsigned int rsize;
> > >        unsigned int wsize;
> > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > > index 469c3dd..364eb96 100644
> > > --- a/fs/cifs/connect.c
> > > +++ b/fs/cifs/connect.c
> > > @@ -2917,11 +2917,11 @@ remote_path_check:
> > >
> > >        spin_lock(&cifs_sb->tlink_tree_lock);
> > >        radix_tree_insert(&cifs_sb->tlink_tree, pSesInfo->linux_uid, tlink);
> > > -       radix_tree_tag_set(&cifs_sb->tlink_tree, pSesInfo->linux_uid,
> > > -                          CIFS_TLINK_MASTER_TAG);
> > >        spin_unlock(&cifs_sb->tlink_tree_lock);
> > >        radix_tree_preload_end();
> > >
> > > +       cifs_sb->master_tlink = tlink;
> > > +
> > >        queue_delayed_work(system_nrt_wq, &cifs_sb->prune_tlinks,
> > >                                TLINK_IDLE_EXPIRE);
> > >
> > > @@ -3275,19 +3275,7 @@ out:
> > >  static struct tcon_link *
> > >  cifs_sb_master_tlink(struct cifs_sb_info *cifs_sb)
> > >  {
> > > -       struct tcon_link *tlink;
> > > -       unsigned int ret;
> > > -
> > > -       spin_lock(&cifs_sb->tlink_tree_lock);
> > > -       ret = radix_tree_gang_lookup_tag(&cifs_sb->tlink_tree, (void **)&tlink,
> > > -                                       0, 1, CIFS_TLINK_MASTER_TAG);
> > > -       spin_unlock(&cifs_sb->tlink_tree_lock);
> > > -
> > > -       /* the master tcon should always be present */
> > > -       if (ret == 0)
> > > -               BUG();
> > > -
> > > -       return tlink;
> > > +       return cifs_sb->master_tlink;
> > 
> > Wondering whether we need a function just to return master_tlink
> > within a cifs_sb.
> > Could the caller(s) of cifs_sb_master_tlink() access master_tlink directly?
> > 
> 
> Sure, but accessor functions are generally a good thing. It insulates
> the callers from needing to know anything about the underlying data
> structure.
> 

What may make some sense however is turning this function into a static
inline or macro. Thoughts?

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>