Gerrit Review Dashboard

(minor) Can a cached read really mark a DIO folio uptodate? A PagePrivate2 folio is allocated by folio_alloc() in osd_get_page() and never inserted into inode->i_mapping, so get_folio_create()/get_folio_lock() can never return it, and osd_get_page() calls folio_clear_uptodate() on every DIO folio just before handing it out.

The only other places osd-ldiskfs sets PG_uptodate are dio_complete_routine() and osd_write_commit(), both of which run in an earlier RPC and are undone by that folio_clear_uptodate(). That leaves the OBD_FAIL_OST_FAKE_RW SetPageUptodate() two lines above the cache-hit test as the only in-tree way to reach this branch with a DIO folio.

If there is another path, it would be good to name it here, because in that case skipping the read is itself wrong (see the comment on osd_io.c).

We can just remove that sentence instead...

(minor) This reproducer does not seem to match the mechanism above. Neither sanity test_155g nor test_63a sets fail_loc=0x238, and read_cache_enable=0 on its own only selects the DIO path - it does not make a DIO folio uptodate. LU-19778 is filed as "sanity test_428: osd_write_commit() ASSERTION( PageLocked(lnb[i].lnb_page) ) failed".

A sequence that does hold together, and matches the ticket:

    test_399b (test_fake_rw read) truncates $tfile to up to 1GB.
    fsize > od_readcache_max_filesize (default totalram/64), so
    osd_bufs_get() picks cache=false and every lnb gets a DIO folio.
    fail_loc=0x238 then makes every folio uptodate, so all of them take
    the cache-hit branch and get unlocked; iobuf->dr_npages stays 0 so
    the guarded early-release loop never runs.
    test_428 later writes 128MB files (also over the filesize limit) on
    the same service thread and trips the write-side LASSERT.

That also explains why this survived since LU-13309: it needs fail_loc=0x238 *and* the non-cached path, and test_399a/399b only take the non-cached path when the test file exceeds readcache_max_filesize.

Again, too much LLM-generated explanation is just causing more issues than it solves.

(minor) The body covers the Architecture change, the -g flag, the configure-stamp dependencies, the install-data-local switch and the mv list, but not the new `rm -rf $(CURDIR)/debian/$(DKMS_PKG)/lib` in the DKMS rule. Worth a sentence saying it undoes the module-install side effect of install-data-local, otherwise it reads as an unrelated hunk.

(minor) This second Fixes: tag looks like it points at the wrong commit. 4ff30bece1 touched no file under debian/, and its config/Makefile.dkms-deb is a verbatim move of the dkms-debs recipe out of autoMakefile.am - the dpkg-buildpackage line and the mv list are byte-identical to what was in autoMakefile.am before it. None of the four things this patch changes (control.main Architecture, -g, the configure-stamp dependencies, install-data-local) live in code that 4ff30bece1 introduced.

The header install had already been broken before it: `AC_CONFIG_FILES` never generated an `include/Makefile`, so `$(MAKE) install -C include` could not work from the moment 9bb1727422 ("LU-18687 build: sync header location") collapsed the two `-C lustre/include` / `-C lnet/include` calls into it. If a second tag is wanted, that is probably the one:

    Fixes: 9bb1727422 ("LU-18687 build: sync header location")

The first tag (8cf1563a7c) does check out - it removed lustre/include/Makefile.am and so broke `install -C lustre/include`.

(minor) After the rebase described just above, does this patch still fix anything these two tags point at? The body says 377ce8531a already landed the header-installation fix and that this revision no longer touches it, and nothing in the diff is header related any more.

What is left is a change to which packages `make dkms-debs` emits and to the `Architecture:` field of `lustre-client-modules-dkms`. That target produced working DKMS packages before this patch, so this reads as a build-scope/packaging cleanup rather than a fix for a breakage either referenced commit introduced.

If that is right, both Fixes: lines can be dropped. If one is still meant to apply, it would help to say in the body what remains broken.

(suggestion) install-data-local is an automake hook, and in autoMakefile.am it is not headers-only - the `if MODULES` half creates $(modulefsdir)/$(modulenetdir) and copies every .ko found under lustre/, ldiskfs/ and lnet/ into them. Modules are enabled here (configure runs with --with-linux), so a `./configure && make && make dkms-debs` tree will have the whole module set copied into the staging dir just to be deleted again two lines below.

A small dedicated target in autoMakefile.am (say install-headers, holding just the three header loops) would decouple the DKMS package from whatever else install-data-local grows later.

(minor) This cleanup silently depends on modulefsdir/modulenetdir starting with /lib - they are derived from `moduledir="/lib/modules/${LINUXRELEASE}/${KMP_MODDIR}"` in config/lustre-build-linux.m4. That holds today, but if that path ever moves under /usr/lib the stale .ko files would ship inside the DKMS package instead. A comment naming the reason would help, or better, don't create them in the first place (see the note above).

(minor) With build-indep reduced to configure-stamp, nothing is compiled during the build phase of a `-g` run any more, so this rule's `$(MAKE) install DESTDIR=.../_tmp_` a few lines down becomes the first thing that compiles the tree. automake resolves `install` through `install-am: all-am`, so lib/, lnet/utils/, lustre/utils/ and lustre/tests/ all get built right there.

Two side effects of that move:

- build-stamp used `$(MAKE) -C $(BUILDDIR) $(PMAKEARGS)`; this call has no `$(PMAKEARGS)`, so the whole userspace build is now single-threaded.
- dpkg-buildpackage runs binary-indep under fakeroot (no Rules-Requires-Root in debian/control), so the compile now happens under fakeroot too, whereas before it ran in the plain build phase.

Would it be cleaner to give build-indep its own stamp that does the `$(MAKE) $(PMAKEARGS)` and leave this rule to just stage and package?

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

Which is better? The error messages of libnl or the error messages of strerror(). I went with the libnl ones.

We could set data->error = -errno;

According to libnl library code nl_syserr2nlerr() maps NLE_NOMEM to ENOBUFS. So if rc is a -NLE_* error then the correct thing to do is

data->errmsg = nl_geterror(rc);
data->error = -errno;

NLE_NOMEM is broken for you?

Yes I never set data->error :-(

This is why I told Alexey

we need to allocate large cb->min_dump_alloc. Otherwise we get this retry loops. Ugh what a mess.

(style) This isn't a bug, but a bare string on the right of `=~` is compiled as a regex, so the `*` quantifies the preceding `/` rather than standing in for the parameter name. It does happen to cover both dictionary spellings (`man4 page` and `man4/ page`), which `m{/man4/? page}` would state outright. As written, an entry later worded `Documentation/man4/<param>.4 page` would silently stop matching and turn the block back off.

(defect) The two-argument macro forms never reach the parameter name here.

In the real macro names `_SEQ_FOPS_` is followed by `RO_`/`RW_`, so the `TYPE\(\s*\w+,\s*` branch cannot match and `\w*\(` wins instead, capturing the first argument. That argument is the module prefix, not the parameter: lprocfs_status.h:795 expands `_RO_TYPE(name, type)` to `LDEBUGFS_SEQ_FOPS_RO(name##_##type)`, and the page is named `<name>.<type>.4`. `_WR_ONLY(name, type)` has no branch of its own and takes the same path.

The trailing `_` also excludes the bare `LPROC_SEQ_FOPS(name)` form, which spelling.txt:208 does nag on (lustre/quota/qsd_lib.c:179).

Checked against this commit with both `Documentation/man4/at_max.4` and `mdt.at_max.4` on disk:

    +LDEBUGFS_SEQ_FOPS_RW_TYPE(mdt, at_max);
    WARNING:TYPO_SPELLING: ... 'create Documentation/man4/ page for this parameter'?

`LUSTRE_RW_ATTR(at_max)` in the same patch is quiet, so the escape hatch works only for the single-argument forms. Since `_RO_TYPE`/`_RW_TYPE` are most of the `LDEBUGFS_SEQ_FOPS_*` uses in the tree, the commit message's claim to cover the `LDEBUGFS_SEQ_FOPS_` entries doesn't hold for them.

Would `_SEQ_FOPS(?:_(?:\w+_TYPE|WR_ONLY)\(\s*\w+,\s*|_?\w*\()` catch all the forms? That captures `at_max`/`force_reint`/`qsd_enabled` on the cases above, and the existing `{,*.}$param.4` glob then finds both the plain and the dotted page name.