Zhenyu Xu bobijam · review.whamcloud.com

(suggestion) LU-19344 is about the cached-read cost of file_read_confine_iter(), and 240cad9a0a quoted a 20-30% regression on 8k cached reads. Since this patch brings the confinement back, and also moves cl_env_get() back in front of the cache read path, could the message include before/after numbers for that workload?

(minor) This list of what the CIT_MISC cl_io_init()/cl_io_fini() pair adds seems incomplete. For a CIT_MISC io without ci_ignore_layout, lov_io_init_composite()/lov_io_init_empty() also do atomic_inc(&lov->lo_active_ios), and lov_io_fini() drops it. That pin is what keeps lov_conf_set() from running lov_layout_change() while the attributes are being read (see the inline comment in file_read_confine_iter()). Could the message mention this?

(minor) vvp_env_new_attr() already does memset(attr, 0, sizeof(*attr)) on the thread's shared cl_attr, so cat_kms is already 0 before the lookup. This rationale doesn't seem to hold. If the patch is refreshed, the bullet (and the explicit assignment) could be dropped.

(minor) "bumped around a write by this client that publishes a larger size" doesn't seem to cover every such write. O_DIRECT writes never queue pages, so vvp_io_write_commit() returns at `npages == 0` before either bump. Large buffered writes that do_file_write_iter() switches to DIO via ll_hybrid_bio_dio_switch_check() take the same path. So whether a racing reader at EOF gets EOF or the appended data depends on the write size. fallocate() and truncate-up don't bump either. If that's intended, could the message say which writes are covered?

The counter also moves on every buffered commit, including overwrites that don't change the size, not only on writes that publish a larger size.

(minor) If this is a fix, the behaviour it restores was removed by 240cad9a0a, which also dropped the original 251b with the rationale that "The read after the write should never return an end-of-file (EOF)". Should the message carry

    Fixes: 240cad9a0a ("LU-19344 llite: set read size correctly")

and explain why that rationale no longer holds?

(minor) attr comes from vvp_env_new_attr(), which already zeroes the whole struct, so this assignment is redundant.

(defect) Without the CIT_MISC io, what keeps the layout stable while this runs? lov_attr_get() uses LOV_2DISPATCH_NOLOCK and says "Layout is protected from changing by ongoing IO". Nothing on this path holds lo_type_guard or raises lo_active_ios, and the ll_layout_version_get() check above is only a snapshot.

A possible race on one client:

    T1: read() -> file_read_confine_iter() -> cl_object_attr_get()
        -> lov_attr_get_composite() walking lo_entries / lo_lsm
    T2: layout lock revoked (e.g. PFL instantiation from another node),
        then stat/write -> ll_layout_refresh() -> lov_conf_set():
        lo_active_ios == 0 -> lov_layout_change()
        -> lov_fini_composite() frees lo_entries and lo_lsm

lov_layout_change() doesn't take coh_attr_guard, so the spinlock held here doesn't exclude it either. Some existing callers have the same pattern (ll_readahead_handle_work(), ll_merge_attr() on the append path), but the old file_read_confine_iter() was pinned by its CIT_MISC io. This patch puts the unpinned lookup on every read() syscall. Would taking lov_conf_freeze()-style protection, or a lightweight lo_active_ios reference, avoid the race without the full cl_io_init() cost?

The new size becomes visible to a reader here, need to bump the lli_size_gen here.  With the  ll_file_io_generic()  bump in place, the second bump in  vvp_io_write_commit()  (after  cl_io_commit_async() ) becomes redundant — the window now closes at  ll_merge_attr() .

(defect) Can this report EOF for data that was in the file before the read started?

`orig_size` is the client's cached i_size, not EOF. Since 240cad9a0a, vvp_prep_size() does `i_size_write(inode, kms)`, and kms can be lower than the real size. lli_size_gen also moves on any buffered commit on this client, including an overwrite that doesn't extend the file. Example:

    node B: appends, file grows 1M -> 2M
    node A, P1: cat reaches offset 1M (i_size == kms == 1M)
                orig_ki_pos >= orig_size, gen == G
                vvp_prep_size(): pos > kms -> glimpse -> 2M
                copies 1M of node B's data
    node A, P2: overwrites offset 0 -> vvp_io_write_commit() -> gen G+2
    node A, P1: gen changed -> revert, return 0

cat stops at 1M even though node B's data was there before the read was issued. That contradicts the comment above, which says "A larger size discovered from another node during the read ... is returned normally" (commit message lines 30-31 say the same). Would it be safer to take the EOF baseline from a validated size (a glimpse, or kms under the lock), and to bump the counter only when a commit actually raises kms?

iov_iter_revert(to, result)  assumes  result  is exactly the amount the iter was advanced. For the fast-read path that holds; for ll_file_io_generic()  (which can restart and internally rewind/reexpand the iter) it's less obvious. The code this replaced used the more forgiving  iov_iter_truncate(to, 0) , which doesn't need to know the consumed count.

(minor) This is bumped only before the write, but the tiny-write size reaches the inode after __generic_file_write_iter(), through ll_merge_attr() below. A reader whose snapshot falls between this increment and ll_merge_attr() sees the new generation with the old i_size, and can then return the appended bytes. vvp_io_write_commit() bumps after the commit to avoid exactly this. Should there be a matching increment after ll_merge_attr() here?

(minor) If the patch is refreshed: "similar as i_version" reads oddly ("similar to i_version"?). Also, the counter is bumped on every buffered write commit and tiny write, not only on writes that make a larger size visible, so "Bumped around a write ... that makes a larger file size visible" overstates it.

These changes are unrelated to the intent of the commit and should probably be part of a separate "cleanup" patch.
This and all other instances of the same change.

Obj_count is only modified for the call to compute blob_size   so this conditional to decrement it back to the original value should be moved to to line 857   just after blob_size = ...
So the increment and decrement are close together.

Even better would be to skip the conditional increment/decrement and do : 

blob_size = lov_user_md_size(obj_count + (comp->llc_flags & LCME_FL_FOR_MERGE) ? 1 : 0, magic);

and thus avoid temporarily modifying obj_count.

(minor) The new clause fires for the first component of every layout, not just mirrors - a plain or default-striped file also has `llc_extent.e_start == 0`.

The practical effect shows up with default striping (LOD_USES_DEFAULT_STRIPE), where stripe_count_min is 3/4 of stripe_count: the allocator now escalates to speed 1 and 2 to fill the remaining quarter. At speed 2 lod_check_and_reserve_ost() stops honouring OS_STATFS_DEGRADED and stops calling lod_should_avoid_ost(), so first components can land on degraded OSTs and on OSTs already used by a conflicting mirror component.

For an FLR patch that last one seems to work against the goal. Was the intent to limit this to mirrored layouts?

Patrick-Bot: Not changed. Whether the extra retry should apply only to the first component of mirrored layouts, or move to its own change, is a design decision for the patch owner.

(minor) The return value is not checked, and `rc` is overwritten by the migrate_open_files() call below. If llapi_layout_mirror_count_get() ever fails, `mirror_cnt` stays 0 and the multi-mirror handling is silently skipped.

Patrick-Bot: Done in patchset 10: a failure to get the mirror count now fails the extend. Leaving this thread open for a related problem in the same hunk. Clearing resync for a multi-mirror merge makes lfs set LL_LEASE_ALLOW_STALE, and since LU-18746 llite refuses that with EOPNOTSUPP unless the client has llite_enable_flr_ec=1. That parameter defaults to 0. So every extend that adds more than one mirror fails on a default client, as in sanity-flr 0d, 0e, 0f, 0m and 44d on rocky9 ldiskfs. With llite_enable_flr_ec=1 those tests pass. test-framework does not set it either: it sets MODOPTS_LLITE, but load_module reads MODOPTS_LUSTRE for the llite module. How the merge should mark the new mirrors stale without needing FLR EC is for the patch owner to decide.

(minor) This picks the whole strategy from the first list entry only. Previously each entry was dispatched on its own `m_file`.

With `lfs mirror extend -N -f victim -N2 -c1 FILE` the loop now calls mirror_extend_file() for the second entry too, with `victim_file == NULL`, which turns into `open(NULL, ...)` and an EFAULT.

The man page does say -f cannot be combined with SETSTRIPE_OPTIONS, but nothing enforces it for extend (mirror_create_sanity_check() is only called from mirror_create()). Should mirror_extend() reject a mixed list up front?

Patrick-Bot: Not changed. Rejecting a mixed list would break sanity-flr test_0d, which runs 'lfs mirror extend -N -S 4M -N -f $tf-2 $tf-1' and expects it to succeed. Handling the -f entries one by one and merging the rest changes the order mirrors are added in, so this is left for the patch owner.

(minor) When more than one mirror is added, the data is now copied by lfs_mirror_resync_file(). That function always opens the file with O_DIRECT, so 'lfs mirror extend -D|--non-direct -N2 FILE' silently ignores -D. Before this change, each mirror was copied by migrate_nonblock() on descriptors that migrate_open_files() opened according to LLAPI_MIGRATION_NONDIRECT. The single-mirror path still honours -D, so the option now behaves differently depending on how many mirrors are added. If the patch is refreshed, could migration_flags be passed down to the resync, or could the man page say that -D only applies to single-mirror extends?

(minor) This changes `lfs getstripe` output for every composite file, and it looks unrelated to the mirror-merge work.

A couple of things go stale with it: the worked example in the comment above this function still shows `- lcme_id:             1`, and Documentation/ec.txt shows decimal ids too. Test suites also scrape this field (`awk '/lcme_id/{print $2}'` in sanity-flr.sh and sanity-ec.sh).

If the hex form is wanted, could it go in its own patch together with the doc updates?

Patrick-Bot: Not changed. Keeping, splitting out or dropping the hex lcme_id output is for the patch owner to decide. If it stays, the example in the comment above and Documentation/ec.txt need updating too.

(style) Carrying this forward from patchset 6, where it was asked whether these cases should be split into separate 63a/b/c/d subtests. test_63 still runs all four migrations in one subtest, so a failure in one hides the others and the cases can't be run on their own.

(defect) The test needs two things on the MDS: the mdt_enable_flr_ec module parameter, which came with LU-12187 in 2.17.51, and the stale-mirror merge support from LU-18771. A 2.16.52-70 gate lets in a released 2.17.0 MDS, which has neither. On that MDS, `cat /sys/module/mdt/parameters/mdt_enable_flr_ec` fails and the test errors out with "cannot read FLR_EC setting" instead of skipping. Should the gate use the 3-component version that includes LU-18771? The skip message should then say why the version is needed, e.g. "need MDS with stale mirror merge support".

(style) This isn't a bug, but several err_str values here end in "\n" (this one, plus "cannot get layout\n", "cannot get mirror count\n", the comp_use/mirror id ones and "cannot delete mirror\n"). The final fprintf() appends ": %s\n", so the strerror text ends up on its own line. Also, mirror_merge(), mirror_extend_layout(), lfs_mirror_resync_file() and mirror_split() already print their own errors, so every failure is reported twice. If the patch is refreshed, consider dropping the newlines and the duplicate messages.

(defect) Can this destroy the only up-to-date copy of the data? lfs_mirror_resync_file() releases its lease before the purge loop starts, and nothing (group lock or lease) stops writers from that point until mirror_split() runs. If the file is written in that window, the MDS picks a primary mirror (possibly an original one) and marks the new mirrors stale. mirror_split(MF_DESTROY) then removes the original. mdd_split_ea() only refuses when no *data* mirror would remain, and stale data mirrors still count. So the file ends up with only stale mirrors, and the write is lost.

The `lfs mirror delete` path checks last_non_stale_mirror() and resyncs before splitting. Should each split here do the same check, or should the whole extend/resync/purge sequence run under a lease or group lock like lfs_migrate() does in its default blocking mode?

(style) This isn't a bug, but free() and llapi_layout_free() both accept NULL, so the `if (ioc)` / `if (m_layout)` / `if (f_layout)` checks can go if the patch is refreshed.

(defect) Now that migrate goes through the mirror path, how is `-N=COUNT` (mirror_total_mode) meant to work? The block below subtracts the file's existing mirror count, but lfs_migrate_to_mirror() then purges all of those original mirrors. For a file with 2 mirrors:

- `lfs migrate -N=4` creates 2 new mirrors and deletes the old 2, leaving 2 mirrors instead of 4.
- `-N=3` gives mirror_count 1, so it falls back to plain lfs_migrate().
- `-N=2` prints "no new mirrors will be created" and then runs lfs_migrate() anyway.

Should `=COUNT` be rejected for migrate, or treated as the absolute target count?

(style) This isn't a bug, but this block-local `int mirror_count` shadows the function-scope `mirror_count` that the -N parsing sets just above. If the patch is refreshed, a distinct name such as `total_mirrors` would make it clear which count the `> 1` check uses.

(defect) With a single mirror (`-N` or `-N1`), mirror_count is 1, so this falls through to lfs_migrate() with `param`/`layout`. With -N, though, every -E component was built into `mirror_list->m_layout` (lpp points there), and `layout` is still NULL. So `lfs migrate -N -E2M -Eeof -c-1 file` builds `param` from the last component's args and migrates to a plain, non-composite layout. The [0,2M) component and any --flags are dropped without a warning.

The new test step "migrate to 1 mirror w/ 2 components" only compares checksums, so it passes even though no component layout is created. Should the single-mirror case also go through lfs_migrate_to_mirror(), or pass `mirror_list->m_layout` to lfs_migrate()? And should the test check verify_comp_count for that step?

(minor) The body doesn't mention the client-side gate added by squashing Ib1e2123ff: the new lustre_check_lov_user_md() and lustre_check_lov_comp_md_v1() in ptlrpc/pack_generic.c, and their calls from ll_lov_setstripe_ea_info() and ll_xattr_set(). These reject a compress layout when `llite.*.enable_compression=0`, which is a user-visible behaviour change. Could a short paragraph describing them be added?

@ablagodarenko@thelustrecollective.com it looks like the patches were rebased, but did not address any of the AI review comments?  While we can't go overboard changing every detail of the patches, I think it does make sense to fix the code where possible so that we don't carry technical debt later.

Hi Andreas. Thanks for poining to this. I have reverted all fixed by Patchset 11 pushing wrong branch. I have reverted to Patchset 10 where I addressed AI bot comments and fixed build.

Squashed Ib1e2123ffdb239c3e1401d682ae9c2c49e3f4a6f into the change. I'll address a couple more comments in another push.

(minor) There is no `.TP` before this entry, so `--compress-type|--compr-type` and its description get folded into the `-z, --extension-size` paragraph above instead of starting their own tagged item. The two entries below it do have theirs.

weird but in my man output it looks ok.

The entry was missing its .TP in the troff source. Some man implementations are lenient and reflow correctly, but others fold the entry into the previous paragraph. Added .TP for correctness.

Done. Added the missing .TP before the --compress-type|--compr-type entry.

Should be fixed if patch is refreshed.

(minor) Neither of the two new man3 pages has an EXAMPLES section. Lustre llapi pages are expected to carry a short usage example showing the required header and a realistic call.

(minor) This, and the matching EINVAL entry under ERRORS, says a `stripe_size` that is not a multiple of `chunk_size` is an error. llapi_layout_compress_set() never fails for that: it decrements `log_bits` until the chunk divides the stripe and stores the smaller chunk (sanity-pfl test_100c expects 3*64KiB to become 128KiB). Similarly, gzip/lz4/lz4fast levels outside the listed ranges are clamped by their from_*_level() helpers, not rejected with EINVAL as the text above says (test_100c expects gzip:15 to read back as 9). If the patch is refreshed, could the page describe the rounding and clamping instead?

(minor) llapi_parse_compress_type() also returns -EINVAL - for a level that isn't a valid number or falls outside +/-COMPR_LEVEL_MAX - which neither RETURN VALUES nor ERRORS mentions. Mixing -1 with -Exxx returns in one function is also awkward for callers; would returning -EINVAL for the unknown-type case be more consistent?

(minor) The 9 used as the lz4 default collides with a real level. from_lz4_level(10) returns 9, and to_lz4_level(9) returns 9 because 9 == LZ4HC_DEFAULT_CLEVEL, so 'lfs setstripe -Z lz4:10' reports back as lz4:9 and level 10 can never be displayed - even though lfs-setstripe.1 documents 1-16 as the valid lz4 range.

(defect) This gate runs only for regular files: ll_lov_setstripe_ea_info(), and the `lov.` branch in ll_xattr_set(). A directory default goes through ll_setstripe_ea() -> ll_dir_setstripe() (and the LL_IOC_LOV_SETSTRIPE case in ll_dir_ioctl()), which never calls lustre_check_lov_user_md(). So on a client with `enable_compression=0`:

    lfs setstripe -E eof -Z lz4 $dir    # accepted
    touch $dir/f                        # MDS instantiates a compress layout

This still produces a file the client can't handle. lsm_lmm_verify_v1v3() rejects it with "file compression not enabled", and the PS14 report shows a later unlink or stripe change hitting the lov_io_layout_at() LBUG. Should ll_dir_setstripe() apply the same check? Or, if allowing compressed defaults on a disabled client is intended, should the client reject the layout cleanly instead of asserting?

(defect) On this `lov.` path nothing validates `size` before lustre_check_lov_user_md() walks the entries. lustre_check_lov_comp_md_v1() reads `lcm_entry_count` and then up to 65535 `lcm_entries[]`, but the buffer here is only the `size` bytes copied in by the VFS.

The `lustre.` xattr namespace needs only write permission on the inode, so any file owner can trigger it:

    setxattr(f, "lustre.lov.add", buf, 8, 0);  /* buf starts with LOV_USER_MAGIC_COMP_V1 */

That reads lcm_entry_count past the end of an 8-byte allocation and keeps reading for up to ~3MB. KASAN reports it as slab-out-of-bounds. Before this patch, little-endian clients only read `lmm_magic` here.

The next change in the series ("utils: add 'lfs find' support for compressed file") adds a `size < need` check just above this call. Since this patch introduces the read, should that check move here? Passing `size` into lustre_check_lov_user_md(), the way lustre_swab_lov_user_md() takes one, would also let it validate the buffer itself.

(minor) This applies the chunk adjustment to every component including extension ones, where llc_stripe_size holds the extension size rather than a real stripe size. lod_declare_layout_add() deliberately skips LCME_FL_EXTENSION components for exactly that reason, so the same input layout gets different chunk_lum_bits depending on whether it arrives via create or via --component-add. Should this site skip extension components as well?

While there: the comment added in lod_declare_layout_add() says chunk adjustment is deferred until instantiation, but nothing calls lod_adjust_compr_chunk_size() on the instantiation path - the only three callers are here, lod_declare_layout_add() and lod_striping_from_default().

(defect) -ENOTSUPP (524) is kernel-internal and has no userspace errno, so this reaches `lfs setstripe` through ioctl()/setxattr() as "Unknown error 524". The erasure-coding check just above it in ll_lov_setstripe_ea_info() returns -EOPNOTSUPP for the same kind of rejection. Should this be -EOPNOTSUPP too?

While here, a couple of nits: `rc` is never changed from 0, there's a double blank line after the EXPORT_SYMBOL(), and the default case in lustre_check_lov_user_md() logs "Invalid LOV magic" but still returns 0.

(minor) The comment above the block still says the readahead goes "up to ra_max_pages_per_file", and the CDEBUG below prints ra_max_read_ahead_whole_pages and ra_max_pages_per_file.  Neither is the limit any more.  If the patch is refreshed, could the comment name the effective limit and the trace print ra_max_window_pages(ra) (or ra_max_pages) so the dump matches what the window is actually clamped to?

(defect) Capping this window makes it possible for ras_whole_file_read to be true while the window no longer covers the file, and nothing recovers from that.

The entry test is kms_pages <= ra_max_read_ahead_whole_pages, but the window is now min(ra_max_window_pages(ra), ra_max_read_ahead_whole_pages).  max_read_ahead_whole_mb is only clamped against ra_max_pages_per_file and max_read_ahead_mb is not clamped against it at all, so ra_max_pages < kms_pages <= ra_max_read_ahead_whole_pages is reachable: with brw_size=16M (whole = 16M) and max_read_ahead_mb=8, a 12M file on its second read gets ras_whole_file_read = true and a 8M window.

From there ll_readahead() takes end_idx = ras_window_start_idx + ras_window_pages - 1 and only stretches it far enough to cover the current read, ras_update() returns early on ras_whole_file_read so the window is never grown or moved, and this function resets ras_next_readahead_idx to 0 on every read() while the whole-file test keeps matching.  The readahead frontier can never pass ra_max_pages, and the tail of the file is read a page per RPC for the life of the fd.

Before the patch the window was ra_max_read_ahead_whole_pages >= kms_pages, so the frontier walked to EOF: ll_ra_count_get() clamped each reservation to the remaining budget, and cached pages return 1 from ll_read_ahead_page() without consuming any reservation.

Would folding the budget into the entry test fix it, so the normal detector keeps running when the budget cannot cover the file?

    if (kms_pages && kms_pages <= min(ra->ra_max_read_ahead_whole_pages,
                                      ra_max_window_pages(ra))) {

(suggestion) This was raised on patchset 1 and the pass criterion is unchanged: 101d still passes or fails on two dd wall-clock numbers, which is what makes it flaky in the first place.

Now that the patch makes the async gate reachable, the new instrumentation already reads the values that would make this deterministic.  Could the test also assert on them unconditionally, e.g. llite.*.read_ahead_stats async_readahead > 0, or that the osc.*.rpc_stats read histogram is not dominated by 1-page RPCs?  That would fail for the LU-20703 reason rather than for timing noise.

[Marc Bot] upstream remarks for reference:

The lod fix this test was written for is already on master (90c093b7850, LU-17468); only the sanity-compr test_1a part was unlanded - ported under a new Change-Id with the original as Was-Change-Id.

[Marc Bot] upstream remarks for reference:

- Rewritten onto the current naming: LL_COMPR_TYPE_SPEC_NONE = 9 added (MAX moves to 10); wire asserts extended accordingly (this tree carries LL_COMPR_TYPE_MAX asserts the original did not have).
- compr_type_table: the NONE row loses its name, SPEC_NONE row ("none") added before the MAX sentinel - the compress_type_2str()/parse helpers were already in the matching form.
- The inheritance-hint struct uses cd_compr_chunk_lum_bits with this series' lum-bits semantics (offset above COMPR_CHUNK_MIN_BITS - no +16 at use sites), and lod_adjust_compr_chunk_size() is called in its take-by-value/return-by-value form.
- lod_qos_parse_config: the convert_plain arm skips all lcme-derived assignments including fields this tree has that the original context did not (llc_mirror_link_id, llc_dstripe_count, llc_cstripe_count).
- SPEC_NONE clearing blocks in lod_parse_striping/lsm_unpackmd applied at the current field naming.
- test_1000d version gate converted to compression_supported.

[Marc Bot] upstream remarks for reference:

The test_1021 tweaks (sleeps, echo moves) re-applied against the unwrapped assertion blocks; ci_need_change_layout renamed as in the original.

warn: vvp_layout_minor_change():passing zero to 'ERR_CAST'

error: vvp_layout_minor_change():'vlcw' dereferencing possible ERR_PTR()

[Marc Bot] upstream remarks for reference:

- The patch re-created a libcfs_string.h/.c pair to host string-mask helpers; master already provides cfs_str2mask()/cfs_mask2str() with u64 masks (lnet/libcfs/debug.c), so both files were dropped and the new compress_types stores call the existing helper (~0ULL masks). A hunk that narrowed the existing capability-mask store from ~0ULL to ~0 was dropped for the same reason.
- mdt.*.compress_types and obdfilter.*.compress_types are implemented as sysfs attributes (the target-side procfs vars tables the original patch extended no longer exist on master).
- The obd_compress_types mask enum / COMPRESS_NAMES use the series' type naming (lz4 for the HC type) and already include zstd, which is a valid layout type at this point in the series.
- OST-side only, as in the original: OBD_CONNECT2_COMPRESS is advertised in OST_CONNECT_SUPPORTED2 and requested by the OSC; MDT enabling remains pending (test_1013b keeps its unconditional skip; its old-version negative-check block, meaningless on master, was dropped).
- compression_supported() in test-framework.sh now probes the OSC import for the compressed_file connect flag instead of comparing versions. (The OSC import, not the MDC one, because MDT support is not enabled.)
- test_1013a/b were placed before test_1015 (the tests they were anchored to arrive later in the series).

[Marc Bot] upstream remarks for reference:

Applied cleanly. Note for reviewers: this changes where the PFID EA is stored for small inodes (no longer embedded in LMA for <=256-byte inodes) - needs interop/lfsck review on master.

(minor) This is an on-wire change: the client now fills bytes in the obdo (formerly o_padding_4) that are sent to the OST/MDS. Test-Parameters: forjanitoronly doesn't exercise interoperability. Consider requesting old-server/new-client and new-server/old-client runs with serverjob/serverbuildno and clientjob/clientbuildno so the padding-reuse stays wire-compatible with unpatched peers.

[Marc Bot] upstream remarks for reference:

Rebase notes: kept the base's comment wording for the chunk-bits field
(2^(16+chunk_lum_bits)); the lov_lsm2layout() wrapper is added alongside
the base's compr_name_from_type() helper in lov_internal.h.

(minor) This comment reword looks unrelated to sending compression info to the server; is it meant to be in this patch? Also the code computes the chunk size with COMPR_CHUNK_MIN_BITS (COMPR_GET_CHUNK_SIZE()), not LOV_MIN_STRIPE_BITS; they happen to both be 16, but referencing the constant the code actually uses would be clearer.

This isn't a bug, but after this patch lov_lsm2layout() has no remaining callers (both lov_io.c and lovsub_object.c now call lov_lsm2layout2()). Folding the olc parameter into lov_lsm2layout() and updating those two call sites would avoid the version-suffixed name and the now-dead wrapper.

[Marc Bot] upstream remarks for reference:

Chunk-bits field naming follows the series (llc_compr_chunk_lum_bits).

[Marc Bot] upstream remarks for reference:

- The strtoul/end-pointer validation this patch adds was already carried (in stricter form) by the earlier level-handling patches of this series; those hunks were dropped.
- The parse-time rewrite of low lz4 levels to the lz4fast default was removed here (matching the original series' final behaviour): the stored level is kept and the compression engine falls back to lz4fast when allocating the compressor. This also makes the negotiated-type test lists in sanity-pfl roundtrip correctly.
- The new negative test was renamed test_1013a -> test_1013c: 1013a already exists (type negotiation test); the duplicate name would have silently shadowed it (latent issue in the original series).

(minor) The diff also touches Documentation/man1/lfs-mirror-extend.1 (documenting compression options for SETSTRIPE_OPTIONS), which the message doesn't mention and which is unrelated to the migrate fix. Is that hunk meant to be part of this patch, or should it be split out / called out in the body?

(minor) This prints `fd`, which is always -1 on this path, so the message reads "Cannot open file X: -1" with no useful detail. The error is in errno from open() – strerror(errno) would be the informative value.

Also note the caller (lfs_migrate/lfs.c) already emits its own llapi_error() when this returns < 0, so this adds a duplicate line. Using llapi_error(LLAPI_MSG_ERROR, errno, ...) here (as the ENOSPC path below does) would decode errno and respect the message level instead of writing to stderr unconditionally.

[Marc Bot] upstream remarks for reference:

Rebase note: kept the base's newer compression-parameter assignment block
in lod_use_defined_striping() (gated on LOV_PATTERN_COMPRESS, with the
extension-component chunk-adjustment deferral); this patch's encryption
checks and attr threading apply on top unchanged.

[Marc Bot] upstream remarks for reference:

Port notes:

- Registered as .cio_commit_async with the current signature (the
  enum cl_io_priority argument was added upstream), next to master's
  renamed .cio_read_ahead_prep hook.
- Reviewer note: master meanwhile carries LU-17364 ("discard un-flushable
  pages on evicted inode teardown"), which addresses the same teardown
  crash from another angle; this patch is kept as an additional guard and
  may be droppable after evaluation.

[Marc Bot] upstream remarks for reference:

ci_incompressible placed in the relocated cl_io bitfield next to ci_size_extend_compression; test_1021 gate converted to compression_supported.

error: osc_io_iter_init():'osc' dereferencing possible ERR_PTR()

[Marc Bot] upstream remarks for reference:

- OBD_CONNECT2_UPDATE_LAYOUT was already reserved on master at the same bit; only the MDT_CONNECT_SUPPORTED2 and client connect lists needed the addition.
- ci_need_change_layout added to the current cl_io bitfield; CAT_COMPRESSIBLE handling merged into the current vvp_attr_update().
- mdd_layout_update_pccro was renamed/extended per the patch on the current (retry-aware) master form.
- FORMATTED_BUF_LEN (used by the new getstripe formatting) is defined locally - the source branch carries it from an unrelated change.
- test_1021 layout-flag assertions applied unconditionally (the original wrapped them in version gates).

error: vvp_layout_minor_change():'vob' dereferencing possible ERR_PTR()

error: vvp_io_fini():'vob' dereferencing possible ERR_PTR()

error: vvp_io_init():'vob' dereferencing possible ERR_PTR()

(minor) The body explains the DoM+compress block, but two hunks pull the other way and aren't mentioned: lov_pattern_available() gains LOV_PATTERN_MDT | LOV_PATTERN_COMPRESS, and llapi_pattern_to_lov() gains MDT|COMPRESS and RAID0|COMPRESS cases. Both make the combination more acceptable, not less. Could the message cover why they belong in a patch whose stated purpose is to disallow it?

(defect) This makes the server accept an MDT|COMPRESS component (lod_lov.c / lov_ea.c call lov_pattern_available()), yet lov_pattern_supported() still excludes it, so client IO marks such an entry invalid (lov_object.c sets lle_valid = 0). The only guard against creating it lives in lfs.c; a liblustreapi caller doing llapi_layout_pattern_set(MDT) + llapi_layout_compress_set() bypasses that and produces a layout the server stores but IO cannot use. If DoM+compress is unsupported, should the block live in the library/server rather than only the CLI?

(defect) errno is set to a negative value here; every other error path in comp_args_to_layout() uses errno = EINVAL. A negative errno breaks strerror(errno)/perror on this -1 return. Should be errno = EINVAL.

(minor) These two cases look unreachable. The COMPRESS bit never reaches llc_pattern: llapi_layout_pattern_set() rejects any pattern carrying it, and llapi_layout_from_lum() stores compression in llc_compr_type, not the pattern. The bit is OR'd into lmm_pattern separately in llapi_layout_to_lum() (blob->lmm_pattern |= LOV_PATTERN_COMPRESS). Was a real caller intended, or can these be dropped?

This bonus is added once per allocated stripe, which is exactly the weight the OS_STATFS_NONROT bonus a few lines above uses, so the two can cancel out.

Two mirrors, no LCME_FL_PREF_WR anywhere, one stripe each:

    mirror 0: compressed,   stripe on a NONROT OST -> pref = 1
    mirror 1: uncompressed, stripe on a rotating OST -> pref = 1

lme_preference ties, so the `!found_preferred` fallback below keeps the lower index (`>` is strict) and sets lme_prefer on mirror 0. lod_primary_pick() then classifies mirror 0 as case B (preferred + compressed) and mirror 1 as case C, and B outranks C, so the write goes to the compressed mirror.

A wider compressed mirror wins outright rather than tying: 4 NONROT stripes compressed = 4 vs 1 NONROT stripe uncompressed = 2.

Mixed flash/disk mirrors are the case the non-rotational preference was added for, so this is not an exotic layout. Should the uncompressed bonus be on a different scale than the per-stripe NONROT count (or applied once per mirror) so it can't be traded away against OST speed?

The comment also says "mirror", but this sits in the per-stripe loop of one component.

(style) Not a bug, but this comment has a UTF-8 em dash in it; the rest of the tree is plain ASCII. A plain '-' would do.

Ranking B (preferred + compressed) above C (uncompressed) reads as "an explicit `lfs setstripe --flags=prefer` beats the compression heuristic". But lme_prefer is not only user intent: when no component carries LCME_FL_PREF_WR, lod_fill_mirrors() picks the highest lme_preference non-stale mirror and sets lme_prefer = 1 on it.

So tier B is reachable on a file the user never flagged, and the heuristic ends up overriding its own compression preference. Should case B be restricted to mirrors that actually carry LCME_FL_PREF_WR, or should the synthesized preference be tracked separately from the user flag?

(minor) ci_ndelay_tried > 0 means "not the first lov_io_mirror_init() for this env", which is broader than "this is an FLR error retry".

ll_file_io_generic() restarts the io for reasons unrelated to mirror failure and preserves the counter:

    partial_io -> io->ci_need_restart = 1   (file.c)
    retried = io->ci_ndelay_tried; goto restart;

A single buffered read larger than max_io_bytes therefore takes the uncompressed mirror for the first chunk only; from the second chunk on ci_ndelay_tried is non-zero, lis_mirror_index has been advanced by one, and the first in-range mirror is accepted even when it is compressed. The layout-change restart in vvp_io_iter_init() has the same shape.

Would gating on something that only the -EAGAIN mirror-failover path sets (io->ci_tried_all_mirrors, or a dedicated flag set where cl_io_loop() retries) keep the failover fix without giving up the preference on ordinary io continuation?

(minor) The first three bullets read as new functionality, but `--comp-flags=[^]compress` and `--comp-flags=[^]nocompr` already work (LCME_FL_COMPRESS/LCME_FL_NOCOMPR are in comp_flags_table[]), and so does `-L compress` (LOV_PATTERN_COMPRESS is in lov_pattern_names[]). This patch documents them and adds test coverage rather than adding them. Could the wording say "document" for those three, so the message matches the diff?

(style) COMPR_LEVEL_MAX is already defined with the same value earlier in this header, just after `enum ll_compr_type`, with a comment explaining it. Two definitions can drift apart later; can this one be dropped?

(style) extra blank line. Also z1..z4 here, and `expect` in test_100d, are not declared `local`.

(style) not a bug, but `compress-chunk` already appears in this long_opts[] further down, so it is listed twice now. getopt_long takes the first match so it still works; `compr-chunk` looks like the only new alias actually needed here.

(typo) this comment uses a UTF-8 em dash; the tree is otherwise plain ASCII. Same in the `%LZ` directory-branch comment in liblustreapi_pfind.c.

(defect) atoi() cannot report a parse failure, so `--compr-level=abc` silently becomes level 0, and `--compr-level=+abc` becomes "level > 0", i.e. every compressed component. The unsigned range check above cannot catch it.

`-Z gzip:abc` is rejected by llapi_parse_compress_type(), and the setstripe `--compress-level` path uses strtol() with an end-pointer check, so the three spellings of the same thing disagree. strtol() plus `*end != '\0'` here would line them up.

(minor) This helper backs every numeric `lfs find` predicate, and the new exact-match branch only changes behaviour when `margin == 0`. `lfs find -z N` with a plain number is such a caller: lfs.c does `fp_ext_size_units /= SEL_UNIT_SIZE`, so 1/1024 becomes 0, and an exact `-z` match that previously reported "no match" now matches. That may well be desirable, but it is a silent change to an unrelated option.

The new compression predicates are the only callers passing margin 0; the rest pass 1 or a unit size, and with margin 1 this function already returns 1 on an exact match. Passing 1 there would avoid touching the shared helper, and would also make `--compr-level=-N` / `--compr-chunk=-N` mean "< N" like `-c -N`, `--size -N` and friends, rather than "<= N".

The table above the function (rows 5 and 6, and the note about the interval `(limit - margin, limit]`) no longer describes the code either.

(defect) Negation is handled differently for level/chunk than for type. `fp_exclude_compr_lvl` is passed into find_value_cmp() as `negopt`, so `found_lvl` ends up meaning "some component does not match" instead of "no component matches", and the aggregation here then ignores the exclude flag entirely - unlike the fp_check_compr_type block just above.

For a file with two compressed components, level 3 and level 5:

    lfs find ! --compr-level=3 dir

entry(lvl 5) makes find_value_cmp() return -1, negopt flips it to 1, found_lvl is set, and the file is reported - even though it does have a level-3 component. The equivalent `! --compr-type=gzip` on a gzip+lz4 file correctly does not match.

Should the level and chunk comparisons drop the negopt argument and be inverted here the same way type is?

(defect) `fp_lum_size` is the size common_param_init() allocated for the lmd buffer (>= XATTR_SIZE_MAX, 64KiB), not the length of the layout stored in it.

For a non-composite lum, llapi_layout_get_by_xattr() never narrows that value, so llapi_layout_objects_in_lum() computes (65536 - 32) / 24 = 2729 objects and __llapi_comp_alloc() rejects it with EINVAL because 2729 > LOV_MAX_STRIPE_COUNT (2000).

So this call fails for every plain V1/V3 file and always falls through to llapi_layout_get_by_path(); only composite layouts reach the new path, because the entry loop resets the size to `ent->lcme_size`. Passing the actual layout length instead would make the fast path work for non-PFL files too.

The body explains why lod_layout_mutex is needed, but two hunks aren't accounted for:

- `lo->ldo_flr_state = 0;` added to the plain-layout branch of lod_parse_striping()
- the extra `!lo->ldo_is_composite ||` term in the lod_declare_update_extents() assertion

Could the body say what each of those is for? As written the text reads as if the assertion should stop firing for plain layouts, but the code makes it fire more often (see the inline comment).

This fixes an LBUG, so it would help to carry a Fixes: tag. The assertion being hit was added by:

    Fixes: ff5eb304fa37 ("LU-10070 lod: SEL: Implement basic spillover space")

That commit introduced lod_declare_update_extents() together with `if (lo->ldo_flr_state == LCM_FL_NONE) LASSERT(start_index == 0 && max_comp == lo->ldo_comp_cnt);` and the unlocked access to ldo_mirrors[]/ldo_comp_cnt that this change now serializes.

The same reset looks missing in the other two places that build a plain in-core layout after freeing the old one:

- lod_use_defined_striping() (lod_qos.c:2217) sets `mo->ldo_is_composite = 0` for LOV_MAGIC_V1/V3 with no ldo_flr_state reset
- lod_qos_parse_config() (lod_qos.c:2557) does the same after lod_free_comp_entries()

lod_free_comp_entries() clears ldo_mirrors, ldo_mirror_count and ldo_is_composite but not ldo_flr_state, so a composite-to-plain transition through either of those still leaves a stale value. lod_declare_layout_change() then dispatches on it, e.g. into lod_declare_update_rdonly() whose `LASSERT(lo->ldo_mirror_count > 0)` cannot hold for a plain layout. Should those get the same fix?

Adding `!lo->ldo_is_composite ||` widens the guard rather than narrowing it: `A || B` is true whenever `B` is true, so the LASSERT is now evaluated in strictly more cases than before, and the plain-layout case (`ldo_is_composite == 0`) now always evaluates it.

The commit message says ldo_flr_state is only meaningful for a composite layout and that a plain file "could get into lod_declare_update_plain()->lod_declare_update_extents() and bump into this assertion", which reads like the intent was to exempt plain layouts:

    if (lo->ldo_is_composite && lo->ldo_flr_state == LCM_FL_NONE)

With `||`, a plain file in the state that produced the LU-18592 LBUG (flr_state == LCM_FL_NONE, stale ldo_mirrors) still hits the same assertion. Which of the two is intended?

Changing `RETURN(-EALREADY)` into `GOTO(out_unlock, rc = -EALREADY)` also routes this through `out:`, so the -EALREADY case now calls lod_striping_free() and drops the cached layout. Previously it returned with the cache intact.

mdd_layout_instantiate_component() turns -EALREADY into success, so every layout-write intent on an already-instantiated component now throws away the in-memory striping and forces a re-read of the LOV EA. Is that intended, or should this path skip the free?

This isn't a bug today, but making the unlock conditional on `rc` is fragile: `out:` is reached both by fall-through with the mutex already released (rc == 0) and by `GOTO(out, ...)` with it still held. It only works because every goto to `out:` happens to set rc != 0 -- a future `GOTO(out, rc = 0)`, or a `lod_declare_update_extents()` return of 0 being propagated, silently leaks the mutex.

A dedicated `out_unlock:` label above `out_free:` (as was done in lod_declare_update_plain()) would make the lock state structural instead of value-dependent. Same shape in lod_declare_update_write_pending() and lod_declare_update_sync_pending().

This assignment to ldo_flr_state is still outside ldo_layout_mutex, but lod_declare_update_write_pending() now takes the mutex and then asserts on the value:

    lod_declare_update_sync_pending()      lod_striping_reload()
      lo->ldo_flr_state = WRITE_PENDING      mutex_lock(ldo_layout_mutex)
                                             lod_parse_striping()
                                               lo->ldo_flr_state = <from disk>
                                             mutex_unlock()
      lod_declare_update_write_pending()
        mutex_lock(ldo_layout_mutex)
        LASSERT(ldo_flr_state == LCM_FL_WRITE_PENDING)  <- fires

Since this is the same class of problem the patch is fixing, should the state transition move inside the mutex (or the mutex be taken here and passed down)?

This isn't a bug, but `layout_lcked` is spelled `layout_locked` in every sibling function (lod_check_and_reserve_ost(), lod_ost_alloc_rr(), lod_ost_alloc_specific(), lod_qos_prep_create()). Worth making it consistent if the patch is refreshed.

This is the one remaining caller passing `false`, and it leaves the two lock orders divergent:

    lod_prepare_create()                    -> lod_ost_alloc_{qos,rr}()
                                               lq_rw_sem / ltd_rw_sem held
                                               -> lod_comp_is_ost_used(..., false)
                                                  -> mutex_lock(ldo_layout_mutex)

    lod_declare_update_plain()  mutex_lock(ldo_layout_mutex)
                                -> lod_declare_instantiate_components()
                                   -> lod_qos_prep_create(..., true)
                                      -> lod_ost_alloc_qos() down_write(lq_rw_sem)

So ldo_layout_mutex is now above lq_rw_sem on the new paths and below it here. This isn't reachable as a hang today (the MDT holds the layout LDLM lock EX for both entry points on a given object), but it makes the ordering depend on an unrelated lock. Would it be cleaner to have lod_prepare_create() take ldo_layout_mutex too and pass `true` everywhere, so lod_obj_for_each_stripe() never has to acquire it as an inner lock?

Related: the new critical section now spans OST allocation, which includes a `down_write_killable()` with a 2s timer in lod_ost_alloc_qos() and lod_sub_declare_create() on OSP objects, so the per-object layout mutex is held far longer than before.

This could also crash if `cl_obd_name` is not allocated?  It would be better to make this a static `char cl_obd_name[OBD_NAME_MAX]` buffer to avoid that.

There are 34 bytes of holes in `struct client_obd` that could be packed a bit better, and this struct is already 3640 bytes in size, so adding a bit more is not _quite_ going to push it over the 4096-byte size.

(style) `obd_name` is a fixed-size buffer, why not use the same instead of allocating a separate buffer?  Otherwise, it seems possible that the import is accessed without `cl_obd_name` being allocated and *that* will crash...

We should check for error here '... || error "nnn"'

This change is correct.

(minor) it would be better to confirm that "FID" had something in it?  Otherwise the "grep didn't find anything" is too easily broken.

Also, add a comment in the code where this "force closing file handle" message is printed to ensure it is not changed.

since it looks like this patch will get refreshed eventually, please also fix this typo -> divide.

truncate

I have a doubt here, if the start of the 1st page is not page aligned, since @to most of time is PAGE_SIZE, so at each loop, the page is always not page aligned as well, is it a glitch here?

Should @to be like this:
       if (from != 0)
              to = min(PAGE_SIZE - from, from + size);
       else
              to = min(PAGE, from + size);
and except the 1st/last page, all pages in the middle should always be aligned full page.

No changes here, code is correct as is.

Hmm, this looks correct - good catch...  But it's blowing up, so probably something wrong I don't see.

Wait, no.  If from != 0, to is still "from + size".  To is always from + size, except when it's outside of the page, in which case it's end-of-page/PAGE_SIZE.

right, I was wrong here.

I'm still investigating why visited page count does not match pv->ldp_count

Thanks, I can't reproduce that bug locally...

from here we can see that the end of the 1st page is page aligned.

and the end of the last page could be page unaligned.

Can we do:

unaligned_dio || skip "Need unaligned dio support"

Instead of server version check?

More importantly, for older servers the unaligned DIO should be handled gracefully in some manner, since applications running on newer clients will try this and we shouldn't allow them to crash the servers. Just skipping the test doesn't solve the interop problem.

One option for UDIO is to expose the DIO alignment via statx, as XFS does. That will allow userspace applications to know what the DIO alignment requirements are, and applications that don't follow them can return EIO or other error directly (as they did befoer UDIO existed, instead of trying to emulate their way through the issues.

typo: write -> write

Will this "wait" for all dd to finish?
As far as I tested, even dd blocked, wait will return without any problem.
I think checking "ps aux | grep dd | grep oflag=sync | wc -l" would be better. And it is necessary to repeat this test for several times. This test can pass for several times (~3) sometimes, but repeating will always hit the stuck.