kg.xu keguang · review.whamcloud.com
Needs your action
3 new · 7 longstanding
69703
master
J –B ✓T ✗5R 0/2
LU-20690 ec: verify parity against data, allow p > k
enforced failed: custom-1001, review-dne-part-7, review-dne-subtest-change, review-dne-zfs-part-7, review-dne-zfs-subtest-change
pt_ecroraid10-like218_review_prioritypw_shepherd
4 unresolved
L +338 −193
49m
LU-20690 ec: verify parity against data, allow p > k
enforced failed: custom-1001, review-dne-part-7, review-dne-subtest-change, review-dne-zfs-part-7, review-dne-zfs-subtest-change
pt_ecroraid10-like218_review_prioritypw_shepherd
4 unresolved
L +338 −193
49m
enforced failed: custom-1001, review-dne-part-7, review-dne-subtest-change, review-dne-zfs-part-7, review-dne-zfs-subtest-change
4 unresolved thread(s) await your reply — last from Keguang Xu
| failed enforced test | platform | detail | |
|---|---|---|---|
| custom-1001 | RHEL 9.7 / x86_64 | ran 3 tests. 1 tests failed: sanity-ec. | session |
| review-dne-part-7 | RHEL 9.8 / x86_64 | ran 6 tests. 1 tests failed: sanity-ec. | session |
| review-dne-subtest-change | RHEL 9.7 / x86_64 | ran 3 tests. 1 tests failed: sanity-ec. | session |
| review-dne-zfs-part-7 | RHEL 9.8 / x86_64 | ran 6 tests. 1 tests failed: sanity-ec. | session |
| review-dne-zfs-subtest-change | RHEL 9.7 / x86_64 | ran 3 tests. 1 tests failed: sanity-ec. | session |
build #135173 SUCCESS · tests all Maloo sessions
no vote yet: Ronnie Sahlberg, Marc Vef, Andreas Dilger
patchsets:
PS1 10-15
PS2 10-15
PS3 10-26
PS4 11-02
threads: 4 need your reply · 0 waiting on others · 0 own notes · 0 bot
reply Keguang Xu lod_lov.c:2242 — Thanks for catching this.
Gerrit AI review for Lustre
(defect) Can the MDS create a parity that this check then rejects?
With more than one raid set, lod_stripe_count_min() returns 0 for a parity, and lod_ost_alloc_rr() then reports success on a short allocation:
lod_ost_alloc_rr()
if (stripe_idx) rc = 0;
lod_ec_add_parities() /* may still be short, e.g. failure domains */
/* Still short; remember how many we got */
lod_comp->llc_stripe_count = stripe_idx;
stripe_idx < stripe_count_min (0) -> no -ENOSPC
lod_comp_prep_create() does not check the count afterwards, so the layout is stored with fewer than (n0 + n1) * p stripes. If the shortfall is a whole raid set, the count is still a multiple of p and passes lod_ec_parity_valid(). Before this patch such a layout went through open replay (LOV_MAGIC_DEFINED -> lod_qos_parse_config() -> lod_verify_striping()). Now the replay gets -EINVAL here, as do HSM release and xattr copies of the file.
Should RR fail a short EC parity the way lod_ost_alloc_qos() does ("we require the full stripe_count to avoid incomplete raid sets")? Otherwise this check is stricter than what the allocator guarantees.
Keguang Xu
Thanks for catching this. Full parity geometry is nsets * p (nsets = ec_raidset_count from the data stripe count and k). RR can finish with stripe_idx < nsets * p when nsets > 1: lod_stripe_count_min() is 0, so a short lod_ec_add_parities() still returns success and sets llc_stripe_count = stripe_idx. QoS instead requires stripe_idx == nsets * p. @rsahlberg@whamcloud.com @adilger@thelustrecollective.com: is stripe_idx < nsets * p for multi-raidset parity from RR intentional? A short layout (e.g. (nsets-1)*p) can break degraded read / EC recovery, leave some raid sets with no parity to rebuild from, and confuse resync/verify and mirror tools. QoS already refuses this. I think RR should match QoS (fail short EC parity) and the verifier should require count == nsets * p again — happy to follow up if you agree.
reply Gerrit AI review for Lustre lod_qos.c:1278 — (defect) Can the MDS still create an instantiated parity whose data component is not instantiated? If so, this check rejects a layout the MD
Gerrit AI review for Lustre
(defect) Can the MDS still create an instantiated parity whose data component is not instantiated? If so, this check rejects a layout the MDS wrote itself.
A mirror-targeted resync instantiates every component of the named mirror, whatever its extent:
lfs mirror copy -i <data> -o -1 f (or -o <parity>)
-> ll_file_resync() lil_mirror_id = parity mirror
-> lod_declare_update_rdonly() -> lod_prepare_resync_mirror()
-> lod_declare_instantiate_components() /* only needs lod_comp_find_linked() != NULL */
-> lod_comp_prep_create() /* sizes parity from data->llc_stripe_count */
lfs_mirror_copy() does not refuse a parity target, and lod_ec_check_data_usable() and lod_ec_add_parities() return early when data has no stripes allocated. With `-E 128M --ec 2+1 -E -1 -c 4 --ec 4+2` and less than 128M written, the parity of [128M, EOF) is stored INIT while its data component is not.
Before this patch that layout parsed fine, and lov_ea.c still skips such a pair (`!lsme_inited(data_entry)`). Now, once the object is reloaded (cache eviction or MDT restart), lod_parse_striping() returns -EINVAL. lod_declare_destroy() returns that error, so the file can no longer be unlinked, and resync and mirror split fail too.
Should an uninstantiated data peer be skipped here, the way the client does it? The other option is to refuse, in lod_declare_instantiate_components(), a parity whose data component is not instantiated and is not being instantiated in the same call.
reply Gerrit AI review for Lustre lod_qos.c:3960 — (minor) This comment doesn't seem to match lod_verify_ec_parity_comps(). That function skips parity components that are not instantiated (`!
Gerrit AI review for Lustre
(minor) This comment doesn't seem to match lod_verify_ec_parity_comps(). That function skips parity components that are not instantiated (`!lod_comp_inited(comp)` -> continue) and never looks at data components. lod_comp_prep_create() only runs on components that are being instantiated, so a layout from disk with a zero lcme_dstripe_count/lcme_cstripe_count still reaches this point unchecked. This test is still the only thing that keeps the `% 0` out of ec_split_stripes(). The old wording ("a layout loaded from disk does not pass through it again") was accurate. If the patch is refreshed, consider restoring it so the check below doesn't look redundant to the next reader.
reply Gerrit AI review for Lustre liblustreapi_layout.c:2833 — (minor) Now that layout_ec_verify() only rejects a zero or out-of-range data_stripe_count, Documentation/man3/llapi_layout_comp_add_parity_o
Gerrit AI review for Lustre
(minor) Now that layout_ec_verify() only rejects a zero or out-of-range data_stripe_count, Documentation/man3/llapi_layout_comp_add_parity_only.3 is out of date. It still says data_stripe_count is used for "bounding K+P", and lists EINVAL when the counts "did not fit data_stripe_count". If the patch is refreshed, consider updating both sentences to the new rule, where P may exceed the data stripe count.
tags: pt_ecro raid10-like 218_review_priority pw_shepherd
owner Keguang Xu · uploader Keguang Xu · PS4 uploaded 2d ago · NEW · open in Gerrit ↗
commit message
LU-20690 ec: verify parity against data, allow p > k With the RAID10-like parity layout the p parities of a raidset share one offset, so the parity of a component fits in its extent whatever p is. Remove the p <= k checks in lfs and llapi; k+p <= 256 and p <= 15 remain. Also drop the mirror-extend stripe_count < p guard in ec_check_data_comp() so --ec 2+3 works on a two-stripe target. An instantiated parity is checked against its data component in lod_verify_ec_parity_comps() after lod_use_defined_striping() / lod_parse_striping(), using lod_comp_find_linked() for the same pairing rule as the allocator (covers layout-change replay). Require a bidirectional link, matching k/p and stripe size, and parity stripe count ec_raidset_count() * p. lod_verify_striping() checks k/p and rejects an instantiated parity with stripe_count == 0 before DEFINED object init. Tests: sanity-ec 4a and 10 accept p > k, and 4b rejects k == 0 and k > 255. 8m covers mirror extend --ec 2+3 on a two-stripe target. 300 covers PFL -E 2M --ec 2+3 create/resync/verify, and 305 covers p > k multi-raidset create/resync/verify. llapi_layout_test test47 allows 3+4. Test-Parameters: testlist=sanity-ec ostcount=8 Signed-off-by: Keguang Xu <kxu@ddn.com> Assisted-by: Cursor:Composer llm_code_and_review_tools Change-Id: I3878502e8ff739cb5ed420ff359e2008376303e7
68361
master
J –B ✓T …R +1/2
LU-20690 ec: store EC parity like RAID10
13 unresolved thread(s) await your reply — last from Gerrit AI review for Lustre
ec2
218_review_prioritypt_ecroraid10-like
13 unresolved
XL +2329 −745
1h
LU-20690 ec: store EC parity like RAID10
13 unresolved thread(s) await your reply — last from Gerrit AI review for Lustre
ec2
218_review_prioritypt_ecroraid10-like
13 unresolved
XL +2329 −745
1h
13 unresolved thread(s) await your reply — last from Gerrit AI review for Lustre
build OK — testing in progress (typically 6–14h)
reviews:
Ronnie Sahlberg +1
no vote yet: Marc Vef, Andreas Dilger
patchsets:
PS20 10-30 Ronnie Sahlberg
PS21 11-01
PS22 11-03 Andreas Dilger
PS23 11-03 Andreas Dilger
PS24 11-03 Andreas Dilger
+11 earlier
PS1 08-26 PS7 09-03 PS8 09-07 PS9 09-10 PS10 09-11 PS11 09-23 PS12 10-08 PS13 10-08 PS17 10-23 PS18 10-29 PS19 10-29 Ronnie Sahlbergthreads: 13 need your reply · 0 waiting on others · 0 own notes · 0 bot
reply Gerrit AI review for Lustre COMMIT_MSG:174 — (minor) The patch also fixes the raid set mapping in degraded reads and resync, which counted rows and raid sets from e_start instead of fil
Gerrit AI review for Lustre
(minor) The patch also fixes the raid set mapping in degraded reads and resync, which counted rows and raid sets from e_start instead of file offset 0 (the wrong-parity rebuild described above). That counting came from landed commits, so a Fixes: line would help with tracking, even though EC has not shipped:
Fixes: 783c13bb0b ("LU-12669 ec: recover data from parity")
The resync side of the same problem dates from 03191be6f4 ("LU-12668 ec: add support to resync stale ec comps").
reply Keguang Xu patchset comment — With the changing of layout from `PARITY|RAID0` TO RAID10-like `PARITY`, we not only store the layout header differently to disk, but also w
Andreas Dilger
Should the removal of the LOV_PATTERN_RAID0 from EC layouts be moved into an earlier patch? That affects the on-disk layout of all the files and should be included into the 2.18.0 release, even if the 1+2 layout is not allowed initially. If this patch doesn't make it then we could force 1+2 to be 2+2 at create time? Not ideal, but probably easier to get landed.
Keguang Xu
With the changing of layout from `PARITY|RAID0` TO RAID10-like `PARITY`, we not only store the layout header differently to disk, but also we need to adjust our access path, say, in the parity component, stripes belong to the same raidset are arranged as RAID1 (logicall, they share the same file offset), stripes from different raidsets are stored as RAID0. To read/write data from/to parity, we need to designate stripe first. We can move the layout changes into an earlier patch. However, for EC to function properly, all of these patches must be landed together as a single set.
reply Gerrit AI review for Lustre lfs-mirror-read.1:30 — (minor) llapi_layout_parity_pos_to_io() starts each component's dump at the slot of the raid set that holds e_start, not at raid set 0. Take
Gerrit AI review for Lustre
(minor) llapi_layout_parity_pos_to_io() starts each component's dump at the slot of the raid set that holds e_start, not at raid set 0. Take -E 4M -c 1 -E eof -S 1M -c 8 --ec 4+1: the second component has O = 3M, e_start is in raid set 1, and its dump begins with the parity object of raid set 1, followed by raid set 0 of the next row. The order matches getstripe only when e_start is a multiple of the stripe width. lfs-getstripe.1 has the same issue ("dumps this RAID0 of all parity stripes"). If the patch is refreshed, could both pages say that the first row starts at the raid set of e_start?
reply Ronnie Sahlberg cl_object.h:2022 — not part of this change any more. And don't want to touch 69517 where this now lives as that one has passed testing.
Andreas Dilger
(minor) "stripe" is not a good name for this, since it is potentially conflated with the data stripe number if we want to do single-stripe read/write for replacement. Maybe "_parity_index" or similar?
Ronnie Sahlberg
not part of this change any more. And don't want to touch 69517 where this now lives as that one has passed testing.
reply Misc Code Checks Robot (Gatekeeper helper) lod_internal.h:751 — warn: lod_comp_shrink_to_extent():always true condition '(lod_comp->llc_stripe_count <= 65535) => (0-u16max <= u16max)'
Misc Code Checks Robot (Gatekeeper helper)
warn: lod_comp_shrink_to_extent():always true condition '(lod_comp->llc_stripe_count <= 65535) => (0-u16max <= u16max)'
reply Misc Code Checks Robot (Gatekeeper helper) lod_internal.h:751 — warn: lod_comp_shrink_to_extent():always true condition '(lod_comp->llc_stripe_count <= 65535) => (0-u16max <= u16max)'
Misc Code Checks Robot (Gatekeeper helper)
warn: lod_comp_shrink_to_extent():always true condition '(lod_comp->llc_stripe_count <= 65535) => (0-u16max <= u16max)'
reply Misc Code Checks Robot (Gatekeeper helper) lod_internal.h:751 — warn: lod_comp_shrink_to_extent():always true condition '(lod_comp->llc_stripe_count <= 65535) => (0-u16max <= u16max)'
Misc Code Checks Robot (Gatekeeper helper)
warn: lod_comp_shrink_to_extent():always true condition '(lod_comp->llc_stripe_count <= 65535) => (0-u16max <= u16max)'
reply Misc Code Checks Robot (Gatekeeper helper) lod_internal.h:751 — warn: lod_comp_shrink_to_extent():always true condition '(lod_comp->llc_stripe_count <= 65535) => (0-u16max <= u16max)'
Misc Code Checks Robot (Gatekeeper helper)
warn: lod_comp_shrink_to_extent():always true condition '(lod_comp->llc_stripe_count <= 65535) => (0-u16max <= u16max)'
reply Misc Code Checks Robot (Gatekeeper helper) lod_internal.h:751 — warn: lod_comp_shrink_to_extent():always true condition '(lod_comp->llc_stripe_count <= 65535) => (0-u16max <= u16max)'
Misc Code Checks Robot (Gatekeeper helper)
warn: lod_comp_shrink_to_extent():always true condition '(lod_comp->llc_stripe_count <= 65535) => (0-u16max <= u16max)'
reply Andreas Dilger lov_internal.h:160 — With RAID0|PARITY gone, lsme_parity_slots_per_raidset() is always 1 for parity and only multiplies poff at two places in lov_io.c. The six
Andreas Dilger
With RAID0|PARITY gone, lsme_parity_slots_per_raidset() is always 1 for parity and only multiplies poff at two places in lov_io.c. The six geometry helpers (lsme_stripe_origin, lsme_stripes_per_slot, lsme_parity_single_slot, lsme_slot_count, lsme_stripe_slot, lsme_parity_slots_per_raidset) reduce to one rule: for parity, origin = e_start, width = (stripe_count / p) * stripe_size, slot = stripe / p; otherwise RAID0. Two helpers would do, and the single-slot case can become an invalid entry. About 25 lines.
reply Ronnie Sahlberg lfs.c:4629 — Silent fallback is addressed. But not the quoting as raid0,overstriped should already be treated by yaml as a single string so quoting shoul
Andreas Dilger
This only matches the exact string "parity", but "lfs getstripe -y" prints "parity,overstriped" for a -C EC component, so "lfs setstripe --yaml" of that output silently falls back to the default pattern. When the pattern has multiple flags, please quote it in the YAML output (pattern: "parity,overstriped") so it is a single scalar, and parse the comma-separated flags here instead of comparing whole strings, which would also cover "raid0,overstriped" and any later flag combinations.
Ronnie Sahlberg
Silent fallback is addressed. But not the quoting as raid0,overstriped should already be treated by yaml as a single string so quoting should not be needed here.
reply Gerrit AI review for Lustre lfs.c:4553 — (suggestion) This isn't a bug, but the ignored[] list repeats the names in lov_pattern_names[] in liblustreapi_layout.c, and llapi_lov_strin
Gerrit AI review for Lustre
(suggestion) This isn't a bug, but the ignored[] list repeats the names in lov_pattern_names[] in liblustreapi_layout.c, and llapi_lov_string_pattern() already parses the comma-separated getstripe form. The copy will drift when a new pattern name is added. If the patch is refreshed, consider calling llapi_lov_string_pattern() and mapping LOV_PATTERN_MDT and LOV_PATTERN_OVERSTRIPING to the LLAPI_LAYOUT_* values. The "unknown_" prefix would still need handling here.
tags: 218_review_priority pt_ecro raid10-like
owner Keguang Xu · uploader Andreas Dilger · PS24 uploaded 1h ago · NEW · open in Gerrit ↗
commit message
LU-20690 ec: store EC parity like RAID10
The k >= p restriction is not a Cauchy/ISA-L limit. It comes from
storing parity as RAID0|PARITY: the p parity objects of a raidset are
concatenated along the file offset, so raidset_count * p * stripe_size
does not fit in a finite PFL extent when p > k (for example
--ec 2+3 -E 2M). This patch changes the parity layout; the p <= k
checks in lfs and llapi stay until a following patch removes them.
Store EC parity as LOV_PATTERN_PARITY instead. A raidset is an EC
split of the stripe set, and the layout is like RAID10 in that it has
the same two axes:
- across the raidsets of one stripe set: like RAID0, one
stripe_size per raidset;
- within one raidset: like RAID1, the p parity stripes share one
file offset (the same object_off).
The RAID1 axis comes from recovery. A raidset of k data and p parity
stripes survives the loss of any p of its k + p stripes, just as a
RAID1 of p + 1 copies survives the loss of any p copies. RAID1 is the
k == 1 case, where each parity is a copy of the data (--ec 1+N). The
p parities protect the same data, so they share its offset: the
offset says which data is protected, the parity index says which
redundancy unit it is.
What differs from RAID1 is I/O, not the role. For k > 1 the parities
are not replicas: each holds a different parity, and a lost stripe is
rebuilt from any k survivors rather than copied from one. The parity
uses the extent of the data component.
RAID0 places data rows from file offset 0, whereas counting EC rows
and raidsets from e_start groups the wrong k stripes when e_start is
not a multiple of the data stripe width: the stripes EC groups as one
raidset are not the objects lod paired with that raidset's parity
OSTs, and a degraded read rebuilds a lost stripe from the wrong
parity. For example -E 4M -c 1 -E eof -S 1M -c 8 --ec 4+1 puts offset
4M on object 4 (raidset 1), which counting from e_start encodes into
the parity of raidset 0. When e_start is not a multiple of
stripe_size, as after a 64K DoM component, counting stripe_size
parity slots from e_start also splits stripes at
e_start + n * stripe_size and overflows e_end.
Count rows and raidsets from file offset 0, as RAID0 maps the data
objects:
row = off / (stripe_count * stripe_size)
ec_off = O
+ (row - row of e_start) * raidset_count * stripe_size
+ raidset * stripe_size + off % stripe_size
Data before e_start is taken as zero, and so is its parity, which is
not stored: the origin O moves back from e_start over the raidsets of
the first row that end before e_start, and over the part of the
raidset of e_start below e_start % stripe_size when e_start is in its
last data stripe. slot % raidset_count stays the raidset, so the
parity OSTs lod pairs are unchanged, and every stored parity byte has
data in the component. Common DoM and PFL layouts with an unaligned
e_start are supported and keep their parity within [e_start, e_end),
such as:
- -E 64K -L mdt -E 1M -c 1 -S 1M -E eof -c 4 -S 4M --ec 4+2;
- -E 64K -L mdt -E 16M -c 2 -S 1M -E eof -c 4 -S 4M --ec 2+2;
- -E 64K -L mdt -E eof -c 4 -S 1M --ec 2+2;
- -E 1M -c 1 -S 1M -E 64M -c 1 -S 4M -E eof -c 4 -S 4M --ec 4+2.
When every raidset has one data stripe (--ec 1+N, or one data
stripe), there is one parity slot per data stripe and nothing to
pack, so rows and O count from file offset 0, as RAID0 does: ec_off
is the data offset, and each parity object is a copy of its data
object at the same object offsets, so it can stand in for it.
Otherwise O depends only on e_start, the data stripe count, k and the
stripe size, not on e_end, and when e_start is a multiple of the
stripe width, O is e_start.
Add ec_raidset_count(), ec_raidset_first(), ec_stripe_to_raidset()
and ec_off_to_raidset_offset() next to ec_split_stripes() in
lustre_user.h, and struct ec_parity_geom with ec_parity_geom_init(),
ec_data_to_parity() and its inverse ec_parity_to_data(), with
ec_parity_width() for the parity bytes of one stripe set, and use
them in lov, lod and llapi instead of open-coding the k0 * n0 split.
ec_parity_geom_init() also records the data row of parity slot 0 in
epg_row0 (0 for 1-data raidsets).
lov caches O in lsme_parity_origin when the layout is unpacked and
returns it from lsme_stripe_origin(), and lov_parity_geom_init()
fails a degraded read whose pair does not match in stripe size,
raidsets or origin; the parity mirror EOF of such a pair is also
bounded by one parity row per stripe set from e_start. Raidset
objects before e_start read as zero in degraded reads and resync.
A read is widened to whole raid sets, but not past the e_end of its
component, so a read ending there does not start an empty recovery
cycle for the next component's first raid set, which may begin before
its e_start.
lsme_parity_raid10() now just returns lsme_is_parity(), as every
parity component is LOV_PATTERN_PARITY, and
lsme_parity_slots_per_raidset() is dropped.
A component shorter than one stripe (e_end < e_start + stripe_size)
whose raidset continues past e_end (e.g. [64K, 1M) with -S 1M -c 2
--ec 2+2, or [1M, 4M) with -S 4M -c 2 --ec 2+2, where the second
stripe is never reached) still has no room for a full stripe_size
parity slot from e_start. ec_parity_fits() detects it:
llapi_layout_sanity() refuses it with LSE_EC_PARITY_FIT,
llapi_ec_check_comp_match() refuses to resync or verify it, and
lod_comp_prep_create() checks it again with the final data stripe
count, also for lfs mirror extend --ec.
The lov slot geometry, the STRIPE I/O scope and the per-stripe llapi
helpers were added by the previous patches and only take effect for
LOV_PATTERN_PARITY. This patch switches the encoding so they apply:
llapi_pattern_to_lov() and llapi_layout_to_lum() emit
LOV_PATTERN_PARITY, lov_pattern_supported() accepts it in place of
RAID0|PARITY (and lov_pattern_available() via that), and lod allocates
and verifies parity components with it. EC has not shipped, so the old
combination is not kept. Finite-extent stripe-count shrinking, now in
one lod_comp_shrink_to_extent() helper instead of three copies, is
skipped for parity components. lsm_unpackmd_comp_md_v1() and
lod_verify_striping() reject a component whose LCME_FL_PARITY flag and
LOV_PATTERN_PARITY disagree, since the parity geometry follows the
flag.
vvp_prep_size() no longer writes KMS into i_size for parity I/O.
ci_parity_eof is derived from i_size, and KMS is only a lower bound
of the data size, so a parity read within KMS shrank ci_parity_eof
for later parity I/O.
Userspace resync and verify move to the RAID10 offsets: one
stripe_size per raidset, each parity addressed by its stripe index,
leaving the part of the first parity slot below e_start alone. The
punch, zero-fill and hole verify of a parity slot are bounded by the
e_end of its component; no slot runs past e_end while
ec_parity_fits() holds, so this is only a defensive bound. Hole
punch-or-clear and hole verify loop over the p parity stripes that
share an offset when they fall back to zero-fill or zero-content
reads.
lfs setstripe --yaml parses the pattern as the comma-separated flags
lfs getstripe -y prints, so "parity,overstriped" keeps overstriping.
lfs mirror read of a parity mirror still dumps every parity object in
RAID0 order, through mirror_dump_parity() in lfs, from e_start. The
dump of a component stops at its e_end: when the extent is not a
multiple of the data stripe set, the raid sets of the last row past
e_end hold the data of the next component. The glimpse lock of such
a read spans every component, so lov_lock_sub_init() no longer
rejects a glimpse because a component the I/O does not cover has
fewer parity stripes than the designated one; lov_io_iter_init()
checks the components the I/O covers.
Example: -E 128M -c 11 -S 1M --ec 4+2
data/parity extent [0, 128M)
raidsets [4,2], [4,2], [3,2] (raidset_count=3)
parity has 6 OST objects
one data stripe set = 11M
one parity stripe set = 3M (not 1M flat, not 6M concat)
stripeset raidset data_pos ec_off concat objects
--------- ------- ----------- ------- ------- -------
0 0 [0, 4M) 0 0 0,1
0 1 [4M, 8M) 1M 2M 2,3
0 2 [8M, 11M) 2M 4M 4,5
1 0 [11M, 15M) 3M 6M 0,1
1 1 [15M, 19M) 4M 8M 2,3
1 2 [19M, 22M) 5M 10M 4,5
The concat column is what RAID0|PARITY would have used, and is what
overflows a finite extent when p > k.
Tests: sanity-ec 12c/12d/34c cover
holes and multi-raidset offsets, 30d and 31b-31e check the parity dump
size, 34d dumps a mixed-p mirror whose first extent is not a multiple
of the stripe set, 63d recovers reads from parity of raidset 1 in a
PFL component that does not start at 0, 301 covers an e_start that
stripe_size does not divide and checks that a component whose parity
would pass e_end is refused for that reason, 302 resyncs, verifies and
reads degraded common PFL layouts with an unaligned e_start, 303
recovers reads when e_start is not a multiple of the stripe width,
with equal and unequal raidsets, 304 covers zero-fill before e_start,
305 truncates at raidset and unaligned component boundaries, and 306
reads degraded across a component boundary where the raid set of the
first component runs into the next one.
llapi_layout_test asserts the component pattern is LLAPI_LAYOUT_PARITY
and 68 checks the parity placement over a range of geometries and
extents, and that 1-data raidset copies are at the data object
offsets.
Test-Parameters: testlist=sanity-ec ostcount=8
Test-Parameters: testlist=llapi_layout_test
Test-Parameters: testlist=sanity-ec ostcount=8 fstype=zfs
Signed-off-by: Keguang Xu <kxu@ddn.com>
Signed-off-by: Ronnie Sahlberg <rsahlberg@whamcloud.com>
Signed-off-by: Andreas Dilger <adilger@thelustrecollective.com>
Assisted-by: Cursor:Composer/Opus5.5 llm_code_and_review_tools
Assisted-by: ClaudeCode:claude-opus-5-5 ltvm llm_code_and_review_tools
Assisted-by: ClaudeCode:claude-fable-5-1 llm_code_and_review_tools
Change-Id: I9574df44969ddd40776f028b3c326d150ea08eb5
67700
master
J –B ✓T ✓R 0/2
LU-20354 quota: cache per-slave DT usage on QMT
carrying Keguang Xu · 10 unresolved thread(s) await your reply — last from Gerrit AI review for Lustre
11 unresolved
L +754 −11
5d
LU-20354 quota: cache per-slave DT usage on QMT
carrying Keguang Xu · 10 unresolved thread(s) await your reply — last from Gerrit AI review for Lustre
11 unresolved
L +754 −11
5d
10 unresolved thread(s) await your reply — last from Gerrit AI review for Lustre
CI green — no vote yet from Sergey Cheremencev, Andreas Dilger, Hongchao Zhang
build #133862 SUCCESS · tests all Maloo sessions
no vote yet: Sergey Cheremencev, Andreas Dilger, Hongchao Zhang
patchsets:
PS13 09-02
PS14 09-29
PS15 10-05
PS16 10-12
PS17 10-15 kg.xu
+12 earlier
PS1 08-03 PS2 08-04 PS3 08-05 PS4 08-06 PS5 08-10 PS6 08-18 PS7 08-24 PS8 08-24 PS9 08-26 PS10 08-28 PS11 08-29 PS12 08-30threads: 10 need your reply · 0 waiting on others · 1 own notes · 0 bot
reply Gerrit AI review for Lustre COMMIT_MSG:10 — (minor) The subject and this sentence scope the cache to DT / block usage, but nothing in the code restricts it to LQUOTA_RES_DT.
Gerrit AI review for Lustre
(minor) The subject and this sentence scope the cache to DT / block usage, but nothing in the code restricts it to LQUOTA_RES_DT. qmt_pool_new_conn() is called once per global index FID, so qmt_usg_extend_sites() also runs on the LQUOTA_RES_MD global pool, and the warming loop in qmt_dqacq0() runs for whatever qti_lqes(env) holds - which is the MD pool lqe on an inode DQACQ, with qb_usage being an inode count. So MD lqes get an lqe_usg_data and per-MDT slots too. Is that intended (in which case the message could say so), or should the warming and the extend be gated on LQUOTA_RES_DT until the inode side is ready?
reply Andreas Dilger COMMIT_MSG:10 — ... block and inode usage ..., I hope? It should be possible to cache the MDT usage as well, in case it is temporarily offline.
Andreas Dilger
... block and inode usage ..., I hope? It should be possible to cache the MDT usage as well, in case it is temporarily offline.
note kg.xu patchset comment — Updated
kg.xu
Hi, some tips on this change, which might be helpful in review,
Focus: Build QMT per-slave DT usage cache only. qmt_usg_serve=0 — GETQUOTA still leaves usage unset (legacy client fan-out).
Read order: qmt_internal.h → qmt_pool.c (DOM/dom_mdts) → qmt_entry.c (update/sum/refresh) → qmt_dqacq0 warm → mdt_qmt_fetch_usage.
Call chains:
```
# warm (live)
QSD DQACQ/REL (+QUOTA_DQACQ_FL_USAGE, qb_usage)
→ qmt_dqacq → qmt_dqacq0 → qmt_update_lqe_usg()
# pool membership → slots
connect/add → qmt_usg_extend_cb | qmt_pool_dom_add → empty slot
shrink → qmt_usg_shrink_cb → qmt_del_lqe_usg()
# refresh (wired, gated off)
qmt_quotactl(GETQUOTA) → qmt_get_usage()
→ (!qmt_usg_serve) -EAGAIN
→ [later] sum → refresh → mdt_qmt_fetch_usage
→ local MDT: lquotactl_slv | OST: OSP Q_GETOQUOTA
→ fail: keep cache | fallback granted
```
kg.xu
Updated
Focus: Build QMT per-slave DT usage cache only.
Read order:
qmt_internal.h / lquota_internal.h (lqu_xa) →
qmt_pool.c (DOM / extend / shrink) →
qmt_entry.c (update / del / sum) →
qmt_dqacq0 warm → qsd_handler.c (QUOTA_DQACQ_FL_USAGE).
Call chains:
```
QSD DQACQ/REL (+QUOTA_DQACQ_FL_USAGE, qb_usage)
→ qmt_dqacq → qmt_dqacq0 → qmt_update_lqe_usg_entry
(also on REPORT; skip shared LQA lqe)
# pool membership → slots
OST/MDT connect/add
→ qmt_usg_extend_cb → empty OST (or MD primary) slot
DOM MDT connect (global DT only)
→ qmt_pool_dom_add → qmt_usg_dom_extend_cb → empty MDT slot
shrink (snapshot then delete; not under cfs_hash_for_each)
→ qmt_usg_shrink_hash → qmt_del_lqe_usg_entry()
(waits lqu_fetching for follow-on refresh)
```
reply Keguang Xu patchset comment — Ping @hongchao@whamcloud.com and @scherementsev@ddn.com for reviews, thanks.
Andreas Dilger
ping @hongchao@whamcloud.com and @scherementsev@ddn.com for reviews on this series, it is useful for the EC project.
Keguang Xu
Ping @hongchao@whamcloud.com and @scherementsev@ddn.com for reviews, thanks.
reply Andreas Dilger wiretest.c:4945 — (style) please use `unsigned int` even though the script currently generates `unsigned` so that we have fewer places to fix in the future.
Andreas Dilger
(style) please use `unsigned int` even though the script currently generates `unsigned` so that we have fewer places to fix in the future.
reply Andreas Dilger lquota_internal.h:182 — seems reasonable to fix if patch is refreshed, or a later patch in this series..
Gerrit AI review for Lustre
(suggestion) This isn't a bug, but the comment says the field is QMT-only, yet it sits outside the `u.me`/`u.se` union. That adds 16 bytes to every `lquota_entry`, including all the QSD entries on every OST and MDT, which never use it. Could it go into `struct lquota_mst_entry` as `lme_usg_xa`, with a `#define lqe_usg_xa u.me.lme_usg_xa` like `lqe_sem`? On a non-debug build `lquota_mst_entry` is smaller than `lquota_slv_entry`, so the union should not grow. `lqe_ref_free()` would then call `qmt_lqe_usg_fini()` only when `lqe_is_master(lqe)` is true, rather than relying on QSD entries having a zeroed xarray.
Andreas Dilger
seems reasonable to fix if patch is refreshed, or a later patch in this series..
reply Andreas Dilger qmt_dev.c:58 — (minor) other parameters like this are named "maxage"
Andreas Dilger
(minor) other parameters like this are named "maxage"
reply Andreas Dilger qmt_dev.c:83 — I don't see the man4 page for this?
wc-checkpatch
(style) 'LPROC_SEQ_FOPS' may be misspelled - perhaps 'LUSTRE_RW_ATTR and create Documentation/man4/ page for param'?
Andreas Dilger
I don't see the man4 page for this?
reply Gerrit AI review for Lustre qmt_handler.c:1195 — (style) This isn't a bug, but qmt_update_lqe_usg_entry() opens with the identical `qpi_lqa || qpi_rtype != LQUOTA_RES_DT` test and returns 0
Gerrit AI review for Lustre
(style) This isn't a bug, but qmt_update_lqe_usg_entry() opens with the identical `qpi_lqa || qpi_rtype != LQUOTA_RES_DT` test and returns 0, so the filter here is duplicated. Dropping one of the two would keep the rule in a single place.
reply Andreas Dilger qmt_internal.h:394 — Presumably this is cached so long only if the target is offline? Otherwise it should be maximum a few seconds.
Andreas Dilger
Presumably this is cached so long only if the target is offline? Otherwise it should be maximum a few seconds.
reply Andreas Dilger sanity-quota.sh:8310 — Adding tests at the end of the file is conflict-intensive. If doing so, add a larger gap like test_350() so there is more room for future g
Andreas Dilger
Adding tests at the end of the file is conflict-intensive. If doing so, add a larger gap like test_350() so there is more room for future growth without conflicts (search for open patches modifying sanity-quota.sh to check). Otherwise, use a gap between 98 and 300, like 200.
owner Keguang Xu · uploader kg.xu · PS17 uploaded 19d ago · NEW · open in Gerrit ↗
commit message
LU-20354 quota: cache per-slave DT usage on QMT Build an in-memory per-slave DT usage cache on the global pool lqe so QMT can track block usage reported by slaves. Named pools do not keep their own slots; qmt_sum_lqe_usg() sums the global entries for the pool's current sarr members. LQA and MD are not cached. Refresh from MDT/OSP and serving the sum on GETQUOTA stay in a follow-on. DQACQ and REL piggyback on-disk usage in qb_usage with QUOTA_DQACQ_FL_USAGE, set by qsd_qbody_set_usage(). REPORT already carries that usage and is left unchanged. qmt_dqacq0() drops the lqe write lock, then qmt_update_lqe_usg_entry() stores the value on the global lqe. A missing slot is created there. The slot stores the update time; a ttl of 0 always misses. The sum holds op_rw_sem (read) across each member array. Slot fields are written with WRITE_ONCE and read with READ_ONCE, with no second lock. Callers must not hold lqe_sem: the recalc thread takes op_rw_sem and then lqe_sem, so the other order deadlocks with a queued pool writer. A missing or expired member, or a pool with no members, returns -ESTALE. An all-zero fresh sum is a valid idle ID. DOM MDTs are recorded only in the global DT pool tgts[MDT] and stay out of lgd and recalc. A slave connect adds the target once; disconnect does not remove it. qmt.*.usage_cache_ttl is in seconds (default 3600, max one day). Zero makes every lookup miss. The cache is freed with the lqe. qmt_usg_test covers update and sum by pool sarr, a named pool sharing the global cache, and TTL. sanity-quota/305 loads it on the MDS. It does not exercise MDT/OSP refresh. Assisted-by: Cursor:Grok-4.7/Opus5.5 llm_code_and_review_tools Signed-off-by: Keguang Xu <kxu@ddn.com> Change-Id: I9acd2b938c45ba9c62d5a2c03ba86af24ddbe90d
▶Longstanding (signal older than a week)
(7)
66567
master
J –B ✓T ✗10R 0/2
LU-20212 lov: skip invalid OST mirrors for aggregate I/O
cc Keguang Xu · enforced failed: review-dne-part-4, review-dne-part-7, review-dne-subtest-change, review-dne-zfs-part-4, review-dne-zfs-part-5, review-dne-zfs-part-7, review-dne-zfs-subtest-change, review-zfs
4 unresolved
L +216 −39
7d
LU-20212 lov: skip invalid OST mirrors for aggregate I/O
cc Keguang Xu · enforced failed: review-dne-part-4, review-dne-part-7, review-dne-subtest-change, review-dne-zfs-part-4, review-dne-zfs-part-5, review-dne-zfs-part-7, review-dne-zfs-subtest-change, review-zfs
4 unresolved
L +216 −39
7d
enforced failed: review-dne-part-4, review-dne-part-7, review-dne-subtest-change, review-dne-zfs-part-4, review-dne-zfs-part-5, review-dne-zfs-part-7, review-dne-zfs-subtest-change, review-zfs
3 thread(s) waiting on others
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-dne-part-4 | RHEL 9.7 / x86_64 | ran 11 tests. 1 tests failed: sanity-flr. | session |
| review-dne-part-4 | RHEL 10.1 / x86_64 | ran 11 tests. 1 tests failed: sanity-flr. | session |
| review-dne-part-7 | RHEL 9.8 / x86_64 | ran 6 tests. 1 tests failed: sanity-ec. | session |
| review-dne-subtest-change | RHEL 9.7 / x86_64 | ran 3 tests. 1 tests failed: sanity-flr. | session |
| review-dne-zfs-part-4 | RHEL 9.8 / x86_64 | ran 11 tests. 1 tests failed: sanity-flr. | session |
| review-dne-zfs-part-4 | RHEL 10.1 / x86_64 | ran 11 tests. 1 tests failed: sanity-flr. | session |
| review-dne-zfs-part-5 | RHEL 9.7 / x86_64 | ran 5 tests. 1 tests failed: sanityn. | session |
| review-dne-zfs-part-7 | RHEL 9.8 / x86_64 | ran 6 tests. 1 tests failed: sanity-ec. | session |
| review-dne-zfs-subtest-change | RHEL 9.7 / x86_64 | ran 3 tests. 1 tests failed: sanity-flr. | session |
| review-zfs | RHEL 8.10 / x86_64 | ran 10 tests. 1 tests failed: sanity-flr. | session |
build #134448 SUCCESS · tests all Maloo sessions
no vote yet: Qian Yingjin, Marc Vef, Andreas Dilger
patchsets:
PS3 06-11
PS4 06-13
PS5 06-16
PS6 06-22
PS7 10-26
+2 earlier
PS1 06-10 PS2 06-10threads: 0 need your reply · 3 waiting on others · 1 own notes · 0 bot
note kg.xu patchset comment — In lov_io_mirror_init(), if `ci_designated_mirror` is set, we can also check lov_mirror_has_inactive_stripe for `CIT_GLIMPSE/LSEEK/DATA_VERS
kg.xu
In lov_io_mirror_init(), if `ci_designated_mirror` is set, we can also check lov_mirror_has_inactive_stripe for `CIT_GLIMPSE/LSEEK/DATA_VERSION`. Since these io types all trigger OST aggregation, this check allows us to skip unnecessary RPCs. In current impl, we only check `CIT_GLIMPSE` during mirror selection (where `ci_designated_mirror` is false).
owner Keguang Xu · uploader Keguang Xu · PS7 uploaded 8d ago · NEW · open in Gerrit ↗
commit message
LU-20212 lov: skip invalid OST mirrors for aggregate I/O I/O that aggregates stripe-local results into one logical answer (glimpse, lseek, data_version) cannot succeed on a mirror while any relevant OST import is deactivated or invalid. In lov_io_mirror_init(), skip mirrors with unreachable OSTs in the I/O extent during FLR selection for aggregate I/O. When every mirror is skipped, glimpse (ndelay) returns -EAGAIN after backoff so the caller retries, and -EIO once it has tried all mirrors several rounds; lseek and data_version fail with -EIO at once. For designated-mirror I/O, apply the same extent-scoped check after resolving ci_designated_mirror; there is no alternate mirror to try, so return -EIO immediately. Detect unreachable stripes with lov_mirror_extent_unreachable(), limited to stripes intersecting the current I/O extent, via lov_sub_unreachable(). Skip parity mirrors for glimpse and data_version as well, so they never return parity object size or version. Size FLR ndelay rounds by the data mirrors that can serve the I/O, not lo_mirror_count, for I/O types that skip parity; this also excludes stale mirrors from the round of a plain FLR read. CIT_EC_RD skips that rotation. Add sanity-flr 31b. Test-Parameters: fortestonly Assisted-by: Cursor:Grok-4.6/Opus5.5 llm_code_and_review_tools Signed-off-by: Keguang Xu <kxu@ddn.com> Change-Id: Iac1c6415127d46fd958016a998ceab68663e0d42
68073
master
J –B ✓T ✓R 0/2
LU-20354 quota: refresh QMT usage cache via MDT/OSP proxy
3 unresolved thread(s) await your reply — last from Gerrit AI review for Lustre
3 unresolved
L +759 −66
16d
LU-20354 quota: refresh QMT usage cache via MDT/OSP proxy
3 unresolved thread(s) await your reply — last from Gerrit AI review for Lustre
3 unresolved
L +759 −66
16d
3 unresolved thread(s) await your reply — last from Gerrit AI review for Lustre
CI green — no vote yet from Sergey Cheremencev, Andreas Dilger, Hongchao Zhang
build #133812 SUCCESS · tests all Maloo sessions
no vote yet: Sergey Cheremencev, Andreas Dilger, Hongchao Zhang
patchsets:
PS2 09-02
PS3 09-04
PS5 09-10
PS6 10-14
PS7 10-16
+1 earlier
PS1 08-18threads: 3 need your reply · 0 waiting on others · 0 own notes · 0 bot
reply Gerrit AI review for Lustre mdt_handler.c:6122 — (typo) Missing space before `*/`.
Gerrit AI review for Lustre
(typo) Missing space before `*/`.
reply Gerrit AI review for Lustre qmt_entry.c:1344 — (minor) The extra members don't really wait for a later refresh. After qmt_refresh_lqe_usg() returns 0, qmt_get_usage() re-sums with check_t
Gerrit AI review for Lustre
(minor) The extra members don't really wait for a later refresh. After qmt_refresh_lqe_usg() returns 0, qmt_get_usage() re-sums with check_ttl=false over the live sarr, not over the copied idxs. So a member added during the walk is summed at whatever age its slot has. Example: `lctl pool_add` adds OST N to a named pool while the serial Q_GETOQUOTA walk is running. OST N already has a slot on the global lqe from an old DQACQ (slots are only freed with the lqe), but that slot expired long ago. Refresh never touches N, and the following sum serves N's old value as part of a fresh aggregate. A member with no slot gets -ESTALE, which is fine. This only matters once qmt_usg_serve is enabled, and the result is one stale reply, not corruption. But either the comment or the post-refresh sum should change, e.g. sum only the idxs that refresh handled, or redo the refresh if op_count changed.
reply Gerrit AI review for Lustre qmt_handler.c:776 — (style) This isn't a bug, but the comment sits directly above an assignment that runs for every command in this case group, while the text o
Gerrit AI review for Lustre
(style) This isn't a bug, but the comment sits directly above an assignment that runs for every command in this case group, while the text only describes the two commands handled by the condition below it. The old comment explained the assignment itself. Moving this line down onto the `if` would read better.
owner Keguang Xu · uploader Keguang Xu · PS7 uploaded 18d ago · NEW · open in Gerrit ↗
commit message
LU-20354 quota: refresh QMT usage cache via MDT/OSP proxy Wire qmt_refresh_lqe_usg() to pull stale or missing per-slave usage through MDT: local OSD for the DOM MDT, OSP Q_GETOQUOTA for OSTs. The cache stays on the global DT lqe. The caller passes that lqe and the pool; refresh walks the pool sarr (alloc outside the lock, copy under it). Fresh slots, including DQACQ-warmed zeros, are left alone. Remote MDT DOM fetch is still TODO. On fetch failure, keep a cached value when the slot exists, otherwise fall back to granted from that slave's index. Degraded values are back-dated to expire after about 60s so the next refresh retries soon. qmt_sum_lqe_usg() takes check_ttl: true for a cache lookup, false after refresh so freshly stored slots are served even when ttl is 0. qc_id is already a filesystem ID. MDS/OSS NIDs belong in a trusted nodemap, so OFD's CLIENT_TO_FS map is identity and no skip-idmap handshake is needed. qmt_get_usage() sums and refreshes only when qmt_usg_serve is on. The flag stays off, so GETQUOTA still leaves usage unset. qmt_usg_fallback counts a granted fallback or a failed serve. Expose qmt.*.usage_cache_stats (hit/miss/fetch_rpc/fallback). Add sanity-quota/310, skipped until qmt_usg_serve is on. Extend qmt_usg_test for ttl 0 with check_ttl true/false. Test-Parameters: trivial testlist=sanity-quota Assisted-by: Cursor:Grok-4.6 llm_code_and_review_tools Signed-off-by: Keguang Xu <kxu@ddn.com> Change-Id: Ied2f4f1e3c7ee6611a69a57e8064d46a59ba845f
62497
master
J !B ✓T ✗5R -1/2−
LU-7266 ldlm: introduce pressure-based SLV
mine needs rebase — checkpatch: cannot be cherry-picked
needs rebase
11 unresolved
XL +2191 −68
27d
LU-7266 ldlm: introduce pressure-based SLV
mine needs rebase — checkpatch: cannot be cherry-picked
needs rebase
11 unresolved
XL +2191 −68
27d
needs rebase — checkpatch: cannot be cherry-picked
enforced failed: review-dne-zfs-part-4, review-dne-zfs-part-6, review-ldiskfs, review-ldiskfs-arm
8 unresolved thread(s) await your reply — last from Gerrit AI review for Lustre
1 thread(s) waiting on others
janitor run: 1 failing config(s), none unique to this patch — janitor results
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-dne-zfs-part-4 | RHEL 10.1 / x86_64 | ran 9 tests. 1 tests failed: sanity-quota. | session |
| review-dne-zfs-part-6 | RHEL 9.7 / x86_64 | ran 4 tests. 1 tests failed: ost-pools. | session |
| review-ldiskfs | RHEL 8.10 / x86_64 | ran 5 tests. 1 tests failed: sanity. | session |
| review-ldiskfs | RHEL 8.10 / x86_64, RHEL 9.7 / x86_64 | ran 5 tests. 1 tests failed: sanity. | session |
| review-ldiskfs-arm | RHEL 8.10 / x86_64, Rocky 9.5 / aarch64 | ran 6 tests. 1 tests failed: sanity. | session |
build #129438 SUCCESS · tests all Maloo sessions
no vote yet: Keguang Xu, Qian Yingjin, Andreas Dilger, Vitaly Fertman, Timothy Day, Patrick Farrell
patchsets:
PS24 07-14 Keguang Xu
PS25 08-08 Keguang Xu
PS26 08-09 Keguang Xu
PS27 08-15 Keguang Xu
PS28 08-15 Keguang Xu
+15 earlier
PS1 11-12 PS4 11-19 PS6 11-26 PS10 02-27 PS13 05-11 PS14 05-11 PS15 05-26 PS16 05-29 PS17 06-11 PS18 06-13 Keguang Xu PS19 06-22 Keguang Xu PS20 06-27 Keguang Xu PS21 07-13 Keguang Xu PS22 07-13 Keguang Xu PS23 07-13 Keguang Xuyour previous vote: -1 on PS19 (2026-06-22 01:12) — now at PS28
threads: 8 need your reply · 1 waiting on others · 2 own notes · 0 bot
note kg.xu patchset comment — Review note:
kg.xu
Review note: The patch is large because of negotiation, dual SLV, rollout, and tests. The new logic is small: server pressure is essentially granted/limit; the client maps pressure to an evict quota and uses a quota-based LRU policy instead of per-lock CLV arithmetic. Suggested review order: 1) lustre/include/lustre_dlm.h, lustre_idl.h — packed SLV layout and pool fields 2) ldlm_pool_calc_slv_by_pressure(), ldlm_pool_pressure_evict_*() — core math (~100 lines) 3) ldlm_cancel_pressure_policy(), ldlm_cancel_lru_policy() — reclaim behavior 4) connect, rollout, sysfs, tests — compatibility and rollout (bulk of the diff)
reply Gerrit AI review for Lustre ldlm.namespaces.pool.slv_mode.4:174 — Not a bug, but the MDT name in this sample output is missing the UUID suffix. mdt_init0() builds the namespace name as "%s-%s" of LUSTRE_MDT
Gerrit AI review for Lustre
Not a bug, but the MDT name in this sample output is missing the UUID suffix. mdt_init0() builds the namespace name as "%s-%s" of LUSTRE_MDT_NAME and obd_uuid.uuid, so it reads mdt-testfs-MDT0000_UUID, matching the filter-testfs-OST0000_UUID line just below.
note kg.xu ldlm_pool.c:309 — Currently, `ldlm_pool` tracks metrics at the per-target level (e.g., 'fsname-target'). I propose aggregating these metrics by 'fsname' inste
kg.xu
Currently, `ldlm_pool` tracks metrics at the per-target level (e.g., 'fsname-target'). I propose aggregating these metrics by 'fsname' instead. Furthermore, we should discuss whether resources should be allocated equally across all file systems, or if we need a weighted policy.
reply Gerrit AI review for Lustre ldlm_pool.c:212 — (style) Not a bug, but with `pressure_adjust == true` this just returns `L` unchanged, and the only caller passing true is the CDEBUG in ldl
Gerrit AI review for Lustre
(style) Not a bug, but with `pressure_adjust == true` this just returns `L` unchanged, and the only caller passing true is the CDEBUG in ldlm_pool_calc_slv_by_pressure(). A boolean that turns the function into the identity is hard to follow - would printing `limit` directly there be clearer?
reply Gerrit AI review for Lustre ldlm_pool.c:1856 — (style) These six new helpers are the only EXPORT_SYMBOL()s in this file; ldlm_pool_get_slv(), ldlm_pool_init() and friends aren't exported.
Gerrit AI review for Lustre
(style) These six new helpers are the only EXPORT_SYMBOL()s in this file; ldlm_pool_get_slv(), ldlm_pool_init() and friends aren't exported. ldlm_lib.c and ldlm_request.c build into the same ptlrpc module (lustre/ldlm/Makefile), so none of them need exporting, and this one is used only inside ldlm_pool.c so it could be static.
reply Gerrit AI review for Lustre ldlm_pool.c:497 — Once the pool is pressure-ready these two limits diverge by roughly 5x, and the legacy scalar looks like it collapses to the floor.
Gerrit AI review for Lustre
Once the pool is pressure-ready these two limits diverge by roughly 5x, and the legacy scalar looks like it collapses to the floor.
ldlm_pools_recalc_task() sets pl_limit = ldlm_ratio2locknr(30)/nr_ns for a ready pool but pl_legacy_limit = LDLM_POOL_HOST_L/nr_ns. HOST_L is 50 locks per MB of RAM, i.e. RAM/20971 locks; ldlm_ratio2locknr(30) is 0.30*RAM/LDLM_LOCK_MEM_OVERHEAD. Packed clients only start evicting at 50% of pl_limit (default curve), so granted settles well above pl_legacy_limit.
With granted > limit here:
grant_usage = max_t(int, limit - (granted - grant_plan), 1); /* -> 1 */
slv_factor = (grant_usage << 10); do_div(slv_factor, limit); /* -> 0 */
slv = slv * slv_factor; /* -> 0 */
and the clamp below lifts it to ldlm_pool_slv_min() == 1. In ldlm_cancel_lrur_policy() slv == 1 means every lock with lv >= 1 (any lock idle for a second or more) is cancelled, so a non-packed client drops essentially its whole LRU on each recalc, and the drop also trips the ns_recalc_pct urgent-recalc path in ldlm_cli_update_pool().
That hits up to 30% of client exports at the default threshold, plus every MDT OSC on an OST since lod never negotiates OBD_CONNECT2_POOL_SLV_PACKED. Does "keep legacy behavior for old clients" still hold once the pool flips, or should the legacy scalar be derived from the limit the pool is actually being filled to?
reply Gerrit AI review for Lustre ldlm_pool.c:1535 — pl_slv_mode is set to pressure_rollout two lines up, so this branch always runs and a brand new server pool gets the pressure budget before
Gerrit AI review for Lustre
pl_slv_mode is set to pressure_rollout two lines up, so this branch always runs and a brand new server pool gets the pressure budget before any client has negotiated packed SLV and before ldlm_pool_fallback_legacy_on_multi_fs_register() has had a chance to force the pool back to legacy_slv. It is also the whole-node budget: ldlm_pools_recalc_task() divides ldlm_ratio2locknr() by ldlm_namespace_nr_read(LDLM_NAMESPACE_SERVER), this does not. The neighbouring initialisers (pl_legacy_limit, pl_grant_plan) still use LDLM_POOL_HOST_L, so pl_server_lock_volume is seeded from one basis and decayed against another until the first recalc pass. Would it be simpler to seed with LDLM_POOL_HOST_L here and let ldlm_pools_recalc_task() apply the pressure budget when the readiness gate says so?
reply Qian Yingjin sanity-ldlm-pool.sh:195 — This could be:
Qian Yingjin
This could be: for ((batch = 0; batch < BATCH_COUNT; batch++)) ...
reply Gerrit AI review for Lustre sanity-ldlm-pool.sh:190 — (style) This commented-out curve set/restore block looks like leftover scaffolding - drop it, or enable it if the suite is meant to cover a
Gerrit AI review for Lustre
(style) This commented-out curve set/restore block looks like leftover scaffolding - drop it, or enable it if the suite is meant to cover a non-default curve?
reply Gerrit AI review for Lustre sanity-ldlm-pool.sh:260 — (style) Carried over from patchset 9 and still open: this reads more naturally as `for ((batch = 0; batch < BATCH_COUNT; batch++))`, with su
Gerrit AI review for Lustre
(style) Carried over from patchset 9 and still open: this reads more naturally as `for ((batch = 0; batch < BATCH_COUNT; batch++))`, with subdir_idx computed from batch directly.
owner kg.xu · uploader Keguang Xu · PS28 uploaded 80d ago · NEW · open in Gerrit ↗
commit message
LU-7266 ldlm: introduce pressure-based SLV Legacy SLV/CLV reacts to server pressure indirectly and can delay lock reclaim. This change introduces a pressure-based SLV path: server sends packed pressure in SLV, and client converts it to evict quota. Packed SLV Format: * Bits 63..60: format version (current 0x1). Legacy SLV leaves these bits clear. * Bits 59..16: reserved for future use. * Bits 15..0: pressure in hundredths of a percent, where 10000 means 100.00%. Here pressure is total-grant-lock/total-limit. Unlike the previous grant-plan-based scalar SLV. Rationale: - Percentage-based eviction scales with client lock footprint, so larger lock holders evict more locks under pressure while smaller clients are not over-penalized. - Gradual rollout is gated by packed-capable export coverage, avoiding a mixed-upgrade case where only a small subset of new clients absorbs repeated reclaim pressure. - Client eviction uses segmented pressure-to-evict mapping, so reclaim is mild at low pressure and stronger at high pressure. At relatively low pressure, clients react faster to keep pressure in a healthy range; at higher pressure, clients use a longer evict interval because each round evicts more locks, reducing oscillation and avoiding over-evicting locks that should be retained; at or above the last curve point the interval shortens so high-pressure reclaim stays frequent. Key updates: - add OBD_CONNECT2_POOL_SLV_PACKED negotiation and packed SLV helpers - add slv_mode (legacy_slv / pressure_rollout), rollout threshold, and client evict-curve sysfs controls - pressure_rollout pools use ldlm.lock_pool_limit_mem_pct (default 30% of RAM / LDLM_LOCK_MEM_OVERHEAD) instead of LDLM_POOL_HOST_L - keep dual SLV state (legacy scalar + pressure scalar) for compatibility, where in pressure path; add quota-based client reclaim policy: ldlm_cancel_pressure_policy - fall back to legacy_slv on mixed-filesystem server nodes - keep legacy behavior for old clients and pre-threshold rollout stages - add sanity-ldlm-pool.sh, sanity 124aa, and man pages for the new pool parameters Performance impact: CPU overhead should be negligible. - The server recomputes pressure once per second per pool. On the client, pool recalc runs on the ~10s timer and when unused locks are decrefed. - On the packed pressure path, reclaim no longer performs per-lock arithmetic during LRU scanning, so lock-heavy workloads should see unchanged or slightly lower CPU usage. Assisted-by: Cursor:auto/codex5.5 Signed-off-by: Keguang Xu <squalfof@gmail.com> Signed-off-by: Keguang Xu <kxu@ddn.com> Change-Id: If8a6f247127576965ae45c349a4ab04fb286615a
68600
master
J ✗45B ✓T ✗11R 0/2
LU-20354 quota: serve LQA usage from QMT cache
janitor: 45 test failures unique to this patch — sanity-quota@ldiskfs+DNE:test_1j, sanity-quota@ldiskfs+DNE:test_3a, sanity-quota@ldiskfs+DNE:test_3b +42 more
45 unique failures
11 unresolved
XL +1391 −198
44d
LU-20354 quota: serve LQA usage from QMT cache
janitor: 45 test failures unique to this patch — sanity-quota@ldiskfs+DNE:test_1j, sanity-quota@ldiskfs+DNE:test_3a, sanity-quota@ldiskfs+DNE:test_3b +42 more
45 unique failures
11 unresolved
XL +1391 −198
44d
janitor: 45 test failures unique to this patch — sanity-quota@ldiskfs+DNE:test_1j, sanity-quota@ldiskfs+DNE:test_3a, sanity-quota@ldiskfs+DNE:test_3b +42 more
enforced failed: review-dne-ipv6, review-dne-part-4, review-dne-part-5, review-dne-zfs-part-3, review-dne-zfs-part-4, review-dne-zfs-part-5, review-dne-zfs-part-7, review-zfs
11 unresolved thread(s) await your reply — last from Gerrit AI review for Lustre
janitor flagged test failures unique to this patch — full janitor results
| unique failing test | history |
|---|---|
| sanity-quota@ldiskfs+DNE:test_1j | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@ldiskfs+DNE:test_3a | seen in 1 other review |
| sanity-quota@ldiskfs+DNE:test_3b | seen in 1 other review |
| sanity-quota@ldiskfs+DNE:test_3c | seen in 1 other review |
| sanity-quota@ldiskfs+DNE:test_5 | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@ldiskfs+DNE:test_6 | seen in 1 other review |
| sanity-quota@ldiskfs+DNE:test_7a | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@ldiskfs+DNE:test_7b | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@ldiskfs+DNE:test_7c | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@ldiskfs+DNE:test_17 | seen in 6 other reviews |
| sanity-quota@ldiskfs+DNE:test_25 | seen in 1 other review |
| sanity-quota@ldiskfs+DNE:test_33 | seen in 4 other reviews |
| sanity-quota@ldiskfs+DNE:test_34 | seen in 4 other reviews |
| sanity-quota@ldiskfs+DNE:test_35 | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@ldiskfs+DNE:test_37 | seen in 4 other reviews |
| sanity-quota@ldiskfs+DNE:test_38 | seen in 4 other reviews |
| sanity-quota@ldiskfs+DNE:test_40c | seen in 1 other review |
| sanity-quota@ldiskfs+DNE:test_64 | seen in 1 other review |
| sanity-quota@ldiskfs+DNE:test_67 | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@ldiskfs+DNE:test_71a | seen in 2 other reviews |
| sanity-quota@ldiskfs+DNE:test_71b | seen in 2 other reviews |
| sanity-quota@ldiskfs+DNE:test_72 | seen in 2 other reviews |
| sanity-quota@ldiskfs+DNE:test_73a | seen in 6 other reviews |
| sanity-quota@ldiskfs+DNE:test_75 | seen in 6 other reviews |
| sanity-quota@ldiskfs+DNE:test_80 | seen in 1 other review |
| sanity-quota@zfs:test_1j | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@zfs:test_3a | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@zfs:test_3b | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@zfs:test_3c | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@zfs:test_5 | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@zfs:test_6 | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@zfs:test_7a | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@zfs:test_7b | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@zfs:test_7c | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@zfs:test_17 | seen in 34 other reviews |
| sanity-quota@zfs:test_25 | seen in 27 other reviews |
| sanity-quota@zfs:test_33 | seen in 33 other reviews |
| sanity-quota@zfs:test_34 | seen in 33 other reviews |
| sanity-quota@zfs:test_35 | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@zfs:test_37 | seen in 33 other reviews |
| sanity-quota@zfs:test_38 | seen in 33 other reviews |
| sanity-quota@zfs:test_64 | seen in 1 other review |
| sanity-quota@zfs:test_72 | seen in 14 other reviews |
| sanity-quota@zfs:test_73a | seen in 17 other reviews |
| sanity-quota@zfs:test_75 | seen in 7 other reviews |
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-dne-ipv6 | RHEL 9.8 / x86_64 | ran 4 tests. 1 tests failed: sanity-lnet. | session |
| review-dne-part-4 | RHEL 9.7 / x86_64 | ran 9 tests. 1 tests failed: sanity-quota. | session |
| review-dne-part-4 | RHEL 10.1 / x86_64 | ran 9 tests. 1 tests failed: sanity-quota. | session |
| review-dne-part-5 | RHEL 9.7 / x86_64 | ran 5 tests. 1 tests failed: lustre-rsync-test. | session |
| review-dne-zfs-part-3 | RHEL 9.8 / x86_64 | ran 3 tests. 1 tests failed: conf-sanity. | session |
| review-dne-zfs-part-3 | RHEL 10.1 / x86_64 | ran 3 tests. 1 tests failed: conf-sanity. | session |
| review-dne-zfs-part-4 | RHEL 9.8 / x86_64 | ran 9 tests. 1 tests failed: sanity-quota. | session |
| review-dne-zfs-part-4 | RHEL 10.1 / x86_64 | ran 9 tests. 1 tests failed: sanity-quota. | session |
| review-dne-zfs-part-5 | RHEL 9.7 / x86_64 | ran 5 tests. 1 tests failed: lustre-rsync-test. | session |
| review-dne-zfs-part-7 | RHEL 9.8 / x86_64 | ran 6 tests. 1 tests failed: sanity-ec. | session |
| review-zfs | RHEL 8.10 / x86_64 | ran 8 tests. 1 tests failed: sanity-quota. | session |
build #132033 SUCCESS · tests all Maloo sessions
patchsets:
PS3 09-13
PS4 09-14
PS5 09-16
PS6 09-17
PS7 09-20
+2 earlier
PS1 09-03 PS2 09-11threads: 11 need your reply · 0 waiting on others · 0 own notes · 0 bot
reply Gerrit AI review for Lustre COMMIT_MSG:41 — (suggestion) This adds a new quotactl command and two new request formats on the MDS-to-OSS/MDT path, and changes what `lfs quota --lqa` pri
Gerrit AI review for Lustre
(suggestion) This adds a new quotactl command and two new request formats on the MDS-to-OSS/MDT path, and changes what `lfs quota --lqa` prints. A `Test-Parameters:` line asking for an interop run against a released server would cover the rolling-upgrade case where the slave does not know LUSTRE_Q_GETOUSAGES, e.g.
Test-Parameters: serverversion=2.16 testlist=sanity-quota
reply Gerrit AI review for Lustre COMMIT_MSG:39 — (minor) The body covers the LQA work well, but two hunks outside it are not mentioned.
Gerrit AI review for Lustre
(minor) The body covers the LQA work well, but two hunks outside it are not mentioned. qmt_dqacq0() now warms the cache with `qb_usage != 0` instead of an unconditional `true`. That changes the non-LQA per-ID path: a slave that legitimately reports zero usage no longer marks its slot fresh, so the next GETQUOTA misses and pays a full refresh round instead of hitting the cache. The inline comment gives the motivation (an on-disk 0 racing the first inode), but a reader of the commit message would not expect a per-ID behaviour change in an LQA patch. qmt_usg_test_init() also switches every `return rc` to `return -EINVAL`, which drops the specific failure code from the insmod error. Could both get a line in the body, or should the dqacq0 change be split out since it stands on its own?
reply Gerrit AI review for Lustre qmt.usage_cache_ttl.4:67 — (minor) This paragraph now sits directly under the new LQA text but describes only the per-ID behaviour. For LQA there is no "previously val
Gerrit AI review for Lustre
(minor) This paragraph now sits directly under the new LQA text but describes only the per-ID behaviour. For LQA there is no "previously valid cached value" kept and no granted substitution - qmt_lqa_refresh_lqe_usg() resets every slot to QMT_USG_UNSET and returns the error - and the 120s degraded TTL never applies either, since qmt_apply_usg_fetch() is not on that path. qmt.usage_cache_stats(4) already carries the "LQA has no granted substitution" caveat; the same note would fit here.
reply Gerrit AI review for Lustre lustre_quota.h:372 — (minor) The kdoc opens with "GETOUSAGES is server-internal", but this second test is what decides whether a non-server export gets in.
Gerrit AI review for Lustre
(minor) The kdoc opens with "GETOUSAGES is server-internal", but this second test is what decides whether a non-server export gets in. The connect-flag test above already matches every MDS/OSS export on its own: lod_add_device() requests OBD_CONNECT_MDS for OSP-to-OST and OBD_CONNECT_MDS_MDS for OSP-to-MDT, and both survive connect negotiation (ofd_obd.c reads exp_connect_flags(exp) & OBD_CONNECT_MDS the same way). So the nodemap clause is not needed for the MDS/OSS case the kdoc uses to justify it; what it does add is every ordinary client that happens to sit in a nodemap with trust_client_ids set, which is a common setting for compute nodes and says nothing about the client being privileged. A client in that nodemap can then read bspace/ispace for up to LQUOTA_IDS_MAX arbitrary IDs per RPC from any OST or MDT. Is that intended, or should the check be the connect flags alone?
reply Gerrit AI review for Lustre mdt_handler.c:6324 — (minor) GETOUSAGES is dispatched to every OST/MDT without any capability check. There is no OBD_CONNECT* bit for it, and nothing here looks
Gerrit AI review for Lustre
(minor) GETOUSAGES is dispatched to every OST/MDT without any capability check. There is no OBD_CONNECT* bit for it, and nothing here looks at exp_connect_flags before sending. Against a peer that predates the opcode the reply is an error rather than data: lquotactl_slv() rejects any qc_cmd other than Q_GETOQUOTA/LUSTRE_Q_ITEROQUOTA with -EOPNOTSUPP, and an older mdt_quotactl() falls into its `default:` case with -EFAULT. That becomes items[i].ufi_rc, which aborts qmt_lqa_refresh_lqe_usg() and wipes the cache, so in a mixed-version cluster every GETQUOTALQA pays the full glb-index walk and RPC fan-out and still ends with usage unknown. Should this be gated on a negotiated connect flag so the QMT can skip the refresh entirely when a slave cannot serve it?
reply Gerrit AI review for Lustre osp_dev.c:1549 — (minor) The comment above promises a length check that req_capsule_server_sized_get() does not perform for this field.
Gerrit AI review for Lustre
(minor) The comment above promises a length check that req_capsule_server_sized_get() does not perform for this field.
RMF_OBD_QUOTA_ACCT is RMF_F_STRUCT_ARRAY, and __req_capsule_get() takes that branch:
len = lustre_msg_buflen(msg, offset);
if (len % field->rmf_size) return NULL;
value = getter(msg, offset, len);
so the rc_area size that server_sized_get() just stored is discarded and lustre_msg_buf() is called with min_size equal to the buffer's own length. Any reply whose ACCT buffer is a non-zero multiple of sizeof(struct lquota_acct_rec) comes back non-NULL, including one shorter than qcb_replen, and the memcpy() then reads past it.
In-tree slaves size the reply from the ID list they received, so this needs a peer that answers rc = 0 with a short array. Would an explicit req_capsule_get_size(pill, &RMF_OBD_QUOTA_ACCT, RCL_SERVER) == qcb_replen test be worth keeping here, or at least a comment that matches what sized_get actually guarantees?
reply Gerrit AI review for Lustre qmt_entry.c:1495 — (style) This isn't a bug, but `qpi` is now available one line up, so `qpi->qpi_qmt->qmt_usg_ttl` would avoid evaluating lqe2qpi() twice. Wor
Gerrit AI review for Lustre
(style) This isn't a bug, but `qpi` is now available one line up, so `qpi->qpi_qmt->qmt_usg_ttl` would avoid evaluating lqe2qpi() twice. Worth folding in if the patch is refreshed.
reply Gerrit AI review for Lustre qmt_entry.c:1712 — (defect) A waiter that just slept on lqu_waitq re-enters here and redoes the whole refresh, even when the previous owner already filled ever
Gerrit AI review for Lustre
(defect) A waiter that just slept on lqu_waitq re-enters here and redoes the whole refresh, even when the previous owner already filled every slot. The per-ID path below is protected against this: after `goto again` its gather loop skips slots with `!qmt_usg_expired(...)`, so `nstale == 0` and it breaks out with zero RPCs. qmt_lqa_refresh_lqe_usg() has no equivalent test - it always calls qmt_lqa_collect_ids() (full walk of the glb index) and then re-fetches every slot. So with the default ttl of 3600, two concurrent `lfs quota --lqa` for the same name serialize on lqu_fetching and then each pay a complete index walk plus a GETOUSAGES fan-out, on an MDS service thread. N concurrent callers give N full refreshes. Would a `qmt_sum_lqe_usg(lqe, &tmp, now, true) == 0` check at the top of qmt_lqa_refresh_lqe_usg() (or before re-taking lqu_fetching) be enough to make the waiters cheap?
reply Gerrit AI review for Lustre qmt_handler.c:1297 — (minor) "LQA no-ops" doesn't quite match qmt_update_lqe_usg_entry_with_ttl(): its early return only fires for `usage_valid && !lqa_store`. q
Gerrit AI review for Lustre
(minor) "LQA no-ops" doesn't quite match qmt_update_lqe_usg_entry_with_ttl(): its early return only fires for `usage_valid && !lqa_store`. qmt_lqe_lookup() substitutes lqa_qid so the LQA entry here always has qid_uid == QMT_LQA_QID, and when the usage being reported is zero the call falls through and inserts/keeps a slot on the shared lqe. Harmless, but the comment reads as if nothing happens at all.
reply Gerrit AI review for Lustre qmt_internal.h:479 — (minor) QMT_USG_SNAP_NONE has no users - qmt_apply_usg_fetch() still compares its snap_usage against QMT_USG_UNSET, and this change is the t
Gerrit AI review for Lustre
(minor) QMT_USG_SNAP_NONE has no users - qmt_apply_usg_fetch() still compares its snap_usage against QMT_USG_UNSET, and this change is the tip of the relation chain, so nothing later picks it up. It is also the same value as QMT_USG_UNSET, which makes two names for one sentinel. Drop it on the next refresh?
reply Gerrit AI review for Lustre qmt_lqa.c:1330 — (suggestion) Any single slave error discards every slot, including ones that were still valid from an earlier successful refresh.
Gerrit AI review for Lustre
(suggestion) Any single slave error discards every slot, including ones that were still valid from an earlier successful refresh. An administratively deactivated OST is enough to reach this: lod_tgt_quotactl_async() returns -ENETDOWN for `!ltd_active || ltd_reap`, mdt_qmt_fetch_usgs_batch() propagates it, and the whole refresh unwinds here. From then on `lfs quota --lqa` reports usage as unknown for as long as that target stays down, while plain `lfs quota` for the same IDs keeps working because qmt_apply_usg_fetch() keeps the cached value or substitutes granted. The commit message presents the all-or-nothing abort as deliberate, so this is a design question rather than a claim that it is wrong: is a target the admin took offline on purpose really a reason to stop reporting LQA usage entirely, rather than reusing that slave's last cached value the way the per-ID path does?
owner Keguang Xu · uploader Keguang Xu · PS7 uploaded 44d ago · NEW · open in Gerrit ↗
commit message
LU-20354 quota: serve LQA usage from QMT cache GETQUOTALQA previously returned limits only. Wire it through the usage cache so lfs quota --lqa can set QIF_SPACE/QIF_INODES from the LQA shared lqe (qid=1). LQA has no llite fan-out and no per-slave DQACQ piggyback, so a miss collects glb IDs in the current ranges and batch-fetches each slave with LUSTRE_Q_GETOUSAGES. Per-slave range sums land on the shared lqe. A slave or setup failure aborts the refresh (no granted fallback, no partial sum) and leaves usage unknown; GETQUOTALQA still returns limits. QIF_SPACE follows DT alone and QIF_INODES follows MD alone. Missing QIF_USAGE is unknown in lfs, not -EIO. lfs quota --lqa accepts -U/-G/-P before or after --lqa, and prints "Disk quotas for <type> LQA <name>:". Range add/remove bumps qpi_lqa_gen with the rbtree update. Later GETQUOTALQA sees lqu_lqa_gen != qpi_lqa_gen, treats the slots as stale, and refreshes. The bump stays under the same write_lock as insert/erase so the new tree and new generation become visible together. GETOUSAGES carries a swabbed __u32 ID list (RMF_OBD_QUOTA_IDS) and lquota_acct_rec reply array (RMF_OBD_QUOTA_ACCT), not the opaque ITER blob used by ITEROQUOTA. A second qtype adding the same DOM MDT still increments LQA DT qpi_slv_nr on -EEXIST. LQA TTL follows usage_cache_ttl, including 0 (refetch on every GETQUOTALQA, same as per-ID GETQUOTA). Add MDT/OFD GETOUSAGES handlers, OSP/LOD iobuf async, and mdt_qmt_fetch_usgs_batch. Cover with kunit and sanity-quota/312. Document LQA behavior in the usage_cache_* man pages. Assisted-by: Cursor:Grok-4.6 llm_code_and_review_tools Signed-off-by: Keguang Xu <kxu@ddn.com> Change-Id: I63f53ba1d21f2bb6cc419c3d09b3f9337eaa1f17
68598
master
J ✗17B ✓T ✗8R 0/2
LU-20354 quota: aggregate MD inode usage for GETQUOTA
janitor: 17 test failures unique to this patch — sanity-quota@ldiskfs+DNE:test_16a, sanity-quota@ldiskfs+DNE:test_17, sanity-quota@ldiskfs+DNE:test_33 +14 more
17 unique failures
11 unresolved
XL +813 −225
44d
LU-20354 quota: aggregate MD inode usage for GETQUOTA
janitor: 17 test failures unique to this patch — sanity-quota@ldiskfs+DNE:test_16a, sanity-quota@ldiskfs+DNE:test_17, sanity-quota@ldiskfs+DNE:test_33 +14 more
17 unique failures
11 unresolved
XL +813 −225
44d
janitor: 17 test failures unique to this patch — sanity-quota@ldiskfs+DNE:test_16a, sanity-quota@ldiskfs+DNE:test_17, sanity-quota@ldiskfs+DNE:test_33 +14 more
enforced failed: review-dne-part-4, review-dne-zfs-part-3, review-dne-zfs-part-4, review-dne-zfs-part-7, review-zfs
11 unresolved thread(s) await your reply — last from Gerrit AI review for Lustre
janitor flagged test failures unique to this patch — full janitor results
| unique failing test | history |
|---|---|
| sanity-quota@ldiskfs+DNE:test_16a | seen in 2 other reviews |
| sanity-quota@ldiskfs+DNE:test_17 | seen in 7 other reviews |
| sanity-quota@ldiskfs+DNE:test_33 | seen in 1 other review |
| sanity-quota@ldiskfs+DNE:test_34 | seen in 1 other review |
| sanity-quota@ldiskfs+DNE:test_35 | seen in 1 other review |
| sanity-quota@ldiskfs+DNE:test_37 | seen in 3 other reviews |
| sanity-quota@ldiskfs+DNE:test_40c | seen in 2 other reviews |
| sanity-quota@ldiskfs+DNE:test_64 | seen in 2 other reviews |
| sanity-quota@ldiskfs+DNE:test_78A | seen in 4 other reviews |
| sanity-quota@ldiskfs+DNE:test_78a | seen in 3 other reviews |
| sanity-quota@zfs:test_17 | NEW unique failure for this branch in the last 30 days, and was seen 1 times across 1 other branches 1 reviews |
| sanity-quota@zfs:test_33 | seen in 1 other review |
| sanity-quota@zfs:test_34 | seen in 1 other review |
| sanity-quota@zfs:test_35 | seen in 1 other review |
| sanity-quota@zfs:test_37 | seen in 1 other review |
| sanity-quota@zfs:test_64 | seen in 2 other reviews |
| sanity-quota@zfs:test_75 | seen in 1 other review |
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-dne-part-4 | RHEL 9.7 / x86_64 | ran 9 tests. 1 tests failed: sanity-quota. | session |
| review-dne-part-4 | RHEL 10.1 / x86_64 | ran 9 tests. 1 tests failed: sanity-quota. | session |
| review-dne-zfs-part-3 | RHEL 9.8 / x86_64 | ran 3 tests. 1 tests failed: conf-sanity. | session |
| review-dne-zfs-part-3 | RHEL 10.1 / x86_64 | ran 3 tests. 1 tests failed: conf-sanity. | session |
| review-dne-zfs-part-4 | RHEL 9.8 / x86_64 | ran 9 tests. 1 tests failed: sanity-quota. | session |
| review-dne-zfs-part-4 | RHEL 10.1 / x86_64 | ran 9 tests. 1 tests failed: sanity-quota. | session |
| review-dne-zfs-part-7 | RHEL 9.8 / x86_64 | ran 6 tests. 1 tests failed: sanity-ec. | session |
| review-zfs | RHEL 8.10 / x86_64 | ran 8 tests. 1 tests failed: sanity-quota. | session |
build #132032 SUCCESS · tests all Maloo sessions
patchsets:
PS1 09-03
PS3 09-14
PS4 09-17
PS5 09-20
threads: 11 need your reply · 0 waiting on others · 0 own notes · 0 bot
reply Gerrit AI review for Lustre COMMIT_MSG:40 — (minor) The `nrpc` out-parameter added to `quf_fetch_batch()` (lustre_quota.h, mdt_handler.c, qmt_entry.c) also changes what `usage_cache_st
Gerrit AI review for Lustre
(minor) The `nrpc` out-parameter added to `quf_fetch_batch()` (lustre_quota.h, mdt_handler.c, qmt_entry.c) also changes what `usage_cache_stats fetch_rpc` counts. It used to be `atomic64_add(nstale, ...)` issued before the fetch, so it counted every stale slot, including the local MDT slot that never produces an RPC; it is now only the OSP requests actually queued. That looks like an improvement, but it is a visible change to an existing counter and the body only mentions the degraded/fallback split. Worth a sentence here, since sanity-quota 310/311 now key off the new meaning.
reply Gerrit AI review for Lustre lustre_quota.h:131 — (style) Not a bug, but now that this also carries KEY_MDT_QUOTACTL, the `ost_` / `oqi_` naming reads as OST-only: `mdt_qmt_fetch_usage_batch
Gerrit AI review for Lustre
(style) Not a bug, but now that this also carries KEY_MDT_QUOTACTL, the `ost_` / `oqi_` naming reads as OST-only: `mdt_qmt_fetch_usage_batch()` declares `struct ost_qctl_info oqi` for the remote-MDT case. `lod_ost_quotactl()` got renamed to `lod_tgt_quotactl()` in this patch for exactly that reason; `struct tgt_qctl_info` with `tqi_*` fields would match, if the patch is refreshed anyway.
reply Misc Code Checks Robot (Gatekeeper helper) osp_dev.c:1571 — error: osp_quotactl_prep():'osp' dereferencing possible ERR_PTR()
Misc Code Checks Robot (Gatekeeper helper)
error: osp_quotactl_prep():'osp' dereferencing possible ERR_PTR()
reply Misc Code Checks Robot (Gatekeeper helper) osp_dev.c:1571 — error: osp_quotactl_prep():'osp' dereferencing possible ERR_PTR()
Misc Code Checks Robot (Gatekeeper helper)
error: osp_quotactl_prep():'osp' dereferencing possible ERR_PTR()
reply Misc Code Checks Robot (Gatekeeper helper) osp_dev.c:1571 — error: osp_quotactl_prep():'osp' dereferencing possible ERR_PTR()
Misc Code Checks Robot (Gatekeeper helper)
error: osp_quotactl_prep():'osp' dereferencing possible ERR_PTR()
reply Misc Code Checks Robot (Gatekeeper helper) osp_dev.c:1571 — error: osp_quotactl_prep():'osp' dereferencing possible ERR_PTR()
Misc Code Checks Robot (Gatekeeper helper)
error: osp_quotactl_prep():'osp' dereferencing possible ERR_PTR()
reply Gerrit AI review for Lustre qmt_entry.c:1601 — (minor) Was promoting these from LQUOTA_DEBUG to LQUOTA_WARN intended? D_WARNING is in D_CANTMASK, so both of these now reach the console (r
Gerrit AI review for Lustre
(minor) Was promoting these from LQUOTA_DEBUG to LQUOTA_WARN intended? D_WARNING is in D_CANTMASK, so both of these now reach the console (rate-limited by the cdls, but still console output). A slave that is briefly unavailable is routine - an OST in recovery answers -EAGAIN, an inactive target gives -ENETDOWN out of lod_tgt_quotactl_async() - and with usage_cache_enabled defaulting to 1 this path is now taken on ordinary `lfs quota` calls, where it previously produced nothing on the console. The degraded counter and qmt.usage_cache_stats(4) already cover the same condition for anyone watching for it. If the console message is wanted, it is probably worth a line in the commit message, since it is new output on a healthy filesystem.
reply Gerrit AI review for Lustre qmt_entry.c:1699 — (minor) The comment that this replaces said the rescan from key 0 was needed because "slots may be added while the lock was dropped for fetc
Gerrit AI review for Lustre
(minor) The comment that this replaces said the rescan from key 0 was needed because "slots may be added while the lock was dropped for fetch/apply". Resuming from the last key drops that property: qmt_usg_extend_sites() -> qmt_update_lqe_usg_entry() only takes lqe_usg_data_lock, it does not wait on lqu_fetching the way qmt_del_lqe_usg_entry() does, so a slave connecting mid-refresh can insert a slot whose key sorts below resume (MDT keys are below OST keys, and a pool add can bring in a lower OST index). That slot stays QMT_USG_UNSET, so the following qmt_sum_lqe_usg(..., check_ttl=false) returns -ESTALE and the whole GETQUOTA falls back to client fan-out. It self-heals on the next call, so this is not a correctness problem, but the commit message presents the resume as a pure optimization. Would a final pass that only looks for lue_usage == QMT_USG_UNSET work here? It still terminates, since a fetched slot is no longer UNSET.
reply Gerrit AI review for Lustre qmt_handler.c:104 — (defect) With `usage_cache_enabled` defaulting to 1, this refresh now runs for every quota ID a client asks about, including IDs that have n
Gerrit AI review for Lustre
(defect) With `usage_cache_enabled` defaulting to 1, this refresh now runs for every quota ID a client asks about, including IDs that have no limits at all and have never been seen before.
`check_owner()` in `ll_quotactl()` only owner-checks USRQUOTA and GRPQUOTA; PRJQUOTA falls through, so an unprivileged user on any client can run `lfs quota -p <projid>` for arbitrary project IDs. Each unseen ID then does, on the MDT0 service thread:
qmt_pool_lqe_lookup() -> lqe_locate_find() /* creates + hashes an lqe */
qmt_get_usage(DT) -> qmt_setup_lqe_usg() + this refresh
qmt_get_usage(MD) -> qmt_setup_lqe_usg() + this refresh
so two full Q_GETOQUOTA fan-outs to every OST/MDT plus a per-slave `lqe_usg_data` attached to the new lqe. QMT lqes are only dropped at site teardown, so the cache grows with the number of distinct IDs queried.
The lqe-per-queried-ID part is pre-existing, but until this patch `qmt_usg_serve` was hard-wired to 0 with no tunable, so `qmt_get_usage()` returned -EAGAIN and neither the slave RPCs nor the per-slave allocation happened; the fan-out ran on the client, in the client's own context and in parallel.
Would it work to skip the refresh when `!lqe->lqe_enforced`, leaving usage unset so the client keeps the legacy fan-out for IDs with no limits? That would cover the enumeration case and still serve the IDs that matter. Otherwise, is defaulting the tunable to 0 the safer landing state?
reply Gerrit AI review for Lustre qmt_handler.c:113 — (minor) `qmt_quotactl()` calls `qmt_get_usage()` twice now, once for DT and once for MD, so `hit`, `miss` and `fallback` became per-resource
Gerrit AI review for Lustre
(minor) `qmt_quotactl()` calls `qmt_get_usage()` twice now, once for DT and once for MD, so `hit`, `miss` and `fallback` became per-resource rather than per-GETQUOTA. One request where both aggregates fail bumps `fallback` twice, and a request where only DT fails bumps it even though `QIF_INODES` was served. The `qmt_usg_fallback` comment in qmt_internal.h (`GETQUOTA unserved`) and the `fallback` text in qmt.usage_cache_stats.4 (`GETQUOTA leaves usage unset so the client fans out`) both read as per-request. If the patch is refreshed, either move the counting up into qmt_quotactl() or reword both to say per-resource.
reply Gerrit AI review for Lustre sanity-quota.sh:8105 — (suggestion) The commit message says 310 is extended "for inode serve", but the test only ever reads --space; nothing asserts the value that
Gerrit AI review for Lustre
(suggestion) The commit message says 310 is extended "for inode serve", but the test only ever reads --space; nothing asserts the value that the MD aggregate produces. The QIF_SPACE gate means a passing space check proves rc_md was 0, so it shows MD *succeeded*, but not that dqb_curinodes is right.
311 does assert the inode count, but it is gated on MDSCOUNT >= 2, so a single-MDT run - the common sanity-quota config - never checks the headline behavior of this patch.
A second line next to this one would close that, e.g.
used=$($LFS quota -u $TSTUSR --inodes -q $DIR | tr -d '*')
(( used > 0 )) || quota_error u $TSTUSR "expected non-zero inode usage, got $used"
owner Keguang Xu · uploader Keguang Xu · PS5 uploaded 44d ago · NEW · open in Gerrit ↗
commit message
LU-20354 quota: aggregate MD inode usage for GETQUOTA Turn on the usage cache (qmt.*.usage_cache_enabled, default on) so GETQUOTA sums both DT and MD from the per-slave cache. Set QIF_INODES from the MD aggregate and require both DT and MD before QIF_SPACE so llite still fans out when inode usage is unavailable. A slave fetch failure substitutes granted (or keeps a prior cache) and still serves: granted may overstate usage, but one unreachable target must not force every client to fan out. Clearing the tunable leaves usage unset and clients keep legacy fan-out. Drop the DT-only usage-cache gates so MD-pool lqes get slots: qmt_setup_lqe_usg / qmt_update_lqe_usg_entry / qmt_usg_extend_sites and DQACQ warming now apply to LQUOTA_RES_MD (LQA still skipped). MD-pool slots return dqb_curinodes; DT (incl. DOM) stays stoqb(dqb_curspace). Fetch remote MDTs via MDT-OSP MDS_QUOTACTL (KEY_MDT_QUOTACTL). MDS/OSS NIDs belong in a trusted nodemap, so CLIENT_TO_FS map is identity and no skip-idmap handshake is needed. usage_cache_ttl=0 means no cache: each GETQUOTA misses and refetches. Unfilled slots use lue_usage=QMT_USG_UNSET; expire=0 is zero freshness (same as ttl=0) so a queued waiter walks again instead of skipping as still fresh. Resume xa_for_each_start across GETOQUOTA batches of QMT_USG_FETCH_PARALLEL 32 (was 16) instead of rescanning from key 0. A cold-cache GETQUOTA on a large filesystem runs the slave fan-out synchronously on MDT0 (DT then MD), serialized per id. That can be slower than legacy client-side parallel fan-out until the cache is warm. Split usage_cache_stats so degraded counts per-slave granted substitution (still served) and fallback counts an unusable aggregate (client fans out). Add kunit coverage for MD-pool sum and ttl-0 miss vs post-refresh sum. Extend sanity-quota/310 for inode serve, no OST fan-out while enabled, and legacy fan-out when disabled. Add 311 for multi-MDT remote fetch (ttl=0; wait for MDT1 get_dqblk before the OSP refresh). Document usage_cache_enabled, usage_cache_ttl, and usage_cache_stats. Assisted-by: Cursor:Grok-4.6 llm_code_and_review_tools Signed-off-by: Keguang Xu <kxu@ddn.com> Change-Id: Ic3864474ec2bf5619ccaf2a0c22c190c0c34a25a
67810
master
J !B ✓T ✗5R 0/2
LU-20354 quota: parallelize usage-cache GETOQUOTA
enforced failed: review-dne-ipv6, review-dne-part-5, review-dne-zfs-part-4, review-dne-zfs-part-7
1 unresolved
L +439 −172
50d
LU-20354 quota: parallelize usage-cache GETOQUOTA
enforced failed: review-dne-ipv6, review-dne-part-5, review-dne-zfs-part-4, review-dne-zfs-part-7
1 unresolved
L +439 −172
50d
enforced failed: review-dne-ipv6, review-dne-part-5, review-dne-zfs-part-4, review-dne-zfs-part-7
1 unresolved thread(s) await your reply — last from Gerrit AI review for Lustre
janitor run: 7 failing config(s), none unique to this patch — janitor results
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-dne-ipv6 | RHEL 9.7 / x86_64 | ran 4 tests. 1 tests failed: sanity. | session |
| review-dne-part-5 | RHEL 9.7 / x86_64 | ran 5 tests. 1 tests failed: lustre-rsync-test. | session |
| review-dne-zfs-part-4 | RHEL 9.8 / x86_64 | ran 9 tests. 1 tests failed: sanity-quota. | session |
| review-dne-zfs-part-7 | RHEL 9.8 / x86_64 | ran 6 tests. 1 tests failed: sanity-ec. | session |
| review-dne-zfs-part-7 | RHEL 9.7 / x86_64 | ran 6 tests. 1 tests failed: sanity-ec. | session |
build #131507 SUCCESS · tests all Maloo sessions
patchsets:
PS1 08-07
PS5 09-02
PS6 09-08
PS7 09-10
threads: 1 need your reply · 0 waiting on others · 0 own notes · 0 bot
reply Gerrit AI review for Lustre lod_dev.c:2468 — (style) This isn't a bug, but the function uses GOTO() while having no ENTRY and returning with a plain return. On a CDEBUG_ENTRY_EXIT build
Gerrit AI review for Lustre
(style) This isn't a bug, but the function uses GOTO() while having no ENTRY and returning with a plain return. On a CDEBUG_ENTRY_EXIT build that logs "Process leaving via out_put" with no matching entry, and the success path logs nothing at all. lod_ost_quotactl() right above uses ENTRY/RETURN - worth matching it if the patch is refreshed.
owner Keguang Xu · uploader Keguang Xu · PS7 uploaded 54d ago · NEW · open in Gerrit ↗
commit message
LU-20354 quota: parallelize usage-cache GETOQUOTA Refresh stale per-slave usage in parallel batches via quf_fetch_batch. MDT queues remote OST Q_GETOQUOTA on a ptlrpc_set through LOD/OSP o_quotactl_async. Local MDT (DOM) stays a synchronous lquotactl_slv in the same batch; remote MDT remains -EOPNOTSUPP. On fetch failure keep the pre-fetch snap_usage when the slot was previously populated, otherwise fall back to granted. Re-lookup the slot by key after the unlocked fetch; shrink waits on lqu_fetching so the entry is still present. This cuts cold-miss / TTL-expire latency on large OST counts without changing GETQUOTA fallback semantics for OST slaves. Assisted-by: Cursor:Grok-4.6 llm_code_and_review_tools Signed-off-by: Keguang Xu <kxu@ddn.com> Change-Id: I700aec594310b9ae2e1cbab0fa460fee15b7f3d3
59862
master
J !B ✓T ✗14R -1/2−
LU-17493 mdc: restore LDLM cancel on blocking callback
mine needs rebase — checkpatch: cannot be cherry-picked
needs rebase
stalled 371d
2 unresolved
M +168 −22
371d
LU-17493 mdc: restore LDLM cancel on blocking callback
mine needs rebase — checkpatch: cannot be cherry-picked
needs rebase
stalled 371d
2 unresolved
M +168 −22
371d
needs rebase — checkpatch: cannot be cherry-picked
enforced failed: review-dne-part-1, review-dne-part-5, review-dne-part-6, review-dne-selinux-ssk-part-1, review-dne-selinux-ssk-part-2, review-dne-zfs-part-1, review-dne-zfs-part-3, review-dne-zfs-part-5, review-ldiskfs, review-ldiskfs-ubuntu
2 unresolved thread(s) await your reply — last from Andreas Dilger
janitor run: 1 failing config(s), none unique to this patch — janitor results
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-dne-part-1 | RHEL 9.5 / x86_64 | ran 3 tests. 1 tests failed: sanity. | session |
| review-dne-part-1 | RHEL 8.10 / x86_64 | ran 3 tests. 1 tests failed: sanity. | session |
| review-dne-part-5 | RHEL 8.10 / x86_64 | ran 5 tests. 2 tests failed: sanityn, recovery-small. | session |
| review-dne-part-5 | RHEL 9.5 / x86_64 | ran 5 tests. 2 tests failed: sanityn, recovery-small. | session |
| review-dne-part-6 | RHEL 9.5 / x86_64 | ran 6 tests. 1 tests failed: replay-single. | session |
| review-dne-selinux-ssk-part-1 | RHEL 8.10 / x86_64 | ran 3 tests. 1 tests failed: sanity. | session |
| review-dne-selinux-ssk-part-2 | RHEL 8.10 / x86_64 | ran 5 tests. 1 tests failed: recovery-small. | session |
| review-dne-zfs-part-1 | RHEL 8.10 / x86_64 | ran 3 tests. 1 tests failed: sanity. | session |
| review-dne-zfs-part-3 | RHEL 8.10 / x86_64 | ran 3 tests. 1 tests failed: conf-sanity. | session |
| review-dne-zfs-part-5 | RHEL 8.10 / x86_64 | ran 5 tests. 3 tests failed: sanityn, recovery-small, lustre-rsync-test. | session |
| review-ldiskfs | RHEL 8.10 / x86_64, SLES 15.5 / x86_64 | ran 5 tests. 1 tests failed: sanity. | session |
| review-ldiskfs | RHEL 8.10 / x86_64, RHEL 9.5 / x86_64 | ran 5 tests. 2 tests failed: node-reset, sanity. | session |
| review-ldiskfs | RHEL 8.10 / x86_64, RHEL 9.5 / x86_64 | ran 5 tests. 1 tests failed: sanity. | session |
| review-ldiskfs-ubuntu | RHEL 8.10 / x86_64, Ubuntu 22.04 / x86_64 | ran 6 tests. 1 tests failed: sanity. | session |
4 optional test failure(s) — informational
build #114694 SUCCESS · tests all Maloo sessions
no vote yet: Andreas Dilger, Lai Siyao, Oleg Drokin, Patrick Farrell
threads: 2 need your reply · 0 waiting on others · 0 own notes · 0 bot
reply Andreas Dilger sanityn.sh:330 — (minor) up to .59 now
Andreas Dilger
(minor) up to .59 now
reply Andreas Dilger sanityn.sh:340 — (style) no need for linefeed escape '\' after "||" or "&&" at end of line
Andreas Dilger
(style) no need for linefeed escape '\' after "||" or "&&" at end of line
owner kg.xu · uploader kg.xu · PS3 uploaded 490d ago · NEW · open in Gerrit ↗
commit message
LU-17493 mdc: restore LDLM cancel on blocking callback In highly contended directories like ROOT/, /home, /projects, abnormal clients holds LCK_PR executing 'ls' may block all other operations. To prevent this, we can prioritize directory modification responsiveness over strict readdir() cache coherency. This approach is supported by the POSIX standard, which does not require full cache coherency for readdir() across processes on the same node (discussed in LU-3308). A directory is considered 'contended' if it falls into one of the following three categories: 1. Critical directories: essential system directories like ROOT/, /home, and /projects. 2. Directories with racing locks: directories experiencing significant lock contention can be identified by checking ldlm_res_check_contention(). To improve performance in these situations, we can yield the CPU during ll_iterate() -> xxx -> mdc_read_page() using the lock flags LDLM_FL_CANCEL_ON_BLOCK and LDLM_FL_CANCEL_ON_CONTEND. - Client-side: The client checks for conditions 1 and sets the LDLM_FL_CANCEL_ON_BLOCK flag. - MDS-side: MDS has complete lock contention information, this could be used to set the LDLM_FL_CANCEL_ON_CONTEND flag on locks. By setting these flags, the LDLM will release the 'ls' locks and quickly grant other pending locks for modifications, ensuring a more responsive system. Note.1: ll_getattr() that satifies condiction 1&2 has been updated accordingly as well. Since its lock:LCK_PR(UPDATE|PERM) encompasses the lock:LCK_PR(UPDATE) required by ll_iterate(), it's preferrable to set CANCEL_ON_BLOCK at the first place in ll_getattr(). Failing to do so would result in this flag being absent from ll_iterate()'s lock. Note.2: we've leave ll_lookup as is. When ll_lookup() is invoked, it lacks the necessary information (specifically, dir_depth) to verify condition 1. Signed-off-by: Keguang Xu <squalfof@gmail.com> Change-Id: I90163f2bba64f7955cec0d538724ad363eed9abb
Mentions
0 waiting on you · 0 openUnresolved threads where someone @-mentioned you (username, email or full name) — the pings that drown in Gerrit's email. Red: you have not replied since the ping. Muted: you replied, but the thread is still open, so it stays on the list until someone resolves it. Both clear on the refresh after the thread is resolved.
No unanswered pings — nobody is waiting on a mention.
0 watched
Nothing watched. Add a change above — e.g. after rebasing someone's patch — and its build/test state shows up here.
3 open
▶Failed / broken
(2)
62497
master
J !B ✓T ✗5R -1/2−
LU-7266 ldlm: introduce pressure-based SLV
needs rebase — checkpatch: cannot be cherry-picked
needs rebase
11 unresolved
XL +2191 −68
27d
LU-7266 ldlm: introduce pressure-based SLV
needs rebase — checkpatch: cannot be cherry-picked
needs rebase
11 unresolved
XL +2191 −68
27d
needs rebase — checkpatch: cannot be cherry-picked
enforced failed: review-dne-zfs-part-4, review-dne-zfs-part-6, review-ldiskfs, review-ldiskfs-arm
8 unresolved thread(s) await your reply — last from Gerrit AI review for Lustre
1 thread(s) waiting on others
janitor run: 1 failing config(s), none unique to this patch — janitor results
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-dne-zfs-part-4 | RHEL 10.1 / x86_64 | ran 9 tests. 1 tests failed: sanity-quota. | session |
| review-dne-zfs-part-6 | RHEL 9.7 / x86_64 | ran 4 tests. 1 tests failed: ost-pools. | session |
| review-ldiskfs | RHEL 8.10 / x86_64 | ran 5 tests. 1 tests failed: sanity. | session |
| review-ldiskfs | RHEL 8.10 / x86_64, RHEL 9.7 / x86_64 | ran 5 tests. 1 tests failed: sanity. | session |
| review-ldiskfs-arm | RHEL 8.10 / x86_64, Rocky 9.5 / aarch64 | ran 6 tests. 1 tests failed: sanity. | session |
build #129438 SUCCESS · tests all Maloo sessions
no vote yet: Keguang Xu, Qian Yingjin, Andreas Dilger, Vitaly Fertman, Timothy Day, Patrick Farrell
patchsets:
PS24 07-14 Keguang Xu
PS25 08-08 Keguang Xu
PS26 08-09 Keguang Xu
PS27 08-15 Keguang Xu
PS28 08-15 Keguang Xu
+15 earlier
PS1 11-12 PS4 11-19 PS6 11-26 PS10 02-27 PS13 05-11 PS14 05-11 PS15 05-26 PS16 05-29 PS17 06-11 PS18 06-13 Keguang Xu PS19 06-22 Keguang Xu PS20 06-27 Keguang Xu PS21 07-13 Keguang Xu PS22 07-13 Keguang Xu PS23 07-13 Keguang Xuyour previous vote: -1 on PS19 (2026-06-22 01:12) — now at PS28
threads: 8 need your reply · 1 waiting on others · 2 own notes · 0 bot
note kg.xu patchset comment — Review note:
kg.xu
Review note: The patch is large because of negotiation, dual SLV, rollout, and tests. The new logic is small: server pressure is essentially granted/limit; the client maps pressure to an evict quota and uses a quota-based LRU policy instead of per-lock CLV arithmetic. Suggested review order: 1) lustre/include/lustre_dlm.h, lustre_idl.h — packed SLV layout and pool fields 2) ldlm_pool_calc_slv_by_pressure(), ldlm_pool_pressure_evict_*() — core math (~100 lines) 3) ldlm_cancel_pressure_policy(), ldlm_cancel_lru_policy() — reclaim behavior 4) connect, rollout, sysfs, tests — compatibility and rollout (bulk of the diff)
reply Gerrit AI review for Lustre ldlm.namespaces.pool.slv_mode.4:174 — Not a bug, but the MDT name in this sample output is missing the UUID suffix. mdt_init0() builds the namespace name as "%s-%s" of LUSTRE_MDT
Gerrit AI review for Lustre
Not a bug, but the MDT name in this sample output is missing the UUID suffix. mdt_init0() builds the namespace name as "%s-%s" of LUSTRE_MDT_NAME and obd_uuid.uuid, so it reads mdt-testfs-MDT0000_UUID, matching the filter-testfs-OST0000_UUID line just below.
note kg.xu ldlm_pool.c:309 — Currently, `ldlm_pool` tracks metrics at the per-target level (e.g., 'fsname-target'). I propose aggregating these metrics by 'fsname' inste
kg.xu
Currently, `ldlm_pool` tracks metrics at the per-target level (e.g., 'fsname-target'). I propose aggregating these metrics by 'fsname' instead. Furthermore, we should discuss whether resources should be allocated equally across all file systems, or if we need a weighted policy.
reply Gerrit AI review for Lustre ldlm_pool.c:212 — (style) Not a bug, but with `pressure_adjust == true` this just returns `L` unchanged, and the only caller passing true is the CDEBUG in ldl
Gerrit AI review for Lustre
(style) Not a bug, but with `pressure_adjust == true` this just returns `L` unchanged, and the only caller passing true is the CDEBUG in ldlm_pool_calc_slv_by_pressure(). A boolean that turns the function into the identity is hard to follow - would printing `limit` directly there be clearer?
reply Gerrit AI review for Lustre ldlm_pool.c:1856 — (style) These six new helpers are the only EXPORT_SYMBOL()s in this file; ldlm_pool_get_slv(), ldlm_pool_init() and friends aren't exported.
Gerrit AI review for Lustre
(style) These six new helpers are the only EXPORT_SYMBOL()s in this file; ldlm_pool_get_slv(), ldlm_pool_init() and friends aren't exported. ldlm_lib.c and ldlm_request.c build into the same ptlrpc module (lustre/ldlm/Makefile), so none of them need exporting, and this one is used only inside ldlm_pool.c so it could be static.
reply Gerrit AI review for Lustre ldlm_pool.c:497 — Once the pool is pressure-ready these two limits diverge by roughly 5x, and the legacy scalar looks like it collapses to the floor.
Gerrit AI review for Lustre
Once the pool is pressure-ready these two limits diverge by roughly 5x, and the legacy scalar looks like it collapses to the floor.
ldlm_pools_recalc_task() sets pl_limit = ldlm_ratio2locknr(30)/nr_ns for a ready pool but pl_legacy_limit = LDLM_POOL_HOST_L/nr_ns. HOST_L is 50 locks per MB of RAM, i.e. RAM/20971 locks; ldlm_ratio2locknr(30) is 0.30*RAM/LDLM_LOCK_MEM_OVERHEAD. Packed clients only start evicting at 50% of pl_limit (default curve), so granted settles well above pl_legacy_limit.
With granted > limit here:
grant_usage = max_t(int, limit - (granted - grant_plan), 1); /* -> 1 */
slv_factor = (grant_usage << 10); do_div(slv_factor, limit); /* -> 0 */
slv = slv * slv_factor; /* -> 0 */
and the clamp below lifts it to ldlm_pool_slv_min() == 1. In ldlm_cancel_lrur_policy() slv == 1 means every lock with lv >= 1 (any lock idle for a second or more) is cancelled, so a non-packed client drops essentially its whole LRU on each recalc, and the drop also trips the ns_recalc_pct urgent-recalc path in ldlm_cli_update_pool().
That hits up to 30% of client exports at the default threshold, plus every MDT OSC on an OST since lod never negotiates OBD_CONNECT2_POOL_SLV_PACKED. Does "keep legacy behavior for old clients" still hold once the pool flips, or should the legacy scalar be derived from the limit the pool is actually being filled to?
reply Gerrit AI review for Lustre ldlm_pool.c:1535 — pl_slv_mode is set to pressure_rollout two lines up, so this branch always runs and a brand new server pool gets the pressure budget before
Gerrit AI review for Lustre
pl_slv_mode is set to pressure_rollout two lines up, so this branch always runs and a brand new server pool gets the pressure budget before any client has negotiated packed SLV and before ldlm_pool_fallback_legacy_on_multi_fs_register() has had a chance to force the pool back to legacy_slv. It is also the whole-node budget: ldlm_pools_recalc_task() divides ldlm_ratio2locknr() by ldlm_namespace_nr_read(LDLM_NAMESPACE_SERVER), this does not. The neighbouring initialisers (pl_legacy_limit, pl_grant_plan) still use LDLM_POOL_HOST_L, so pl_server_lock_volume is seeded from one basis and decayed against another until the first recalc pass. Would it be simpler to seed with LDLM_POOL_HOST_L here and let ldlm_pools_recalc_task() apply the pressure budget when the readiness gate says so?
reply Qian Yingjin sanity-ldlm-pool.sh:195 — This could be:
Qian Yingjin
This could be: for ((batch = 0; batch < BATCH_COUNT; batch++)) ...
reply Gerrit AI review for Lustre sanity-ldlm-pool.sh:190 — (style) This commented-out curve set/restore block looks like leftover scaffolding - drop it, or enable it if the suite is meant to cover a
Gerrit AI review for Lustre
(style) This commented-out curve set/restore block looks like leftover scaffolding - drop it, or enable it if the suite is meant to cover a non-default curve?
reply Gerrit AI review for Lustre sanity-ldlm-pool.sh:260 — (style) Carried over from patchset 9 and still open: this reads more naturally as `for ((batch = 0; batch < BATCH_COUNT; batch++))`, with su
Gerrit AI review for Lustre
(style) Carried over from patchset 9 and still open: this reads more naturally as `for ((batch = 0; batch < BATCH_COUNT; batch++))`, with subdir_idx computed from batch directly.
owner kg.xu · uploader Keguang Xu · PS28 uploaded 80d ago · NEW · open in Gerrit ↗
commit message
LU-7266 ldlm: introduce pressure-based SLV Legacy SLV/CLV reacts to server pressure indirectly and can delay lock reclaim. This change introduces a pressure-based SLV path: server sends packed pressure in SLV, and client converts it to evict quota. Packed SLV Format: * Bits 63..60: format version (current 0x1). Legacy SLV leaves these bits clear. * Bits 59..16: reserved for future use. * Bits 15..0: pressure in hundredths of a percent, where 10000 means 100.00%. Here pressure is total-grant-lock/total-limit. Unlike the previous grant-plan-based scalar SLV. Rationale: - Percentage-based eviction scales with client lock footprint, so larger lock holders evict more locks under pressure while smaller clients are not over-penalized. - Gradual rollout is gated by packed-capable export coverage, avoiding a mixed-upgrade case where only a small subset of new clients absorbs repeated reclaim pressure. - Client eviction uses segmented pressure-to-evict mapping, so reclaim is mild at low pressure and stronger at high pressure. At relatively low pressure, clients react faster to keep pressure in a healthy range; at higher pressure, clients use a longer evict interval because each round evicts more locks, reducing oscillation and avoiding over-evicting locks that should be retained; at or above the last curve point the interval shortens so high-pressure reclaim stays frequent. Key updates: - add OBD_CONNECT2_POOL_SLV_PACKED negotiation and packed SLV helpers - add slv_mode (legacy_slv / pressure_rollout), rollout threshold, and client evict-curve sysfs controls - pressure_rollout pools use ldlm.lock_pool_limit_mem_pct (default 30% of RAM / LDLM_LOCK_MEM_OVERHEAD) instead of LDLM_POOL_HOST_L - keep dual SLV state (legacy scalar + pressure scalar) for compatibility, where in pressure path; add quota-based client reclaim policy: ldlm_cancel_pressure_policy - fall back to legacy_slv on mixed-filesystem server nodes - keep legacy behavior for old clients and pre-threshold rollout stages - add sanity-ldlm-pool.sh, sanity 124aa, and man pages for the new pool parameters Performance impact: CPU overhead should be negligible. - The server recomputes pressure once per second per pool. On the client, pool recalc runs on the ~10s timer and when unused locks are decrefed. - On the packed pressure path, reclaim no longer performs per-lock arithmetic during LRU scanning, so lock-heavy workloads should see unchanged or slightly lower CPU usage. Assisted-by: Cursor:auto/codex5.5 Signed-off-by: Keguang Xu <squalfof@gmail.com> Signed-off-by: Keguang Xu <kxu@ddn.com> Change-Id: If8a6f247127576965ae45c349a4ab04fb286615a
59862
master
J !B ✓T ✗14R -1/2−
LU-17493 mdc: restore LDLM cancel on blocking callback
needs rebase — checkpatch: cannot be cherry-picked
needs rebase
stalled 371d
2 unresolved
M +168 −22
371d
LU-17493 mdc: restore LDLM cancel on blocking callback
needs rebase — checkpatch: cannot be cherry-picked
needs rebase
stalled 371d
2 unresolved
M +168 −22
371d
needs rebase — checkpatch: cannot be cherry-picked
enforced failed: review-dne-part-1, review-dne-part-5, review-dne-part-6, review-dne-selinux-ssk-part-1, review-dne-selinux-ssk-part-2, review-dne-zfs-part-1, review-dne-zfs-part-3, review-dne-zfs-part-5, review-ldiskfs, review-ldiskfs-ubuntu
2 unresolved thread(s) await your reply — last from Andreas Dilger
janitor run: 1 failing config(s), none unique to this patch — janitor results
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-dne-part-1 | RHEL 9.5 / x86_64 | ran 3 tests. 1 tests failed: sanity. | session |
| review-dne-part-1 | RHEL 8.10 / x86_64 | ran 3 tests. 1 tests failed: sanity. | session |
| review-dne-part-5 | RHEL 8.10 / x86_64 | ran 5 tests. 2 tests failed: sanityn, recovery-small. | session |
| review-dne-part-5 | RHEL 9.5 / x86_64 | ran 5 tests. 2 tests failed: sanityn, recovery-small. | session |
| review-dne-part-6 | RHEL 9.5 / x86_64 | ran 6 tests. 1 tests failed: replay-single. | session |
| review-dne-selinux-ssk-part-1 | RHEL 8.10 / x86_64 | ran 3 tests. 1 tests failed: sanity. | session |
| review-dne-selinux-ssk-part-2 | RHEL 8.10 / x86_64 | ran 5 tests. 1 tests failed: recovery-small. | session |
| review-dne-zfs-part-1 | RHEL 8.10 / x86_64 | ran 3 tests. 1 tests failed: sanity. | session |
| review-dne-zfs-part-3 | RHEL 8.10 / x86_64 | ran 3 tests. 1 tests failed: conf-sanity. | session |
| review-dne-zfs-part-5 | RHEL 8.10 / x86_64 | ran 5 tests. 3 tests failed: sanityn, recovery-small, lustre-rsync-test. | session |
| review-ldiskfs | RHEL 8.10 / x86_64, SLES 15.5 / x86_64 | ran 5 tests. 1 tests failed: sanity. | session |
| review-ldiskfs | RHEL 8.10 / x86_64, RHEL 9.5 / x86_64 | ran 5 tests. 2 tests failed: node-reset, sanity. | session |
| review-ldiskfs | RHEL 8.10 / x86_64, RHEL 9.5 / x86_64 | ran 5 tests. 1 tests failed: sanity. | session |
| review-ldiskfs-ubuntu | RHEL 8.10 / x86_64, Ubuntu 22.04 / x86_64 | ran 6 tests. 1 tests failed: sanity. | session |
4 optional test failure(s) — informational
build #114694 SUCCESS · tests all Maloo sessions
no vote yet: Andreas Dilger, Lai Siyao, Oleg Drokin, Patrick Farrell
threads: 2 need your reply · 0 waiting on others · 0 own notes · 0 bot
reply Andreas Dilger sanityn.sh:330 — (minor) up to .59 now
Andreas Dilger
(minor) up to .59 now
reply Andreas Dilger sanityn.sh:340 — (style) no need for linefeed escape '\' after "||" or "&&" at end of line
Andreas Dilger
(style) no need for linefeed escape '\' after "||" or "&&" at end of line
owner kg.xu · uploader kg.xu · PS3 uploaded 490d ago · NEW · open in Gerrit ↗
commit message
LU-17493 mdc: restore LDLM cancel on blocking callback In highly contended directories like ROOT/, /home, /projects, abnormal clients holds LCK_PR executing 'ls' may block all other operations. To prevent this, we can prioritize directory modification responsiveness over strict readdir() cache coherency. This approach is supported by the POSIX standard, which does not require full cache coherency for readdir() across processes on the same node (discussed in LU-3308). A directory is considered 'contended' if it falls into one of the following three categories: 1. Critical directories: essential system directories like ROOT/, /home, and /projects. 2. Directories with racing locks: directories experiencing significant lock contention can be identified by checking ldlm_res_check_contention(). To improve performance in these situations, we can yield the CPU during ll_iterate() -> xxx -> mdc_read_page() using the lock flags LDLM_FL_CANCEL_ON_BLOCK and LDLM_FL_CANCEL_ON_CONTEND. - Client-side: The client checks for conditions 1 and sets the LDLM_FL_CANCEL_ON_BLOCK flag. - MDS-side: MDS has complete lock contention information, this could be used to set the LDLM_FL_CANCEL_ON_CONTEND flag on locks. By setting these flags, the LDLM will release the 'ls' locks and quickly grant other pending locks for modifications, ensuring a more responsive system. Note.1: ll_getattr() that satifies condiction 1&2 has been updated accordingly as well. Since its lock:LCK_PR(UPDATE|PERM) encompasses the lock:LCK_PR(UPDATE) required by ll_iterate(), it's preferrable to set CANCEL_ON_BLOCK at the first place in ll_getattr(). Failing to do so would result in this flag being absent from ll_iterate()'s lock. Note.2: we've leave ll_lookup as is. When ll_lookup() is invoked, it lacks the necessary information (specifically, dir_depth) to verify condition 1. Signed-off-by: Keguang Xu <squalfof@gmail.com> Change-Id: I90163f2bba64f7955cec0d538724ad363eed9abb
▶Review feedback — your turn
(0)
nothing here right now
▶In CI
(0)
nothing here right now
▶Needs reviewers
(0)
nothing here right now
▶Waiting on reviewers
(0)
nothing here right now
▶Ready to land
(0)
nothing here right now
▶Parked (WIP / self-blocked)
(1)
66190
master
J ✗3B ✗T –R -1/2−
LU-12550 ldlm: extent expansion with new_contention signal
work in progress
needs rebase
3 unique failures
WIP
1 unresolved
M +112 −4
134d
LU-12550 ldlm: extent expansion with new_contention signal
work in progress
needs rebase
3 unique failures
WIP
1 unresolved
M +112 −4
134d
janitor flagged test failures unique to this patch — full janitor results
| unique failing test | history |
|---|---|
| sanityn@ldiskfs+DNE:test_16a | seen in 9 other reviews |
| sanityn@ldiskfs+DNE:test_16b | seen in 6 other reviews |
| sanityn@ldiskfs+DNE:test_16c | seen in 9 other reviews |
build #125844 FAILURE
no vote yet: Qian Yingjin, Andreas Dilger, Patrick Farrell
patchsets:
PS1 05-27
PS2 05-28
PS3 06-01
threads: 0 need your reply · 0 waiting on others · 1 own notes · 0 bot
note kg.xu patchset comment — Follow-up to ldlm_contention (#35287): also cap PW/CW extent lock expansion when a resource is in the sustained contention hold window, not
kg.xu
Follow-up to ldlm_contention (#35287): also cap PW/CW extent lock expansion when a resource is in the sustained contention hold window, not only under high instantaneous conflict (conflicting > 32).
owner kg.xu · uploader kg.xu · PS3 uploaded 155d ago · NEW · open in Gerrit ↗
commit message
LU-12550 ldlm: extent expansion with new_contention signal Introduce ldlm_resource_is_contended() in ldlm extent expansion fixup, which checks whether the resource is still in the contention hold window. When contended, reset expand start to the requested start. Cap expanded end to req_start + LDLM_MAX_GROWN_EXTENT for small requests; cap to req_end + LDLM_MAX_GROWN_EXTENT otherwise. This helps to reduce lock cancellation. Signed-off-by: Keguang Xu <squalfof@gmail.com> Change-Id: Id8d98fc74b8b3df9caed85968acc210d1b2d5c60
Carrying — uploaded by me, owned by others
17 open▶Failed / broken
(5)
69703
master
J –B ✓T ✗5R 0/2
LU-20690 ec: verify parity against data, allow p > k
enforced failed: custom-1001, review-dne-part-7, review-dne-subtest-change, review-dne-zfs-part-7, review-dne-zfs-subtest-change
pt_ecroraid10-like218_review_prioritypw_shepherd
4 unresolved
L +338 −193
49m
LU-20690 ec: verify parity against data, allow p > k
enforced failed: custom-1001, review-dne-part-7, review-dne-subtest-change, review-dne-zfs-part-7, review-dne-zfs-subtest-change
pt_ecroraid10-like218_review_prioritypw_shepherd
4 unresolved
L +338 −193
49m
enforced failed: custom-1001, review-dne-part-7, review-dne-subtest-change, review-dne-zfs-part-7, review-dne-zfs-subtest-change
4 unresolved thread(s) await your reply — last from Keguang Xu
| failed enforced test | platform | detail | |
|---|---|---|---|
| custom-1001 | RHEL 9.7 / x86_64 | ran 3 tests. 1 tests failed: sanity-ec. | session |
| review-dne-part-7 | RHEL 9.8 / x86_64 | ran 6 tests. 1 tests failed: sanity-ec. | session |
| review-dne-subtest-change | RHEL 9.7 / x86_64 | ran 3 tests. 1 tests failed: sanity-ec. | session |
| review-dne-zfs-part-7 | RHEL 9.8 / x86_64 | ran 6 tests. 1 tests failed: sanity-ec. | session |
| review-dne-zfs-subtest-change | RHEL 9.7 / x86_64 | ran 3 tests. 1 tests failed: sanity-ec. | session |
build #135173 SUCCESS · tests all Maloo sessions
no vote yet: Ronnie Sahlberg, Marc Vef, Andreas Dilger
patchsets:
PS1 10-15
PS2 10-15
PS3 10-26
PS4 11-02
threads: 4 need your reply · 0 waiting on others · 0 own notes · 0 bot
reply Keguang Xu lod_lov.c:2242 — Thanks for catching this.
Gerrit AI review for Lustre
(defect) Can the MDS create a parity that this check then rejects?
With more than one raid set, lod_stripe_count_min() returns 0 for a parity, and lod_ost_alloc_rr() then reports success on a short allocation:
lod_ost_alloc_rr()
if (stripe_idx) rc = 0;
lod_ec_add_parities() /* may still be short, e.g. failure domains */
/* Still short; remember how many we got */
lod_comp->llc_stripe_count = stripe_idx;
stripe_idx < stripe_count_min (0) -> no -ENOSPC
lod_comp_prep_create() does not check the count afterwards, so the layout is stored with fewer than (n0 + n1) * p stripes. If the shortfall is a whole raid set, the count is still a multiple of p and passes lod_ec_parity_valid(). Before this patch such a layout went through open replay (LOV_MAGIC_DEFINED -> lod_qos_parse_config() -> lod_verify_striping()). Now the replay gets -EINVAL here, as do HSM release and xattr copies of the file.
Should RR fail a short EC parity the way lod_ost_alloc_qos() does ("we require the full stripe_count to avoid incomplete raid sets")? Otherwise this check is stricter than what the allocator guarantees.
Keguang Xu
Thanks for catching this. Full parity geometry is nsets * p (nsets = ec_raidset_count from the data stripe count and k). RR can finish with stripe_idx < nsets * p when nsets > 1: lod_stripe_count_min() is 0, so a short lod_ec_add_parities() still returns success and sets llc_stripe_count = stripe_idx. QoS instead requires stripe_idx == nsets * p. @rsahlberg@whamcloud.com @adilger@thelustrecollective.com: is stripe_idx < nsets * p for multi-raidset parity from RR intentional? A short layout (e.g. (nsets-1)*p) can break degraded read / EC recovery, leave some raid sets with no parity to rebuild from, and confuse resync/verify and mirror tools. QoS already refuses this. I think RR should match QoS (fail short EC parity) and the verifier should require count == nsets * p again — happy to follow up if you agree.
reply Gerrit AI review for Lustre lod_qos.c:1278 — (defect) Can the MDS still create an instantiated parity whose data component is not instantiated? If so, this check rejects a layout the MD
Gerrit AI review for Lustre
(defect) Can the MDS still create an instantiated parity whose data component is not instantiated? If so, this check rejects a layout the MDS wrote itself.
A mirror-targeted resync instantiates every component of the named mirror, whatever its extent:
lfs mirror copy -i <data> -o -1 f (or -o <parity>)
-> ll_file_resync() lil_mirror_id = parity mirror
-> lod_declare_update_rdonly() -> lod_prepare_resync_mirror()
-> lod_declare_instantiate_components() /* only needs lod_comp_find_linked() != NULL */
-> lod_comp_prep_create() /* sizes parity from data->llc_stripe_count */
lfs_mirror_copy() does not refuse a parity target, and lod_ec_check_data_usable() and lod_ec_add_parities() return early when data has no stripes allocated. With `-E 128M --ec 2+1 -E -1 -c 4 --ec 4+2` and less than 128M written, the parity of [128M, EOF) is stored INIT while its data component is not.
Before this patch that layout parsed fine, and lov_ea.c still skips such a pair (`!lsme_inited(data_entry)`). Now, once the object is reloaded (cache eviction or MDT restart), lod_parse_striping() returns -EINVAL. lod_declare_destroy() returns that error, so the file can no longer be unlinked, and resync and mirror split fail too.
Should an uninstantiated data peer be skipped here, the way the client does it? The other option is to refuse, in lod_declare_instantiate_components(), a parity whose data component is not instantiated and is not being instantiated in the same call.
reply Gerrit AI review for Lustre lod_qos.c:3960 — (minor) This comment doesn't seem to match lod_verify_ec_parity_comps(). That function skips parity components that are not instantiated (`!
Gerrit AI review for Lustre
(minor) This comment doesn't seem to match lod_verify_ec_parity_comps(). That function skips parity components that are not instantiated (`!lod_comp_inited(comp)` -> continue) and never looks at data components. lod_comp_prep_create() only runs on components that are being instantiated, so a layout from disk with a zero lcme_dstripe_count/lcme_cstripe_count still reaches this point unchecked. This test is still the only thing that keeps the `% 0` out of ec_split_stripes(). The old wording ("a layout loaded from disk does not pass through it again") was accurate. If the patch is refreshed, consider restoring it so the check below doesn't look redundant to the next reader.
reply Gerrit AI review for Lustre liblustreapi_layout.c:2833 — (minor) Now that layout_ec_verify() only rejects a zero or out-of-range data_stripe_count, Documentation/man3/llapi_layout_comp_add_parity_o
Gerrit AI review for Lustre
(minor) Now that layout_ec_verify() only rejects a zero or out-of-range data_stripe_count, Documentation/man3/llapi_layout_comp_add_parity_only.3 is out of date. It still says data_stripe_count is used for "bounding K+P", and lists EINVAL when the counts "did not fit data_stripe_count". If the patch is refreshed, consider updating both sentences to the new rule, where P may exceed the data stripe count.
tags: pt_ecro raid10-like 218_review_priority pw_shepherd
owner Keguang Xu · uploader Keguang Xu · PS4 uploaded 2d ago · NEW · open in Gerrit ↗
commit message
LU-20690 ec: verify parity against data, allow p > k With the RAID10-like parity layout the p parities of a raidset share one offset, so the parity of a component fits in its extent whatever p is. Remove the p <= k checks in lfs and llapi; k+p <= 256 and p <= 15 remain. Also drop the mirror-extend stripe_count < p guard in ec_check_data_comp() so --ec 2+3 works on a two-stripe target. An instantiated parity is checked against its data component in lod_verify_ec_parity_comps() after lod_use_defined_striping() / lod_parse_striping(), using lod_comp_find_linked() for the same pairing rule as the allocator (covers layout-change replay). Require a bidirectional link, matching k/p and stripe size, and parity stripe count ec_raidset_count() * p. lod_verify_striping() checks k/p and rejects an instantiated parity with stripe_count == 0 before DEFINED object init. Tests: sanity-ec 4a and 10 accept p > k, and 4b rejects k == 0 and k > 255. 8m covers mirror extend --ec 2+3 on a two-stripe target. 300 covers PFL -E 2M --ec 2+3 create/resync/verify, and 305 covers p > k multi-raidset create/resync/verify. llapi_layout_test test47 allows 3+4. Test-Parameters: testlist=sanity-ec ostcount=8 Signed-off-by: Keguang Xu <kxu@ddn.com> Assisted-by: Cursor:Composer llm_code_and_review_tools Change-Id: I3878502e8ff739cb5ed420ff359e2008376303e7
66567
master
J –B ✓T ✗10R 0/2
LU-20212 lov: skip invalid OST mirrors for aggregate I/O
cc Keguang Xu · enforced failed: review-dne-part-4, review-dne-part-7, review-dne-subtest-change, review-dne-zfs-part-4, review-dne-zfs-part-5, review-dne-zfs-part-7, review-dne-zfs-subtest-change, review-zfs
4 unresolved
L +216 −39
7d
LU-20212 lov: skip invalid OST mirrors for aggregate I/O
cc Keguang Xu · enforced failed: review-dne-part-4, review-dne-part-7, review-dne-subtest-change, review-dne-zfs-part-4, review-dne-zfs-part-5, review-dne-zfs-part-7, review-dne-zfs-subtest-change, review-zfs
4 unresolved
L +216 −39
7d
enforced failed: review-dne-part-4, review-dne-part-7, review-dne-subtest-change, review-dne-zfs-part-4, review-dne-zfs-part-5, review-dne-zfs-part-7, review-dne-zfs-subtest-change, review-zfs
3 thread(s) waiting on others
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-dne-part-4 | RHEL 9.7 / x86_64 | ran 11 tests. 1 tests failed: sanity-flr. | session |
| review-dne-part-4 | RHEL 10.1 / x86_64 | ran 11 tests. 1 tests failed: sanity-flr. | session |
| review-dne-part-7 | RHEL 9.8 / x86_64 | ran 6 tests. 1 tests failed: sanity-ec. | session |
| review-dne-subtest-change | RHEL 9.7 / x86_64 | ran 3 tests. 1 tests failed: sanity-flr. | session |
| review-dne-zfs-part-4 | RHEL 9.8 / x86_64 | ran 11 tests. 1 tests failed: sanity-flr. | session |
| review-dne-zfs-part-4 | RHEL 10.1 / x86_64 | ran 11 tests. 1 tests failed: sanity-flr. | session |
| review-dne-zfs-part-5 | RHEL 9.7 / x86_64 | ran 5 tests. 1 tests failed: sanityn. | session |
| review-dne-zfs-part-7 | RHEL 9.8 / x86_64 | ran 6 tests. 1 tests failed: sanity-ec. | session |
| review-dne-zfs-subtest-change | RHEL 9.7 / x86_64 | ran 3 tests. 1 tests failed: sanity-flr. | session |
| review-zfs | RHEL 8.10 / x86_64 | ran 10 tests. 1 tests failed: sanity-flr. | session |
build #134448 SUCCESS · tests all Maloo sessions
no vote yet: Qian Yingjin, Marc Vef, Andreas Dilger
patchsets:
PS3 06-11
PS4 06-13
PS5 06-16
PS6 06-22
PS7 10-26
+2 earlier
PS1 06-10 PS2 06-10threads: 0 need your reply · 3 waiting on others · 1 own notes · 0 bot
note kg.xu patchset comment — In lov_io_mirror_init(), if `ci_designated_mirror` is set, we can also check lov_mirror_has_inactive_stripe for `CIT_GLIMPSE/LSEEK/DATA_VERS
kg.xu
In lov_io_mirror_init(), if `ci_designated_mirror` is set, we can also check lov_mirror_has_inactive_stripe for `CIT_GLIMPSE/LSEEK/DATA_VERSION`. Since these io types all trigger OST aggregation, this check allows us to skip unnecessary RPCs. In current impl, we only check `CIT_GLIMPSE` during mirror selection (where `ci_designated_mirror` is false).
owner Keguang Xu · uploader Keguang Xu · PS7 uploaded 8d ago · NEW · open in Gerrit ↗
commit message
LU-20212 lov: skip invalid OST mirrors for aggregate I/O I/O that aggregates stripe-local results into one logical answer (glimpse, lseek, data_version) cannot succeed on a mirror while any relevant OST import is deactivated or invalid. In lov_io_mirror_init(), skip mirrors with unreachable OSTs in the I/O extent during FLR selection for aggregate I/O. When every mirror is skipped, glimpse (ndelay) returns -EAGAIN after backoff so the caller retries, and -EIO once it has tried all mirrors several rounds; lseek and data_version fail with -EIO at once. For designated-mirror I/O, apply the same extent-scoped check after resolving ci_designated_mirror; there is no alternate mirror to try, so return -EIO immediately. Detect unreachable stripes with lov_mirror_extent_unreachable(), limited to stripes intersecting the current I/O extent, via lov_sub_unreachable(). Skip parity mirrors for glimpse and data_version as well, so they never return parity object size or version. Size FLR ndelay rounds by the data mirrors that can serve the I/O, not lo_mirror_count, for I/O types that skip parity; this also excludes stale mirrors from the round of a plain FLR read. CIT_EC_RD skips that rotation. Add sanity-flr 31b. Test-Parameters: fortestonly Assisted-by: Cursor:Grok-4.6/Opus5.5 llm_code_and_review_tools Signed-off-by: Keguang Xu <kxu@ddn.com> Change-Id: Iac1c6415127d46fd958016a998ceab68663e0d42
68600
master
J ✗45B ✓T ✗11R 0/2
LU-20354 quota: serve LQA usage from QMT cache
janitor: 45 test failures unique to this patch — sanity-quota@ldiskfs+DNE:test_1j, sanity-quota@ldiskfs+DNE:test_3a, sanity-quota@ldiskfs+DNE:test_3b +42 more
45 unique failures
11 unresolved
XL +1391 −198
44d
LU-20354 quota: serve LQA usage from QMT cache
janitor: 45 test failures unique to this patch — sanity-quota@ldiskfs+DNE:test_1j, sanity-quota@ldiskfs+DNE:test_3a, sanity-quota@ldiskfs+DNE:test_3b +42 more
45 unique failures
11 unresolved
XL +1391 −198
44d
janitor: 45 test failures unique to this patch — sanity-quota@ldiskfs+DNE:test_1j, sanity-quota@ldiskfs+DNE:test_3a, sanity-quota@ldiskfs+DNE:test_3b +42 more
enforced failed: review-dne-ipv6, review-dne-part-4, review-dne-part-5, review-dne-zfs-part-3, review-dne-zfs-part-4, review-dne-zfs-part-5, review-dne-zfs-part-7, review-zfs
11 unresolved thread(s) await your reply — last from Gerrit AI review for Lustre
janitor flagged test failures unique to this patch — full janitor results
| unique failing test | history |
|---|---|
| sanity-quota@ldiskfs+DNE:test_1j | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@ldiskfs+DNE:test_3a | seen in 1 other review |
| sanity-quota@ldiskfs+DNE:test_3b | seen in 1 other review |
| sanity-quota@ldiskfs+DNE:test_3c | seen in 1 other review |
| sanity-quota@ldiskfs+DNE:test_5 | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@ldiskfs+DNE:test_6 | seen in 1 other review |
| sanity-quota@ldiskfs+DNE:test_7a | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@ldiskfs+DNE:test_7b | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@ldiskfs+DNE:test_7c | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@ldiskfs+DNE:test_17 | seen in 6 other reviews |
| sanity-quota@ldiskfs+DNE:test_25 | seen in 1 other review |
| sanity-quota@ldiskfs+DNE:test_33 | seen in 4 other reviews |
| sanity-quota@ldiskfs+DNE:test_34 | seen in 4 other reviews |
| sanity-quota@ldiskfs+DNE:test_35 | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@ldiskfs+DNE:test_37 | seen in 4 other reviews |
| sanity-quota@ldiskfs+DNE:test_38 | seen in 4 other reviews |
| sanity-quota@ldiskfs+DNE:test_40c | seen in 1 other review |
| sanity-quota@ldiskfs+DNE:test_64 | seen in 1 other review |
| sanity-quota@ldiskfs+DNE:test_67 | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@ldiskfs+DNE:test_71a | seen in 2 other reviews |
| sanity-quota@ldiskfs+DNE:test_71b | seen in 2 other reviews |
| sanity-quota@ldiskfs+DNE:test_72 | seen in 2 other reviews |
| sanity-quota@ldiskfs+DNE:test_73a | seen in 6 other reviews |
| sanity-quota@ldiskfs+DNE:test_75 | seen in 6 other reviews |
| sanity-quota@ldiskfs+DNE:test_80 | seen in 1 other review |
| sanity-quota@zfs:test_1j | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@zfs:test_3a | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@zfs:test_3b | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@zfs:test_3c | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@zfs:test_5 | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@zfs:test_6 | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@zfs:test_7a | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@zfs:test_7b | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@zfs:test_7c | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@zfs:test_17 | seen in 34 other reviews |
| sanity-quota@zfs:test_25 | seen in 27 other reviews |
| sanity-quota@zfs:test_33 | seen in 33 other reviews |
| sanity-quota@zfs:test_34 | seen in 33 other reviews |
| sanity-quota@zfs:test_35 | NEW unique failure for this branch in the last 30 days, and was seen 2 times across 1 other branches 1 reviews |
| sanity-quota@zfs:test_37 | seen in 33 other reviews |
| sanity-quota@zfs:test_38 | seen in 33 other reviews |
| sanity-quota@zfs:test_64 | seen in 1 other review |
| sanity-quota@zfs:test_72 | seen in 14 other reviews |
| sanity-quota@zfs:test_73a | seen in 17 other reviews |
| sanity-quota@zfs:test_75 | seen in 7 other reviews |
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-dne-ipv6 | RHEL 9.8 / x86_64 | ran 4 tests. 1 tests failed: sanity-lnet. | session |
| review-dne-part-4 | RHEL 9.7 / x86_64 | ran 9 tests. 1 tests failed: sanity-quota. | session |
| review-dne-part-4 | RHEL 10.1 / x86_64 | ran 9 tests. 1 tests failed: sanity-quota. | session |
| review-dne-part-5 | RHEL 9.7 / x86_64 | ran 5 tests. 1 tests failed: lustre-rsync-test. | session |
| review-dne-zfs-part-3 | RHEL 9.8 / x86_64 | ran 3 tests. 1 tests failed: conf-sanity. | session |
| review-dne-zfs-part-3 | RHEL 10.1 / x86_64 | ran 3 tests. 1 tests failed: conf-sanity. | session |
| review-dne-zfs-part-4 | RHEL 9.8 / x86_64 | ran 9 tests. 1 tests failed: sanity-quota. | session |
| review-dne-zfs-part-4 | RHEL 10.1 / x86_64 | ran 9 tests. 1 tests failed: sanity-quota. | session |
| review-dne-zfs-part-5 | RHEL 9.7 / x86_64 | ran 5 tests. 1 tests failed: lustre-rsync-test. | session |
| review-dne-zfs-part-7 | RHEL 9.8 / x86_64 | ran 6 tests. 1 tests failed: sanity-ec. | session |
| review-zfs | RHEL 8.10 / x86_64 | ran 8 tests. 1 tests failed: sanity-quota. | session |
build #132033 SUCCESS · tests all Maloo sessions
patchsets:
PS3 09-13
PS4 09-14
PS5 09-16
PS6 09-17
PS7 09-20
+2 earlier
PS1 09-03 PS2 09-11threads: 11 need your reply · 0 waiting on others · 0 own notes · 0 bot
reply Gerrit AI review for Lustre COMMIT_MSG:41 — (suggestion) This adds a new quotactl command and two new request formats on the MDS-to-OSS/MDT path, and changes what `lfs quota --lqa` pri
Gerrit AI review for Lustre
(suggestion) This adds a new quotactl command and two new request formats on the MDS-to-OSS/MDT path, and changes what `lfs quota --lqa` prints. A `Test-Parameters:` line asking for an interop run against a released server would cover the rolling-upgrade case where the slave does not know LUSTRE_Q_GETOUSAGES, e.g.
Test-Parameters: serverversion=2.16 testlist=sanity-quota
reply Gerrit AI review for Lustre COMMIT_MSG:39 — (minor) The body covers the LQA work well, but two hunks outside it are not mentioned.
Gerrit AI review for Lustre
(minor) The body covers the LQA work well, but two hunks outside it are not mentioned. qmt_dqacq0() now warms the cache with `qb_usage != 0` instead of an unconditional `true`. That changes the non-LQA per-ID path: a slave that legitimately reports zero usage no longer marks its slot fresh, so the next GETQUOTA misses and pays a full refresh round instead of hitting the cache. The inline comment gives the motivation (an on-disk 0 racing the first inode), but a reader of the commit message would not expect a per-ID behaviour change in an LQA patch. qmt_usg_test_init() also switches every `return rc` to `return -EINVAL`, which drops the specific failure code from the insmod error. Could both get a line in the body, or should the dqacq0 change be split out since it stands on its own?
reply Gerrit AI review for Lustre qmt.usage_cache_ttl.4:67 — (minor) This paragraph now sits directly under the new LQA text but describes only the per-ID behaviour. For LQA there is no "previously val
Gerrit AI review for Lustre
(minor) This paragraph now sits directly under the new LQA text but describes only the per-ID behaviour. For LQA there is no "previously valid cached value" kept and no granted substitution - qmt_lqa_refresh_lqe_usg() resets every slot to QMT_USG_UNSET and returns the error - and the 120s degraded TTL never applies either, since qmt_apply_usg_fetch() is not on that path. qmt.usage_cache_stats(4) already carries the "LQA has no granted substitution" caveat; the same note would fit here.
reply Gerrit AI review for Lustre lustre_quota.h:372 — (minor) The kdoc opens with "GETOUSAGES is server-internal", but this second test is what decides whether a non-server export gets in.
Gerrit AI review for Lustre
(minor) The kdoc opens with "GETOUSAGES is server-internal", but this second test is what decides whether a non-server export gets in. The connect-flag test above already matches every MDS/OSS export on its own: lod_add_device() requests OBD_CONNECT_MDS for OSP-to-OST and OBD_CONNECT_MDS_MDS for OSP-to-MDT, and both survive connect negotiation (ofd_obd.c reads exp_connect_flags(exp) & OBD_CONNECT_MDS the same way). So the nodemap clause is not needed for the MDS/OSS case the kdoc uses to justify it; what it does add is every ordinary client that happens to sit in a nodemap with trust_client_ids set, which is a common setting for compute nodes and says nothing about the client being privileged. A client in that nodemap can then read bspace/ispace for up to LQUOTA_IDS_MAX arbitrary IDs per RPC from any OST or MDT. Is that intended, or should the check be the connect flags alone?
reply Gerrit AI review for Lustre mdt_handler.c:6324 — (minor) GETOUSAGES is dispatched to every OST/MDT without any capability check. There is no OBD_CONNECT* bit for it, and nothing here looks
Gerrit AI review for Lustre
(minor) GETOUSAGES is dispatched to every OST/MDT without any capability check. There is no OBD_CONNECT* bit for it, and nothing here looks at exp_connect_flags before sending. Against a peer that predates the opcode the reply is an error rather than data: lquotactl_slv() rejects any qc_cmd other than Q_GETOQUOTA/LUSTRE_Q_ITEROQUOTA with -EOPNOTSUPP, and an older mdt_quotactl() falls into its `default:` case with -EFAULT. That becomes items[i].ufi_rc, which aborts qmt_lqa_refresh_lqe_usg() and wipes the cache, so in a mixed-version cluster every GETQUOTALQA pays the full glb-index walk and RPC fan-out and still ends with usage unknown. Should this be gated on a negotiated connect flag so the QMT can skip the refresh entirely when a slave cannot serve it?
reply Gerrit AI review for Lustre osp_dev.c:1549 — (minor) The comment above promises a length check that req_capsule_server_sized_get() does not perform for this field.
Gerrit AI review for Lustre
(minor) The comment above promises a length check that req_capsule_server_sized_get() does not perform for this field.
RMF_OBD_QUOTA_ACCT is RMF_F_STRUCT_ARRAY, and __req_capsule_get() takes that branch:
len = lustre_msg_buflen(msg, offset);
if (len % field->rmf_size) return NULL;
value = getter(msg, offset, len);
so the rc_area size that server_sized_get() just stored is discarded and lustre_msg_buf() is called with min_size equal to the buffer's own length. Any reply whose ACCT buffer is a non-zero multiple of sizeof(struct lquota_acct_rec) comes back non-NULL, including one shorter than qcb_replen, and the memcpy() then reads past it.
In-tree slaves size the reply from the ID list they received, so this needs a peer that answers rc = 0 with a short array. Would an explicit req_capsule_get_size(pill, &RMF_OBD_QUOTA_ACCT, RCL_SERVER) == qcb_replen test be worth keeping here, or at least a comment that matches what sized_get actually guarantees?
reply Gerrit AI review for Lustre qmt_entry.c:1495 — (style) This isn't a bug, but `qpi` is now available one line up, so `qpi->qpi_qmt->qmt_usg_ttl` would avoid evaluating lqe2qpi() twice. Wor
Gerrit AI review for Lustre
(style) This isn't a bug, but `qpi` is now available one line up, so `qpi->qpi_qmt->qmt_usg_ttl` would avoid evaluating lqe2qpi() twice. Worth folding in if the patch is refreshed.
reply Gerrit AI review for Lustre qmt_entry.c:1712 — (defect) A waiter that just slept on lqu_waitq re-enters here and redoes the whole refresh, even when the previous owner already filled ever
Gerrit AI review for Lustre
(defect) A waiter that just slept on lqu_waitq re-enters here and redoes the whole refresh, even when the previous owner already filled every slot. The per-ID path below is protected against this: after `goto again` its gather loop skips slots with `!qmt_usg_expired(...)`, so `nstale == 0` and it breaks out with zero RPCs. qmt_lqa_refresh_lqe_usg() has no equivalent test - it always calls qmt_lqa_collect_ids() (full walk of the glb index) and then re-fetches every slot. So with the default ttl of 3600, two concurrent `lfs quota --lqa` for the same name serialize on lqu_fetching and then each pay a complete index walk plus a GETOUSAGES fan-out, on an MDS service thread. N concurrent callers give N full refreshes. Would a `qmt_sum_lqe_usg(lqe, &tmp, now, true) == 0` check at the top of qmt_lqa_refresh_lqe_usg() (or before re-taking lqu_fetching) be enough to make the waiters cheap?
reply Gerrit AI review for Lustre qmt_handler.c:1297 — (minor) "LQA no-ops" doesn't quite match qmt_update_lqe_usg_entry_with_ttl(): its early return only fires for `usage_valid && !lqa_store`. q
Gerrit AI review for Lustre
(minor) "LQA no-ops" doesn't quite match qmt_update_lqe_usg_entry_with_ttl(): its early return only fires for `usage_valid && !lqa_store`. qmt_lqe_lookup() substitutes lqa_qid so the LQA entry here always has qid_uid == QMT_LQA_QID, and when the usage being reported is zero the call falls through and inserts/keeps a slot on the shared lqe. Harmless, but the comment reads as if nothing happens at all.
reply Gerrit AI review for Lustre qmt_internal.h:479 — (minor) QMT_USG_SNAP_NONE has no users - qmt_apply_usg_fetch() still compares its snap_usage against QMT_USG_UNSET, and this change is the t
Gerrit AI review for Lustre
(minor) QMT_USG_SNAP_NONE has no users - qmt_apply_usg_fetch() still compares its snap_usage against QMT_USG_UNSET, and this change is the tip of the relation chain, so nothing later picks it up. It is also the same value as QMT_USG_UNSET, which makes two names for one sentinel. Drop it on the next refresh?
reply Gerrit AI review for Lustre qmt_lqa.c:1330 — (suggestion) Any single slave error discards every slot, including ones that were still valid from an earlier successful refresh.
Gerrit AI review for Lustre
(suggestion) Any single slave error discards every slot, including ones that were still valid from an earlier successful refresh. An administratively deactivated OST is enough to reach this: lod_tgt_quotactl_async() returns -ENETDOWN for `!ltd_active || ltd_reap`, mdt_qmt_fetch_usgs_batch() propagates it, and the whole refresh unwinds here. From then on `lfs quota --lqa` reports usage as unknown for as long as that target stays down, while plain `lfs quota` for the same IDs keeps working because qmt_apply_usg_fetch() keeps the cached value or substitutes granted. The commit message presents the all-or-nothing abort as deliberate, so this is a design question rather than a claim that it is wrong: is a target the admin took offline on purpose really a reason to stop reporting LQA usage entirely, rather than reusing that slave's last cached value the way the per-ID path does?
owner Keguang Xu · uploader Keguang Xu · PS7 uploaded 44d ago · NEW · open in Gerrit ↗
commit message
LU-20354 quota: serve LQA usage from QMT cache GETQUOTALQA previously returned limits only. Wire it through the usage cache so lfs quota --lqa can set QIF_SPACE/QIF_INODES from the LQA shared lqe (qid=1). LQA has no llite fan-out and no per-slave DQACQ piggyback, so a miss collects glb IDs in the current ranges and batch-fetches each slave with LUSTRE_Q_GETOUSAGES. Per-slave range sums land on the shared lqe. A slave or setup failure aborts the refresh (no granted fallback, no partial sum) and leaves usage unknown; GETQUOTALQA still returns limits. QIF_SPACE follows DT alone and QIF_INODES follows MD alone. Missing QIF_USAGE is unknown in lfs, not -EIO. lfs quota --lqa accepts -U/-G/-P before or after --lqa, and prints "Disk quotas for <type> LQA <name>:". Range add/remove bumps qpi_lqa_gen with the rbtree update. Later GETQUOTALQA sees lqu_lqa_gen != qpi_lqa_gen, treats the slots as stale, and refreshes. The bump stays under the same write_lock as insert/erase so the new tree and new generation become visible together. GETOUSAGES carries a swabbed __u32 ID list (RMF_OBD_QUOTA_IDS) and lquota_acct_rec reply array (RMF_OBD_QUOTA_ACCT), not the opaque ITER blob used by ITEROQUOTA. A second qtype adding the same DOM MDT still increments LQA DT qpi_slv_nr on -EEXIST. LQA TTL follows usage_cache_ttl, including 0 (refetch on every GETQUOTALQA, same as per-ID GETQUOTA). Add MDT/OFD GETOUSAGES handlers, OSP/LOD iobuf async, and mdt_qmt_fetch_usgs_batch. Cover with kunit and sanity-quota/312. Document LQA behavior in the usage_cache_* man pages. Assisted-by: Cursor:Grok-4.6 llm_code_and_review_tools Signed-off-by: Keguang Xu <kxu@ddn.com> Change-Id: I63f53ba1d21f2bb6cc419c3d09b3f9337eaa1f17
68598
master
J ✗17B ✓T ✗8R 0/2
LU-20354 quota: aggregate MD inode usage for GETQUOTA
janitor: 17 test failures unique to this patch — sanity-quota@ldiskfs+DNE:test_16a, sanity-quota@ldiskfs+DNE:test_17, sanity-quota@ldiskfs+DNE:test_33 +14 more
17 unique failures
11 unresolved
XL +813 −225
44d
LU-20354 quota: aggregate MD inode usage for GETQUOTA
janitor: 17 test failures unique to this patch — sanity-quota@ldiskfs+DNE:test_16a, sanity-quota@ldiskfs+DNE:test_17, sanity-quota@ldiskfs+DNE:test_33 +14 more
17 unique failures
11 unresolved
XL +813 −225
44d
janitor: 17 test failures unique to this patch — sanity-quota@ldiskfs+DNE:test_16a, sanity-quota@ldiskfs+DNE:test_17, sanity-quota@ldiskfs+DNE:test_33 +14 more
enforced failed: review-dne-part-4, review-dne-zfs-part-3, review-dne-zfs-part-4, review-dne-zfs-part-7, review-zfs
11 unresolved thread(s) await your reply — last from Gerrit AI review for Lustre
janitor flagged test failures unique to this patch — full janitor results
| unique failing test | history |
|---|---|
| sanity-quota@ldiskfs+DNE:test_16a | seen in 2 other reviews |
| sanity-quota@ldiskfs+DNE:test_17 | seen in 7 other reviews |
| sanity-quota@ldiskfs+DNE:test_33 | seen in 1 other review |
| sanity-quota@ldiskfs+DNE:test_34 | seen in 1 other review |
| sanity-quota@ldiskfs+DNE:test_35 | seen in 1 other review |
| sanity-quota@ldiskfs+DNE:test_37 | seen in 3 other reviews |
| sanity-quota@ldiskfs+DNE:test_40c | seen in 2 other reviews |
| sanity-quota@ldiskfs+DNE:test_64 | seen in 2 other reviews |
| sanity-quota@ldiskfs+DNE:test_78A | seen in 4 other reviews |
| sanity-quota@ldiskfs+DNE:test_78a | seen in 3 other reviews |
| sanity-quota@zfs:test_17 | NEW unique failure for this branch in the last 30 days, and was seen 1 times across 1 other branches 1 reviews |
| sanity-quota@zfs:test_33 | seen in 1 other review |
| sanity-quota@zfs:test_34 | seen in 1 other review |
| sanity-quota@zfs:test_35 | seen in 1 other review |
| sanity-quota@zfs:test_37 | seen in 1 other review |
| sanity-quota@zfs:test_64 | seen in 2 other reviews |
| sanity-quota@zfs:test_75 | seen in 1 other review |
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-dne-part-4 | RHEL 9.7 / x86_64 | ran 9 tests. 1 tests failed: sanity-quota. | session |
| review-dne-part-4 | RHEL 10.1 / x86_64 | ran 9 tests. 1 tests failed: sanity-quota. | session |
| review-dne-zfs-part-3 | RHEL 9.8 / x86_64 | ran 3 tests. 1 tests failed: conf-sanity. | session |
| review-dne-zfs-part-3 | RHEL 10.1 / x86_64 | ran 3 tests. 1 tests failed: conf-sanity. | session |
| review-dne-zfs-part-4 | RHEL 9.8 / x86_64 | ran 9 tests. 1 tests failed: sanity-quota. | session |
| review-dne-zfs-part-4 | RHEL 10.1 / x86_64 | ran 9 tests. 1 tests failed: sanity-quota. | session |
| review-dne-zfs-part-7 | RHEL 9.8 / x86_64 | ran 6 tests. 1 tests failed: sanity-ec. | session |
| review-zfs | RHEL 8.10 / x86_64 | ran 8 tests. 1 tests failed: sanity-quota. | session |
build #132032 SUCCESS · tests all Maloo sessions
patchsets:
PS1 09-03
PS3 09-14
PS4 09-17
PS5 09-20
threads: 11 need your reply · 0 waiting on others · 0 own notes · 0 bot
reply Gerrit AI review for Lustre COMMIT_MSG:40 — (minor) The `nrpc` out-parameter added to `quf_fetch_batch()` (lustre_quota.h, mdt_handler.c, qmt_entry.c) also changes what `usage_cache_st
Gerrit AI review for Lustre
(minor) The `nrpc` out-parameter added to `quf_fetch_batch()` (lustre_quota.h, mdt_handler.c, qmt_entry.c) also changes what `usage_cache_stats fetch_rpc` counts. It used to be `atomic64_add(nstale, ...)` issued before the fetch, so it counted every stale slot, including the local MDT slot that never produces an RPC; it is now only the OSP requests actually queued. That looks like an improvement, but it is a visible change to an existing counter and the body only mentions the degraded/fallback split. Worth a sentence here, since sanity-quota 310/311 now key off the new meaning.
reply Gerrit AI review for Lustre lustre_quota.h:131 — (style) Not a bug, but now that this also carries KEY_MDT_QUOTACTL, the `ost_` / `oqi_` naming reads as OST-only: `mdt_qmt_fetch_usage_batch
Gerrit AI review for Lustre
(style) Not a bug, but now that this also carries KEY_MDT_QUOTACTL, the `ost_` / `oqi_` naming reads as OST-only: `mdt_qmt_fetch_usage_batch()` declares `struct ost_qctl_info oqi` for the remote-MDT case. `lod_ost_quotactl()` got renamed to `lod_tgt_quotactl()` in this patch for exactly that reason; `struct tgt_qctl_info` with `tqi_*` fields would match, if the patch is refreshed anyway.
reply Misc Code Checks Robot (Gatekeeper helper) osp_dev.c:1571 — error: osp_quotactl_prep():'osp' dereferencing possible ERR_PTR()
Misc Code Checks Robot (Gatekeeper helper)
error: osp_quotactl_prep():'osp' dereferencing possible ERR_PTR()
reply Misc Code Checks Robot (Gatekeeper helper) osp_dev.c:1571 — error: osp_quotactl_prep():'osp' dereferencing possible ERR_PTR()
Misc Code Checks Robot (Gatekeeper helper)
error: osp_quotactl_prep():'osp' dereferencing possible ERR_PTR()
reply Misc Code Checks Robot (Gatekeeper helper) osp_dev.c:1571 — error: osp_quotactl_prep():'osp' dereferencing possible ERR_PTR()
Misc Code Checks Robot (Gatekeeper helper)
error: osp_quotactl_prep():'osp' dereferencing possible ERR_PTR()
reply Misc Code Checks Robot (Gatekeeper helper) osp_dev.c:1571 — error: osp_quotactl_prep():'osp' dereferencing possible ERR_PTR()
Misc Code Checks Robot (Gatekeeper helper)
error: osp_quotactl_prep():'osp' dereferencing possible ERR_PTR()
reply Gerrit AI review for Lustre qmt_entry.c:1601 — (minor) Was promoting these from LQUOTA_DEBUG to LQUOTA_WARN intended? D_WARNING is in D_CANTMASK, so both of these now reach the console (r
Gerrit AI review for Lustre
(minor) Was promoting these from LQUOTA_DEBUG to LQUOTA_WARN intended? D_WARNING is in D_CANTMASK, so both of these now reach the console (rate-limited by the cdls, but still console output). A slave that is briefly unavailable is routine - an OST in recovery answers -EAGAIN, an inactive target gives -ENETDOWN out of lod_tgt_quotactl_async() - and with usage_cache_enabled defaulting to 1 this path is now taken on ordinary `lfs quota` calls, where it previously produced nothing on the console. The degraded counter and qmt.usage_cache_stats(4) already cover the same condition for anyone watching for it. If the console message is wanted, it is probably worth a line in the commit message, since it is new output on a healthy filesystem.
reply Gerrit AI review for Lustre qmt_entry.c:1699 — (minor) The comment that this replaces said the rescan from key 0 was needed because "slots may be added while the lock was dropped for fetc
Gerrit AI review for Lustre
(minor) The comment that this replaces said the rescan from key 0 was needed because "slots may be added while the lock was dropped for fetch/apply". Resuming from the last key drops that property: qmt_usg_extend_sites() -> qmt_update_lqe_usg_entry() only takes lqe_usg_data_lock, it does not wait on lqu_fetching the way qmt_del_lqe_usg_entry() does, so a slave connecting mid-refresh can insert a slot whose key sorts below resume (MDT keys are below OST keys, and a pool add can bring in a lower OST index). That slot stays QMT_USG_UNSET, so the following qmt_sum_lqe_usg(..., check_ttl=false) returns -ESTALE and the whole GETQUOTA falls back to client fan-out. It self-heals on the next call, so this is not a correctness problem, but the commit message presents the resume as a pure optimization. Would a final pass that only looks for lue_usage == QMT_USG_UNSET work here? It still terminates, since a fetched slot is no longer UNSET.
reply Gerrit AI review for Lustre qmt_handler.c:104 — (defect) With `usage_cache_enabled` defaulting to 1, this refresh now runs for every quota ID a client asks about, including IDs that have n
Gerrit AI review for Lustre
(defect) With `usage_cache_enabled` defaulting to 1, this refresh now runs for every quota ID a client asks about, including IDs that have no limits at all and have never been seen before.
`check_owner()` in `ll_quotactl()` only owner-checks USRQUOTA and GRPQUOTA; PRJQUOTA falls through, so an unprivileged user on any client can run `lfs quota -p <projid>` for arbitrary project IDs. Each unseen ID then does, on the MDT0 service thread:
qmt_pool_lqe_lookup() -> lqe_locate_find() /* creates + hashes an lqe */
qmt_get_usage(DT) -> qmt_setup_lqe_usg() + this refresh
qmt_get_usage(MD) -> qmt_setup_lqe_usg() + this refresh
so two full Q_GETOQUOTA fan-outs to every OST/MDT plus a per-slave `lqe_usg_data` attached to the new lqe. QMT lqes are only dropped at site teardown, so the cache grows with the number of distinct IDs queried.
The lqe-per-queried-ID part is pre-existing, but until this patch `qmt_usg_serve` was hard-wired to 0 with no tunable, so `qmt_get_usage()` returned -EAGAIN and neither the slave RPCs nor the per-slave allocation happened; the fan-out ran on the client, in the client's own context and in parallel.
Would it work to skip the refresh when `!lqe->lqe_enforced`, leaving usage unset so the client keeps the legacy fan-out for IDs with no limits? That would cover the enumeration case and still serve the IDs that matter. Otherwise, is defaulting the tunable to 0 the safer landing state?
reply Gerrit AI review for Lustre qmt_handler.c:113 — (minor) `qmt_quotactl()` calls `qmt_get_usage()` twice now, once for DT and once for MD, so `hit`, `miss` and `fallback` became per-resource
Gerrit AI review for Lustre
(minor) `qmt_quotactl()` calls `qmt_get_usage()` twice now, once for DT and once for MD, so `hit`, `miss` and `fallback` became per-resource rather than per-GETQUOTA. One request where both aggregates fail bumps `fallback` twice, and a request where only DT fails bumps it even though `QIF_INODES` was served. The `qmt_usg_fallback` comment in qmt_internal.h (`GETQUOTA unserved`) and the `fallback` text in qmt.usage_cache_stats.4 (`GETQUOTA leaves usage unset so the client fans out`) both read as per-request. If the patch is refreshed, either move the counting up into qmt_quotactl() or reword both to say per-resource.
reply Gerrit AI review for Lustre sanity-quota.sh:8105 — (suggestion) The commit message says 310 is extended "for inode serve", but the test only ever reads --space; nothing asserts the value that
Gerrit AI review for Lustre
(suggestion) The commit message says 310 is extended "for inode serve", but the test only ever reads --space; nothing asserts the value that the MD aggregate produces. The QIF_SPACE gate means a passing space check proves rc_md was 0, so it shows MD *succeeded*, but not that dqb_curinodes is right.
311 does assert the inode count, but it is gated on MDSCOUNT >= 2, so a single-MDT run - the common sanity-quota config - never checks the headline behavior of this patch.
A second line next to this one would close that, e.g.
used=$($LFS quota -u $TSTUSR --inodes -q $DIR | tr -d '*')
(( used > 0 )) || quota_error u $TSTUSR "expected non-zero inode usage, got $used"
owner Keguang Xu · uploader Keguang Xu · PS5 uploaded 44d ago · NEW · open in Gerrit ↗
commit message
LU-20354 quota: aggregate MD inode usage for GETQUOTA Turn on the usage cache (qmt.*.usage_cache_enabled, default on) so GETQUOTA sums both DT and MD from the per-slave cache. Set QIF_INODES from the MD aggregate and require both DT and MD before QIF_SPACE so llite still fans out when inode usage is unavailable. A slave fetch failure substitutes granted (or keeps a prior cache) and still serves: granted may overstate usage, but one unreachable target must not force every client to fan out. Clearing the tunable leaves usage unset and clients keep legacy fan-out. Drop the DT-only usage-cache gates so MD-pool lqes get slots: qmt_setup_lqe_usg / qmt_update_lqe_usg_entry / qmt_usg_extend_sites and DQACQ warming now apply to LQUOTA_RES_MD (LQA still skipped). MD-pool slots return dqb_curinodes; DT (incl. DOM) stays stoqb(dqb_curspace). Fetch remote MDTs via MDT-OSP MDS_QUOTACTL (KEY_MDT_QUOTACTL). MDS/OSS NIDs belong in a trusted nodemap, so CLIENT_TO_FS map is identity and no skip-idmap handshake is needed. usage_cache_ttl=0 means no cache: each GETQUOTA misses and refetches. Unfilled slots use lue_usage=QMT_USG_UNSET; expire=0 is zero freshness (same as ttl=0) so a queued waiter walks again instead of skipping as still fresh. Resume xa_for_each_start across GETOQUOTA batches of QMT_USG_FETCH_PARALLEL 32 (was 16) instead of rescanning from key 0. A cold-cache GETQUOTA on a large filesystem runs the slave fan-out synchronously on MDT0 (DT then MD), serialized per id. That can be slower than legacy client-side parallel fan-out until the cache is warm. Split usage_cache_stats so degraded counts per-slave granted substitution (still served) and fallback counts an unusable aggregate (client fans out). Add kunit coverage for MD-pool sum and ttl-0 miss vs post-refresh sum. Extend sanity-quota/310 for inode serve, no OST fan-out while enabled, and legacy fan-out when disabled. Add 311 for multi-MDT remote fetch (ttl=0; wait for MDT1 get_dqblk before the OSP refresh). Document usage_cache_enabled, usage_cache_ttl, and usage_cache_stats. Assisted-by: Cursor:Grok-4.6 llm_code_and_review_tools Signed-off-by: Keguang Xu <kxu@ddn.com> Change-Id: Ic3864474ec2bf5619ccaf2a0c22c190c0c34a25a
67810
master
J !B ✓T ✗5R 0/2
LU-20354 quota: parallelize usage-cache GETOQUOTA
enforced failed: review-dne-ipv6, review-dne-part-5, review-dne-zfs-part-4, review-dne-zfs-part-7
1 unresolved
L +439 −172
50d
LU-20354 quota: parallelize usage-cache GETOQUOTA
enforced failed: review-dne-ipv6, review-dne-part-5, review-dne-zfs-part-4, review-dne-zfs-part-7
1 unresolved
L +439 −172
50d
enforced failed: review-dne-ipv6, review-dne-part-5, review-dne-zfs-part-4, review-dne-zfs-part-7
1 unresolved thread(s) await your reply — last from Gerrit AI review for Lustre
janitor run: 7 failing config(s), none unique to this patch — janitor results
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-dne-ipv6 | RHEL 9.7 / x86_64 | ran 4 tests. 1 tests failed: sanity. | session |
| review-dne-part-5 | RHEL 9.7 / x86_64 | ran 5 tests. 1 tests failed: lustre-rsync-test. | session |
| review-dne-zfs-part-4 | RHEL 9.8 / x86_64 | ran 9 tests. 1 tests failed: sanity-quota. | session |
| review-dne-zfs-part-7 | RHEL 9.8 / x86_64 | ran 6 tests. 1 tests failed: sanity-ec. | session |
| review-dne-zfs-part-7 | RHEL 9.7 / x86_64 | ran 6 tests. 1 tests failed: sanity-ec. | session |
build #131507 SUCCESS · tests all Maloo sessions
patchsets:
PS1 08-07
PS5 09-02
PS6 09-08
PS7 09-10
threads: 1 need your reply · 0 waiting on others · 0 own notes · 0 bot
reply Gerrit AI review for Lustre lod_dev.c:2468 — (style) This isn't a bug, but the function uses GOTO() while having no ENTRY and returning with a plain return. On a CDEBUG_ENTRY_EXIT build
Gerrit AI review for Lustre
(style) This isn't a bug, but the function uses GOTO() while having no ENTRY and returning with a plain return. On a CDEBUG_ENTRY_EXIT build that logs "Process leaving via out_put" with no matching entry, and the success path logs nothing at all. lod_ost_quotactl() right above uses ENTRY/RETURN - worth matching it if the patch is refreshed.
owner Keguang Xu · uploader Keguang Xu · PS7 uploaded 54d ago · NEW · open in Gerrit ↗
commit message
LU-20354 quota: parallelize usage-cache GETOQUOTA Refresh stale per-slave usage in parallel batches via quf_fetch_batch. MDT queues remote OST Q_GETOQUOTA on a ptlrpc_set through LOD/OSP o_quotactl_async. Local MDT (DOM) stays a synchronous lquotactl_slv in the same batch; remote MDT remains -EOPNOTSUPP. On fetch failure keep the pre-fetch snap_usage when the slot was previously populated, otherwise fall back to granted. Re-lookup the slot by key after the unlocked fetch; shrink waits on lqu_fetching so the entry is still present. This cuts cold-miss / TTL-expire latency on large OST counts without changing GETQUOTA fallback semantics for OST slaves. Assisted-by: Cursor:Grok-4.6 llm_code_and_review_tools Signed-off-by: Keguang Xu <kxu@ddn.com> Change-Id: I700aec594310b9ae2e1cbab0fa460fee15b7f3d3
▶Review feedback — your turn
(3)
68361
master
J –B ✓T …R +1/2
LU-20690 ec: store EC parity like RAID10
13 unresolved thread(s) await your reply — last from Gerrit AI review for Lustre
ec2
218_review_prioritypt_ecroraid10-like
13 unresolved
XL +2329 −745
1h
LU-20690 ec: store EC parity like RAID10
13 unresolved thread(s) await your reply — last from Gerrit AI review for Lustre
ec2
218_review_prioritypt_ecroraid10-like
13 unresolved
XL +2329 −745
1h
13 unresolved thread(s) await your reply — last from Gerrit AI review for Lustre
build OK — testing in progress (typically 6–14h)
reviews:
Ronnie Sahlberg +1
no vote yet: Marc Vef, Andreas Dilger
patchsets:
PS20 10-30 Ronnie Sahlberg
PS21 11-01
PS22 11-03 Andreas Dilger
PS23 11-03 Andreas Dilger
PS24 11-03 Andreas Dilger
+11 earlier
PS1 08-26 PS7 09-03 PS8 09-07 PS9 09-10 PS10 09-11 PS11 09-23 PS12 10-08 PS13 10-08 PS17 10-23 PS18 10-29 PS19 10-29 Ronnie Sahlbergthreads: 13 need your reply · 0 waiting on others · 0 own notes · 0 bot
reply Gerrit AI review for Lustre COMMIT_MSG:174 — (minor) The patch also fixes the raid set mapping in degraded reads and resync, which counted rows and raid sets from e_start instead of fil
Gerrit AI review for Lustre
(minor) The patch also fixes the raid set mapping in degraded reads and resync, which counted rows and raid sets from e_start instead of file offset 0 (the wrong-parity rebuild described above). That counting came from landed commits, so a Fixes: line would help with tracking, even though EC has not shipped:
Fixes: 783c13bb0b ("LU-12669 ec: recover data from parity")
The resync side of the same problem dates from 03191be6f4 ("LU-12668 ec: add support to resync stale ec comps").
reply Keguang Xu patchset comment — With the changing of layout from `PARITY|RAID0` TO RAID10-like `PARITY`, we not only store the layout header differently to disk, but also w
Andreas Dilger
Should the removal of the LOV_PATTERN_RAID0 from EC layouts be moved into an earlier patch? That affects the on-disk layout of all the files and should be included into the 2.18.0 release, even if the 1+2 layout is not allowed initially. If this patch doesn't make it then we could force 1+2 to be 2+2 at create time? Not ideal, but probably easier to get landed.
Keguang Xu
With the changing of layout from `PARITY|RAID0` TO RAID10-like `PARITY`, we not only store the layout header differently to disk, but also we need to adjust our access path, say, in the parity component, stripes belong to the same raidset are arranged as RAID1 (logicall, they share the same file offset), stripes from different raidsets are stored as RAID0. To read/write data from/to parity, we need to designate stripe first. We can move the layout changes into an earlier patch. However, for EC to function properly, all of these patches must be landed together as a single set.
reply Gerrit AI review for Lustre lfs-mirror-read.1:30 — (minor) llapi_layout_parity_pos_to_io() starts each component's dump at the slot of the raid set that holds e_start, not at raid set 0. Take
Gerrit AI review for Lustre
(minor) llapi_layout_parity_pos_to_io() starts each component's dump at the slot of the raid set that holds e_start, not at raid set 0. Take -E 4M -c 1 -E eof -S 1M -c 8 --ec 4+1: the second component has O = 3M, e_start is in raid set 1, and its dump begins with the parity object of raid set 1, followed by raid set 0 of the next row. The order matches getstripe only when e_start is a multiple of the stripe width. lfs-getstripe.1 has the same issue ("dumps this RAID0 of all parity stripes"). If the patch is refreshed, could both pages say that the first row starts at the raid set of e_start?
reply Ronnie Sahlberg cl_object.h:2022 — not part of this change any more. And don't want to touch 69517 where this now lives as that one has passed testing.
Andreas Dilger
(minor) "stripe" is not a good name for this, since it is potentially conflated with the data stripe number if we want to do single-stripe read/write for replacement. Maybe "_parity_index" or similar?
Ronnie Sahlberg
not part of this change any more. And don't want to touch 69517 where this now lives as that one has passed testing.
reply Misc Code Checks Robot (Gatekeeper helper) lod_internal.h:751 — warn: lod_comp_shrink_to_extent():always true condition '(lod_comp->llc_stripe_count <= 65535) => (0-u16max <= u16max)'
Misc Code Checks Robot (Gatekeeper helper)
warn: lod_comp_shrink_to_extent():always true condition '(lod_comp->llc_stripe_count <= 65535) => (0-u16max <= u16max)'
reply Misc Code Checks Robot (Gatekeeper helper) lod_internal.h:751 — warn: lod_comp_shrink_to_extent():always true condition '(lod_comp->llc_stripe_count <= 65535) => (0-u16max <= u16max)'
Misc Code Checks Robot (Gatekeeper helper)
warn: lod_comp_shrink_to_extent():always true condition '(lod_comp->llc_stripe_count <= 65535) => (0-u16max <= u16max)'
reply Misc Code Checks Robot (Gatekeeper helper) lod_internal.h:751 — warn: lod_comp_shrink_to_extent():always true condition '(lod_comp->llc_stripe_count <= 65535) => (0-u16max <= u16max)'
Misc Code Checks Robot (Gatekeeper helper)
warn: lod_comp_shrink_to_extent():always true condition '(lod_comp->llc_stripe_count <= 65535) => (0-u16max <= u16max)'
reply Misc Code Checks Robot (Gatekeeper helper) lod_internal.h:751 — warn: lod_comp_shrink_to_extent():always true condition '(lod_comp->llc_stripe_count <= 65535) => (0-u16max <= u16max)'
Misc Code Checks Robot (Gatekeeper helper)
warn: lod_comp_shrink_to_extent():always true condition '(lod_comp->llc_stripe_count <= 65535) => (0-u16max <= u16max)'
reply Misc Code Checks Robot (Gatekeeper helper) lod_internal.h:751 — warn: lod_comp_shrink_to_extent():always true condition '(lod_comp->llc_stripe_count <= 65535) => (0-u16max <= u16max)'
Misc Code Checks Robot (Gatekeeper helper)
warn: lod_comp_shrink_to_extent():always true condition '(lod_comp->llc_stripe_count <= 65535) => (0-u16max <= u16max)'
reply Andreas Dilger lov_internal.h:160 — With RAID0|PARITY gone, lsme_parity_slots_per_raidset() is always 1 for parity and only multiplies poff at two places in lov_io.c. The six
Andreas Dilger
With RAID0|PARITY gone, lsme_parity_slots_per_raidset() is always 1 for parity and only multiplies poff at two places in lov_io.c. The six geometry helpers (lsme_stripe_origin, lsme_stripes_per_slot, lsme_parity_single_slot, lsme_slot_count, lsme_stripe_slot, lsme_parity_slots_per_raidset) reduce to one rule: for parity, origin = e_start, width = (stripe_count / p) * stripe_size, slot = stripe / p; otherwise RAID0. Two helpers would do, and the single-slot case can become an invalid entry. About 25 lines.
reply Ronnie Sahlberg lfs.c:4629 — Silent fallback is addressed. But not the quoting as raid0,overstriped should already be treated by yaml as a single string so quoting shoul
Andreas Dilger
This only matches the exact string "parity", but "lfs getstripe -y" prints "parity,overstriped" for a -C EC component, so "lfs setstripe --yaml" of that output silently falls back to the default pattern. When the pattern has multiple flags, please quote it in the YAML output (pattern: "parity,overstriped") so it is a single scalar, and parse the comma-separated flags here instead of comparing whole strings, which would also cover "raid0,overstriped" and any later flag combinations.
Ronnie Sahlberg
Silent fallback is addressed. But not the quoting as raid0,overstriped should already be treated by yaml as a single string so quoting should not be needed here.
reply Gerrit AI review for Lustre lfs.c:4553 — (suggestion) This isn't a bug, but the ignored[] list repeats the names in lov_pattern_names[] in liblustreapi_layout.c, and llapi_lov_strin
Gerrit AI review for Lustre
(suggestion) This isn't a bug, but the ignored[] list repeats the names in lov_pattern_names[] in liblustreapi_layout.c, and llapi_lov_string_pattern() already parses the comma-separated getstripe form. The copy will drift when a new pattern name is added. If the patch is refreshed, consider calling llapi_lov_string_pattern() and mapping LOV_PATTERN_MDT and LOV_PATTERN_OVERSTRIPING to the LLAPI_LAYOUT_* values. The "unknown_" prefix would still need handling here.
tags: 218_review_priority pt_ecro raid10-like
owner Keguang Xu · uploader Andreas Dilger · PS24 uploaded 1h ago · NEW · open in Gerrit ↗
commit message
LU-20690 ec: store EC parity like RAID10
The k >= p restriction is not a Cauchy/ISA-L limit. It comes from
storing parity as RAID0|PARITY: the p parity objects of a raidset are
concatenated along the file offset, so raidset_count * p * stripe_size
does not fit in a finite PFL extent when p > k (for example
--ec 2+3 -E 2M). This patch changes the parity layout; the p <= k
checks in lfs and llapi stay until a following patch removes them.
Store EC parity as LOV_PATTERN_PARITY instead. A raidset is an EC
split of the stripe set, and the layout is like RAID10 in that it has
the same two axes:
- across the raidsets of one stripe set: like RAID0, one
stripe_size per raidset;
- within one raidset: like RAID1, the p parity stripes share one
file offset (the same object_off).
The RAID1 axis comes from recovery. A raidset of k data and p parity
stripes survives the loss of any p of its k + p stripes, just as a
RAID1 of p + 1 copies survives the loss of any p copies. RAID1 is the
k == 1 case, where each parity is a copy of the data (--ec 1+N). The
p parities protect the same data, so they share its offset: the
offset says which data is protected, the parity index says which
redundancy unit it is.
What differs from RAID1 is I/O, not the role. For k > 1 the parities
are not replicas: each holds a different parity, and a lost stripe is
rebuilt from any k survivors rather than copied from one. The parity
uses the extent of the data component.
RAID0 places data rows from file offset 0, whereas counting EC rows
and raidsets from e_start groups the wrong k stripes when e_start is
not a multiple of the data stripe width: the stripes EC groups as one
raidset are not the objects lod paired with that raidset's parity
OSTs, and a degraded read rebuilds a lost stripe from the wrong
parity. For example -E 4M -c 1 -E eof -S 1M -c 8 --ec 4+1 puts offset
4M on object 4 (raidset 1), which counting from e_start encodes into
the parity of raidset 0. When e_start is not a multiple of
stripe_size, as after a 64K DoM component, counting stripe_size
parity slots from e_start also splits stripes at
e_start + n * stripe_size and overflows e_end.
Count rows and raidsets from file offset 0, as RAID0 maps the data
objects:
row = off / (stripe_count * stripe_size)
ec_off = O
+ (row - row of e_start) * raidset_count * stripe_size
+ raidset * stripe_size + off % stripe_size
Data before e_start is taken as zero, and so is its parity, which is
not stored: the origin O moves back from e_start over the raidsets of
the first row that end before e_start, and over the part of the
raidset of e_start below e_start % stripe_size when e_start is in its
last data stripe. slot % raidset_count stays the raidset, so the
parity OSTs lod pairs are unchanged, and every stored parity byte has
data in the component. Common DoM and PFL layouts with an unaligned
e_start are supported and keep their parity within [e_start, e_end),
such as:
- -E 64K -L mdt -E 1M -c 1 -S 1M -E eof -c 4 -S 4M --ec 4+2;
- -E 64K -L mdt -E 16M -c 2 -S 1M -E eof -c 4 -S 4M --ec 2+2;
- -E 64K -L mdt -E eof -c 4 -S 1M --ec 2+2;
- -E 1M -c 1 -S 1M -E 64M -c 1 -S 4M -E eof -c 4 -S 4M --ec 4+2.
When every raidset has one data stripe (--ec 1+N, or one data
stripe), there is one parity slot per data stripe and nothing to
pack, so rows and O count from file offset 0, as RAID0 does: ec_off
is the data offset, and each parity object is a copy of its data
object at the same object offsets, so it can stand in for it.
Otherwise O depends only on e_start, the data stripe count, k and the
stripe size, not on e_end, and when e_start is a multiple of the
stripe width, O is e_start.
Add ec_raidset_count(), ec_raidset_first(), ec_stripe_to_raidset()
and ec_off_to_raidset_offset() next to ec_split_stripes() in
lustre_user.h, and struct ec_parity_geom with ec_parity_geom_init(),
ec_data_to_parity() and its inverse ec_parity_to_data(), with
ec_parity_width() for the parity bytes of one stripe set, and use
them in lov, lod and llapi instead of open-coding the k0 * n0 split.
ec_parity_geom_init() also records the data row of parity slot 0 in
epg_row0 (0 for 1-data raidsets).
lov caches O in lsme_parity_origin when the layout is unpacked and
returns it from lsme_stripe_origin(), and lov_parity_geom_init()
fails a degraded read whose pair does not match in stripe size,
raidsets or origin; the parity mirror EOF of such a pair is also
bounded by one parity row per stripe set from e_start. Raidset
objects before e_start read as zero in degraded reads and resync.
A read is widened to whole raid sets, but not past the e_end of its
component, so a read ending there does not start an empty recovery
cycle for the next component's first raid set, which may begin before
its e_start.
lsme_parity_raid10() now just returns lsme_is_parity(), as every
parity component is LOV_PATTERN_PARITY, and
lsme_parity_slots_per_raidset() is dropped.
A component shorter than one stripe (e_end < e_start + stripe_size)
whose raidset continues past e_end (e.g. [64K, 1M) with -S 1M -c 2
--ec 2+2, or [1M, 4M) with -S 4M -c 2 --ec 2+2, where the second
stripe is never reached) still has no room for a full stripe_size
parity slot from e_start. ec_parity_fits() detects it:
llapi_layout_sanity() refuses it with LSE_EC_PARITY_FIT,
llapi_ec_check_comp_match() refuses to resync or verify it, and
lod_comp_prep_create() checks it again with the final data stripe
count, also for lfs mirror extend --ec.
The lov slot geometry, the STRIPE I/O scope and the per-stripe llapi
helpers were added by the previous patches and only take effect for
LOV_PATTERN_PARITY. This patch switches the encoding so they apply:
llapi_pattern_to_lov() and llapi_layout_to_lum() emit
LOV_PATTERN_PARITY, lov_pattern_supported() accepts it in place of
RAID0|PARITY (and lov_pattern_available() via that), and lod allocates
and verifies parity components with it. EC has not shipped, so the old
combination is not kept. Finite-extent stripe-count shrinking, now in
one lod_comp_shrink_to_extent() helper instead of three copies, is
skipped for parity components. lsm_unpackmd_comp_md_v1() and
lod_verify_striping() reject a component whose LCME_FL_PARITY flag and
LOV_PATTERN_PARITY disagree, since the parity geometry follows the
flag.
vvp_prep_size() no longer writes KMS into i_size for parity I/O.
ci_parity_eof is derived from i_size, and KMS is only a lower bound
of the data size, so a parity read within KMS shrank ci_parity_eof
for later parity I/O.
Userspace resync and verify move to the RAID10 offsets: one
stripe_size per raidset, each parity addressed by its stripe index,
leaving the part of the first parity slot below e_start alone. The
punch, zero-fill and hole verify of a parity slot are bounded by the
e_end of its component; no slot runs past e_end while
ec_parity_fits() holds, so this is only a defensive bound. Hole
punch-or-clear and hole verify loop over the p parity stripes that
share an offset when they fall back to zero-fill or zero-content
reads.
lfs setstripe --yaml parses the pattern as the comma-separated flags
lfs getstripe -y prints, so "parity,overstriped" keeps overstriping.
lfs mirror read of a parity mirror still dumps every parity object in
RAID0 order, through mirror_dump_parity() in lfs, from e_start. The
dump of a component stops at its e_end: when the extent is not a
multiple of the data stripe set, the raid sets of the last row past
e_end hold the data of the next component. The glimpse lock of such
a read spans every component, so lov_lock_sub_init() no longer
rejects a glimpse because a component the I/O does not cover has
fewer parity stripes than the designated one; lov_io_iter_init()
checks the components the I/O covers.
Example: -E 128M -c 11 -S 1M --ec 4+2
data/parity extent [0, 128M)
raidsets [4,2], [4,2], [3,2] (raidset_count=3)
parity has 6 OST objects
one data stripe set = 11M
one parity stripe set = 3M (not 1M flat, not 6M concat)
stripeset raidset data_pos ec_off concat objects
--------- ------- ----------- ------- ------- -------
0 0 [0, 4M) 0 0 0,1
0 1 [4M, 8M) 1M 2M 2,3
0 2 [8M, 11M) 2M 4M 4,5
1 0 [11M, 15M) 3M 6M 0,1
1 1 [15M, 19M) 4M 8M 2,3
1 2 [19M, 22M) 5M 10M 4,5
The concat column is what RAID0|PARITY would have used, and is what
overflows a finite extent when p > k.
Tests: sanity-ec 12c/12d/34c cover
holes and multi-raidset offsets, 30d and 31b-31e check the parity dump
size, 34d dumps a mixed-p mirror whose first extent is not a multiple
of the stripe set, 63d recovers reads from parity of raidset 1 in a
PFL component that does not start at 0, 301 covers an e_start that
stripe_size does not divide and checks that a component whose parity
would pass e_end is refused for that reason, 302 resyncs, verifies and
reads degraded common PFL layouts with an unaligned e_start, 303
recovers reads when e_start is not a multiple of the stripe width,
with equal and unequal raidsets, 304 covers zero-fill before e_start,
305 truncates at raidset and unaligned component boundaries, and 306
reads degraded across a component boundary where the raid set of the
first component runs into the next one.
llapi_layout_test asserts the component pattern is LLAPI_LAYOUT_PARITY
and 68 checks the parity placement over a range of geometries and
extents, and that 1-data raidset copies are at the data object
offsets.
Test-Parameters: testlist=sanity-ec ostcount=8
Test-Parameters: testlist=llapi_layout_test
Test-Parameters: testlist=sanity-ec ostcount=8 fstype=zfs
Signed-off-by: Keguang Xu <kxu@ddn.com>
Signed-off-by: Ronnie Sahlberg <rsahlberg@whamcloud.com>
Signed-off-by: Andreas Dilger <adilger@thelustrecollective.com>
Assisted-by: Cursor:Composer/Opus5.5 llm_code_and_review_tools
Assisted-by: ClaudeCode:claude-opus-5-5 ltvm llm_code_and_review_tools
Assisted-by: ClaudeCode:claude-fable-5-1 llm_code_and_review_tools
Change-Id: I9574df44969ddd40776f028b3c326d150ea08eb5
67700
master
J –B ✓T ✓R 0/2
LU-20354 quota: cache per-slave DT usage on QMT
carrying Keguang Xu · 10 unresolved thread(s) await your reply — last from Gerrit AI review for Lustre
11 unresolved
L +754 −11
5d
LU-20354 quota: cache per-slave DT usage on QMT
carrying Keguang Xu · 10 unresolved thread(s) await your reply — last from Gerrit AI review for Lustre
11 unresolved
L +754 −11
5d
10 unresolved thread(s) await your reply — last from Gerrit AI review for Lustre
CI green — no vote yet from Sergey Cheremencev, Andreas Dilger, Hongchao Zhang
build #133862 SUCCESS · tests all Maloo sessions
no vote yet: Sergey Cheremencev, Andreas Dilger, Hongchao Zhang
patchsets:
PS13 09-02
PS14 09-29
PS15 10-05
PS16 10-12
PS17 10-15 kg.xu
+12 earlier
PS1 08-03 PS2 08-04 PS3 08-05 PS4 08-06 PS5 08-10 PS6 08-18 PS7 08-24 PS8 08-24 PS9 08-26 PS10 08-28 PS11 08-29 PS12 08-30threads: 10 need your reply · 0 waiting on others · 1 own notes · 0 bot
reply Gerrit AI review for Lustre COMMIT_MSG:10 — (minor) The subject and this sentence scope the cache to DT / block usage, but nothing in the code restricts it to LQUOTA_RES_DT.
Gerrit AI review for Lustre
(minor) The subject and this sentence scope the cache to DT / block usage, but nothing in the code restricts it to LQUOTA_RES_DT. qmt_pool_new_conn() is called once per global index FID, so qmt_usg_extend_sites() also runs on the LQUOTA_RES_MD global pool, and the warming loop in qmt_dqacq0() runs for whatever qti_lqes(env) holds - which is the MD pool lqe on an inode DQACQ, with qb_usage being an inode count. So MD lqes get an lqe_usg_data and per-MDT slots too. Is that intended (in which case the message could say so), or should the warming and the extend be gated on LQUOTA_RES_DT until the inode side is ready?
reply Andreas Dilger COMMIT_MSG:10 — ... block and inode usage ..., I hope? It should be possible to cache the MDT usage as well, in case it is temporarily offline.
Andreas Dilger
... block and inode usage ..., I hope? It should be possible to cache the MDT usage as well, in case it is temporarily offline.
note kg.xu patchset comment — Updated
kg.xu
Hi, some tips on this change, which might be helpful in review,
Focus: Build QMT per-slave DT usage cache only. qmt_usg_serve=0 — GETQUOTA still leaves usage unset (legacy client fan-out).
Read order: qmt_internal.h → qmt_pool.c (DOM/dom_mdts) → qmt_entry.c (update/sum/refresh) → qmt_dqacq0 warm → mdt_qmt_fetch_usage.
Call chains:
```
# warm (live)
QSD DQACQ/REL (+QUOTA_DQACQ_FL_USAGE, qb_usage)
→ qmt_dqacq → qmt_dqacq0 → qmt_update_lqe_usg()
# pool membership → slots
connect/add → qmt_usg_extend_cb | qmt_pool_dom_add → empty slot
shrink → qmt_usg_shrink_cb → qmt_del_lqe_usg()
# refresh (wired, gated off)
qmt_quotactl(GETQUOTA) → qmt_get_usage()
→ (!qmt_usg_serve) -EAGAIN
→ [later] sum → refresh → mdt_qmt_fetch_usage
→ local MDT: lquotactl_slv | OST: OSP Q_GETOQUOTA
→ fail: keep cache | fallback granted
```
kg.xu
Updated
Focus: Build QMT per-slave DT usage cache only.
Read order:
qmt_internal.h / lquota_internal.h (lqu_xa) →
qmt_pool.c (DOM / extend / shrink) →
qmt_entry.c (update / del / sum) →
qmt_dqacq0 warm → qsd_handler.c (QUOTA_DQACQ_FL_USAGE).
Call chains:
```
QSD DQACQ/REL (+QUOTA_DQACQ_FL_USAGE, qb_usage)
→ qmt_dqacq → qmt_dqacq0 → qmt_update_lqe_usg_entry
(also on REPORT; skip shared LQA lqe)
# pool membership → slots
OST/MDT connect/add
→ qmt_usg_extend_cb → empty OST (or MD primary) slot
DOM MDT connect (global DT only)
→ qmt_pool_dom_add → qmt_usg_dom_extend_cb → empty MDT slot
shrink (snapshot then delete; not under cfs_hash_for_each)
→ qmt_usg_shrink_hash → qmt_del_lqe_usg_entry()
(waits lqu_fetching for follow-on refresh)
```
reply Keguang Xu patchset comment — Ping @hongchao@whamcloud.com and @scherementsev@ddn.com for reviews, thanks.
Andreas Dilger
ping @hongchao@whamcloud.com and @scherementsev@ddn.com for reviews on this series, it is useful for the EC project.
Keguang Xu
Ping @hongchao@whamcloud.com and @scherementsev@ddn.com for reviews, thanks.
reply Andreas Dilger wiretest.c:4945 — (style) please use `unsigned int` even though the script currently generates `unsigned` so that we have fewer places to fix in the future.
Andreas Dilger
(style) please use `unsigned int` even though the script currently generates `unsigned` so that we have fewer places to fix in the future.
reply Andreas Dilger lquota_internal.h:182 — seems reasonable to fix if patch is refreshed, or a later patch in this series..
Gerrit AI review for Lustre
(suggestion) This isn't a bug, but the comment says the field is QMT-only, yet it sits outside the `u.me`/`u.se` union. That adds 16 bytes to every `lquota_entry`, including all the QSD entries on every OST and MDT, which never use it. Could it go into `struct lquota_mst_entry` as `lme_usg_xa`, with a `#define lqe_usg_xa u.me.lme_usg_xa` like `lqe_sem`? On a non-debug build `lquota_mst_entry` is smaller than `lquota_slv_entry`, so the union should not grow. `lqe_ref_free()` would then call `qmt_lqe_usg_fini()` only when `lqe_is_master(lqe)` is true, rather than relying on QSD entries having a zeroed xarray.
Andreas Dilger
seems reasonable to fix if patch is refreshed, or a later patch in this series..
reply Andreas Dilger qmt_dev.c:58 — (minor) other parameters like this are named "maxage"
Andreas Dilger
(minor) other parameters like this are named "maxage"
reply Andreas Dilger qmt_dev.c:83 — I don't see the man4 page for this?
wc-checkpatch
(style) 'LPROC_SEQ_FOPS' may be misspelled - perhaps 'LUSTRE_RW_ATTR and create Documentation/man4/ page for param'?
Andreas Dilger
I don't see the man4 page for this?
reply Gerrit AI review for Lustre qmt_handler.c:1195 — (style) This isn't a bug, but qmt_update_lqe_usg_entry() opens with the identical `qpi_lqa || qpi_rtype != LQUOTA_RES_DT` test and returns 0
Gerrit AI review for Lustre
(style) This isn't a bug, but qmt_update_lqe_usg_entry() opens with the identical `qpi_lqa || qpi_rtype != LQUOTA_RES_DT` test and returns 0, so the filter here is duplicated. Dropping one of the two would keep the rule in a single place.
reply Andreas Dilger qmt_internal.h:394 — Presumably this is cached so long only if the target is offline? Otherwise it should be maximum a few seconds.
Andreas Dilger
Presumably this is cached so long only if the target is offline? Otherwise it should be maximum a few seconds.
reply Andreas Dilger sanity-quota.sh:8310 — Adding tests at the end of the file is conflict-intensive. If doing so, add a larger gap like test_350() so there is more room for future g
Andreas Dilger
Adding tests at the end of the file is conflict-intensive. If doing so, add a larger gap like test_350() so there is more room for future growth without conflicts (search for open patches modifying sanity-quota.sh to check). Otherwise, use a gap between 98 and 300, like 200.
owner Keguang Xu · uploader kg.xu · PS17 uploaded 19d ago · NEW · open in Gerrit ↗
commit message
LU-20354 quota: cache per-slave DT usage on QMT Build an in-memory per-slave DT usage cache on the global pool lqe so QMT can track block usage reported by slaves. Named pools do not keep their own slots; qmt_sum_lqe_usg() sums the global entries for the pool's current sarr members. LQA and MD are not cached. Refresh from MDT/OSP and serving the sum on GETQUOTA stay in a follow-on. DQACQ and REL piggyback on-disk usage in qb_usage with QUOTA_DQACQ_FL_USAGE, set by qsd_qbody_set_usage(). REPORT already carries that usage and is left unchanged. qmt_dqacq0() drops the lqe write lock, then qmt_update_lqe_usg_entry() stores the value on the global lqe. A missing slot is created there. The slot stores the update time; a ttl of 0 always misses. The sum holds op_rw_sem (read) across each member array. Slot fields are written with WRITE_ONCE and read with READ_ONCE, with no second lock. Callers must not hold lqe_sem: the recalc thread takes op_rw_sem and then lqe_sem, so the other order deadlocks with a queued pool writer. A missing or expired member, or a pool with no members, returns -ESTALE. An all-zero fresh sum is a valid idle ID. DOM MDTs are recorded only in the global DT pool tgts[MDT] and stay out of lgd and recalc. A slave connect adds the target once; disconnect does not remove it. qmt.*.usage_cache_ttl is in seconds (default 3600, max one day). Zero makes every lookup miss. The cache is freed with the lqe. qmt_usg_test covers update and sum by pool sarr, a named pool sharing the global cache, and TTL. sanity-quota/305 loads it on the MDS. It does not exercise MDT/OSP refresh. Assisted-by: Cursor:Grok-4.7/Opus5.5 llm_code_and_review_tools Signed-off-by: Keguang Xu <kxu@ddn.com> Change-Id: I9acd2b938c45ba9c62d5a2c03ba86af24ddbe90d
68073
master
J –B ✓T ✓R 0/2
LU-20354 quota: refresh QMT usage cache via MDT/OSP proxy
3 unresolved thread(s) await your reply — last from Gerrit AI review for Lustre
3 unresolved
L +759 −66
16d
LU-20354 quota: refresh QMT usage cache via MDT/OSP proxy
3 unresolved thread(s) await your reply — last from Gerrit AI review for Lustre
3 unresolved
L +759 −66
16d
3 unresolved thread(s) await your reply — last from Gerrit AI review for Lustre
CI green — no vote yet from Sergey Cheremencev, Andreas Dilger, Hongchao Zhang
build #133812 SUCCESS · tests all Maloo sessions
no vote yet: Sergey Cheremencev, Andreas Dilger, Hongchao Zhang
patchsets:
PS2 09-02
PS3 09-04
PS5 09-10
PS6 10-14
PS7 10-16
+1 earlier
PS1 08-18threads: 3 need your reply · 0 waiting on others · 0 own notes · 0 bot
reply Gerrit AI review for Lustre mdt_handler.c:6122 — (typo) Missing space before `*/`.
Gerrit AI review for Lustre
(typo) Missing space before `*/`.
reply Gerrit AI review for Lustre qmt_entry.c:1344 — (minor) The extra members don't really wait for a later refresh. After qmt_refresh_lqe_usg() returns 0, qmt_get_usage() re-sums with check_t
Gerrit AI review for Lustre
(minor) The extra members don't really wait for a later refresh. After qmt_refresh_lqe_usg() returns 0, qmt_get_usage() re-sums with check_ttl=false over the live sarr, not over the copied idxs. So a member added during the walk is summed at whatever age its slot has. Example: `lctl pool_add` adds OST N to a named pool while the serial Q_GETOQUOTA walk is running. OST N already has a slot on the global lqe from an old DQACQ (slots are only freed with the lqe), but that slot expired long ago. Refresh never touches N, and the following sum serves N's old value as part of a fresh aggregate. A member with no slot gets -ESTALE, which is fine. This only matters once qmt_usg_serve is enabled, and the result is one stale reply, not corruption. But either the comment or the post-refresh sum should change, e.g. sum only the idxs that refresh handled, or redo the refresh if op_count changed.
reply Gerrit AI review for Lustre qmt_handler.c:776 — (style) This isn't a bug, but the comment sits directly above an assignment that runs for every command in this case group, while the text o
Gerrit AI review for Lustre
(style) This isn't a bug, but the comment sits directly above an assignment that runs for every command in this case group, while the text only describes the two commands handled by the condition below it. The old comment explained the assignment itself. Moving this line down onto the `if` would read better.
owner Keguang Xu · uploader Keguang Xu · PS7 uploaded 18d ago · NEW · open in Gerrit ↗
commit message
LU-20354 quota: refresh QMT usage cache via MDT/OSP proxy Wire qmt_refresh_lqe_usg() to pull stale or missing per-slave usage through MDT: local OSD for the DOM MDT, OSP Q_GETOQUOTA for OSTs. The cache stays on the global DT lqe. The caller passes that lqe and the pool; refresh walks the pool sarr (alloc outside the lock, copy under it). Fresh slots, including DQACQ-warmed zeros, are left alone. Remote MDT DOM fetch is still TODO. On fetch failure, keep a cached value when the slot exists, otherwise fall back to granted from that slave's index. Degraded values are back-dated to expire after about 60s so the next refresh retries soon. qmt_sum_lqe_usg() takes check_ttl: true for a cache lookup, false after refresh so freshly stored slots are served even when ttl is 0. qc_id is already a filesystem ID. MDS/OSS NIDs belong in a trusted nodemap, so OFD's CLIENT_TO_FS map is identity and no skip-idmap handshake is needed. qmt_get_usage() sums and refreshes only when qmt_usg_serve is on. The flag stays off, so GETQUOTA still leaves usage unset. qmt_usg_fallback counts a granted fallback or a failed serve. Expose qmt.*.usage_cache_stats (hit/miss/fetch_rpc/fallback). Add sanity-quota/310, skipped until qmt_usg_serve is on. Extend qmt_usg_test for ttl 0 with check_ttl true/false. Test-Parameters: trivial testlist=sanity-quota Assisted-by: Cursor:Grok-4.6 llm_code_and_review_tools Signed-off-by: Keguang Xu <kxu@ddn.com> Change-Id: Ied2f4f1e3c7ee6611a69a57e8064d46a59ba845f
▶In CI
(0)
nothing here right now
▶Needs reviewers
(1)
69074
master
J ✗47B ✓T ✓R 0/2
LU-20354 tests: extra QMT usage-cache coverage
CI green, no reviews yet — add reviewers
M +218 −0
44d
LU-20354 tests: extra QMT usage-cache coverage
CI green, no reviews yet — add reviewers
M +218 −0
44d
janitor flagged test failures unique to this patch — superseded (tests have since passed) — full janitor results
| unique failing test | history |
|---|---|
| sanity-quota@ldiskfs+DNE:test_1j | seen in 1 other review |
| sanity-quota@ldiskfs+DNE:test_3a | seen in 2 other reviews |
| sanity-quota@ldiskfs+DNE:test_3b | seen in 2 other reviews |
| sanity-quota@ldiskfs+DNE:test_3c | seen in 2 other reviews |
| sanity-quota@ldiskfs+DNE:test_5 | seen in 1 other review |
| sanity-quota@ldiskfs+DNE:test_6 | seen in 2 other reviews |
| sanity-quota@ldiskfs+DNE:test_7a | seen in 1 other review |
| sanity-quota@ldiskfs+DNE:test_7b | seen in 1 other review |
| sanity-quota@ldiskfs+DNE:test_7c | seen in 1 other review |
| sanity-quota@ldiskfs+DNE:test_17 | seen in 7 other reviews |
| sanity-quota@ldiskfs+DNE:test_25 | seen in 2 other reviews |
| sanity-quota@ldiskfs+DNE:test_33 | seen in 5 other reviews |
| sanity-quota@ldiskfs+DNE:test_34 | seen in 5 other reviews |
| sanity-quota@ldiskfs+DNE:test_35 | seen in 1 other review |
| sanity-quota@ldiskfs+DNE:test_37 | seen in 5 other reviews |
| sanity-quota@ldiskfs+DNE:test_38 | seen in 5 other reviews |
| sanity-quota@ldiskfs+DNE:test_40c | seen in 2 other reviews |
| sanity-quota@ldiskfs+DNE:test_64 | seen in 2 other reviews |
| sanity-quota@ldiskfs+DNE:test_67 | seen in 1 other review |
| sanity-quota@ldiskfs+DNE:test_71a | seen in 3 other reviews |
| sanity-quota@ldiskfs+DNE:test_71b | seen in 3 other reviews |
| sanity-quota@ldiskfs+DNE:test_72 | seen in 3 other reviews |
| sanity-quota@ldiskfs+DNE:test_73a | seen in 7 other reviews |
| sanity-quota@ldiskfs+DNE:test_75 | seen in 7 other reviews |
| sanity-quota@ldiskfs+DNE:test_78A | NEW unique failure for this branch in the last 30 days, and was seen 0 times across 0 other branches 0 reviews |
| sanity-quota@ldiskfs+DNE:test_80 | seen in 2 other reviews |
| sanity-quota@zfs:test_1j | seen in 1 other review |
| sanity-quota@zfs:test_3a | seen in 1 other review |
| sanity-quota@zfs:test_3b | seen in 1 other review |
| sanity-quota@zfs:test_3c | seen in 1 other review |
| sanity-quota@zfs:test_5 | seen in 1 other review |
| sanity-quota@zfs:test_6 | seen in 1 other review |
| sanity-quota@zfs:test_7a | seen in 1 other review |
| sanity-quota@zfs:test_7b | seen in 1 other review |
| sanity-quota@zfs:test_7c | seen in 1 other review |
| sanity-quota@zfs:test_16a | seen in 2 other reviews |
| sanity-quota@zfs:test_17 | seen in 35 other reviews |
| sanity-quota@zfs:test_25 | seen in 28 other reviews |
| sanity-quota@zfs:test_33 | seen in 34 other reviews |
| sanity-quota@zfs:test_34 | seen in 34 other reviews |
| sanity-quota@zfs:test_35 | seen in 1 other review |
| sanity-quota@zfs:test_37 | seen in 34 other reviews |
| sanity-quota@zfs:test_38 | seen in 34 other reviews |
| sanity-quota@zfs:test_64 | seen in 2 other reviews |
| sanity-quota@zfs:test_72 | seen in 15 other reviews |
| sanity-quota@zfs:test_73a | seen in 18 other reviews |
| sanity-quota@zfs:test_75 | seen in 8 other reviews |
patchsets:
PS1 09-20
PS2 09-20
owner Keguang Xu · uploader Keguang Xu · PS2 uploaded 44d ago · NEW · open in Gerrit ↗
commit message
LU-20354 tests: extra QMT usage-cache coverage Add sanity-quota 313-316 on top of the LQA GETOUSAGES path in 312. 313 covers per-ID TTL hit/expire/ttl=0. 314 reconciles QMT aggregates with per-slave GETOQUOTA. 315 checks granted fallback versus LQA abort when an OSP is down. 316 invalidates LQA cache after range add/remove. Test-Parameters: trivial Assisted-by: Cursor:Grok-4.6 llm_code_and_review_tools Signed-off-by: Keguang Xu <kxu@ddn.com> Change-Id: I36ced70e821e49d7376c21705122c8e9ba6ffbad
▶Waiting on reviewers
(0)
nothing here right now
▶Ready to land
(4)
66882
master
J –B ✓T ✓R +2/2
LU-20211 mdt: trunc(0) layout replace on inactive stripes
cc Keguang Xu · all green — ready to land (not in master-next yet)
pt_ecro218_review_priority
3 unresolved
XL +1320 −64
17h
LU-20211 mdt: trunc(0) layout replace on inactive stripes
cc Keguang Xu · all green — ready to land (not in master-next yet)
pt_ecro218_review_priority
3 unresolved
XL +1320 −64
17h
all green — ready to land (not in master-next yet)
3 reviewer comment(s) to address if a respin is needed — last from Ronnie Sahlberg
build #135174 SUCCESS · tests all Maloo sessions
reviews:
Andreas Dilger +1 · Zhenyu Xu +1
no vote yet: Ronnie Sahlberg, Qian Yingjin, Marc Vef
patchsets:
PS41 10-28
PS42 10-30
PS43 11-02 Andreas Dilger
PS44 11-02 Andreas Dilger
PS45 11-02 Andreas Dilger
+29 earlier
PS1 06-26 PS13 07-27 PS14 07-28 PS15 07-31 PS16 08-05 PS17 08-06 PS18 08-07 PS19 08-12 PS20 08-20 PS21 08-25 PS22 08-25 PS23 09-02 PS24 09-08 PS25 09-08 PS26 09-09 PS27 09-11 PS28 09-17 PS29 10-08 Andreas Dilger PS30 10-08 Andreas Dilger PS31 10-08 Andreas Dilger PS32 10-12 PS33 10-12 PS34 10-13 Andreas Dilger PS35 10-15 Andreas Dilger PS36 10-15 Andreas Dilger PS37 10-16 Andreas Dilger PS38 10-19 Andreas Dilger PS39 10-22 Andreas Dilger PS40 10-26 Andreas Dilgerthreads: 3 need your reply · 0 waiting on others · 0 own notes · 0 bot
reply Ronnie Sahlberg cl_object.h:226 — Or change CL_FSYNC_LOCAL = 0 and a BUILD_BUG_ON(CL_FSYNC_LOCAL != 0) would make it safe if future code blindly clears the conf where t
Ronnie Sahlberg
The risk is for future code. A new layout-change caller that just zeroes conf would get CL_FSYNC_NONE without anyone noticing. Prune would start writeback but not wait for it, and the old layout would be torn down while dirty pages were still being written to its objects. The likely result is lost or misdirected writes, or crashes and assertion failures during cleanup. The code compiles fine and nothing warns, so this would be hard to trace back to the missing field.
Ronnie Sahlberg
Or change CL_FSYNC_LOCAL = 0 and a BUILD_BUG_ON(CL_FSYNC_LOCAL != 0) would make it safe if future code blindly clears the conf where this enum is embedded.
reply Gerrit AI review for Lustre mdd_object.c:4453 — (style) This isn't a bug, but the kernel-doc for this non-static function does not describe @env, @obj or @mlc, and has no Return: section.
Gerrit AI review for Lustre
(style) This isn't a bug, but the kernel-doc for this non-static function does not describe @env, @obj or @mlc, and has no Return: section. `kernel-doc -v` warns about all three parameters. If the patch is refreshed, consider adding them.
reply Gerrit AI review for Lustre mdt_reint.c:955 — (defect) Can the victim's new layout land on the same OST that lod_ost_offline() just reported as offline?
Gerrit AI review for Lustre
(defect) Can the victim's new layout land on the same OST that lod_ost_offline() just reported as offline?
The template keeps the source stripe count as an assigned count, so lod_stripe_count_min() requires all of it. A stopped OST is not excluded by the allocator. osp_statfs() returns -EAGAIN for a DISCON import, lod_statfs_and_check() keeps it active, and the last RR pass accepts it:
lod_ost_alloc_qos() -> -EAGAIN (too few good OSTs)
lod_ost_alloc_rr(): speed == 2, connecting -> rc = 0
osp_precreate_reserve(): opd_pre_status -ENODEV, objects still in the pool -> success
The reserve hands out a precreated object without contacting the OST. Take 4 OSTs, a file created with `-c -1` and OST0 stopped. trunc(0) instantiates 4 stripes with one on OST0, swaps, bumps the generation and replies MS_LAYOUT_REPLACED. The client's punch of the new object then blocks on OST0, the same hang this patch is meant to avoid. The swap and the new objects are wasted as well. The comment in sanity-flr test_50e ("RR last-resort may place on the DISCONN OST") suggests this was seen in testing.
Would it work to run mo_layout_check(MD_LAYOUT_UNAVAILABLE) on the victim after mo_layout_change() and skip the swap if it still reports an unavailable stripe? setattr would then fall back to the normal trunc(0), as the commit message describes for allocation failures.
tags: pt_ecro 218_review_priority
owner Keguang Xu · uploader Andreas Dilger · PS45 uploaded 1d ago · NEW · open in Gerrit ↗
commit message
LU-20211 mdt: trunc(0) layout replace on inactive stripes When truncate(0) hits a file whose layout references deactivated or disconnected OSTs, and the client advertises OBD_CONNECT2_LAYOUT_REPLACE, replace the layout: move the old layout onto a volatile victim and instantiate a fresh layout on the file from the existing pattern via QoS/RR. A full or read-only OST is still usable and does not trigger this. If the new layout cannot be allocated, keep the old layout and do a normal trunc(0). Skip it for a lease holder (MDS_TRUNC_KEEP_LEASE): lfs mirror resync and copy truncate only one mirror, and the in-sync mirror must stay. An OST is offline if it was deactivated or its OSP import is not connected, which is checked directly since LOD only learns of the disconnection at its next statfs refresh. After the layout swap, do not update the parent FID of the volatile victim's objects, which are destroyed on close, nor of any object on an offline OST. That update reads the object's filter_fid first, and the RPC would wait for an unreachable OST while LAYOUT EX is held. An OST that reconnects between the declare and execute passes is also skipped at execute, since no OSP sub-transaction was declared for it. Skip replace when any component has LCME_FL_NOSYNC. That flag freezes the mirror's existing objects; swap would discard the snapshot. The NOSYNC test runs once, inside mdt_replace_unavailable_layout(), which returns 0 so setattr falls through to a normal trunc(0). mdt_reint_setattr() does not check NOSYNC. It probes for an unavailable layout before mdt_attr_set() and without a DLM lock, so a healthy trunc(0) skips LAYOUT EX. If the probe finds one, mdt_attr_set() takes UPDATE|LAYOUT|XATTR EX in a single enqueue and replaces the layout under it. A second lock on the file while the saved UPDATE lock is held could queue behind a setxattr waiting for both, and deadlock (LU-3601). The unavailable-stripe result is re-checked under the lock, so a concurrent trunc(0) that already replaced the layout does not allocate a second set of objects. Extend the layout API with MD_LAYOUT_UNAVAILABLE (detect stripes on offline OSTs) and MD_LAYOUT_INSTANTIATE (allocate from a source pattern). Add lod_generate_lovea_template() to emit uninstantiated LOVEA templates that omit placement hints so new stripes use QoS/RR, and trigger replacement from mdt_reint_setattr() via mdt_replace_unavailable_layout(). Layout-type handling for templates / instantiate: - DoM: never carries OST objects; force stripe_count to 0 and skip the OST object array so LOVEA size matches INIT DoM. - PFL/DoM: keep the DoM component and replace only OST components. - SEL: coerce LOV_MAGIC_SEL to COMP_V1 for lod_qos_parse_config(); keep LCME_FL_EXTENSION and restamp SEL when LOVEA is written. - EC: re-emit bound data/parity as unbound (IS_LINK_ID + shared link id) so lod_qos_prep_create() can pair before component IDs exist. Layout replace creates a volatile victim via mdt_orphan_open(). Set mlc_no_inherit on MD_LAYOUT_INSTANTIATE so the new layout does not inherit the local ROOT default pool from the orphan parent. Zero mti_spec first defensively before the volatile create; mdt_thread_info_reset() already clears it per request, and setattr does not leave create state there. After a successful swap, sync the object before replying so the new layout is durable; replay skips replace. On success, return MS_LAYOUT_REPLACED (with OBD_MD_FLEASIZE) in the setattr reply so the client applies the new LOVEA. Before applying it, the client discards its dirty pages past the returned size (all of them for trunc(0)), so prune does not write them back to unavailable OSTs. This lets a later server replace only the OST objects past a non-zero truncate size. The client reserves the default layout size in the setattr reply buffer of any truncate. A resent trunc(0) setattr (lost or truncated reply) returns the current layout with MS_LAYOUT_REPLACED, whether or not the original request replaced it; the client ignores a layout whose generation is unchanged. Add sanity/test_27xt (plain, PFL/DoM, SEL), sanity-ec/test_74b (EC), and sanity-flr/test_50e (mirrors, resync), which stop the OST. Add sanity/test_27xv, which holds the replace with OBD_FAIL_MDS_LAYOUT_REPLACE_PAUSE while a setxattr from a second mount waits for the file, with sync_lock_cancel=never so the saved UPDATE lock stays PW, to check for the deadlock. Test-Parameters: testlist=sanity serverversion=2.17 env=SANITY_EXCEPT="24D 27D" Test-Parameters: testlist=sanity-flr ostcount=5 serverversion=2.17 env=SANITY_FLR_EXCEPT="0a 34b" Test-Parameters: testlist=sanity clientversion=2.17 Assisted-by: Cursor:Grok-4.6 llm_code_and_review_tools Signed-off-by: Keguang Xu <kxu@ddn.com> Signed-off-by: Andreas Dilger <adilger@thelustrecollective.com> Assisted-by: ClaudeCode:claude-opus-5.5 ltvm,llm_code_and_review_tools Assisted-by: Cursor:Composer Change-Id: I07e0792a51e144d935cece4b028e17f659e4ccfd
69517
master
J –B ✓T ✓R +2/2
LU-20690 ec: implement STRIPE I/O scope
all green — ready to land (not in master-next yet)
218_review_blockerraid10-like218_review_prioritypt_ecro
3 unresolved
L +455 −99
1d
LU-20690 ec: implement STRIPE I/O scope
all green — ready to land (not in master-next yet)
218_review_blockerraid10-like218_review_prioritypt_ecro
3 unresolved
L +455 −99
1d
all green — ready to land (not in master-next yet)
3 reviewer comment(s) to address if a respin is needed — last from Marc Vef
build #134549 SUCCESS · tests all Maloo sessions
reviews:
Ronnie Sahlberg +1 · Andreas Dilger +1
no vote yet: Marc Vef, Zhenyu Xu
patchsets:
PS5 10-17
PS6 10-22 Marc Vef
PS7 10-23
PS8 10-27
PS9 10-27
+4 earlier
PS1 10-08 PS2 10-10 PS3 10-13 PS4 10-14threads: 3 need your reply · 0 waiting on others · 0 own notes · 0 bot
reply Marc Vef patchset comment — I think there is a slight misunderstanding. The diagram puts each raidset's parity at the data offset of the raidset's first stripe. So, (0,
Ronnie Sahlberg
I attach an ASCII diagram of the layout, I this should go into ec.txt : === -c 8 -S ss --ec 4x2 Stripe size is ss 8 stripes per Stripe Set 2 Raid Sets, each 4 data stripes. 2 Parities, each with 2 stripes The SCOPE_STRIPE objects are conceptually sparse. Parity data is aligned to the same offset as the first data stripe in the raidset they protect. +-----------+----------+-----------------+-----------------+ | OFFSET | DATA | PARITY P0 | PARITY P1 | | | (1 obj | ci_designated_ | ci_designated_ | | | each) | stripe = 0 | stripe = 1 | +-----------+----------+-----------------+-----------------+ | 0 | d0 | phys stripe 0 | phys stripe 1 | \ \ | ss | d1 | | | | R 0 | | 2*ss | d2 | | | | | | 3*ss | d3 | | | / | +-----------+----------+-----------------+-----------------+ | S 0 | 4*ss | d4 | phys stripe 2 | phys stripe 3 | \ | | 5*ss | d5 | | | | R 1 | | 6*ss | d6 | | | | | | 7*ss | d7 | | | / / +-----------+----------+-----------------+-----------------+ | 8*ss | d0 | phys stripe 0 | phys stripe 1 | \ \ | 9*ss | d1 | | | | R 0 | | 10*ss | d2 | | | | | S 1 | 11*ss | d3 | | | / | +-----------+----------+-----------------+-----------------+ ... === I like that you align the parity stripe offset into the SCOPE_STRIPE P* "mirror" to the same offset as the first data stripe of the raidset it protects. That makes it trivial to map between a raidset offset and where the corresponding parity data is laid out. But it does however also mean that if an application wants to read all the parity data it would have to be aware of the ec geometry so that it could compute the stride between the individual parity stripes. You can't just "open, SCOPE_STRIPE(P0) then read all the data from offset 0 to eof and you have all the parity data for P0. Should it perhaps in the future also plug into faking SEEK_DATA/SEEK_HOLE for this so [0, ss[ would be DATA and [ss,4*ss[ would be HOLE ? I think that would be very useful for applications that would like to use this API as it would somewhat reduce the need to be ec-geometry-aware when reading from SCOPE_STRIPE. But that could be added much later.
Marc Vef
I think there is a slight misunderstanding. The diagram puts each raidset's parity at the data offset of the raidset's first stripe. So, (0, 4·ss, 8·ss) with holes.
But, the code packs them one stripe size apart (0, 1·ss, 2·ss). Keguang described this in more detail in LU-20690 ec: like-RAID10 parity layout | https://review.whamcloud.com/c/fs/lustre-release/+/68361 where he updated ec.txt. And Section 3 (of ec.txt) describes this. This matches what `lov_offset.c` does in 69516 (previous patch).
from ec.txt in 68361:
```
parity-offset = stripeset * raidset-count * stripe-size
+ raid-set * stripe-size
parity-object = raid-set * number-of-parities
```
This means a reader with a fixed index gets all of that parity back to back without holes
reply Andreas Dilger lov_io.c:1235 — What does "Acknowledged" mean here? That you will fix this in a later iteration, a later patch, comment is incorrect, other?
Gerrit AI review for Lustre
(minor) Carried over from patchset 8, where it was marked Done: the stripe index is still validated separately here, in lov_io_lru_reserve() and in lov_lock_sub_init(), and lov_page.c checks for a negative stripe. lov_io_mirror_init() still has no check. The checks also still disagree: this one runs only after uninstantiated components are skipped, while lov_lock_sub_init() checks every component in the range. One check in the ci_designated_mirror branch of lov_io_mirror_init(), once the mirror is resolved, would remove the per-caller copies.
Keguang Xu
Acknowledged
Andreas Dilger
What does "Acknowledged" mean here? That you will fix this in a later iteration, a later patch, comment is incorrect, other?
reply Gerrit AI review for Lustre lov_io.c:1319 — (style) Carried over from patchset 8, where it was Acknowledged: this isn't a bug, but this branch runs only for truncate. A truncate is nev
Gerrit AI review for Lustre
(style) Carried over from patchset 8, where it was Acknowledged: this isn't a bug, but this branch runs only for truncate. A truncate is never CIT_READ/CIT_WRITE, so lov_foreach_io_stripe() always visits every stripe here. If the patch is refreshed, the plain `for (stripe = 0; stripe < r0->lo_nr; stripe++)` loop would keep the churn, and conflicts with the EC read patches, smaller.
tags: 218_review_blocker raid10-like 218_review_priority pt_ecro
owner Keguang Xu · uploader Keguang Xu · PS9 uploaded 7d ago · NEW · open in Gerrit ↗
commit message
LU-20690 ec: implement STRIPE I/O scope In the RAID10-like EC parity layout the p stripes of a raidset share one file offset but hold different data, so read and write must name one of them. Implement LL_IO_SCOPE_STRIPE for that. ll_io_scope_validate() accepts STRIPE with ios_mirror_id in [1, MIRROR_ID_MAX] and ios_stripe_idx below LOV_EC_MAX_CODING_STRIPES. ios_stripe_idx must be 0 for FILE and MIRROR. It is not checked against p here: the scope outlives the layout and the I/O type is not known yet. ll_io_scope_init() handles STRIPE like MIRROR and, for STRIPE, copies ios_stripe_idx into cl_io::ci_designated_stripe and sets ci_designated_stripe_set. OBJECT stays reserved. ci_designated_stripe is an unsigned index. 0 is a valid stripe, so ci_designated_stripe_set says whether it is in use. A zeroed cl_io is unset. The index is meaningful only with ci_designated_mirror set. lov visits only the stripes an I/O touches through lov_foreach_io_stripe(), and maps an offset to a stripe through lov_io_stripe_number(). On a parity component, read and write use only the designated stripe of each raidset and fail with -EINVAL if it is unset or not less than p. Punch, truncate, fallocate and lseek reach every stripe and ignore the index, so a leftover STRIPE scope does not fail them. These helpers target the bare LOV_PATTERN_PARITY encoding for stripe picking. lov_page_init_composite() also refuses CPT_CACHEABLE pages when ci_designated_mirror is set on any parity component (lsm_entry_is_parity()), including RAID0|PARITY. That covers MIRROR and STRIPE scopes and LL_IOC_FLR_SET_MIRROR: a designated parity read that falls back to the page cache (fcntl clears O_DIRECT, or a short O_DIRECT read finished through the cache) returns -EINVAL. sanity-ec test_312 checks that case. llapi_io_scope_set(3) and the uapi comment document STRIPE, and that ios_layout_version 0 uses the fd version from the last lease release or resync lease. LL_IOC_SET_IO_SCOPE reuses nr 177 with LL_IOC_FLR_SET_MIRROR (different size) to save an ioctl number. sanity-flr test_213a checks the ioctl limits for FILE, MIRROR and STRIPE, that OBJECT stays reserved, that mirror resync still goes through the SET_MIRROR alias, and that a STRIPE read of a non-parity mirror ignores the index. sanity-ec test_311 checks that lseek, truncate and fallocate succeed for any index, and skips below 6 OSTs. io_scope gains -b, -x, -r, -t, -f and -l for that. Test-Parameters: testlist=sanity-flr Test-Parameters: testlist=sanity-ec ostcount=8 Assisted-by: Cursor:Composer-2 llm_code_and_review_tools Signed-off-by: Keguang Xu <kxu@ddn.com> Change-Id: Ia8c9e729df4f8651ac4f9dad88d4aef8b056100d
69575
master
J –B ✓T ✓R +2/2
LU-20690 utils: add EC parity-stripe I/O helpers
all green — ready to land (not in master-next yet)
218_review_priorityraid10-like218_review_blockerpt_ecro
1 unresolved
L +244 −115
1d
LU-20690 utils: add EC parity-stripe I/O helpers
all green — ready to land (not in master-next yet)
218_review_priorityraid10-like218_review_blockerpt_ecro
1 unresolved
L +244 −115
1d
all green — ready to land (not in master-next yet)
1 reviewer comment(s) to address if a respin is needed — last from Gerrit AI review for Lustre
build #134756 SUCCESS · tests all Maloo sessions
reviews:
Ronnie Sahlberg +1 · Andreas Dilger +1
no vote yet: Marc Vef
patchsets:
PS4 10-23
PS5 10-23
PS6 10-23
PS7 10-29 Ronnie Sahlberg
PS8 10-29 Ronnie Sahlberg
+3 earlier
PS1 10-11 PS2 10-13 PS3 10-14threads: 1 need your reply · 0 waiting on others · 0 own notes · 0 bot
reply Gerrit AI review for Lustre lfs.c:17527 — (suggestion) 'lfs mirror write' now refuses a parity mirror, but 'lfs mirror copy' still writes into one. 'lfs mirror copy -i1 -o<parity_id>
Gerrit AI review for Lustre
(suggestion) 'lfs mirror write' now refuses a parity mirror, but 'lfs mirror copy' still writes into one. 'lfs mirror copy -i1 -o<parity_id>' does, and so does the documented '-o -1' form, because get_other_mirror_ids() collects every mirror including parity. The path is llapi_mirror_copy_many() -> llapi_mirror_write(), which overwrites the parity with raw data. LL_LEASE_RESYNC_DONE then marks that mirror in sync, so it is the same corruption the new write check prevents. This predates the patch. verify_mirror_id_by_fd() now returns is_parity, though, so this call site could reject an explicit parity target, and the -1 case could skip parity mirrors. If the patch is refreshed, or in a follow-on, consider doing that.
tags: 218_review_priority raid10-like 218_review_blocker pt_ecro
owner Keguang Xu · uploader Ronnie Sahlberg · PS8 uploaded 5d ago · NEW · open in Gerrit ↗
commit message
LU-20690 utils: add EC parity-stripe I/O helpers Prepare userspace for the RAID10-like EC parity layout, where the p parity stripes of a raidset share one file offset and must be addressed by (pos, parity index) through LL_IO_SCOPE_STRIPE. Add internal llapi_mirror_read_parity() and llapi_mirror_write_parity() (lustreapi_internal.h, not lustreapi.h), which pin the I/O to one parity stripe with LL_IO_SCOPE_STRIPE. They share the pread/pwrite loop of llapi_mirror_read() and llapi_mirror_write(). llapi_ec_write_parities() and llapi_ec_verify_parities() take the parity component instead of stripe size and id, and do parity I/O through the _parity helpers. Offsets are unchanged: parity i is still at ec_pos + i * stripe_size. With the current RAID0|PARITY encoding lov ignores the designated stripe, so resync and verify behave as before. The zero-fill fallback of parity punch, the hole check of verify, and 'lfs mirror read' and 'lfs mirror copy' of a parity mirror still use mirror I/O; they need the same change before the RAID10 layout is enabled. Add llapi_layout_is_parity_mirror() to the public API and use it in lfs in place of its own is_parity_mirror() and mirror id lookup. 'lfs mirror write' now refuses a parity mirror and points to 'lfs mirror resync'. Test-Parameters: testlist=sanity-ec ostcount=8 Signed-off-by: Keguang Xu <kxu@ddn.com> Signed-off-by: Ronnie Sahlberg <rsahlberg@whamcloud.com> Assisted-by: Cursor:Grok-4.6/Opus5.5 llm_code_and_review_tools Assisted-by: ClaudeCode:claude-opus-5-5 Change-Id: I58eae79087e99eb6a5751919a9da17970074c215
69762
master
J –B ✓T ✓R +2/2
LU-20287 lov: don't block setattr on unavailable OST
all green — queued for landing (master-next)
pt_ecro218_review_priority
master-next
1 unresolved
L +319 −25
1d
LU-20287 lov: don't block setattr on unavailable OST
all green — queued for landing (master-next)
pt_ecro218_review_priority
master-next
1 unresolved
L +319 −25
1d
all green — queued for landing (master-next)
1 reviewer comment(s) to address if a respin is needed — last from Andreas Dilger
build #134595 SUCCESS · tests all Maloo sessions
reviews:
Ronnie Sahlberg +1 · Andreas Dilger +1
no vote yet: Marc Vef
patchsets:
PS3 10-22
PS4 10-25
PS5 10-27
PS6 10-27 Marc Vef
PS7 10-28
+2 earlier
PS1 10-17 PS2 10-18threads: 1 need your reply · 0 waiting on others · 0 own notes · 0 bot
reply Andreas Dilger cl_object.h:2567 — seems reasonable if patch is refreshed
Gerrit AI review for Lustre
(minor) The new mtime and the new atime are both compared with one shared baseline, max(ctime, sa_cur_time). sa_cur_time is the max of the old mtime and atime, the cached OST mtime and atime, and the inode mtime and atime that vvp_attr_get() reads at iter_init. ll_md_setattr() has already written the new values into the inode by then. ll_merge_attr() merges each field on its own (atime with atime, mtime with mtime), so a newer value in the *other* field can never win the max() for this one.
With the shared baseline:
touch -m -d '+1 hour' f # mtime ahead, no lock (new >= ctime)
touch -a f # atime = now < base (mtime +1h)
The second command now counts as backward. vvp_io_setattr_lock() takes a CLM_WRITE [0, EOF] lock, which revokes other clients' extent locks even when every OST is up. If an OST is down, the command also blocks. Before this patch it took no lock. touch -m behaves the same way when the atime is ahead. So does utimensat() with atime < mtime when both are later than ctime, because the new mtime is already in the inode.
Would it be safer to keep separate mtime and atime baselines (sa_cur_mtime / sa_cur_atime), so that each new time is compared only with its own field? The comments on sa_cur_time, in cl_setattr_ost() and in vvp_io_setattr_iter_init() would then also match what is actually collected.
Andreas Dilger
seems reasonable if patch is refreshed
tags: pt_ecro 218_review_priority
owner Keguang Xu · uploader Keguang Xu · PS7 uploaded 6d ago · NEW · open in Gerrit ↗
commit message
LU-20287 lov: don't block setattr on unavailable OST chown/chgrp/chmod/touch wait in cl_setattr_ost() when a stripe OST is down: VFS sets ATTR_CTIME and the client sends OST_SETATTR to every stripe. That RPC only carries times/flags; UID/GID/PROJID are authoritative on the MDT and are pushed by MDS osp_sync (persistent llog, retried until the OST returns). Skipping the client RPC therefore does not leave stale ownership on OST objects. For non-truncate/non-fallocate setattr on any layout, mark LOV sub-IOs ci_ndelay so OSC sends OST_SETATTR with rq_no_delay and rq_no_resend (plus a 5s delay limit for idle import reconnect), and ignore -EAGAIN/-ENOTCONN/-ESHUTDOWN/-ETIMEDOUT for such sub-IOs in osc_io_setattr_end(). Ndelay applies only when the new mtime/atime are >= max(ctime, sa_cur_time), where sa_cur_time is the known prior inode/cached OST times: ll_merge_attr() takes max(MDT, OST), so skipping an OST that already holds a newer time would leave that value visible. Missed updates are not repaired later, and on a slow but connected OST the client update may also be dropped. The CLM_WRITE [0, EOF] lock in vvp_io_setattr_lock() uses that same baseline, not ctime alone. Plain touch/utime(NULL) therefore takes the lock when sa_cur_time is ahead of the client clock (writer clock skew, or a future timestamp) and revokes other clients' cached extent locks; that path does not set ndelay. That is intentional: without it ndelay would skip the newer OST and max() would keep the wrong time. Still block when: - mtime/atime moves earlier than that baseline; - OP_XVALID_FLAGS is set (chattr, encryption flag): MDS never pushes flags to OSTs. This only unblocks the setattr RPC itself (including chmod(2)). stat/glimpse can still hang when no usable stripe/mirror is reachable: e.g. coreutils chmod stats via FTS before chmod(2), so the command may block on plain files even though the syscall would not. That is unchanged here (see LU-11962 for MDT SOM). Also re-push UID/GID/PROJID from the MDT inode on FLR RESYNC_DONE in lod_layout_change() with the layout version update. osp_sync normally retries ownership pushes; this is belt-and-suspenders if an MDS->OST ownership setattr was skipped (e.g. exercised with OBD_FAIL_LFSCK_BAD_OWNER; layout LFSCK can also repair that). Add sanity 39v (plain file) and sanity-flr 100a/100b. Test-Parameters: testlist=sanity env=ONLY=39v Test-Parameters: testlist=sanity-flr Assisted-by: Cursor:Grok-4.6/Opus5.5 llm_code_and_review_tools Signed-off-by: Keguang Xu <kxu@ddn.com> Change-Id: I5005451e10f293279c441c1fc6d012fb5337d95d
▶Parked (WIP / self-blocked)
(4)
69511
master
J –B ✗T –R -1/2−
LU-20802 lov: add OBJECT I/O scope read and lseek
work in progress
needs rebase
WIP
2 unresolved
XL +1103 −63
2d
LU-20802 lov: add OBJECT I/O scope read and lseek
work in progress
needs rebase
WIP
2 unresolved
XL +1103 −63
2d
build #133120 FAILURE
threads: 2 need your reply · 0 waiting on others · 0 own notes · 0 bot
reply Ronnie Sahlberg patchset comment — Aside from READ and LSEEK, should this also modify fstat and friends?
Ronnie Sahlberg
Aside from READ and LSEEK, should this also modify fstat and friends? It feels like at least block-size and eof of the object would be usefule to retreive, possibly the timestamps as well.
reply Ronnie Sahlberg lov_io.c:871 — Should we check here if the object has been administratively deactivated, like osc.active=0 and fail early?
Ronnie Sahlberg
Should we check here if the object has been administratively deactivated, like osc.active=0 and fail early?
owner Keguang Xu · uploader Keguang Xu · PS2 uploaded 26d ago · NEW · open in Gerrit ↗
commit message
LU-20802 lov: add OBJECT I/O scope read and lseek Open LL_IO_SCOPE_OBJECT for O_DIRECT pread and lseek (SEEK_DATA, SEEK_HOLE, SEEK_END). The caller names one OST object by (ios_comp_id, ios_stripe_idx); pos is an offset on that object. ios_comp_id must be non-zero and stripe index 0 is valid. A non-zero ios_obj_fid only checks that the pair still names that object. FID alone is rejected. The ioctl only checks the arguments; the target is resolved on each I/O. Add ci_obj_io / ci_obj_target and a separate lov_obj_io_ops table so OBJECT I/O is one identity sub-IO. File-offset helpers (lov_io_rw_iter_init, lov_io_iter_init, lov_io_lseek_end, lov_offset.c) are unchanged. EOF is the target object's kms, a third case beside i_size and ci_parity_eof. Do not copy the FLR layout generation into ci_layout_version; ios_layout_version is checked when the caller sets it. Reject mmap, buffered read, write, truncate, and fallocate on an OBJECT-scope fd. Dead objects return -EIO without CIT_EC_RD. OBJECT is checked before the scope switch, so STRIPE can update that switch on its own. sanity-flr test_213a and test_214 cover the ioctl and object read/lseek. Test-Parameters: ignore forjanitoronly Assisted-by: Cursor:Grok-4.6/Opus5.5 llm_code_and_review_tools Assisted-by: Cursor:Grok-4.6 llm_code_and_review_tools Signed-off-by: Keguang Xu <kxu@ddn.com> Change-Id: If4dd254f6d2d50a300d9c31d575565daacb89873
69075
master
J ✗46B ✓T ✗8R 0/2
LU-20354 quota: add usage_cache_clear for uid/gid/pid/LQA
work in progress
46 unique failures
WIP
1 unresolved
L +613 −0
38d
LU-20354 quota: add usage_cache_clear for uid/gid/pid/LQA
work in progress
46 unique failures
WIP
1 unresolved
L +613 −0
38d
janitor flagged test failures unique to this patch — full janitor results
| unique failing test | history |
|---|---|
| sanity-quota@ldiskfs+DNE:test_1j | seen in 2 other reviews |
| sanity-quota@ldiskfs+DNE:test_3a | seen in 3 other reviews |
| sanity-quota@ldiskfs+DNE:test_3b | seen in 3 other reviews |
| sanity-quota@ldiskfs+DNE:test_3c | seen in 3 other reviews |
| sanity-quota@ldiskfs+DNE:test_5 | seen in 2 other reviews |
| sanity-quota@ldiskfs+DNE:test_6 | seen in 3 other reviews |
| sanity-quota@ldiskfs+DNE:test_7a | seen in 2 other reviews |
| sanity-quota@ldiskfs+DNE:test_7b | seen in 2 other reviews |
| sanity-quota@ldiskfs+DNE:test_7c | seen in 2 other reviews |
| sanity-quota@ldiskfs+DNE:test_16a | seen in 3 other reviews |
| sanity-quota@ldiskfs+DNE:test_17 | seen in 8 other reviews |
| sanity-quota@ldiskfs+DNE:test_25 | seen in 3 other reviews |
| sanity-quota@ldiskfs+DNE:test_33 | seen in 6 other reviews |
| sanity-quota@ldiskfs+DNE:test_34 | seen in 6 other reviews |
| sanity-quota@ldiskfs+DNE:test_35 | seen in 2 other reviews |
| sanity-quota@ldiskfs+DNE:test_37 | seen in 6 other reviews |
| sanity-quota@ldiskfs+DNE:test_38 | seen in 6 other reviews |
| sanity-quota@ldiskfs+DNE:test_40c | seen in 3 other reviews |
| sanity-quota@ldiskfs+DNE:test_64 | seen in 3 other reviews |
| sanity-quota@ldiskfs+DNE:test_67 | seen in 2 other reviews |
| sanity-quota@ldiskfs+DNE:test_71a | seen in 4 other reviews |
| sanity-quota@ldiskfs+DNE:test_71b | seen in 4 other reviews |
| sanity-quota@ldiskfs+DNE:test_72 | seen in 4 other reviews |
| sanity-quota@ldiskfs+DNE:test_73a | seen in 8 other reviews |
| sanity-quota@ldiskfs+DNE:test_75 | seen in 8 other reviews |
| sanity-quota@ldiskfs+DNE:test_80 | seen in 3 other reviews |
| sanity-quota@zfs:test_1j | seen in 2 other reviews |
| sanity-quota@zfs:test_3a | seen in 2 other reviews |
| sanity-quota@zfs:test_3b | seen in 2 other reviews |
| sanity-quota@zfs:test_3c | seen in 2 other reviews |
| sanity-quota@zfs:test_5 | seen in 2 other reviews |
| sanity-quota@zfs:test_6 | seen in 2 other reviews |
| sanity-quota@zfs:test_7a | seen in 2 other reviews |
| sanity-quota@zfs:test_7b | seen in 2 other reviews |
| sanity-quota@zfs:test_7c | seen in 2 other reviews |
| sanity-quota@zfs:test_17 | seen in 36 other reviews |
| sanity-quota@zfs:test_25 | seen in 29 other reviews |
| sanity-quota@zfs:test_33 | seen in 35 other reviews |
| sanity-quota@zfs:test_34 | seen in 35 other reviews |
| sanity-quota@zfs:test_35 | seen in 2 other reviews |
| sanity-quota@zfs:test_37 | seen in 35 other reviews |
| sanity-quota@zfs:test_38 | seen in 35 other reviews |
| sanity-quota@zfs:test_64 | seen in 3 other reviews |
| sanity-quota@zfs:test_72 | seen in 16 other reviews |
| sanity-quota@zfs:test_73a | seen in 19 other reviews |
| sanity-quota@zfs:test_75 | seen in 9 other reviews |
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-dne-part-4 | RHEL 9.7 / x86_64 | ran 9 tests. 1 tests failed: sanity-quota. | session |
| review-dne-part-4 | RHEL 10.1 / x86_64 | ran 9 tests. 1 tests failed: sanity-quota. | session |
| review-dne-zfs-part-3 | RHEL 9.8 / x86_64 | ran 3 tests. 1 tests failed: conf-sanity. | session |
| review-dne-zfs-part-3 | RHEL 10.1 / x86_64 | ran 3 tests. 1 tests failed: conf-sanity. | session |
| review-dne-zfs-part-4 | RHEL 9.8 / x86_64 | ran 9 tests. 1 tests failed: sanity-quota. | session |
| review-dne-zfs-part-4 | RHEL 10.1 / x86_64 | ran 9 tests. 1 tests failed: sanity-quota. | session |
| review-dne-zfs-part-7 | RHEL 9.8 / x86_64 | ran 6 tests. 1 tests failed: sanity-ec. | session |
| review-zfs | RHEL 8.10 / x86_64 | ran 8 tests. 1 tests failed: sanity-quota. | session |
build #132036 SUCCESS · tests all Maloo sessions
threads: 1 need your reply · 0 waiting on others · 0 own notes · 0 bot
reply Andreas Dilger COMMIT_MSG:16 — This is repeatedly failing sanity-quota testing, enough that the sanity-quota 33, 34, 35, ... failures are the top failing subtests in the p
Andreas Dilger
This is repeatedly failing sanity-quota testing, enough that the sanity-quota 33, 34, 35, ... failures are the top failing subtests in the past week: https://testing.whamcloud.com/search?client_branch=master&server_branch=master&status%5B%5D=FAIL&status%5B%5D=TIMEOUT&status%5B%5D=CRASH&test_set_script_id=61149410-4a46-11e0-a7f6-52540025f9af&sub_test_script_id=0ffc6336-df89-11e0-9909-52540025f9af&start_date=2026-09-14&end_date=2026-09-20&source=sub_tests#redirect For more experimental patches, please use `Test-Parameters: fortestonly testlist=sanity-quota` to run only a single session and/or `Test-Parameters: ignore forjanitoronly` so that Jenkins skips the builds and Autotest does not run at all. The Autotest queues are 2000 sessions deep, so we need to pay attention to our testing requests...
owner Keguang Xu · uploader Keguang Xu · PS1 uploaded 44d ago · NEW · open in Gerrit ↗
commit message
LU-20354 quota: add usage_cache_clear for uid/gid/pid/LQA Allow invalidating per-slave usage-cache values without dropping slots or restarting QMT, so tests and admins can force a refresh of stale ID or LQA aggregates via a write-only lprocfs parameter. LQA clear targets the shared LQA lqe (qid=1) on the DT and MD pools. Slots are marked QMT_USG_UNSET so a later sum still misses. Assisted-by: Cursor:Grok-4.6 llm_code_and_review_tools Signed-off-by: Keguang Xu <kxu@ddn.com> Change-Id: Id5cbf1304ac5f200f49e184373e79ebc66a9c93e
67998
master
J !B ✗T –R 0/2
LU-20566 ec: reuse surviving DIO bounce pages on EC recovery
work in progress
WIP
2 unresolved
L +735 −23
81d
LU-20566 ec: reuse surviving DIO bounce pages on EC recovery
work in progress
WIP
2 unresolved
L +735 −23
81d
janitor run: 1 failing config(s), none unique to this patch — janitor results
build #129341 FAILURE
threads: 2 need your reply · 0 waiting on others · 0 own notes · 0 bot
reply Misc Code Checks Robot (Gatekeeper helper) lov_io.c:2117 — warn: lov_ec_read_stripe_pages():variable dereferenced before check 'inode' (see line 2094)
Misc Code Checks Robot (Gatekeeper helper)
warn: lov_ec_read_stripe_pages():variable dereferenced before check 'inode' (see line 2094)
reply Misc Code Checks Robot (Gatekeeper helper) cl_io.c:1462 — warn: cl_ec_reuse_take():missing conversion: 'seg->crs_index + seg->crs_npages' 'unit_page + unit_array_size'
Misc Code Checks Robot (Gatekeeper helper)
warn: cl_ec_reuse_take():missing conversion: 'seg->crs_index + seg->crs_npages' 'unit_page + unit_array_size'
owner Keguang Xu · uploader Keguang Xu · PS1 uploaded 81d ago · NEW · open in Gerrit ↗
commit message
LU-20566 ec: reuse surviving DIO bounce pages on EC recovery When llite.*.ec_dio_reuse is enabled, preserve successfully filled unaligned DIO bounce pages across the CIT_EC_RD restart and adopt them as EC_DPG_UPTODATE so healthy stripes are not re-fetched. Layout generation is checked after locks are held; on mismatch the container is dropped and full recovery proceeds. Default off. Encrypted files, aligned DIO, and AIO are excluded from reuse. Test-Parameters: trivial Signed-off-by: Keguang Xu <kxu@ddn.com> Change-Id: I2ff8c2bfc7de81ebd44aefc34fd627d8f2e630ea
67878
master
J !B ✗T –R 0/2
LU-20566 ec: recover O_DIRECT reads with transient pages
work in progress
WIP
3 unresolved
L +649 −241
83d
LU-20566 ec: recover O_DIRECT reads with transient pages
work in progress
WIP
3 unresolved
L +649 −241
83d
janitor run: 1 failing config(s), none unique to this patch — janitor results
build #129151 FAILURE
threads: 3 need your reply · 0 waiting on others · 0 own notes · 0 bot
reply Misc Code Checks Robot (Gatekeeper helper) lov_io.c:2053 — warn: lov_ec_read_stripe_pages():variable dereferenced before check 'inode' (see line 2004)
Misc Code Checks Robot (Gatekeeper helper)
warn: lov_ec_read_stripe_pages():variable dereferenced before check 'inode' (see line 2004)
reply Misc Code Checks Robot (Gatekeeper helper) lov_io.c:3738 — warn: lov_io_ec_rd_start():potentially one past the end of array 'recov_ptrs[k]'
Misc Code Checks Robot (Gatekeeper helper)
warn: lov_io_ec_rd_start():potentially one past the end of array 'recov_ptrs[k]'
reply Misc Code Checks Robot (Gatekeeper helper) lov_io.c:3675 — warn: lov_io_ec_rd_start():potentially one past the end of array 'recov_ptrs[k]'
Misc Code Checks Robot (Gatekeeper helper)
warn: lov_io_ec_rd_start():potentially one past the end of array 'recov_ptrs[k]'
owner Keguang Xu · uploader Keguang Xu · PS2 uploaded 83d ago · NEW · open in Gerrit ↗
commit message
LU-20566 ec: recover O_DIRECT reads with transient pages Keep sync DIO EC recovery off the page cache: reconstruct into CPT_TRANSIENT pages and copy_page_to_iter() to the caller. Fall back to the buffered path when native allocation or encryption bounce pages are unavailable. AIO EC recovery remains unsupported. Successful and failed data-stripe submits keep non-owning out_pages[] aliases for reconstruction; ownership stays on the page list and lov_ec_forget_listed_pages() drops the aliases in rg_cleanup (buffered and DIO share this rule). Test-Parameters: trivial Signed-off-by: Keguang Xu <kxu@ddn.com> Change-Id: I396daeda49c8a522b6b67f9fe618f4be9130b6d6
4 open
▶Re-review needed — new patchset since your vote
(0)
nothing here right now
▶Awaiting your review — CI green
(2)
69493
master
J –B ✓T ✓R +3/2
LU-19823 lod: unique failure domains per EC raid set
reviewing Andreas Dilger · CI green — awaiting your review
pt_ecro218_review_priority218_review_blocker
L +464 −119
29m
LU-19823 lod: unique failure domains per EC raid set
reviewing Andreas Dilger · CI green — awaiting your review
pt_ecro218_review_priority218_review_blocker
L +464 −119
29m
build #134652 SUCCESS · tests all Maloo sessions
reviews:
Keguang Xu +1 · Ronnie Sahlberg +1 · Maximilian Dilger +1
no vote yet: Marc Vef, Oleg Drokin
patchsets:
PS1 10-07
PS2 10-13
PS3 10-24
PS4 10-27
PS5 10-28
tags: pt_ecro 218_review_priority 218_review_blocker
owner Andreas Dilger · uploader Andreas Dilger · PS5 uploaded 6d ago · NEW · open in Gerrit ↗
commit message
LU-19823 lod: unique failure domains per EC raid set
With "lfs setstripe -c N --ec K+M" the allocator required every data
and parity stripe of the component to be in a different failure
domain, while with -C this was only needed within each raid set.
A wide file such as "-c 40 --ec 8+2" on 40 OSTs needs 10 domains
per raid set but 50 for the component, so it failed with ENOSPC
unless the file was overstriped.
Each raid set is protected on its own, so require different failure
domains only within each raid set, for both -c and -C. With -c the
data stripes are still on different OSTs, so the file can use the
bandwidth of all OSTs, and parity shares OSTs with the data of other
raid sets. A -c component short of OSTs is split into raid sets
again after its stripes are placed, so keep each run of consecutive
data stripes as long as the largest possible raid set in different
domains. QoS no longer trims the data stripes of a -c component with
several raid sets to leave separate OSTs for its parity.
Each raid set still needs its data and parity stripes in different
failure domains, but the data stripes of a -c component need fewer, so
with too few domains the data could be placed and the RR allocator
then accepted a parity component with too few stripes. Check the
domains before placing the data, and use raid sets of fewer data
stripes, down to K - K/4 as in lod_stripe_count_min(), or fail with
ENOSPC. A single raid set gets fewer data stripes, so "-c 3 --ec 2+1"
with 3 domains becomes "-c 2 --ec 2+1". Also fail a parity component
that RR could not place completely, as QoS does, since other mirrors
can use up domains that this check does not see.
An EC file is also two mirrors, so the FLR check required different
domains for all stripes of the data component too. Leave the domains
within an EC component to the EC check.
Also fix lod_comp_find_linked() matching a plain mirror component to
the parity of an EC mirror, which let the plain mirror share a failure
domain with that parity, and restore the count of OSTs available to
mirrors when QoS gives up, so the RR fallback can place EC stripes.
Rename lod_fd_check_*() to lod_fdom_check_*() so "fd" is not read as a
file descriptor. Describe the mirror rules in
obdfilter.failure_domain.4 and reference it from lfs-setstripe.1,
lfs-mirror-create.1 and lfs-mirror-extend.1.
Fixes: 026de09ead3d ("LU-19823 lod: Make the stripe allocators failure domain aware.")
Fixes: c53abca9f9ee ("LU-19852 lod: raidset aware stripe allocator")
Signed-off-by: Andreas Dilger <adilger@thelustrecollective.com>
Assisted-by: ClaudeCode:claude-opus-5.5 ltvm,llm_code_and_review_tools
Change-Id: Ic197df8fba1a9e0a313442cc435d1959763ebbe5
69771
master
J –B ✓T ✓R 0/2
LU-11962 lod: prepare for a SOM mirror in FLR files
reviewing Andreas Dilger · CI green — awaiting your review
pw_shepherdssom
1 unresolved
M +168 −81
3h
LU-11962 lod: prepare for a SOM mirror in FLR files
reviewing Andreas Dilger · CI green — awaiting your review
pw_shepherdssom
1 unresolved
M +168 −81
3h
build #134987 SUCCESS · tests all Maloo sessions
no vote yet: Maximilian Dilger, Artem Blagodarenko
patchsets:
PS1 10-17
PS2 10-28
PS3 10-31
threads: 1 need your reply · 0 waiting on others · 0 own notes · 0 bot
reply Gerrit AI review for Lustre liblustreapi_layout.c:4179 — (minor) When the only stale component is the SOM one, this returns before the "Output at least one log, regardless of stats_interval" block
Gerrit AI review for Lustre
(minor) When the only stale component is the SOM one, this returns before the "Output at least one log, regardless of stats_interval" block at the end of the function. So `lfs mirror resync --stats` prints no YAML at all for that file.
Nothing creates a SOM mirror yet, but once strict SOM adds one to non-mirrored files, a write followed by a resync will hit this path on almost every such file. b285e26138 ("LU-12668 ec: add --stats and -W into EC resync") made an EC-only resync emit stats for lamigo liveness (sanity-ec test_12g checks for `copied:`). Should this path emit one zero-byte stats record too, e.g. by skipping the copy loop rather than returning early?
tags: pw_shepherd ssom
owner Andreas Dilger · uploader Andreas Dilger · PS3 uploaded 3d ago · NEW · open in Gerrit ↗
commit message
LU-11962 lod: prepare for a SOM mirror in FLR files Strict Size-on-MDT of files that are not mirrored will use a mirror made of a single data-less foreign HSM component flagged HS_SOM, so that clients send a layout write intent before they modify the file. Prepare servers and utilities to handle files with such a mirror: - move the code that appends an HSM mirror to a layout out of lod_declare_pccro_set() into lod_layout_add_hsm_mirror(); - define HS_SOM, shown as "som" by "lfs getstripe"; - clear the stale flag of a SOM component at RESYNC_DONE, as it has no data to resync, and accept its ID in the list of resynced ones; - copy no data into a SOM mirror in llapi_mirror_resync_many(), and read from no foreign mirror in llapi_mirror_find(), so "lfs mirror resync" makes such a file RDONLY again; - do not reuse a SOM mirror as the HSM mirror of PCC-RO. Test-Parameters: trivial testlist=sanity-flr,sanity-pcc Signed-off-by: Andreas Dilger <adilger@thelustrecollective.com> Assisted-by: ClaudeCode:claude-opus-5-5 ltvm llm_code_and_review_tools Change-Id: I28ceb8221a6402dfd69362989e9705ccb85056a8
▶Awaiting review — CI not green yet
(2)
66557
master
J ✗7B ✓T ✗15R -1/2−
LU-20253 mdt: implement RENAME_NOREPLACE support
reviewing Timothy Day · blocked on the owner: janitor: test failures unique to this patch; enforced tests failed
7 unique failures
11 unresolved
M +82 −2
89d
LU-20253 mdt: implement RENAME_NOREPLACE support
reviewing Timothy Day · blocked on the owner: janitor: test failures unique to this patch; enforced tests failed
7 unique failures
11 unresolved
M +82 −2
89d
blocked on the owner: janitor: test failures unique to this patch; enforced tests failed
awaiting review (CI not green yet)
janitor flagged test failures unique to this patch — full janitor results
| unique failing test | history |
|---|---|
| sanity1@ldiskfs+DNE:test_24Ka | NEW unique failure for this branch in the last 30 days, and was seen 0 times across 0 other branches 0 reviews |
| sanity1@ldiskfs+DNE:test_24Kb | NEW unique failure for this branch in the last 30 days, and was seen 0 times across 0 other branches 0 reviews |
| sanity2@ldiskfs+DNE:test_160k | seen in 1 other review |
| sanity2@ldiskfs+DNE:test_160w | seen in 1 other review |
| sanity1@zfs:test_24Ka | NEW unique failure for this branch in the last 30 days, and was seen 0 times across 0 other branches 0 reviews |
| sanity2@zfs:test_160k | seen in 2 other reviews |
| sanity2@zfs:test_160w | seen in 1 other review |
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-dne-part-1 | RHEL 9.7 / x86_64 | ran 3 tests. 1 tests failed: sanity. | session |
| review-dne-part-1 | RHEL 10.1 / x86_64 | ran 3 tests. 1 tests failed: sanity. | session |
| review-dne-part-5 | RHEL 9.7 / x86_64 | ran 5 tests. 1 tests failed: lustre-rsync-test. | session |
| review-dne-selinux-ssk-part-1 | RHEL 8.10 / x86_64 | ran 3 tests. 1 tests failed: sanity. | session |
| review-dne-subtest-change | RHEL 9.7 / x86_64 | ran 3 tests. 1 tests failed: sanity. | session |
| review-dne-subtest-change | RHEL 9.7 / x86_64 | ran 3 tests. 1 tests failed: sanity. | session |
| review-dne-zfs-part-1 | RHEL 9.7 / x86_64 | ran 3 tests. 1 tests failed: sanity. | session |
| review-dne-zfs-part-1 | RHEL 10.1 / x86_64 | ran 3 tests. 1 tests failed: sanity. | session |
| review-dne-zfs-part-5 crashed | RHEL 9.7 / x86_64 | ran 5 tests. 2 tests failed: sanityn, lustre-rsync-test. %% THIS TEST SESSION CRASHED %% | session |
| review-dne-zfs-subtest-change | RHEL 9.7 / x86_64 | ran 3 tests. 1 tests failed: sanity. | session |
| review-dne-zfs-subtest-change | RHEL 9.7 / x86_64 | ran 3 tests. 1 tests failed: sanity. | session |
| review-ldiskfs | RHEL 8.10 / x86_64 | ran 5 tests. 1 tests failed: sanity. | session |
| review-ldiskfs | RHEL 8.10 / x86_64, RHEL 9.7 / x86_64 | ran 5 tests. 1 tests failed: sanity. | session |
| review-ldiskfs-arm | RHEL 8.10 / x86_64, Rocky 9.5 / aarch64 | ran 6 tests. 1 tests failed: sanity. | session |
| review-ldiskfs-ubuntu | RHEL 8.10 / x86_64, Ubuntu 24.04 / x86_64 | ran 6 tests. 1 tests failed: sanity. | session |
build #128743 SUCCESS · tests all Maloo sessions
no vote yet: Qian Yingjin, Andreas Dilger, Lai Siyao
patchsets:
PS1 06-09
PS2 06-26
PS3 08-05
threads: 11 need your reply · 0 waiting on others · 0 own notes · 0 bot
reply Gerrit AI review for Lustre COMMIT_MSG:9 — (minor) The body quotes the man page but doesn't describe what the patch does: the new MDS_RENAME_NOREPLACE op_bias bit, the sp_rename_norep
Gerrit AI review for Lustre
(minor) The body quotes the man page but doesn't describe what the patch does: the new MDS_RENAME_NOREPLACE op_bias bit, the sp_rename_noreplace spec flag, where the check lands in mdt_reint_rename(), why replay is exempt, and the two new sanity cases. The RENAME_WHITEOUT commit message walks through its implementation and negotiation; this one leaves every hunk unexplained.
reply Gerrit AI review for Lustre COMMIT_MSG:17 — (style) Signed-off-by: normally comes before Change-Id: - the reversed order usually means the Lustre commit hooks aren't installed.
Gerrit AI review for Lustre
(style) Signed-off-by: normally comes before Change-Id: - the reversed order usually means the Lustre commit hooks aren't installed. A Test-Parameters: line would also be usual for a change that adds a new on-wire op_bias bit.
reply Andreas Dilger lustre_idl.h:2118 — (minor) rather than use up three of the few remaining bits for the rename types, they could be encoded into two bits? According to the man
Andreas Dilger
(minor) rather than use up three of the few remaining bits for the rename types, they could be encoded into two bits? According to the man page, the rename types are mutually exclusive.
reply Gerrit AI review for Lustre lustre_idl.h:2120 — (defect) Every mds_op_bias value up through MDS_RENAME_WHITEOUT has a CHECK_VALUE_X() in lustre/utils/wirecheck.c and a matching LASSERTF()
Gerrit AI review for Lustre
(defect) Every mds_op_bias value up through MDS_RENAME_WHITEOUT has a CHECK_VALUE_X() in lustre/utils/wirecheck.c and a matching LASSERTF() in both lustre/utils/wiretest.c and lustre/ptlrpc/wiretest.c. Should MDS_RENAME_NOREPLACE (0x20000000) be added to all three?
reply Gerrit AI review for Lustre lustre_idl.h:2120 — (minor) The suggestion from an earlier patchset to pack the rename types into fewer bits rather than one bit each doesn't look answered yet.
Gerrit AI review for Lustre
(minor) The suggestion from an earlier patchset to pack the rename types into fewer bits rather than one bit each doesn't look answered yet. One input for that: the rename types are not all mutually exclusive. do_renameat2() only rejects NOREPLACE/WHITEOUT when combined with EXCHANGE, so NOREPLACE|WHITEOUT is a legal combination and an encoded field would need five states, not four.
reply Gerrit AI review for Lustre mdt_reint.c:3032 — (minor) The replay exemption is the subtle half of this hunk and the comment doesn't mention it. Presumably it is here because a replayed re
Gerrit AI review for Lustre
(minor) The replay exemption is the subtle half of this hunk and the comment doesn't mention it. Presumably it is here because a replayed rename re-executes against pre-crash state where the target name still exists, and failing it would break recovery - worth saying so, otherwise the !req_is_replay() reads like an afterthought.
reply Andreas Dilger sanity.sh:1711 — Better not to hide the error messages here, so that it can be seen what error is generated. Otherwise, it may be possible that this test is
Andreas Dilger
Better not to hide the error messages here, so that it can be seen what error is generated. Otherwise, it may be possible that this test is failing for the wrong reason and it would be hard to know (eg. if renameat2 binary is missing or whatever)
reply Gerrit AI review for Lustre sanity.sh:1701 — (defect) 2.17.54 is below the current tree version (2.17.56) and below the 2.17.56 gate that test_24Ja/24Jb use for the renameat2 support th
Gerrit AI review for Lustre
(defect) 2.17.54 is below the current tree version (2.17.56) and below the 2.17.56 gate that test_24Ja/24Jb use for the renameat2 support this builds on, so the gate lets the test run on servers that don't have the feature. Should this be the version the patch actually lands in? The client side matters too: ll_rename() rejects RENAME_NOREPLACE without this patch, so a CLIENT_VERSION check may be wanted as well.
reply Gerrit AI review for Lustre sanity.sh:1713 — (minor) Raised on an earlier patchset and still here - discarding stderr hides which error renameat2 actually reported, which makes a failur
Gerrit AI review for Lustre
(minor) Raised on an earlier patchset and still here - discarding stderr hides which error renameat2 actually reported, which makes a failure for an unrelated reason hard to diagnose.
reply Gerrit AI review for Lustre sanity.sh:1729 — (defect) After a successful rename src is gone, so this test is false and the AND-list is the last command of the function - test_24Ka retur
Gerrit AI review for Lustre
(defect) After a successful rename src is gone, so this test is false and the AND-list is the last command of the function - test_24Ka returns 1 and run_one() turns that into "test_24Ka failed with 1". The other `... && error` lines at the end of a test in this file append `|| true` for exactly this reason. test_24Kb has the same ending.
reply Gerrit AI review for Lustre sanity.sh:1734 — (style) Not a bug, but the neighbouring test writes the run condition in the positive form, which is the convention in this suite:
Gerrit AI review for Lustre
(style) Not a bug, but the neighbouring test writes the run condition in the positive form, which is the convention in this suite:
(( MDSCOUNT >= 2 )) || skip "needs >= 2 MDTs"
owner Timothy Day · uploader Timothy Day · PS3 uploaded 90d ago · NEW · open in Gerrit ↗
commit message
LU-20253 mdt: implement RENAME_NOREPLACE support For completeness, implement RENAME_NOREPLACE on Lustre. Quoting from the man page [1]: Don't overwrite newpath of the rename. Return an error if newpath already exists. [1] https://www.man7.org/linux/man-pages/man2/rename.2.html Change-Id: I321669b38ce74d7b211788f5a223fb98142826f3 Signed-off-by: Timothy Day <timday@thelustrecollective.com>
61956
master
J !B ✓T ✓R -2/2−
LU-19264 ldlm: server notify lock clients to reclaim LRU locks
reviewing Qian Yingjin · blocked on the owner: needs rebase — checkpatch: cannot be cherry-picked
needs rebase
stalled 134d
19 unresolved
M +240 −8
134d
LU-19264 ldlm: server notify lock clients to reclaim LRU locks
reviewing Qian Yingjin · blocked on the owner: needs rebase — checkpatch: cannot be cherry-picked
needs rebase
stalled 134d
19 unresolved
M +240 −8
134d
blocked on the owner: needs rebase — checkpatch: cannot be cherry-picked
awaiting review (CI not green yet)
janitor run: 1 failing config(s), none unique to this patch — janitor results
5 optional test failure(s) — informational
build #117706 SUCCESS · tests all Maloo sessions
reviews:
Andreas Dilger -1
patchsets:
PS1 10-22
PS2 10-22
PS3 10-22
threads: 14 need your reply · 1 waiting on others · 4 own notes · 0 bot
reply Andreas Dilger COMMIT_MSG:16 — This should be reworded to make it clear that these issues only exist in the old version of the code where the lock server is making the dec
Andreas Dilger
This should be reworded to make it clear that these issues only exist in the old version of the code where the lock server is making the decision for which locks to cancel. Something like: ``` If the lock server selects specific locks to cancel, this can lead ... ```
reply Andreas Dilger COMMIT_MSG:17 — `... in active use on the client ...`
Andreas Dilger
`... in active use on the client ...`
reply Andreas Dilger COMMIT_MSG:18 — `Server-selected lock revocation ...`
Andreas Dilger
`Server-selected lock revocation ...`
reply Andreas Dilger COMMIT_MSG:24 — Please describe the mechanism implemented.
Andreas Dilger
Please describe the mechanism implemented.
reply Andreas Dilger patchset comment — > IMHO I'd rather keep the CLV/SLV mechnism since it DO give the clients
Andreas Dilger
If we are moving forward with a new server-side notification mechanism to cancel client locks (instead of the CLV/SLV exchange), then we should consider to disable this mechanism completely once all clients support the new mechanism. The old mechanism generates a non-trivial amount of overhead on the client to update the CLV for each namespace (it was originally done on 1-second intervals for *all* namespaces on the client, but had to be scaled down to one namespace per 1-second interval to avoid impacting client performance). If the server starts sending the "magic cancel" then the client could stop calculating the CLV and the server can stop sending it as well.
kg.xu
IMHO I'd rather keep the *CLV/SLV* mechnism since it `DO` give the clients the choice to select the locks that they prefer to return. With "magic cancel", clients still need to figure out which locks they should cancel, it's just another form of *CLV*? The issue to the SLV is it's not aligned with CLV in some degree right now, I'm on it and will upload a patch hoping to fix it(with CLV intact). Still, we need reclaim notification here as the safety net that should be relied upon once *CLV/SLV* broken.
Andreas Dilger
> IMHO I'd rather keep the CLV/SLV mechnism since it DO give the clients > the choice to select the locks that they prefer to return. > With "magic cancel", clients still need to figure out which locks > they should cancel, it's just another form of CLV? Sorry, I don't understand your comment. In both cases, CLV/SLV and "magic cancel" (or set_info), the client needs to decide *which* locks to cancel, while the server decides *how many* to cancel (though I guess the client could still use this as a "hint" and cancel more or fewer locks if needed). We *want* the client to decide this, since it has more information about the current lock usage than the server. One benefit of "magic cancel" is that it is a simpler mechanism to understand, a clear notification to cancel "some" locks. The CLV/SLV mechanism has (AFAIK) been at least partly broken for many years and nobody can ever get it working. The main benefit of CLV/SLV (AFAIK) is that the client is essentially notified of a "rate" to cancel locks, so the server does not need to continually send RPCs to each client. I guess the same could be done with "magic cancel", at which point they are nearly the same thing. It would definitely be awesome of you could fix this code to be more robust.
reply Andreas Dilger ldlm_lockd.c:2447 — (defect) I would prefer that this check all lock resource fields are {0,0,0,0} so that we can have other "special-case" requests in the futu
Andreas Dilger
(defect) I would prefer that this check all lock resource fields are {0,0,0,0} so that we can have other "special-case" requests in the future.
reply Andreas Dilger ldlm_lockd.c:2454 — Presumably this CERROR() is for debugging only? There may be reason the client cannot cancel these locks (eg. it just cancelled locks befor
Andreas Dilger
(style) spaces around that '=' (typo) "to" is not needed here
Andreas Dilger
Presumably this CERROR() is for debugging only? There may be reason the client cannot cancel these locks (eg. it just cancelled locks before this RPC was received).
reply Andreas Dilger ldlm_lockd.c:2456 — Should this send the reply *before* shrinking the LRU (to avoid RPC timeout and resend), or are the cancelled locks added into this reply RP
Andreas Dilger
Should this send the reply *before* shrinking the LRU (to avoid RPC timeout and resend), or are the cancelled locks added into this reply RPC?
note kg.xu ldlm_reclaim.c:240 — (typo) across
kg.xu
(typo) across (style) comment part: @params ...
reply Andreas Dilger ldlm_reclaim.c:241 — (minor) it isn't clear that the number of clients holding locks is needed? Clients holding no (or few) locks do not need to be sent any cal
Andreas Dilger
(minor) it isn't clear that the number of clients holding locks is needed? Clients holding no (or few) locks do not need to be sent any callback. The important thing is the ratio of each export's locks out of the total number of granted locks.
reply Andreas Dilger ldlm_reclaim.c:257 — I think yes, there should be some small number of locks left on the client? Otherwise, the client locks may be thrashing on a few locks tha
Andreas Dilger
I think yes, there should be some small number of locks left on the client? Otherwise, the client locks may be thrashing on a few locks that the client needs (eg. user home directory), even though some other client may have thousands of locks.
reply Andreas Dilger ldlm_reclaim.c:301 — What happens if an old client gets an AST like this? Does it just ignore the zero handle that it doesn't have a matching local handle for?
Andreas Dilger
What happens if an old client gets an AST like this? Does it just ignore the zero handle that it doesn't have a matching local handle for?
owner Qian Yingjin · uploader Qian Yingjin · PS3 uploaded 377d ago · NEW · open in Gerrit ↗
commit message
LU-19264 ldlm: server notify lock clients to reclaim LRU locks In large-scale Lustre cluster with a number of clients, the server side lock namespace must manage all locks granted to clients. And these locks are cached in server side memory. Due to the limited server memory capacity, the server must reclaim locks when it is under memory pressure. However, when lock reclaim is triggered, the LRU or frequency of locks cached on the server lock namespace may not reflect recent used accurately. This can lead to premature reclaim of locks that are actually still in active use or may be reused in the near future by the client. The lock revocation will drop the cache data or metadata protected by the lock on the client. This may result in unnecessary network traffic, re-reading the data (such as memory resident library files) dropped from cache wrongly and degraded system performance. To solve this problem, this patch implements a mechanism to let the lock server to notify clients, which have much accurate and realtime information about lock usage, to make better decision for lock reclaim on client side. Signed-off-by: Yingjin Qian <qian@ddn.com> Change-Id: I55a7d5876e5dfbdfbfc84d073e86dc0156f886cd
▶Voted on current patchset
(0)
nothing here right now
CC'd — on your radar, review not requested
(3)
64086
master
J –B ✓T ✓R +1/2
LU-20958 lov: handle unavailable EC stripes in lseek
cc Patrick Farrell ·
ec2
pt_ecro218_review_priority
6 unresolved
L +453 −12
9h
LU-20958 lov: handle unavailable EC stripes in lseek
cc Patrick Farrell ·
ec2
pt_ecro218_review_priority
6 unresolved
L +453 −12
9h
build #134718 SUCCESS · tests all Maloo sessions
reviews:
Andreas Dilger +1
no vote yet: Keguang Xu, Marc Vef, Zhenyu Xu
patchsets:
PS44 08-19 Maximilian Dilger
PS45 10-14 Andreas Dilger
PS46 10-16 Andreas Dilger
PS47 10-21 Andreas Dilger
PS48 10-29 Andreas Dilger
+43 earlier
PS1 02-17 PS2 03-29 PS3 03-29 PS4 03-29 PS5 03-29 Ronnie Sahlberg PS6 03-30 Ronnie Sahlberg PS7 03-31 Ronnie Sahlberg PS8 03-31 PS9 03-31 Ronnie Sahlberg PS10 04-01 Ronnie Sahlberg PS11 04-01 Ronnie Sahlberg PS12 04-07 Ronnie Sahlberg PS13 04-08 PS14 04-09 PS15 04-09 PS16 04-09 PS17 04-09 PS18 04-09 PS19 04-09 PS20 04-09 PS21 04-09 PS22 04-10 PS23 04-10 PS24 04-10 PS25 04-10 PS26 04-10 PS27 04-14 PS28 04-14 PS29 04-14 PS30 04-15 PS31 04-15 PS32 04-15 PS33 04-25 PS34 05-20 Marc Vef PS35 06-04 Ronnie Sahlberg PS36 07-13 Marc Vef PS37 07-13 Marc Vef PS38 07-13 Marc Vef PS39 07-15 Marc Vef PS40 07-16 Marc Vef PS41 07-26 Marc Vef PS42 07-26 Marc Vef PS43 08-17 Marc Vefthreads: 6 need your reply · 0 waiting on others · 0 own notes · 0 bot
reply Marc Vef patchset comment — Highlighting a conflict with 69111. This patch (and the parent) needs to be refreshed due to 68361 since the code is no longer compatible. (
Marc Vef
Highlighting a conflict with 69111. This patch (and the parent) needs to be refreshed due to 68361 since the code is no longer compatible. (-1) Does this patch need the current parent patches or can it be rebased and updatd for the RAID10 patch now?
reply Marc Vef patchset comment — Conflicts with #69694: it sits on ps3 of #69694, and the current patch set (ps5) conflicts with it in lustre/lov/lov_io.c, lustre/tests/sani
Marc Vef
Conflicts with #69694: it sits on ps3 of #69694, and the current patch set (ps5) conflicts with it in lustre/lov/lov_io.c, lustre/tests/sanity-ec.sh. It needs a rebase onto #69694 before it can land.
reply Gerrit AI review for Lustre file.c:5435 — (defect) The code here depends on lli_attr_valid losing OBD_MD_FLSIZE when the UPDATE lock is cancelled. That clearing is added by 69828 ("L
Gerrit AI review for Lustre
(defect) The code here depends on lli_attr_valid losing OBD_MD_FLSIZE when the UPDATE lock is cancelled. That clearing is added by 69828 ("LU-20958 llite: drop strict SOM size with the UPDATE lock"), but 69828 is not an ancestor of this patch set. The parent is 8bef60849e (an older patch set of 69694), which sits directly on 40b80bc048 on master. In this tree, lli_attr_valid is written only in ll_update_inode(), and nothing clears it in ll_md_blocking_ast().
Without 69828, this sequence goes wrong:
client A: open/stat while the file is in sync -> OBD_MD_FLSIZE, i_size = S
client B: writes the file up to T > S (MDS revokes LAYOUT|UPDATE, marks the SOM stale)
an OST of a data stripe becomes unavailable
client A: lseek(SEEK_DATA, S) -> ci_ec_unavail, but FLSIZE is still set
-> no revalidate; vvp_io_lseek_start() skips ll_merge_attr()
-> eof = S, so the result is -ENXIO although [S, T) has data
Because ll_merge_attr() is skipped, this also misses B's data on the stripes that are available. Should this patch be rebased onto 69828 so that the two cannot land in the wrong order?
reply Andreas Dilger lov_io.c:3833 — Not yet in PS48: 68361 is still at PS17 and its stack does not apply to master. This will be done when 64086 is rebased onto the refreshed
Andreas Dilger
This needs to be updated for the LU-20690 parity layout in https://review.whamcloud.com/68361 ("LU-20690 ec: like-RAID10 parity layout"), which removes RAID0|PARITY: parity components become LOV_PATTERN_PARITY with the p parity objects of a raid set sharing one offset. Once that lands, this check returns -EOPNOTSUPP for every EC file and the parity SEEK_DATA/SEEK_HOLE path is never used. Please check for the parity flag/pattern instead and use the new mapping below.
Andreas Dilger
Not yet in PS48: 68361 is still at PS17 and its stack does not apply to master. This will be done when 64086 is rebased onto the refreshed 68361.
reply Andreas Dilger lov_io.c:3862 — Not yet in PS48, for the same reason. The slot helpers and absolute-offset row/set mapping will be used once 68361 is refreshed.
Andreas Dilger
The parity offset needs to change with 68361 in two ways: 1. RAID10 layout: one stripe_size per raid set per row, and the parity index selects the object rather than the offset, i.e. poff = e_start + (row * R + set) * ss + intra, pstripe = (stripe of that slot) + pi with R = esc_n0 + esc_n1, via the 69516/68361 slot helpers (lsme_parity_slots_per_raidset()) rather than lov_stripe_number() on the concatenated offset. 2. The row/set must come from the absolute data offset, not offset - e_start (see my lov_io.c comment on 68361 PS17): row = off / (c * ss), offset_i = (off / ss) % c, and poff relative to row0 = e_start / (c * ss). Otherwise when e_start is not a multiple of the stripe width this looks at the parity of a different raid set than the one holding the data. Ideally use the shared helper that 68361 adds for this mapping so the kernel recovery, llapi resync, hedge (64919) and lseek all agree. Rebase onto that series once it is updated.
Andreas Dilger
Not yet in PS48, for the same reason. The slot helpers and absolute-offset row/set mapping will be used once 68361 is refreshed.
reply Gerrit AI review for Lustre lov_io.c:3865 — (minor) lov_parity_mirror_index_from_data() looks up the parity link from the data component at lio->lis_pos, which is the lseek start, not
Gerrit AI review for Lustre
(minor) lov_parity_mirror_index_from_data() looks up the parity link from the data component at lio->lis_pos, which is the lseek start, not from dsub's component dindex. If the seek starts in an earlier component of the mirror that has no parity link (e.g. a non-EC first PFL component), pmirror is < 0, and the stripe is reported as all data even though in-sync parity exists. The result is still correct, only less precise. If this is changed to use lsm_entries[dindex]->lsme_mirror_link_id, note that the LASSERTF in lov_io_sub_init() does the same lis_pos lookup, so it would need to stay in step for lov_sub_get() on the parity stripe.
tags: pt_ecro 218_review_priority
owner Patrick Farrell · uploader Andreas Dilger · PS48 uploaded 6d ago · NEW · open in Gerrit ↗
commit message
LU-20958 lov: handle unavailable EC stripes in lseek
SEEK_DATA/SEEK_HOLE on an EC file with a data stripe on an OST that
was deactivated would fail with -ESHUTDOWN, and with an OST that was
down, e.g. during an OSS failover, the lseek would wait for the OST to
return, although reads of the file are rebuilt from parity at once.
Do the lseek with no-delay lock enqueues, as reads do. When the
sub-lock enqueue of a stripe in an EC data component fails, mark it in
this IO's lov_io_sub::sub_ec_err and do not start its sub-IO. Retry
with blocking enqueues once every mirror has been tried. The
unavailable stripe leaves its size out of the merged stripe
attributes, so take EOF from the strict SOM size from the MDS instead,
fetched again as stat() does if the UPDATE lock was dropped. Without
one, wait for the OST, or return -EIO if it stays unavailable, rather
than miss data past a smaller EOF. A stripe whose extent lock is
still cached is locked locally, so its OST_SEEK still waits for the
OST.
Resync writes parity only where a data stripe of its raid set has
data, page aligned, and punches the rest, so a hole in an in-sync
parity stripe is a hole in every data stripe of the raid set. Seek an
available parity stripe of the unavailable stripe's raid set from the
matching offset, and map the result back, for the RAID0|PARITY parity
layout. Each row of a raid set holds one stripe_size chunk in every
data and parity object, so offsets in the two objects advance
together. The result can report data where only another stripe of the
raid set has data, but never a hole where the stripe may have data.
With no parity stripe that can answer, report the stripe as data from
where the IO starts in it, with no hole before its end, which is
always correct: a seek must not skip data, but may report a hole as
data. Since such a stripe has no hole at its end, report the hole at
EOF for SEEK_HOLE if nothing before it is found, and -ENXIO for
SEEK_DATA at EOF.
The parity object is not covered by the lseek lock, so seek it with a
server lock, and do not trust the LVB size of an unlocked object in
osc_io_lseek_start(). Seek parity without delay, failing at once if
its OST is disconnected; an idle import still reconnects for the seek.
Add sanity-ec test_79a, which stops the OST of a stripe, and
test_79b, which checks holes found from parity, and that the stripe
is reported as data when the parity OSTs are stopped too.
Test-Parameters: testlist=sanity-ec ostcount=8
Fixes: 783c13bb0b8f ("LU-12669 ec: recover data from parity")
Signed-off-by: Patrick Farrell <patrick@thelustrecollective.com>
Signed-off-by: Andreas Dilger <adilger@thelustrecollective.com>
Assisted-by: ClaudeCode:claude-opus-5-5 ltvm llm_code_and_review_tools
Change-Id: I5cce4e0ea51c68b0c6fda1d83b694af19cad57bd
67599
master
J –B ✓T ✓R -1/2−
LU-20531 mdc: do not delay MDS_CLOSE for import recovery
cc Vandana Rungta · blocked on the owner: needs rebase — checkpatch: cannot be cherry-picked
needs rebase
5 unresolved
M +110 −0
2d
LU-20531 mdc: do not delay MDS_CLOSE for import recovery
cc Vandana Rungta · blocked on the owner: needs rebase — checkpatch: cannot be cherry-picked
needs rebase
5 unresolved
M +110 −0
2d
2 optional test failure(s) — informational
build #135009 SUCCESS · tests all Maloo sessions
no vote yet: Keguang Xu, Qian Yingjin, Andreas Dilger, Alex Zhuravlev, Duncan Vogel, Timothy Day
patchsets:
PS12 08-15
PS13 08-15
PS14 10-29 patrick bot
PS15 10-30
PS16 10-31
+11 earlier
PS1 07-28 PS2 07-28 PS3 08-04 PS4 08-04 PS5 08-04 PS6 08-14 PS7 08-14 PS8 08-14 PS9 08-14 PS10 08-15 PS11 08-15threads: 5 need your reply · 0 waiting on others · 0 own notes · 0 bot
reply Gerrit AI review for Lustre COMMIT_MSG:46 — Not quite accurate: a close that overtakes its open's replay carries the old handle and always gets -ESTALE from mdt_close_internal(). That
Gerrit AI review for Lustre
Not quite accurate: a close that overtakes its open's replay carries the old handle and always gets -ESTALE from mdt_close_internal(). That is mapped to success only when the open is committed. Either way, the open is then recreated on the MDT by its replay and never closed. See the comment on mdc_request.c.
reply Gerrit AI review for Lustre COMMIT_MSG:66 — The earlier reproducer (OBD_FAIL_LDLM_REPLAY_PAUSE plus opencache on a remote directory, then checking that the MDC import reaches FULL) did
Gerrit AI review for Lustre
The earlier reproducer (OBD_FAIL_LDLM_REPLAY_PAUSE plus opencache on a remote directory, then checking that the MDC import reaches FULL) did observe the hang through a stable interface: the import state stays in REPLAY_LOCKS. Could test_171 use that, so it fails without the fix, rather than adding a test that passes either way?
reply Gerrit AI review for Lustre mdc_request.c:941 — (style) This isn't a bug, but the 27-line comment largely repeats the commit message and the ticket. If the patch is refreshed, a few lines
Gerrit AI review for Lustre
(style) This isn't a bug, but the 27-line comment largely repeats the commit message and the ticket. If the patch is refreshed, a few lines on why the close must not be delayed, with a pointer to LU-20531, would be enough.
reply Gerrit AI review for Lustre mdc_request.c:966 — (defect) Does this send the close with a stale open handle when the import is still in LUSTRE_IMP_REPLAY?
Gerrit AI review for Lustre
(defect) Does this send the close with a stale open handle when the import is still in LUSTRE_IMP_REPLAY? This flag applies to every close, not only the one issued from ldlm_cancel_unused_locks_for_replay(). Any close(2) or ll_clear_inode() during REPLAY now goes out right away, packed with the pre-failover handle. The old code parked it in imp_delayed_list. Then mdc_replay_open() rewrote epoch->mio_open_handle through mod->mod_close_req, so the close went out with the new handle once the import was FULL. mdt_close_internal() calls mdt_open_handle2mfd() with req_is_replay() false, so the old cookie is only looked up with class_handle2object(). After the MDS restarts that lookup fails, even if the open has already been replayed, because the new mfd keeps the old cookie only in mfd_open_handle_old. The result is -ESTALE: - uncommitted open (still on imp_replay_list): mdc_close() returns -ESTALE and clears mod_close_req. ptlrpc_replay_next() then replays the open anyway, because it doesn't check rq_replay. The MDT ends up holding an mfd that is never closed. - committed open whose replay is already in flight: -ESTALE is turned into 0, but the replayed open leaves the same leaked mfd. The leaked open lasts until the client disconnects. It keeps unlinked files as orphans and raises mot_open_count, which makes leases, HSM release and swap_layouts return -EBUSY. The note in this comment about -ESTALE on a committed open covers only part of this.
reply Gerrit AI review for Lustre mdc_request.c:966 — (defect) What transno does the MDT give this close if it reaches a target that is still replaying other clients' requests?
Gerrit AI review for Lustre
(defect) What transno does the MDT give this close if it reaches a target that is still replaying other clients' requests? target_queue_recovery_request() processes a request with no transno right away. If mdt_mfd_close() opens a transaction (atime/mtime update through mo_attr_set(), mdt_lsom_update(), orphan destroy in mdd_close()), tgt_last_rcvd_update() assigns ++lut_last_transno. During recovery lut_last_transno is the highest transno replayed so far, so the new number can equal one that another client still has to replay. The reply's transno is above last_committed, so ptlrpc_retain_replayable_request() puts the close on this client's imp_replay_list. While the import is in REPLAY, ptlrpc_replay_next() can then replay it in the same recovery. Two replays with the same transno hit "dropping replay: transno has been claimed by another client" in target_queue_recovery_request(). One of them gets no reply, and that client has to time out and reconnect. Version-based recovery (VBR) can also fail if the close updated the version of an object another client's replay depends on. This also applies to the REPLAY_LOCKS case this patch targets: one client can be replaying locks while the target is still replaying other clients' transnos. Until now a recovering client never sent a fresh modifying request before its import was FULL, which avoided this. The ldlm_cli_cancel_req() precedent is an LDLM_CANCEL, which creates no transaction.
owner Vandana Rungta · uploader Vandana Rungta · PS16 uploaded 3d ago · NEW · open in Gerrit ↗
commit message
LU-20531 mdc: do not delay MDS_CLOSE for import recovery
Ten ldlm_bl threads on an MDS that also carries a client mount were
reported blocked for over 122 seconds, all with the same stack:
obd_get_mod_rpc_slot <- ptlrpc_get_mod_rpc_slot <- mdc_close
<- lmv_close <- ll_close_inode_openhandle <- ll_md_real_close
<- ll_clear_inode <- ll_delete_inode <- evict <- iput
<- d_prune_aliases <- ll_prune_aliases <- ll_lock_cancel_bits
<- ll_md_blocking_ast <- ldlm_cancel_callback
<- ldlm_cli_cancel_local <- ldlm_cli_cancel_list_local
<- ldlm_bl_thread_blwi
A close can be driven from lock cancellation, and that path is also
walked inline on the import's own lock replay thread, by
ldlm_cancel_unused_locks_for_replay(). Cancelling a LOOKUP or PERM
lock runs ll_prune_aliases(), which can drop the last dentry of an
inode whose open handle is still cached by an OPEN lock, so the inode
is evicted and ll_clear_inode() has to issue MDS_CLOSE.
MDS_CLOSE keeps the default rq_send_state of LUSTRE_IMP_FULL, so
ptlrpc_import_delay_req() parks it on imp_delayed_list. That is a
deadlock: mdc_close() holds the reserved close modify-RPC slot across
ptlrpc_queue_wait(), and the import cannot leave recovery until the
same thread returns, so the close waits for a recovery that is waiting
for the close. It cannot time out either - a non-FULL import is not
pinged, ptlrpc_set_import_discon() requires FULL, and
ptlrpc_abort_inflight() skips requests of the current generation.
Every other close then blocks in obd_get_mod_rpc_slot(), which waits
uninterruptibly with no timeout, so each ldlm_bl thread that picks up
an LRU cancel batch is consumed and lock revocation on the node stops.
A target already handles MDS_CLOSE while it is recovering:
tgt_filter_recovery_request() lists it, and
target_queue_recovery_request() processes rather than queues a request
with no transno, which a fresh close has. So set rq_allow_replay on
the close, as ldlm_cli_cancel_req() already does for a cross-MDT
LDLM_CANCEL to avoid an analogous deadlock; that guard is gated on
OBD_CONNECT_MDS_MDS and so never covers a close issued through llite.
Should a close ever overtake its own open's replay, mdc_close()
already maps -ESTALE on a committed open to success.
Measured with a matched A/B on real hardware, same harness, this one
line as the only variable:
unpatched patched
o35 "req waiting for recovery" 1 0
o35 "allow during recovery" 0 26
ldlm_bl parked (state S) 727 samples 0 of 730
imp_replay_inflight stuck at 10 drained to 0
recovery outcome COMPLETE COMPLETE
This removes the demonstrated trigger, not the whole class: the
untimed wait in obd_get_mod_rpc_slot() and the inline Data-on-MDT
flush in ll_lock_cancel_bits() are unchanged and remain separate
blocking points.
The new subtest is a regression guard, not a reproducer. It passes
with and without the fix. See the ticket for why there is no
reproducer: the condition is not observable through any stable
interface, so a test would have to assert on the debug text emitted by
the two branches of the very "if" that rq_allow_replay controls.
Test-Parameters: mdscount=2 testlist=recovery-small
Signed-off-by: Vandana Rungta <vrungta@amazon.com>
Change-Id: I88beb57e6fe4ee49cc7741187ea5fc9f85117dbf
65862
master
J !B ✓T ✓R -1/2−
LU-20253 mdt: implement RENAME_WHITEOUT support
cc Timothy Day · blocked on the owner: needs rebase — checkpatch: cannot be cherry-picked
needs rebase
13 unresolved
L +451 −12
21d
LU-20253 mdt: implement RENAME_WHITEOUT support
cc Timothy Day · blocked on the owner: needs rebase — checkpatch: cannot be cherry-picked
needs rebase
13 unresolved
L +451 −12
21d
janitor run: 1 failing config(s), none unique to this patch — janitor results
6 optional test failure(s) — informational
build #128951 SUCCESS · tests all Maloo sessions
reviews:
Keguang Xu +1 · Andreas Dilger -1
no vote yet: Qian Yingjin, Lai Siyao
patchsets:
PS7 07-07
PS8 07-16
PS9 08-03
PS10 08-03
PS11 08-08
+6 earlier
PS1 05-13 PS2 06-08 PS3 06-09 PS4 06-10 PS5 06-19 PS6 06-26threads: 13 need your reply · 0 waiting on others · 0 own notes · 0 bot
reply Andreas Dilger COMMIT_MSG:22 — This still doesn't preclude using `Test-Parameters:` to run interop testing with the relevant sessions against 2.17 (though be aware there a
Gerrit AI review for Lustre
(suggestion) This changes on-wire behavior (new mds_op_bias value sent in rn_bias). Consider adding a Test-Parameters: line requesting interop coverage against an older server (e.g. serverversion=...) so the new-client/old-server path is exercised.
Andreas Dilger
Agreed. This testing can be requested on an existing patch via its "Maloo Results" page posted in a comment at the start of every test run, as long as the build is still available. Even better is to split the patch into client and server components to ensure that each half runs a full set of tests in "interop" mode with a peer that does not have the feature.
Timothy Day
I think that'd only work if the patches were not in a series. Otherwise, the second patch wouldn't get interop testing. It seems odd that we don't have one last-LTS test session on the normal patch review.
Andreas Dilger
This still doesn't preclude using `Test-Parameters:` to run interop testing with the relevant sessions against 2.17 (though be aware there are some existing interop test failures that still need to be fixed).
reply Gerrit AI review for Lustre COMMIT_MSG:7 — The component tag is `mdt:` but the bulk of the change is in lustre/mdd/ (166 of 449 added lines), with lustre/mdt/ getting 5 lines. Would `
Gerrit AI review for Lustre
The component tag is `mdt:` but the bulk of the change is in lustre/mdd/ (166 of 449 added lines), with lustre/mdt/ getting 5 lines. Would `mdd:` be a better fit, or a feature tag since this also spans llite/lod/tests?
reply Gerrit AI review for Lustre COMMIT_MSG:26 — Two behavior changes to shared code don't seem to be accounted for by the body:
Gerrit AI review for Lustre
Two behavior changes to shared code don't seem to be accounted for by the body: - `lod_fid_alloc()` gains a new `lu_object_remote(parent)` branch, changing FID placement for any caller that passes a plain remote parent, not just the whiteout. - `mdi_chlog_declared` changes from a boolean to a `BIT(CL_*)` mask, which changes the gating in `mdd_changelog_ns_store()` for every namespace operation, not only rename. Could the body call these out (and say why the changelog gating has to change), so neither hunk reads as accidental?
reply Andreas Dilger lustre_idl.h:859 — The other patches should also be landed in the 2.18.0 release.
Gerrit AI review for Lustre
(suggestion) The name and comment claim the whole renameat2() flag set, but the server side only implements RENAME_WHITEOUT; ll_rename() rejects everything else with -EINVAL. Since ll_rename() gates on `flags && !exp_connect_renameat2()`, a later client that adds RENAME_EXCHANGE or RENAME_NOREPLACE will read this bit on a 2.17-era server as "supports those too" and will have to spend another connect bit (or a version check) to tell them apart. Connect bits are scarce, and the meaning of this one is frozen once it ships. Would naming it for what it actually advertises - e.g. OBD_CONNECT2_RENAME_WHITEOUT - keep the door open, or is the intent that the follow-on patches extend the meaning of this same bit?
Andreas Dilger
The other patches should also be landed in the 2.18.0 release.
reply Andreas Dilger lustre_idl.h:859 — The OBD_CONNECT2_RENAMEAT2 reservation is now its own patch, https://review.whamcloud.com/69528 (LU-20253 idl: reserve OBD_CONNECT2_RENAMEAT
Andreas Dilger
The OBD_CONNECT2_RENAMEAT2 reservation is now its own patch, https://review.whamcloud.com/69528 (LU-20253 idl: reserve OBD_CONNECT2_RENAMEAT2 flag), in a chain of connect flag reservations in bit order so they can land ahead of the feature patches. Please rebase this patch onto 69528 and drop the flag definition, its obd_connect_names[] entry and the wiretest/wirecheck lines from here.
reply Gerrit AI review for Lustre mdd_dir.c:3721 — (minor) The return values of mdd_linkea_prepare() and mdd_links_add() are discarded. If linkea_data_new() returns -ENOMEM, `wh_ldata.ld_leh`
Gerrit AI review for Lustre
(minor) The return values of mdd_linkea_prepare() and mdd_links_add() are discarded. If linkea_data_new() returns -ENOMEM, `wh_ldata.ld_leh` stays NULL, mdd_links_write() returns 0 immediately, and the whiteout is left with no linkEA (no fid2path, and LFSCK sees it as missing). A CDEBUG/CERROR on failure would at least make that visible.
reply Andreas Dilger mdd_dir.c:1407 — (defect) This seems like a significant issue, not just a nice-to-have change.
Gerrit AI review for Lustre
Turning this into a per-type check makes the gate depend on the declared type matching the stored type exactly, and the unlink path doesn't match.
`mdd_declare_unlink()` always declares `CL_UNLINK`:
rc = mdd_declare_changelog_store(env, mdd, CL_UNLINK, name,
NULL, handle);
but `mdd_unlink()` stores `is_dir ? CL_RMDIR : CL_UNLINK`. For an rmdir only `BIT(CL_UNLINK)` is ever set, so this test fails and the record is dropped. `mdd_trans_create()` zeroes the mask per transaction, so nothing else can set `BIT(CL_RMDIR)`.
That would mean every directory removal loses its changelog entry, which changelog consumers rely on. Should `mdd_declare_unlink()` declare the same type it will store?
Andreas Dilger
(defect) This seems like a significant issue, not just a nice-to-have change.
reply Gerrit AI review for Lustre mdd_dir.c:3835 — This is reached after the cross-MDT symlink shortcut above, so `MDS_RENAME_WHITEOUT` is dropped there:
Gerrit AI review for Lustre
This is reached after the cross-MDT symlink shortcut above, so `MDS_RENAME_WHITEOUT` is dropped there:
if (mdd_object_remote(mdd_sobj) && S_ISLNK(sattr->la_mode) &&
sattr->la_nlink == 1 && !tobj) {
...
rc = mdd_migrate_object(...);
GOTO(out_pending, rc);
}
With srcdir on MDT0 and tgtdir on MDT1 the rename runs on MDT1, so a symlink in srcdir is remote and single-linked, and `renameat2(RENAME_WHITEOUT)` returns 0 with no whiteout at the source name. For overlayfs that is the silent inconsistency the connect-flag gating was added to avoid. Should the migrate path also create the whiteout, or should the whiteout flag force the non-migrate path?
reply Andreas Dilger mdd_dir.c:3679 — (minor) this should be fixed if the patch is refreshed, but I don't think it is critical since the whiteout should normally not be visible a
Gerrit AI review for Lustre
(minor) The whiteout attributes are built by hand here, so they skip the project ID inheritance that every other create gets from mdd_create_sanity_check():
if (pattr->la_flags & LUSTRE_PROJINHERIT_FL) {
cattr->la_projid = pattr->la_projid;
cattr->la_valid |= LA_PROJID;
}
With LA_PROJID absent from mw_attr.la_valid and la_projid left at 0 by the memset, a whiteout created in a `chattr +P` directory is charged to project 0 instead of the parent's project, and `lfs project -r -c` on that directory will report it as non-conforming. mdd_rename() already re-stamps the renamed object's projid a few lines further down (change_projid), so the source-parent side looks inconsistent. Should the parent's projid be copied in when spattr has LUSTRE_PROJINHERIT_FL?
Andreas Dilger
(minor) this should be fixed if the patch is refreshed, but I don't think it is critical since the whiteout should normally not be visible at all in the places where it is used? That said, it would still account whiteouts to the wrong PROJID.
reply Andreas Dilger mdd_dir.c:4127 — Seems reasonable?
Gerrit AI review for Lustre
(minor) This drops the whiteout with the nocache variant on every path, including the success path. lu_object_put_nocache() sets LU_OBJECT_HEARD_BANSHEE, so a whiteout that was just created and committed is torn down instead of staying cached - and the next thing overlayfs does after renameat2() is look that name up again, forcing a full re-instantiation. Only the paths where the object was destroyed (or never created) need nocache. Could this use mdd_object_put() when the whiteout survived, e.g. keyed off wh_created && !rc?
Andreas Dilger
Seems reasonable?
reply Gerrit AI review for Lustre replay-dual.sh:1365 — This check can't fail, so the whiteout placement isn't actually verified.
Gerrit AI review for Lustre
This check can't fail, so the whiteout placement isn't actually verified. `lfs getstripe -m` on a non-directory goes through `cb_get_mdt_index()`, which does `open(path, O_RDONLY | O_NOCTTY)` before `LL_IOC_GET_MDTIDX`. Opening a 0:0 character device returns -ENXIO (no cdev registered at major 0), so `lfs` prints to stderr and stdout is empty, leaving `mdtidx` unset. In `(( ))` an empty variable evaluates to 0, so the comparison is always true. Since this is the only coverage for the new `lod_fid_alloc()` remote-parent branch, could the index be read another way, e.g. from the FID sequence via `$LFS path2fid` on the whiteout plus `$LFS fid2path`, or by checking the parent's `getdirstripe`?
reply Andreas Dilger replay-dual.sh:1356 — (style) prefer `==` for comparison in `[[ ... ]]`
Andreas Dilger
(style) prefer `==` for comparison in `[[ ... ]]`
owner Timothy Day · uploader Timothy Day · PS11 uploaded 87d ago · NEW · open in Gerrit ↗
commit message
LU-20253 mdt: implement RENAME_WHITEOUT support We must support RENAME_WHITEOUT for overlayfs support on Lustre. Quoting from the man page [1]: This operation makes sense only for overlay/union filesystem implementations. Specifying RENAME_WHITEOUT creates a "whiteout" object at the source of the rename at the same time as performing the rename. The whole operation is atomic, so that if the rename succeeds then the whiteout will also have been created. [1] https://www.man7.org/linux/man-pages/man2/rename.2.html Implement RENAME_WHITEOUT as an optional flag to mdd_rename(). Add tests to sanity.sh and replay-dual.sh to test this support and ensure that it's crash-consistent. Add a new renameat2 helper program. Negotiate support via a new OBD_CONNECT2_RENAMEAT2 connect flag. The client only sends RENAME_WHITEOUT (as the MDS_RENAME_WHITEOUT op_bias) when the MDS advertises the flag, and fails renameat2() with -EINVAL otherwise, so new clients interoperate safely with servers that do not support renameat2() flags. Signed-off-by: Timothy Day <timday@thelustrecollective.com> Change-Id: Iefb381cb12950c8751a0ecfcdfdcf4dedaa4b033
Merged — last 30 days
(1)
66436
master
J –B ✓T ✓R +5/2
LU-12668 ec: ensure hole ranges for raidset parity
mine
ec2
pt_ecro
3 unresolved
L +636 −228
5d
LU-12668 ec: ensure hole ranges for raidset parity
mine
ec2
pt_ecro
3 unresolved
L +636 −228
5d
reviews:
Ronnie Sahlberg +1 · Marc Vef +1 · Andreas Dilger +1 · Oleg Drokin +2
patchsets:
PS13 07-08 Keguang Xu
PS14 07-08 Keguang Xu
PS17 07-20 Keguang Xu
PS18 07-26 Marc Vef
PS19 07-28 Keguang Xu
+10 earlier
PS1 06-03 PS4 06-05 PS5 06-06 PS6 06-06 PS7 06-06 PS8 06-22 Marc Vef PS9 07-03 Keguang Xu PS10 07-03 Keguang Xu PS11 07-03 Keguang Xu PS12 07-06 Keguang Xutags: pt_ecro
owner kg.xu · uploader Oleg Drokin · PS20 uploaded 69d ago · MERGED · open in Gerrit ↗
commit message
LU-12668 ec: ensure hole ranges for raidset parity Complete the block-granularity EC parity resync/verify path so partial writes and verification stay consistent with sparse parity mirrors. When a raidset has no data coverage, punch-or-zero or seek-or-zero-check that the parity mirror is holed for the entire raidset through llapi_ec_hole_range_verify_or_clear(), shared with the stripeset-level hole path and with clearer mismatch diagnostics. On osd-zfs, punch falls back to zero-fill via llapi_mirror_punch_or_clear(); verify uses llapi_ec_verify_hole_by_seek_or_zero_content() when SEEK_DATA may report zero-filled extents instead of a true hole. For raidsets that contain data, in the resync path, clear the full parity raidset once (punch-or-zero) and then write the @cov ranges. Previously llapi_ec_write_parities() only wrote @cov and did no clearing, so stale parity could remain outside those intervals. Clearing the raidset once up front removes that stale parity and keeps the resync path simpler. In the verify path, @cov is irrelevant: the whole stripe must be compared, covering both data-backed ranges and zero-filled ranges, otherwise inconsistency would be missed. Right after @cov is created, expand and merge coverage intervals to page boundaries via ec_cov_page_align_and_merge(), since files in resync operation is opened with O_DIRECT. Add sanity-ec/12d,12e,12f. Test-Parameters: testlist=sanity-ec Test-Parameters: testlist=sanity-ec fstype=zfs Signed-off-by: Keguang Xu <kxu@ddn.com> Change-Id: I3f68bab823277679a3e1804bbd2262ce7b6904a8 Reviewed-on: https://review.whamcloud.com/c/fs/lustre-release/+/66436 Tested-by: jenkins <devops@whamcloud.com> Reviewed-by: Ronnie Sahlberg <rsahlberg@whamcloud.com> Reviewed-by: Andreas Dilger <adilger@thelustrecollective.com> Reviewed-by: Oleg Drokin <green@whamcloud.com> Tested-by: Maloo <maloo@whamcloud.com> Reviewed-by: Marc Vef <mvef@whamcloud.com>