Gerrit Review Dashboard

(defect) The body still carries the `--WIP--` marker, and says the tool "can currently only be run by the user, but will be adapted to allow integration to sanity-ec" -- but this revision already adds that integration: sanity-ec test_50/51/52 drive the orchestrator directly.

Is this still meant as work in progress, or should the message be rewritten to describe what actually landed? As it stands it reads as a stack of amend notes rather than a description of the change.

(typo) "the user can disabled specific OSTs" -> "can disable".

(defect) Parts of the diff aren't accounted for anywhere in the body, so it's hard to tell what is deliberate:

- sanity-ec test_40b/40c/40d, which are plain `fail_loc` degraded-read tests and don't use the orchestrator at all
- the `EXCEPT_SLOW="50 51"` gate
- the curses TUI in ec_fault_gui.py
- the `--matrix`, `--write-verify` and `--layout-audit` modes
- the report directory machinery (run.info / summary.txt / per-FAIL `lctl dk` + dmesg capture)
- the `--ost-host` ssh routing for multi-node

Also "8 OSTs are required to run" no longer matches the code: 52 needs 6, and 40b/40c need 3. Could the body be brought back in line with the diff?

(minor) "Every fault is read back from the owning OSS before the read is issued" holds for --matrix, but not for --soak, which the same message describes a few lines down as injecting the fault mid-I/O. run_soak_loop() starts the read thread first, sleeps a fraction of the baseline, then arms the fault and only afterwards calls _verify_fault_armed(). Worth rewording to "every fault is read back from the owning OSS" without the ordering claim?

BUILD

(defect) This takes a live OST down with `umount -f`, mounts the raw ldiskfs elsewhere, corrupts a block and remounts -- all outside the test framework. Two concerns:

The remount is a bare `mount -t lustre $dev $mntpt`, so `$OST_MOUNT_OPTS` / `$MOUNTOPT` from the config are lost. `stop ost$n` / `start ost$n` in test-framework.sh handle those (and `wait_osc_import_state`).

Recovery is only a Python `finally`. If the process is SIGKILLed -- an auster timeout, for instance -- the OST stays down and the backend can be left mounted at /tmp/ec_wv_ostbk_N, which breaks every later suite in the run. sanity-ec test_52 registers no `stack_trap` to put the OST back either.

(typo) The bitmask is bits 0-15, not bits 16+. cfs_fail_index() treats any fail_val above 0xffff as bitmask mode and then tests BIT(index) for index < 16, which matches what _apply_fail_loc() builds (0x10000 | bitmask) and what deactivate_ost() logs ("only supports 0-15").

(minor) "At least 8 OSTs configured" is left over from an earlier revision. _soak_random_params(), _wv_geometry() and run_matrix() all scale down to a 2+1 geometry, and the commit message says three OSTs is a real run.

(minor) The MDS is not covered by this. _all_oss_hosts() is built purely from ost.server_host, which comes from --ost-host or the OSC ost_conn_uuid NID, and sanity-ec only passes --ost-host entries. There is no MDS host anywhere in the tool, so on a config where the MDS is a separate node its catastrophe flag is never read.

Either drop the MDS from the claim, or add an --mds-host that sanity-ec fills in from facet_host mds1.

(minor) This readback, _apply_fail_loc() and _clear_fault_params() all take run_on_host()'s default timeout of 10 s, while SSH_OPTS alone allows ConnectTimeout=10. On a loaded OSS the ssh gets SIGKILLed at the 10 s mark and this returns "fail_loc readback failed", which the soak and the matrix then report as an EC failure rather than a transport problem.

run_on_host()'s own docstring says "server-side callers pass generous timeouts" - these three are the ones that don't. The umount/mount and dmesg paths already pass 30-120 s.

(defect) This poll returns True on the first iteration whether or not the OST came back.

`lfs df` prints one line per OST regardless of state: mntdf() synthesises the uuid as "OST%04x" when the statfs failed, and showdf() prints `<uuid>: inactive device` for -ENODATA and `<uuid>: <strerror>` for any other error. Every one of those lines contains "OST", so `grep -c OST` is always OSTCOUNT and the `>= self.ostcount` test is satisfied immediately.

That makes the guard in _wv_corrupt_check() dead, and `lfs mirror verify` can run before the just-remounted OST has reconnected. Would checking that each OST line actually reports space (or polling the osc import state, as wait_osc_import_state() does) work better here?

(defect) A degraded read that hangs is scored here as a correct failure.

_matrix_degraded_read() gets its verdict from _compute_file_hash(), which returns None both when dd exits with -EIO and when run_cmd() kills it on matrix_read_timeout. Either way read_ok is False, so this counts npass += 1.

The comment above _matrix_degraded_read() says a read that hangs instead of returning -EIO "is exactly the regression this mode hunts", but as written the over-tolerance loop cannot fail on it. A handful of hung reads at 120 s each stays well inside the 1800 s budget, so unrun is 0 and the check passes.

Could run_cmd()/_compute_file_hash() distinguish a timeout from a non-zero exit, so a timed-out over-tolerance read is a failure rather than the expected outcome?

(minor) "stop handing out EC layouts" doesn't quite match the code. mdt_pattern_types is only read in mdt_connect_internal(); nothing in mdt/ or lod/ consults it when a layout is created, so an MDT with parity removed will still create EC layouts on request.

What it stops is clients *using* parity components, and only for clients that mount afterwards. Worth noting too that the negotiated mask does not gate creation on the client either: with lustre.enable_flr_ec=0 (the default) a client can still create an EC file via llite.*.enable_erasure_coding and then get -EIO writing it. Should layout creation be gated on the negotiated mask as well?

This baseline omits LOV_PATTERN_COMPRESS, but lov_pattern_supported() right below still lists `LOV_PATTERN_RAID0 | LOV_PATTERN_COMPRESS` as a pattern this client's IO stack handles.

So for a compressed component lov_lsme_usable() -> lov_pattern_available_mds() computes

    pattern_base = 0x801
    0x801 & ~LOV_MDS_PATTERN_SUPPORT_217 = 0x800   /* != 0 */

and returns false, on both branches: the compat branch uses this macro, and the negotiated branch uses the MDS mask, which is capped by MDT_PATTERN_TYPES_SUPPORTED = baseline | PARITY. There is no value of mdt.*.pattern_types that can put COMPRESS back (conf-sanity test_163a asserts `+compress` is silently dropped).

The result is that lov_init_composite() leaves every compressed component !lle_valid and skips lco_init(), so lov_io_rw_iter_init() returns -EAGAIN/-EIO for a plain compressed file. This is reachable today: lsme_unpack() accepts COMPRESS components when llite.*.enable_compression is set (lov_ea.c), and lod_generate_lovea() packs them.

Should the mask be derived from what the client actually supports (i.e. include COMPRESS in both the 2.17 baseline and MDT_PATTERN_TYPES_SUPPORTED), rather than a hard-coded RAID0/MDT/OVERSTRIPING set?

(style) F_HOLE and F_RELEASED are layout flags rather than pattern types, and every consumer strips them first (`pattern & ~LOV_PATTERN_F_MASK` in lov_pattern_available_mds(), `& ~(F_RELEASED | F_MASK)` in lov_pattern_supported()). lov_pattern_bit2str() also has no names for bits 30/31, so they never show up in mdt.*.pattern_types or lov.*.mds_pattern_support.

They only put two bits nobody reads on the wire in ocd_pattern_support. Could the macro just be RAID0 | MDT | OVERSTRIPING?

(minor) `data` here comes from obd_get_info(KEY_CONN_DATA) on sbi->ll_md_exp, which is the LMV export, and lmv_get_info() forwards that key to `lmv_tgt(lmv, 0)` only.

So on DNE the stored mask is whatever MDT index 0 negotiated; mdt.<fs>-MDT0001.pattern_types has no effect on any client, even though the parameter is per-MDT and the man page shows a single-MDT example. Should the client intersect the masks from all MDTs, or should the caveat be documented?

(minor) This version gate was flagged on an earlier patchset ("this version should be updated when the patch is refreshed") and answered "Done", but it is still 2.17.52.224 here and in test_163a/test_163b.

The things these tests check (`pattern_support` in the mdc/osc import, mdt.*.pattern_types) only exist from this patch, so any server in 2.17.53..2.17.57 passes the gate without the feature. The tests do fall through to a skip on the missing parameter, but the gate should be the version this lands in.

(minor) The body only describes `client_limit`. Some of the diff isn't accounted for: the new read-only `client_count` parameter (four man pages plus mdt/ofd sysfs attrs), the per-nodemap `client_limit`/`client_count` files, the `class_exp2tgt()` -> `class_obd2tgt()` split, the `exp` local added to target_handle_disconnect(), and the CERROR reformat in lustre_fill_super(). Could the message name each new parameter, say what the nodemap-level limit is for, and split out the unrelated cleanups?

"Server to server connections are unaffected" is also not quite right on the MDT: MDT-MDT connections are exempt from being refused, but they are still part of the count the limit is compared against.

(defect) this doesn't hold on the MDT: MDT-MDT exports are part of `lut_num_clients`, which is what `client_count` prints. On an OST the value is always 0 because nothing increments that counter there.

(style) the description paragraph runs on from the `Config` `.TP` entry, so it renders as part of it - it needs a `.PP` to break out of the list.

(defect) the nodemap parameter is read-only, so this `set_param` returns an error; nodemap properties are normally set with `lctl nodemap_modify`. The 644 permissions listed below don't match the RO fops either, and "Present on MDS and OSS nodes" over-promises given that the OST-side limit never triggers.

(style) `is_server_connection()` is a very generic name for a global in a widely included header; something like `target_is_server_connection()` would be less likely to clash. The argument could be `const struct obd_connect_data *`.

(style) plain `int` is the spelling used elsewhere in this struct; `signed int` stands out. The two limits also test differently - the nodemap check uses `!= -1` and the target check uses `>= 0` - so picking one form for both would settle what a value like -5 means.

(defect) `lut_num_clients` includes server-to-server exports, so exempting them here only stops them from being refused - they still consume the limit. lod_lov.c:193 requests `OBD_CONNECT_MULTIMODRPCS` on the MDT-MDT OSP connection and mdt_handler.c:7265 grants it, so tgt_client_new() counts those exports (tgt_lastrcvd.c:1137). On a 4-MDT filesystem each MDT starts at client_count = 3, so `client_limit=N` admits N-3 real clients.

This also runs ahead of the `obd_uuid_equals(&cluuid, &target->obd_uuid)` "lctl gets a backstage, all-access pass" bypass, so a self-UUID/administrative connection is refused as well once the target is at its limit. Intended?

(defect) On an OST this counter never moves, so `obdfilter.*.client_limit` cannot work. tgt_client_new() only does `atomic_inc(&tgt->lut_num_clients)` when `tgt_is_multimodrpcs_client()` is true, and ofd_obd.c:131 masks the connect flags with OST_CONNECT_SUPPORTED, which does not include `OBD_CONNECT_MULTIMODRPCS` (only the MDT grants it, mdt_handler.c:7265).

So `obdfilter.*.client_count` always reads 0: any positive limit never fires, and `client_limit=0` refuses every client. Should the OST side count exports instead?

(defect) obd_connect() has already added this export to the nodemap by now (mdt_obd_connect() -> nodemap_add_member() -> nm_member_add()), so `count` includes the client currently connecting. With `nm_client_limit = N` the check fires on the Nth client, so only N-1 are admitted.

The rejection path also looks incomplete: the export created by obd_connect() holds a last_rcvd slot (tgt_client_new()) and a nodemap membership, and `out:` only does class_export_put() - no obd_disconnect()/class_disconnect(), unlike the -ENODEV path a few lines below. Doesn't each refused client leave a stale export holding a `lut_num_clients` and `nm_client_count` slot until the ping evictor reaps it?

The `BUG: spinlock bad magic` trace posted on patchset 31 points at this call (`nodemap_get_from_exp+0x118 <- target_handle_connect`, thread `ll_mgs_0002` during sanity-sec setup). This call site is byte-identical in patchset 32, so it doesn't look addressed yet.

One place to check: mgs_init_export() returns at mgs_handler.c:798, before `spin_lock_init(&exp->exp_target_data.ted_nodemap_lock)`, for an export whose client UUID equals the target UUID - that is the `dont_check_exports` connect, which reaches this check with `export` non-NULL. nodemap_get_from_exp() takes that lock only when nodemap_active is set, which fits a failure that appears in sanity-sec.

(minor) any negative value is accepted, and the two enforcement sites disagree on what one other than -1 means. Rejecting `val < -1` with -EINVAL would keep the documented -1 as the only "unlimited" value.

(typo) is_server_connection() is defined in lustre_net.h, not ldlm_lib.c.

(defect) Both files are `LDEBUGFS_SEQ_FOPS_RO` and there is no other writer - `nm_client_limit` is only ever assigned in nodemap_inherit_properties(), so it stays -1 for the life of the nodemap. Without a `lctl nodemap_modify` property (and the matching IAM/llog persistence the other nodemap properties have), the enforcement block in target_handle_connect() can never run, and `nodemap.client_limit.4` documents a `lctl set_param` that fails.

(minor) The ll_readahead_handle_work() piece fixes a pre-existing bug that has nothing to do with EC: ll_ra_count_get() reserves the pages and nothing puts them back when cl_io_rw_init() fails, and ll_ra_count_put() is the only decrement of ra_cur_pages. A Fixes: line would let the maintenance branches pick it up:

    Fixes: c2791674260b ("LU-12043 llite: improve single-thread read performance")

(defect) The lov_io_set_range() hunk fixes an LBUG that 84c1a4a074 introduced, and that commit is three patches back in this same unlanded series. That leaves 84c1a4a074, 13af17f231 and b718cadc7b each panicking the client on a two-component EC layout, so the series is not bisectable and those revisions cannot be tested on their own.

Can the clamp be folded into 84c1a4a074 instead? If it has to stay a separate patch, it needs its own tag alongside the existing one:

    Fixes: 84c1a4a07423 ("LU-12669 ec: recover data from parity")

Good catch.  Definitely best to check for the inactive OSC device early.  This would be set some seconds after the OST goes offline, so IO shouldn't wait to detect that every time.

Max, I rebased this on Bobi's latest patch - you were based on an older version

(minor) This label bypasses cl_io_fini(). cl_io_init()'s contract is that the caller calls cl_io_fini() no matter what it returned, and the commit message points out that this exit stops being rare once a dead import can fail an EC read at init. Should the new path run cl_io_fini(env, io) before dropping the reservation?

(defect) On a component with lsme_dstripe_count == 0 this can push eoff well past lio->lis_endpos.

Neither place that sets the cycle end rounds it to a recovery group on that branch: lov_io_set_range() skips its end-rounding block when dstripe_count == 0, and lov_io_ec_rd_iter_init() only rounds when dstripe_count > 1. So lis_endpos is just the request end, while eoff becomes soff + RGs, and RGs there is ss * 4 (or ss * lo_nr).

lov_ec_read_stripe_pages() classifies pages against eoff alone, so the pages in [lis_endpos, soff + RGs) are grabbed and submitted. lov_io_lock() enqueued only [ec_inner.crw_pos, +crw_bytes) for this cycle, and lov_ecio_add_data_sub() skipped those stripes (lov_stripe_intersects() false against the cycle extent), so lov_sub_get() allocates a fresh sub-IO with no DLM lock -- the same "uncovered page!" LBUG in osc_req_attr_set() that the lov_io_set_range() hunk is fixing.

Worked example, layout "-E 128M --ec 4+2 -E 512M -c 4" (EC component followed by a plain one, as in the mirror layouts in this suite), read [0, 130M) with a dead OST:

    cycle in the plain component: lis_pos 128M, lis_endpos 130M
    dcount 4, ss 1M => RGs 4M, soff 128M
    eoff was min(132M, 130M) = 130M, now min(132M, 512M) = 132M
    stripes 2 and 3 cover [130M, 132M) -- outside the enqueued lock

Before the change those pages were EC_DPG_ZERO and never touched. Should the clamp stay bounded by lio->lis_endpos on the dcount == 0 branch, where there is no recovery group to complete?

(minor) The sibling switch below clears the retry count before returning:

    io->ci_switch_ec_io = 1;
    io->ci_need_restart = 1;
    io->ci_ndelay_tried = 0;
    RETURN(-ENODATA);

This one leaves ci_ndelay_tried alone, and ll_file_io_generic() carries it across the restart (retried = io->ci_ndelay_tried). So if the read had already restarted once before the import went invalid, the CIT_EC_RD pass lands on ndelay_tried: with a non-zero count and can still take the 10 ms schedule_timeout_interruptible() and set ci_tried_all_mirrors -- the backoff the commit message says this path avoids. Should it reset the count too?

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

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

[AI review - fable] (defect) Including CIT_READ here looks dangerous. After the continue, the top lock is granted without covering this stripe, and nothing in the normal read path checks lso_status (only the EC_RD and LSEEK paths do), so pages on the dead stripe are still generated and submitted, and osc_build_rpc() -> cl_req_attr_set() -> osc_req_attr_set() hits the "uncovered page!" LBUG in osc_object.c when osc_dlmlock_at_pgoff() finds nothing - the very crash the commit message wants to avoid.

The lov_io_mirror_init() check does not guard this path when ci_cross_ec is unset (a read of a non-EC component of a file whose layout has parity entries elsewhere - lov_lsm_has_parity() is file-wide, and lov_io_ec_rd_start() explicitly supports dcount == 0 components), or when the import goes inactive after cl_io_init(). Before this change the enqueue failed fast with -ESHUTDOWN and the IO returned an error or restarted into EC_RD instead of crashing.

Should the skip be limited to CIT_EC_RD, letting CIT_READ fail the enqueue so the existing restart logic switches to EC_RD? As a side effect the CIT_READ marking also leaves a stale LSS_READ_ERR on the lovsub (the normal read path never resets it), which lov_io_lseek_end() then treats as degraded even after the OST is reactivated.

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

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

(style) This isn't a bug, but the include groups are inverted here - `<lustre_osc.h>` is a lustre header and belongs before the local `"lov_cl_internal.h"`. The same include added to lov_io.c in this patch is ordered that way.

(style) The summary reads as past tense; house style is imperative. "update mirror split for EC support" would match the rest of the tree.

(minor) The body calls out `test_44f` and the `test_7b` rewrite by name, but sanity-ec `test_7e`, `test_7f`, `test_7g` and the new `identify_ec_mirrors()` helper (~240 new lines) aren't mentioned anywhere. Worth a sentence so a reader knows the new coverage is intentional and what it exercises.

@mvef@whamcloud.com, @rsahlberg@whamcloud.com, I see that `--force-no-ec` is listed in LU-19548, but I don't recall what the realistic use case is for that option? 

Leaving an unanchored EC mirror behind on a file doesn't seem very useful, and there doesn't seem to be any way to reconnect it to a new data mirror even if it was created (nor assurance that the OSTs used in the data mirror are suitable for that EC).

(Apologies in advance to Max, but ...) I'm wondering if `--force-no-ec` option should be removed, and EC mirrors should always be moved to the victim file or deleted?  That would remove a lot of complexity in the code that I doesn't seem to add any real value.

(style) The .TH date is older than the lfs-mirror-delete.1 date set in the same patch, and predates the current content. Worth bumping to the refresh date.

(minor) The victim now gets two mirrors, and lod_declare_layout_merge() rejects any merge source that has more than one:

    if (le16_to_cpu(merge_lcm->lcm_mirror_count))
        RETURN(-EBUSY);

So `lfs mirror extend -N -f NEW_FILE` (documented in lfs-mirror-extend.1) fails with EBUSY on a file produced this way, unlike every other split victim. Should this paragraph say so, until extend learns to merge a data+parity pair?

(style) function comment should follow the style as `mdd_split_ea`.

(question) For a data+parity victim, if either side was *STALE* on the source, should the new file always set data *in-sync* and parity *STALE*? The victim has no other data mirror, so data has to be the non-stale primary or the layout is unusable (all-stale is -EPERM; in-sync parity as primary is -EUCLEAN). Only when both were already in-sync should both stay in-sync. Can current mask accomplish this or not?

(defect) llapi_mirror_find_stale() skips LCME_FL_PARITY, so a remaining
parity mirror always yields comp_size == 0 and this returns false.
lfs mirror split/delete then skips the last-good-copy resync even when
every remaining data mirror is stale.
Should remaining parity mirrors be ignored here the same way
data_mirror_remains() ignores them? Co-split also takes the paired
parity, so that parity is not a copy that will survive the split.
Example: file has D1 (EC data, in-sync after a write), P1 (parity),
and D2 (data, stale).
  lfs mirror split --mirror-id D1
last_non_stale_mirror(D1) sees remaining mirrors {P1, D2}. P1 is
skipped by find_stale, so comp_size == 0 and the function returns
false. Resync is skipped, co-split removes D1+P1, and the source is
left with only stale D2.

(minor) The comment is narrower than the condition. `mflags & MF_DESTROY` with a NULL victim_file is also true for `lfs mirror split -d` (which cannot take -f at all), for `lfs pcc detach` (`MF_DESTROY | MF_FOREIGN`), and for the internal `mirror_split(name, 1, NULL, MF_DESTROY, NULL)` call. Naming only `lfs mirror delete` makes the flag look narrower than it is.

(minor) mirror_split() also backs `lfs mirror delete` (SO_MIRROR_DELETE sets MF_DESTROY) and `lfs pcc detach`, so a user who typed `lfs mirror delete` is told the mirror is being "split". sanity-ec test_7f greps for this text from a `lfs mirror delete` run, so that wording is on a tested path.

Also, every other message in this function is prefixed with `progname`; this one isn't, so in a pipeline it isn't obvious which tool emitted it.

Skipping the orphan parity comp leaves `comp_array[i].lrc_synced` at the value `_mirror_find_stale()` set, which is unconditionally `true`:

    comp[idx].lrc_synced = true;   /* liblustreapi_layout.c */

Back in lfs_mirror_resync_file() every entry with `lrc_synced` is fed to LL_LEASE_RESYNC_DONE, and lod_declare_update_sync_pending() then does `llc_flags &= ~LCME_FL_STALE` for each id.

So `lfs mirror resync` returns success and drops LCME_FL_STALE from a parity component it never recomputed - `lfs getstripe` afterwards shows it as in sync. Would `comp_array[i].lrc_synced = false;` before the `continue` be more honest here?

(suggestion) The body says an interop run skips the new subtests rather than failing them, but nothing here exercises that. Would adding a second line such as `Test-Parameters: testlist=sanity-ec ostcount=8 serverversion=2.17.55` prove the version gate actually skips against an older MDS?

(minor) The extra storage for EC(D+P) is PARITY/DATA of the file size, not (DATA+PARITY)/DATA - (DATA+PARITY)/DATA is the total. The 8+2 example further down gets this right ("1.25x the file size, vs 2x for a plain mirror"), so the two read as contradicting each other. Reword as total storage, or use PARITY/DATA here?

(defect) Should this refuse a data component that is already linked, instead of overwriting its link id?

`data_lcme->lcme_time_and_id` may already carry the mirror id of an existing parity mirror. The old id is dropped here and replaced with the newly merged one, so the earlier parity mirror ends up half-linked from the data side - the exact state the `!link_id` check a few lines up refuses.

The only guard against that today is client-side, in `build_parity_layout_for_mirror()`, and it is not race-free: `llapi_layout_get_by_path()` reads the layout before `mirror_extend_layout()` takes any lease, so two `lfs mirror extend --mirror-id N --ec D+P` runs on different clients both see mirror N unprotected and both merge. The second merge re-stamps the data component; `verify_new_mirror_id()` then reports -EBUSY, but the layout has already been committed. `llapi_layout_comp_add_parity_only()` is also a new public, man-paged entry point, so any application can build the same merge buffer directly.

The result is a file with two parity mirrors on one data mirror, which the commit message says is rejected for now. `lod_verify_striping()` already re-checks the k+m bound with "Cannot trust userspace; enforce here too" - the same reasoning seems to apply to the link.

    if (lcme_timestamp_id_unpack(time_and_id))
            GOTO(out, rc = -EEXIST);

This needs to be fixed before the EC feature goes into production, but is still under discussion and not the target of this patch.

(minor) `llapi_layout_pool_name_get()` never sets ENODATA - it snprintf()s `comp->llc_pool_name` (empty when there is no pool) and returns 0, failing only on a NULL current component, a NULL `dest`, or a foreign pattern (EINVAL). So the `errno != ENODATA` carve-out never triggers and the comment describes behaviour the API doesn't have. If it stays, the non-ENODATA failure leaves `rc` at -1 until the next assignment overwrites it, which is easy to trip over later.

Going forward, all of these lines should be replaced with a label:

 Assisted-by: ClaudeCode:MODEL_VERSION [TOOLNAME ...]

https://wiki.lustre.org/Commit_Comments#AI/LLM/Tool_Attribution

Why is this for `fortestonly`? Should this be consolidated with another patch, or that annotation be removed?

Probably it was generated initially via AI, and nobody removed that designation as the patch was being updated?

(typo) The third path is `lov_io_lseek_end()`, which is the `.cio_end` entry in `lov_io_ops[CIT_LSEEK]` and runs from `cl_io_end()`, not from unlock. `lov_io_unlock()` is a separate op. Should this read "during sub-IO end"?

[Marc Bot] (style) This attribution line was flagged on patchset 34 and is still unresolved: it should use the `Assisted-by:` label format described at https://wiki.lustre.org/Commit_Comments#AI/LLM/Tool_Attribution instead of the free-form line.

"three independent paths" does not seem to hold for the third one.

The LSEEK sub-lock enqueue happens in cl_lockset_lock(), which cl_io_lock() runs *after* every cio_lock(), so the stripe is already marked by the time lov_io_call(cl_io_start) runs and the sub-IO is skipped there. A sub-IO that never started still has:

    sub_io->ci_result = 0            /* lov_io_sub_init() */
    sub_io->u.ci_lseek.ls_result = -ENXIO   /* inherited from the parent in lov_io_sub_inherit(); ll_lseek() seeds it */

lov_io_lseek_end() already ignores both (`ci_result == 0` is a no-op, `sub_off == -ENXIO` hits the existing continue), so there is nothing for the third hunk to catch.

Also, ci_result propagation happens in .cio_end (lov_io_lseek_end), not during unlock.

(style) This was raised on an earlier patchset and the line is unchanged: tool attribution should use the `Assisted-by:` trailer format documented at https://wiki.lustre.org/Commit_Comments#AI/LLM/Tool_Attribution rather than a free-form sentence.

This guard sits in `lov_io_call()`, which is the shared dispatcher for four different ops:

    lov_io_lock()      -> lov_io_call(cl_io_lock)
    lov_io_start()     -> lov_io_call(cl_io_start)
    lov_io_iter_fini() -> lov_io_call(lov_io_iter_fini_wrapper)
    lov_io_unlock()    -> lov_io_call(lov_io_unlock_wrapper)

All four are registered for CIT_LSEEK, so a stripe marked LSS_READ_ERR also skips `cl_io_unlock()` and `cl_io_iter_fini()` on its sub-IO, not just `cl_io_start()`. Its `ci_state` then goes CIS_LOCKED -> CIS_IO_FINISHED (set by `lov_io_end_wrapper()` in `lov_io_lseek_end()`) -> CIS_FINI, never passing through CIS_UNLOCKED/CIS_IT_ENDED.

Nothing leaks today because `osc_io_ops[CIT_LSEEK]` registers only cio_start/cio_end/cio_fini and the LSEEK DLM lock is taken on the top IO by `vvp_io_lseek_lock()`. But the intent is only to skip the data-fetch pass -- would putting the check in `lov_io_start()` (or keying it on `iofunc == cl_io_start`) keep the cleanup passes balanced?

(style) This isn't a bug, but `str` reads like a string; the rest of this file spells it `stripe` (see `lov_io_lseek_end()` a few hundred lines down, which uses `index`/`stripe` for the same two values). Worth matching if the patch is refreshed.

[Marc Bot] (defect) Skipping the stripe treats its extents as holes, but on an EC file that data is still readable via parity reconstruction. If the only data between ls_start and the next healthy-stripe data lives on the degraded stripe, SEEK_DATA returns the later offset, or -ENXIO if none, so sparse-aware tools (cp, tar) silently drop data that read() would return.

Also, if every sub-IO in lis_active is skipped (e.g. single-stripe data component), offset stays -ENXIO and SEEK_HOLE fails with -ENXIO even though ls_start < file size, which breaks the SEEK_HOLE contract of a virtual hole at EOF.

Would it be safer to treat a degraded stripe's covered range as data instead of skipping it, or to return an error rather than a misleading data map? The commit message claim that the remaining healthy stripes provide valid seek results does not hold in these cases.

(defect) Skipping cl_io_start() for the degraded stripe means its allocation map never contributes to the seek result, and lseek does not reconstruct anything from parity the way CIT_EC_RD does. So the answer is not "the remaining healthy stripes are sufficient" -- it is an answer computed from an incomplete extent map.

Concrete case, 4+2 EC, one data OST deactivated, data written only in the range that maps to that stripe:

    lseek(fd, 0, SEEK_DATA)
      -> degraded sub skipped, others report -ENXIO
      -> offset stays -ENXIO, lseek fails

and with data further out on a healthy stripe it returns that later offset instead. Sparse-aware copies (cp --sparse, tar, rsync) would silently drop the bytes that a plain read() still returns via parity.

SEEK_HOLE has the mirror problem: if every sub-IO covering ls_start is skipped, `offset` stays -ENXIO and ll_lseek() returns -ENXIO for an offset below i_size, which breaks the "there is always a virtual hole at EOF" contract.

Is returning an error preferable to returning a wrong offset here? Alternatively, could the degraded stripe's range be reported as data (conservative) rather than dropped?

+1; error should be preferable compared with a wrong offset

(typo) This comment uses a non-ASCII em dash; the rest of the tree is plain ASCII. Plain "-" or "--" instead.

This hunk looks like it has no effect. Any stripe marked LSS_READ_ERR was already skipped by the new check in lov_io_call(), so its sub-IO never reached cl_io_start(): ci_result is still 0 from lov_io_sub_init(), and ls_result is still the -ENXIO that lov_io_sub_inherit() copied from the parent. The `if (io->ci_result == 0)` assignment and the `sub_off == -ENXIO` continue below both already handle that.

Is there a path where a sub-IO is marked LSS_READ_ERR but still ran? If not, dropping this hunk would keep the two skip conditions from having to stay in sync.

(minor) The range here starts at 49b, but 49a "test concurrent reads during EC recovery" is added by this patch too. Should it read 49a-50b? Every other test added is accounted for by one of the ranges.

(minor) Two hunks are not accounted for by any of the ranges above: the new `[[ "$SLOW" == "no" ]] && EXCEPT_SLOW="74a 75b"` line, and the `head -n1` fix in `enable_ec()` for the multi-mount `get_param` output.

The `EXCEPT_SLOW` one also changes an existing test: 74a is not added by this patch and is not named in any of the range lists, so a reader can't tell from the message that it is being moved onto the SLOW list. Worth a sentence for each.

(defect) With 3 of 4 data stripes unreadable and pcount=2, -EIO is the only correct outcome; returning reconstructed-but-wrong bytes is silent corruption. Treating it as an acceptable pass means a real reconstruction bug in this path would go unnoticed.

48a, 48c and 64b in this same patch take the opposite position ("handing back silently wrong data would be a corruption bug, so assert against it"). Should 52b assert rc != 0 instead?

(minor) These eight files (2M..16M) are never removed. The same applies to the extra files in 71b/71c/71d, 73b's .pure, 73c's .zero and the directories in 75e/75f; 73e is the only new test that registers a cleanup.

Added up, the new tests write on the order of 1.5G and leave most of it in place for the rest of the run, which on a small test filesystem can push later subtests into ENOSPC. A `stack_trap "rm -f ..."` next to each creation would keep the footprint bounded.

(suggestion) Both of these are parked against the feature ticket itself. 53a in particular is described as intermittently reconstructing wrong data, which is a silent-corruption symptom rather than a test problem, and 41d is an OSC retry loop that never terminates.

Would separate LU tickets referenced here keep them from being lost when LU-12668 is closed? The convention elsewhere in this file (12a -> LU-19631, 5b/12b -> LU-20435) is one ticket per known failure.

(minor) A fresh `stack_trap` is pushed on every call, including repeat calls for the same index, so loop-driven tests accumulate identical cleanup entries: 75b registers 50, 74a 20, and 58a/63b/68b one per pair.

Each entry re-runs `ec_apply_fault`, which is a `do_nodes` to every OSS plus a `cancel_lru_locks osc`, so teardown does that work dozens of times over. Registering the trap only when the bit was not already set would make it one entry per OST.

(minor) When no stripe list is given this walks every data stripe and returns the first parity-free one, which can be a stripe that holds no data.

For a sub-raid-set file that is the vacuous-pass mode this helper is documented as preventing: 42a writes 512K into a 1M-stripe 4+2 layout, so only stripe 0 has data. If stripe 0's OST happens to double as parity, the fault is armed on stripe 1's (empty) object and the checksum comparison succeeds without reconstructing anything.

`ec_data_stripe_osts()` handles the analogous case by calling `skip_env`. Would returning non-zero (so `ec_start_read_fault()` skips) be safer than falling through to a later stripe?

(minor) 44c and 44a are the same test - same layout, same `ec_start_all_reads_fail`, same EIO check, same clear-and-reread - differing only in the error strings. 43c is that body minus the reread. Could these collapse into one?

While here, 43c's description says "too many OST failures (3+ OSTs)" but `ec_start_all_reads_fail` uses `fail_val=0`, which fails every OST, not three.

(minor) The comment says "an OST that is a safe data OST for all files", but only `$f2` is classified. `$f1` (2+1, 3 objects) and `$f3` (2+2, 4 objects) get whatever the allocator gave them, so on an 8-OST config the victim frequently holds none of their objects and the `$s1`/`$s3` comparisons pass without any recovery running.

71a, 71c, 71d and 75e/75f have the same shape (classify one file, assert on all of them). That is defensible for a batch test, but here the comment claims something stronger than the code does.

(defect) `safe_osts` here still comes from the `ec_classify_osts $tf` above, but the SEEK_DATA/SEEK_HOLE checks below run on `$tfs`, which was created separately and gets its own object placement from the allocator.

So the OST taken out need not hold any of `$tfs`'s stripes, and on a run where it doesn't, the two `lseek_test` assertions execute against a fully healthy file.

73c already documents and avoids exactly this ("safe_osts still describes $tf; $tf2 has its own object placement") by calling `ec_data_stripe_osts $tf2 0` first. Should 75d do the same for `$tfs`?

Related: the comment says the seek "crosses the degraded stripe", but the data lives at 5M, i.e. stripe 1 with `-c 4 -S 1M`, while `safe_osts[0]` is just the lowest-numbered data OST.

(minor) The negative-index handling, and the "Index -1 is the last stripe" note in the header comment, appear to be unreachable: all eleven callers pass 0..4. Worth dropping the branch and the doc line unless a caller is coming.

(style) The suite convention is a `#define` comment naming the fault right above the line that arms it, so a reader does not have to look up the bare hex. `ec_start_all_reads_fail()` and 41d both do this; this call site and the one in `ec_apply_fault()` do not.

    #define OBD_FAIL_OST_BRW_READ_BULK 0x20f
    ec_ost_fail_loc 0x20f $(( 0x10000 | mask ))

The value itself is right (obd_support.h has 0x20f), it is only the annotation that is missing.

(minor) ... so it could also be set and checked via `chattr +t FILE` and `lsattr FILE` commands from e2fsprogs.

The "Changes:" list mentions adding LUSTRE_NOTAIL_FL to LUSTRE_FL_USER_MODIFIABLE, but the same hunk also adds LUSTRE_COMPR_FL to that mask and removes the duplicate LUSTRE_NOATIME_FL entry. Neither is explained. The COMPR change is an independent server-side behavior change - should it be split into its own patch?

The lustre/utils/lfs.c hunk (skipping the range validation for nohybrid) isn't described either.

This changes UAPI flag definitions that the MDT interprets, so interop with an older server is worth an explicit test run. Consider adding something like:

    Test-Parameters: testlist=sanity serverversion=2.16.0

New wire flag, but wirecheck.c and the two wiretest.c copies don't look updated - the neighbouring LUSTRE_*_FL values have CHECK_VALUE_X entries. Same for LU_LADVISE_NOHYBRID, which needs a CHECK_VALUE next to the other LU_LADVISE_* ones (LU_LADVISE_AHEAD seems to have been missed earlier too).

The compatibility claim points at the wrong side. Older clients aren't the problem; older servers are.

On a pre-patch MDS, LUSTRE_FL_USER_VISIBLE has no NOTAIL bit, so mdt_setattr_unpack() hits

    if (rec->sa_attr_flags & ~LUSTRE_FL_USER_VISIBLE)
            RETURN(-EOPNOTSUPP);

and lfs ladvise -a nohybrid fails with EOPNOTSUPP. Clearing the flag still "succeeds" as a no-op, so the two directions behave differently. Worth stating the required server version here.

This was asked on patchset 7 and looks unaddressed: adding LUSTRE_COMPR_FL here is unrelated to nohybrid and changes server behavior.

mdt_setattr_unpack() masks with LUSTRE_FL_USER_MODIFIABLE, and osd_attr_set() replaces the whole masked set, so this makes the compression flag both settable and clearable on the MDT inode by any client. Is that intended ahead of the compression work landing?

(defect) why is NOATIME being removed?

Is the COMPR flag really user modifiable or just visible?  And does it make sense to allow this to be set before CSDC is landed to master?

Should this use NOHYBRID?

Does this also set the flag directly on the inode, or is the inode here the root or parent directory?

parse ll_file_ioctl():error: Function too hairy.  Giving up. 4 seconds
warn: ll_file_ioctl():Function too hairy.  No more merges.

ll_inode2ext_flags() is not a full picture of the file's flags - it rebuilds them from inode->i_flags via ll_inode_to_ext_flags(), which only knows SYNC/NOATIME/APPEND/DIRSYNC/IMMUTABLE/ENCRYPT, plus the PROJINHERIT and (new) NOHYBRID lli_flags bits.

LUSTRE_NODUMP_FL and LUSTRE_NOCOMPR_FL are in LUSTRE_FL_USER_MODIFIABLE but have no i_flags or lli_flags representation, so they come back as 0 here. osd_attr_set() then does a wholesale replace:

    ei->i_flags = (ei->i_flags & ~LDISKFS_OSD_USER_MODIFIABLE) |
                  (attr->la_flags & LDISKFS_OSD_USER_MODIFIABLE);

so those bits get cleared on disk. chattr +d FILE followed by lfs ladvise -a nohybrid FILE should lose the 'd' flag.

The FS_IOC_SETFLAGS path avoids this by calling fileattr_get() first, which fetches body->mbo_flags from the MDT. Should this do the same before OR-ing in LUSTRE_NOHYBRID_FL?

Related: ll_set_project() builds op_attr_flags from ll_xflags_to_ext_flags() and also sets OP_XVALID_FLAGS, so lfs project on a file appears to clear the nohybrid flag for the same reason.

That seems like a pretty big hammer, basically breaking AIO completely for EC files, rather than just the recovery path?  Or is the comment wrong and this is triggered only for EC recovery when `-EIOCBQUEUED` is returned?

Would it be better to push **all** EC recovery to a workqueue instead of keeping it directly in the IO path, then it can fire the AIO completion when the reconstruction is complete, and synchronous readers would wait on the completion?

Andreas, yes, correct. This is essentially making AIO synchronous for all reads on EC files because we can't tell at submission time if we need reconstruction. So, this patch is just a stopgap to allow AIO to work for degraded reads, albeit with collateral damage.

I'd agree that a workqueue is the better long-term approach, however, I'm not familiar enough with the code path at the minute here to gauge the effort required. So, I'd keep this patch as a short-term fix for AIO for degraded reads, and open a ticket so we address this properly with a workqueue.

Do you agree? If yes, I open the ticket

(minor) aio_ec_sync deliberately survives the goto restart at the end of the function, and it has to. cda_no_aio_complete and cda_creator_free stay set on the cl_dio_aio across restarts, so a later pass that ends with anything other than -EIOCBQUEUED (cl_io_rw_init() failing, or cl_io_loop() returning a hard error) still needs both !is_aio || aio_ec_sync tests to be true.

If it were cleared at restart, __cl_sync_io_note() would skip the free because creator_free is 1, and the creator branch would skip it too, leaking the cl_dio_aio and its cda_obj reference.

This isn't a bug as written, but the coupling between a stack bool and two flags on a heap struct isn't obvious. Could the comment above say the flag is intentionally sticky, so it doesn't get reset in a later cleanup?

on refresh