From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shirish Pargaonkar Subject: Re: [PATCH 1/2] cifs: store pointer to master tlink in superblock Date: Thu, 28 Oct 2010 11:54:57 -0500 Message-ID: References: <1288265619-20616-1-git-send-email-jlayton@redhat.com> <1288265619-20616-2-git-send-email-jlayton@redhat.com> <20101028122325.31997c58@tlielax.poochiereds.net> <20101028125007.734e0b27@tlielax.poochiereds.net> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Jeff Layton Return-path: In-Reply-To: <20101028125007.734e0b27-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org> Sender: linux-cifs-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-ID: On Thu, Oct 28, 2010 at 11:50 AM, Jeff Layton wrot= e: > On Thu, 28 Oct 2010 12:23:25 -0400 > Jeff Layton wrote: > >> On Thu, 28 Oct 2010 10:06:05 -0500 >> Shirish Pargaonkar wrote: >> >> > On Thu, Oct 28, 2010 at 6:33 AM, Jeff Layton = wrote: >> > > Instead of keeping a tag on the master tlink in the tree, just k= eep a >> > > pointer to the master in the superblock. That eliminates the nee= d for >> > > using the radix tree to look up a tagged entry. >> > > >> > > Signed-off-by: Jeff Layton >> > > --- >> > > =A0fs/cifs/cifs_fs_sb.h | =A0 =A02 +- >> > > =A0fs/cifs/connect.c =A0 =A0| =A0 18 +++--------------- >> > > =A02 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 @@ >> > > >> > > =A0struct cifs_sb_info { >> > > =A0 =A0 =A0 =A0struct radix_tree_root tlink_tree; >> > > -#define CIFS_TLINK_MASTER_TAG =A0 =A0 =A0 =A0 =A00 =A0 =A0 =A0 = /* is "master" (mount) tcon */ >> > > =A0 =A0 =A0 =A0spinlock_t tlink_tree_lock; >> > > + =A0 =A0 =A0 struct tcon_link *master_tlink; >> > > =A0 =A0 =A0 =A0struct nls_table *local_nls; >> > > =A0 =A0 =A0 =A0unsigned int rsize; >> > > =A0 =A0 =A0 =A0unsigned 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: >> > > >> > > =A0 =A0 =A0 =A0spin_lock(&cifs_sb->tlink_tree_lock); >> > > =A0 =A0 =A0 =A0radix_tree_insert(&cifs_sb->tlink_tree, pSesInfo-= >linux_uid, tlink); >> > > - =A0 =A0 =A0 radix_tree_tag_set(&cifs_sb->tlink_tree, pSesInfo-= >linux_uid, >> > > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0CIFS_TLINK_= MASTER_TAG); >> > > =A0 =A0 =A0 =A0spin_unlock(&cifs_sb->tlink_tree_lock); >> > > =A0 =A0 =A0 =A0radix_tree_preload_end(); >> > > >> > > + =A0 =A0 =A0 cifs_sb->master_tlink =3D tlink; >> > > + >> > > =A0 =A0 =A0 =A0queue_delayed_work(system_nrt_wq, &cifs_sb->prune= _tlinks, >> > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0T= LINK_IDLE_EXPIRE); >> > > >> > > @@ -3275,19 +3275,7 @@ out: >> > > =A0static struct tcon_link * >> > > =A0cifs_sb_master_tlink(struct cifs_sb_info *cifs_sb) >> > > =A0{ >> > > - =A0 =A0 =A0 struct tcon_link *tlink; >> > > - =A0 =A0 =A0 unsigned int ret; >> > > - >> > > - =A0 =A0 =A0 spin_lock(&cifs_sb->tlink_tree_lock); >> > > - =A0 =A0 =A0 ret =3D radix_tree_gang_lookup_tag(&cifs_sb->tlink= _tree, (void **)&tlink, >> > > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 =A0 =A0 0, 1, CIFS_TLINK_MASTER_TAG); >> > > - =A0 =A0 =A0 spin_unlock(&cifs_sb->tlink_tree_lock); >> > > - >> > > - =A0 =A0 =A0 /* the master tcon should always be present */ >> > > - =A0 =A0 =A0 if (ret =3D=3D 0) >> > > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 BUG(); >> > > - >> > > - =A0 =A0 =A0 return tlink; >> > > + =A0 =A0 =A0 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 insulate= s >> the callers from needing to know anything about the underlying data >> structure. >> > > What may make some sense however is turning this function into a stat= ic > inline or macro. Thoughts? > > -- > Jeff Layton > I think inline function would be my preferred choice.