Gerrit Review Dashboard

I few issues.

1) Don't add new fault injection to lctl. Instead add it to lnetctl

2) No new ioctls. You need to update lnet_fault_cmd() and friends in api.c instead.
   The Netlink interface is IPv6 freindly.

Can you tell me what the YAML net fault looks like? I can help you with this.

I added lnetctl commands together with Netlink support and also removed ipv4 specific logic to make it ipv6 friendly. I have not removed the ioctl and lctl commands yet as it can be used as fallback.

(style) This isn't a bug, but the fonts in the new synopsis are inconsistent: `-r` and `-i` are bold while `-s`, `-d`, `-z`, `-f`, `-n`, `-m`, `-p` and `-o` are plain, and the placeholders are lowercase italic. man-pages(7) style (and the rest of Documentation/man8) is bold for the option including its dashes and italic uppercase for the value, e.g. `\fB\-s\fR \fISRC\fR`.

(minor) This reads as if a portal selector simply has no effect on REPLY, but it actually removes REPLY from the rule. lnet_fault_attr_validate() narrows fa_msg_mask to GET|PUT as soon as fa_ptl_mask is non-zero, and lnet_corrupt_rule_add() then masks with PUT|REPLY, so PUT is all that is left.

So `-p 52` with no `-m` gives a PUT-only rule even though the previous sentence says both are selected by default, and `-m REPLY -p 52` is rejected outright - lnetctl just prints "failed to add corrupt rule: Invalid argument" with no hint about which option caused it.

Could the text say that specifying a portal restricts the rule to PUT?

(minor) Lustre man pages are expected to carry an EXAMPLES entry for each major way of using a sub-command, and lnetctl.8 already has that pattern at the end of the page for udsp. Could a short example be added here for the two scheduling modes, e.g. a rate rule

    lnetctl fault corrupt add -s 10.0.0.1@tcp -d 10.0.0.2@tcp -r 100 -z 8 -f 4096 -m PUT -p 52

and an interval rule? The offset/offset_range/size interaction is hard to infer from the prose alone.

No ioctl!!!! This is not IPv6 friendly. You need to update lnet_fault_cmd() in api.c instead.

(minor) corrupt-fault.sh is added to noinst_SCRIPTS but not to lustre/tests/test-groups/regression, so once `Test-Parameters: testlist=corrupt-fault` has run for this change the new suite will not be picked up by any of the standard sessions again.

Since the subtests are LNet-selftest based, would it be simpler to add them to sanity-lnet.sh, which is already in the regression group, or to add corrupt-fault to that list?

(typo) The test added by this patch is `test_113`, not `test_630`, so the message no longer greps against the code.

Also "interace" on the next line.

(style) This isn't a bug, but the `case` is at the same indent as the enclosing `for` body while `esac` is one tab deeper. Both should line up one tab inside the loop.

(defect) These rules name `tcp$i` while the nets above are created as `${NETTYPE}$i`. The guard at the top of the test only requires `tcp*`, so with `NETTYPE=tcp1` the nets are `tcp11`..`tcp1140` and none of the UDSP rules match anything.

I guess based on your other comments that this has *not* been implemented yet?  If that is the case then I don't think this patch can land until the interop code is finished.  Otherwise, this patch will break interop testing for master and there is a risk that the interop patch is not completed before the 2.18.0 release, which would be a major problem for sites trying to upgrade to this release.

Yes. The interop support has not been implemented in this series yet. (Sorry, the commit message was wrong)

I am currently working on it and testing it, based on the previous compatibility work (https://review.whamcloud.com/c/fs/lustre-release/+/63517 ). However, as I go through the validation, I am finding additional cases that were not sufficiently considered, so I do not yet have a reliable estimate for when it will be complete.

I agree that this patch should not land until the interop code is implemented and tested. Otherwise it could break master interop testing and create problems for sites upgrading to 2.18.0, as you pointed out.

The ofd_attr_set() hunk keeping `ofo_atime_ondisk` in sync isn't described here, and it isn't a units change - it is an independent fix for stale cached atime from 7c9ce8aac9e8 ("LU-13383 ofd: lazy atime update"). Could it be split into its own patch with a Fixes: tag so it can land separately?

The hui_* LASSERTF reordering in both wiretest.c copies is also unexplained; see the comment there.

(minor) The body doesn't mention `sb->s_time_gran = 1` in client_common_fill_super(). That is a fairly central part of the change - without it current_time() keeps truncating to whole seconds, so nothing the client generates locally would ever carry a sub-second value. Worth a line in the "sinks" bullet.

This changes the interpretation of several wire fields (ost_lvb, mdt_body, mdt_rec_*), so it should carry a Test-Parameters: line requesting interop runs against an older peer, e.g.

    Test-Parameters: clientversion=2.15 testlist=sanity,sanityn
    Test-Parameters: serverversion=2.15 testlist=sanity,sanityn

Without it the mixed-version paths this patch changes are never exercised by the automated test runs.

(minor) s64 nanoseconds since the epoch runs out at 2262-04-11 23:47:16 UTC (S64_MAX / NSEC_PER_SEC = 9223372036 s), not 2292 - this is the usual ktime_t limit. Worth correcting, since that number is the bound on what the new representation can hold.

(minor) The client advertisement is added by this patch: client_common_fill_super() gains OBD_CONNECT_NANOSEC_TIME for both the MDC and the OSC connect flags. "already advertised by clients" reads as if it came from an earlier change.

Hi Andreas and James,

I’d like to discuss this patch with you.

This patch does not currently include any compatibility support. For example, timestamps will break in configurations such as an ns-unaware MDS/OSS with an ns-aware client, or an ns-aware MDS/OSS with an ns-unaware client.

I’d like to discuss the following two points:

1. Whether a compatibility patch is necessary. For example, couldn't we avoid the need for a compatibility patch by merging this patch right before the release?

2. How far we should go in supporting compatibility. For example, should we also consider communication between an ns-aware MDS and an ns-unaware MDS? If so, the required patch would become quite large.

Best regards,
Sohei

Sohei, there *must* be compatibility between at least old/new clients/servers.  This should use the `OBD_CONNECT_NANOSEC_TIME` flag to determine if the clients are sending nsec or sec timestamps.  This had been part of the other nanosecond timestamp patches, so I had assumed it is part of this series as well.

Even if the patch was landed right before a release, there is no requirement/guarantee that clients and servers are upgraded together.  We require at least interop between successive LTS releases (e.g. 2.15.latest to 2.18.0), but prefer interop for longer than this if possible (e.g. 2.14.x or earlier).

There does not need to be major version interoperability between different MDS versions because they are typically upgraded together, but if this is practical to implement (i.e. it uses mostly the same code as client interoperability) it would be good to have this as well.

No interop handling :-( I would suggest splitting this patch into 2 new patches. One for clients and one for servers. That way we can see if "interop" works properly.

OBD_CONNECT_NANOSEC_TIME is missing from MDT_CONNECT_SUPPORTED here, and from OST_CONNECT_SUPPORTED below.

The client asks for it in client_common_fill_super(), but both servers unconditionally strip unsupported bits from the reply:

    mdt_connect_internal():  data->ocd_connect_flags &= MDT_CONNECT_SUPPORTED;
    ofd_parse_connect_data(): data->ocd_connect_flags &= OST_CONNECT_SUPPORTED;

so the bit is cleared on every connect and exp_connect_nanosec_time()/imp_connect_nanosec_time() return false even when both peers are new. Should the two masks be extended in this patch, so the follow-on compatibility patch has a flag that can actually be negotiated?

(minor) The matching comments on struct lu_attr's la_mtime/la_atime/la_ctime/la_btime in lustre/include/lu_object.h still read "in seconds since Epoch". Those fields change units in this patch too, and the follow-on rename patch leaves the comments alone as well.

I guess I had assumed when seeing these functions checking `OBD_CONNECT_NANOSEC_TIME` that the protocol interop was implemented.

(style) These are defined in obdclass/obdo.c right next to lustre_set_wire_obdo()/lustre_get_wire_obdo(), whose prototypes live in lustre_obdo.h. Declaring the new pair there would keep the obdo wire helpers together - not a bug, just placement.

The body explains the data-structure swap but not the reference-counting changes that come with it. class_export_put() is added in class_unlink_export() and tgt_client_free(), and removed in tgt_reply_data_init() and update_recovery_update_ses(). Those are behavior changes, not a mechanical hash->xarray conversion, and it would help to say what the intended ownership rule for obd_gen_ids is.

The old gen_kepcmp() also filtered on !exp->exp_failed, so lookups could never return a failed export; xa_load() has no such filter. That change deserves a line in the message too.

(style) Signed-off-by: normally comes before Change-Id:; the reversed order usually means the Lustre commit hook is not installed locally.

(style) The comment above still calls this a hash body. While renaming the field, `obd_gen_ids` reads as if it stores ids rather than exports keyed by generation - something like obd_gen_exports would say what it holds.

(defect) Two problems with this put.

First, ted_lcd is non-NULL for every target export (tgt_client_alloc() allocates it for all non-self exports), while an obd_gen_ids entry only exists for exports inserted by tgt_clients_data_init() when tgt_is_multimodrpcs_record() is true. So a regular client that connected after mount was never in the xarray, yet still gets a put here. The old code was guarded by hlist_unhashed(&exp->exp_gen_hash), which was exactly the "is it in the table" test; xa_erase()'s return value can serve the same purpose.

Second, nothing takes a matching reference on insert any more, so even for exports that are in the xarray this put has no counterpart.

Worth noting too: this runs with obd_dev_lock held, and if it ever is the final reference, class_export_put() -> obd_zombie_export_add() takes obd_dev_lock again.

(minor) xa_destroy() on the next line already erases every entry and frees the nodes, so this loop looks like a no-op. If the intent was to release the export references held by the xarray, the loop needs a class_export_put() on each entry (and a reference has to be taken on insert first).

Reusing `flag`, the char * used above for parsing the lcfg flag string, as the xa_for_each() entry variable is also confusing to read.

(defect) This drops a reference the xarray never took, and it does so from a context where the refcount is already zero.

The main caller chain is class_export_put() -> obd_zombie_exp_cull() -> class_export_destroy() -> obd_destroy_export() -> mdt_destroy_export()/ofd_destroy_export() -> tgt_client_free(). class_export_destroy() opens with

    LASSERT(refcount_read(&exp->exp_handle.h_ref) == 0);

so the class_export_put() here hits its own LASSERT(refcount_read(...) > 0) and LBUGs. That is on the teardown path of every regular server export, not just the ones recovered from last_rcvd.

mdt_init_export()'s err_free: path also calls tgt_client_free() directly, where the export is still live, so there it silently loses a reference instead.

The old code only removed the export when it was actually hashed (hlist_unhashed()); the xarray entry only exists for exports added by tgt_clients_data_init() with a non-zero lcd_generation, so an unconditional put cannot be right either way. xa_erase() returning the old entry could be used to tell the two cases apart.

(defect) The old cfs_hash_add_unique() took a reference on the export through gen_hash_ops.hs_get()->class_export_get(). ll_xa_insert() does not, so after the class_export_put() two lines below the xarray holds a bare pointer with no reference at all.

Every surviving class_export_put() that was paired with that reference is now unbalanced - see class_unlink_export(), tgt_client_free(), the error path below at tgt_set_reply_slot(), and distribute_txn_replay_handle(). Should this be class_export_get(exp) before the insert?

(minor) GFP_KERNEL here; Lustre server allocations in this path normally use GFP_NOFS (OBD_ALLOC_PTR() a few lines up does). This runs during target mount while reading last_rcvd, so reclaim recursing back into the same device seems worth avoiding.

(defect) The success-path class_export_put() below was removed with the switch to xa_load(), but this one on the tgt_set_reply_slot() error path was left behind. xa_load() returns no reference, so this drops one that belongs to someone else.

(defect) cfs_hash_lookup() returned a referenced export, and that reference was handed to tsi->tsi_exp and released later by distribute_txn_replay_handle():

    if (tgt_ses_info(env)->tsi_exp != NULL) {
            class_export_put(tgt_ses_info(env)->tsi_exp);

xa_load() returns an unreferenced pointer, so that put now drops a reference this path never acquired, and tsi_exp can be freed under the replay while it is still in use.

Separately, gen_kepcmp() used to reject exports with exp_failed set, so an evicted export could never end up in tsi_exp. xa_load() has no such filter - is update replay expected to cope with a failed export here?

(typo) "utilies" -> "utilities" in the subject, and "utilites" -> "utilities" on the body line below.

(minor) The body only covers the lsvcgssd/l_getauth split, but the diff also appends $(UTILS_CFLAGS) to AM_CFLAGS. Is that intended to ride along here, or should it be a separate change? Either way it should be mentioned so the hunk isn't a surprise.

(minor) This hunk isn't part of the client/server split. If the intent is to pick up the distro build flags the way lustre/utils/Makefile.am does, note that file also sets

    AM_LDFLAGS := $(UTILS_LDFLAGS)

and UTILS_LDFLAGS is exported alongside UTILS_CFLAGS in lustre.spec.in. Here the link flags are still unset, and each program overrides them with its own lsvcgssd_LDFLAGS/l_getauth_LDFLAGS/lgss_keyring_LDFLAGS/lgss_sk_LDFLAGS = $(KRBLDFLAGS), so an AM_LDFLAGS alone wouldn't take effect either - $(UTILS_LDFLAGS) would have to be added to each of those lines. Should the hardening flags be applied consistently, in a patch of their own?

(suggestion) While completing the client/server split: lustre/scripts/systemd/Makefile.am still installs lsvcgss.service unconditionally under HAVE_SYSTEMD, and the spec adds it to lustre.files the same way, even though lsvcgss_sysd and the init script are already SERVER-only. So a client package keeps shipping a unit for a daemon it no longer builds. Worth wrapping in SERVER here too?

With these two now server-only, a client-only install has no l_getauth and no lsvcgssd, but test-framework.sh still resolves both on the local (client) node and has no /usr/sbin fallback:

    export L_GETAUTH=${L_GETAUTH:-"$LUSTRE/utils/gss/l_getauth"}
    [ ! -f "$L_GETAUTH" ] && export L_GETAUTH=$(which l_getauth 2> /dev/null)
    export LSVCGSSD=${LSVCGSSD:-"$LUSTRE/utils/gss/lsvcgssd"}
    [ ! -f "$LSVCGSSD" ] && export LSVCGSSD=$(which lsvcgssd 2> /dev/null)

When the test node runs a client build both expand to the empty string, and the values are then used on the server nodes:

    do_nodes $nodes "$LSVCGSSD -vvv -s -m -o -z $options" || return 1
    do_nodesv $node "$L_GETAUTH -d"
    do_nodesv $servers "$LCTL set_param sptlrpc.gss.rsi_upcall=$L_GETAUTH"

start_gss_daemons() then runs "-vvv -s -m -o -z" remotely and returns 1, and init_gss() does start_gss_daemons() || error_exit, so setup aborts for any SHARED_KEY/krb5 run on a mixed client/server install. Should this patch also give L_GETAUTH/LSVCGSSD a "/usr/sbin/..." fallback (the way LR_READER and LSOM_SYNC do a few lines above), or resolve them on a server facet instead?

Should this get a `Fixes:` label?

I think this is basically a change to the fixture of Lustre since the beginning of time so there's nothing to declare here?

(minor) The double-iput fix described here lands in revalidate_statahead_dentry(), not ll_statahead_interpret(). The latter only igrab()s the inode into se_inode; it is revalidate_statahead_dentry() that calls ll_splice_alias() and now clears se_inode before the IS_ERR() check. Worth correcting the name so the change stays findable via git log later.

(minor) Not a bug, but is "both of them consequences of the OBF namespace" complete? d_lustre_invalidate() only sets a flag (llite_internal.h), so a directory renamed on another client keeps its old-parent dentry hashed and on the inode alias list. A later lookup of the new path then gets that alias from __d_find_any_alias(), with a different d_parent and no .lustre/fid anywhere in the picture, so it takes the same cross-parent __d_unalias() trylock path and the same -ESTALE.

That is ordinary d_splice_alias() semantics and not a reason to change the patch, but as written the paragraph reads as if the by_fid test bounds the new -ESTALE exposure, and it doesn't.

well....

[  368.091239] Lustre: lustre-MDT0000-mdc-ffff89a08865b000: Force grant RPC slot (1 current) to proc with flag: 208840.
[  368.091527] Lustre: Skipped 3 previous similar messages
[  370.228394] Lustre: lustre-MDT0000-mdc-ffff89a041f49000: Force grant RPC slot (1 current) to proc with flag: 208840.
[  374.536843] Lustre: lustre-MDT0000-mdc-ffff89a041f49000: Force grant RPC slot (1 current) to proc with flag: 208840.
[  374.536967] Lustre: Skipped 2 previous similar messages
[  382.893306] Lustre: lustre-MDT0000-mdc-ffff89a041f49000: Force grant RPC slot (8 current) to proc with flag: 208840.
[  382.893500] Lustre: Skipped 51 previous similar messages
[  399.281310] Lustre: lustre-MDT0000-mdc-ffff89a08865b000: Force grant RPC slot (4 current) to proc with flag: 208840.
[  399.282289] Lustre: Skipped 140 previous similar messages
[  402.163522] obd_memory max: 4662112620, obd_memory current: 4661744265
[  402.163950] kthreadd invoked oom-killer: gfp_mask=0x400dc0(GFP_KERNEL_ACCOUNT|__GFP_ZERO), order=2, oom_score_adj=0
[  402.164047] CPU: 0 PID: 2 Comm: kthreadd Tainted: G           O      -------  ---  5.14.0 #26
[  402.164115] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.17.0-9.fc43 06/10/2025
[  402.164175] Call Trace:
[  402.165043]  <TASK>
[  402.166569]  dump_stack_lvl+0x57/0x7d
[  402.167511]  dump_header+0x4f/0x486
[  402.167545]  ? ___ratelimit+0x83/0x100
[  402.167578]  oom_kill_process.cold+0x54/0x79
[  402.167614]  ? lock_release+0x1c7/0x2e0
[  402.167648]  ? _raw_spin_unlock+0x1a/0x30
[  402.167679]  out_of_memory+0xc8/0x510
[  402.167712]  __alloc_pages_slowpath.constprop.0+0x6d3/0xcb0
[  402.167751]  __alloc_pages+0x2ba/0x2d0
[  402.167781]  copy_process+0x1fd/0x24b0
[  402.167813]  ? update_load_avg+0x60/0x260
[  402.167845]  ? lock_acquire+0x15c/0x2f0
[  402.167875]  ? __update_load_avg_se+0x153/0x380
[  402.167913]  ? trace_pelt_se_tp+0x29/0xc0
[  402.167945]  ? lock_release+0x1c7/0x2e0
[  402.167976]  kernel_clone+0x86/0x340
[  402.168005]  ? do_raw_spin_unlock+0x46/0x90
[  402.168051]  ? finish_task_switch.isra.0+0xef/0x330
[  402.168088]  kernel_thread+0x4a/0x50
[  402.168118]  ? kthread_park+0x80/0x80
[  402.168151]  kthreadd+0xc5/0x110
[  402.168185]  ? kthread_is_per_cpu+0x20/0x20
[  402.168220]  ret_from_fork+0x1f/0x30
[  402.168290]  </TASK>
[  402.168321] Mem-Info:
[  402.168347] active_anon:255934 inactive_anon:234092 isolated_anon:0
[  402.168347]  active_file:236 inactive_file:1651 isolated_file:214
[  402.168347]  unevictable:0 dirty:0 writeback:192
[  402.168347]  slab_reclaimable:9149 slab_unreclaimable:1153520
[  402.168347]  mapped:875 shmem:467586 pagetables:2320
[  402.168347]  sec_pagetables:0 bounce:0
[  402.168347]  kernel_misc_reclaimable:0
[  402.168347]  free:30537 free_pcp:910 free_cma:0

so, the OOM is real, it was traced to a longer retention of RPCs in replay list, mostly related to directory-churn workloads it seems.

It is also affected by Alex's case of not carrying the previous patch in the series (67391) so with 2000 stripes seen the normally relatively small RPCs blow up to 256k greatly exacerbating the memory consumption.

the problem is open requests are leaked because somehow atomic open machinery returns IT_OPEN_OPEN that sets rq_replay that is never cleared.

after some more thinking, this looks wrong.

For regular files we absolutely want to reuse these stale dentries, all this means is we lost the lookup bit some time ago, so what?

I guess for DIRECTORIES we don't and if it's an open by fid we can have a directory here, so we should also check if we found a directory before we throw it away.

Or should we only throw away for the by_fid regardless of mode? Aliases for regular files don't really much do they so probably not?

Though in instrumentation this is NOT a big signal compared to the leak (200 hits here for 8000 rq_replay requests stuck)

(defect) For regular files this throws away a still-usable alias instead of reusing it. Before this patch ll_find_alias()'s invalid alias was always d_move()'d and reused; a d_lustre_invalid() dentry just means the LOOKUP lock was dropped, which is the common case. Now every non-directory lookup that finds such an alias discards it (d_invalidate() + dput()) and re-adds a fresh dentry via d_add().

An open note on an earlier patchset raised the same doubt and is still unresolved. Should the discard be gated on the by_fid / directory case only, leaving regular-file aliases to be reused as before?

(style) This isn't a bug, but %pd prints the raw name and bypasses the filename obfuscation that obd_enable_fname_encoding controls. The rest of namei.c uses DNAME with encode_fn_dentry(), e.g.

    "cannot splice inode %p as "DNAME": rc = %d\n", inode, encode_fn_dentry(de), ...

(nit) de cannot be NULL here - the argument is never NULL and the else branch only reassigns it when d_splice_alias() returned non-NULL - so IS_ERR_OR_NULL() is just IS_ERR(). It also reads oddly next to the plain !IS_ERR(de) guard a few lines below; if the patch is refreshed anyway, making both the same would help.

warn: ll_splice_alias():'de' can also be NULL

(minor) This comment now says the opposite of what the code requires: d_splice_alias() opens with BUG_ON(!d_unhashed(de)), and the commit message spends a paragraph establishing that no caller ever hands over a hashed dentry. Worth rewording while it is being touched (and it carries two typos, "Atoimc" and "passin").

Several hunks aren't accounted for by the description, and it isn't obvious which are deliberate:

- lnet/utils/lnetconfig/liblnetconfig_netlink.c empty-flow-set handling
- lov_obd.c switching class_register_type() from enable_proc=true to false
- llapi_param_display_value() changing its success return from 0 to the raw yaml rc
- llapi_get_poolbuf() gaining `poolname++`
- lu_tgt_pool.op_array changing from __u32 to u32
- lod_pool.c setting op_obd_idx (LOD has no .o_pool_find, so nothing reads it yet)

Could the message name the new symbols too - LUSTRE_CMD_POOLS, o_pool_find()/lov_pool_list(), lustre_pools_dump() - so the change is findable via git log?

(typo) The kernel-doc name doesn't match the enum: `lustre_pool_props_attrs` here vs `lustre_pool_prop_attrs` below, and there is no " - description" after the name. kernel-doc warns:

    lustre_kernelcomm.h:248: expecting prototype for enum lustre_pool_props_attrs. Prototype was for enum lustre_pool_prop_attrs instead

The same slip already exists for lustre_target_prop_attrs, so it looks copied from there - as is the LUSTRE_POOL_ATTR_SOURCE description "obd device targets belong too", which describes targets rather than pools.

(style) This isn't a bug, but `s` is read from `file->private_data` before seq_open() has set it and then reassigned two lines later - the initializer can just be dropped.

(typo) The reference is now held for the debugfs entry rather than a /proc file.

(minor) The kernel-doc headers were not renamed along with the functions. contrib/scripts/kernel-doc reports four warnings on this file now:

    lov_pool.c:154: expecting prototype for pool_proc_next(). Prototype was for pool_seq_next() instead
    lov_pool.c:192: expecting prototype for pool_proc_start(). Prototype was for pool_seq_start() instead
    lov_pool.c:243: expecting prototype for pool_proc_stop(). Prototype was for pool_seq_stop() instead
    lov_pool.c:270: expecting prototype for pool_proc_show(). Prototype was for pool_seq_show() instead

The /proc wording in those blocks ("/proc sequence file iteration tracking structure", "Start seq_file iteration via /proc", "inode to store iteration state for /proc") is also stale now that these back a debugfs file.

(style) This isn't a bug, but the commented-out call here and the matching one in lustre_pools_end_cb() read as work-in-progress. Lustre C files also avoid // comments. Can they come out before this lands?

(defect) With pools now living only under obd_debugfs_entry, the old world-readable /proc/fs/lustre/lov/<dev>/pools/<pool> is gone and the only copy is under /sys/kernel/debug, which is mode 0700.

llapi_get_poollist() and llapi_get_poolmembers() grew a Netlink fallback for that, but llapi_search_ost() did not - it still does poolpath() -> fopen(), and poolpath() -> get_lustre_param_path() -> cfs_get_param_paths() globs /sys/kernel/debug/lustre/lov/<fs>-*/pools, which a normal user cannot traverse (GLOB_NOMATCH -> -ENOENT).

That propagates to llapi_stripe_param_verify(), which turns it into "pool '%s fsname %s' does not exist" and -EINVAL, so `lfs setstripe -p <pool>` fails for an unprivileged user. `lfs df --pool` and llapi_layout_sanity() -> llapi_search_ost() hit the same path.

This change is the top of the relation chain, so nothing later picks it up. Should llapi_search_ost() get the same Netlink fallback in this patch?

(defect) On the -EMSGSIZE path `hdr` is neither ended nor cancelled, so genlmsg_end() never runs and `nlh->nlmsg_len` still covers only the fixed header.

The LUSTRE_PARAM_ATTR_HDR/SOURCE attributes already appended to the skb therefore sit past `nlmsg_len`, and userspace reads those bytes as the start of the next nlmsghdr.

warn: lustre_obd_genl_many_dump():inconsistent returns '&(ctx->ogc_xas.xa)->xa_lock'.

(minor) The `! ` prefix is skipped but nothing ever negates the match - lov_pool_list() just does `glob_match(poolname, pool->pool_name)`, so `! foo` selects the same pools as `foo`. No other parameter path in the tree uses a `! ` prefix either. Is the exclusion meant to be implemented, or can this come out?

(defect) `all` is fixed at three slots, but `start` ends up declaring one NLA_NESTED attribute per pool, and lnet_genl_parse_list() consumes one sub-list from `data[]` for each NLA_NESTED key it emits:

    rc = lnet_genl_parse_list(msg, data, ++idx);
    idx = rc;

With two pools the second nested key looks at `data[2]`, which is NULL, so parse_list() returns 0 without emitting anything and resets `idx` to 0; the third nested key then re-uses `data[1]`. lustre_stats_dump() allocates `ls_num + 2` and fills `all[1..ls_num]` for exactly this reason.

Should this be `pool_count + 2` with `all[1..pool_count]` all pointing at `props`? lustre_pools_free_keys() hard-codes 3 in its OBD_FREE_PTR_ARRAY() as well.

(defect) `lkl_list[]` is a flexible array member, so it is not counted by `sizeof(struct ln_key_list)` and this assignment copies only `lkl_maxattr` - the `pools` and `source` entries stay zeroed from the OBD_ALLOC.

Compiling against the real header gives `sizeof(struct ln_key_list)` = 8 and `sizeof(struct ln_key_props)` = 16, and after `*start = pool_keys` both `lkl_list[1].lkp_value` and `lkl_list[2].lkp_value` are NULL.

lnet_genl_parse_list() then emits those two keys with no LN_SCALAR_ATTR_VALUE and NLA_UNSPEC as the type, so the reply carries no `pools`/`source` names and llapi_param_display_value() never dispatches to print_out_pools(). This branch is taken as soon as `pool_count > 1`, so does `lfs pool_list` still work on a filesystem with two pools?

Would copying `hdr_len + sizeof(struct ln_key_props) * (LUSTRE_POOL_ATTR_MAX + 1)` bytes from `pool_keys` work here instead of the struct assignment?

(style) LU-20587 is filed as "lnetctl peer show -v 2 causes kernel oops", but the body only describes a capacity limit. Could it say that this is fixing a crash, and describe how running out of skb space in lnet_peer_ni_show_dump() leads to the oops? That makes it much easier to decide whether the change needs backporting.

(defect) This fixes a kernel oops, so it should carry a Fixes: tag. lnet_peer_ni_show_dump() and its single-skb dump came in with:

Fixes: f0be00678cc8 ("LU-9680 lnet: collect data about peer_ni by using Netlink")

(typo) "for it struct sk_buf" - the type is `struct sk_buff`, and "it" should be "its". Same misspelling on the line below.

(style) Not a bug, but 2580 has no derivation in the source, so it silently goes stale the next time an attribute is added to lnet_peer_ni_keys/lnet_peer_ni_list. Same for the literals below: 64 is LNET_NIDSTR_SIZE and the 128 in the version 4 comment is LNET_MAX_SHOW_NUM_NID - using the macros would at least keep them tied to what they measure.

Carrying forward the unbounded-allocation point from patchset 1, which does not look addressed yet: nothing caps cb->min_dump_alloc, and there is no error when the computed size is unreasonable. The sibling handlers in this file and in obdclass do bound it, e.g. lnet_route_show_start():

    len += ROUTER_MSG_VALUES_SIZE * rlist->lgrl_count;
    if (len > BIT(sizeof(cb->min_dump_alloc) << 3)) {
            NL_SET_ERR_MSG(extack, "Netlink msg is too large");
            rc = -EMSGSIZE;
    } else {
            cb->min_dump_alloc = len;
    }

Peers are unbounded (unlike NIs, which are capped by lnet_interfaces_max), so the same guard - or a cap on how many peers one dump covers - seems needed here too.

(typo) LNET_GENL_VERION -> LNET_GENL_VERSION.

(defect) Nothing caps cb->min_dump_alloc and there is no error when the computed total is unreasonable. This was raised on patchset 1 ("allocating an 4G in kernel - very very bad style") and the reply agreed peers still need overflow handling since, unlike NIs, they are not bounded by lnet_interfaces_max - but patchset 4 still accumulates straight into the field.

The sibling handler in this file bounds it, in lnet_route_show_start():

    len += ROUTER_MSG_VALUES_SIZE * rlist->lgrl_count;
    if (len > BIT(sizeof(cb->min_dump_alloc) << 3)) {
            NL_SET_ERR_MSG(extack, "Netlink msg is too large");
            rc = -EMSGSIZE;
    } else {
            cb->min_dump_alloc = len;
    }

and lustre/obdclass/kernelcomm.c does the same. Two reasons that guard matters here:

min_dump_alloc is u16 in netlink_callback before v5.7, so on the older supported kernels the += silently wraps modulo 65536 - and can wrap to less than PEER_KEY_TABLE_MSG_SIZE.

On newer kernels the skb head is still one contiguous kmalloc, not a kvmalloc: netlink_dump() calls alloc_skb(alloc_min_size, GFP_KERNEL), and __alloc_skb() -> kmalloc_reserve() ends in kmalloc_node_track_caller(). So above KMALLOC_MAX_SIZE the dump can only fail with -ENOBUFS, and below it every dump invocation is asking for a high-order allocation.

(defect) Skipping the peer NI walk for version 0 undercounts, because the dump still emits per peer NI at version 0. In lnet_peer_ni_show_dump() the `if (gnlh->version)` guard only covers the credits/stats block; the nest, LNET_PEER_NI_LIST_ATTR_NID and LNET_PEER_NI_LIST_ATTR_STATE are written for every version:

    nid_list = nla_nest_start(msg, LNET_PEER_NI_ATTR_PEER_NI_LIST);
    while ((lpni = lnet_get_next_peer_ni_locked(lp, NULL, lpni)) != NULL) {
            struct nlattr *peer_nid = nla_nest_start(msg, count++);
            nla_put_string(msg, LNET_PEER_NI_LIST_ATTR_NID, ...);

That is 4 + nla_total_size(strlen(nid) + 1) + nla_total_size(5) per peer NI, plus 4 for the nid_list nest, none of which appears in PEER_VERSION_0_VALUES_MSG_SIZE.

For a peer with a 17-character NID: 20 (genl header) + 24 (primary nid) + 4 (multirail) + 4 (nest) + 40 per peer NI. One NI is 92 and fits in 136; three NIs is 172 and does not. Longer NIDs push the single-NI case over as well.

Version 0 is not a corner case - `lnetctl peer show` with no -v passes detail = 0 as the genl version (lnetctl.c jt_show_peer()), and `lnetctl peer list` passes 0 literally, so this is the common invocation.

Related: the per-peer-NI NID string is not charged in any of the PEER_VERSION_* terms. For version >= 1 the slack in PEER_VERSION_1_VALUES_MSG_SIZE happens to absorb it, but nothing in the table accounts for it, so it goes wrong again the next time one of these constants is tightened. LNET_PEER_NI_ATTR_HDR is documented as 48 bytes here but is nla_put_string(msg, ..., ""), i.e. 8 bytes, and only for the first peer - that leftover is what makes the version 0 number look adequate.

(minor) This builds the whole UDSP info for every peer NI - lnet_udsp_get_construct_info() does a peer table hash lookup and fills a ~2KB struct - and the only thing read back is `cud_pref_rtr_nid[0] != 0`. That is just `!list_empty(&lpni->lpni_rtr_pref_nids)`, which needs no buffer and no lookup.

Related to the same allocation: it is done unconditionally at the top, so version 0 and versions 1-3 pay for a buffer they never touch, and LIBCFS_ALLOC_ATOMIC uses GFP_ATOMIC even though this is ->start() in process context - the genradix_ptr_alloc() calls a few lines above use GFP_KERNEL.

Worth noting the cost overall: this loop repeats the peer and peer NI traversal that lnet_peer_ni_show_dump() then does again, under ln_api_mutex, so `lnetctl peer show -v 4` now walks every peer twice.

(defect) None of the nla_nest_start() results in this handler are checked, and nla_nest_end() writes through the pointer without a NULL test:

    static inline int nla_nest_end(struct sk_buff *skb, struct nlattr *start)
    {
            unsigned long diff = skb_tail_pointer(skb) - (unsigned char *)start;
            start->nla_len = diff;

So once the skb runs out of room, nla_nest_start() returns NULL and the matching nla_nest_end() at the bottom of the loop faults. The nests opened for the -v 2 stats (send/recv/drop/health) are the ones LU-20587 names in its title.

Enlarging the skb makes this less likely, but any residual under-estimate still lands here. Should the dump bail out with -EMSGSIZE when a nest cannot be opened, so a short estimate degrades into a truncated dump instead of an oops?

(defect) This fixes a crash, so it should carry a Fixes: tag. The unchecked nla_nest_start()/nla_put_*() calls in lnet_peer_ni_show_dump() arrived with the original Netlink conversion:

    Fixes: f0be00678cc8 ("LU-9680 lnet: collect data about peer_ni by using Netlink")

(style) This isn't a bug, but Change-Id: normally sits after Signed-off-by:. Having it first usually means the Lustre commit hooks aren't installed.

(defect) lnet_udsp_info_send() is left on the unchecked path. It ignores nla_nest_start() returning NULL and then runs

    nla_nest_end(msg, udsp_attr);
    nla_nest_end(msg, udsp_info);

at the end of the function, which writes through the NULL start pointer. It also returns 0 on a full skb, so rc here cannot distinguish that from success.

`lnetctl peer show -v 4` sets gnlh->version to 4 and reaches this, so the oops named in the subject still looks reachable from lnet_peer_ni_show_dump().

(minor) The -ENOMEM branch here still leaves a half-built message in the skb.

By this point genlmsg_put() has run and PRIMARY_NID / the nest headers / the peer NI string have been appended, so skb->tail is past the nlmsg_len that genlmsg_put() wrote. Going straight to unlock_api_mutex without genlmsg_cancel() leaves that short header followed by orphan attribute bytes; netlink then appends NLMSG_DONE after them, and a NLMSG_NEXT walk from the short header lands in the middle of the leftovers.

The new EMSGSIZE branch above cancels, so it would be consistent for this one to cancel too before returning.

(defect) lgpl_hdr_sent is set as soon as the attribute is appended, but the message it lives in can still be rolled back.

Every `goto msg_full` below reaches `genlmsg_cancel(msg, hdr)`, which trims the skb back past LNET_PEER_NI_ATTR_HDR. The flag stays true, so when this peer is retried against the fresh skb the attribute is not emitted again - and no later entry emits it either.

The first entry is the one most exposed. lnet_peer_ni_show_start() never sets cb->min_dump_alloc (lnet_net_show_start() sets U16_MAX), so the first skb is NLMSG_GOODSIZE, and the ~65 keys in all[] have already consumed most of it before the loop starts. A version-4 peer with two or three peer NIs then overflows partway through this entry.

On the tools side that attribute is exactly what stops the header repeating - liblnetconfig_netlink.c clears keys[1].lkp_value ("peer") only via

    if (attr && parent->lkp_value) {
        free(parent->lkp_value);
        parent->lkp_value = nla_strdup(attr);
    }

so if it never arrives, `strlen(keys[1].lkp_value)` stays non-zero and "peer:" is printed once per netlink message instead of once per dump.

Would setting the flag after genlmsg_end() succeeds, or clearing it in msg_full when this iteration set it, keep it tied to what actually reached user space?

(minor) A realloc() failure is swallowed here - yaml_parse_value_list() returns void and neither data->error nor data->errmsg is set, so lnetctl prints a truncated YAML document and still exits 0.

The older grow in yaml_netlink_msg_parse() returns NL_STOP for the same condition. Could this one report the failure too, so a short document isn't mistaken for the full listing?

(minor) This is the mirror of the accounting fixed a few lines above: data->buffer moves backwards here, so the free space grows and it should be `*size += data->indent + 2;`.

It only fires when a snprintf() returns negative, so it isn't reachable today, but leaving one of the two sites inverted invites the next reader to copy the wrong one.

(minor) `<= 1` also passes when the header is missing altogether, which is the other way the once-per-dump attribute can go wrong. The dump promises exactly one, so `((hdrs == 1))` would catch both a lost and a repeated header.

(minor) The comment above says the check also confirms the retry path doesn't duplicate entries, but grep -q only proves each primary NID appears at least once.

Comparing `grep -c "primary nid: " $out` against $num_peers would actually cover the duplicate case.

(style) Not a bug, but the peers and $out are only cleaned up when every assertion passes - any error() above leaves 300 peers (1500 with SLOW=yes) configured and the YAML file in $TMP for the next subtest.

A `stack_trap "rm -f $out"` plus a stack_trap for the peer teardown registered right after the add loop would make the failure path tidy, and would let the del loop drop out of the verification loop.

cl2osc() is container_of_safe() on oo_cl, which sits at offset 0, so IS_ERR_OR_NULL(cl2osc(obj)) is the same test as IS_ERR_OR_NULL(obj). It checks the cl_object pointer that was passed in, not whether the osc_object still exists; an object that was freed while still referenced is an ordinary pointer, not NULL or an ERR_PTR.

Could the message name the pointer that actually becomes NULL or an error, and where it is set that way?

ldlm/ldlm_resource.c:
 warn: ldlm_resource_get():inconsistent indenting

This paragraph explains the bucket-array sizing change, but not that the bucket selection function changed shape as well. ldlm_res_hop_hash() summed all four words of the res_id; ldlm_res_hop_fid_hash() hashes only the FID in name[0] and name[1]. Worth a sentence, since it changes which resources share an ldlm_ns_bucket.

The new LDLM_FL_KUNIT_TESTING flag in lustre_dlm_flags.h isn't mentioned anywhere in the body either - is that meant to be part of this patch, or a separate cleanup of the bare BIT(63) uses?

ldlm/ldlm_resource.c:
 warn: ldlm_resource_get():inconsistent indenting

(suggestion) No Test-Parameters: line for a change of this reach - it rewrites the resource lookup path for MDC/MDT/OSC/OST/MGC/MGT namespaces and touches recovery, eviction and quota. Something like

    Test-Parameters: testlist=recovery-small,replay-single,replay-dual,conf-sanity,sanity-quota

would get the paths this rewrites covered by the test run.

lr_hash and lr_rcu share storage, so call_rcu(&res->lr_rcu, ...) writes over lr_hash.next. That was safe with cfs_hash because every chain walk held the bucket lock; rhashtable readers walk the chain under rcu_read_lock() alone.

ldlm_resource_putref() -> __ldlm_resource_putref_final() does rhashtable_remove_fast(), then ldlm_resource_free(res, true) -> call_rcu(), and call_rcu() sets head->next = NULL right away. RCU removal requires the removed node's next pointer to stay intact for a grace period, because a reader can already be sitting on that node when it is unlinked.

A concurrent ldlm_resource_get() walking that bucket then loads NULL as the next pointer. rht_is_a_nulls() only tests bit 0, so NULL is not recognised as end-of-chain: the loop body runs with pos == NULL, rht_obj() subtracts head_offset, and rs_cmp() dereferences the result.

struct lu_object_header keeps loh_hash and loh_rcu as separate fields, with a comment about exactly this requirement. Should lr_rcu be split out of the union the same way?

This isn't a bug, but BIT() is unsigned long while l_flags is __u64, so BIT(63) is out of range on a 32-bit build. Every other flag in this file spells the value out as 0x...ULL with a /* bit N */ comment and provides ldlm_is_/ldlm_set_ helpers - consider matching that:

    #define LDLM_FL_KUNIT_TESTING 0x8000000000000000ULL /* bit 63 */
    #define ldlm_is_kunit_testing(_l) LDLM_TEST_FLAG((_l), 1ULL << 63)
    #define ldlm_set_kunit_testing(_l) LDLM_SET_FLAG((_l), 1ULL << 63)

which would also let the two ldlm_lock.c users read like the surrounding flag tests.

This gives lock_res_and_lock() a new failure return, but only 8 of the 111 call sites look at the result; the rest go straight on to unlock_res_and_lock(), which is unlock_res(lock->l_resource) on the very pointer that produced the error.

As far as I can tell the error can't fire today: l_resource is only cleared by the rcu_assign_pointer(lock->l_resource, NULL) in ldlm_lock_put(), which runs after h_ref has already reached zero, so no caller that holds a lock reference can observe it, and nothing assigns an ERR_PTR to l_resource at all.

So either it is unreachable, and the error plumbing added through ldlm_lock_enqueue(), ldlm_callback_handler(), osc_ldlm_glimpse_ast() and osc_extent_release() could go, or it is reachable and the other 103 callers need converting in the same patch. Which is it?

(minor) This critical section only spans the two pointer loads - res is dereferenced at the lr_type test below and handed to ldlm_resource_putref(), and ns is used by lprocfs_counter_decr(), all after rcu_read_unlock(). If l_resource really needed RCU protection here the section would have to reach as far as the putref; as written it protects nothing that the lock's own resource reference doesn't already cover.

The LASSERT(lock->l_resource != LP_POISON) a few lines up reads the same field bare, which suggests the plain read was fine. Could this just go back to a direct load?

ns_reclaim_start is now only ever assigned 0 in ldlm_namespace_new(); the ldlm_res_to_ns(res)->ns_reclaim_start++ that used to advance it went away with the per-bucket bookkeeping in ldlm_reclaim_lock_cb(). rcd_start is therefore always 0, the rcd_skip/rcd_cursor fast-forward below can never trigger, and every reclaim pass restarts on the same resources. nsb_reclaim_start is incremented but no longer read either.

That drops the round-robin scan the ldlm_reclaim_res() comment still describes, and it is not mentioned in the commit message. Is the intent to keep the round-robin, or to remove it and the now-dead rcd_skip/rcd_cursor/rcd_start fields with it?

Also worth noting the units no longer match: rcd_cursor counts resources while tbl->size is a bucket count.

(minor) This comment doesn't match the new scheme. With the rhashtable the table holds no reference of its own: an entry is removed exactly when `lr_refcount` hits zero in `__ldlm_resource_putref_final()`, and `rs_cmp()` hides zero-refcount entries from lookups. During the walk the only extra reference is the `refcount_inc_not_zero()` in `ldlm_resource_for_each()`.

So the count printed below is one lower than the number of stray references, and a resource holding exactly one leaked reference no longer produces the message at all.

(style) This is a new exported API and it has no kerneldoc, unlike `ldlm_resource_get()` right below. Worth documenting the callback contract, in particular that the callback owns no reference (it must not putref), that a non-zero return stops the walk, and that a callback can be invoked more than once for the same resource - `rhashtable_walk_start()` here is the void variant, so the `-EAGAIN` a resize produces is discarded and the walk restarts from slot 0. That last property is what forced the `l_pending_chain` check in `ldlm_chain_lock_for_replay()`, and every future callback author needs to know about it.

(minor) duplicate `Signed-off-by: Minh`...

(style) instead of putting this here, it would be better to `#define GENRADIX_NODE_SIZE PAGE_SIZE` in the compat code and use that instead of `LUSTRE_NL_ENTRY_SIZE_MAX`.

So the table stuff is meant to obd specific but this is really really generic. The code seems to be a mix of both behaviors of being obd specific and then super generic. I would prefer that it was only obd specific. Its not going to be easy to make dump_one() flexible for all cases. So I'm not sold on a dump function.

Same here. While its labled as obd specific its really super generic.

`ops->collect()` is called with `obd_device_lock()` held, and that is `xa_lock(&obd_devs)` -- a spinlock. So a collect callback can't sleep, and anything it allocates (the natural implementation is `genradix_ptr_alloc()`, which existing callers such as tgt_mount.c use with GFP_KERNEL) has to be GFP_ATOMIC.

Nothing in the descriptor comment in lustre_kernelcomm.h says this, and the header also doesn't say that `collect` owns updating `ctx->count` and must store entries at 0..count-1 (both the cleanup loop here and `lustre_obd_nl_dump()` assume that). Since there is no in-tree implementation yet to copy from, could the contract be spelled out with the struct?

Holding the spinlock across the whole device scan is also a change in character from the existing handlers -- `lustre_stats_start()` does its scan outside `obd_device_lock()`.

`cb->min_dump_alloc` is set to a fixed `ops->min_alloc` regardless of how many entries `collect()` gathered. The two existing handlers scale it: `lustre_device_list_start()` uses `class_obd_devs_count() * DEVICE_VALUE_PACKET_SIZE`, and `lustre_stats_start()` accumulates `STATS_MSG_DATASET_SIZE * rc`. With a constant, any node with more devices than the descriptor author assumed overflows the skb -- and per the comment below, that ends the dump with an error rather than continuing. Should this be `ops->min_alloc * ctx->count` plus the key-table size?

When the skb fills, this returns -EMSGSIZE and the whole dump is aborted, dropping every entry from `ctx->index` onwards. The usual pattern -- and what `lustre_device_list_dump()` does -- is to stop adding messages and return the byte count so netlink flushes the skb and re-enters the dump:

    return rc < 0 ? rc : msg->len;

`ctx->index` is already carried in the context, so resumption would work; only the return value is missing. Same for the `dump_one()` failure just below, where `rc` may well be -EMSGSIZE too.

No caller exists yet, so this isn't reachable today, but it looks worth fixing before the first command is wired up.

This looks like a reasonable suggestion?

(minor) This is the same loop that is still open-coded in `lustre_stats_dump()` above. The patch adds the `nla_put` return checks to that copy and lands a second, identical copy here, so a future fix to one will miss the other -- which is the opposite of what the commit message says the change does.

Could `lustre_stats_dump()` just call this?

    rc = lustre_nl_put_dataset(msg, stats, LUSTRE_STATS_ATTR_DATASET);
    if (rc)
        GOTO(out_cancel, rc);

This also seems reasonable

(style) dropping the `static struct genl_family lustre_family;` forward declaration left two blank lines here; one is enough.

(style) not a bug, but `stats_dataset_list` becomes a kernel-global symbol here with no subsystem prefix, which is easy to collide with. `lustre_stats_dataset_list` would match `lustre_family` and `lustre_stats_dump()` alongside it. The extern in lustre_kernelcomm.h would need the same rename.

(style) doesn't need a whole saga.  Just a short comment like `copy by element, struct copy only copies fixed header` or maybe use `unsafe_memcpy()` to avoid the warning.

(minor) This repeats the xa_lock()/__xa_alloc()/atomic_inc() sequence from ldebugfs_stats_alloc() verbatim, so the two copies can drift apart.

This was raised on the first revision and still applies: ldebugfs_stats_alloc() could compute its truncated source string and then call ldebugfs_stats_register(), leaving only the debugfs file creation behind and keeping one copy of the xarray insertion.

(style) presumably should have a `Fixes:` line?

(style) prefer `Change-Id:` after `Signed-off-by:`

Could the message name the fixes it is relying on, and the minimum kernel version they are in? As written "newer kernels and fixes landed for better NFS support" is not verifiable, and LU-12661 is still Open with summary "sanity test_817: FAIL: failed to execute 'true' command".

The exclusion 3e2c28437404 removed here was widened from 4.14 to 4.12 specifically because SLES 4.12.14 hit it, and 4fed33473ca2 recorded that the failure happens "regardless of whether the nfs export is backed by a lustre mount or not" - i.e. it is nfsd holding the file open, not something llite can fix. That makes it worth spelling out which change is expected to have altered this.

(typo) "sanity tets 817" -> "tests".

(style) carried over from the previous revision, which was rebased without changes: a `Fixes:` line still seems appropriate here since this undoes the exclusion added by

    Fixes: 3e2c28437404 ("LU-12661 tests: skip sanity 817 for kernel 4.12+")

(style) also carried over from the previous revision: `Change-Id:` is still before `Signed-off-by:`.

Dropping the block entirely re-enables 817 on every kernel, not just new ones. All currently supported clients in lustre/ChangeLog are above the 4.12 threshold - RHEL 8.10 (4.18.0-553), RHEL 9.7/9.8 (5.14.0), SLES15 SP5 (5.14.21), Ubuntu 22.04 (5.15) - so the test now runs on exactly the kernels the exclusion was added for, with nothing gating it on the "newer kernels" the commit message refers to.

If the fix really is version dependent, would a bounded guard be safer, e.g.

    if (( $LINUX_VERSION_CODE < $(version_code X.Y.0) )); then
            always_except LU-12661 817
    fi

with X.Y.0 being the first kernel where nfsd releases the file?

With the exclusion gone this line runs again, and it execs immediately after the copy with no wait. exec() returns ETXTBSY while any writer holds the inode, and nfsd drops its write reference asynchronously (filecache / NFSv4 open state), not at the point `cp` returns - which is what 4fed33473ca2 described as "nfsd is not releasing the file after write".

Even on a kernel where nfsd does eventually release it, checking once immediately after the copy looks racy. Could this either unmount $tmpdir first to force the client CLOSE, or retry, e.g.

    wait_update $HOSTNAME "$DIR/nfsexp/true && echo ok" "ok" 30 ||
            error "failed to execute 'true' command"

(defect) The body only describes removing unused variables, but the osc_page.c hunk also adds a new early return when the LRU wait is interrupted. That is a behaviour change, not a variable removal, and nothing in the message accounts for it.

Could the message describe the new abort path (and why it is correct) so a reader isn't surprised by it? If the intent is really just to silence the warning, the alternative would be to drop the assignment entirely and leave the retry loop as-is.

(minor) If the abort-on-signal behaviour is intentional, this is a fix for a loop that could not be interrupted, and it would help to carry a Fixes: tag pointing at the commit that added the uninterruptible retry:

    Fixes: e8b421531c16 ("LU-6271 osc: further OSC cleanup after eviction")

(minor) This sha doesn't look like the origin of the bug. osc_lru_reserve() as added by e8b421531c16 had no retry loop at all - a single best-effort cmpxchg, returning 0 when it could not reserve, and no `rc` variable.

The `again:` loop over an unchecked l_wait_event_abortable() came in later:

    2a34dc95bd10 ("LU-12142 clio: fix hang on urgent cached pages")

and `rc` only became set-but-unused when 776e163cf542 ("LU-17180 ptlrpc: don't block ptlrpcd too long") replaced `rc = ptlrpcd_queue_work(cli->cl_writeback_work); if (rc) return 0;` with a plain schedule_work().

Since the substance of this hunk is making the wait abortable again, 2a34dc95bd10 looks like the right target:

    Fixes: 2a34dc95bd10 ("LU-12142 clio: fix hang on urgent cached pages")

Up here the `rc` is checked and the loop is aborted if the wait is interrupted.

(defect) osc_lru_reserve() returns `unsigned long`, and its value is the number of pages reserved - there is no error channel. Returning -EINTR here hands the caller (unsigned long)-4, i.e. 0xfffffffffffffffc.

osc_io_lru_reserve() stores it verbatim and discards nothing:

    oio->oi_lru_reserved = osc_lru_reserve(osc_cli(osc), npages);
    RETURN(0);

So the I/O keeps running with a bogus reservation, and two things follow.

osc_lru_alloc() takes the reserved-slot shortcut for every page of the I/O:

    if (oio->oi_lru_reserved > 0) {
            --oio->oi_lru_reserved;
            goto out;
    }

cl_lru_busy is incremented but cl_lru_left is never debited, so the LRU budget is bypassed for the rest of the I/O.

Then osc_io_rw_iter_fini() gives the remainder back:

    if (oio->oi_lru_reserved > 0)
            osc_lru_unreserve(osc_cli(osc), oio->oi_lru_reserved);

and osc_lru_unreserve() does atomic_long_add(npages, cli->cl_lru_left) - the huge unsigned value converts to a large negative long, so cl_lru_left is permanently reduced by roughly (4 + pages touched). cl_lru_left points into the mount-wide cl_cache->ccc_lru_left, so every OSC on the mount is affected, and once it goes negative the next unreserved allocation trips

    LASSERT(atomic_long_read(cli->cl_lru_left) >= 0);

in osc_lru_alloc(). The trigger is a fatal signal delivered while a writer waits for LRU slots, which is exactly the case l_wait_event_abortable() exists for.

Would `return 0;` (nothing reserved) work here instead? osc_lru_alloc() then does its own per-page reservation and already handles the abort correctly with `rc = -EINTR; break;`. Alternatively, propagate the failure through osc_io_lru_reserve()'s int return so the I/O actually stops - but that needs the callers of cio_lru_reserve to honour it.

> (defect) osc_lru_reserve() returns unsigned long, and its value is the number of pages reserved - there is no error channel. Returning -EINTR here hands the caller (unsigned long)-4, i.e. 0xfffffffffffffffc.

This look correct. I think even coverity will flag this. (-1 just to highlight this)

(style) Not a bug, but there's a stray `;` left after the comment:

    return 0; /* reserve nothing */;

(Your thoughts?) Tim, looks like cksum_counter is already being calculated on successful read. Why not (debug)print it below along with client_cksum. Instead of deleting it completely.? This can give us number of successful read's inbetween failures where it is reset to 0.

I'm skeptical it would be useful. We could get similar info (i.e. number of successful reads vs failures) by comparing llite stats to the number of error/debug messages. Plus, static variables like this aren't thread safe. The stat may not be reliable anyway.

I think in the old days the cksum_counter was used to change the `CDEBUG(D_PAGE)` into `CDEBUG(D_ERROR)` when the number of errors exceeded some threshold?  I see something similar below with `cksum_missed`.

That said, I don't think there is much value to keeping this anymore, just overhead that doesn't provide any value.

(typo) "so are current actions voliates this feature" -> "so our current actions violate this feature".

(defect) Is this claim accurate? The controls the test suite leans on hardest are debugfs-only and have no netlink or sysfs equivalent.

fail_loc, fail_val, fail_err, debug, subsystem_debug, dump_kernel (lctl dk), daemon_file and debug_marker are all created by lnet_insert_debugfs() from lnet_table in lnet/libcfs/module.c, so they exist only under /sys/kernel/debug/lnet/.

Every suite that does `$LCTL set_param fail_loc=...` or `$LCTL dk` needs those. So the suite still requires debugfs to be mounted; the patch only stops mounting it on our own.

(style) Not a bug, but cfs_try_mount_sys_kernel_debug() was the only user of <sys/vfs.h> (statfs), <linux/magic.h> (DEBUGFS_MAGIC), <sys/mount.h> (mount) and <stdbool.h> (the static bool). Worth dropping those four includes in the same patch.

(defect) conf-sanity test_5g "handle missing debugfs" still checks for this auto-mount and is not updated here:

    umount /sys/kernel/debug
    $LCTL get_param -n devices | grep -E -v "error" && error "lctl can't access debugfs data"
    grep " debugfs " /etc/mtab || error "debugfs failed to remount"

With the mount gone nothing remounts it, so the last line fails.

Worse, the test now leaves /sys/kernel/debug unmounted for the remainder of the conf-sanity run, so later subtests reading debugfs parameters, the `lctl dk` log dump on error(), and the KMEMLEAK check at /sys/kernel/debug/kmemleak all break too. Should test_5g be removed in this patch?

(defect) The netlink alternative cited in the commit message only applies to non-root callers.

lctl get_param / list_param / set_param fall back to llapi_param_display_value() / llapi_param_set_value() only under `rc == -ENOENT && getuid() != 0` (lustre/utils/lustre_param.c:1181, 1243, 1349, 1416, 1640). Root always stays on the glob path.

mount(2) needs CAP_SYS_ADMIN, so the code being deleted only ever did anything for root — non-root already got EPERM, which was silently ignored. That means the one caller class this removal changes is exactly the one with no netlink fallback: on a node where debugfs is not mounted, root now gets ENOENT for everything under /sys/kernel/debug/{lnet,lustre} and has no other route to it.

This was raised on patchset 1 (keep trying to mount for systems that can still use it, just don't print on failure) and the code is unchanged in patchset 2. Dropping only the fprintf() would keep the working case working while satisfying the secure-boot concern.

(comment) Fix lockdep bug by splitting function lod_obj_for_each_stripe() into lock/unlocked version

Could we change this to LU-18658 LU ticket number

Sure, I can do that when I refresh.

what if the layout is changing in another thread at the moment?

Sadly this function is called by both the qos and lod object code using two different mutex. Normally we do nested but I don't think nesting the mutexs in this case would be the right fit. Maybe we can use wait_on_bit() which would tell us when the layout_mutex is in use?

Nesting the mutexes is fine, as long as they're consistently nested in the same order. I can see if I can rework this - might require a larger refactor.

It would be useful to have an explanation of what the problem is, and what this patch is doing to fix it.

Is it intended for landing or just running testing?

(style)  space required before the open brace '{'

Please fix if patch is refreshed.

(minor) llite.*.uuid prints the UUID with a trailing newline, this returns it without one, so cat $MOUNT/.lustre/uuid runs straight into the shell prompt. Since this is a new user-visible interface, is it worth matching the sysfs form now?

(style) Extra parentheses around the argument - fid_is_uuid(&lli->lli_fid) reads better, and matches the PFID(&lli->lli_fid) a few lines up. ll_inode2fid(inode) would work too.

(style) Not a bug, but the tab alignment between type and name is back after being asked about on earlier patchsets - a single space is what new code uses:

    struct md_object *mdo;
    struct mdd_object *mdd_uuid;
    struct lu_fid fid = LU_UUID_FID;
    int rc;

(suggestion) Nothing ever reads m->mdd_dot_lustre_objs.mdd_uuid. mdd_obf and mdd_lpf are kept because they carry mo_dir_ops, but this one has no ops attached, so the mdo_locate() / new field / mdd_object_put() trio only pins the object in the lu_site cache. Would mdd_local_file_create() on its own be enough here?

(defect) .lustre/uuid only exists on an MDS that carries this patch, so against an older server llapi_open_by_fid() returns -ENOENT here and ASSERTF() -> DIE() exits the whole binary.

sanity.sh test_154g, which runs llapi_fid_test, is gated only on MDS1_VERSION >= 2.6.92, so on an interop run test_154g fails and test40/test41/test42 in this binary never execute.

Can this be gated on the server version - e.g. tolerate -ENOENT here, or move the check into a version-gated shell test instead?

ldiskfs/xattr.c:
 warning: sparse:no newline at end of file

ldiskfs/xattr.c:
 warning: sparse:no newline at end of file

The body only describes the ldiskfs credit reuse. Several hunks aren't accounted for:

- `lnet/libcfs/tracefile.c` drops `static` from `cfs_trace_data` - nothing else in the patch touches libcfs. Is this a debugging leftover that should be dropped?
- `osd_trans_dump_creds()` changes from `struct thandle *` to `struct osd_thandle *`, touching four call sites. That's a standalone cleanup.
- The ldiskfs patches add `EXPORT_SYMBOL(ext4_xattr_ibody_get)` alongside the one that is actually needed.
- `osd_declare_attr_set()` stops declaring `OSD_OT_XATTR_SET` unconditionally and gates it on the LMA flags. That is a behaviour change worth a sentence of its own.

Also a few wording slips: "old code don't able to account" -> "the old code cannot account", "lets reuse" -> "let's reuse".

(typo) The quoted subject doesn't match the referenced commit. `git log -1 --format=%s 278fa1fb23` gives:

    LU-12652 ldiskfs: disable credits check in 4.18

The leading `35750: ` is the Gerrit change number, not part of the subject.

I manually reverted rhel9.6/9.7 series so it points back at 5.14 patch again and the build is restored so I believe this patch is just broken.

It looks like RHEL9.6/9.7 kernel have a changes in ext4 version. sometimes it have ext4/crypto.c - sometimes not. I have tested with lastes 9.6/9.7 version but our code it looks not a last.

I checked again with 5.14.0-570.62.1.el9_6.x86_64 and 5.14.0-611.35.1.el9_7.x86_64.
It looks you needs to update a kernel versions.

ext4 never sets few EAs in a single transaction AFAIU. so it's unlikely this kind of change can make into vanilla kernel.

sure. it's sets. example of this in the patch. You lost it ?

sorry? how many EAs an user can set using clean ext4 in a single transaction?

this is

(defect) This `Index:` header has no `---`/`+++` lines and no hunks - the crypto.c change is missing from this file (the linux-6.12 copy has it). This file also never patches `fs/ext4/super.c`.

`ext4_set_context()` lives in `fs/ext4/crypto.c` on 5.16+ and in `fs/ext4/super.c` before that, and in both it calls

    ext4_xattr_set_credits(inode, len, false, &credits)

while the xattr.h hunk above changes the prototype to 6 arguments. `ldiskfs/Makefile.in` copies every `@EXT4_SRC_DIR@/*.c` for patching and only filters `crypto.c` out of the object list when `CONFIG_FS_ENCRYPTION` is unset, so with encryption enabled this is `error: too few arguments to function 'ldiskfs_xattr_set_credits'`.

That breaks every series still pointing at this file - rhel9.1 through 9.5, sles15sp4/sp5, oe2203, the 5.15/5.19 ubuntu20 series, ubuntu24 -45/-100/-106, ubuntu26, and all of 6.1/6.6/6.7/6.10/6.11/6.12/6.18/7.0-ml.

This is the same breakage that was reported earlier on rhel9.6/9.7; switching those two series to the linux-6.12 copy moved the failure rather than fixing it.

(minor) `ext4_xattr_ibody_get` doesn't seem to have any caller outside ldiskfs - grepping the tree for `ldiskfs_xattr_ibody_get` finds nothing. Is this export needed, or left over from an earlier version?

does not look like it actually applies to rhel9.6?

I wonder if this also works?

(defect) Only `6.8.0-90` was moved to the linux-6.12 copy. `ldiskfs-6.8.0-45-ubuntu24.series`, `ldiskfs-6.8.0-100-ubuntu24.series` and `ldiskfs-6.8.0-106-ubuntu24.series` still reference `linux-5.14/ext4-xattr-disable-credits-check.patch`, and those kernels have `fs/ext4/crypto.c`.

Same for the mainline series (6.6/6.12/6.18/7.0-ml) and 7.0.0-14-ubuntu26. Should the remaining series be switched too, or should the linux-5.14 copy be completed instead?

warning: sparse:symbol 'cfs_trace_data' was not declared. Should it be static?

(minor) This one is not a pure move. The old check sat inside the `--enable-efa` block and ran with EXTRA_CHECK_INCLUDE set, so with `--with-o2ib=<MOFED>` it resolved `<rdma/ib_verbs.h>` out of the external OFED tree; the ksym always reads the in-kernel header, and it now runs unconditionally.

The new answer looks like the correct one for efalnd, since lnet/klnds/efalnd/Makefile only adds `-I $(EFA_INCLUDE_PATH)` and not the OFED includes. Since it changes what gets detected on MOFED builds, worth a line in the commit message rather than listing it as a plain move.

(minor) The comment being dropped recorded two things this file doesn't: that 4.18.0-193.el8 carries the backport, and why the _rtnl variant is used instead of _rcu (the current locking is rtnl). ksymfind() has an `extra` field for exactly this kind of note, so the rationale could be carried over.

(suggestion) The test being replaced compiled `kernel_bind(NULL, (struct sockaddr_unsized *)0, 0)` under -Werror, so it proved the prototype that lnet_sock_bind()/lnet_sock_connect() and gss_svc_upcall.c actually depend on. Grepping for the struct definition only proves the type exists.

Upstream split those apart: bf33247a90d3e adds the struct to linux/socket.h, and the `kernel_bind()`/`kernel_connect()` conversions land in later commits (0e50474fa514, 85cb0757d7e1). On a tree carrying only the struct, HAVE_STRUCT_SOCKADDR_UNSIZED is defined, the `sockaddr_unsized` -> `sockaddr` fallback is skipped, and the casts in lnet/lnet/lib-socket.c hit an incompatible pointer type.

Would matching the converted prototype in include/linux/net.h be a closer equivalent?

(style) Not a bug, but collapsing the double space before `=` in the GNILND/KFILND conditionals has nothing to do with moving the lnet checks to ksym, and the commit message doesn't mention it. Drop it or split it out?

warning: sparse:symbol 'ptlrpc_pmqos_use_stats_for_duration' was not declared. Should it be static?

all these need to be static

warning: sparse:symbol 'cpus_latency_qos' was not declared. Should it be static?

and this one too.

I suspect this might not be safe either. ptlrpc_connection_addref() is just an unchecked increment. Same thing is done under rev_import_reconnect(). Export connections can be swapped and old ones dropped in target_handle_connect() (ldlm_lib.c:1604-1615. Since connections are now freed immediately when ref reaches zero, I think this becomes another potential use-after-free in both the reply handling and revert-import reconnect paths.

Do we need a reference here since connection is dereferenced below (line 915)? I'm concerned this could race with the connection being replaced in import_select_connection(). If I'm right then is potentially use-after-free.

```
checking for /tmp/ktest-out/kernel_build.x86_64/.config... noconfigure: error: 

Kernel config could not be found.
```
The userland build shouldn't depend on the kernel config. I think we'd need to skip this check somehow.

I think I know what's wrong: --disable-modules isn't propagated when we try to make the debian packages. There's a couple other tweaks we need as well. I can push an updated patch.

The kernel really doesn't like it if you attempt to build userspace components against the Linux source tree. They recommend (https://kernelnewbies.org/KernelHeaders) either building against the installed headers (i.e. what you already have implemented) or building against a version of the headers copied from Linux. This would be equivalent to building against the openSFS headers. I think this should be supported somehow. Perhaps --disable-modules could have an argument to decided whether to use the installed headers or openSFS headers?

The question is which UAPI headers to test against? Some of the autoconf test for the utilities look at the UAPI headers to decided what is supported.

Oops. I added Shaun so we can verify. Do the autoconf test use the kernel UAPI pointed to by --with-kernel. I do agree we don't need to look at the .config for the kernel. The only complex case is the dummy fscrypt kernel option that changed.

> Do the autoconf test use the kernel UAPI pointed to by --with-kernel

Yes.
The test builds work the same as any kernel module build. You can see generated examples with:

$ export PARALLEL_BUILD_OPT=debug
$ ./configure ...

Maybe you are asking about user space utilities? Those do now know anything about the kernel headers, they should build just like any other user space utility looking under /usr/include and /usr/lib

(minor) this looks like an `sptlrpc` parameter.  Also, version numbers never really explain what they are for.  It would be better to name this something more clear like `lst_force_large_nid`.

Yucky. Can't we do version handshaking instead. My next patch in this area will try the hand shaking approach.

srpc_force_v2 is for disabling v1 on v2 hosts. If it is not used, there's "handshaking": v2 is able to initiate a session with v1 (ipv4) or v2 (both ipv6 and ipv4), v1 is able to initiate an ipv4 session to v2 and v1.

Is there no mechanism that LNet (and LST) can use to automatically negotiate protocol feature support?  If not, then this should be added as part of this compatibility-breaking change.

Or do I misread this, and the `srpc_force_v2` is only used to force the new large-NID functionality on systems that don't need it?

(defect) this should have `env=FORCE_LARGE_NID=true` to run IPv6 sessions?

(minor) variable-sized `lnet_processid` should go after `nde_state`

(minor) These should all be replaced with fixed-size types (e.g. `__u32`)

(minor) should remove pointers from UAPI structs, especially in the middle

I thought I ported the SID handling to Netlink.

??

(minor) Most parameters allow other values to be written to clear it, like "0", and not just "clear".

same

...

James, the lack of ability to remove other components than the last one is a deliberate choice to prevent the creation of incomplete layouts.  There is nothing about the current approach that can’t support removing other components, it’s just a sanity restriction.

Think about file join work with 2 mirrors each with 3 components and another file with also 2 mirrors also with 3 components. Its going to be merged right. The current approach would require allocating a new larger array. Then copying items from file 1 static array and then copying items from file 2 static array. Then deleting the old array. Some slicing is needed due the 2 mirrors. Compared to Xarray its erasing the reference to pointers in file 2 xarray and adding a reference of that same pointer into file 1 Xarray. Do you feel the former approach is way better?

Honestly the biggest impact is for the file join project so the really important opinion I need is from Alexey. Alexey what do you think?

Yes, I do - it's very simple.  You allocate a larger array and write the layout to it.  Very simple.

It requires no complex data structures and no locking.  The idea that we should have references across two xarrays is very concerning - that means the layout components need reference counting then, I guess?  They do not today.

There is no particular impact on file join - it is just as easy to do file join via layout copy as any other operation.

James, file layout join/migrate/mirror happen _maybe_ only once or twice in the lifetime of a file, so it doesn't make sense to add a complex data structure and overhead to optimize a 0.00001% usage case.  That is doubly a problem if it means many small allocations and/or additional locking needs to be added to handle this extremely rare use case.

never kill a subsystem check. I will make developers work harder.

I'm hoping to use the tracepoints to enable/disable subsystem logging. There's a tracepoint enabled macro I could use to restore this check.

Does this add 256 bytes of stack usage to every function that calls CDEBUG()?  That could be pretty bad in some call chains.

Yes. This is super hack-y. There's definitely a better way to do this.

you are really want to kill a performance? I think yes.

This entire __ltrace_printk() macro sucks. I need to rewrite it entirely. I have an idea that should make this macro much better.

There are tunable parameters for the CDEBUG rate limiting, why not use them here?

I'll probably have to, since old kernels don't like '#include <linux/ratelimit_types.h>'. I was hoping that these macro would be simpler.

This will also need to be fixed.

I think this is fine? This is just redefining this function to be always false is debugging is disabled.

Sorry, I was thinking this was the no-op version of the actual libcfs_debug_msg() which had been removed.  Carry on.

Doesn't build on aarch64 and ppc. Needs to go in debug.c probably. These are pretty huge functions anyway.

Same.

Same.

I am really suspicious about this patch.
current master-next on boilpot started to get strange use after free problems which seem to be accessing OBD device that's already freed.

Four distinct trace so far: 
https://knox.linuxhacker.ru/crashdb_ui_external.py.cgi?newid=73710

https://knox.linuxhacker.ru/crashdb_ui_external.py.cgi?newid=73714
https://knox.linuxhacker.ru/crashdb_ui_external.py.cgi?newid=73724
https://knox.linuxhacker.ru/crashdb_ui_external.py.cgi?newid=73725

I am going to omit it next time over and we'll see how it works. Not setting -1 just yet

dropping this patch resulted in total disappearance of the problems reported in the latest master-next over the last week

Odd. I wonder if this is exposing an issue introduced in a previous patch somehow?

would be nice to have some examples of the new names, or before/after

Do the extra embedded '-' separators cause parsing issues later on?

%pU is not correct here as sbi->ll_sb_uuid.uuid is a max 40 byte string.

It is safe to require the '-mdc-' naming? Probably but I have not looked to be positive.

Extra '-' separators ... do the confuse a parser somewhere?

That is my concern as well if the format of the parameters is suddenly getting much longer and having extra dashes in them.  In the past we had to deal with fsname that contained multiple '-' in them, so some code is using "strrchr()" to find the fsname from the end of the instance name.

The current parameters look like:
 
# lctl list_param llite.*.max*
llite.myth-ffff909824e7f800.max_easize
llite.myth-ffff909824e7f800.max_read_ahead_async_active
llite.myth-ffff909824e7f800.max_read_ahead_mb
llite.myth-ffff909824e7f800.max_read_ahead_per_file_mb
llite.myth-ffff909824e7f800.max_read_ahead_whole_mb
llite.myth-ffff909824e7f800.max_cached_mb

but with this patch they would become:
 
# lctl list_param llite.*.max*
llite.myth-f1da1f0c-f89b-473e-b604-8c265d1c1ecd.max_easize
llite.myth-f1da1f0c-f89b-473e-b604-8c265d1c1ecd.max_read_ahead_async_active
llite.myth-f1da1f0c-f89b-473e-b604-8c265d1c1ecd.max_read_ahead_mb
llite.myth-f1da1f0c-f89b-473e-b604-8c265d1c1ecd.max_read_ahead_per_file_mb
llite.myth-f1da1f0c-f89b-473e-b604-8c265d1c1ecd.max_read_ahead_whole_mb
llite.myth-f1da1f0c-f89b-473e-b604-8c265d1c1ecd.max_cached_mb

which not only makes them longer and harder to parse, but also much less readable.  If the main concern here is ASLR then we can avoid printing
the raw pointer in the parameter name and work out something else.

For example, the kernel could save a random 64-bit number at startup time and XOR with that, and if it needs the original pointer back it can XOR again.

you probably wants to update this comment to be inline with the syntax.

The check could be done before trying the copy