James Simmons jsimmons · review.whamcloud.com

(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?

This isn't a bug, but the size is hard-coded with the constant only in a comment. CHANGELOG_USER_NAMELEN_FULL is available from uapi/linux/lustre/lustre_idl.h; using it keeps the strscpy() in changelog_collect_user_cb() from silently truncating if the constant ever grows. (lustre_user.h has the same literal, but that one can't include lustre_idl.h - this header can.)

(minor) Nothing ever sends on this multicast group - there is no genlmsg_multicast()/genl_notify() call in mdd. Registering it publishes a "changelog" group name that userspace can subscribe to and then never hear anything on. Is it meant to be wired up in a later patch, or can it be dropped until there is a notification to send?

(defect) What keeps ctxt->loc_handle alive while this runs? The llog_ctxt reference from llog_get_context() pins the context, but not the catalog handle.

On umount, mdt_stack_fini() sends LCFG_CLEANUP down the stack, and mdd_process_config() -> mdd_device_shutdown() -> mdd_changelog_fini() then does:

    ctxt = llog_get_context(obd, LLOG_CHANGELOG_USER_ORIG_CTXT);
    llog_cat_close(env, ctxt->loc_handle);  /* NULLs loc_handle, llog_close() frees it */
    llog_cleanup(env, ctxt);                /* only now waits for our ctxt ref */

The MDD obd still has OBDF_SET_UP and no OBDF_STOPPING at that point, and those flags are only checked once before this long, sleeping catalog walk anyway. A dump in progress keeps walking a freed cat_llh. A dump that arrives after llog_cat_close() but before llog_cleanup() finishes gets ctxt->loc_handle == NULL, and llog_cat_process_or_fork() dereferences cat_llh->lgh_hdr on its first line.

The debugfs changelog_users reader avoids this because mdd_procfs_fini() runs before mdd_device_shutdown(). Here, would it work to hold mdd->mdd_changelog_mutex around the llog_get_context()/llog_cat_process() pair and skip the device unless CLM_INIT_DONE is set under the mutex? mdd_changelog_fini() takes the same mutex and resets mc_flags to CLM_CLEANUP_DONE.

(suggestion) Should this op have .flags = GENL_ADMIN_PERM? As written, any local user in init_net can dump it.

The same data under mdd.*.changelog_users is in debugfs, which only root can read. The LNet family's show dumps (LNET_CMD_NETS, PEERS, ROUTES, STATS, ...) are all GENL_ADMIN_PERM.

Each dump also runs a synchronous llog_cat_process() over every MDT's users catalog from .start. For a family without parallel_ops, that runs under genl_mutex. So an unprivileged caller can repeatedly drive disk I/O on every MDT while blocking all other genetlink families, and can also hit the umount window noted above.

Should this check CLM_INIT_DONE before using `ctxt->loc_handle`? Every other changelog llog consumer in mdd does - mdd_changelog_users_seq_show()@lustre/mdd/mdd_lproc.c:209 and mdd_changelog_size_seq_show()@:247 both return -ENXIO when the flag is clear, and mdd_changelog_recalc_mask() takes mdd_changelog_mutex.

llog_setup() for LLOG_CHANGELOG_USER_ORIG_CTXT happens at mdd_device.c:556 but loc_handle is only assigned by the llog_open_create() that follows at :567, so for the whole duration of that create (and llog_init_handle() after it) the context is registered with loc_handle still NULL. mdd_prepare() runs from server_start_targets() long after class_setup() has set OBDF_SET_UP, so the obd is already visible to this loop during that window.

llog_cat_process() -> llog_cat_process_or_fork() dereferences `cat_llh->lgh_hdr` on its first line, so a dump landing in that window is a NULL dereference.

The teardown side has the same exposure: mdd_changelog_fini() does llog_cat_close(env, ctxt->loc_handle) then llog_cleanup(env, ctxt) under mdd_changelog_mutex, which this path does not take.

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?

(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?

(minor) Nothing in the patch makes an iter_iov_len() available: what is added is compat_iter_iov_len(), under a guard that makes it the only thing vvp_io.c ever calls. Could the body also say that vvp_mmap_locks() gains an iter_is_ubuf() special case, since that is a behaviour change rather than a rename?

(defect) `len` is not declared - the declaration above is `ssize_t nr`. With -Werror this test fails to compile on every kernel, so HAVE_ITER_IOV_LEN is never defined and the result is silently "no".

iter_iov_len() returns size_t, so this probably wants the same form as the neighbouring LC_SRC_HAVE_IOVEC_WITH_IOV_MEMBER test:

    size_t len __attribute__ ((unused));

(typo) `384492c48e6e` does not resolve; the commit is 384492c48e6a ("net: devmem: support single IOV with sendmsg").

(defect) Once the autoconf test compiles, a 6.3+ kernel defines both HAVE_ITER_IOV_LEN and HAVE_USER_BACKED_ITER, so this helper is not compiled - but vvp_mmap_locks() calls compat_iter_iov_len() unconditionally, so vvp_io.c stops building. Right now the only thing preventing that is the broken test above; fixing either one alone breaks the build.

Would it be simpler to follow the iter_iov_addr() pattern just above and supply the upstream name, `#ifndef HAVE_ITER_IOV_LEN` / `static inline size_t iter_iov_len(...)`, and call iter_iov_len() from vvp_io.c? Then the call site really does match upstream, and there is no Lustre-only name to keep in sync.

(defect) The advance expression now uses `bytes`, which the loop body modifies: `bytes += addr & ~PAGE_MASK` below, and `bytes -= vma->vm_end - addr` in the inner while loop. The old code advanced by `iov.iov_len` from the iovec copy, which the body never touched (it worked on `bytes` instead).

So for an unaligned multi-segment iovec the iterator is advanced by `iov_len + (addr & ~PAGE_MASK)` per segment. The following segment is then scanned from a bogus offset, and a segment shorter than the overshoot is skipped completely, so no mmap lock is taken for it.

Keeping the segment length in its own variable, used only by iov_iter_advance(), would preserve the old behaviour.

(minor) Can this branch be reached? The early return above returns 0 unless the iter is ITER_IOVEC or ITER_KVEC, and iter_is_iovec() is false for ITER_UBUF, so a ubuf iter never gets to the loop. Either the branch is dead code, or the gate above is what needs relaxing - the ubuf iter missing its mmap locks entirely looks like the pre-existing problem here.

(minor) compat_iter_iov_len() returns `iov_len - iov_offset`, while the iov_iter_iovec() being replaced returned `min(count, iov_len - iov_offset)`.

The clamp is reachable: vvp_io_rw_lock() calls vvp_io_update_iov() -> iov_iter_truncate(vui_iter, crw_bytes) immediately before this, and lov_io_rw_iter_init() has already clipped crw_bytes to the current stripe chunk. A 4M read into a single 4M buffer on a 1M-striped file now asks policy_from_vma() for a 4M extent instead of 1M, and that extent can run past the end of the vma.

Clamping with the remaining iov_iter_count() would keep the old extent.

(defect?) I doubt the UUID should have a linefeed at the end?

(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")

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.

(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) 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) Nothing caps cb->min_dump_alloc and there is no error return when the computed total is unreasonable - it just accumulates. This is the objection from patchset 1 ("allocating an 4G in kernel - very very bad style. Please use a multipart messages."), which was answered with "I do agree peer needs to still handle overflows since peers unlike NI (lnet_interfaces_max) is not bounded". Patchset 5 still has no bound.

The sibling handler in this file does bound it, 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;
    }

At version 4/5 this charges roughly 945 bytes for a single-NI peer, so a few thousand peers already asks for a multi-megabyte buffer on every `lnetctl peer show`. Could the same guard, or a cap on how many peers one dump covers, be applied here?

(style) This isn't a bug, but `obd:` isn't a component tag used elsewhere in the tree. The change is mostly in obdclass (plus ldlm), so `obdclass:` would fit better.

(typo) "This is problem" -> "This is a problem", and "is to small" -> "is too small".

The message describes only the MGC uuid change. Several behavior changes in the diff aren't mentioned:

- import_set_conn() now parses NID-shaped uuids directly and matches existing connections by `c_peer` NID instead of `oic_uuid`. This affects every OSC/MDC/OSP/LWP import, not just the MGC.
- lustre_start_simple() now ignores `s2` for every caller.
- lustre_add_mgc_failnodes() now creates one connection per failover NID instead of one per failover node, and maps every failover NID to the MGC's uuid.

Were these intended? If so, could the message explain why they are safe?

(minor) ptlrpc_connection_get() returns NULL (not an ERR_PTR) when allocation fails, and only ptlrpc_uuid_to_connection() converts that to ERR_PTR(-ENOENT). On this branch the IS_ERR() check below misses NULL, so `oic_conn` can be stored as NULL and dereferenced later in import_select_connection(). It's an ENOMEM-only path, but the same `c ? c : ERR_PTR(-ENOMEM)` conversion would keep it consistent.

(defect) Matching on the peer NID string instead of `oic_uuid` breaks lookups whose uuid isn't the peer NID string. The MGC's primary connection is the clearest case: its uuid is now the random MGC uuid, so it never matches here. import_set_conn_priority() then returns -ENOENT, and ptlrpc_recover_import(imp, uuid) fails, e.g. `lctl set_param mgc.*.import=connection=<uuid>`.

It also fails for NID-shaped uuids when LNetPrimaryNID() replaced the peer NID with a different primary in ptlrpc_connection_get(). Repeated adds then create duplicate connections, and the IR reconnect in mgc_apply_recover_logs() -> ptlrpc_recover_import(imp, uuid.uuid, 1) can't find the connection.

client_import_del_conn() and client_import_add_nids_to_conn() still key on `oic_uuid`, so these lookups are now inconsistent. Also, `strncmp(..., sizeof(uuid->uuid))` is effectively strcmp() for a NUL-terminated 40-byte uuid, so the truncated-uuid case described in the comment above won't match either.

(defect) `node` is now the MGC's own obd_uuid, the same key lustre_start_mgc() used for the primary MGS NIDs. So every failover MGS NID is added to the same uuid_nid_data entry as the primary MGS NIDs. class_add_uuid() then calls LNetAddPeer() with the whole entry, telling LNet that the primary and failover MGS nodes are one multi-rail peer. With the default lock_prim_nid=1, LNetAddPeer() can call lnet_peer_force_merge() and merge an existing failover-node peer into the primary MGS peer.

The old code used a separate uuid per failover node for exactly this reason. Two other side effects:

- The primary connection's ptlrpc_uuid_to_peer() can now pick a failover node's NID.
- The entry is capped at MTI_NIDS_MAX (32) NIDs across all MGS nodes, and on reuse of an existing MGC, across every mount's mgsnode list.

Should the failover NIDs keep a per-node key that differs from the primary's?

(minor) class_nid2uuid() doesn't truncate. It returns -EOVERFLOW when the NID string doesn't fit in UUID_MAX, and the `break` then skips this NID's connection and the rest of the current node. So a failover MGS with a long IPv6 NID still gets no connection, which is the case the commit message says is fixed. Separately, the "can't add failover peer" CWARN below prints `node` (the MGC uuid) where the failover NID was probably intended.

(defect) This replaces `s2` with `uuid` for every lustre_start_simple() caller, not only lustre_start_mgc(). The OSD start in tgt_mount.c passes the mount flags in `s2`:

    snprintf(flagstr, sizeof(flagstr), "%lu:%u", mflags, lmd_flags);
    lustre_start_simple(lsi->lsi_osd_obdname, lsi->lsi_osd_type,
                        lsi->lsi_osd_uuid, lmd->lmd_dev, flagstr, ...);

Now buf 2 holds the OSD uuid string instead. osd-ldiskfs osd_mount() runs `sscanf(str, "%lu:%lu", &s_flags, &lmd_flags)` on it, the parse fails, and both stay 0. So MS_RDONLY, LMD_FLG_DEV_RDONLY and LMD_FLG_NOSCRUB are silently dropped: an `-o ro` target mounts read-write, and `noscrub` is ignored. osd-zfs does `strstr(str, ":")` and loses rdonly_dev/noscrub the same way. That would explain the sanity-scrub failures seen on every patchset, since that suite mounts with `noscrub`.

It would be safer to pass the uuid as `s2` from lustre_start_mgc() only (and keep the kernel-doc for `s2` accurate).

(defect) Every new MGC now registers its NIDs under a fresh random uuid, but nothing calls class_del_uuid() for that key when the MGC is stopped (lustre_stop_mgc() doesn't touch the uuid table). Before this patch the key was the NID string, so repeated mounts reused one entry. Now each mount/umount cycle (autofs, for example) leaves another uuid_nid_data (~700 bytes) on g_uuid_list until module unload. lustre_uuid_to_peer() and class_add_uuid() scan that list linearly, so they get slower as it grows. Should the MGC remove its entry on cleanup?

(minor) If the patch is refreshed, consider saying concretely what was wrong with the old code. ll_mnt.mnt was set to current->fs->root.mnt, which is the mounting process's root vfsmount, not the Lustre mount, and no reference was held on it. So may_umount() was testing the wrong mount (in practice always busy, so every umount -f slept the full 10s), and it could touch a freed vfsmount if that root went away, e.g. when a container exits. The body also doesn't mention the test-framework.sh change that bind-mounts the helper for build-tree testing.

(minor) This line can never fire. A successful umount already leaves via "&& exit 0" on line 10, so by this point rc is always non-zero. If the patch is refreshed, it can be dropped.

(trivial) (( .... ))

(defect) This is not your code. But worth fixing. 'continue' is wrong here.

```
[[ -f "$sbin_mount" ]] || touch "$sbin_mount" || true	
[[ -f "$sbin_umount" ]] || touch "$sbin_umount" || true		

```

-1 only for this.

(defect) A bind mount needs its target to exist, but nothing creates $sbin_umount first. The mount.lustre bind just above writes a "STUB MARK" file for exactly this reason. On a build-tree node that does not have the lustre RPM installed, /sbin/umount.lustre is missing, so the mount fails with "mount point does not exist" and error() aborts load_modules_local(). That means llmount.sh and every suite fail at setup. Autotest runs from installed RPMs, where $LUSTRE/utils/mount.lustre does not exist, so it never takes this branch and won't catch it.

Could this create a stub the same way sbin_mount does, or only bind when [[ -e $sbin_umount ]]?

(typo) "bund" -> "bind".

(trivial) can be fixed.

(minor) This does not undo what load_modules_local() set up. When the bind succeeded, /sbin/umount.lustre is still a mount point here, so rm fails with EBUSY and the bind stays in place. The next load_modules_local() call then finds mount.lustre unbound and stacks a second bind on top of it. Should this follow the sbin_mount pattern just above? That is: umount $sbin_umount if it appears in /proc/mounts, then remove it only if it is a stub.

(typo) "Do object"?

(style) The subject has no grammatical subject - "allow to be started" leaves out what is being started. Something like "allow components to start at offset zero" reads better, and the trailing period is not usual for Lustre subjects.

More importantly, neither the subject nor the body names anything that this patch actually adds: `LOV_PATTERN_ZEROSTART`, `LLAPI_LAYOUT_ZEROSTART`, the `zerostart` layout type, or `lfs setstripe --zerostart`. That makes the change hard to find later with `git log --grep`.

(typo) "Do object" - DoM?

This was raised on an earlier patchset and still reads the same way here.

Several hunks are not accounted for by the description; could they be explained, or split out?

- osc_cache.c: the LASSERTF message gains `io %px`, which looks like debug instrumentation for this work rather than part of the feature.
- lfs.c `case 'C'`: the guard changes from `lsa_pattern == LLAPI_LAYOUT_MDT` to `(lsa_pattern & LLAPI_LAYOUT_RAID0) == 0`, a behaviour change to `-C|--overstripe-count` validation.
- lov_io.c `lov_io_layout_at()` and lov_offset.c `lov_stripe_offset()`: `return` converted to `RETURN()`.
- lov_offset.c: blank line after the SPDX tag removed.
- sanity-pfl.sh test_16b: two blank lines removed.
- lov_cl_internal.h: `lov_foreach_io_layout()` split into two macros.

It would also help to state the interop story here: `LOV_PATTERN_ZEROSTART` is a new on-disk/on-wire pattern bit, so a client older than this patch fails `lov_pattern_supported()` on such a layout and cannot open the file, and an older MDS rejects the setstripe. Is a version/feature gate expected, or is "both ends must be new" the intended contract?

(typo) "Do object" -> "DoM object". This was raised on patchset 30 and is still here.

The body describes the motivation well but never names anything the patch adds, so this won't be findable later with `git log --grep`. Could it mention `--zerostart`, `LOV_PATTERN_ZEROSTART`/`LLAPI_LAYOUT_ZEROSTART` and `lsme_is_zerostart()` explicitly?

It also doesn't cover the largest kernel-side piece: the rework of `lov_io_iter_init()` that pulls the last stripe of the preceding component into a truncate so the KMS survives when a ZEROSTART component is truncated to its own start. That deserves a paragraph.

A few hunks are not explained at all and read as leftovers - should they be split out, or described?

- `lustre/osc/osc_cache.c`: the `osc_extent_find()` LASSERTF() gains the `io %px` argument; it looks like a debugging aid rather than part of the feature.
- `lustre/utils/lfs.c`: the `-C|--overstripe-count` check changes from `== LLAPI_LAYOUT_MDT` to `& LLAPI_LAYOUT_RAID0` and the message text changes. That is a user-visible behaviour change and isn't needed for `--zerostart` to work.
- `lustre/tests/sanity-dom.sh`: dropping the explicit `mkdir -p $MOUNT2` / `mount_client $MOUNT2` is unrelated to this feature (it is redundant with `MOUNT_2=yes` + `check_and_setup_lustre`, so it looks correct, just out of scope).

(typo) "Do object" was pointed out on patch set 30 and is still here. Should it be "DoM object"?

(style) The body explains the motivation but not what the patch actually changes, so it is hard to find later with git log. If the patch is refreshed, could it name:

- the new `LOV_PATTERN_ZEROSTART` / `LLAPI_LAYOUT_ZEROSTART` pattern bit
- the `lfs setstripe --zerostart` and `-L raid0,zerostart` options
- the truncate change in lov_io_iter_init(), which pulls the last stripe of the preceding component into a truncate landing on a ZEROSTART component start
- the new sanity-zero.sh suite

Also, the subject should not end with a period.

(minor) There is no `lfs join` command or `lfs-join.1` man page in the tree - the join-file feature was removed, as the commit message itself notes. This cross-reference will be a dangling link; could the sentence just describe the property directly?

(typo) "below" - the `--zerostart` description is above this one in the page.

(minor) EXAMPLES doesn't gain a `--zerostart` case. A short composite example, e.g. a DoM or plain first component followed by a zerostart one, would show how the option is meant to be combined - the option description alone leaves that to the reader.

(typo) "see --zerostart below": the --zerostart option is described above this point in the page.

The body says this handles the replacement of fscrypt_inherit_context(), but all three call sites are still there and unguarded:

    lustre/llite/namei.c:1230
    lustre/llite/namei.c:1851
    lustre/llite/dir.c:537

llcrypt_set_context() is only wired up in one place (ll_new_node_finish() for symlinks); everywhere else the new call is commented out. Should the message say the conversion is partial?

A few hunks aren't accounted for by the description - can they be explained, or split out?

    - config/Makefile.exports.in + lnet/libcfs/Makefile + lustre_compat/fs/crypto/Makefile: switching the llcrypt build gate from a config.h grep to an exported make variable
    - lustre/llite/statahead.c: ll_xattr_cache_insert() -> ll_set_encflags()
    - lustre/llite/namei.c: dropping the -ERANGE retry loop around get_context()
    - lustre/llite/namei.c: dropping the `encrypt && (open_flags & O_CREAT) && d_inode(dentry)` branch that used to call ll_set_encflags() with preload=true
    - the CDEBUG text changes ("it_finish server returned ...", "statahead server returned ...")

(defect) Two hunks aren't accounted for by the description:

- a new UAPI ioctl, LL_IOC_GET_ENCRYPTION_NONCE / llcrypt_ioctl_get_nonce()
- the folio signature change of llcrypt_encrypt_pagecache_blocks(), llcrypt_decrypt_pagecache_blocks() and llcrypt_finalize_bounce_page(), plus the new lustre_compat/linux/folio.h include

Both are independently landable. Should they be split out, or at least named in the body?

(defect) These now hand a folio to the in-kernel helpers, but on 5.15 - the kernel this patch is targeting - fscrypt_encrypt_pagecache_blocks() and fscrypt_decrypt_pagecache_blocks() take a `struct page *`, and fscrypt_finalize_bounce_page() at line 77 takes a `struct page **` (osc_request.c passes `foliop`). The folio conversions are much more recent.

There is no HAVE_* guard or compat shim here, so the in-kernel build looks like it would still fail on 5.15 with incompatible-pointer errors. Should a config/*.m4 test cover the page-vs-folio variants?

(defect) FS_IOC_GET_ENCRYPTION_NONCE and fscrypt_ioctl_get_nonce() only appeared in v5.7, and the mapping at line 96 is unguarded. Since the commit message says in-kernel fscrypt is supported down to 5.8-era kernels today, is a HAVE_FSCRYPT_IOCTL_GET_NONCE test needed before this can be mapped unconditionally?

(defect) This `#else` arm also covers the `--disable-crypto` build (no HAVE_LUSTRE_CRYPTO, no CONFIG_LL_ENCRYPTION), and llcrypt_context_for_new_inode() has no declaration there -- it is declared only at line 67 for the native branch, and in include/lustre_compat/linux/llcrypt.h only inside `#ifdef CONFIG_LL_ENCRYPTION`. dir.c and namei.c call it unguarded, so the build stops on an implicit declaration.

llcrypt_prepare_new_inode() has the matching problem on the link side: it is declared here, but keysetup.c only defines it under `#if !defined(HAVE_FSCRYPT_SET_CONTEXT) || defined(CONFIG_LL_ENCRYPTION)`. On a kernel where HAVE_FSCRYPT_SET_CONTEXT is set (the autoconf test at config/lustre-core.m4:787 runs regardless of --enable-crypto), neither condition holds and nothing defines it. Should the no-crypto section of llcrypt.h grow stubs for both?

(style) Not a bug, but this `len` shadows the `size_t len` parameter that is handed to md_create() further down, which makes the block harder to follow. If the patch is refreshed, something like `ctxsize` would read better.

Since `encrypt` is forced to true a few lines up whenever IS_ENCRYPTED(dir) and open_flags is non-zero, the value passed in by ll_lookup_it_finish() is now only meaningful for the open_flags == 0 case. Was the parameter meant to stay?

(defect) This runs on every lookup, not just creates - ll_lookup_it_finish() is reached from plain ll_lookup_nd() too.

Two consequences for an encrypted parent:

- llcrypt_prepare_new_inode() returns -ENOKEY when the master key isn't loaded, so `ls` / `stat` of anything inside an encrypted directory without the key now fails instead of showing no-key names.
- with the key loaded it ends in llcrypt_get_encryption_info(inode), i.e. a getxattr RPC for the child's context, issued *before* the server-supplied context is stored. That is exactly what the comment three lines below says this code is arranged to avoid ("save an extra getxattr and avoid deadlock").

Was this meant to be gated on the create disposition?

(defect) GOTO(free_fake_inode, ...) lands on a label that is still inside this `if` block, and nothing after the label rechecks the error - control just falls through to the security-context setup, ll_intent_lock() and ll_lookup_it_finish(), and `retval` is overwritten by the success assignment at the end.

So a failure from llcrypt_prepare_new_inode() or llcrypt_context_for_new_inode() is silently discarded, and the OBD_ALLOC failure at the -ENOMEM goto continues with op_file_encctx == NULL, creating the file in the encrypted directory with no encryption context at all.

ll_dir_setdirstripe() and ll_new_node_prepare() both put an `if (err) GOTO(out..., err)` after the label; that seems to be missing here.

(defect) The volatile branch above takes a reference via volatile_ref_file() and the old code released it with fput() once the context had been fetched. Now the only fput() left is on the !ref_inode error path, so the reference leaks on every volatile-file create in an encrypted directory.

The fake-inode branch also skips llcrypt_put_encryption_info(ref_inode) before destroy_inode(), so the llcrypt_info that llcrypt_prepare_new_inode() attached leaks too - dir.c:539 and namei.c:1843 both call it.

(defect) Swapping ll_xattr_cache_insert() for llcrypt_set_context() ties caching the server-supplied context to the *parent's* policy, which it did not depend on before.

For a directory that has its own policy but sits under an unencrypted parent, mdt_pack_encctx_in_reply() still sets OBD_MD_ENCCTX because the child carries LUSTRE_ENCRYPT_FL. Here `parent` is not encrypted, so llcrypt_prepare_new_inode() above returns 0 and leaves `encrypt` false, and then:

    llcrypt_set_context() -> llcrypt_inherit_context(parent, ...)
      -> llcrypt_get_encryption_info(parent) -> get_context() -> -ENODATA

(native path: fscrypt_set_context() hits WARN_ON_ONCE(!ci) and returns -ENOKEY).

The CWARN reads as non-fatal, but `rc` is no longer cleared on the way out -- the old `GOTO(out, rc = 0)` before the `out:` label is gone and `if (encrypt)` does not run to overwrite it, so ll_lookup_it_finish() returns the error and ll_lookup_it() turns it into ERR_PTR(). Does that make `ls`/`stat` of such a directory fail?

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.

It would be better to have a table declaring the stats, then have a loop iterating over all entries in the table to call lpricfs_counter_init() for each entry.

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?

(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"

(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.

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?

```
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

[ 2231.421259] ------------[ cut here ]------------
[ 2231.422249] kernel BUG at fs/jbd2/transaction.c:2148!
[ 2231.423289] invalid opcode: 0000 [#1] SMP PTI
[ 2231.424158] CPU: 0 PID: 129424 Comm: ldlm_bl_03 Kdump: loaded Tainted: G           OE     -------- -  - 4.18.0-553.85.1.el8_10.x86_64 #1
[ 2231.426385] Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
[ 2231.427473] RIP: 0010:jbd2_journal_try_to_free_buffers+0xfa/0x100 [jbd2]
[ 2231.428770] Code: 00 75 f4 eb a7 5b 4c 89 ef 5d 41 5c 41 5d 41 5e 41 5f e9 f9 7f ef de 5b 31 c0 5d 41 5c 41 5d 41 5e 41 5f c3 cc cc cc cc 0f 0b <0f> 0b 0f 1f 40 00 66 66 66 66 90 41 56 41 55 41 54 55 53 4c 8b 2f
[ 2231.432135] RSP: 0018:ffffa4c40584b9e8 EFLAGS: 00010246
[ 2231.433142] RAX: 000fffffc001206d RBX: ffffedd9006f92c0 RCX: 0000000000000000
[ 2231.434497] RDX: 0000000000000000 RSI: ffffedd9006f92c0 RDI: ffff9313873d5000
[ 2231.435845] RBP: ffff9312ac220b80 R08: 0000000000000000 R09: ffffa4c40584b9b8
[ 2231.437180] R10: ffffffffffffffff R11: 000000000000003f R12: 0000000000000000
[ 2231.438513] R13: ffffedd9006f92c0 R14: 0000000000000000 R15: 0000000000000000
[ 2231.439846] FS:  0000000000000000(0000) GS:ffff9313bbc00000(0000) knlGS:0000000000000000
[ 2231.441346] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 2231.442443] CR2: 00005654730b7f44 CR3: 000000001ee10005 CR4: 00000000000606f0
[ 2231.443776] Call Trace:
[ 2231.444312]  ? __die_body+0x1a/0x60
[ 2231.445060]  ? die+0x2a/0x50
[ 2231.445662]  ? do_trap+0xe7/0x110
[ 2231.446327]  ? jbd2_journal_try_to_free_buffers+0xfa/0x100 [jbd2]
[ 2231.447503]  ? do_invalid_op+0x36/0x40
[ 2231.448243]  ? jbd2_journal_try_to_free_buffers+0xfa/0x100 [jbd2]
[ 2231.449410]  ? invalid_op+0x14/0x20
[ 2231.450127]  ? jbd2_journal_try_to_free_buffers+0xfa/0x100 [jbd2]
[ 2231.451286]  invalidate_inode_page+0x72/0x90
[ 2231.452153]  __invalidate_mapping_pages+0x156/0x3c0
[ 2231.453100]  ? vsnprintf+0x340/0x520
[ 2231.453821]  ? snprintf+0x49/0x70
[ 2231.454488]  pcc_inode_put+0x118/0x140 [lustre]
[ 2231.455579]  pcc_layout_invalidate+0x12a/0x550 [lustre]
[ 2231.456629]  vvp_conf_set+0xbc/0xe0 [lustre]
[ 2231.457505]  cl_conf_set+0x62/0x140 [obdclass]
[ 2231.458613]  ll_layout_conf+0x14d/0x420 [lustre]
[ 2231.459553]  ? ll_lock_cancel_bits+0x484/0x9b0 [lustre]
[ 2231.460612]  ll_lock_cancel_bits+0x484/0x9b0 [lustre]
[ 2231.461630]  ll_md_blocking_ast+0x2d9/0x450 [lustre]
[ 2231.462624]  ldlm_cancel_callback+0x7b/0x260 [ptlrpc]
[ 2231.463897]  ldlm_cli_cancel_local+0xa3/0x490 [ptlrpc]
[ 2231.464949]  ldlm_cli_cancel_list_local+0x113/0x330 [ptlrpc]
[ 2231.466082]  ldlm_bl_thread_main+0x74b/0x8b0 [ptlrpc]
[ 2231.467122]  ? finish_wait+0x80/0x80
[ 2231.467875]  ? ldlm_handle_bl_callback+0x400/0x400 [ptlrpc]
[ 2231.468993]  kthread+0x134/0x150
[ 2231.469678]  ? set_kthread_struct+0x50/0x50
[ 2231.470499]  ret_from_fork+0x35/0x40
[ 2231.471222] Modules linked in: loop mgc(OE) lustre(OE) mdc(OE) lov(OE) osc(OE) lmv(OE) fid(OE) fld(OE) ksocklnd(OE) ptlrpc(OE) obdclass(OE) lnet(OE) libcfs(OE) rpcsec_gss_krb5 auth_rpcgss nfsv4 dns_resolver nfs lockd grace fscache intel_rapl_msr intel_rapl_common crct10dif_pclmul crc32_pclmul ghash_clmulni_intel joydev pcspkr i2c_piix4 virtio_balloon sunrpc ext4 mbcache jbd2 ata_generic ata_piix libata virtio_net crc32c_intel serio_raw net_failover virtio_blk failover

Sure

It would be better to say something like:

The lnetctl tool supports gateways specified with a nidrange syntax.
For example:
gateway: 10.151.0.[1,2,3,4,5,6,7,8]@o2ib

Is strncmp needed vs. strcmp? We use strcmp everywhere else.

I would prefer you remove "route" from the cmd_mappings array (line 4363), and then move this conditional block to same level as the ip2nets conditional:

```
  if (mapping) {
    ...
  } else if (!strcmp(scalar_value, "route")) {
    ...
  } else if (!strcmp(scalar_value, "ip2nets")) {
```

The cmd_mappings array should only be for items that don't require a custom handler.