Hi Swen,

Yes, please do.

Matt

----- "Swen Schillig" <s...@vnet.ibm.com> wrote:

> On Mo, 2016-10-10 at 11:46 -0400, Matt Benjamin wrote:
> > Hi Swen,
> >
> > Are you going to re-push your recent closed PR as 2 new PRs as
> > planned?
> >
> > Dan has made some time this week to review and test, so if you have
> > time, it will be easier to get to merge.
> >
> > Cheers,
> >
> > Matt
> Hi Matt
> 
> It is re-pushed , I just didn't create a new PR yet.
> I was thinking we should test and review before creating a new PR.
> 
> But if you prefer to have a PR right away, I can do that.
> 
> Cheers Swen
> >
> > ----- Original Message -----
> > >
> > > From: "Matt Benjamin" <mbenja...@redhat.com>
> > > To: "Swen Schillig" <s...@vnet.ibm.com>
> > > Cc: "Daniel Gryniewicz" <d...@redhat.com>, "nfs-ganesha-devel"
> <nfs
> > > -ganesha-de...@lists.sourceforge.net>
> > > Sent: Tuesday, September 6, 2016 4:16:39 PM
> > > Subject: Re: [ntirpc] refcount issue ?
> > >
> > > Hi,
> > >
> > > inline
> > >
> > > ----- Original Message -----
> > > >
> > > > From: "Swen Schillig" <s...@vnet.ibm.com>
> > > > To: "Matt Benjamin" <mbenja...@redhat.com>, "Daniel Gryniewicz"
> > > > <d...@redhat.com>
> > > > Cc: "nfs-ganesha-devel"
> <nfs-ganesha-devel@lists.sourceforge.net>
> > > > Sent: Tuesday, September 6, 2016 4:10:32 PM
> > > > Subject: [ntirpc] refcount issue ?
> > > >
> > > > Matt, Dan.
> > > >
> > > > Could you please have a look at the following code areas and
> > > > verify
> > > > what I think is a refcount issue.
> > > >
> > > > clnt_vc_ncreate2()
> > > > {
> > > > ...
> > > >         if ((oflags & RPC_DPLX_LKP_OFLAG_ALLOC) || (!rec->hdl.xd)) {
> > > >                 xd = rec->hdl.xd = alloc_x_vc_data();
> > > >                 ...
> > > >         } else {
> > > >                 xd = rec->hdl.xd;
> > > >                 ++(xd->refcnt);     <=== this is not right. we're not
> > > > taking an addtl'
> > > >                 ref
> > >
> > > Aren't we?  We now share a ref to previously allocated
> rec->hdl.xd.
> > >
> > > >
> > > >                 here.
> > > >         }
> > > > ...
> > > >         clnt->cl_p1 = xd;           <=== but here we should increment
> > > > the
> > > >         refocunt.
> > > > }
> > > >
> > > > another code section with the same handling.
> > > >
> > > > makefd_xprt()
> > > > {
> > > > ...
> > > >         if ((oflags & RPC_DPLX_LKP_OFLAG_ALLOC) || (!rec->hdl.xd)) {
> > > >                 newxd = true;
> > > >                 xd = rec->hdl.xd = alloc_x_vc_data();
> > > >                 ...
> > > >         } else {
> > > >                 xd = (struct x_vc_data *)rec->hdl.xd;
> > > >                 /* dont return destroyed xprts */
> > > >                 if (!(xd->flags & X_VC_DATA_FLAG_SVC_DESTROYED)) {
> > > >                         if (rec->hdl.xprt) {
> > > >                                 xprt = rec->hdl.xprt;
> > > >                                 /* inc xprt refcnt */
> > > >                                 SVC_REF(xprt, SVC_REF_FLAG_NONE);
> > > >                         } else
> > > >                                 ++(xd->refcnt);    <==== not right, no
> > > > addtl' ref to xd taken.
> > >
> > > so this looks more likely to be incorrect, need to review
> > >
> > > >
> > > >                 }
> > > >                 /* return extra ref */
> > > >                 rpc_dplx_unref(rec,
> > > >                                RPC_DPLX_FLAG_LOCKED |
> > > > RPC_DPLX_FLAG_UNLOCK);
> > > >                 *allocated = FALSE;
> > > >
> > > >                 /* return ref'd xprt */
> > > >                 goto done_xprt;
> > > >         }
> > > >         ...
> > > >         xprt->xp_p1 = xd;    <==== but here we should increment the
> > > > refcount
> > > >
> > > >         ...
> > > > }
> > > >
> > > > Both areas handle the refcount'ing wrong, but it might balance
> > > > out
> > > > sometimes.
> > > >
> > > >
> > > > What do you think ?
> > > >
> > > > Cheers Swen
> > > >
> > > >
> > >
> > > --
> > > Matt Benjamin
> > > Red Hat, Inc.
> > > 315 West Huron Street, Suite 140A
> > > Ann Arbor, Michigan 48103
> > >
> > > http://www.redhat.com/en/technologies/storage
> > >
> > > tel.  734-707-0660
> > > fax.  734-769-8938
> > > cel.  734-216-5309
> > >
> >
> 
> 
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> _______________________________________________
> Nfs-ganesha-devel mailing list
> Nfs-ganesha-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel

-- 
Matt Benjamin
CohortFS, LLC.
315 West Huron Street, Suite 140A
Ann Arbor, Michigan 48103

http://cohortfs.com

tel.  734-761-4689 
fax.  734-769-8938 
cel.  734-216-5309 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
Nfs-ganesha-devel mailing list
Nfs-ganesha-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel

Reply via email to