RE: [PATCH RFC 4/4] 9p: fix race issue in fid contention.

[Jianyong Wu]

Hi Dominique,

> -----Original Message-----
> From: Dominique Martinet <asmadeus@xxxxxxxxxxxxx>
> Sent: Monday, September 14, 2020 4:32 PM
> To: Jianyong Wu <Jianyong.Wu@xxxxxxx>
> Cc: ericvh@xxxxxxxxx; lucho@xxxxxxxxxx; v9fs-
> developer@xxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Justin He
> <Justin.He@xxxxxxx>; Greg Kurz <groug@xxxxxxxx>
> Subject: Re: [PATCH RFC 4/4] 9p: fix race issue in fid contention.
>
> Jianyong Wu wrote on Mon, Sep 14, 2020:
> > > Not having exceptions for that will also make the code around
> > > fid_atomic_dec much simpler: just have clunk do an atomic dec and
> > > only do the actual clunk if that hit zero, and we should be able to
> > > get rid of that helper?
> >
> > Sorry, I think always-one refcount  won't work at this point, as the
> > fid will be clunked only by file context itself not the every consumer
> > of every fid. We can't decrease the refcounter at just one static
> > point.
> > Am I wrong?
>
> I don't understand the "We can't decrease the refcounter at just one static
> point".
> Basically everywhere you added a fid_atomic_dec() will just need to be
> changed to clunk -- the basic rule of refcounting is that anywhere you take a
> ref you need to put it back.
>
Oh, maybe I just miss your point. It is ok to  put the fid_atomic_dec() into p9_client_clunk() and
Let the clunk replace the refcount dec.

> All these places take a fid to do some RPC already so it's not a problem to add
> a clunk and do one more; especially since the "clunk" will just be just a deref.
> For consistency I'd advocate for the kref API as we use that for requests
> already; it might be better to rename the clunk calls to p9_fid_put or
> something but I think that's more churn than it's worth....
>
Ok, I see.
>
> Is there anywhere you think cannot do that?
>
No.
> > This enum value is not functionally necessary, but I think it can
> > reduce the contention of fid, as there are really lots of scenarios
> > that fid from inode is not necessary.
>
> I really don't think it makes things slower if done correctly (e.g. no waiting as
> currently done but the last deref does the actual clunk), and would rather
> keep code simpler unless the difference is big (so would need to do an actual
> benchmark of both if you're convinced it helps) -- sorry.
>
Ok, fair enough.

> >> If possible put the patch first in the series so commits can be
> >> tested independently.
> >
> > Ah, this patch depends on the previous patches, how can I put it as
> > the first of the series?
>
> Basically build the logic in the first patch, then either apply the other three as
> close as they currently are as possible and add the missing refcalls in a new
> patch or incorporate the refcounting in them as well.
> It's fine if you keep it it last, that was just a greedy request on my part to be
> able to test async clunk more easily ; forget about it.

Ok, keep this in original state is easy for me.

Thanks
Jianyong
>
> --
> Dominique
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.