lkml.org 
[lkml]   [2020]   [Sep]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH RFC v2 4/4] 9p: fix race issue in fid contention.

    Overall looks good; a few comments.

    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?
    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...)

    > + 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.

    > @@ -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)

    > 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.

    > 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.


    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!

    --
    Dominique




    \
     
     \ /
      Last update: 2020-09-23 16:50    [W:3.397 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site