fs/9p/fid.c | 71 +++++++++++++++------------- fs/9p/fid.h | 6 +-- fs/9p/vfs_addr.c | 4 +- fs/9p/vfs_dentry.c | 4 +- fs/9p/vfs_dir.c | 2 +- fs/9p/vfs_file.c | 9 ++-- fs/9p/vfs_inode.c | 97 +++++++++++++++++---------------------- fs/9p/vfs_inode_dotl.c | 79 ++++++++++++------------------- fs/9p/vfs_super.c | 8 ++-- fs/9p/xattr.c | 8 ++-- include/net/9p/client.h | 3 ++ include/trace/events/9p.h | 48 +++++++++++++++++++ net/9p/client.c | 42 +++++++++++------ 13 files changed, 211 insertions(+), 170 deletions(-)
So:
- I could reproduce Tyler's generic 531 leak, fixed it by the first
commit in v9fs_vfs_atomic_open_dotl
- Found another less likely leak while reworking code
- Christian's comment that it's not obvious that clunk is just a
refcount decrease was very true: I think it's worth the churn,
so I've rename this all through a new helper...
- ... with the not-so-hidden intent to improve debugging by adding
a tracepoint for them, which I have also done.
I've also taken my comment in the other thread further and went ahead
and made it in its own commit. Tyler, if you're ok with this I'll just
squash it up. You can see rebased patches here:
https://github.com/martinetd/linux/
Note that I also took the permission to add back '/* clone */' as a
comment to your changing p9_client_walk's arguments: while I can agree
variables are hard to track, figuring out what the heck that boolean
argument means is much harder to me and I honestly preferred the
variable.
Having both through a comment is good enough for me if you'll accept
this:
----
@@ -222,7 +221,8 @@ static struct p9_fid *v9fs_fid_lookup_with_uid(struct dentry *dentry,
* We need to hold rename lock when doing a multipath
* walk to ensure none of the patch component change
*/
- fid = p9_client_walk(old_fid, l, &wnames[i], clone);
+ fid = p9_client_walk(old_fid, l, &wnames[i],
+ old_fid == root_fid /* clone */);
/* non-cloning walk will return the same fid */
if (fid != old_fid) {
p9_client_clunk(old_fid);
----
The last commit is just cleanups and should be no real change.
Dominique Martinet (6):
9p: fix fid refcount leak in v9fs_vfs_atomic_open_dotl
9p: fix fid refcount leak in v9fs_vfs_get_link
9p: v9fs_fid_lookup_with_uid fix's fix suggestion
9p fid refcount: add p9_fid_get/put wrappers
9p fid refcount: add a 9p_fid_ref tracepoint
9p fid refcount: cleanup p9_fid_put calls
(diff stats include Tyler's commits)
fs/9p/fid.c | 71 +++++++++++++++-------------
fs/9p/fid.h | 6 +--
fs/9p/vfs_addr.c | 4 +-
fs/9p/vfs_dentry.c | 4 +-
fs/9p/vfs_dir.c | 2 +-
fs/9p/vfs_file.c | 9 ++--
fs/9p/vfs_inode.c | 97 +++++++++++++++++----------------------
fs/9p/vfs_inode_dotl.c | 79 ++++++++++++-------------------
fs/9p/vfs_super.c | 8 ++--
fs/9p/xattr.c | 8 ++--
include/net/9p/client.h | 3 ++
include/trace/events/9p.h | 48 +++++++++++++++++++
net/9p/client.c | 42 +++++++++++------
13 files changed, 211 insertions(+), 170 deletions(-)
--
2.35.1
On 2022-06-12 17:53:23, Dominique Martinet wrote:
> So:
> - I could reproduce Tyler's generic 531 leak, fixed it by the first
> commit in v9fs_vfs_atomic_open_dotl
> - Found another less likely leak while reworking code
> - Christian's comment that it's not obvious that clunk is just a
> refcount decrease was very true: I think it's worth the churn,
> so I've rename this all through a new helper...
> - ... with the not-so-hidden intent to improve debugging by adding
> a tracepoint for them, which I have also done.
>
> I've also taken my comment in the other thread further and went ahead
> and made it in its own commit. Tyler, if you're ok with this I'll just
> squash it up. You can see rebased patches here:
> https://github.com/martinetd/linux/
>
> Note that I also took the permission to add back '/* clone */' as a
> comment to your changing p9_client_walk's arguments: while I can agree
> variables are hard to track, figuring out what the heck that boolean
> argument means is much harder to me and I honestly preferred the
> variable.
> Having both through a comment is good enough for me if you'll accept
> this:
> ----
> @@ -222,7 +221,8 @@ static struct p9_fid *v9fs_fid_lookup_with_uid(struct dentry *dentry,
> * We need to hold rename lock when doing a multipath
> * walk to ensure none of the patch component change
> */
> - fid = p9_client_walk(old_fid, l, &wnames[i], clone);
> + fid = p9_client_walk(old_fid, l, &wnames[i],
> + old_fid == root_fid /* clone */);
This is no problem at all. The rebased patches look good to me. Squash
your fix to my fix and it should be ready to go.
Thanks again!
Tyler
> /* non-cloning walk will return the same fid */
> if (fid != old_fid) {
> p9_client_clunk(old_fid);
> ----
>
>
> The last commit is just cleanups and should be no real change.
>
> Dominique Martinet (6):
> 9p: fix fid refcount leak in v9fs_vfs_atomic_open_dotl
> 9p: fix fid refcount leak in v9fs_vfs_get_link
> 9p: v9fs_fid_lookup_with_uid fix's fix suggestion
> 9p fid refcount: add p9_fid_get/put wrappers
> 9p fid refcount: add a 9p_fid_ref tracepoint
> 9p fid refcount: cleanup p9_fid_put calls
>
>
> (diff stats include Tyler's commits)
>
> fs/9p/fid.c | 71 +++++++++++++++-------------
> fs/9p/fid.h | 6 +--
> fs/9p/vfs_addr.c | 4 +-
> fs/9p/vfs_dentry.c | 4 +-
> fs/9p/vfs_dir.c | 2 +-
> fs/9p/vfs_file.c | 9 ++--
> fs/9p/vfs_inode.c | 97 +++++++++++++++++----------------------
> fs/9p/vfs_inode_dotl.c | 79 ++++++++++++-------------------
> fs/9p/vfs_super.c | 8 ++--
> fs/9p/xattr.c | 8 ++--
> include/net/9p/client.h | 3 ++
> include/trace/events/9p.h | 48 +++++++++++++++++++
> net/9p/client.c | 42 +++++++++++------
> 13 files changed, 211 insertions(+), 170 deletions(-)
>
> --
> 2.35.1
>
Tyler Hicks wrote on Mon, Jun 13, 2022 at 03:20:53PM -0500: > On 2022-06-12 17:53:23, Dominique Martinet wrote: > > @@ -222,7 +221,8 @@ static struct p9_fid *v9fs_fid_lookup_with_uid(struct dentry *dentry, > > * We need to hold rename lock when doing a multipath > > * walk to ensure none of the patch component change > > */ > > - fid = p9_client_walk(old_fid, l, &wnames[i], clone); > > + fid = p9_client_walk(old_fid, l, &wnames[i], > > + old_fid == root_fid /* clone */); > > This is no problem at all. The rebased patches look good to me. Squash > your fix to my fix and it should be ready to go. Thanks for all your reviews :) I've rebased my branch if you want to check: https://github.com/martinetd/linux/commits/9p-next We've just started a cycle so I'll submit the first three patches (fixes to stable) next week, and the rest for when 5.20 cycle starts. Feel free to remind me if it looks like I forgot. -- Dominique
On 2022-06-14 06:00:21, Dominique Martinet wrote: > Tyler Hicks wrote on Mon, Jun 13, 2022 at 03:20:53PM -0500: > > On 2022-06-12 17:53:23, Dominique Martinet wrote: > > > @@ -222,7 +221,8 @@ static struct p9_fid *v9fs_fid_lookup_with_uid(struct dentry *dentry, > > > * We need to hold rename lock when doing a multipath > > > * walk to ensure none of the patch component change > > > */ > > > - fid = p9_client_walk(old_fid, l, &wnames[i], clone); > > > + fid = p9_client_walk(old_fid, l, &wnames[i], > > > + old_fid == root_fid /* clone */); > > > > This is no problem at all. The rebased patches look good to me. Squash > > your fix to my fix and it should be ready to go. > > Thanks for all your reviews :) > I've rebased my branch if you want to check: > https://github.com/martinetd/linux/commits/9p-next I've diffed the individual patches from before and after the rebase. It all looks great to me. > We've just started a cycle so I'll submit the first three patches (fixes > to stable) next week, and the rest for when 5.20 cycle starts. That sounds like the right plan to me. > Feel free to remind me if it looks like I forgot. Will do! Tyler > -- > Dominique >
© 2016 - 2026 Red Hat, Inc.