| Message ID | 1419405208-25975-21-git-send-email-loghyr@primarydata.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi again, On 12/24/2014 02:12 AM, Tom Haynes wrote: > From: Peng Tao <tao.peng@primarydata.com> > > Signed-off-by: Peng Tao <tao.peng@primarydata.com> > Signed-off-by: Tom Haynes <Thomas.Haynes@primarydata.com> > --- > fs/nfs/pnfs.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c > index 2d25670..fa00b56 100644 > --- a/fs/nfs/pnfs.c > +++ b/fs/nfs/pnfs.c > @@ -1288,7 +1288,6 @@ pnfs_update_layout(struct inode *ino, > struct nfs_client *clp = server->nfs_client; > struct pnfs_layout_hdr *lo; > struct pnfs_layout_segment *lseg = NULL; > - bool first; > > if (!pnfs_enabled_sb(NFS_SERVER(ino))) > goto out; > @@ -1321,16 +1320,15 @@ pnfs_update_layout(struct inode *ino, > if (pnfs_layoutgets_blocked(lo, 0)) > goto out_unlock; > atomic_inc(&lo->plh_outstanding); > - > - first = list_empty(&lo->plh_layouts) ? true : false; > spin_unlock(&ino->i_lock); > > - if (first) { > + if (list_empty(&lo->plh_layouts)) { > /* The lo must be on the clp list if there is any > * chance of a CB_LAYOUTRECALL(FILE) coming in. > */ > spin_lock(&clp->cl_lock); > - list_add_tail(&lo->plh_layouts, &server->layouts); > + if (list_empty(&lo->plh_layouts)) > + list_add_tail(&lo->plh_layouts, &server->layouts); > spin_unlock(&clp->cl_lock); > } Do we really need to call list_empty() twice? Would there be a serious performance drawback if we removed the outer layer if condition and then always call list_empty() under the cl_lock? Thanks, Anna > > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/05/2015 04:20 PM, Trond Myklebust wrote: > > On Jan 5, 2015 12:59 PM, "Anna Schumaker" <Anna.Schumaker@netapp.com <mailto:Anna.Schumaker@netapp.com>> wrote: >> >> Hi again, >> >> On 12/24/2014 02:12 AM, Tom Haynes wrote: >> > From: Peng Tao <tao.peng@primarydata.com <mailto:tao.peng@primarydata.com>> >> > >> > Signed-off-by: Peng Tao <tao.peng@primarydata.com <mailto:tao.peng@primarydata.com>> >> > Signed-off-by: Tom Haynes <Thomas.Haynes@primarydata.com <mailto:Thomas.Haynes@primarydata.com>> >> > --- >> > fs/nfs/pnfs.c | 8 +++----- >> > 1 file changed, 3 insertions(+), 5 deletions(-) >> > >> > diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c >> > index 2d25670..fa00b56 100644 >> > --- a/fs/nfs/pnfs.c >> > +++ b/fs/nfs/pnfs.c >> > @@ -1288,7 +1288,6 @@ pnfs_update_layout(struct inode *ino, >> > struct nfs_client *clp = server->nfs_client; >> > struct pnfs_layout_hdr *lo; >> > struct pnfs_layout_segment *lseg = NULL; >> > - bool first; >> > >> > if (!pnfs_enabled_sb(NFS_SERVER(ino))) >> > goto out; >> > @@ -1321,16 +1320,15 @@ pnfs_update_layout(struct inode *ino, >> > if (pnfs_layoutgets_blocked(lo, 0)) >> > goto out_unlock; >> > atomic_inc(&lo->plh_outstanding); >> > - >> > - first = list_empty(&lo->plh_layouts) ? true : false; >> > spin_unlock(&ino->i_lock); >> > >> > - if (first) { >> > + if (list_empty(&lo->plh_layouts)) { >> > /* The lo must be on the clp list if there is any >> > * chance of a CB_LAYOUTRECALL(FILE) coming in. >> > */ >> > spin_lock(&clp->cl_lock); >> > - list_add_tail(&lo->plh_layouts, &server->layouts); >> > + if (list_empty(&lo->plh_layouts)) >> > + list_add_tail(&lo->plh_layouts, &server->layouts); >> > spin_unlock(&clp->cl_lock); >> > } >> >> Do we really need to call list_empty() twice? Would there be a serious performance drawback if we removed the outer layer if condition and then always call list_empty() under the cl_lock? > > What is the problem with that? It avoids unnecessary contention on a per-server global lock. I was thinking about the case where the plh_layouts list becomes empty after the outer if. I took a closer look at the code and that only happens when the layout is being freed, so it shouldn't be an issue. Anna > > Please keep it, > >> >> > >> > >> > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c index 2d25670..fa00b56 100644 --- a/fs/nfs/pnfs.c +++ b/fs/nfs/pnfs.c @@ -1288,7 +1288,6 @@ pnfs_update_layout(struct inode *ino, struct nfs_client *clp = server->nfs_client; struct pnfs_layout_hdr *lo; struct pnfs_layout_segment *lseg = NULL; - bool first; if (!pnfs_enabled_sb(NFS_SERVER(ino))) goto out; @@ -1321,16 +1320,15 @@ pnfs_update_layout(struct inode *ino, if (pnfs_layoutgets_blocked(lo, 0)) goto out_unlock; atomic_inc(&lo->plh_outstanding); - - first = list_empty(&lo->plh_layouts) ? true : false; spin_unlock(&ino->i_lock); - if (first) { + if (list_empty(&lo->plh_layouts)) { /* The lo must be on the clp list if there is any * chance of a CB_LAYOUTRECALL(FILE) coming in. */ spin_lock(&clp->cl_lock); - list_add_tail(&lo->plh_layouts, &server->layouts); + if (list_empty(&lo->plh_layouts)) + list_add_tail(&lo->plh_layouts, &server->layouts); spin_unlock(&clp->cl_lock); }