Re: [Devel] [PATCH RK hci-7.4 2/2] fs/fuse kio: repoint RDMA handlers and restrack names on RK load

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] fs/fuse kio: RK fixup misses RDMA endpoints of clusters being torn down
- [Low] fuse kio: pcs_rdma_rk_fixup_ep() and _flush() are non-static with no shared prototype
- [Low] fuse kio: unguarded kpatch-macros.h include breaks regular module build
- [Low] fs/fuse kio: RK post-patch callback takes pcs_clients_lock before spin_lock_init()
- [Low] fuse kio: pcs_rdma_rk_fixup_ep() rescans the device MR restrack table per endpoint
--

commit 27821832ddfd0c8f4b03f262b44ec34021d6ecc0
Author: Denis V. Lunev <den@openvz.org>

fs/fuse kio: repoint RDMA handlers and restrack names on RK load

This patch adds a kpatch post-patch callback. The callback rewrites the RDMA
event handlers of live pcs connections to the fuse_kio_pcs originals. It
also rewrites the restrack names of their PDs, CQs and MRs to the
fuse_kio_pcs module name. Endpoints are found through pcs_client_list and
the engine hash tables.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-151314

> diff --git a/fs/fuse/kio/pcs/pcs_fuse_kdirect.c b/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
> --- a/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
> +++ b/fs/fuse/kio/pcs/pcs_fuse_kdirect.c

[ ... ]

> @@ -2139,3 +2139,91 @@ module_exit(kpcs_mod_exit);
>  
>  MODULE_LICENSE("GPL v2");
>  MODULE_AUTHOR("Virtuozzo <devel@openvz.org>");
> +
> +#include <linux/rcupdate.h>
> +#include "kpatch-macros.h"

[Severity: Low]
Is this include expected to build outside of kpatch-build? A search of the
tree finds no kpatch-macros.h. fs/fuse/kio/pcs/Makefile has no include path
or define that would provide it. The include is not under an #ifdef, and
rk_fixup_rdma() uses patch_object and KPATCH_POST_PATCH_CALLBACK. A regular
build of fs/fuse/kio/pcs/pcs_fuse_kdirect.c with CONFIG_FUSE_KIO_PCS could
therefore fail with "kpatch-macros.h: No such file or directory".

Should this code be guarded, or moved somewhere that is only built for the
RK?

> +
> +void pcs_rdma_rk_fixup_ep(struct pcs_rpc *ep, const char *name);
> +void pcs_rdma_rk_fixup_flush(void);

[Severity: Low]
This isn't a bug, but pcs_rdma_rk_fixup_ep() and pcs_rdma_rk_fixup_flush()
are defined non-static in pcs_rdma_io.c with no prototype in scope. They
are declared by hand here instead. That triggers -Wmissing-prototypes under
W=1, and the compiler cannot check that the two signatures agree.

Could both prototypes go into pcs_rdma_io.h?

> +
> +/*
> + * [RK] Grab a reference to every RPC endpoint. Returns false if more
> + * endpoints than @cap were found, the caller has to retry then.
> + */
> +static bool rk_collect_eps(struct pcs_rpc **eps, unsigned int cap,
> +			   unsigned int *nr)
> +{
> +	struct pcs_fuse_cluster *pfc;
> +	struct pcs_rpc *ep;
> +	unsigned int n = 0, i;
> +	bool fit = true;
> +
> +	spin_lock(&pcs_clients_lock);
> +	list_for_each_entry(pfc, &pcs_client_list, list) {

[Severity: Medium]
Can this walk miss endpoints that still have live RDMA connections? The
commit message says "All endpoints are reached through pcs_client_list and
the engine hash". Consider the teardown path in kpcs_conn_fini():

    WRITE_ONCE(fc->kio.ctx, NULL);
    unregister_client(pfc);
    synchronize_rcu();
    flush_workqueue(pcs_wq);
    flush_workqueue(pcs_cpu_wq);
    flush_workqueue(pcs_cleanup_wq);
    pcs_cluster_fini(pfc);

unregister_client() removes the pfc from pcs_client_list. The endpoints and
their rios are only closed and destroyed afterwards, in pcs_cluster_fini().
Their rio->id.event_handler can still be an old RK copy and can still
receive CM events. For example, rio_abort() calls rdma_disconnect() during
this teardown.

If rk_fixup_rdma() runs inside that window, rk_collect_eps() never sees the
cluster. Those connections keep their old-RK handlers, and the outgoing RK
can then be unloaded while CM, QP or CQ events are still being delivered.
Is there something that covers clusters that have left pcs_client_list but
whose endpoints have not been destroyed yet?

[ ... ]

> +static unsigned int rk_count_eps(void)
> +{
> +	struct pcs_fuse_cluster *pfc;
> +	unsigned int n = 0;
> +
> +	spin_lock(&pcs_clients_lock);

[Severity: Low]
Is pcs_clients_lock initialized when this callback can run? The lock is
declared as a plain spinlock_t and only initialized by spin_lock_init() in
kpcs_mod_init().

klp_module_coming() can call klp_post_patch_callback() while the module is
still MODULE_STATE_COMING, before the init function runs. This happens if
an RK that patches fuse_kio_pcs is already enabled when fuse_kio_pcs is
loaded.

In that case rk_count_eps() and rk_collect_eps() take an uninitialized lock.
On CONFIG_DEBUG_SPINLOCK or lockdep kernels this would produce a splat. The
callback has nothing to fix up in that scenario anyway.

Would DEFINE_SPINLOCK() for pcs_clients_lock, or skipping the fixup when
obj->mod->state is not MODULE_STATE_LIVE, be appropriate?

[ ... ]

> +			mutex_lock(&eps[i]->mutex);
> +			pcs_rdma_rk_fixup_ep(eps[i], obj->mod->name);

[Severity: Low]
This isn't a bug, but pcs_rdma_rk_fixup_ep() does a full scan of the
device-wide MR restrack table for every endpoint. The scan runs under the
device-wide xa_lock. The total cost is endpoints times device MRs. That
includes MRs owned by other users of the HCA. The same xa_lock is taken by
rdma_restrack_add() and rdma_restrack_del(), so concurrent MR allocations
can stall behind it.

Would it be better to collect the PDs first and do a single pass over the MR
table?

[ ... ]

> diff --git a/fs/fuse/kio/pcs/pcs_rdma_io.c b/fs/fuse/kio/pcs/pcs_rdma_io.c
> --- a/fs/fuse/kio/pcs/pcs_rdma_io.c
> +++ b/fs/fuse/kio/pcs/pcs_rdma_io.c

[ ... ]

> +#include "../../../../drivers/infiniband/core/restrack.h"
> +
> +void pcs_rdma_rk_fixup_ep(struct pcs_rpc *ep, const char *name)
> +{

[ ... ]

> +	xa = &pd->device->res[RDMA_RESTRACK_MR].xa;
> +	xa_lock(xa);
> +	xa_for_each(xa, index, res) {
> +		if (container_of(res, struct ib_mr, res)->pd == pd)
> +			WRITE_ONCE(res->kern_name, name);
> +	}
> +	xa_unlock(xa);
> +}

See the comment above on rescanning the whole MR table per endpoint.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/cover.1792700354.git.den@openvz.org?part=2

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel