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

[Jianyong Wu]

Hi Dominique,

> -----Original Message-----
> From: Dominique Martinet <asmadeus@xxxxxxxxxxxxx>
> Sent: Wednesday, September 23, 2020 10:50 PM
> To: Jianyong Wu <Jianyong.Wu@xxxxxxx>
> Cc: ericvh@xxxxxxxxx; lucho@xxxxxxxxxx; qemu_oss@xxxxxxxxxxxxx;
> groug@xxxxxxxx; v9fs-developer@xxxxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Justin He <Justin.He@xxxxxxx>
> Subject: Re: [PATCH RFC v2 4/4] 9p: fix race issue in fid contention.
>
>
> Overall looks good; a few comments.

Thanks
>
> Jianyong Wu wrote on Wed, Sep 23, 2020:
> > open-unlink-f*syscall test:
> > I have tested for f*syscall include: ftruncate fstat fchown fchmod faccessat.
>
> Given the other thread, what did you test this with?
Er, I just use Greg's qemu of https://github.com/gkurz/qemu.git: 9p-attr-fixes. I should have referenced it in commit message.

> Since qemu doesn't work apparently do you have a in-house server at arm I
> could test?
> (I'll try with ganesha otherwise, it keeps files open so it should work I think...)
>
Yeah, I test this on my arm server. But as these codes are arch-free, we can test it in whatever a machine.
(also the server in arm can't be accessed by outer space, so sorry)
But I think that this test are far from serious and complete.

> > +atomic_set(&fid->count, 1);
>
> I kind of like the refcount API becauese it has some extra overflow checks;
> but it requires a bit more work around clunk (instead of bailing out early if
> counter hits 0, you need to have it call a separate function in case it does)
>
> That's mostly esthetics though I'm not going to fuss over that.
>
Sorry, I'm not sure what's the context of this line, does this line lie in "__add_fid”. I'm not sure about
why it do harm to clunk?

> > @@ -74,6 +77,7 @@ static struct p9_fid *v9fs_fid_find_inode(struct
> > inode *inode, kuid_t uid)  void v9fs_open_fid_add(struct inode *inode,
> > struct p9_fid *fid)  {
> >  spin_lock(&inode->i_lock);
> > +atomic_set(&fid->count, 1);
>
> Hm, that should be done at fid creation time in net/9p/client.c p9_fid_create ;
> no ?
> (you do it there already, I don't see what reseting count here brings except
> confusion)
>
I put this counter set op before the fids are added to hlist. So I can make sure the counter value is 1 before
fids are used. It's redundant code. I can remove it in both "__add_fid" and "v9fs_open_fid_add", but we must take care of it that
no clunk is called between fids are created and added to hlist. Both are good for me.

> > diff --git a/fs/9p/fid.h b/fs/9p/fid.h index
> > dfa11df02818..1fed96546728 100644
> > --- a/fs/9p/fid.h
> > +++ b/fs/9p/fid.h
> > @@ -22,6 +22,14 @@ static inline struct p9_fid *clone_fid(struct
> > p9_fid *fid)  }  static inline struct p9_fid *v9fs_fid_clone(struct
> > dentry *dentry)  {
> > -return clone_fid(v9fs_fid_lookup(dentry));
> > +struct p9_fid *fid, *nfid;
> > +
> > +fid = v9fs_fid_lookup(dentry);
> > +if (!fid || IS_ERR(fid))
> > +return fid;
> > +
> > +nfid = p9_client_walk(fid, 0, NULL, 1);
>
> I think you clone_fid() here is slightly easier to understand; everyone doesn't
> know that a walk with no component is a clone.
> The compiler will optimize that IS_ERR(fid) is checked twice, it's fine.
>
Er, I rewrite it because I must acquire the intermedia fid from v9fs_fid_lookup and clunk it.

> > diff --git a/include/net/9p/client.h b/include/net/9p/client.h index
> > ce7882da8e86..58ed9bd306bd 100644
> > --- a/include/net/9p/client.h
> > +++ b/include/net/9p/client.h
> > @@ -140,10 +140,16 @@ struct p9_client {
> >   *
> >   * TODO: This needs lots of explanation.
> >   */
> > +enum fid_source {
> > +FID_FROM_OTHER,
> > +FID_FROM_INODE,
> > +FID_FROM_DENTRY,
> > +};
>
> leftovers from previous iteration.
>
Sorry, need remove it.

>
> Overall looks good to me.
> I'd need to spend some time checking the actual counting part & hammering
> the fs a bit then confirming no fid got forgotten (there's a pr_info at umount
> time) but I'm happy with this ; thanks!
>
Yeah, it's a tedious job to do that. Also we need to find a reliable test suit. Then we can check
this patch both from code and test.

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.