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
