Re: [PATCH net v11 15/17] afs: Fix lack of setting call->server when doing FS.InlineBulkStatus

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] This patch adds `call->server = afs_use_server(op->server,
  false, afs_server_trace_use_call)` in afs_make_op_call()…

--

afs: Fix lack of setting call->server when doing FS.InlineBulkStatus

This commit makes afs_make_op_call() take an active count on op->server
and store it in call->server. That stops afs_done_fs_inline_bulk_status()
from oopsing when the server doesn't support FS.InlineBulkStatus. The
function moves further down internal.h so it can use afs_use_server(), and
a new afs_server_trace_use_call trace value is added.

Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824091645.415423-1-dhowells%40redhat.com

> diff --git a/fs/afs/internal.h b/fs/afs/internal.h
> index 330654ed16ece..40dc85d9a5c95 100644
> --- a/fs/afs/internal.h
> +++ b/fs/afs/internal.h

[ ... ]

> @@ -1771,6 +1755,23 @@ static inline struct inode *AFS_VNODE_TO_I(struct afs_vnode *vnode)
>  	return &vnode->netfs.inode;
>  }
>  
> +static inline void afs_make_op_call(struct afs_operation *op, struct afs_call *call,
> +				    gfp_t gfp)
> +{
> +	struct afs_addr_list *alist = op->estate->addresses;
> +
> +	op->call	= afs_get_call(call, afs_call_trace_get);
> +	op->type	= call->type;
> +	call->op	= op;
> +	call->server	= afs_use_server(op->server, false, afs_server_trace_use_call);

[Severity: Medium]
Can this leave a server record that is no longer used with no GC timer
armed?

With this change, every op call holds an active count on op->server.
afs_free_call() drops it with the _notime variant:

fs/afs/rxrpc.c:afs_free_call() {
    ...
	afs_unuse_server_notime(call->net, call->server, afs_server_trace_unuse_call);
    ...
}

When active reaches zero on a server that hasn't expired, in a cell that is
still live, afs_unuse_server_notime() does nothing:

fs/afs/server.c:afs_unuse_server_notime() {
    ...
	if (atomic_dec_and_test(&server->active)) {
		if (test_bit(AFS_SERVER_FL_EXPIRED, &server->flags) ||
		    READ_ONCE(server->cell->state) >= AFS_CELL_REMOVING)
			schedule_work(&server->destroyer);
	}
    ...
}

Only afs_unuse_server() sets unuse_time and calls afs_set_server_timer()
when active reaches zero. Before this patch, op calls didn't pin the
server, so the last active count was always dropped by the server list's
afs_unuse_server().

In the async read path, the call can outlive the op's server list:

afs_fetch_data_async_rx()
  afs_read_receive()
    afs_put_call(call)          <- drops the op's ref
    afs_select_fileserver(op)   <- may afs_put_serverlist() the old list
    or
    afs_end_read()->afs_put_operation()->afs_put_serverlist()
  afs_put_call(call)            <- work item's ref, reaches afs_free_call()

Suppose the volume's server list is replaced while the read is in flight,
for example after a VLDB update or a VMOVED abort, and server X is in no
other list. The old list's afs_unuse_server() takes active from 2 to 1,
which doesn't arm the timer. The call's afs_unuse_server_notime() then
takes it from 1 to 0, which does nothing.

Would server X then stay in cell->fs_servers and on the probe lists? It
would keep being probed and its callbacks would not be given up, until
afs_purge_servers() runs at cell removal or the server is looked up again.
The periodic afs_fs_get_capabilities() probes also unuse with the _notime
variant, so they wouldn't re-arm GC either.

Probe and CM calls already have the same _notime behaviour. This change
extends it to every FS operation call, including async calls that often
outlive their op.

Could op calls be released with afs_unuse_server() instead? Another option
is for afs_unuse_server_notime() to set unuse_time and arm the timer when
active reaches zero on a live server.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923133706.1496540-1-dhowells%40redhat.com