[GIT,PULL,v3] afs, rxrpc: Clean up refcounting on afs_cell and afs_server records

Pull-request

git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git tags/afs-next-20250303

Message

David Howells March 3, 2025, 9:49 a.m. UTC

Hi Christian,

Could you pull this into the VFS tree onto a stable branch, replacing the
earlier pull?  The patches were previously posted here as part of a longer
series:

  https://lore.kernel.org/r/[e-mail]/

This fixes an occasional hang that's only really encountered when rmmod'ing
the kafs module, one of the reasons why I'm proposing it for the next merge
window rather than immediate upstreaming.  The changes include:

 (1) Remove the "-o autocell" mount option.  This is obsolete with the
     dynamic root and removing it makes the next patch slightly easier.

 (2) Change how the dynamic root mount is constructed.  Currently, the root
     directory is (de)populated when it is (un)mounted if there are cells
     already configured and, further, pairs of automount points have to be
     created/removed each time a cell is added/deleted.

     This is changed so that readdir on the root dir lists all the known
     cell automount pairs plus the @cell symlinks and the inodes and
     dentries are constructed by lookup on demand.  This simplifies the
     cell management code.

 (3) A few improvements to the afs_volume tracepoint.

 (4) A few improvements to the afs_server tracepoint.

 (5) Pass trace info into the afs_lookup_cell() function to allow the trace
     log to indicate the purpose of the lookup.

 (6) Remove the 'net' parameter from afs_unuse_cell() as it's superfluous.

 (7) In rxrpc, allow a kernel app (such as kafs) to store a word of
     information on rxrpc_peer records.

 (8) Use the information stored on the rxrpc_peer record to point to the
     afs_server record.  This allows the server address lookup to be done
     away with.

 (9) Simplify the afs_server ref/activity accounting to make each one
     self-contained and not garbage collected from the cell management work
     item.

(10) Simplify the afs_cell ref/activity accounting to make each one of
     these also self-contained and not driven by a central management work
     item.

     The current code was intended to make it such that a single timer for
     the namespace and one work item per cell could do all the work
     required to maintain these records.  This, however, made for some
     sequencing problems when cleaning up these records.  Further, the
     attempt to pass refs along with timers and work items made getting it
     right rather tricky when the timer or work item already had a ref
     attached and now a ref had to be got rid of.

Thanks,
David

Changes
=======
ver #3)
 - Fix the fix for an error check of the form "unsigned value < 0".

ver #2)
 - Fix an error check of the form "unsigned value < 0".

Link: https://lore.kernel.org/r//[e-mail]/ # v1
Link: https://lore.kernel.org/r//[e-mail]/ # v2

---
The following changes since commit 1e15510b71c99c6e49134d756df91069f7d18141:

  Merge tag 'net-6.14-rc5' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net (2025-02-27 09:32:42 -0800)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git tags/afs-next-20250303

for you to fetch changes up to 73f77882c18d849cc8a26e5f06af5e6116169aba:

  afs: Simplify cell record handling (2025-03-03 09:44:36 +0000)

----------------------------------------------------------------
afs: Fix ref leak in rmmod

----------------------------------------------------------------
David Howells (10):
      afs: Remove the "autocell" mount option
      afs: Change dynroot to create contents on demand
      afs: Improve afs_volume tracing to display a debug ID
      afs: Improve server refcount/active count tracing
      afs: Make afs_lookup_cell() take a trace note
      afs: Drop the net parameter from afs_unuse_cell()
      rxrpc: Allow the app to store private data on peer structs
      afs: Use the per-peer app data provided by rxrpc
      afs: Fix afs_server ref accounting
      afs: Simplify cell record handling

 fs/afs/addr_list.c         |  50 ++++
 fs/afs/cell.c              | 437 ++++++++++++++------------------
 fs/afs/cmservice.c         |  82 +------
 fs/afs/dir.c               |   5 +-
 fs/afs/dynroot.c           | 484 +++++++++++++++---------------------
 fs/afs/fs_probe.c          |  32 ++-
 fs/afs/fsclient.c          |   4 +-
 fs/afs/internal.h          |  98 ++++----
 fs/afs/main.c              |  16 +-
 fs/afs/mntpt.c             |   5 +-
 fs/afs/proc.c              |  15 +-
 fs/afs/rxrpc.c             |   8 +-
 fs/afs/server.c            | 601 +++++++++++++++++++--------------------------
 fs/afs/server_list.c       |   6 +-
 fs/afs/super.c             |  25 +-
 fs/afs/vl_alias.c          |   7 +-
 fs/afs/vl_rotate.c         |   2 +-
 fs/afs/volume.c            |  15 +-
 include/net/af_rxrpc.h     |   2 +
 include/trace/events/afs.h |  83 ++++---
 net/rxrpc/ar-internal.h    |   1 +
 net/rxrpc/peer_object.c    |  30 ++-
 22 files changed, 904 insertions(+), 1104 deletions(-)

Comments

Christian Brauner March 4, 2025, 9:45 a.m. UTC | #1

On Mon, 03 Mar 2025 09:49:10 +0000, David Howells wrote:
> Could you pull this into the VFS tree onto a stable branch, replacing the
> earlier pull?  The patches were previously posted here as part of a longer
> series:
> 
>   https://lore.kernel.org/r/[e-mail]/
> 
> This fixes an occasional hang that's only really encountered when rmmod'ing
> the kafs module, one of the reasons why I'm proposing it for the next merge
> window rather than immediate upstreaming.  The changes include:
> 
> [...]

Pulled into the vfs-6.15.shared.afs branch of the vfs/vfs.git tree.
Patches in the vfs-6.15.shared.afs branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series or pull request allowing us to
drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-6.15.shared.afs

https://git.kernel.org/vfs/vfs/c/acf689e88306

David Howells March 6, 2025, 9:12 a.m. UTC | #2

Al spotted another bug (fix below).  Subject to his okaying of the patch, I'll
fold it down and ask for a repull.

David
---
afs: Fix afs_atcell_get_link() to handle RCU pathwalk

The ->get_link() method may be entered under RCU pathwalk conditions (in
which case, the dentry pointer is NULL).  This is not taken account of by
afs_atcell_get_link() and lockdep will complain when it tries to lock an
rwsem.

Fix this by marking net->ws_cell as __rcu and using RCU access macros on it
and by making afs_atcell_get_link() just return a pointer to the name in
RCU pathwalk without taking net->cells_lock or a ref on the cell as RCU
will protect the name storage (the cell is already freed via call_rcu()).

Reported-by: Alexander Viro <[e-mail]>
Signed-off-by: David Howells <[e-mail]>
---
 fs/afs/cell.c     |   11 ++++++-----
 fs/afs/dynroot.c  |   21 +++++++++++++++++----
 fs/afs/internal.h |    2 +-
 fs/afs/proc.c     |    2 +-
 4 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/fs/afs/cell.c b/fs/afs/cell.c
index 4ca713d3862c..0168bbf53fe0 100644
--- a/fs/afs/cell.c
+++ b/fs/afs/cell.c
@@ -57,7 +57,8 @@ static struct afs_cell *afs_find_cell_locked(struct afs_net *net,
 		return ERR_PTR(-ENAMETOOLONG);
 
 	if (!name) {
-		cell = net->ws_cell;
+		cell = rcu_dereference_protected(net->ws_cell,
+						 lockdep_is_held(&net->cells_lock));
 		if (!cell)
 			return ERR_PTR(-EDESTADDRREQ);
 		goto found;
@@ -394,8 +395,8 @@ int afs_cell_init(struct afs_net *net, const char *rootcell)
 
 	/* install the new cell */
 	down_write(&net->cells_lock);
-	old_root = net->ws_cell;
-	net->ws_cell = new_root;
+	old_root = rcu_replace_pointer(net->ws_cell, new_root,
+				       lockdep_is_held(&net->cells_lock));
 	up_write(&net->cells_lock);
 
 	afs_unuse_cell(old_root, afs_cell_trace_unuse_ws);
@@ -869,8 +870,8 @@ void afs_cell_purge(struct afs_net *net)
 	_enter("");
 
 	down_write(&net->cells_lock);
-	ws = net->ws_cell;
-	net->ws_cell = NULL;
+	ws = rcu_replace_pointer(net->ws_cell, NULL,
+				 lockdep_is_held(&net->cells_lock));
 	up_write(&net->cells_lock);
 	afs_unuse_cell(ws, afs_cell_trace_unuse_ws);
 
diff --git a/fs/afs/dynroot.c b/fs/afs/dynroot.c
index 0b66865e3535..9732a1e17db3 100644
--- a/fs/afs/dynroot.c
+++ b/fs/afs/dynroot.c
@@ -210,12 +210,23 @@ static const char *afs_atcell_get_link(struct dentry *dentry, struct inode *inod
 	const char *name;
 	bool dotted = vnode->fid.vnode == 3;
 
-	if (!net->ws_cell)
+	if (!dentry) {
+		/* We're in RCU-pathwalk. */
+		cell = rcu_dereference(net->ws_cell);
+		if (dotted)
+			name = cell->name - 1;
+		else
+			name = cell->name;
+		/* Shouldn't need to set a delayed call. */
+		return name;
+	}
+
+	if (!rcu_access_pointer(net->ws_cell))
 		return ERR_PTR(-ENOENT);
 
 	down_read(&net->cells_lock);
 
-	cell = net->ws_cell;
+	cell = rcu_dereference_protected(net->ws_cell, lockdep_is_held(&net->cells_lock));
 	if (dotted)
 		name = cell->name - 1;
 	else
@@ -324,12 +335,14 @@ static int afs_dynroot_readdir(struct file *file, struct dir_context *ctx)
 		return 0;
 
 	if (ctx->pos == 2) {
-		if (net->ws_cell && !dir_emit(ctx, "@cell", 5, 2, DT_LNK))
+		if (rcu_access_pointer(net->ws_cell) &&
+		    !dir_emit(ctx, "@cell", 5, 2, DT_LNK))
 			return 0;
 		ctx->pos = 3;
 	}
 	if (ctx->pos == 3) {
-		if (net->ws_cell && !dir_emit(ctx, ".@cell", 6, 3, DT_LNK))
+		if (rcu_access_pointer(net->ws_cell) &&
+		    !dir_emit(ctx, ".@cell", 6, 3, DT_LNK))
 			return 0;
 		ctx->pos = 4;
 	}
diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index addce2f03562..440b0e731093 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -288,7 +288,7 @@ struct afs_net {
 	/* Cell database */
 	struct rb_root		cells;
 	struct idr		cells_dyn_ino;	/* cell->dynroot_ino mapping */
-	struct afs_cell		*ws_cell;
+	struct afs_cell __rcu	*ws_cell;
 	atomic_t		cells_outstanding;
 	struct rw_semaphore	cells_lock;
 	struct mutex		cells_alias_lock;
diff --git a/fs/afs/proc.c b/fs/afs/proc.c
index 0af94c846504..9d4df9e4562b 100644
--- a/fs/afs/proc.c
+++ b/fs/afs/proc.c
@@ -207,7 +207,7 @@ static int afs_proc_rootcell_show(struct seq_file *m, void *v)
 
 	net = afs_seq2net_single(m);
 	down_read(&net->cells_lock);
-	cell = net->ws_cell;
+	cell = rcu_dereference_protected(net->ws_cell, lockdep_is_held(&net->cells_lock));
 	if (cell)
 		seq_printf(m, "%s\n", cell->name);
 	up_read(&net->cells_lock);