(minor) There is no `.TP` before this entry, so `--compress-type|--compr-type` and its description get folded into the `-z, --extension-size` paragraph above instead of starting their own tagged item. The two entries below it do have theirs.
weird but in my man output it looks ok.
The entry was missing its .TP in the troff source. Some man implementations are lenient and reflow correctly, but others fold the entry into the previous paragraph. Added .TP for correctness.
Done. Added the missing .TP before the --compress-type|--compr-type entry.
Should be fixed if patch is refreshed.
LU-10026 csdc: set compress component for file
* 'lfs setstripe' to support compress component
--compress|-Z <type>[:<level>]
Set component compression algorithm <type> and compress <level>;
--compress-chunk=<size>
Set compress data chunk size in KiB used by the compression
algorithm, the value will be adjusted to power-of-two multiples of
the base 64KiB.
Example:
$ lfs setstripe -Eeof -Z lz4:5 --compress-chunk=512 <file>
* 'lfs getstripe' to show compress component parameters
Display component's compression parameters if possible.
--compress-type|--compr-type
Print only the compress type if possible.
--compress-level|--compr-level
Print only the compress level if possible.
--compress-chunk|--compr-chunk
Print only the compress chunk size in KiB if possible.
Example:
$ lfs getstripe <file>
lcme_compr_type: lz4
lcme_compr_lvl: 5
lcme_compr_chunk_kb: 512
lmm_pattern: raid0,compress
Signed-off-by: Bobi Jam <bobijam@whamcloud.com>
Signed-off-by: Artem Blagodarenko <ablagodarenko@ddn.com>
Signed-off-by: Marc Vef <mvef@whamcloud.com>
Change-Id: Ife0382469cbc5099e0c6dc96534bb169ddeff61e
LU-19833 acl: fix generation of changelogs with ACLs
Adding, modifying, or removing ACLs on files did not generate a
changelog. Only changes to default ACLs were generating a changelog.
Now a changelog is always generated.
Fixes: 7b3bfb09dbcc ("LU-1304 mdd: changes related to acl")
Signed-off-by: Gauthier EVRAERD <Gauthier.EVRAERD@cea.fr>
Change-Id: Ice29e0d8b00f41abe94135e223d1b6fcb314ba91
(minor) the date should be updated
LU-10026 utils: add 'lfs find' support for compressed file * Add "--comp-flags=[^]compress" to locate file with/without compressed components. * Add "--comp-flags=[^]nocompr" to locate file with/without setting component compress preference. * Add "[!] --layout=compress" to locate file with/without compressed components. * Add "[!] --compress-type=<compress-type>" to locate compressed file with/without specified compress algorithm. * Add "[!] --compress-level=[+-]<compress-level>" to locate compressed file with/without specified compress level. * Add "[!] --compress-chunk=[+-]<compress-chunk>" to locate compressed file with/without specified compress chunk in KiB. utils: add -Z option for 'lfs getstripe/find' Add support for "lfs getstripe -Z" to get the last instantiated component compression information. Add support for "lfs find -Z <type>[:[+-]<level>]" to keep consistent options with "lfs setstripe -Z". DDN-bug-id: EX-6856 Was-Change-Id: Ia5e2a2ea6937dc2b46e224cec808504a196b974c utils: logical AND for 'lfs find' compression exprs All search expressions provided to 'lfs find' must be combined as a logical AND. Fix newly added options for compression support, so that they comply with this logical AND. DDN-bug-id: EX-7714 Was-Change-Id: I3b28cd87c1d304df6d04753b413d46f5abcfe16e csdc: don't set compression layout when disabled When llite_enable_compression is disabled (lfs set_param llite.*.enable_compression=0), we should check it before sending it to MDS lest we get a file with compressed component which we cannot handle. DDN-bug-id: EX-7593 Was-Change-Id: Ib1e2123ffdb239c3e1401d682ae9c2c49e3f4a6f utils: support 'lfs find --printf %LZ' Add support for "lfs find --printf %LZ" to print the compression type:level of the last instantiated component of a file. DDN-bug-id: EX-6856 Was-Change-Id: Iaf1b6c031b06c70e7b5be51354697aa6bdcc9850 csdc: right error code in case compression is disabled Currently, if compression is disabled on a client the error message is confusing "Cannot set layout EA: Unknown error 524" Let's replace the error code from ENOTSUPP to EOPNOTSUPP, so error messages become more informative ctl get_param -n llite.*.enable_compression 0 lfs setstripe -i 0 -c 1 -E -1 -Z lz4 /mnt/lustre/foo.txt Cannot set layout EA: Operation not supported lfs setstripe: cannot create composite file '/mnt/lustre/foo.txt': Inappropriate ioctl for device This message could be even more informative, but this version is already much better. DDN-bug-id: EX-10577 Was-Change-Id: I7fd67d5d3d20bc7de169f35f6fa63b53f2bfe718 Signed-off-by: Bobi Jam <bobijam@whamcloud.com> Signed-off-by: Artem Blagodarenko <ablagodarenko@ddn.com> Signed-off-by: Marc Vef <mvef@whamcloud.com> Change-Id: Ic36946738c3463fd862aeca4ee2e2c2ed85eff84
(style) line length of 83 exceeds 80 columns
(style) "contains" -> "has"
LU-10026 csdc: prefer uncompressed mirror for read When accessing a mirrored file with both compressed and uncompressed components, choose uncompressed components for read. csdc: prefer uncompressed mirror for write When writing to mirrored files with both compressed and uncompressed mirrors, prefer the uncompressed components to write, and that is better for performance, more compatible with older clients, and better fits the model of compressing files after initial write. DDN-bug-id: EX-6510 Was-Change-Id: I62a117d5cc3d34e2c0c96d1a9ade8eef0a2d1291 Signed-off-by: Bobi Jam <bobijam@whamcloud.com> Signed-off-by: Artem Blagodarenko <ablagodarenko@gmail.com> Signed-off-by: Marc Vef <mvef@whamcloud.com> Change-Id: I043b27bd891c039901075a08c76630f8f0f9f182
Does this now match where these fields appear when the file is auto-generated?
Is there a reason this was moved? It just seems like needless churn.
LU-10026 csdc: add ll_compr_hdr structure It is an important structure for CSDC feature. Will be used in the next patches. Also adds LL_COMPR_TYPE_UNCHANGED enum value (used to signal that the compression type should not be changed on update) and the LL_LZ4FAST_DEF_LEVEL define (default level for the LZ4FAST algorithm). Signed-off-by: Artem Blagodarenko <ablagodarenko@ddn.com> Signed-off-by: Marc Vef <mvef@whamcloud.com> Change-Id: I26f2fc7c0212f1a4cad36805c75aa766d6aed548
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-ldiskfs-dne-arm | RHEL 8.10 / x86_64, Rocky 9.5 / aarch64 | ran 5 tests. 1 tests failed: sanity. | session |
The `.YS` only goes at the end of the SYNOPSIS section.
LU-18222 man: lctl lqa man page Add man pages for LQA feature. Signed-off-by: Sergey Cheremencev <scherementsev@ddn.com> Signed-off-by: Artem Blagodarenko <ablagodarenko@thelustrecollective.com> Test-Parameters: trivial Change-Id: I11360fd2ea58a311b92c651057ee415d3c99e4f3
| unique failing test | history |
|---|---|
| sanity1@zfs:test_56ab | seen in 21 other reviews |
(defect) The message describes a "sec: page_pools shrinker fix" (element_size / ppp_idle_idx / IDLE_IDX_MAX changes) and adds sanity-compr test_2000 gated on client 2.17.53 "for compression OOM fix", but there is no page_pools.c change anywhere in the diff, so test_2000 can't pass on this commit. A few hunks also aren't accounted for by the message: the ll_lov_getstripe_ea_info() switch to md_getattr() when filename is NULL in llite/file.c, and the LINVRNT->LASSERT change in cl_io_rw_init(). Are these meant to be part of this patch? Also, since this touches the OST_WRITE/OST_READ wire path (OBD_BRW_COMPRESSED, o_size/OBD_MD_FLSIZE), a Test-Parameters: line requesting client/server interop testing would be worthwhile.
(defect) this needs to add: ``` Test-Parameters: testlist=sanity-compr ``` otherwise that test session is not being run on master.
(defect) 0x418 is already used by OBD_FAIL_OSC_FIEMAP two lines above, so this fault injection point collides with it (triggering one fires the other). 0x41a or 0x420 appear free.
(defect) On the decompress path *type is llch_compr_type read from storage/the wire, so a corrupt header carrying LL_COMPR_TYPE_BEST or LL_COMPR_TYPE_FAST would LBUG the client here. The sibling change that replaced an assert with EIO ("we shouldn't assert on values read from storage") applies - return -EIO instead of asserting?
(minor) If fill_cpga() fails here, `dst` (obtained just above from obd_pool_get_objects) has not yet been stored in bp_cmp_chunk, and the out: path only releases `src`, so the chunk buffer looks leaked on this error return.
(defect) compressed_pages comes from llch.llch_compr_size, which is read straight out of the server reply by is_chunk_start() (that only checks llch_magic). It is bounded above only by page_count - i, not by pages_per_chunk. merge_chunk() then copies compressed_pages * PAGE_SIZE bytes into `src`, a single chunk_size buffer from obd_pool_get_objects(&src, buf_bits). A corrupt or hostile compr_size larger than one chunk overflows `src`. The LASSERT(src_size <= chunk_size) on the next line runs only after the copy (and asserting on wire data is itself unsafe). Can compressed_pages be bounded against pages_per_chunk before merging, returning -EUCLEAN otherwise?
(defect) On a resend, osc_brw_redo_request() re-enters this function with aa->aa_ppga. For a compressed write that array is the cpga built by compress_request()/fill_cpga() - bare brw_pages allocated with OBD_ALLOC_PTR and freed with a plain OBD_FREE, not embedded in an osc_async_page. brw_page2oap(pga[0]) then points before the allocation, so oap2cl_page() and the following clpage->cp_inode / cp_type / cp_compr_type reads touch invalid memory. -EINPROGRESS/recoverable resends are common, so this looks like a client crash on any compressed-write resend. Should the resend derive inode/compressed from aa_ncppga instead?
(style) This loop is indented with spaces instead of tabs.
LU-10026 osc: osc brw request compression This patch adds client-side compression/decompression. The client-side data compression project (CSDC) reduces storage and network utilization by leveraging the more plentiful memory and CPU resources on the local client. Data is sent compressed over the network, saved directly to storage on the server side, and decompressed back on the client side. Uncompressed data is kept in client page cache, all while being functionally transparent to the end user and application. As an example, a test file is compressed and decompressed. The resulting file is compared with the original one. The test case shows 2.5x compression ratio: 356K /mnt/lustre/d460.sanity/sanity.sh 884K /tmp/cmp-46ofie/decompressed_sanity.sh Compression should read whole chunk even if offset and size differ. Let's modify readahead to force reading data from the offset and size multiple to the chunk size. utils: fix cp_comp_type size cp_comp_type should be 8 bits, as llch_compr_type and all associated variables are declared as u8. So remove useless cp_comp_enabled and fix code to test for compressed component with cp_comp_type against LL_COMPR_TYPE_NONE. And update LL_COMPR_TYPE_MAX value to 255 to avoid conflicts with future compression types. DDN-bug-id: EX-7775 Was-Change-Id: Ia15868ac0ac003b62942540a57f782226ae8c141 tests: test compression without bzip2/HDF5 Run as much of sanity test_460a compression tests as possible, even if bunzip2 or HDF5 file are unavailable. Print a clear message in test_84 if bunzip2 unavailable. DDN-bug-id: EX-6127 Was-Change-Id: I36251834f636600eb9b0194ccd14c8b203da32e5 lov: refactor lov_io_lsme_at lov_io_lsme_at needs some minor changes to be called from lov_io_slice_init(). DDN-bug-id: EX-7601 Was-Change-Id: I0611d66052e22d349932eb26257369e07b9b8167 osc: don't check for start inside the chunk Chunk size is the same for the whole request and every chunk offset is multiple to a chunk size. No need to search for compression header in every page. It is enough to check every with offset multiple to a chunk size. DDN-bug-id: EX-7818 Was-Change-Id: Ie2ef645130656279e152ea1f7e6db01cb33836ca osc: minor compression cleanups This cleans up some style and argument issues I found made the code a little harder to follow. DDN-bug-id: EX-8270 Was-Change-Id: Ia3492ae79acf6c83d724cc91b0201c7872325853 osc: move common CSDC code to the library CSDC repacks a chunk on the server side in case of the partial rewrite. There are routines that can be shared between client and server. This patch moves common compression code to the libcfs. DDN-bug-id: EX-7601 Was-Change-Id: I824211a3435b0479f7a3b8f08598a5b567b67d3c osc: use correct count Using the number of bytes in the compressed page creates gaps in the RDMA, which IB memory registration cannot accept. Fix this by always setting count to PAGE_SIZE for compressed pages and otherwise using the count from the original source page. Setting PAGE_SIZE for compressed pages is valid because client only does compression for aligned IO, except for the trailing chunk. For the trailing chunk, the file size is set on the server, so any trailing bytes are ignored. DDN-bug-id: EX-8245 Was-Change-Id: Ied89d3ac328fb6020079392f5a8812ad5637b4a4 csdc: remove holes from struct ll_compr_hdr This patch reorganizes struct ll_compr_hdr to remove alignment holes. DDN-bug-id: EX-8353 Was-Change-Id: I59800b00e3a17972d621bae21ba06509a39b1036 osc: apply compressed flag to dst page The existing code to apply brw flags to compressed pages has two issues: 1. The dst_page is NOT an osc async page, it is a bare BRW page. This means the brw_page2oap macro isn't right, because there is no oap page. Because oap_brw_flags is actually oap_brw_page.flag, we don't ever access the memory pointed at by OAP, just use it to find an offset back in to the brw page. This means the flags are set correctly, but we still shouldn't use this macro. 2. However, the function then overwrites these flags by copying from a page in the source, so OBD_BRW_COMPRESSED is lost. Add OBD_BRW_COMPRESSED when we set flags. This ensures the flag is actually sent to the server on compressed IO. This was not causing any problems because the server does not actually use the OBD_BRW_COMPRESSED flag yet. (EX-7601 uses this flag) DDN-bug-id: EX-7601 Was-Change-Id: Ia94cdc803868ce16a0b66fd58578ec8b2d00cbae osc: remove unused 'wrkmem' compress_chunk() takes a wrkmem buffer, which it does not use. Remove this and its allocation in compress_request. DDN-bug-id: EX-7601 Was-Change-Id: I6f236f018f5b79c57cc8725ca0f95125810a4064 osc: walk chunk unaligned RPC correctly For decompression, the client must start looking for compressed chunks at a chunk aligned offset. Implement this in decompress_request. DDN-bug-id: EX-7601 Was-Change-Id: I3273135990ddf51e8b3c651734e19350e91f659c ofd: add obd level compression lib Some compression functions will be used by several areas of of Lustre, so they need to be in obdclass. This moves merge_chunk and unmerge_chunk there and adds the ability for them to merge lnbs. This is used in a future patch. DDN-bug-id: EX-7601 Was-Change-Id: If4a318119bb7685e41adb9f3b31a66074031e6ac osc: remove cpga fill bits cpga fill bits are not needed now that we don't support compression and encryption. DDN-bug-id: EX-7601 Was-Change-Id: I13c2278e085e9b288bd896585947e28e2ea505ca osc: only set compressed flag on compressed pages The code accidentally sets the compressed flag on all pages processed through fill_cpga, even if they're not compressed. Oops. Also stop setting pg->index on the pages in the compressed pga, this is only used by encryption and that's no longer supported with compression. DDN-bug-id: EX-7601 Was-Change-Id: I313fd943a18b71cd52493852a6884f30d187e52f osc: use pages_left in unmerge_chunk Since we have compressed chunks < chunk_size (if they're after EOF), we must use pages_left in unmgerge_chunk or it will go off the end of the page array. This also lets us remove the workaround where unmerge_chunk would skip pages that were not present. unmerge_chunk always works with a known and complete set of pages, so this check is unneeded. We should also check that our count of bytes is correct when we finish. DDN-bug-id: EX-7600 Was-Change-Id: I88896307990ff839514e54e9a7e18390a457e5d8 osc: rename 'done' Rename the ambiguous 'done' and remove it where not used. DDN-bug-id: EX-7601 Was-Change-Id: I8fb88b7a91fcc7dbd5ce2d29a61c18330fc0cda3 osc: cleanup compression variables Make usage of the compression variables more readable. DDN-bug-id: EX-7601 Was-Change-Id: I6daff56b56877c8f36e02303cc0579ba7faa731b ofd: make compress_chunk take chunk_bits Chunk bits is used everywhere, have compress_chunk convert to log bits rather than have the callers do it. DDN-bug-id: EX-7601 Was-Change-Id: Ic01bb749425cb95d9c5717965d692a18138ceeb7 obd: move module load to function This is a trivial code change to make alloc_compr a bit shorter. DDN-bug-id: EX-7601 Was-Change-Id: I0a790afe7afebde1d223420d9a578529da6ff7e5 osc: variable cleanup in decompress_req Use type and lvl variables in decompress_request. Remove an unused variable and an assert which can never fire. DDN-bug-id: EX-7601 Was-Change-Id: Ieff57411a2a41215fd368d731614801bd0f43e38 osc: replace assert with error We shouldn't assert on values read from storage, instead if they are incorrect, we should give EIO. DDN-bug-id: EX-7601 Was-Change-Id: Icda213e3c5a90a848c9b008788e92ee49e2efcb1 osc: rearrange compress_request A trivial rearrangement of compress_request to make it more readable before redoing the core logic. DDN-bug-id: EX-7601 Was-Change-Id: I1d34cd2a2a6d84bc30cc7dae8eb07586c4837f7d osc: give compress_request explicit success Compress_request has explicit failure handling, but the success handling just follows the failure handling. This is confusing - on failure, we do: page_count = *pcount then immediately do: *pcount = page_count It also sets *orig_pga = pga on success OR failure, which is wrong because compress_request may have modified pga and then failed. DDN-bug-id: EX-7601 Was-Change-Id: I121ec71cfe35babc4a572951e93f7581887ade80 osc: remove &pga usage in compress_request The usage of 'pga' and '&pga' in compress_request is confusing, but also, compress_request modifies &pga by allocating a new compressed page array. Except if we fail in compress_request, we free that new page array. This means failing in compress_request replaces 'pga' with a pointer to freed memory. Instead, create an explicit cpga pointer in the caller and use that. This allows compress_request to fail safely. DDN-bug-id: EX-7601 Was-Change-Id: Idaf592103c57b0e9ce76ab520a69b819d4f37be9 obd: move type switching to alloc_compr callers The code is much cleaner if we can eliminated applied type and handle that issue once per compression or decompression rather than for every chunk. This requires moving the type switching inside alloc_compr. (Also improve some error messages - alloc_compr can fail with ENOMEM as well.) The compression code currently allocates a transform for every chunk on the client. This is relatively cheap, but it also complicates the code by repeatedly checking if a particular compression type is supported (this is the "applied type" code). Moving alloc_compr to compress/decompress request makes the code much simpler. DDN-bug-id: EX-7601 Was-Change-Id: I162e81577db721a9715d57b3f262fcabbcbf308a osc: rename pages_in_chunk Chunks can have variable numbers of pages in them. DDN-bug-id: EX-7601 Was-Change-Id: If199d777367569e62c21305f6e4b9f3e4cce6d06 osc: allow multiple chunks in read It's rare, but reads can sometimes have multiple discontiguous chunks. Update decompress_request to handle this case. DDN-bug-id: EX-7601 Was-Change-Id: I880af95db285dce76db3610e8140a0f54baa401b ofd: do not overwrite rc in unmerge_chunk unmerge_chunk should not be responsible for setting the lnb rc, because this overwrites the result of any previous activity on the lnb. Plus, unmerge_chunk can't fail. DDN-bug-id: EX-7601 Was-Change-Id: Id1ce590c7f1da3ab7faddbd685d264a33c08d639 osc: calculate compressed size reduction accurately Compression reduces space used if it results in allocating at least one fewer block on disk. Modify the checks in compress_chunk to reflect this, rather than using the simpler "reduce size by at least 4K" calculation. Also do not attempt to compress chunks if they are less than 4K in size, since they can't possibly get a space benefit. This improved my measured ratio on a version of the Linux kernel source data set from 1.24 to 1.56, so this is significant for datasets with many small files. (This version of the source had large incompressible files removed, to focus on smaller files. The unmodified data set would not improve as much.) Note this is still short of our estimates, so either the estimate or Lustre still needs adjustment. TBD. DDN-bug-id: EX-7601 Was-Change-Id: I815706914b88de4f532a674d773769aa3a64d218 osc: rewrite compress_request The existing version of compress_request can't handle discontiguous RPCs. Rewrite the logic to handle this case properly. This also implements kms handling. If a write chunks ends at the known minimum size, we know this write is after all other data in the file and so there is no compressed data under it. This means we can compress this chunk. DDN-bug-id: EX-7601 Was-Change-Id: I8a912d9e279d04c8ff07de39e63a1ec9b490d921 osc: handle partial chunks in decompress_request Now that we have compression for incomplete chunks at the end of files, decompress_request needs to handle these chunks. This patch modifies it to understand compressed chunks which are less than chunk_size pages. DDN-bug-id: EX-7601 Was-Change-Id: I877550fa0d418def406e0308392a5336ec9f3ab6 osc: add check to decompress_request decompress_request should check to see if there's room in the RPC for the decompressed data, since this can occur if there's a bug or data corruption, and otherwise we will go past the end of the RPC during decompression. DDN-bug-id: EX-7601 Was-Change-Id: Ib1bf19bf39701b72f0f5a61b2aaff2f2fdad1897 osc: debug fix in decompress_request Debug message had an incorrect subtraction. DDN-bug-id: EX-7601 Was-Change-Id: I5daf360766ca77b98dc5af3d72c42ac38f5782bc obd: add 'lvl' for best and fast 'best' and 'fast' compression types must also set a level, because not all levels are supported by all algorithms. Rather than trying to be clever, just use simple universally supported values, except for lz4fast, where we special case this, because otherwise '0' is the slowest setting (and lz4fast is likely to remain our default fastest). DDN-bug-id: EX-6269 Was-Change-Id: I7c29659d4f027af2e44285ae38e4c9d91e35509a osc: decompress with algorithm from server Data may not be compressed with the compression type and level from the layout, so we must use the compression type and level from storage for decompression. DDN-bug-id: EX-6269 Was-Change-Id: Ib4cdccf294ef631a25147413d7f5c1a847c9504e osc: handle different compression types Allow the client to handle different compression types in a single component. This shouldn't happen normally, but it may happen in the future if there is dynamic compression algorithm selection for "fast" or "best" types (e.g. compress based on available CPU and network bandwidth or RPC backlog). DDN-bug-id: EX-6269 Was-Change-Id: Ide2731c60a68584e7cbb474bee88a17e9a7b8fec osc: add COMPR_GAP check to compress_request Currently, compress_request will build the compression buffer (calling merge_chunk()) for requests which are less than the minimum compression gap. This is noticed in the compression code when it checks if there's enough data to attempt compression, but we can do a trivial check in compress_request() to save that work. Also fix a few minor style things. This is not an important fix, but I discovered it while investigating another issue and it's trivial to resolve. DDN-bug-id: EX-7601 Was-Change-Id: Ieb32e6297e10d229f23c58e2ef4d933ce3dda4f2 lustre: add uncompressed size to compression header It's useful to have the uncompressed size of the data in the compression header. Also, we have three checksum fields - compressed, uncompressed, and header, but in practice, checksumming the compressed data including the header is enough to cover all of these. This patch cleans up all of this at the same time. DDN-bug-id: EX-8851 Was-Change-Id: Ie82e0dbe9c862ddc88999b109cea1f27577dbbff csdc: rename "cp_comp_*" to "cp_compr_*" This patch renames "cp_comp_type", "cp_comp_level", and "cp_chunk_log_bits" to use "compr" in the name to be consistent with other variable names. DDN-bug-id: EX-8353 Was-Change-Id: I428ff3a789b33da02832dee02f316b02d97137e2 osc: Do not iterate over chunk pages If osc_decompress() knows chunk size, no need to iterate other all pages in chunk. Thay can be skipped. DDN-bug-id: EX-7729 Was-Change-Id: Ib84b060075c55c97eba9f74ef017c0a956e85b12 csdc: Fix the upper mergeable chunk pointer If the full chunk is followed by un mergeable page, the upper mergeable chunk pointer is occasionally set to this unmanageable page. The chunk size is calculated wrongly then and the next condition suggest not to compress this chunk, because its size is not equal to the expected size. The pointer should be moved to the first instruction after the can_merge_pages(). DDN-bug-id: EX-9192 Was-Change-Id: I09fedc770c8bbcac4864b32372a941da5e0c7ac3 obdclass: reset bits after decompression as uncompressed data can be less than chunk/page, but still be visiable to userspace as a part of a sparse file. DDN-bug-id: EX-9873 Was-Change-Id: I4114b0704fb685013f4e03cf2d80ccde2cc8c87f osc: preserve compressed pages for OST_WRITE replay it's incorrect to release compressed pages right after reply as we may resend them during OST_WRITE replay. DDN-bug-id: EX-9895 Was-Change-Id: I3edc16d6556ddd60735d2f14fe879fc0f45231d7 csdc: Fix the next available algorithm selection Currently, if a chosen algorithm is not available, then next preferable is chosen, but an error code is not cleared so __alloc_compr() returns this wrong code. Data if written uncompressed while it can be compressed with next appropriate algorithm. This patch adds this error code clearing. Test is provided. DDN-bug-id: EX-9889 Was-Change-Id: I59f65058a0fe9b108de3d4ba7cf5950f18e32204 ptlrpc: reset refcount reusing pages for new req when a set of compressed pages are reused for a redo RPC, we have to reset refcounter. DDN-bug-id: EX-10061 Was-Change-Id: I16c46d857d60cf4dd0f6317190e7c99b2667e135 ptlrpc: drop extra reference to compressed pages if the request is not replayable, so brw_commit() won't be called. DDN-bug-id: EX-10184 Was-Change-Id: Ib29ab19b69fd15ef339cc18c5dfef17217a32cb1 osc: remove noisy warning if we can't compress data on the client side for a reason (e.g. too random), we just send data uncompressed, makes sense to hide the error message confusing customers: kernel: LustreError: 936336:0:(lustre_compr.c:345:compress_chunk()) exafs-OST0007-osc-ff49193c293f3800: Compression failed, type 5, lvl 5, -22 DDN-bug-id: EX-10593 Was-Change-Id: Icbc8be38b0372f8f67690e7d94384a16ab02cbe4 osc: fix osc_compress ENOMEM handling Send already compressed chunks in compress_request even if can't allocate memory for the next chunks. This patch also fixes statistic in case of ENOMEM. Without that write_chunks_incompressible hasn't been incremented in case of ENOMEM. Add sanity-compr_2002 to check write_chunks_compressible and write_chunks_incompressible in case of ENOMEM DDN-bug-id: EX-11035 Was-Change-Id: I3fc58ece6cf22582166ea77e4a217bc7a9efd77d csdc: remove buffer double sizes Pools now (POOL_ADD_PAGE is defined) allocate additional page for memory misuse debug, so no need to allocate double-sized buffers to prevent lz4 decompress problem. DDN-bug-id: EX-8276 Was-Change-Id: Iea04943ae0f95c8983c5ba7ec9a2c63003afe351 utils: fix LL_COMPR_TYPE wire checks Fix LL_COMPR_TYPE* wire checks to avoid duplication. The value of LL_COMPR_TYPE_UNCHANGED is also declared as 255 so that is does not conflict with other potential real compression types in the future. DDN-bug-id: EX-7775 Was-Change-Id: Iab9830f09f0778e1e1f3b1ea4c9878ce1017de8d sec: page_pools shrinker fix page_pool shrinker should consider ppp_free_pages as a number of memory regions with the size of 2^pool_order pages. Otherwhise it startis shrinking only when becomes too large, for example 2GB instead of 64MB. It often might be the reason of OOM especially when compressed files with different chunk-sizes have been written. In a such case several pools created for different chunk sizes might hold gygabytes of memory without any chance to release it. Fix element_size to return correct element size instead of number of pages. Because of that elements in page_pools had below sizes: order 0 = 4096 order 1 = 2 order 2 = 4 .... order 17 = 131072 This made possible to have elements with sizes 2,4,8,... bytes that have been allocated with OBD_VMALLOC. Now all sizes are miltiple of PAGE_SIZE: order 0 = 4096 order 1 = 8192 ... order 5 = 131072 Change the logic around ppp_idle_idx. 1. Recalculate ppp_idle_idx in __sptlrpc_pool_put_pages the same way as it done in __sptlrpc_pool_get_pages. It is possible that ppp_idle_idx might become 0 or very small (less than 10). If there is no new allocations it would be impossible to free anything from the pool during 40 seconds(CACHE_QUIESCENT_PERIOD) despite the large amount of free elements ready to shrinking. 2. In pool_shrink_count when ppp_idle_idx == IDLE_IDX_MAX, it means no one accessed pool for CACHE_QUIESCENT_PERIOD (40 seconds), i.e. we may release as much as possible pages. Earlier in a such case pool_shrink_count always returned 0. Another words it was impossible to shrink anything from the pool if there was no allocations for 40s. Add sanity-compr 2000 that reproduces the OOM problem on a client without this patch. DDN-bug-id: EX-10094 Was-Change-Id: I6b55ba67b0d21cdffdb57034e8e66063745f796e osc: Add BRW_COMPRESSED flag to reads We need to add the BRW_COMPRESSED flag to reads so servers can know if the client is able to decompress data. This lets servers decide if a client can be sent compressed data and the result won't be nonsense/corruption. This is important for future support of GPU direct, where the server will need to do the decompression. DDN-bug-id: EX-6269 Was-Change-Id: I36b5b73f983ce8f2e5297c3e9dc778a5eca54e6a osc: don't discard decompress_request error The error handling for decompress_request is unusual - non-zero returns are just discarded. And rc2 is just discarded. The read() doesn't fail or get a short read. Fix this so if decompression fails with an error. DDN-bug-id: EX-7807 Was-Change-Id: Idd01947c7375c9586a64f064dd6ee0ac2308ea86 osc: save compressed object size CSDC uses a sparse file feature. A client writes compressed data chunks to the original offsets so the same data is expected to be read from the same offsets. There are no writes after the last compressed chunk, so no "hole" after the last compressed chunk. Compressed file size (based on OST objects size) is smaller than the original on the "original last chunk size - compressed last chunk size" delta. Object size should be set to uncompressed size. This size is used to calculate file size and to remove the workaround of not compressing the last chunk in the file. DDN-bug-id: EX-7600 Was-Change-Id: I387c282e1cf788c3b8f6230ef555d73ffffe49c1 osd: save compressed object size on zfs "osc: save compressed object size" added means to transfer object size to the osd and added ldiskfs support. This patch adds saving objec size to the ZFS backend. Currently this fix submitted as separete patch, for testing purpouse, but can be marged to the main patch latter. DDN-bug-id: EX-7600 Was-Change-Id: I99e29e3f756a070b5f3cece12c4ca58f668a2ecf ldiskfs: fix detection of compressed extent The code in ldiskfs_map_inode_pages which detects a compressed extent checks the first lnb for that extent, but it's possible for some lnbs and not others to be compressed in a given extent, so we must check all of them. This occurs when multiple writes have been combined in to one RPC. If we don't detect compression correctly, we won't set the file size correctly and we'll get data corruption. DDN-bug-id: EX-7601 Was-Change-Id: I11d50bdc45c40d93bb1b829fcd930165b7626432 llite: DIO fallback on compressed files Fully supporting direct I/O on compressed files is tricky because we cannot pull the full chunk in to the page cache (because there is no page cache for DIO). So instead we fall back to buffered I/O for DIO on compressed files. This patch adds the check and a test for this. llite: allow aligned DIO with compression DDN-bug-id: EX-6127 DDN-bug-id: EX-7601 Was-Change-Id: I8224ef9b8ad1d912d8a11eccad37d3dff8dd8498 If a DIO is fully aligned to compression chunk boundaries, it is safe to do on a compressed file, so allow it. Was-Change-Id: If8fa3397c9424254538738f4d77f9f50d1c21129 lov: set IO property check in lov_io_init_composite Move compression IO check down to lov_io_init_composite(). DDN-bug-id: EX-9853 Was-Change-Id: I6d4ce3388d8eef788d31dabf4b3a5dd5eff5091f llite: DIO fallback on compressed files Direct I/O on compressed files is still not perfect for now, so we'd fall back to buffered I/O for DIO on compressed files. DDN-bug-id: EX-6266 Was-Change-Id: I2255638e4fbde766abf061989d07b37ad9f369a1 llite: Compute compression chunk ranges Determine the edges of any leading and trailing compression chunks touched by this IO and store them in the cl_io struct. The functionality in this patch also allows us to adjust the lock and read rounding to do them more intelligently, this will be done in a future patch. DDN-bug-id: EX-7601 Was-Change-Id: I526563ea347fb0246f97f3532b823c4345c3fa27 llite: round LDLM lock requests to chunk When we do IO with compression, we may need to 'fill' the compression chunk, reading up pages which have already been written to storage, so we can compress the whole chunk. Doing this safely requires that any dlmlock we're using always covers the full chunk. The easiest way to do this is to round the entire locking process to include leading or trailing compression chunks. DDN-bug-id: EX-7601 Was-Change-Id: I3c365844561d0da909e6290f4b58ef2211c2d255 csdc: is_chunk_start should return header copy In is_chunk_start() *ret_header = header; ... kunmap_atomic(header); ret_header is used after is_chunk_start(). The header copy should be returned from is_chunk_start() for safe work. DDN-bug-id: EX-9878 Was-Change-Id: Ib5e828d6b61e90dcd70c28589931a4490cf19c22 Signed-off-by: Artem Blagodarenko <ablagodarenko@ddn.com> Signed-off-by: Marc Vef <mvef@whamcloud.com> Change-Id: I9b41ab815db3df9ad7bdea5fca4c093cbda8814b
| unique failing test | history |
|---|---|
| sanity1@zfs:test_56ab | seen in 22 other reviews |
(minor) These CDEBUG() pairs print start_idx/end_idx before and after the rounding with no descriptive text, on the D_SEC (security) mask which is unrelated to readahead. Same at lines 813 and 816. They look like leftover debugging scaffolding; if kept, D_READA would be the right mask.
(defect) vui_ra_pages is initialized once per read syscall in vvp_io_read_start() and only reset at IO teardown, but ll_readahead() runs once per page (ll_readpage()->ll_io_read_page()->ll_readahead()). Won't this += accumulate across every not-uptodate page of a compressed read, progressively inflating the RA window enlarge at lines 806-808 and ria_end_idx_min at line 863? Also, the delta is taken from end_idx before the EOF clamp just below (lines 819-828), so vui_ra_pages can grow past the file size. ria_end_idx_min then feeds ll_read_ahead_pages() at lines 478-479, which raises ria_end_idx without an ria_eof guard - so readahead can extend past eof_index, the opposite of the "restrict readahead to eof" goal in the commit message.
LU-10026 llite: getting stripe info optimization ll_lov_getstripe_ea_info() is expensive call and should be avoided if possible. Let's use cached chunk size rather than get it from stripe info every time. llite: restrict readahead to eof Compressed file readahead rounding needs to come before readahead is limited to EOF. DDN-bug-id: EX-7601 Was-Change-Id: I4e9e7fe63301c08efcb05f170726735593a9431d llite: fix chunk_bits usage in readahead For the minimum compression chunk size, chunk bits is zero, so we cannot use if (chunk_bits) to determine if we're doing compression. This also fixes two other things: 1. A rounding error when rounding to chunk 2. Move rounding of end_idx to before first usage of end_idx, so calculation of number of pages is correct Without this, when the user reads 1 page or less, readahead will calculate the readahead page count as 0 and will exit without reading the chunk. DDN-bug-id: EX-6127 Was-Change-Id: I273506fd4f6ed5f0b8b5020357fd7caf0531e61c Test-Parameters: forjanitoronly Signed-off-by: Patrick Farrell <pfarrell@whamcloud.com> Signed-off-by: Sebastien Buisson <sbuisson@ddn.com> Signed-off-by: Artem Blagodarenko <ablagodarenko@ddn.com> Signed-off-by: Marc Vef <mvef@whamcloud.com> Change-Id: Id08487ec782f797e242e3f673c4a4dd8d526c9cc
The commit message only describes disabling fallocate, but the diff also adds a `rsync --sparse` decompression round-trip (the `decomp2` / "fiomap case" block) to compress_type() in sanity.sh. That change is about the sparse-copy/FIEMAP read path, not fallocate, and isn't mentioned here. Is it meant to be part of this patch, or should it be split into its own change with its own Change-Id?
(suggestion) This only checks lsm_entries[0]. lsme_compr_type is set per component (lov_ea.c copies lcme_compr_type for each LOV_PATTERN_COMPRESS entry), so a composite/PFL file could have an uncompressed component 0 and a compressed later component. Would fallocate over such a file still be allowed even though it targets a compressed component? (The ll_fiemap() guard has the same entry[0]-only limitation.)
(minor) `lio` and `loo` here just re-derive values already in scope: the outer `lio` (declared at the top of the function) is the same `cl2lov_io(env, ios)`, and `loo` is `lio->lis_object`. The inner `struct lov_io *lio` also shadows the outer one. This could just use the existing `lio->lis_object` without the extra declarations.
(minor) fallocate(2) is directly user-triggerable, so a CWARN here lets an unprivileged caller flood the console by repeatedly calling fallocate on a compressed file. The equivalent compressed-file guard in ll_fiemap() (lov_object.c around line 2021) returns -EOPNOTSUPP silently. Consider CDEBUG(D_VFSTRACE, ...) or a rate-limited message instead.
LU-10026 lov: fallocate is not allowed for compressed files Client Side Data Compression allocates blocks after a compression. It is impossible to preallocate blocks for the whole file, so fallocate should be disabled in case of compression. Test-Parameters: forjanitoronly Signed-off-by: Artem Blagodarenko <ablagodarenko@ddn.com> Signed-off-by: Marc Vef <mvef@whamcloud.com> Change-Id: Ie834ace183fdcec0d7d6f747237e0964c3c4120b
| unique failing test | history |
|---|---|
| sanity-lfsck@zfs:test_18c | seen in 25 other reviews |
| sanity-scrub@ldiskfs+DNE:test_9 | seen in 5 other reviews |
This is false.
These modules do not compile on newer kernels (6.18, at least). I think it should be possible to build without these.
just as the smatch implies this should really be IS_ERR, I don't think filp_open can return NULL?
smatch highlighted it in the past and it was still not addressed, but this looks like an inconsistency and should be && even though I guess all parts evaluate to 0 or 1
Oleg, I looked into this. For all three comments of yours, this is still exactly the same in the kernel today: https://github.com/torvalds/linux/blob/master/lib/lz4/lz4_decompress.c However, this discussion did come up on the kernel mailing list as well where the maintainer explained why & is used over && since smatch also flagged it there. There is a comment (line 147-148) on this here because of it. Basically, this is for performance reasons where it is noticeably beneficial in hot code segments. There are actually several points (very briefly, I linked the detailed discussion below): 1. `&&` indroduces a serial dependency but it is better for style. `&` can be executed in parallel 2. Too many densely packed branches impact the micro-op cache 3. A denser pack of branches impacts branch prediction In summary, they argue that it is _generally_ better to use `&` over `&&` (iff conditions permit it) but it only matters in very hot code segments, which this is. Much more detailed source: https://lore.kernel.org/all/D4762145-BBC5-4574-BF68-8C1A3AF41D98@fb.com/
is this likely really just for the first part of this statement, or for the whole while condition (and the parenthesis is then misplaced)
hmm, I'm not an expert in this code here, but this looks to be correct. IIUC, likely() here is only used for the safety/bounds check which looks to be an over-read guard when there is fewer of 15 bytes (`RUN_MASK` as the safety margin?) of input left. I guess, a valid input is generally considered the common case and thus `likely`. `s == 255` is deciding whether we iterate further. That part is variable and should not be included in the `likely` statement.
LU-10026 lustre: add lz4 and lz4hc kernel modules lz4 and lz4hc kernel modules implement compression according to the lz4 and lz4hc algorithms respectively, through the kernel Crypto API. lz4 module provides 2 cipher drivers under the generic name 'lz4': * lz4-lustre-generic of type compression * lz4-lustre-scomp of type scomp lz4hc module provides 2 cipher drivers under the generic name 'lz4hc': * lz4hc-lustre-generic of type compression * lz4hc-lustre-scomp of type scomp lz4 and lz4hc kernel module sources are copied from linux v6.1-rc5, and renamed to llz4.c and llz4hc.c respectively to avoid name collisions. Use of vmalloc has been changed to kvmalloc since it is faster in most cases. They implement the Crypto API interface, and rely on the lz4/lz4hc kernel library for compression implementation. They have been modified to grok a compression acceleration/level, as read from the top 4 bits of the crypto_tfm flags, and pass it to the underlying library. The lz4/lz4hc library sources are also copied from linux v6.1-rc5 and built statically, so lz4_compress, lz4_decompress and lz4hc_compress sources have been "de-modulified", and EXPORT_SYMBOLs removed. Headers have also been copied from linux v6.1-rc5 for consistency, and source files modified to include the copied headers instead of the system headers. All aforementioned sources are located in the lustre_compat/crypto/lz4 directory. The lz4/lz4hc modules are built only if the kernel does not provide them. This is verified by checking if the CONFIG_CRYPTO_LZ4 and CONFIG_CRYPTO_LZ4HC kernel config options are defined. This patch provides unit testing of several compression modules, such as lz4, lz4hc, lzo and gzip. This is done via a new test kernel module kcompr.ko, and new sanity test_84. File lustre/tests/kernel/kcompr.c contains examples of how to call the compression/decompression routines. EX-7998 lustre: Fix build with crypto_tfm_ctx on kernel 6.2 In recent kernels, commit e634ac4 "crypto: api - Add crypto_tfm_ctx_dma" moved crypto_tfm_ctx into algapi.h. So this file must be included from lustre_crypto.h. Was-Change-Id: If10f721355da3e58ca541c17615e978334e8d718 EX-7683 utils: always try to use our own lz4/lz4hc lz4/lz4hc provided by the kernel do not grok a compression level. The built-in lz4/lz4hc do, so always build them as dedicated kernel modules llz4.ko and llz4hc.ko, with the same .cra_name but with a slightly higher .cra_priority = 110, so that they are preferred over the in-kernel modules if any. And try to manually load the llz4/llz4hc kernel modules when a file requires compression with the corresponding alg. This is a "one-shot" try that allows us to prefer our modules that has level support, but continues to at least compress/decompress files even if our own modules are not available. Was-Change-Id: I0bdf267f998e21df81e460250a653aed34e3215d EX-bug-id: EX-6275 Test-Parameters: testlist=sanity env=ONLY=84 Signed-off-by: Sebastien Buisson <sbuisson@ddn.com> Signed-off-by: Artem Blagodarenko <ablagodarenko@ddn.com> Signed-off-by: Marc Vef <mvef@whamcloud.com> Change-Id: I74ce95ff18194c6052d291588d7e8c79596a7f23
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-dne-part-1 | RHEL 8.10 / x86_64 | ran 3 tests. 1 tests failed: sanity. | session |
| review-dne-part-2 | RHEL 8.10 / x86_64 | ran 11 tests. 1 tests failed: sanity-sec. | session |
LU-10026 lustre: add lgzip kernel module lgzip kernel module implements compression according to the deflate/zlib algorithm, through the kernel Crypto API. It provides 2 cipher drivers under the generic name 'deflate': * deflate-lustre-generic of type compression * deflate-lustre-scomp of type scomp Note the 'deflate' name is identical to the in-kernel module, but lgzip registers it with a slightly higher .cra_priority = 110, so that it is preferred over the in-kernel module. Our 'deflate' is also different in that it accepts a compression level as explained below. lgzip kernel module sources are copied from linux v6.2-rc5 and renamed to gzip.c to avoid name collisions. It implements the Crypto API interface, and rely on the deflate/zlib kernel library for compression implementation. It has been modified to grok a compression level, as read from the top 4 bits of the crypto_tfm flags, and pass it to the underlying library. The deflate/zlib library sources are also copied from linux v6.2-rc5 and built statically. Headers have also been copied from linux v6.2-rc5 for consistency, and source files modified to include the copied headers instead of the system headers. All aforementioned sources are located in the lustre_compat/crypto/gzip directory. The lgzip module is always built with Lustre. This patch enhances the test kernel module kcompr.ko to exercise the compression level of the provided 'deflate' module. It also tries to manually load the lgzip kernel module when a file requires compression with the 'delfate' alg. This is a "one-shot" try that allows us to prefer our module that has level support, but continues to at least compress/decompress files even if our own module is not available. EX-bug-id: EX-6206 Signed-off-by: Sebastien Buisson <sbuisson@ddn.com> Signed-off-by: Artem Blagodarenko <ablagodarenko@ddn.com> Signed-off-by: Marc Vef <mvef@whamcloud.com> Change-Id: I10b0ec03cf7d0e4fe3dd2898e77f357667209755
| unique failing test | history |
|---|---|
| sanity-hsm@ldiskfs+DNE:test_251 | seen in 1 other review |
| sanity-hsm@ldiskfs+DNE:test_254b | NEW unique failure for this branch in the last 30 days, and was seen 0 times across 0 other branches 0 reviews |
| sanityn@ldiskfs+DNE:test_51c | seen in 3 other reviews |
And what is this part of the patch about?
static int osd_sync(const struct lu_env *env, struct dt_device *d)
{
int rc;
struct super_block *s = osd_sb(osd_dt_dev(d));
ENTRY;
down_read(&s->s_umount);
rc = s->s_op->sync_fs(s, 1);
up_read(&s->s_umount);
CDEBUG(D_CACHE, "%s: synced OSD: rc = %d\n", osd_dt_dev(d)->od_svname,
rc);
return rc;
}
It used to be like this
And further on in ldiskfs_sync_fs() there is still flush_workqueue(sbi->s_misc_wq);
(minor) this version is outdated
LU-16032 osd: move unlink of large objects to separate thread
Final unlink and freeing of blocks for large objects can lead to
a thread hung with this call stack:
Net: Service thread pid 1739 was inactive for 200.16s.
The thread might be hung, or it might only be slow and will
resume later.
Dumping the stack trace for debugging purposes:
__wait_on_buffer+0x2a/0x30
ldiskfs_wait_block_bitmap+0xe0/0xf0 [ldiskfs]
ldiskfs_read_block_bitmap+0x31/0x60 [ldiskfs]
ldiskfs_free_blocks+0x329/0xbb0 [ldiskfs]
ldiskfs_ext_remove_space+0x8a9/0x1150 [ldiskfs]
ldiskfs_ext_truncate+0xb0/0xe0 [ldiskfs]
ldiskfs_truncate+0x3b7/0x3f0 [ldiskfs]
ldiskfs_evict_inode+0x58a/0x630 [ldiskfs]
evict+0xb4/0x180
iput+0xfc/0x190
osd_object_delete+0x1f8/0x370 [osd_ldiskfs]
lu_object_free.isra.30+0x68/0x170 [obdclass]
lu_object_put+0xc5/0x3e0 [obdclass]
ofd_destroy_by_fid+0x20e/0x500 [ofd]
ofd_destroy_hdl+0x267/0x9f0 [ofd]
tgt_request_handle+0xaee/0x15f0 [ptlrpc]
ptlrpc_server_handle_request+0x24b/0xab0 [ptlrpc]
ptlrpc_main+0xb34/0x1470 [ptlrpc]
kthread+0xd1/0xe0
Let's move final unlink to workqueue if inode size > 1GB. The size
threshold be configured by setting the minimum async truncate size
with the "osd-ldiskfs.*.delay_unlink_mb" parameter.
Writes to "osd-ldiskfs.*.force_sync" parameter will flush pending
delayed unlinks so that space can be reclaimed as needed.
Lustre-change: https://review.whamcloud.com/47995
Lustre-commit: a772e90243ea0ff1de6ae9c67e1f6384c431d200
Change-Id: Id535ae4c58732769effabee42835bc2da8cb5cc1
Signed-off-by: Artem Blagodarenko <ablagodarenko@whamcloud.com>
DDN-bug-id: DDN-3144
Reviewed-by: Andreas Dilger <adilger@whamcloud.com>
Reviewed-by: Alex Zhuravlev <bzzz@whamcloud.com>
Signed-off-by: Etienne AUJAMES <eaujames@ddn.com>
| unique failing test | history |
|---|---|
| sanity-lfsck@zfs:test_18c | seen in 25 other reviews |
| sanity-scrub@ldiskfs+DNE:test_9 | seen in 5 other reviews |
This is false.
These modules do not compile on newer kernels (6.18, at least). I think it should be possible to build without these.
just as the smatch implies this should really be IS_ERR, I don't think filp_open can return NULL?
smatch highlighted it in the past and it was still not addressed, but this looks like an inconsistency and should be && even though I guess all parts evaluate to 0 or 1
Oleg, I looked into this. For all three comments of yours, this is still exactly the same in the kernel today: https://github.com/torvalds/linux/blob/master/lib/lz4/lz4_decompress.c However, this discussion did come up on the kernel mailing list as well where the maintainer explained why & is used over && since smatch also flagged it there. There is a comment (line 147-148) on this here because of it. Basically, this is for performance reasons where it is noticeably beneficial in hot code segments. There are actually several points (very briefly, I linked the detailed discussion below): 1. `&&` indroduces a serial dependency but it is better for style. `&` can be executed in parallel 2. Too many densely packed branches impact the micro-op cache 3. A denser pack of branches impacts branch prediction In summary, they argue that it is _generally_ better to use `&` over `&&` (iff conditions permit it) but it only matters in very hot code segments, which this is. Much more detailed source: https://lore.kernel.org/all/D4762145-BBC5-4574-BF68-8C1A3AF41D98@fb.com/
is this likely really just for the first part of this statement, or for the whole while condition (and the parenthesis is then misplaced)
hmm, I'm not an expert in this code here, but this looks to be correct. IIUC, likely() here is only used for the safety/bounds check which looks to be an over-read guard when there is fewer of 15 bytes (`RUN_MASK` as the safety margin?) of input left. I guess, a valid input is generally considered the common case and thus `likely`. `s == 255` is deciding whether we iterate further. That part is variable and should not be included in the `likely` statement.
LU-10026 lustre: add lz4 and lz4hc kernel modules lz4 and lz4hc kernel modules implement compression according to the lz4 and lz4hc algorithms respectively, through the kernel Crypto API. lz4 module provides 2 cipher drivers under the generic name 'lz4': * lz4-lustre-generic of type compression * lz4-lustre-scomp of type scomp lz4hc module provides 2 cipher drivers under the generic name 'lz4hc': * lz4hc-lustre-generic of type compression * lz4hc-lustre-scomp of type scomp lz4 and lz4hc kernel module sources are copied from linux v6.1-rc5, and renamed to llz4.c and llz4hc.c respectively to avoid name collisions. Use of vmalloc has been changed to kvmalloc since it is faster in most cases. They implement the Crypto API interface, and rely on the lz4/lz4hc kernel library for compression implementation. They have been modified to grok a compression acceleration/level, as read from the top 4 bits of the crypto_tfm flags, and pass it to the underlying library. The lz4/lz4hc library sources are also copied from linux v6.1-rc5 and built statically, so lz4_compress, lz4_decompress and lz4hc_compress sources have been "de-modulified", and EXPORT_SYMBOLs removed. Headers have also been copied from linux v6.1-rc5 for consistency, and source files modified to include the copied headers instead of the system headers. All aforementioned sources are located in the lustre_compat/crypto/lz4 directory. The lz4/lz4hc modules are built only if the kernel does not provide them. This is verified by checking if the CONFIG_CRYPTO_LZ4 and CONFIG_CRYPTO_LZ4HC kernel config options are defined. This patch provides unit testing of several compression modules, such as lz4, lz4hc, lzo and gzip. This is done via a new test kernel module kcompr.ko, and new sanity test_84. File lustre/tests/kernel/kcompr.c contains examples of how to call the compression/decompression routines. EX-7998 lustre: Fix build with crypto_tfm_ctx on kernel 6.2 In recent kernels, commit e634ac4 "crypto: api - Add crypto_tfm_ctx_dma" moved crypto_tfm_ctx into algapi.h. So this file must be included from lustre_crypto.h. Was-Change-Id: If10f721355da3e58ca541c17615e978334e8d718 EX-7683 utils: always try to use our own lz4/lz4hc lz4/lz4hc provided by the kernel do not grok a compression level. The built-in lz4/lz4hc do, so always build them as dedicated kernel modules llz4.ko and llz4hc.ko, with the same .cra_name but with a slightly higher .cra_priority = 110, so that they are preferred over the in-kernel modules if any. And try to manually load the llz4/llz4hc kernel modules when a file requires compression with the corresponding alg. This is a "one-shot" try that allows us to prefer our modules that has level support, but continues to at least compress/decompress files even if our own modules are not available. Was-Change-Id: I0bdf267f998e21df81e460250a653aed34e3215d EX-bug-id: EX-6275 Test-Parameters: testlist=sanity env=ONLY=84 Signed-off-by: Sebastien Buisson <sbuisson@ddn.com> Signed-off-by: Artem Blagodarenko <ablagodarenko@ddn.com> Signed-off-by: Marc Vef <mvef@whamcloud.com> Change-Id: I74ce95ff18194c6052d291588d7e8c79596a7f23
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-dne-part-1 | RHEL 8.10 / x86_64 | ran 3 tests. 1 tests failed: sanity. | session |
| review-dne-part-2 | RHEL 8.10 / x86_64 | ran 11 tests. 1 tests failed: sanity-sec. | session |
LU-10026 lustre: add lgzip kernel module lgzip kernel module implements compression according to the deflate/zlib algorithm, through the kernel Crypto API. It provides 2 cipher drivers under the generic name 'deflate': * deflate-lustre-generic of type compression * deflate-lustre-scomp of type scomp Note the 'deflate' name is identical to the in-kernel module, but lgzip registers it with a slightly higher .cra_priority = 110, so that it is preferred over the in-kernel module. Our 'deflate' is also different in that it accepts a compression level as explained below. lgzip kernel module sources are copied from linux v6.2-rc5 and renamed to gzip.c to avoid name collisions. It implements the Crypto API interface, and rely on the deflate/zlib kernel library for compression implementation. It has been modified to grok a compression level, as read from the top 4 bits of the crypto_tfm flags, and pass it to the underlying library. The deflate/zlib library sources are also copied from linux v6.2-rc5 and built statically. Headers have also been copied from linux v6.2-rc5 for consistency, and source files modified to include the copied headers instead of the system headers. All aforementioned sources are located in the lustre_compat/crypto/gzip directory. The lgzip module is always built with Lustre. This patch enhances the test kernel module kcompr.ko to exercise the compression level of the provided 'deflate' module. It also tries to manually load the lgzip kernel module when a file requires compression with the 'delfate' alg. This is a "one-shot" try that allows us to prefer our module that has level support, but continues to at least compress/decompress files even if our own module is not available. EX-bug-id: EX-6206 Signed-off-by: Sebastien Buisson <sbuisson@ddn.com> Signed-off-by: Artem Blagodarenko <ablagodarenko@ddn.com> Signed-off-by: Marc Vef <mvef@whamcloud.com> Change-Id: I10b0ec03cf7d0e4fe3dd2898e77f357667209755
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-ldiskfs-dne-arm | RHEL 8.10 / x86_64, Rocky 9.5 / aarch64 | ran 5 tests. 1 tests failed: sanity. | session |
The `.YS` only goes at the end of the SYNOPSIS section.
LU-18222 man: lctl lqa man page Add man pages for LQA feature. Signed-off-by: Sergey Cheremencev <scherementsev@ddn.com> Signed-off-by: Artem Blagodarenko <ablagodarenko@thelustrecollective.com> Test-Parameters: trivial Change-Id: I11360fd2ea58a311b92c651057ee415d3c99e4f3
| unique failing test | history |
|---|---|
| sanity1@zfs:test_56ab | seen in 21 other reviews |
(defect) The message describes a "sec: page_pools shrinker fix" (element_size / ppp_idle_idx / IDLE_IDX_MAX changes) and adds sanity-compr test_2000 gated on client 2.17.53 "for compression OOM fix", but there is no page_pools.c change anywhere in the diff, so test_2000 can't pass on this commit. A few hunks also aren't accounted for by the message: the ll_lov_getstripe_ea_info() switch to md_getattr() when filename is NULL in llite/file.c, and the LINVRNT->LASSERT change in cl_io_rw_init(). Are these meant to be part of this patch? Also, since this touches the OST_WRITE/OST_READ wire path (OBD_BRW_COMPRESSED, o_size/OBD_MD_FLSIZE), a Test-Parameters: line requesting client/server interop testing would be worthwhile.
(defect) this needs to add: ``` Test-Parameters: testlist=sanity-compr ``` otherwise that test session is not being run on master.
(defect) 0x418 is already used by OBD_FAIL_OSC_FIEMAP two lines above, so this fault injection point collides with it (triggering one fires the other). 0x41a or 0x420 appear free.
(defect) On the decompress path *type is llch_compr_type read from storage/the wire, so a corrupt header carrying LL_COMPR_TYPE_BEST or LL_COMPR_TYPE_FAST would LBUG the client here. The sibling change that replaced an assert with EIO ("we shouldn't assert on values read from storage") applies - return -EIO instead of asserting?
(minor) If fill_cpga() fails here, `dst` (obtained just above from obd_pool_get_objects) has not yet been stored in bp_cmp_chunk, and the out: path only releases `src`, so the chunk buffer looks leaked on this error return.
(defect) compressed_pages comes from llch.llch_compr_size, which is read straight out of the server reply by is_chunk_start() (that only checks llch_magic). It is bounded above only by page_count - i, not by pages_per_chunk. merge_chunk() then copies compressed_pages * PAGE_SIZE bytes into `src`, a single chunk_size buffer from obd_pool_get_objects(&src, buf_bits). A corrupt or hostile compr_size larger than one chunk overflows `src`. The LASSERT(src_size <= chunk_size) on the next line runs only after the copy (and asserting on wire data is itself unsafe). Can compressed_pages be bounded against pages_per_chunk before merging, returning -EUCLEAN otherwise?
(defect) On a resend, osc_brw_redo_request() re-enters this function with aa->aa_ppga. For a compressed write that array is the cpga built by compress_request()/fill_cpga() - bare brw_pages allocated with OBD_ALLOC_PTR and freed with a plain OBD_FREE, not embedded in an osc_async_page. brw_page2oap(pga[0]) then points before the allocation, so oap2cl_page() and the following clpage->cp_inode / cp_type / cp_compr_type reads touch invalid memory. -EINPROGRESS/recoverable resends are common, so this looks like a client crash on any compressed-write resend. Should the resend derive inode/compressed from aa_ncppga instead?
(style) This loop is indented with spaces instead of tabs.
LU-10026 osc: osc brw request compression This patch adds client-side compression/decompression. The client-side data compression project (CSDC) reduces storage and network utilization by leveraging the more plentiful memory and CPU resources on the local client. Data is sent compressed over the network, saved directly to storage on the server side, and decompressed back on the client side. Uncompressed data is kept in client page cache, all while being functionally transparent to the end user and application. As an example, a test file is compressed and decompressed. The resulting file is compared with the original one. The test case shows 2.5x compression ratio: 356K /mnt/lustre/d460.sanity/sanity.sh 884K /tmp/cmp-46ofie/decompressed_sanity.sh Compression should read whole chunk even if offset and size differ. Let's modify readahead to force reading data from the offset and size multiple to the chunk size. utils: fix cp_comp_type size cp_comp_type should be 8 bits, as llch_compr_type and all associated variables are declared as u8. So remove useless cp_comp_enabled and fix code to test for compressed component with cp_comp_type against LL_COMPR_TYPE_NONE. And update LL_COMPR_TYPE_MAX value to 255 to avoid conflicts with future compression types. DDN-bug-id: EX-7775 Was-Change-Id: Ia15868ac0ac003b62942540a57f782226ae8c141 tests: test compression without bzip2/HDF5 Run as much of sanity test_460a compression tests as possible, even if bunzip2 or HDF5 file are unavailable. Print a clear message in test_84 if bunzip2 unavailable. DDN-bug-id: EX-6127 Was-Change-Id: I36251834f636600eb9b0194ccd14c8b203da32e5 lov: refactor lov_io_lsme_at lov_io_lsme_at needs some minor changes to be called from lov_io_slice_init(). DDN-bug-id: EX-7601 Was-Change-Id: I0611d66052e22d349932eb26257369e07b9b8167 osc: don't check for start inside the chunk Chunk size is the same for the whole request and every chunk offset is multiple to a chunk size. No need to search for compression header in every page. It is enough to check every with offset multiple to a chunk size. DDN-bug-id: EX-7818 Was-Change-Id: Ie2ef645130656279e152ea1f7e6db01cb33836ca osc: minor compression cleanups This cleans up some style and argument issues I found made the code a little harder to follow. DDN-bug-id: EX-8270 Was-Change-Id: Ia3492ae79acf6c83d724cc91b0201c7872325853 osc: move common CSDC code to the library CSDC repacks a chunk on the server side in case of the partial rewrite. There are routines that can be shared between client and server. This patch moves common compression code to the libcfs. DDN-bug-id: EX-7601 Was-Change-Id: I824211a3435b0479f7a3b8f08598a5b567b67d3c osc: use correct count Using the number of bytes in the compressed page creates gaps in the RDMA, which IB memory registration cannot accept. Fix this by always setting count to PAGE_SIZE for compressed pages and otherwise using the count from the original source page. Setting PAGE_SIZE for compressed pages is valid because client only does compression for aligned IO, except for the trailing chunk. For the trailing chunk, the file size is set on the server, so any trailing bytes are ignored. DDN-bug-id: EX-8245 Was-Change-Id: Ied89d3ac328fb6020079392f5a8812ad5637b4a4 csdc: remove holes from struct ll_compr_hdr This patch reorganizes struct ll_compr_hdr to remove alignment holes. DDN-bug-id: EX-8353 Was-Change-Id: I59800b00e3a17972d621bae21ba06509a39b1036 osc: apply compressed flag to dst page The existing code to apply brw flags to compressed pages has two issues: 1. The dst_page is NOT an osc async page, it is a bare BRW page. This means the brw_page2oap macro isn't right, because there is no oap page. Because oap_brw_flags is actually oap_brw_page.flag, we don't ever access the memory pointed at by OAP, just use it to find an offset back in to the brw page. This means the flags are set correctly, but we still shouldn't use this macro. 2. However, the function then overwrites these flags by copying from a page in the source, so OBD_BRW_COMPRESSED is lost. Add OBD_BRW_COMPRESSED when we set flags. This ensures the flag is actually sent to the server on compressed IO. This was not causing any problems because the server does not actually use the OBD_BRW_COMPRESSED flag yet. (EX-7601 uses this flag) DDN-bug-id: EX-7601 Was-Change-Id: Ia94cdc803868ce16a0b66fd58578ec8b2d00cbae osc: remove unused 'wrkmem' compress_chunk() takes a wrkmem buffer, which it does not use. Remove this and its allocation in compress_request. DDN-bug-id: EX-7601 Was-Change-Id: I6f236f018f5b79c57cc8725ca0f95125810a4064 osc: walk chunk unaligned RPC correctly For decompression, the client must start looking for compressed chunks at a chunk aligned offset. Implement this in decompress_request. DDN-bug-id: EX-7601 Was-Change-Id: I3273135990ddf51e8b3c651734e19350e91f659c ofd: add obd level compression lib Some compression functions will be used by several areas of of Lustre, so they need to be in obdclass. This moves merge_chunk and unmerge_chunk there and adds the ability for them to merge lnbs. This is used in a future patch. DDN-bug-id: EX-7601 Was-Change-Id: If4a318119bb7685e41adb9f3b31a66074031e6ac osc: remove cpga fill bits cpga fill bits are not needed now that we don't support compression and encryption. DDN-bug-id: EX-7601 Was-Change-Id: I13c2278e085e9b288bd896585947e28e2ea505ca osc: only set compressed flag on compressed pages The code accidentally sets the compressed flag on all pages processed through fill_cpga, even if they're not compressed. Oops. Also stop setting pg->index on the pages in the compressed pga, this is only used by encryption and that's no longer supported with compression. DDN-bug-id: EX-7601 Was-Change-Id: I313fd943a18b71cd52493852a6884f30d187e52f osc: use pages_left in unmerge_chunk Since we have compressed chunks < chunk_size (if they're after EOF), we must use pages_left in unmgerge_chunk or it will go off the end of the page array. This also lets us remove the workaround where unmerge_chunk would skip pages that were not present. unmerge_chunk always works with a known and complete set of pages, so this check is unneeded. We should also check that our count of bytes is correct when we finish. DDN-bug-id: EX-7600 Was-Change-Id: I88896307990ff839514e54e9a7e18390a457e5d8 osc: rename 'done' Rename the ambiguous 'done' and remove it where not used. DDN-bug-id: EX-7601 Was-Change-Id: I8fb88b7a91fcc7dbd5ce2d29a61c18330fc0cda3 osc: cleanup compression variables Make usage of the compression variables more readable. DDN-bug-id: EX-7601 Was-Change-Id: I6daff56b56877c8f36e02303cc0579ba7faa731b ofd: make compress_chunk take chunk_bits Chunk bits is used everywhere, have compress_chunk convert to log bits rather than have the callers do it. DDN-bug-id: EX-7601 Was-Change-Id: Ic01bb749425cb95d9c5717965d692a18138ceeb7 obd: move module load to function This is a trivial code change to make alloc_compr a bit shorter. DDN-bug-id: EX-7601 Was-Change-Id: I0a790afe7afebde1d223420d9a578529da6ff7e5 osc: variable cleanup in decompress_req Use type and lvl variables in decompress_request. Remove an unused variable and an assert which can never fire. DDN-bug-id: EX-7601 Was-Change-Id: Ieff57411a2a41215fd368d731614801bd0f43e38 osc: replace assert with error We shouldn't assert on values read from storage, instead if they are incorrect, we should give EIO. DDN-bug-id: EX-7601 Was-Change-Id: Icda213e3c5a90a848c9b008788e92ee49e2efcb1 osc: rearrange compress_request A trivial rearrangement of compress_request to make it more readable before redoing the core logic. DDN-bug-id: EX-7601 Was-Change-Id: I1d34cd2a2a6d84bc30cc7dae8eb07586c4837f7d osc: give compress_request explicit success Compress_request has explicit failure handling, but the success handling just follows the failure handling. This is confusing - on failure, we do: page_count = *pcount then immediately do: *pcount = page_count It also sets *orig_pga = pga on success OR failure, which is wrong because compress_request may have modified pga and then failed. DDN-bug-id: EX-7601 Was-Change-Id: I121ec71cfe35babc4a572951e93f7581887ade80 osc: remove &pga usage in compress_request The usage of 'pga' and '&pga' in compress_request is confusing, but also, compress_request modifies &pga by allocating a new compressed page array. Except if we fail in compress_request, we free that new page array. This means failing in compress_request replaces 'pga' with a pointer to freed memory. Instead, create an explicit cpga pointer in the caller and use that. This allows compress_request to fail safely. DDN-bug-id: EX-7601 Was-Change-Id: Idaf592103c57b0e9ce76ab520a69b819d4f37be9 obd: move type switching to alloc_compr callers The code is much cleaner if we can eliminated applied type and handle that issue once per compression or decompression rather than for every chunk. This requires moving the type switching inside alloc_compr. (Also improve some error messages - alloc_compr can fail with ENOMEM as well.) The compression code currently allocates a transform for every chunk on the client. This is relatively cheap, but it also complicates the code by repeatedly checking if a particular compression type is supported (this is the "applied type" code). Moving alloc_compr to compress/decompress request makes the code much simpler. DDN-bug-id: EX-7601 Was-Change-Id: I162e81577db721a9715d57b3f262fcabbcbf308a osc: rename pages_in_chunk Chunks can have variable numbers of pages in them. DDN-bug-id: EX-7601 Was-Change-Id: If199d777367569e62c21305f6e4b9f3e4cce6d06 osc: allow multiple chunks in read It's rare, but reads can sometimes have multiple discontiguous chunks. Update decompress_request to handle this case. DDN-bug-id: EX-7601 Was-Change-Id: I880af95db285dce76db3610e8140a0f54baa401b ofd: do not overwrite rc in unmerge_chunk unmerge_chunk should not be responsible for setting the lnb rc, because this overwrites the result of any previous activity on the lnb. Plus, unmerge_chunk can't fail. DDN-bug-id: EX-7601 Was-Change-Id: Id1ce590c7f1da3ab7faddbd685d264a33c08d639 osc: calculate compressed size reduction accurately Compression reduces space used if it results in allocating at least one fewer block on disk. Modify the checks in compress_chunk to reflect this, rather than using the simpler "reduce size by at least 4K" calculation. Also do not attempt to compress chunks if they are less than 4K in size, since they can't possibly get a space benefit. This improved my measured ratio on a version of the Linux kernel source data set from 1.24 to 1.56, so this is significant for datasets with many small files. (This version of the source had large incompressible files removed, to focus on smaller files. The unmodified data set would not improve as much.) Note this is still short of our estimates, so either the estimate or Lustre still needs adjustment. TBD. DDN-bug-id: EX-7601 Was-Change-Id: I815706914b88de4f532a674d773769aa3a64d218 osc: rewrite compress_request The existing version of compress_request can't handle discontiguous RPCs. Rewrite the logic to handle this case properly. This also implements kms handling. If a write chunks ends at the known minimum size, we know this write is after all other data in the file and so there is no compressed data under it. This means we can compress this chunk. DDN-bug-id: EX-7601 Was-Change-Id: I8a912d9e279d04c8ff07de39e63a1ec9b490d921 osc: handle partial chunks in decompress_request Now that we have compression for incomplete chunks at the end of files, decompress_request needs to handle these chunks. This patch modifies it to understand compressed chunks which are less than chunk_size pages. DDN-bug-id: EX-7601 Was-Change-Id: I877550fa0d418def406e0308392a5336ec9f3ab6 osc: add check to decompress_request decompress_request should check to see if there's room in the RPC for the decompressed data, since this can occur if there's a bug or data corruption, and otherwise we will go past the end of the RPC during decompression. DDN-bug-id: EX-7601 Was-Change-Id: Ib1bf19bf39701b72f0f5a61b2aaff2f2fdad1897 osc: debug fix in decompress_request Debug message had an incorrect subtraction. DDN-bug-id: EX-7601 Was-Change-Id: I5daf360766ca77b98dc5af3d72c42ac38f5782bc obd: add 'lvl' for best and fast 'best' and 'fast' compression types must also set a level, because not all levels are supported by all algorithms. Rather than trying to be clever, just use simple universally supported values, except for lz4fast, where we special case this, because otherwise '0' is the slowest setting (and lz4fast is likely to remain our default fastest). DDN-bug-id: EX-6269 Was-Change-Id: I7c29659d4f027af2e44285ae38e4c9d91e35509a osc: decompress with algorithm from server Data may not be compressed with the compression type and level from the layout, so we must use the compression type and level from storage for decompression. DDN-bug-id: EX-6269 Was-Change-Id: Ib4cdccf294ef631a25147413d7f5c1a847c9504e osc: handle different compression types Allow the client to handle different compression types in a single component. This shouldn't happen normally, but it may happen in the future if there is dynamic compression algorithm selection for "fast" or "best" types (e.g. compress based on available CPU and network bandwidth or RPC backlog). DDN-bug-id: EX-6269 Was-Change-Id: Ide2731c60a68584e7cbb474bee88a17e9a7b8fec osc: add COMPR_GAP check to compress_request Currently, compress_request will build the compression buffer (calling merge_chunk()) for requests which are less than the minimum compression gap. This is noticed in the compression code when it checks if there's enough data to attempt compression, but we can do a trivial check in compress_request() to save that work. Also fix a few minor style things. This is not an important fix, but I discovered it while investigating another issue and it's trivial to resolve. DDN-bug-id: EX-7601 Was-Change-Id: Ieb32e6297e10d229f23c58e2ef4d933ce3dda4f2 lustre: add uncompressed size to compression header It's useful to have the uncompressed size of the data in the compression header. Also, we have three checksum fields - compressed, uncompressed, and header, but in practice, checksumming the compressed data including the header is enough to cover all of these. This patch cleans up all of this at the same time. DDN-bug-id: EX-8851 Was-Change-Id: Ie82e0dbe9c862ddc88999b109cea1f27577dbbff csdc: rename "cp_comp_*" to "cp_compr_*" This patch renames "cp_comp_type", "cp_comp_level", and "cp_chunk_log_bits" to use "compr" in the name to be consistent with other variable names. DDN-bug-id: EX-8353 Was-Change-Id: I428ff3a789b33da02832dee02f316b02d97137e2 osc: Do not iterate over chunk pages If osc_decompress() knows chunk size, no need to iterate other all pages in chunk. Thay can be skipped. DDN-bug-id: EX-7729 Was-Change-Id: Ib84b060075c55c97eba9f74ef017c0a956e85b12 csdc: Fix the upper mergeable chunk pointer If the full chunk is followed by un mergeable page, the upper mergeable chunk pointer is occasionally set to this unmanageable page. The chunk size is calculated wrongly then and the next condition suggest not to compress this chunk, because its size is not equal to the expected size. The pointer should be moved to the first instruction after the can_merge_pages(). DDN-bug-id: EX-9192 Was-Change-Id: I09fedc770c8bbcac4864b32372a941da5e0c7ac3 obdclass: reset bits after decompression as uncompressed data can be less than chunk/page, but still be visiable to userspace as a part of a sparse file. DDN-bug-id: EX-9873 Was-Change-Id: I4114b0704fb685013f4e03cf2d80ccde2cc8c87f osc: preserve compressed pages for OST_WRITE replay it's incorrect to release compressed pages right after reply as we may resend them during OST_WRITE replay. DDN-bug-id: EX-9895 Was-Change-Id: I3edc16d6556ddd60735d2f14fe879fc0f45231d7 csdc: Fix the next available algorithm selection Currently, if a chosen algorithm is not available, then next preferable is chosen, but an error code is not cleared so __alloc_compr() returns this wrong code. Data if written uncompressed while it can be compressed with next appropriate algorithm. This patch adds this error code clearing. Test is provided. DDN-bug-id: EX-9889 Was-Change-Id: I59f65058a0fe9b108de3d4ba7cf5950f18e32204 ptlrpc: reset refcount reusing pages for new req when a set of compressed pages are reused for a redo RPC, we have to reset refcounter. DDN-bug-id: EX-10061 Was-Change-Id: I16c46d857d60cf4dd0f6317190e7c99b2667e135 ptlrpc: drop extra reference to compressed pages if the request is not replayable, so brw_commit() won't be called. DDN-bug-id: EX-10184 Was-Change-Id: Ib29ab19b69fd15ef339cc18c5dfef17217a32cb1 osc: remove noisy warning if we can't compress data on the client side for a reason (e.g. too random), we just send data uncompressed, makes sense to hide the error message confusing customers: kernel: LustreError: 936336:0:(lustre_compr.c:345:compress_chunk()) exafs-OST0007-osc-ff49193c293f3800: Compression failed, type 5, lvl 5, -22 DDN-bug-id: EX-10593 Was-Change-Id: Icbc8be38b0372f8f67690e7d94384a16ab02cbe4 osc: fix osc_compress ENOMEM handling Send already compressed chunks in compress_request even if can't allocate memory for the next chunks. This patch also fixes statistic in case of ENOMEM. Without that write_chunks_incompressible hasn't been incremented in case of ENOMEM. Add sanity-compr_2002 to check write_chunks_compressible and write_chunks_incompressible in case of ENOMEM DDN-bug-id: EX-11035 Was-Change-Id: I3fc58ece6cf22582166ea77e4a217bc7a9efd77d csdc: remove buffer double sizes Pools now (POOL_ADD_PAGE is defined) allocate additional page for memory misuse debug, so no need to allocate double-sized buffers to prevent lz4 decompress problem. DDN-bug-id: EX-8276 Was-Change-Id: Iea04943ae0f95c8983c5ba7ec9a2c63003afe351 utils: fix LL_COMPR_TYPE wire checks Fix LL_COMPR_TYPE* wire checks to avoid duplication. The value of LL_COMPR_TYPE_UNCHANGED is also declared as 255 so that is does not conflict with other potential real compression types in the future. DDN-bug-id: EX-7775 Was-Change-Id: Iab9830f09f0778e1e1f3b1ea4c9878ce1017de8d sec: page_pools shrinker fix page_pool shrinker should consider ppp_free_pages as a number of memory regions with the size of 2^pool_order pages. Otherwhise it startis shrinking only when becomes too large, for example 2GB instead of 64MB. It often might be the reason of OOM especially when compressed files with different chunk-sizes have been written. In a such case several pools created for different chunk sizes might hold gygabytes of memory without any chance to release it. Fix element_size to return correct element size instead of number of pages. Because of that elements in page_pools had below sizes: order 0 = 4096 order 1 = 2 order 2 = 4 .... order 17 = 131072 This made possible to have elements with sizes 2,4,8,... bytes that have been allocated with OBD_VMALLOC. Now all sizes are miltiple of PAGE_SIZE: order 0 = 4096 order 1 = 8192 ... order 5 = 131072 Change the logic around ppp_idle_idx. 1. Recalculate ppp_idle_idx in __sptlrpc_pool_put_pages the same way as it done in __sptlrpc_pool_get_pages. It is possible that ppp_idle_idx might become 0 or very small (less than 10). If there is no new allocations it would be impossible to free anything from the pool during 40 seconds(CACHE_QUIESCENT_PERIOD) despite the large amount of free elements ready to shrinking. 2. In pool_shrink_count when ppp_idle_idx == IDLE_IDX_MAX, it means no one accessed pool for CACHE_QUIESCENT_PERIOD (40 seconds), i.e. we may release as much as possible pages. Earlier in a such case pool_shrink_count always returned 0. Another words it was impossible to shrink anything from the pool if there was no allocations for 40s. Add sanity-compr 2000 that reproduces the OOM problem on a client without this patch. DDN-bug-id: EX-10094 Was-Change-Id: I6b55ba67b0d21cdffdb57034e8e66063745f796e osc: Add BRW_COMPRESSED flag to reads We need to add the BRW_COMPRESSED flag to reads so servers can know if the client is able to decompress data. This lets servers decide if a client can be sent compressed data and the result won't be nonsense/corruption. This is important for future support of GPU direct, where the server will need to do the decompression. DDN-bug-id: EX-6269 Was-Change-Id: I36b5b73f983ce8f2e5297c3e9dc778a5eca54e6a osc: don't discard decompress_request error The error handling for decompress_request is unusual - non-zero returns are just discarded. And rc2 is just discarded. The read() doesn't fail or get a short read. Fix this so if decompression fails with an error. DDN-bug-id: EX-7807 Was-Change-Id: Idd01947c7375c9586a64f064dd6ee0ac2308ea86 osc: save compressed object size CSDC uses a sparse file feature. A client writes compressed data chunks to the original offsets so the same data is expected to be read from the same offsets. There are no writes after the last compressed chunk, so no "hole" after the last compressed chunk. Compressed file size (based on OST objects size) is smaller than the original on the "original last chunk size - compressed last chunk size" delta. Object size should be set to uncompressed size. This size is used to calculate file size and to remove the workaround of not compressing the last chunk in the file. DDN-bug-id: EX-7600 Was-Change-Id: I387c282e1cf788c3b8f6230ef555d73ffffe49c1 osd: save compressed object size on zfs "osc: save compressed object size" added means to transfer object size to the osd and added ldiskfs support. This patch adds saving objec size to the ZFS backend. Currently this fix submitted as separete patch, for testing purpouse, but can be marged to the main patch latter. DDN-bug-id: EX-7600 Was-Change-Id: I99e29e3f756a070b5f3cece12c4ca58f668a2ecf ldiskfs: fix detection of compressed extent The code in ldiskfs_map_inode_pages which detects a compressed extent checks the first lnb for that extent, but it's possible for some lnbs and not others to be compressed in a given extent, so we must check all of them. This occurs when multiple writes have been combined in to one RPC. If we don't detect compression correctly, we won't set the file size correctly and we'll get data corruption. DDN-bug-id: EX-7601 Was-Change-Id: I11d50bdc45c40d93bb1b829fcd930165b7626432 llite: DIO fallback on compressed files Fully supporting direct I/O on compressed files is tricky because we cannot pull the full chunk in to the page cache (because there is no page cache for DIO). So instead we fall back to buffered I/O for DIO on compressed files. This patch adds the check and a test for this. llite: allow aligned DIO with compression DDN-bug-id: EX-6127 DDN-bug-id: EX-7601 Was-Change-Id: I8224ef9b8ad1d912d8a11eccad37d3dff8dd8498 If a DIO is fully aligned to compression chunk boundaries, it is safe to do on a compressed file, so allow it. Was-Change-Id: If8fa3397c9424254538738f4d77f9f50d1c21129 lov: set IO property check in lov_io_init_composite Move compression IO check down to lov_io_init_composite(). DDN-bug-id: EX-9853 Was-Change-Id: I6d4ce3388d8eef788d31dabf4b3a5dd5eff5091f llite: DIO fallback on compressed files Direct I/O on compressed files is still not perfect for now, so we'd fall back to buffered I/O for DIO on compressed files. DDN-bug-id: EX-6266 Was-Change-Id: I2255638e4fbde766abf061989d07b37ad9f369a1 llite: Compute compression chunk ranges Determine the edges of any leading and trailing compression chunks touched by this IO and store them in the cl_io struct. The functionality in this patch also allows us to adjust the lock and read rounding to do them more intelligently, this will be done in a future patch. DDN-bug-id: EX-7601 Was-Change-Id: I526563ea347fb0246f97f3532b823c4345c3fa27 llite: round LDLM lock requests to chunk When we do IO with compression, we may need to 'fill' the compression chunk, reading up pages which have already been written to storage, so we can compress the whole chunk. Doing this safely requires that any dlmlock we're using always covers the full chunk. The easiest way to do this is to round the entire locking process to include leading or trailing compression chunks. DDN-bug-id: EX-7601 Was-Change-Id: I3c365844561d0da909e6290f4b58ef2211c2d255 csdc: is_chunk_start should return header copy In is_chunk_start() *ret_header = header; ... kunmap_atomic(header); ret_header is used after is_chunk_start(). The header copy should be returned from is_chunk_start() for safe work. DDN-bug-id: EX-9878 Was-Change-Id: Ib5e828d6b61e90dcd70c28589931a4490cf19c22 Signed-off-by: Artem Blagodarenko <ablagodarenko@ddn.com> Signed-off-by: Marc Vef <mvef@whamcloud.com> Change-Id: I9b41ab815db3df9ad7bdea5fca4c093cbda8814b
| unique failing test | history |
|---|---|
| sanity1@zfs:test_56ab | seen in 22 other reviews |
(minor) These CDEBUG() pairs print start_idx/end_idx before and after the rounding with no descriptive text, on the D_SEC (security) mask which is unrelated to readahead. Same at lines 813 and 816. They look like leftover debugging scaffolding; if kept, D_READA would be the right mask.
(defect) vui_ra_pages is initialized once per read syscall in vvp_io_read_start() and only reset at IO teardown, but ll_readahead() runs once per page (ll_readpage()->ll_io_read_page()->ll_readahead()). Won't this += accumulate across every not-uptodate page of a compressed read, progressively inflating the RA window enlarge at lines 806-808 and ria_end_idx_min at line 863? Also, the delta is taken from end_idx before the EOF clamp just below (lines 819-828), so vui_ra_pages can grow past the file size. ria_end_idx_min then feeds ll_read_ahead_pages() at lines 478-479, which raises ria_end_idx without an ria_eof guard - so readahead can extend past eof_index, the opposite of the "restrict readahead to eof" goal in the commit message.
LU-10026 llite: getting stripe info optimization ll_lov_getstripe_ea_info() is expensive call and should be avoided if possible. Let's use cached chunk size rather than get it from stripe info every time. llite: restrict readahead to eof Compressed file readahead rounding needs to come before readahead is limited to EOF. DDN-bug-id: EX-7601 Was-Change-Id: I4e9e7fe63301c08efcb05f170726735593a9431d llite: fix chunk_bits usage in readahead For the minimum compression chunk size, chunk bits is zero, so we cannot use if (chunk_bits) to determine if we're doing compression. This also fixes two other things: 1. A rounding error when rounding to chunk 2. Move rounding of end_idx to before first usage of end_idx, so calculation of number of pages is correct Without this, when the user reads 1 page or less, readahead will calculate the readahead page count as 0 and will exit without reading the chunk. DDN-bug-id: EX-6127 Was-Change-Id: I273506fd4f6ed5f0b8b5020357fd7caf0531e61c Test-Parameters: forjanitoronly Signed-off-by: Patrick Farrell <pfarrell@whamcloud.com> Signed-off-by: Sebastien Buisson <sbuisson@ddn.com> Signed-off-by: Artem Blagodarenko <ablagodarenko@ddn.com> Signed-off-by: Marc Vef <mvef@whamcloud.com> Change-Id: Id08487ec782f797e242e3f673c4a4dd8d526c9cc
The commit message only describes disabling fallocate, but the diff also adds a `rsync --sparse` decompression round-trip (the `decomp2` / "fiomap case" block) to compress_type() in sanity.sh. That change is about the sparse-copy/FIEMAP read path, not fallocate, and isn't mentioned here. Is it meant to be part of this patch, or should it be split into its own change with its own Change-Id?
(suggestion) This only checks lsm_entries[0]. lsme_compr_type is set per component (lov_ea.c copies lcme_compr_type for each LOV_PATTERN_COMPRESS entry), so a composite/PFL file could have an uncompressed component 0 and a compressed later component. Would fallocate over such a file still be allowed even though it targets a compressed component? (The ll_fiemap() guard has the same entry[0]-only limitation.)
(minor) `lio` and `loo` here just re-derive values already in scope: the outer `lio` (declared at the top of the function) is the same `cl2lov_io(env, ios)`, and `loo` is `lio->lis_object`. The inner `struct lov_io *lio` also shadows the outer one. This could just use the existing `lio->lis_object` without the extra declarations.
(minor) fallocate(2) is directly user-triggerable, so a CWARN here lets an unprivileged caller flood the console by repeatedly calling fallocate on a compressed file. The equivalent compressed-file guard in ll_fiemap() (lov_object.c around line 2021) returns -EOPNOTSUPP silently. Consider CDEBUG(D_VFSTRACE, ...) or a rate-limited message instead.
LU-10026 lov: fallocate is not allowed for compressed files Client Side Data Compression allocates blocks after a compression. It is impossible to preallocate blocks for the whole file, so fallocate should be disabled in case of compression. Test-Parameters: forjanitoronly Signed-off-by: Artem Blagodarenko <ablagodarenko@ddn.com> Signed-off-by: Marc Vef <mvef@whamcloud.com> Change-Id: Ie834ace183fdcec0d7d6f747237e0964c3c4120b
(minor) There is no `.TP` before this entry, so `--compress-type|--compr-type` and its description get folded into the `-z, --extension-size` paragraph above instead of starting their own tagged item. The two entries below it do have theirs.
weird but in my man output it looks ok.
The entry was missing its .TP in the troff source. Some man implementations are lenient and reflow correctly, but others fold the entry into the previous paragraph. Added .TP for correctness.
Done. Added the missing .TP before the --compress-type|--compr-type entry.
Should be fixed if patch is refreshed.
LU-10026 csdc: set compress component for file
* 'lfs setstripe' to support compress component
--compress|-Z <type>[:<level>]
Set component compression algorithm <type> and compress <level>;
--compress-chunk=<size>
Set compress data chunk size in KiB used by the compression
algorithm, the value will be adjusted to power-of-two multiples of
the base 64KiB.
Example:
$ lfs setstripe -Eeof -Z lz4:5 --compress-chunk=512 <file>
* 'lfs getstripe' to show compress component parameters
Display component's compression parameters if possible.
--compress-type|--compr-type
Print only the compress type if possible.
--compress-level|--compr-level
Print only the compress level if possible.
--compress-chunk|--compr-chunk
Print only the compress chunk size in KiB if possible.
Example:
$ lfs getstripe <file>
lcme_compr_type: lz4
lcme_compr_lvl: 5
lcme_compr_chunk_kb: 512
lmm_pattern: raid0,compress
Signed-off-by: Bobi Jam <bobijam@whamcloud.com>
Signed-off-by: Artem Blagodarenko <ablagodarenko@ddn.com>
Signed-off-by: Marc Vef <mvef@whamcloud.com>
Change-Id: Ife0382469cbc5099e0c6dc96534bb169ddeff61e
(minor) the date should be updated
LU-10026 utils: add 'lfs find' support for compressed file * Add "--comp-flags=[^]compress" to locate file with/without compressed components. * Add "--comp-flags=[^]nocompr" to locate file with/without setting component compress preference. * Add "[!] --layout=compress" to locate file with/without compressed components. * Add "[!] --compress-type=<compress-type>" to locate compressed file with/without specified compress algorithm. * Add "[!] --compress-level=[+-]<compress-level>" to locate compressed file with/without specified compress level. * Add "[!] --compress-chunk=[+-]<compress-chunk>" to locate compressed file with/without specified compress chunk in KiB. utils: add -Z option for 'lfs getstripe/find' Add support for "lfs getstripe -Z" to get the last instantiated component compression information. Add support for "lfs find -Z <type>[:[+-]<level>]" to keep consistent options with "lfs setstripe -Z". DDN-bug-id: EX-6856 Was-Change-Id: Ia5e2a2ea6937dc2b46e224cec808504a196b974c utils: logical AND for 'lfs find' compression exprs All search expressions provided to 'lfs find' must be combined as a logical AND. Fix newly added options for compression support, so that they comply with this logical AND. DDN-bug-id: EX-7714 Was-Change-Id: I3b28cd87c1d304df6d04753b413d46f5abcfe16e csdc: don't set compression layout when disabled When llite_enable_compression is disabled (lfs set_param llite.*.enable_compression=0), we should check it before sending it to MDS lest we get a file with compressed component which we cannot handle. DDN-bug-id: EX-7593 Was-Change-Id: Ib1e2123ffdb239c3e1401d682ae9c2c49e3f4a6f utils: support 'lfs find --printf %LZ' Add support for "lfs find --printf %LZ" to print the compression type:level of the last instantiated component of a file. DDN-bug-id: EX-6856 Was-Change-Id: Iaf1b6c031b06c70e7b5be51354697aa6bdcc9850 csdc: right error code in case compression is disabled Currently, if compression is disabled on a client the error message is confusing "Cannot set layout EA: Unknown error 524" Let's replace the error code from ENOTSUPP to EOPNOTSUPP, so error messages become more informative ctl get_param -n llite.*.enable_compression 0 lfs setstripe -i 0 -c 1 -E -1 -Z lz4 /mnt/lustre/foo.txt Cannot set layout EA: Operation not supported lfs setstripe: cannot create composite file '/mnt/lustre/foo.txt': Inappropriate ioctl for device This message could be even more informative, but this version is already much better. DDN-bug-id: EX-10577 Was-Change-Id: I7fd67d5d3d20bc7de169f35f6fa63b53f2bfe718 Signed-off-by: Bobi Jam <bobijam@whamcloud.com> Signed-off-by: Artem Blagodarenko <ablagodarenko@ddn.com> Signed-off-by: Marc Vef <mvef@whamcloud.com> Change-Id: Ic36946738c3463fd862aeca4ee2e2c2ed85eff84
(style) line length of 83 exceeds 80 columns
(style) "contains" -> "has"
LU-10026 csdc: prefer uncompressed mirror for read When accessing a mirrored file with both compressed and uncompressed components, choose uncompressed components for read. csdc: prefer uncompressed mirror for write When writing to mirrored files with both compressed and uncompressed mirrors, prefer the uncompressed components to write, and that is better for performance, more compatible with older clients, and better fits the model of compressing files after initial write. DDN-bug-id: EX-6510 Was-Change-Id: I62a117d5cc3d34e2c0c96d1a9ade8eef0a2d1291 Signed-off-by: Bobi Jam <bobijam@whamcloud.com> Signed-off-by: Artem Blagodarenko <ablagodarenko@gmail.com> Signed-off-by: Marc Vef <mvef@whamcloud.com> Change-Id: I043b27bd891c039901075a08c76630f8f0f9f182
Does this now match where these fields appear when the file is auto-generated?
Is there a reason this was moved? It just seems like needless churn.
LU-10026 csdc: add ll_compr_hdr structure It is an important structure for CSDC feature. Will be used in the next patches. Also adds LL_COMPR_TYPE_UNCHANGED enum value (used to signal that the compression type should not be changed on update) and the LL_LZ4FAST_DEF_LEVEL define (default level for the LZ4FAST algorithm). Signed-off-by: Artem Blagodarenko <ablagodarenko@ddn.com> Signed-off-by: Marc Vef <mvef@whamcloud.com> Change-Id: I26f2fc7c0212f1a4cad36805c75aa766d6aed548
| unique failing test | history |
|---|---|
| sanity-hsm@ldiskfs+DNE:test_251 | seen in 1 other review |
| sanity-hsm@ldiskfs+DNE:test_254b | NEW unique failure for this branch in the last 30 days, and was seen 0 times across 0 other branches 0 reviews |
| sanityn@ldiskfs+DNE:test_51c | seen in 3 other reviews |
And what is this part of the patch about?
static int osd_sync(const struct lu_env *env, struct dt_device *d)
{
int rc;
struct super_block *s = osd_sb(osd_dt_dev(d));
ENTRY;
down_read(&s->s_umount);
rc = s->s_op->sync_fs(s, 1);
up_read(&s->s_umount);
CDEBUG(D_CACHE, "%s: synced OSD: rc = %d\n", osd_dt_dev(d)->od_svname,
rc);
return rc;
}
It used to be like this
And further on in ldiskfs_sync_fs() there is still flush_workqueue(sbi->s_misc_wq);
(minor) this version is outdated
LU-16032 osd: move unlink of large objects to separate thread
Final unlink and freeing of blocks for large objects can lead to
a thread hung with this call stack:
Net: Service thread pid 1739 was inactive for 200.16s.
The thread might be hung, or it might only be slow and will
resume later.
Dumping the stack trace for debugging purposes:
__wait_on_buffer+0x2a/0x30
ldiskfs_wait_block_bitmap+0xe0/0xf0 [ldiskfs]
ldiskfs_read_block_bitmap+0x31/0x60 [ldiskfs]
ldiskfs_free_blocks+0x329/0xbb0 [ldiskfs]
ldiskfs_ext_remove_space+0x8a9/0x1150 [ldiskfs]
ldiskfs_ext_truncate+0xb0/0xe0 [ldiskfs]
ldiskfs_truncate+0x3b7/0x3f0 [ldiskfs]
ldiskfs_evict_inode+0x58a/0x630 [ldiskfs]
evict+0xb4/0x180
iput+0xfc/0x190
osd_object_delete+0x1f8/0x370 [osd_ldiskfs]
lu_object_free.isra.30+0x68/0x170 [obdclass]
lu_object_put+0xc5/0x3e0 [obdclass]
ofd_destroy_by_fid+0x20e/0x500 [ofd]
ofd_destroy_hdl+0x267/0x9f0 [ofd]
tgt_request_handle+0xaee/0x15f0 [ptlrpc]
ptlrpc_server_handle_request+0x24b/0xab0 [ptlrpc]
ptlrpc_main+0xb34/0x1470 [ptlrpc]
kthread+0xd1/0xe0
Let's move final unlink to workqueue if inode size > 1GB. The size
threshold be configured by setting the minimum async truncate size
with the "osd-ldiskfs.*.delay_unlink_mb" parameter.
Writes to "osd-ldiskfs.*.force_sync" parameter will flush pending
delayed unlinks so that space can be reclaimed as needed.
Lustre-change: https://review.whamcloud.com/47995
Lustre-commit: a772e90243ea0ff1de6ae9c67e1f6384c431d200
Change-Id: Id535ae4c58732769effabee42835bc2da8cb5cc1
Signed-off-by: Artem Blagodarenko <ablagodarenko@whamcloud.com>
DDN-bug-id: DDN-3144
Reviewed-by: Andreas Dilger <adilger@whamcloud.com>
Reviewed-by: Alex Zhuravlev <bzzz@whamcloud.com>
Signed-off-by: Etienne AUJAMES <eaujames@ddn.com>
LU-19833 acl: fix generation of changelogs with ACLs
Adding, modifying, or removing ACLs on files did not generate a
changelog. Only changes to default ACLs were generating a changelog.
Now a changelog is always generated.
Fixes: 7b3bfb09dbcc ("LU-1304 mdd: changes related to acl")
Signed-off-by: Gauthier EVRAERD <Gauthier.EVRAERD@cea.fr>
Change-Id: Ice29e0d8b00f41abe94135e223d1b6fcb314ba91
I have no objections about the patch, but looking to testing sesssion log, sanity-compr hasn't been executed == sanity-compr test 1000: compressed vs uncompressed allocation ========================================================== 22:20:15 (1784931615) CMD: trevis-152vm120 lctl get_param -n osd*.*OST0000*.extents_dense trevis-152vm120: error: get_param: param_path 'osd*/*OST0000*/extents_dense': No such file or directory pdsh@trevis-152vm90: trevis-152vm120: ssh exited with exit code 2 SKIP: sanity-compr test_1000 no dense writes supported I think it is imported for changed test to be executed, so I set. -1 now, but this is not about the patch, but about incomplete testing.
Very strange. The test ran 50 iterations without issues in v5 of the patch, and v6 was just a simple rebase. I have the `extents_dense` parameter on my local system, so it isn't clear why it wouldn't exist for this test run... It also worked fine for 20 ldiskfs sessions for Janitor testing: https://testing.whamcloud.com/gerrit-janitor/66667/results.html
Done
The session was skipped again. I will push an updated patch with more debugging in this case so we can see why it is failing.
It looks like ext4-mballoc-dense.patch was not included in the el9 and el10 series because the sanity-compr.sh test case itself is skipped when `osd-ldiskfs.*.extents_dense` is not present, and that is conditional upon `LDISKFS_GET_BLOCKS_VERY_DENSE` being added by the patch. This meansthis testing is not being run on newer kernels which have become the new default for master test sessions. This is one reason why it is better to have test cases check the version and fail with an error if some feature is missing, though it is OK to run the test if the feature is present on older servers.
(defect) `$OST1_ID` and `$OST1_VERSION_ID` are never set, so this prints "port ext4-mballoc-dense.patch to " with nothing after "to".
`lustre_os_release ost1` exports the `${FACET}_OS_` prefixed names:
OST1_OS_ID, OST1_OS_ID_LIKE, OST1_OS_VERSION_ID
so `$OST1_OS_ID$OST1_OS_VERSION_ID` would give e.g. `rhel9.4`.
This is the path that actually runs today: ext4-mballoc-dense.patch is only listed in the ldiskfs-4.18-rhel8.* series, so on an el9/el10/SLES OST `$dense` is empty while the fstype and version guards above both pass. Losing the platform name here defeats the point of the extra debugging.
(minor) Not something this patch introduces, but since the threshold is being retuned and the counts are now printed: the read loop also counts the trailing filefrag summary line, so both $nr and $nonr come out one too high.
For "/mnt/lustre/f1000.sanity-compr: 1 extent found" the fields are $EX=path:, $PS="found", which is neither "ext:" nor contains "File", so it falls through to (( ${PS%%.*} == PREV + 1 )); "found" evaluates to 0 in arithmetic context and the counter is bumped. A perfectly contiguous dense file therefore reports dense=2, not 1.
That bias is what makes nonr/nr >= 2 stricter than intended exactly where it matters - at true 128 vs 256 the measured ratio is 129/257, which integer-divides to 1 and fails. Skipping lines whose second field is not an offset (or matching "extent[s] found") would fix both the check and the new debug line.
LU-17761 tests: fix sanity-compr/1000 ratio/repeats
The sanity-compr.sh test_1000 fails intermittently on an aged
filesystem or if run in a loop. It should have a ratio of
allocated extents between non-dense and dense files at least 2,
but this was failing
Create larger files to give more time for allocator to work
and write them as sparse files to better exercise the code.
Change calculation to allow better comparison of the ratio.
Allow files to be created on multiple OSTs to avoid contention
on the block allocator of a single OST when test is looping.
Wait for unlinked objects to be destroyed between runs.
Add more debugging in case test fails again in the future.
Test-Parameters: trivial
Fixes: 686dee707f ("LU-10026 osd-ldiskfs: use preallocation for dense writes")
Signed-off-by: Andreas Dilger <adilger@thelustrecollective.com>
Change-Id: Ib4f4a906e335f36d0768824948f0ab14049f97d3
| unique failing test | history |
|---|---|
| sanity-scrub@ldiskfs+DNE:test_5 | seen in 1 other review |
(minor) better to write "records" to avoid confusion with "reqs" (which I thought was just a typo)
is this safe to do if transno is 0? Would we call commit callback on it even though the opd_sync_rpcs_in_progress was already decreased in line 656 above?
(style) CDEBUG does not rate-limit console messages, use CDEBUG_LIMIT
(defect) should use CERROR() to avoid spamming the console log
LU-19810 osp: cancel recs if can't repeat rpc
if osp can not repeat an RPC, then it's better to corresponding
record, otherwise that record will pin the plain llog file and
even worse - catalog wrapping will be impossible.
Fixes: 27f787daa7 ("LU-15737 ofd: don't block destroys")
Signed-off-by: Alex Zhuravlev <bzzz@whamcloud.com>
Change-Id: I8a939deaf359267c8a4809cf64524100f17186dd
| failed enforced test | platform | detail | |
|---|---|---|---|
| custom-1001 | RHEL 8.10 / x86_64 | ran 3 tests. 1 tests failed: sanity-compr. | session |
| review-dne-subtest-change | RHEL 8.10 / x86_64 | ran 3 tests. 1 tests failed: sanity-compr. | session |
Now, with the patch reverted test system shows compr-sanity result as is. Don't hide fails as here https://testing.whamcloud.com/test_sets/02c83a33-7992-44a5-813f-3ab3428728d4 So, should be reverted. But we still need to solve cleanup problem somehow.
Yes, it will now be extra work to resolve all of these regressions in the testing. There are 13 subtest failures in the other test run for this patch:
https://testing.whamcloud.com/test_sets/8a522653-a82c-44b4-9e55-2b5d662f8517
It looks like several of the issues were introduced by landing patch https://review.whamcloud.com/53651 ("LU-16904 tests: use 2-component layout in sanity-compr") after it was rebased to ignore the errors returned by sanity/sanityn.
- test_27M: Error: '(13) pool found: test_27Ia'
- test_44f: Error: 'multiop failed'
- test_56wc: Error: 'long option without argument not supported'
- test_56xb: Error: 'hard links individually migrated (0 != 11)'
- test_56xd: Error: 'cannot migrate /mnt/lustre/d56xd.sanity/f56xd.sanity.mgrt with --yaml /mnt/lustre/d56xd.sanity/yamlfile'
- test_56xe: Error: 'cannot migrate /mnt/lustre/d56xe.sanity/f56xe.sanity by lfs migrate'
- test_56xf: Error: 'cannot migrate /mnt/lustre/d56xf.sanity/f56xf.sanity by lfs migrate'
- test_56Ec: Error: 'setstripe did not set OST index correctly'
- test_65r: Error: 'lfs setstripe --copy failed'
- test_101i: Error: 'expected misses 5 but got 6'
- test_184d: Error: 'lovea -E 1048576 -c 1 -S 1048576 -L raid0 -i 0 -E -1 -c 1 -S 4194304 -L raid0 -i -1 != -E 1048576 -c 1 -S 1048576 -L raid0 -i 0 -E -1 -c 1 -S 4194304 -L raid0 -i 1'
- test_270h: Error: 'Can't create file from OST|DOM mirror layout'
- test_cleanup: Error: 'test_sanity failed with 1'
- test_1000: Error: 'dense (25) should have less extents (87)'
The custom test session for that patch showed 9 of the same 13 errors:
https://testing.whamcloud.com/test_sets/4786296b-c82d-4222-b2d4-76b997d16a1c
My preference would be to fix these errors, since I suspect most of them relate to incorrect assumptions in the tests (e.g. test_56*) and not actual code bugs or regressions.
I think it is important to get this script working properly before the CSDC patches get too far along to ensure that this feature is working properly as it lands to master. I don't _think_ it was this way on b_es6_0, but I don't know for sure.
Yes, it will now be extra work to resolve all of these regressions in the testing. There are 13 subtest failures in the other test run for this patch:
https://testing.whamcloud.com/test_sets/8a522653-a82c-44b4-9e55-2b5d662f8517
It looks like several of the issues were introduced by landing patch https://review.whamcloud.com/53651 ("LU-16904 tests: use 2-component layout in sanity-compr") after it was rebased to ignore the errors returned by sanity/sanityn.
- test_27M: Error: '(13) pool found: test_27Ia'
- test_44f: Error: 'multiop failed'
- test_56wc: Error: 'long option without argument not supported'
- test_56xb: Error: 'hard links individually migrated (0 != 11)'
- test_56xd: Error: 'cannot migrate /mnt/lustre/d56xd.sanity/f56xd.sanity.mgrt with --yaml /mnt/lustre/d56xd.sanity/yamlfile'
- test_56xe: Error: 'cannot migrate /mnt/lustre/d56xe.sanity/f56xe.sanity by lfs migrate'
- test_56xf: Error: 'cannot migrate /mnt/lustre/d56xf.sanity/f56xf.sanity by lfs migrate'
- test_56Ec: Error: 'setstripe did not set OST index correctly'
- test_65r: Error: 'lfs setstripe --copy failed'
- test_101i: Error: 'expected misses 5 but got 6'
- test_184d: Error: 'lovea -E 1048576 -c 1 -S 1048576 -L raid0 -i 0 -E -1 -c 1 -S 4194304 -L raid0 -i -1 != -E 1048576 -c 1 -S 1048576 -L raid0 -i 0 -E -1 -c 1 -S 4194304 -L raid0 -i 1'
- test_270h: Error: 'Can't create file from OST|DOM mirror layout'
- test_cleanup: Error: 'test_sanity failed with 1'
- test_1000: Error: 'dense (25) should have less extents (87)'
The custom test session for that patch showed 9 of the same 13 errors:
https://testing.whamcloud.com/test_sets/4786296b-c82d-4222-b2d4-76b997d16a1c
My preference would be to fix these errors, since I suspect most of them relate to incorrect assumptions in the tests (e.g. test_56*) and not actual code bugs or regressions.
I think it is important to get this script working properly before the CSDC patches get too far along to ensure that this feature is working properly as it lands to master. I don't _think_ the sanity/sanityn error return was ignored this way on b_es6_0, but I don't know for sure.
Revert "LU-17761 tests: make sanity-compr sanity/sanityn return 0"
This reverts commit 1fcfd6e62e80673ecadda43e22892867bd60fd02.
If the sub-test error return from sanity or sanityn is not returned,
the errors are logged in the test output, but Maloo considers testing
as passed. This hides test failures, since passed tests are not seen.
This same issue was previously seen and fixed in sanity-dom.
Change-Id: Ide240fbd98e2120ca5c203ecfc913632fc2fb93a
Test-Parameters: trivial testlist=sanity-compr
Fixes: 1fcfd6e62e ("LU-17761 tests: make sanity-compr sanity/sanityn return 0")
Signed-off-by: Andreas Dilger <adilger@thelustrecollective.com>
I think if s_mb_cX_failed_ttl[cr] == 0 then it will be decreased to the MAX_LONG value and the feature is not deleted, but endless
Ok, the value is signed and check < 0, so it should work.
LU-19023 ldiskfs: mballoc cr loops optimisation Remember cr loop allocation failures and skip the loops for requests of the same or bigger size. Reset the values after a tunable number of loop skips. This commit adds ext4-mballoc-optimise-cr-loop patch to rhel9.2, rhel9.4, rhel9.5 ldiskfs patch series. Running "fallocate -o 0 -l 100G biggile" on a filesystem where each block group has 8% used and 92% free space: unpatched: 84.3s patched: 1.1s HPE-bug-id: LUS-12311 Signed-off-by: Alexander Zarochentsev <alexander.zarochentsev@hpe.com> Change-Id: I70d7198473c3a59358009a33a08db12da0c2510a
| failed enforced test | platform | detail | |
|---|---|---|---|
| custom-101 | CentOS 8.3/x86_64 | ran 1 tests. 1 tests failed: node-provisioning. %% NODE-PROVISIONING AND/OR LUSTRE-INIT FAILED MULTIPLE TIMES | session |
| review-dne-zfs-part-1 | CentOS 8.3/x86_64 | ran 4 tests. 2 tests failed: sanity-pfl, sanity. | session |
LU-9883 tests: restore sanity_407 Transaction errors are checked better after LU-8411/MRP-3609, so sanity_407 was disabled because it leaves filesystem in broken state. Make sanity_407 to work same as before LU-8411/MRP-3609 i.e check only remote transaction errors. Change-Id: Ieeb519fe0af478f53435dec9b784d120bae9465f Cray-bug-id: LUS-2492 Test-Parameters: testlist=sanity/407 Signed-off-by: Andriy Skulysh <c17819@cray.com> Reviewed-by: Artem Blagodarenko <c17828@cray.com> Reviewed-by: Andrew Perepechko <c17827@cray.com> Tested-by: Elena Gryaznova <c17455@cray.com>
| unique failing test | history |
|---|---|
| sanity-lnet@zfs:test_450 | NEW unique failure for this branch in the last 30 days, and was seen 0 times across 0 other branches 0 reviews |
The trash path is `.lustre/.ltrash/MDTXXXX` (`dot_ltrash_name` is ".ltrash"), not `.lustre/trash/MDTXXXX`. Same in the body and in the new mdd_trash.c comment. This was raised on patchset 10 and marked Done, but only lu_object.h was corrected.
Which mount option is this? `mdt_trash_init_on_startup` has no assignment anywhere in the tree - there is no `LMD_FLG_*` bit, no `lmd_parse()` handling, no `mdt_init0()` assignment, and nothing in mount_lustre.c. So the bit is always 0 and `mdt_trash_setup()` in `mdt_postrecov()` can never run. Compare `mdt_skip_lfsck`, which is wired up in mdt_handler.c from `LMD_FLG_SKIP_LFSCK` and documented in lustre/utils/mount_lustre.c. Should the option be added here, or should the sentence be dropped and the startup path removed until a later patch adds it?
A few hunks aren't accounted for in the description - are they all meant to be here? - the new lustre/mdd/mdd_trash.c and its Makefile entry - splitting `lu_generic_thread_stop()` into `lu_generic_thread_wait(thread, abort)` in obdclass - `complete()` -> `complete_all()` in `mdt_trash_setup_thread()` - the new `dd_rdonly` early return in `mdt_trash_setup()`
(style) `Change-Id:` should come after `Signed-off-by:`. The ordering suggests the Lustre commit hooks aren't installed locally.
(defect) The two implementations of this op don't agree on what the arguments mean. `mdd_trash_init()` treats both as required - it does `mdd_object_find(env, mdd, fid)`. `mdt_trash_can_init()` ignores both and just kicks off the whole setup thread. The only caller that goes through the op pointer (trash_can_enable_store) passes `NULL, dev, NULL`, which would crash immediately if it ever landed on the MDD implementation. So the kerneldoc above describes the MDD contract only. Would two separate ops be clearer - one "start trash setup" on the top device and one "cache this trash FID" on the child - rather than one op with two meanings?
error: mdd_trash_init():'mdd' dereferencing possible ERR_PTR()
error: mdd_trash_init():'mdd' dereferencing possible ERR_PTR()
(style) This collides with `mdd_dot_lustre_objs.mdd_trash` a few lines above, which is a different object - that one is `.lustre/.ltrash`, this one is `.lustre/.ltrash/MDTXXXX`. Something like `mdd_trash_dir` would keep the two apart.
(defect) `ldo_trash_init` isn't checked before the call, and `ls_top_dev` isn't the MDT for the whole life of the device. `lu_site_init()` sets `ls_top_dev` to the OSD device; mdt_stack_init() only repoints it to the MDT *after* `class_setup()` of the MDD has already run `mdd_procfs_init()` and published this attribute. A write in that window calls a NULL function pointer, since `osd_lu_ops` has no `.ldo_trash_init`. tgt_mount.c guards the analogous op with `if (dev && dev->ld_ops->ldo_prepare)` - worth doing the same here.
(style) `RETURN()` in a function with no `ENTRY`, and mixed with plain `return` on the other paths. Plain `return rc;` fits the rest of this file's sysfs handlers.
(typo) Path is `.lustre/.ltrash/MDTXXXX` - lu_object.h and the test both use `.ltrash`.
(defect) `mdt_trash_setup()` only reports whether the thread *started*, and `lu_generic_thread_wait()` discards the thread's own rc, so this returns 0 even when the trash was never created: - `mdt_trash_setup()` returns 0 early for `dd_rdonly` - the thread bails with -ENOMEM at `OBD_ALLOC_PTR(env)` or on `lu_env_init()` failure The caller in mdd_lproc.c then sets `mdd_trash_can_enable = true` with `mdd_trash_inited` still false and `mdd->mdd_trash` NULL. A retry can't recover either: `mdt_trash_setup()` returns 0 immediately because `lgt_init` is set, and `complete_all()` leaves the completion permanently signalled, so the wait returns at once. `trash_can_enable` then reads 1 for the rest of the mount with no trash object behind it. Should the thread's result be plumbed back, e.g. via `lgt_data` or a new rc field?
LU-18456 tcu: delay trash creation ".lustre/trash/MDTXXXX" In the previous patch, the global visible trash dir ".lustre/trash/MDTXXX" is created immediately when a server finished MDT stack setup and recovery. However, this may result in sanity-scrub.sh/test_4b failure. The creation and initialization of the Trash Can dir will trigger the OI scrub fixing and update the scrub accounting and result in "prior_updated" checking failed. In this patch, the Trash Can initialization will be disabled by default at the server startup time. Or it can be enabled via a MDT mount option. In the meanwhile, we improve the tunable "mdd.*.trash_can_enable". When Trash Can is enabled, the server will check whether it is initialized. If not, the server will launch a thread to create and initial the Trash Can dir. Add a test case sanityn/test_117a. Change-Id: I9989bb8530d492ed063dc8106ac34be8ee80643e Signed-off-by: Yingjin Qian <qian@ddn.com>
(minor) The body describes the trash directory creation but not two other pieces of the diff: mdd_generic_thread_start()/stop() being renamed, moved to obdclass and exported as lu_generic_thread_start()/stop(), and mdt_fid_alloc() moving from mdt_restripe.c into mdt_internal.h. Could those be mentioned here, or split into their own preparatory patch?
(typo) "precedure" -> "procedure".
(style) Change-Id: should come after the Signed-off-by: lines. Installing contrib/git-hooks/commit-msg locally puts it in the right place automatically.
(minor) This adds a new wire constant (FID_OID_DOT_LUSTRE_TRASH) and a new object in the .lustre namespace, so a `Test-Parameters:` line asking for interop coverage would be useful here - e.g. a `serverversion=` run so an older MDT0 talking to a new MDT1 (and the reverse) is exercised, since the parent .ltrash only exists on MDT0 once this lands.
(minor) dot_trash_name has no users anywhere in this patch. Since UAPI names are hard to take back once published, would it be better to add it in the patch that first uses it?
(minor) dot_trash_name (".Trash") is added but not referenced anywhere in this patch. Is it meant for a later change in the series? If so it could be added there instead.
(style) Review error messages to ensure they are useful, and not just status/debug
(style) can use single quotes `'fid'` without backslash instead of double wuotes
Will be fixed next patch.
(style) Review error messages to ensure they are useful, and not just status/debug
(style) Can use angle quotes
Will be fixed next patch.
(minor) mdd_object_init() already assigns mo_dir_ops = &mdd_dir_ops, so this line is a no-op. The obf and lpf setup functions install *different* ops here, and in particular lost+found gets mdd_lpf_dir_ops with dummy create/rename/link/unlink so it can't be modified through the namespace. Is leaving .ltrash with the full mdd_dir_ops intentional, given the per-MDT trash dirs underneath it are server-managed? Also, mdd_dot_lustre_objs.mdd_trash is set and released but never read - presumably a later patch in the series uses it?
This should be ".lustre/.ltrash/MDTXXXX".
(style) "for trash dir" is redundant and could be removed to fit comment on one line
Fixed.
(style) this comment found now fit in a single line
Will be fixed next patch.
this really doe not add much because we are goign to crash in the next line anyway trying to dereference it.
I've referenced some other call of "lu_context_key_get", and seems it is normal to put a 'LASSERT' here. So I think this should be ok.
I mean info->xxx = yyy jsut below would crash with a NULL pointer so asserting that info is not NULL is useless and kernel guidelines say not to include such asserts (and we are tryign to remove the existing useless ones like this with mixed success)
Got it. So just remove it. It will be done.
(defect) lum_stripe_offset is stored little-endian and read back with le32_to_cpu() - see lod_ah_init() in lod_object.c - but this assigns the native value, while lum_magic two lines up is converted with cpu_to_le32(). mdt_auto_split_prep() in mdt_restripe.c converts both. On a big-endian MDS index 1 becomes 0x01000000, lod_declare_create() then finds no MDT matching ldo_dir_stripe_offset and fails the create.
LU-18456 mdt: create trash dir for MDT after MDT stack setup In this patch, the trash directories corresponding to various MDTs are created in mdt_postrecov(). The trash directory should be visible in Lustre namespace by the path ".lustre/.ltrash/MDTXXXX". The ".lustre/.ltrash/MDT0000" is a local trash directory for MDT0. However, for the trash directories ".lustre/.ltrash/MDTXXXX" corresponding to MDTs other than MDT0, they are all remote directories with parent ".lustre/.ltrash" (FID: LU_TRASH_FID) on MDT0. Moreover, the trash directory is created in an extra thread in ->o_postrecov() to avoid blocking the MDT stack setup and recovery. The reasons are as follows: - all devices belonging to the MDT stack are configured and setup properly. - The recovery precedure is finished, and the server becomes ready to handle the normal RPCs (i.e. create remote directory for a trash directory). Change-Id: Ibf2598f6b0e1b543755d68ffeb669c19be4bf018 Signed-off-by: Qian Yingjin <qian@ddn.com> Signed-off-by: Xiyang Wang <xiwang@ddn.com>
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-ldiskfs-dne-arm | RHEL 8.10 / x86_64, Rocky 9.5 / aarch64 | ran 5 tests. 1 tests failed: sanity. | session |
LU-17044 ptlrpc: tbf cleanup ahead of minrate Non-functional janitorial preparation for minrate floor scheduling: - Add struct nrs_tbf_token_info to hold per-bucket token accounting fields - Move existing token state fields into tc_max in nrs_tbf_client - Rename tr_rpc_rate/tr_depth to tr_max_rpc_rate/tr_max_depth in nrs_tbf_rule - Rename ts_rpc_rate/tc_rpc_rate to ts_max_rpc_rate/tc_max_rpc_rate in commands Test-Parameters: trivial Test-Parameters: testlist=sanityn Signed-off-by: Chris Horn <chris.horn@hpe.com> Change-Id: I23c5309e1e2fa13001d480f53239b0ca24e83d26
LU-18456 mdd: move tree with multiple levels into trash In this patch, we implement the functionality to move deleting tree with multiple levels into Trash Can. Add sanityn/test_117c to verify it works as expected. Change-Id: I528e7c800a7ab75e8f2e594f43cfa8ad306a816f Signed-off-by: Qian Yingjin <qian@ddn.com>
LU-18456 tcu: add option to set trash can type Add a tunable mdd.*.trash_can_type to set trash can type. Currently three trash can types are supported: - "plain": Plain Trash Can The deleting files are moved into "Trash/MDTXXXX" directly; - "uid": Per-User Trash Can A per-user ".ltrash/MDTXXXX/UID/" directory that is owned by that UID and mode 0700 should always be created in the top-level directory to avoid world readable access to deleted files. That avoids tracking space usage more clearly for each UID, so that a user's data can be found and purged more quickly if they are exceeding their quotas. - "nodemap": Per-Tenant Trash Can Files and directories deleted from within a subdirectory mount of a Nodemap are stored in a ".ltrash/MDTXXXX/NODEMAP/UID/" directory to isolate the files/directories from different tenants. Test-Parameters: trivial Change-Id: I02c8d39c3c45239835e70aa44d59f6a90bc06bc6 Signed-off-by: Yingjin Qian <qian@ddn.com>
Tim, there is a patch later in the series that allows configuring the .Trash name, since this is a virtual directory and could potentially be anything.
I know. And I understand, in theory, why we might want to prevent users from creating files with name .Trash. If you enable trashcan and populate every directory with virtual .Trash files, the user created file will be invisible. But I'm not sure that this patch solves this very well. Especially since the name that we have to block can change via configuration. You could switch the name to .Whatever, create a real .Trash file, then swap it back. Lustre would have to do something sane here, even if it's a bit silly. If we have to deal with that edge case anyway, why add this check at all?
You don't explain why files with .Trash are forbidden and why this can't be solved in a way that won't break applications that might want to write a .Trash file.
LU-18800 tcu: forbid to create a file with name ".Trash" It is forbidden to create a file with the target name of ".Trash" via the system calls such as mkdir()/create()/mknod/rename()/ link()/symlink(). Add a test case sanityn/117i to check it. Change-Id: I606e2f0f57886025ba6e99b5f9213c6ba98131b4 Signed-off-by: Yingjin Qian <qian@ddn.com>
LU-13920 hsm: process hsm_actions only after mdd setup
There is no guarantee that MDD setup is finished
at the moment when coordinator is started by
config params processing. If MDD setup is not finished,
hsm actions llog is not inited(mdd_hsm_actions_llog_init).
Hence hsm_pending_restore will not be called, i.e.
RESTORE requests will be sent to agents without taken
layout locks. I believe it may cause different problems.
I faced at least a kernel panic when llog includes
RESTORE request in ARS_WAITING that hasn't bee sent to
agent before failover. And the 2nd one RESTORE request
to the same fid was resent after recovery. Finally
agent handles to RESTORE to the same FID in parallel
that resulted in a panic with following bt:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000040
IP: [<ffffffffc0b03bec>] thandle_get_sub_by_dt+0x14c/0x420 [ptlrpc]
...
[<ffffffffc1202732>] lod_sub_get_thandle+0x2f2/0x400 [lod]
[<ffffffffc1205021>] lod_sub_declare_xattr_set+0x61/0x300 [lod]
[<ffffffffc11db0d5>] lod_obj_stripe_replace_parent_fid_cb+0x245/0x450 [lod]
[<ffffffffc11eae0e>] lod_obj_for_each_stripe+0x11e/0x2d0 [lod]
[<ffffffffc11ebfe2>] lod_replace_parent_fid+0x2a2/0x390 [lod]
[<ffffffffc11dae90>] ? lod_attr_get+0x110/0x110 [lod]
[<ffffffffc11f8faf>] lod_declare_xattr_set+0x24f/0xf70 [lod]
[<ffffffffc077b251>] ? lprocfs_counter_sub+0xc1/0x130 [obdclass]
[<ffffffffc1091ff4>] mdo_declare_xattr_set+0x74/0x2b0 [mdd]
[<ffffffffc077b129>] ? lprocfs_counter_add+0xf9/0x160 [obdclass]
[<ffffffffc0fa2f7b>] ? osd_trans_create+0xbb/0x620 [osd_ldiskfs]
[<ffffffffc1094903>] mdd_declare_xattr_set+0x33/0x70 [mdd]
[<ffffffffc1094b4e>] mdd_object_pfid_replace+0x7e/0x1e0 [mdd]
[<ffffffffc109c2c6>] mdd_swap_layouts+0xa76/0x1dc0 [mdd]
[<ffffffffc10a5e1a>] ? mdd_trans_stop+0x3a/0x174 [mdd]
[<ffffffffc114b489>] hsm_cdt_request_completed.isra.14+0xc89/0xf50 [mdt]
[<ffffffffc077b129>] ? lprocfs_counter_add+0xf9/0x160 [obdclass]
[<ffffffffc114d844>] mdt_hsm_update_request_state+0x544/0x7b0 [mdt]
[<ffffffffc0a82277>] ? lustre_msg_buf+0x17/0x60 [ptlrpc]
[<ffffffffc10fec92>] ? ucred_set_audit_enabled.isra.15+0x22/0x60 [mdt]
[<ffffffffc112b98f>] mdt_hsm_progress+0x1ef/0x3f0 [mdt]
[<ffffffffc0aefcfa>] tgt_request_handle+0x96a/0x1640 [ptlrpc]
[<ffffffffc06cca9e>] ? libcfs_nid2str_r+0xfe/0x130 [lnet]
[<ffffffffc0a91466>] ptlrpc_server_handle_request+0x256/0xb10 [ptlrpc]
[<ffffffffc0a95fbc>] ptlrpc_main+0xb3c/0x14d0 [ptlrpc]
[<ffffffffc0a95480>] ? ptlrpc_register_service+0xf90/0xf90 [ptlrpc]
[<ffffffffa04c1c31>] kthread+0xd1/0xe0
[<ffffffffa04c1b60>] ? insert_kthread_work+0x40/0x40
[<ffffffffa0b76c37>] ret_from_fork_nospec_begin+0x21/0x21
[<ffffffffa04c1b60>] ? insert_kthread_work+0x40/0x40
Code: 74 29 4c 3b a0 50 ff ff ff 75 e4 4d 85 ed 74 1b bf 01 00 00 00 e8 c5 b8 ff ff 85 c0 0f 85 98 00 00 00 49 8b 45 00 e9 04 ff ff ff <49> 8b 44 24 40 48 8b 40 08 48 85 c0 0f 84 b3 02 00 00 4c 89 e6
RIP [<ffffffffc0b03bec>] thandle_get_sub_by_dt+0x14c/0x420 [ptlrpc]
Note, I faced this panic while testing https://review.whamcloud.com/#/c/38867/,
however I believe the same issue may exist even without 38867.
Patch makes mdt_hsm_cdt_start to wait until MDT initialization becomes
finished. Without this fix you should see below error in dmesg
each time after MDS restart if HSM is enbaled.
mdt_hsm_cdt_start()) lustre-MDT0000: cannot take the layout locks needed for registered restore: -2
Lustre-change: https://review.whamcloud.com/40028
Lustre-commit: a558006b83dfe32798cce644aa888c37e805d50b
LU-14399 hsm: process hsm_actions in coordinator
Wait for mdd setup in a separate thread to don't block mount.
The patch adds conf-sanity_131 to verify the fix.
Lustre-change: https://review.whamcloud.com/41445
Lustre-commit e26d7cc3992252e5fce5a51aee716f933b04c13a
Fixes: a558006b ("LU-13920 hsm: process hsm_actions only after mdd setup")
HPE-bug-id: LUS-9750
Test-Parameters: testlist=sanity-hsm
Change-Id: I4c4edaa72a562576ea71d89a4b60618d099ec4f5
Signed-off-by: Sergey Cheremencev <sergey.cheremencev@hpe.com>
Reviewed-by: Ben Evans <beevans@whamcloud.com>
Reviewed-by: Nathan Rutman <nrutman@gmail.com>
Reviewed-by: Nikitas Angelinas <nikitas.angelinas@hpe.com>
Reviewed-by: Alexander Boyko <alexander.boyko@hpe.com>
Reviewed-by: Artem Blagodarenko <artem.blagodarenko@hpe.com>
Signed-off-by: Etienne AUJAMES <eaujames@ddn.com>
LU-15262 osd: bio_integrity_prep_fn return value processing There is osd_bio_integrity_handle() fn in lustre/osd-ldiskfs/osd_io.c It checks the returned code of bio_integrity_prep_fn() but between mainstream Linux 4.12 and 4.13 kernel integrity API has changed and in 4.13+ (as well as for any RHEL8 including first beta) bio_integrity_prep() returns boolean true on success. Lustre-change: https://review.whamcloud.com/45646 Lustre-commit: 41c813d14ec9b353f9cf5ac82638996dcb5273d7 HPe-bug-id: LUS-10443 Signed-off-by: Alexey Lyashkov <alexey.lyashkov@hpe.com> Change-Id: I973aa8ccae024157ad863d26afc7b1264a5c7149 Reviewed-by: Andreas Dilger <adilger@whamcloud.com> Reviewed-by: Artem Blagodarenko <artem.blagodarenko@hpe.com> Reviewed-by: Li Dongyang <dongyangli@ddn.com> Reviewed-by: Andrew Perepechko <andrew.perepechko@hpe.com> Reviewed-by: Artem Blagodarenko <ablagodarenko@ddn.com>
what's zerooffset? we have zero hits in the entirety of the codebase and commit messages.
Did we ever get a confirmation this is not going to affect application api / old/new library linked apps problems?
This should be giving a checkpatch error.
I would assume that the old `LLAPI_LAYOUT_RAID0 = 0` value would also be accepted? Otherwise, this will introduce a compatibility issue. Even so, this means that applications using the new library would break with ones compiled using the old headers. Maybe a better option would be to add `LLAPI_LAYOUT_RAID0_SET = 0x0000040ULL` that can be set and checked in the code, but still keep the `LLAPI_LAYOUT_RAID0 = 0` value for compatibility for a few years.
(style) should use `#ifndef BIT`
(style) spurious blank line
(style) line length of 95 exceeds 80 columns
It isn't clear what these lines are for? They should be removed.
my bad. some debuging.
(style) line length of 89 exceeds 80 columns
This can be wrapped easily
it's in 80 chars i think.
Is this change correct? Swaps from overstriping to & raid0? Definitely a semantic shift?
(defect?) this should be in the same order as lov_pattern_names[], since `setstripe` doesn't produce any output, so the `getstripe` output is what matters.
(minor) there is now `llapi_convert_str2mask()` that implements this in userspace, like `cfs_str2mask()` in the kernel. It needs a simple `bit2str()` helper function, instead of reimplementing it for each set it bitmasks. It would probably make sense for `llapi_lov_string_pattern()` and `llapi_lov_pattern_string()' to be converted over to use that as well.
LU-18461 llapi: replace pattern with bitmask use a bitmask as llapi_pattern, to simplify overstripe / zerooffset handing. Fix usage a LOV_PATTERN_PARITY on the userspace (llapi) code. Signed-off-by: Alexey Lyashkov <alexey.lyashkov@hpe.com> Change-Id: I29a6943af1f58ed3834076d3bb3d618dd6b83983
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-dne-zfs-part-5 failed 2× crashed | RHEL 9.7 / x86_64 | ran 5 tests. 1 tests failed: sanityn. %% THIS TEST SESSION CRASHED %% | session |
The message says this implements "moving regular files into trash", but the code also moves directories: mdd_unlink() takes the trash path when `is_dir` is true, mdd_finish_unlink() reaches mdd_trash_insert() for `is_dir`, and mdd_trash_insert_obj()/mdd_trash_declare_insert() have explicit S_ISDIR handling that re-points ".." at the trash parent. test_117b asserts the directory case too, and its title says "and empty dir". Should the body describe the directory handling as well, or should the directory support be split into its own patch?
(typo) "verfiy" -> "verify".
mdd_object_trashable() only consults the tunable, with nothing excluding objects that already live under the trash directory. Unlinking `.lustre/.ltrash/MDTXXXX/<pfid>/f` calls mdd_trash_parent_find() with the `<pfid>` directory as the parent, which allocates a new trash parent named after *that* directory's FID and re-links the file there. That means files can be moved around inside the trash forever but never actually destroyed, so no space is ever reclaimed. How is the trash meant to be emptied - does this need a check that the parent is not itself inside the trash subtree?
(defect) A trash-insert failure here still fails the whole unlink, which was raised on patch set 14 and looks unchanged. The reply pointed at a separate ENOSPC change, but every other error still propagates. By the time this runs the transaction has already executed __mdd_index_delete() on the parent and mdo_ref_del() on the child. osd_trans_stop() has no rollback - it calls ldiskfs_journal_stop() regardless of `th_result` - so on a non-zero `rc` the name entry is gone and nlink is 0, but the object was never inserted into the trash and never added to PENDING by mdd_orphan_insert(). The inode and its OST objects are then unreferenced until LFSCK runs. Falling back to the normal orphan/destroy path when mdd_trash_insert() fails would avoid both the user-visible error and the leaked inode.
(defect) Does `mdd_ptobj` leak here? mdd_trash_parent_find() returns a held reference via mdd_object_find(), but the only mdd_object_put() is after the `stop:` label. Both of these returns bypass it:
rc = mdd_unlink_sanity_check(...);
if (rc)
RETURN(rc); /* mdd_ptobj still held */
handle = mdd_trans_create(env, mdd);
if (IS_ERR(handle))
RETURN(PTR_ERR(handle)); /* same */
mdd_may_delete() can fail for ordinary reasons - `-ENOTEMPTY` from mdd_dir_is_empty() for `rmdir` on a non-empty directory, `-EPERM` for sticky/immutable, `-EACCES` from the permission check - and the `is_dir` case always takes the trash lookup first. So a user loop of `rmdir` on a non-empty directory leaks one lu_object reference per call.
When the trash parent name is not yet present, mdd_trash_parent_find() also burns a freshly allocated FID via dt_fid_alloc() on each of those attempts, so each leaked object is a distinct one. A `goto stop`-style exit (or a put before each return) would cover both paths.
(minor) mdd_create_object() takes `mdd_write_lock(env, son, DT_TGT_CHILD)` internally, but mdd_unlink() already holds DT_TGT_CHILD on the object being unlinked when it reaches here through mdd_finish_unlink(). osd-ldiskfs has no `do_write_lock` op, so dt_write_lock() falls through to `down_write_nested(&dt->dd_sem, role)` - two nested acquisitions of the same lock class with the same subclass, which lockdep reports as possible recursive locking on CONFIG_PROVE_LOCKING kernels. Since the patch is already adding DT_TGT_TRASH for the trash directory, would a distinct role for the trash parent object work here too?
(defect) What happens when the same name is unlinked twice from the same directory? The trash parent is keyed on the parent FID and the entry keeps the original name, so:
touch d/f; rm d/f # -> trash/MDTXXXX/<fid-of-d>/f
touch d/f; rm d/f # -> dt_insert("f") into the same trash parent
The second dt_insert() hits the existing name and ldiskfs returns -EEXIST, which propagates out of mdd_finish_unlink() and fails the unlink. That is an ordinary user workload, not an error case.
The patch set 14 discussion said EEXIST would be handled by renaming and retrying internally, but there is no such handling in this function - the only thing here is the "TODO: error handling" comment below.
The object's linkEA is not updated when it is moved into the trash. mdd_finish_unlink() does not call mdd_links_del() on this branch (and there is no mdd_links_rename() here), so `trusted.link` still records the original parent FID and name while the only name entry now lives in the trash parent. That makes `lfs fid2path` on a trashed file report a path that no longer exists, and leaves an inconsistency for the LFSCK namespace scrub to report/repair. Note mdd_declare_finish_unlink() still declares mdd_declare_links_del() for this case, so the credits are reserved but nothing uses them.
(minor) Only `spec->u` is cleared, so the rest of `info->mdi_spec` keeps whatever the previous operation on this thread left behind - `sp_cr_flags`, `no_create`, `sp_cr_lookup`, `sp_cr_file_secctx_name`, `sp_cr_file_encctx`, `sp_cr_job_xattr`. mdd_declare_create_object() and mdd_create_object() branch on all of those (e.g. the unconditional `if (spec->sp_cr_file_encctx != NULL)` xattr set, and `mdo_declare_xattr_set(..., hsm_buf, XATTR_NAME_HSM, ...)` with the NULL `hsm_buf` passed in from here). mdd_rename() does `memset(spec, 0, sizeof(*spec))` before reusing mdi_spec for exactly this reason - worth doing the same here. Same idea for `__mdd_index_insert_only(env, trash, pfid, ...)` below: `pfid` is `info->mdi_fid`, which only still holds the right value because nothing between mdd_trash_parent_find() and here happens to touch it. `mdd_object_fid(parent)` says what is meant and cannot drift.
(minor) The trash parent is created with `la_mode = S_IFDIR` and no permission bits, and `la_valid` carries no LA_UID/LA_GID, so it lands as a mode 0000 directory owned by root. Only a caller with CAP_DAC_OVERRIDE can traverse it, so a user cannot reach their own deleted files. Compare mdd_trash_setup() in mdd_device.c, which uses `S_IFDIR | 0500` for `.lustre/.ltrash`. Since the commit message says this directory will later be renamed into place and inherit the deleted parent's attributes, would it be better to capture the original parent's mode/uid/gid here rather than start from zero?
(minor) The declare hardcodes S_IFDIR as the entry type while mdd_trash_insert_obj() passes `mdd_object_type(obj)`, which is a regular file for the case this patch is about. Should this use `mode` (already a parameter) so declare and execute agree?
(style) `pobj` is never used in this function. The prototype in mdd_internal.h also names the fourth parameter `ptobj` while the definition calls it `pt`; worth making them match if the patch is refreshed.
LU-18456 tcu: move regular files into trash upon last unlink "last unlink" moving into trash can will create a directory named after the parent FID in the trash dir for the corresponding MDT. Then, if/when the parent directory is also removed, the FID-named directory in trash will rename to use its original file name (and also inherit the actual FID or other xattrs like crypt, selinux, etc) from the now-deleted parent. This can avoid having to move all of the deleted files over to the "real" deleted parent. This patch implements the part of moving regular files into trash upon last unlink. Add sanityn/test_117b to verfiy it works as expected. Change-Id: Iec59fbb3719c52c4f84c97c74e40ee0cfadad75d Signed-off-by: Qian Yingjin <qian@ddn.com>
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-dne-zfs-part-1 failed 2× crashed | RHEL 9.7 / x86_64 | ran 3 tests. 1 tests failed: sanity. %% THIS TEST SESSION CRASHED %% | session |
(minor) The body documents the tunable as "mdd.*.trash_can_enabled", but LUSTRE_RW_ATTR(trash_can_enable) creates mdd.*.trash_can_enable (no trailing 'd'). A user copying the name from here would hit -ENOENT on `lctl set_param`. Please reconcile the message with the actual parameter name.
(minor) Placing this bool between mdd_dt_conf and the mdd_orphans pointer adds a padding hole. Grouping it with the other bool members would pack the struct better. This restates an unaddressed earlier-patchset review point; noting it in case the patch is refreshed.
LU-18456 mdd: add option to enable/disable Trash Can Add option "mdd.*.trash_can_enabled" to enable or disable Trash Can for a given MDT. Now it is disabled and will be enabled by default when it is stable. Change-Id: Ide7f06aab56168d44eef2534d88fe88d70c6c1f3 Signed-off-by: Qian Yingjin <qian@ddn.com>
(minor) `off` is computed at the call site as vma->vm_pgoff<<PAGE_SHIFT and passed in, but never used in the body. Drop the parameter, or use it.
(suggestion) The block is keyed on the file open mode, so an fd opened O_RDWR that only creates a PROT_READ mapping is rejected with -EOPNOTSUPP even though it never writes the file (a common open-O_RDWR-then-mmap-read pattern). Since read mmap of compressed files is supported, would keying on the mapping's write intent (vma->vm_flags VM_WRITE/VM_SHARED) rather than the open mode let those read-only mappings through while still avoiding the write SIGBUS?
(minor) The message names cl_io_rw_init(), but this path calls cl_io_init().
(defect) These treat O_RDONLY/O_WRONLY/O_RDWR as bitmask flags, but the access mode is a 2-bit field. O_RDONLY is 0, so `fd_flags & O_RDONLY` is always 0, and for an O_RDONLY fd `fd_flags & O_RDWR` is also 0, so mmap_mode stays 0 (PROT_NONE) instead of PROT_READ.
Mask and compare instead:
switch (fd_flags & O_ACCMODE) {
case O_RDONLY: mmap_mode = PROT_READ; break;
case O_WRONLY: mmap_mode = PROT_WRITE; break;
case O_RDWR: mmap_mode = PROT_READ | PROT_WRITE; break;
}
Because of this, the `oSMc` case in test_460e maps PROT_NONE, so "readonly mmap should work" passes without ever exercising a readable mapping.
LU-10026 llite: allow mmap reads of compressed files mmap reads of compressed files work, so we should only block writes. We cannot block the actual fault operations because that will cause the application to get a SIGBUS, so we check the file open mode when we go to create the memory mapping. llite: disable mmap on compressed files disable mmap(2) on compressed files until well tested. DDN-bug-id: EX-6265 Was-Change-Id: I3464a03b16708edcd0692bc9db337eb8473ea047 llite: use CIT_MISC in check_compression ll_mmap_check_compression uses a CIT_FAULT io type and initializes it with cl_io_rw_init. This doesn't initialize the IO correctly and this sometimes results in the following crash during io_fini, because the ft_page pointer is initialized to something else by cl_io_rw_init: BUG: unable to handle kernel NULL pointer dereference at 000000000000004a RIP: 0010:cl_pagevec_put+0x9f/0x3a0 [obdclass] vvp_io_fault_fini+0x21/0x40 [lustre] cl_io_fini+0x7a/0x230 [obdclass] ll_mmap_check_compression+0x403/0x540 [lustre] DDN-bug-id: EX-7812 Was-Change-Id: Ib1f270f071370d6c045bb9d799ab5b7b41a6c6be Test-Parameters: forjanitoronly Signed-off-by: Alex Zhuravlev <bzzz@whamcloud.com> Signed-off-by: Patrick Farrell <pfarrell@whamcloud.com> Signed-off-by: Artem Blagodarenko <ablagodarenko@ddn.com> Signed-off-by: Marc Vef <mvef@whamcloud.com> Change-Id: I636f398fd247ddcd153f94bf8116440540e8469c
| unique failing test | history |
|---|---|
| sanity-pcc@zfs:test_22 | seen in 4 other reviews |
the reason is now known and should be included here. Note for me to update later
(defect) The resend is rebuilt from the uncompressed arrays, so besides aa_requested_nob the niobuf/page counts also change (compression yields fewer niobufs), yet only aa_requested_nob is propagated back into `aa` before `new_req->rq_async_args = request->rq_async_args` overwrites new_aa. After the resend new_aa->aa_nio_count still holds the compressed count, while the reply's RMF_RCS vector is sized from the uncompressed `niocount` in osc_brw_prep_request().
check_write_rcs(req, aa->aa_requested_nob, aa->aa_nio_count, ...)
check_write_rcs() then only inspects the first aa_nio_count per-niobuf rcs, so a server-side write error on a niobuf past the compressed count would be skipped. aa_page_count is similarly stale. Should aa_nio_count (and aa_page_count) be updated here the same way as aa_requested_nob?
LU-10026 osc: do not compress resends There's some issue with doing compression on resent requests, so this patch works around it with two things: 1. Use the uncompressed page array for resend (this was always necessary unless we modified resend to know it already had compressed pages as input) 2. Disable compression on resend (not clear why 1. wasn't enough) csdc: Update async_args after resend It is decided to send an uncompressed request on redo. osc_brw_prep_request() processes uncompressed data and prepares a request, so some parts of the old request are outdated. Let's update the old request with information from the new one. DDN-bug-id: EX-8814 Was-Change-Id: Idb1c6ee9db64cb1f2ea1c1562b1c5aae443263e3 Test-Parameters: forjanitoronly Signed-off-by: Artem Blagodarenko <ablagodarenko@ddn.com> Signed-off-by: Patrick Farrell <pfarrell@whamcloud.com> Signed-off-by: Marc Vef <mvef@whamcloud.com> Change-Id: I5fbbdc2771f8c2c7b5c28f0b70d89b8b6015147f
| unique failing test | history |
|---|---|
| sanityn@ldiskfs+DNE:test_108a | seen in 14 other reviews |
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-dne-part-2 | RHEL 9.7 / x86_64 | ran 11 tests. 1 tests failed: sanity-lfsck. | session |
| review-dne-part-5 | RHEL 8.10 / x86_64 | ran 5 tests. 1 tests failed: sanityn. | session |
| review-dne-zfs-part-5 | RHEL 8.10 / x86_64 | ran 5 tests. 1 tests failed: sanityn. | session |
(defect?) this value is not used by anything?
(minor) this shouldn't really care about the other flags, only that `u` is present. Otherwise, this could fail if compression or some other feature is enabled. Something like:
```
local attrs=$(lsattr $parent)
[[ "$attrs" =~ u*\ $parent ]] ||
error "Trash object attrs '$attrs' has no UNRM"
```
(defect) need to have correct version for master:
```
(( ($MDS1_REL == EXA7 && $MDS1_VERSION >= $(version_code 2.16.0-ddn25)) ||
$MDS1_VERSION >= $(version_code 2.17.54) )) ||
```
(style) prefer `[[...]` for bash
LU-18456 trash: mark LUSTRE_UNRM_FL for a file moving to trash Mark LUSTRE_UNRM_FL flag for a file moving into trash. As moving a file into trash is implementing in MDD layer, the server does not take any DLM ibits locks, thus it may result in inconsistent access for the files in the trash from clients. i.e. readdir() may miss some new "undeleted" files moving into trash. To solve the inconsistent access problem, when access the file marked with LUSTRE_UNRM_FL on a client by using POSIX API, the client drops the cache after used or drop the DLM locks immediately for the metadata access for files in trash on clients. We reuse the existing FS_UMRM_FL for LUSTRE_UNRM_FL to indicate that the file has been deleted, and can be "undeleted" from Trash Can. That preserves the semantics of that flag, and allows regular tools like "lsattr" to print this flag, without conflicting with other flags. In this patch, the cache for dirent content is dropped for each readddir() call on the client for the dir on the trash. Add sanityn/test_117f to verify that readdir() does not miss any dentry in trash. Change-Id: I08327b5118788d81b3bc67c3b818c007397afc48 Signed-off-by: Yingjin <qian@ddn.com>
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-dne-part-2 | RHEL 8.10 / x86_64 | ran 11 tests. 1 tests failed: sanity-sec. | session |
| review-dne-zfs-part-2 | RHEL 8.10 / x86_64 | ran 11 tests. 1 tests failed: sanity-sec. | session |
LU-18456 tcu: add "notcu" MDT mount option disable TCU setup Add "notcu" MDT mount option to disable TCU initialization during MDT stack setup. The server usually mounts without this option and will initialize the Trash Can dir on MDT during the mount by default. Change-Id: Ib083c199941d51c6e05b0efa3784a4ddb83e86a0 Signed-off-by: Yingjin Qian <qian@ddn.com>
| unique failing test | history |
|---|---|
| sanityn@ldiskfs+DNE:test_117yb | seen in 13 other reviews |
| sanityn@ldiskfs+DNE:test_118Aa | seen in 10 other reviews |
| sanityn@ldiskfs+DNE:test_118Ab | seen in 10 other reviews |
| sanityn@zfs:test_118g | NEW unique failure for this branch in the last 30 days, and was seen 1 times across 1 other branches 1 reviews |
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-dne-part-2 | RHEL 9.5 / x86_64 | ran 11 tests. 1 tests failed: sanity-lfsck. | session |
| review-dne-part-5 | RHEL 8.10 / x86_64 | ran 5 tests. 1 tests failed: sanityn. | session |
| review-dne-zfs-part-5 | RHEL 8.10 / x86_64 | ran 5 tests. 1 tests failed: sanityn. | session |
| review-dne-zfs-subtest-change failed 2× | RHEL 8.10 / x86_64 | ran 3 tests. 1 tests failed: sanityn. | session |
(typo) "destroy"
What does this mean exactly? Can the "orphan trash object" be deleted normally during trash cleanup, or would it cause problems for tools trying to clean up the trash?
(defect) this version check also needs to check for master - 2.17.54
LU-19723 tcu: tolerate ENOSPC error and bypass Trash Can
When unlink a file with TCU enabled, it may fail with ENOSPC
error for Trash index insert or pFID stub dir creation as the
inode space is used out.
In this patch, it simply bypass tolerates the ENOSPC error and
bypass TCU and treat it as normal unlink to destory the file
directly.
Add two test cases sanityn/118{g, h}.
Signed-off-by: Yingjin Qian <qian@ddn.com>
Change-Id: If08da85b530ac81e86a7b9890fa1a63aab34bc95
NB: the `ignore` keyword is even stronger, in that Jenkins won't even try to build the patch if it is only posted for review/reference
LU-19410 utils: lnetdump Implement a tool to capture LNet packet dumps using eBPF. Test-Parameters: forbuildonly Signed-off-by: Timothy Day <timday@amazon.com> Change-Id: I9b19e59ec758de19d95a09fb291066414cf6bf8e
| unique failing test | history |
|---|---|
| sanity1@zfs:test_56od | seen in 3 other reviews |
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-dne-part-4 failed 2× | RHEL 8.10 / x86_64 | ran 9 tests. 1 tests failed: sanity-quota. | session |
| review-dne-part-4 failed 2× | RHEL 9.7 / x86_64 | ran 9 tests. 1 tests failed: sanity-quota. | session |
| review-dne-zfs-part-4 failed 2× | RHEL 8.10 / x86_64 | ran 9 tests. 1 tests failed: sanity-quota. | session |
remind: nodes=$(comma_list $(mdts_nodes)) should change to nodes=$(mdts_nodes)
LU-18917 tcu: implement per-user subdirectories in Trash Can Implement per-user subdirectories in Trash Can to separate user files. The directory name should be the UID of the inode being deleted, not the UID of the process doing the deletion. Add test case sanityn/117d to verify it works as expected. Change-Id: I66cc736ec156019e28669ca06b04605b188e50ff Signed-off-by: Yingjin Qian <qian@ddn.com>
| 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-zfs-part-2 | RHEL 8.10 / x86_64 | ran 11 tests. 1 tests failed: sanity-sec. | session |
| review-dne-zfs-part-4 | RHEL 8.10 / x86_64 | ran 9 tests. 1 tests failed: sanity-quota. | session |
LU-18456 trash: I/O operative limiting for a file in Trash Can This patch adds certain I/O operative limiting for Trash Can: - Forbid the normal file creation under Trash Can from users; - Forbid to move a file into Trash Can; - Forbid to change attributes for a file in Trash Can; - Forbid to set xattrs for a file in Trash Can; - Only allow to readonly open a file in Trash Can; - Forbid to write the content for a file in Trash Can; - Configurable ability to read the content for a file in Trash Can via "llite.*.trash_file_read"; - Forbid to mmap a file in Trash Can on a client; Add a test case: sanityn/117h. Change-Id: I4a96b35fa4f0bb50d56a1f2f7fddc69b353bf55e Signed-off-by: Yingjin Qian <qian@ddn.com>
| 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-zfs-part-2 | RHEL 8.10 / x86_64 | ran 11 tests. 1 tests failed: sanity-sec. | session |
LU-18800 trash: implement virtual ".Trash" subdir for undelete This patch implements virtual ".Trash" subdirectory accessible in each directory in the filesystem that can be used to browse files or directories in the trash can and access them for recovery. The FID of the ".Trash" directory is derived from the FID of the parent directory (pFID), by looking up the corresponding "stub" directory with FID-named directory ".lustre/.ltrash/MDTXXXX/pFID". Essentially this is just a virtual shortcut to the stub directory (if the parent is not a striped dir) that is accessible in each directory if specified by name, though it is not visible in each directory to avoid issues with "rm -r", backups, etc. Add sanityn/test_117g to verify that a client can access files in the trash for a given directory via its virtual subdirectory ".Trash". Change-Id: I6f8f0e0e48c75fe2cd5500eaf47c512c05bea6b8 Signed-off-by: Yingjin <qian@ddn.com>
| unique failing test | history |
|---|---|
| sanity-lfsck@zfs:test_18c | seen in 17 other reviews |
LU-18456 tcu: replicate XATTRs for a dir moving into trash When remove a directory with trash can enabled, if the directory named with its FID already existed in the trash dir, then it will use that directory as a replication of the deleting directory. In this case, it still needs to replicate XATTRs from the original deleting directory. Add sanityn.sh/test_117e to verify that XATTRs are kept when a dir has moved into trash. Change-Id: Ic8ec759176f0c535ec32e63bd9159b8f39122a47 Signed-off-by: Qian Yingjin <qian@ddn.com>
LU-18932 tcu: implement user command lfs trash move|mv Implement user command "lfs trash move|mv $src $dst" to move a file or a directory tree in Trash can to a destination path. The file or path can be moved into destination path via POSIX ->rename() interface. However, it still needs to clear the LUSTRE_UNRM_FL flag for the moved files. Add sanityn/test_117j to verify this command works as expected. Signed-off-by: Yingjin Qian <qian@ddn.com> Change-Id: I61f89b5e5536637207bbedbbd0e760cad623387d Lustre-change: https://review.whamcloud.com/60417 Tested-by: Maloo <maloo@whamcloud.com> Tested-by: jenkins <devops@whamcloud.com> Reviewed-by: Artem Blagodarenko <ablagodarenko@ddn.com> Reviewed-by: Li Xi <lixi@ddn.com> Signed-off-by: Xiyang Wang <xiwang@ddn.com>
| unique failing test | history |
|---|---|
| sanityn@ldiskfs+DNE:test_117yb | seen in 13 other reviews |
| sanityn@ldiskfs+DNE:test_118Aa | seen in 10 other reviews |
| sanityn@ldiskfs+DNE:test_118Ab | seen in 10 other reviews |
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-ldiskfs-dne-arm | RHEL 8.10 / x86_64, Rocky 9.5 / aarch64 | ran 5 tests. 1 tests failed: sanity. | session |
This is not only about typ fix. We get the client-side old_cli_projid from the mds1 facet wrongly... This patch fixes it.
LU-19735 tests: get/set the correct projid in sanityn.sh/117v Fix the typo and get/set the correct client projid in sanityn.sh test_117v. Also, fix two compilation errors. Test-Parameters: trivial Change-Id: Iae1224f116f9584a831cfa13eb9d09cdd6fd7e6c Signed-off-by: Emoly Liu <emoly@whamcloud.com>
| unique failing test | history |
|---|---|
| sanityn@ldiskfs+DNE:test_117yb | seen in 13 other reviews |
| sanityn@ldiskfs+DNE:test_118Aa | seen in 10 other reviews |
| sanityn@ldiskfs+DNE:test_118Ab | seen in 10 other reviews |
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-dne-selinux-ssk-part-1 | RHEL 8.10 / x86_64 | ran 3 tests. 1 tests failed: sanity. | session |
LU-19711 tcu: avoid caching negative dentry for dir in Trash The client should not allow to cache negative dentry for a dir in Trash Can. Otherwise, it may cause inconsistenty problem as the operations in Trash Can do not take any DLM lock. Add a new test case: sanityn/118f. Signed-off-by: Yingjin Qian <qian@ddn.com> Change-Id: I138da54e17e39da1f214997cbe0e46b0d6ae9de7
| unique failing test | history |
|---|---|
| sanityn@zfs:test_118g | seen in 3 other reviews |
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-ldiskfs-dne-arm | RHEL 8.10 / x86_64, Rocky 9.5 / aarch64 | ran 5 tests. 1 tests failed: sanity. | session |
LU-19793 tcu: access Trash Can from subdir (fileset) mount For subdir (fileset) mount, a client should be able to access Trash Can by using llapi_open_by_fid via LU_TRASH_FID to traverse the files or directories on Trash Can. Add a test case saniyt/118l. Test-Parameters: trivial Signed-off-by: Yingjin Qian <qian@ddn.com> Change-Id: I95728577fe3633e3a2d7c534fa9a6ef8ead18892
| unique failing test | history |
|---|---|
| sanity-scrub@ldiskfs+DNE:test_4b | seen in 15 other reviews |
| sanity-scrub@ldiskfs+DNE:test_4c | seen in 15 other reviews |
| sanity-scrub@ldiskfs+DNE:test_5 | seen in 18 other reviews |
| sanity-scrub@ldiskfs+DNE:test_6 | seen in 18 other reviews |
| sanity-scrub@ldiskfs+DNE:test_7 | seen in 18 other reviews |
| sanity-scrub@ldiskfs+DNE:test_10a | seen in 18 other reviews |
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-dne-part-5 crashed | RHEL 8.10 / x86_64 | ran 8 tests. 1 tests failed: sanity-scrub. %% THIS TEST SESSION CRASHED %% | session |
| review-dne-part-5 | RHEL 9.4 / x86_64 | ran 6 tests. 1 tests failed: sanity-scrub. | session |
| review-dne-zfs-part-4 | RHEL 8.10 / x86_64 | ran 9 tests. 1 tests failed: sanity-quota. | session |
| review-ldiskfs | RHEL 8.10 / x86_64, RHEL 9.4 / x86_64 | ran 5 tests. 1 tests failed: sanity-lnet. | session |
| review-ldiskfs | RHEL 8.10 / x86_64, SLES 15.5 / x86_64 | ran 5 tests. 1 tests failed: sanity-lnet. | session |
This should re-use LUSTRE_NODUMP_FL, which means "do not backup", and definitely saving a file in Trash Can is like a form of backup.
LU-18958 tcu: add NOTRASH flag for files and directories Add a NOTRASH attribute to a file or directory, so that when it is deleted the MDS will bypass the Trash Can completely and delete the file immediately (as it was without the Trash Can feature). If the NOTRASH attribute is set on a directory, then new files in that directory will inherit the NOTRASH attribute. This is intended to mark directories that contain very temporary files that should not consume space in the Trash Can. Add the command to set/clear trash flags (i.e. NOTRASH) on given files or directories: lfs trash modify [--set|--clear] [--notrash] FILE [...] Add the command to display the trash state and information for given files or directories: lfs trash state FILE [...] Add sanityn/test_117n to verify it works as expected. Signed-off-by: Yingjin Qian <qian@ddn.com> Change-Id: I66037cf0bd13f3fcdad886042e4269b0cac25bf6
| unique failing test | history |
|---|---|
| sanityn@ldiskfs+DNE:test_117yb | seen in 7 other reviews |
| sanityn@ldiskfs+DNE:test_118Aa | seen in 4 other reviews |
| sanityn@ldiskfs+DNE:test_118Ab | seen in 4 other reviews |
| sanityn@ldiskfs+DNE:test_118d | NEW unique failure for this branch in the last 30 days, and was seen 0 times across 0 other branches 0 reviews |
LU-19588 tcu: fix repeated deletion failure for remote dir Repeated deletion with the same fname for remote directories may return -EEXIST error. This error is reported in mdd_trans_stop() as remote updates are executed at the end of a transaction via OUT. OUT will rollback failed updates on remote nodes. However, the local updates are mostly succeeded in the previous failed transcation. Thus during the transcation redo to recover the failure of the repeated deletion, it only needs to redo the remote updates on the remote object via OUT. Add sanityn.sh/test_118d to verify the fix. Signed-off-by: Yingjin Qian <qian@ddn.com> Change-Id: I451e721a91c02e71854defdac7da1a3301c9734f
| unique failing test | history |
|---|---|
| sanity-quota@zfs:test_48 | seen in 11 other reviews |
LU-18932 tcu: implement user command to empty Trash Can
Implement user command to empty Trash Can.
It scans Trash Can from .lustre/trash and iterates all "MDTXXXX"
to clean up all files in Trash Can recursively.
In the current implementation, the stub directories are retained.
All files under stub directories in Trash Can are deleted.
Next step, the UID/NODEMAP and retain time period will take into
consideration.
This patch also fixes missing readdir() entries which is reported
by the repeat subtest of sanityn/test_117l.
The reason is that we drop each dentry page on read call.
We fix it by using a more loose mechanism. Keep the page cache
for dentries (dcache) until closedir() is called.
In closedir(), we drop the page cache of dentries for directories
marked with LUSTRE_UNRM_FL.
Add test case sanityn/test_117m.
Fixes: e71149a29e ("LU-18456 trash: mark LUSTRE_UNRM_FL for a file moving to trash")
Signed-off-by: Yingjin Qian <qian@ddn.com>
Change-Id: I90e1b56f4715309e0e9e618f3742ae0ec41ad929
| unique failing test | history |
|---|---|
| sanity1@zfs:test_45 | seen in 18 other reviews |
| sanity-quota@zfs:test_48 | seen in 12 other reviews |
LU-18932 tcu: implement user command lfs trash unrm Implement a user command "lfs trash unrm $path" to recover files or directories from Trash Can. The path name recovering from Trash Can should be in the form: - "$lustre_path/.Trash/basename": Recover the top level sub file specified by @basename under the stub dir ".Trash"; - "$lustre_path": If only specified a Lustre file path (a Lustre directory), then it will recover all files under ".Trash" in Trash Can. Add tese case sanityn/test_117k to verify it works as expected. Signed-off-by: Yingjin Qian <qian@ddn.com> Change-Id: Id1528ed5ec3e6a2d295cd8e6856e15e11a8d802b
LU-18456 tcu: update LinkEA when move file into Trash Can Update LinkEA data accordingly when moving a file into Trash Can. As rename() operation also needs to update LinkEA, to avoid conflict, we define a seperate LinkEA data and buffer for the update of LinkEA of the Trash Object. Add a test case sanityn/test_117n to verify it works as expected. Signed-off-by: Yingjin Qian <qian@ddn.com> Change-Id: Ie4fc5bda896877ff0b4cb86c878f3ecd7d98226d
LU-18932 tcu: implement user command lfs trash clean Implement user command for managing files in the trahs: "lfs trash clean [--recursive] [DIR]" immediately clean up files in Trash Can dir DIR if specified or current directory. Add sanityn/test_117l to verify it works as expected. Signed-off-by: Yingjin Qian <qian@ddn.com> Change-Id: I01bc994adf029e28c79ef69cf2ab88e77a180a73
LU-16162 ldiskfs: keep low tracks allocated by mballoc Far tracks of rotational disks may have 40% worse read/write performance compared with low tracks at the beginning of the disk but the mballoc code treats all tracks equally using slow tracks on empty or moderately full disks. The patch modifies C0 allocations to not allow the streaming allocator to advance mb_last_group till the previous block groups are filled with given allocation ratio. Test-Parameters: trivial fstype=ldiskfs serverdistro=el8.5 Test-Parameters: trivial fstype=ldiskfs serverdistro=el8.6 HPE-bug-id: LUS-3719 Signed-off-by: Alexander Zarochentsev <alexander.zarochentsev@hpe.com> Change-Id: I3ecd161285fd5d1e89225910aeb8bb2023804f6e
| unique failing test | history |
|---|---|
| sanity-scrub@ldiskfs+DNE:test_4b | seen in 16 other reviews |
| sanity-scrub@ldiskfs+DNE:test_4c | seen in 16 other reviews |
| sanity-scrub@ldiskfs+DNE:test_5 | seen in 19 other reviews |
| sanity-scrub@ldiskfs+DNE:test_6 | seen in 19 other reviews |
| sanity-scrub@ldiskfs+DNE:test_7 | seen in 19 other reviews |
| sanity-scrub@ldiskfs+DNE:test_10a | seen in 19 other reviews |
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-dne-part-5 crashed | RHEL 8.10 / x86_64 | ran 8 tests. 1 tests failed: sanity-scrub. %% THIS TEST SESSION CRASHED %% | session |
| review-dne-part-5 crashed | RHEL 9.5 / x86_64 | ran 8 tests. 1 tests failed: sanity-scrub. %% THIS TEST SESSION CRASHED %% | session |
| review-dne-part-6 | RHEL 9.5 / x86_64 | ran 4 tests. 1 tests failed: replay-single. | session |
| review-dne-zfs-part-4 | RHEL 8.10 / x86_64 | ran 9 tests. 1 tests failed: sanity-hsm. | session |
LU-18985 tcu: use master pfid as stub dir name for striped dir When files in a striped directory are deleted, it should use master parent FID as the stub dir name in Trash Can (instead of the stripe shard FID). Add sanityn/test_117o to verify it. Signed-off-by: Yingjin Qian <qian@ddn.com> Change-Id: I82238b949fd1b2556f0624f5a92f67741ea85368
(minor) The diffstat doesn't match the body of the re-port. Counting the hunks gives ext4.h 25 changed lines (23 added, 2 removed) rather than 24, mballoc.c 180 insertions rather than 179, and a total of 231 insertions / 2 deletions rather than 229 / 2. Worth regenerating the header when the patch is next refreshed so the numbers describe the patch that is actually here.
Not critical. In general this stats block should be removed from copied patches for this reason (if patch is refreshed), but I don't think that is a reason to refresh the patch by itself.
(minor) This hunk header looks hand-edited: the postimage start repeats the preimage start. The surrounding mballoc.c hunks are self-consistent - `-3946,6 +3974,30` and `-4011,6 +4063,9` put the running offset at +55, and the next one, `-4226,6 +4286,11`, is at +60. So this one should read `+4168,11`, not `+4113,11`. Both `patch` and `git apply` locate the hunk by context and ignore the postimage number, so it still applies, but the wrong number makes it hard to tell a real rebase from a hand edit later on.
Like backported patches, these `Tested-by:` lines should be removed if they are not real people, if patch is refreshed.
@ablagodarenko@thelustrecollective.com Should this be `unsafe_memcpy()`? This doesn't affect functionality, but I think it can trigger build warnings with some configs.
(minor) dot_reclen is set here but never read afterwards.
The linux-6.12 copy that the el10.0/el10.1 series use has a third occurrence of it, replacing upstream's fixed size in the branch that gives ".." the rest of the block:
de->rec_len = ext4_rec_len_to_disk(blocksize -
(csum_size + dot_reclen), blocksize);
Here that branch is left at the upstream form, blocksize - (csum_size + ext4_dir_rec_len(1, NULL)), and nothing else in ldiskfs-6.12-rhel10.2.series touches it, so ".." is sized as if "." were always 12 bytes.
It does not bite today: osd_add_dot_dotdot_internal() sets dot_ldp->edp_magic = 0, so data1 resolves to NULL and dot_reclen stays 12. But if dot data is ever passed, ".." would claim to run past blocksize - csum_size. Was dropping that hunk intentional, or should dot_reclen be wired up the way the el10.1 copy does?
Separately, de->rec_len is already little-endian by this point, so the conversion wants to be le16_to_cpu().
LU-20070 kernel: add rhel10.2 server support Add four ldiskfs patches and a series file for RHEL 10.2 (kernel 6.12): ext4-add-fstrim-mount-option, ext4-add-periodic-superblock-update.patch, ext4-dirdata-lustre-compat, and ext4-mballoc-for-hybrid. Add the x86_64 server kernel config for RHEL 10.2 (6.12.0-211.40.1.el10_2), and register the 6.12-rhel10.2.series patch series in which_patch and ChangeLog. Test-Parameters: trivial fstype=ldiskfs mdtcount=4 mdscount=2 \ clientdistro=el10.2 serverdistro=el10.2 testlist=sanity Test-Parameters: trivial fstype=zfs mdtcount=4 mdscount=2 \ clientdistro=el10.2 serverdistro=el10.2 testlist=sanity Test-Parameters: optional fstype=ldiskfs mdtcount=4 mdscount=2 \ clientdistro=el10.2 serverdistro=el10.2 testgroup=full-dne-part-1 Test-Parameters: optional fstype=ldiskfs mdtcount=4 mdscount=2 \ clientdistro=el10.2 serverdistro=el10.2 testgroup=full-dne-part-2 Test-Parameters: optional fstype=ldiskfs mdtcount=4 mdscount=2 \ clientdistro=el10.2 serverdistro=el10.2 testgroup=full-dne-part-3 Test-Parameters: optional fstype=zfs mdtcount=4 mdscount=2 \ clientdistro=el10.2 serverdistro=el10.2 testgroup=full-dne-zfs-part-1 Test-Parameters: optional fstype=zfs mdtcount=4 mdscount=2 \ clientdistro=el10.2 serverdistro=el10.2 testgroup=full-dne-zfs-part-2 Test-Parameters: optional fstype=zfs mdtcount=4 mdscount=2 \ clientdistro=el10.2 serverdistro=el10.2 testgroup=full-dne-zfs-part-3 TLC-bug-id: TLU-123 Signed-off-by: Minh Diep <mdiep@thelustrecollective.com> Change-Id: I4f7da2801d3821bba2247588cd8f379fd0f13ca5
| unique failing test | history |
|---|---|
| sanity-lfsck@ldiskfs+DNE:test_18c | seen in 8 other reviews |
| failed enforced test | platform | detail | |
|---|---|---|---|
| custom-1001 | RHEL 9.7 / x86_64 | ran 3 tests. 1 tests failed: sanity. | session |
| custom-1002 | RHEL 9.7 / x86_64 | ran 3 tests. 1 tests failed: sanity. | session |
(minor) All three runs are `testlist=sanity`, so the sanity-compr.sh hunk is never exercised by the listed sessions. Adding `Test-Parameters: testlist=sanity-compr` would cover the new test_sanity() path.
(minor) The `COMPR_EXTRA_LAYOUT` fallback that used to sit above this line is dropped here, and the commit message doesn't mention it. Anyone still passing `COMPR_EXTRA_LAYOUT=...` silently falls back to the built-in default instead of their layout. Is the alias intentionally being retired? If so it'd be worth a line in the description, since it was added as an explicit backward-compatibility shim.
(minor) This changes the layout of $DIR for the rest of the session with no matching restore — sanity.sh has no script-level `save_layout_restore_at_exit`, unlike sanity-compr.sh which pairs its `setstripe_getstripe $MOUNT` with one. For `sanity_STRIPEPARAMS`, described as a per-suite override, the layout then persists into whatever auster runs next. Also, when the value came from `fs_STRIPEPARAMS`, check_and_setup_lustre() already applied it to $MOUNT just above, so this re-applies the same layout a second time.
(style) Not a bug, but this moves the continuation from one extra tab to tab-alignment under the `$(`. Lustre style is one extra tab past the parent statement, which is what the line had before.
LU-16904 tests: PFL skip sanity/34h+184d, fix 101i Add sanity.sh test_34h and 184d to always_except until issues fixed. Fix sanity.sh test_101i, use plain layout for file which test expects. Apply fs_STRIPEPARAMS default layout for all test scripts if set. Add sanity_STRIPEPARAMS override the global layout for sanity.sh. Inherit compr_STRIPEPARAMS in sanity-compr.sh for test_sanity(). Test-Parameters: trivial Test-Parameters: testlist=sanity env=fs_STRIPEPARAMS="-E 1M -c1 -E eof" Test-Parameters: testlist=sanity env=fs_STRIPEPARAMS="-E 64k -c 1 -E 1M -c 2 -E eof -c 4 -S 4M" Signed-off-by: Wei Liu <sarah@whamcloud.com> Change-Id: Ie3c877dd124fbf109f69bb9f7aeb217f695a9af7
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-dne-selinux-ssk-part-2 | RHEL 8.10 / x86_64 | ran 5 tests. 1 tests failed: recovery-small. | session |
BUILD
Should osd_fini_iobuf() just have a smp_rmb() inside it?
LU-16245 osd-ldiskfs: ensure ordering of dr_elapsed_valid After waiting for iobuf->dr_numreqs to become zero, we must ensure that the read of iobuf->dr_elapsed_valid in osd_fini_iobuf() sees the correct and up-to-date value. Without a read memory barrier, the CPU or compiler may reorder these reads, leading to incorrect behavior. This patch inserts smp_rmb() after the wait_event to prevent such reordering and ensure proper memory visibility. Signed-off-by: liuqinfei <lucas.liuqinfei@huawei.com> Signed-off-by: luo rixin <luorixin@huawei.com> Change-Id: Ibb20bb70728489aec90bd932426b31e32931a100
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-dne-part-2 | RHEL 9.5 / x86_64 | ran 11 tests. 1 tests failed: sanity-lfsck. | session |
| review-dne-selinux-ssk-part-2 | RHEL 8.10 / x86_64 | ran 5 tests. 1 tests failed: recovery-small. | session |
LU-16246 osd-ldiskfs: ensure r/w ordering of do_index_ops Add smp_rmb() in lod_lookup() to guarantee ordered reads of obj->do_index_ops and next->do_index_ops variables on ARM platform. Add smp_wmb() in osd_index_try() to ensure ordered assignment of dt->do_index_ops variable on ARM platform. Both barriers prevent potential out-of-order execution issues specific to ARM architecture. Signed-off-by: liuqinfei <lucas.liuqinfei@huawei.com> Signed-off-by: luo rixin <luorixin@huawei.com> Change-Id: If27c4770c3c84560e51de955849364846661252d
(defect) This version check should be `2.15.8`. Test 9 was never run for this patch as it was skipped.
More strictly, it should be "> 2.15.8" because the 2.15.8.0 release didn't have this fix either. Any patch built on b2_15 will be at least 2.15.8.1, so that test will pass. More importantly, this except file is only going to be used for interop testing with *older clients* running with this on the server, and we need this `mmp.ex` file **on master as well**, so that unpatched/older clients will also skip the test when running against master servers (which is where most of the testing is being run).
LU-19784 tests: mmp/test_8 wait time changes To prepare for the e2fsprogs/libext2fs changes reducing the wait time when the MMP sequence number is EXT4_MMP_SEQ_CLEAN, LU-19784 ext2fs: mmp wait time ajustments https://review.whamcloud.com/63478 We need to make sure the sequence number is not EXT4_MMP_SEQ_CLEAN before running e2fsck. We can use debugfs to change the MMP sequence number, rename mark_mmp_block() to set_mmp_seq() to reflect this. Remove mmp_mark.sh, which is not needed any more, and use set_mmp_seq() in test_9. Make sure we tear down the dm flakey device in test_9 otherwise during mmp_fini()->disable_mmp() will corrupt the filesystem. Lustre-change: https://review.whamcloud.com/63488 Lustre-commit: ad051b359db12ef7e752abcf2aa8e34199ded041 Test-Parameters: trivial testlist=mmp Signed-off-by: Li Dongyang <dongyangli@ddn.com> Change-Id: I81b9d28061862a99b04fa92de0cc18a536c9d17b Reviewed-by: Artem Blagodarenko <ablagodarenko@thelustrecollective.com> Reviewed-by: Andreas Dilger <adilger@thelustrecollective.com> Reviewed-by: Alex Zhuravlev <bzzz@whamcloud.com>
LU-19784 tests: mmp/test_8 wait time changes To prepare for the e2fsprogs/libext2fs changes reducing the wait time when the MMP sequence number is EXT4_MMP_SEQ_CLEAN, LU-19784 ext2fs: mmp wait time ajustments https://review.whamcloud.com/63478 We need to make sure the sequence number is not EXT4_MMP_SEQ_CLEAN before running e2fsck. We can use debugfs to change the MMP sequence number, rename mark_mmp_block() to set_mmp_seq() to reflect this. Remove mmp_mark.sh, which is not needed any more, and use set_mmp_seq() in test_9. Make sure we tear down the dm flakey device in test_9 otherwise during mmp_fini()->disable_mmp() will corrupt the filesystem. Lustre-change: https://review.whamcloud.com/63488 Lustre-commit: ad051b359db12ef7e752abcf2aa8e34199ded041 Test-Parameters: trivial Signed-off-by: Li Dongyang <dongyangli@ddn.com> Change-Id: I81b9d28061862a99b04fa92de0cc18a536c9d17b Reviewed-by: Artem Blagodarenko <ablagodarenko@thelustrecollective.com> Reviewed-by: Andreas Dilger <adilger@thelustrecollective.com> Reviewed-by: Alex Zhuravlev <bzzz@whamcloud.com>
LU-19784 tests: mmp/test_8 wait time changes To prepare for the e2fsprogs/libext2fs changes reducing the wait time when the MMP sequence number is EXT4_MMP_SEQ_CLEAN, LU-19784 ext2fs: mmp wait time ajustments https://review.whamcloud.com/63478 We need to make sure the sequence number is not EXT4_MMP_SEQ_CLEAN before running e2fsck. We can use debugfs to change the MMP sequence number, rename mark_mmp_block() to set_mmp_seq() to reflect this. Remove mmp_mark.sh, which is not needed any more, and use set_mmp_seq() in test_9. Make sure we tear down the dm flakey device in test_9 otherwise during mmp_fini()->disable_mmp() will corrupt the filesystem. Lustre-change: https://review.whamcloud.com/63488 Lustre-commit: ad051b359db12ef7e752abcf2aa8e34199ded041 Test-Parameters: trivial Signed-off-by: Li Dongyang <dongyangli@ddn.com> Change-Id: I81b9d28061862a99b04fa92de0cc18a536c9d17b Reviewed-by: Artem Blagodarenko <ablagodarenko@thelustrecollective.com> Reviewed-by: Andreas Dilger <adilger@thelustrecollective.com> Reviewed-by: Alex Zhuravlev <bzzz@whamcloud.com>
courtesy of gerrit not accepting test results at times you did not get your early warning. This introduces a sleeping under spinlock right at server init time and so every test has it https://testing.whamcloud.com/gerrit-janitor/63997/results.html [ 817.527107] alg: No test for zstdfast (zstdfast-lustre-generic) [ 817.534750] BUG: sleeping function called from invalid context at include/linux/sched/mm.h:226 [ 817.541176] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 26101, name: cryptomgr_test [ 817.550885] CPU: 3 PID: 26101 Comm: cryptomgr_test Kdump: loaded Tainted: G W O -------- - - 4.18.0rh8.10-debug #2 [ 817.559137] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.17.0-10.fc44 06/10/2025 [ 817.565102] Call Trace: [ 817.566955] ? dump_stack+0xbb/0x10e [ 817.575504] ? ___might_sleep.cold.92+0xd9/0x107 [ 817.580904] ? __might_sleep+0x59/0xc0 [ 817.583799] ? slab_pre_alloc_hook.constprop.64+0x11f/0x1d0 [ 817.590298] ? kmem_cache_alloc_node_trace+0x5e/0x470 [ 817.595682] ? __get_vm_area_node+0xd6/0x220 [ 817.599009] ? __vmalloc_node_range+0x79/0x3c0 [ 817.603765] ? __zstd_compress+0x100/0x210 [lzstd] [ 817.606239] ? __vmalloc_node+0x4e/0x90 [ 817.608884] ? __zstd_compress+0x100/0x210 [lzstd] [ 817.615296] ? vzalloc+0x4e/0x60 [ 817.616509] ? __zstd_compress+0x100/0x210 [lzstd] [ 817.620607] ? zstd_scompress+0x32/0x60 [lzstd] [ 817.625250] ? scomp_acomp_comp_decomp+0x129/0x200 [ 817.627994] ? scomp_acomp_compress+0x17/0x20 [ 817.633796] ? test_acomp+0x2cc/0x5a0 [ 817.635204] ? cryptomgr_probe+0x150/0x150 [ 817.638300] ? alg_test_comp+0xc8/0xf0 [ 817.640295] ? alg_test+0x1f0/0x390 [ 817.642645] ? do_raw_spin_unlock+0x75/0x190 [ 817.646249] ? _raw_spin_unlock_irq+0x12/0x50 [ 817.648952] ? finish_task_switch+0x125/0x560 [ 817.651578] ? __schedule+0x369/0xcb0 [ 817.656565] ? do_raw_spin_unlock+0x75/0x190 [ 817.660800] ? cryptomgr_probe+0x150/0x150 [ 817.663411] ? cryptomgr_test+0x37/0x70 [ 817.665939] ? kthread+0x1d1/0x200 [ 817.669914] ? set_kthread_struct+0x70/0x70 [ 817.673595] ? ret_from_fork+0x1f/0x30 [ 817.689553] alg: No test for zstdfast (zstdfast-lustre-scomp)
this one is kinda suspect and should be &&?
Got flagged on the kernel by smatch as well: https://lists.openwall.net/linux-kernel/2025/12/18/1007 But, I suspect this is the same reason as for the lz4 patch (details there). There is no dependency between the two conditions here, and so this could be beneficial for performance. It's consistent to what is upstream: https://github.com/torvalds/linux/blob/master/lib/zstd/decompress/huf_decompress.c#L650
this one too?
Same as above. Consistent with upstream: https://github.com/torvalds/linux/blob/master/lib/zstd/decompress/huf_decompress.c#L1431
LU-10026 lustre: add lzstd kernel module
lzstd kernel module implements compression according to the zstd
algorithm, through the kernel Crypto API.
It provides 2 cipher drivers under the generic name 'zstd':
* zstd-lustre-generic of type compression
* zstd-lustre-scomp of type scomp
And 2 cipher drivers under the generic name 'zstdfast':
* zstdfast-lustre-generic of type compression
* zstdfast-lustre-scomp of type scomp
Note the 'zstd' name is identical to the in-kernel module, but lzstd
registers it with a slightly higher .cra_priority = 110, so that it is
preferred over the in-kernel module.
Our 'zstd' is also different in that it accepts a compression level
as explained below. 'zstdfast' is handling the 'negative' compression
levels, interpreted as an acceleration factor.
lzstd kernel module sources are copied from linux v6.9. This module
implements the Crypto API interface, and rely on the zstd kernel
library for compression implementation. It has been modified to grok
a compression level, as read from the top 4 bits of the crypto_tfm
flags, and pass it to the underlying library.
The zstd library sources are also copied from linux v6.9 and built
statically. Headers have also been copied from linux v6.9 for
consistency, and source files modified to include the copied headers
instead of the system headers.
In order to maximize compatibility with various distributions, we copy
xxhash sources from linux v6.9, and link these statically.
All aforementioned sources are located in the
lustre_compat/crypto/zstd directory.
The lzstd module is always built with Lustre.
This patch enhances the test kernel module kcompr.ko to exercise the
compression level of the provided 'zstd' module.
It also tries to manually load the lzstd kernel module when a file
requires compression with the 'zstd' alg. This is a "one-shot" try
that allows us to prefer our module that has level support, but
continues to at least compress/decompress files even if our own module
is not available.
EX-9678 csdc: defer memory allocation in zstd/zstdfast
Zstandard normally proceeds to memory allocation for its context
(workspace) while in the init phase, which corresponds to a call to
crypto_alloc_comp() when used via the Crypto API.
At this stage we cannot be aware of the compression level or the chunk
size. So we have to make zstd allocate a workspace that can fit any
level/chunk. And the higher the level is, the larger the workspace
needs to be. Problem is zstd is very memory eager, and by default for
the higher compression level it requires a workspace of 650 MiB. This
is insane compared to our usual compression chunk sizes.
So we defer workspace initialization to the first time zstd actually
compresses data. This corresponds to a call to crypto_comp_compress()
when used via the Crypto API. And by that time we know the compression
level and chunk size, which enables us to make zstd allocate a finer
grained workspace, that just fits our actual needs. This saves a lot
on memory allocation, as can be seen from these values (workspace size
expressed in bytes):
no src hint 64kB src
level 1 cwksp size 582392 303864
level 2 cwksp size 779000 402168
level 3 cwksp size 1303288 664312
level 4 cwksp size 2614008 1057528
level 5 cwksp size 3662584 1057528
level 6 cwksp size 3662584 1057528
level 7 cwksp size 6808312 1057528
level 8 cwksp size 6808312 1057528
level 9 cwksp size 13099768 1057528
level 11 cwksp size 25682680 1319672
level 13 cwksp size 34071288 1468856
level 15 cwksp size 67625720 1791095
level 17 cwksp size 50997688 1791095
level 19 cwksp size 85196598 1791095
level 21 cwksp size 336854838 1791095
level 22 cwksp size 680943414 1791095
And of course because we reduce contention on memory a lot, we improve
performance when zstd is used via the Crypto API, which is done in
osc_brw_prep_request() for ever RPC that involves a file compressed
with zstd.
Was-Change-Id: I470331ea0f2e550761ce7223e7be7e1373701bd2
Was-Change-Id: Idf7382093feec21bed5b6e62001a085a386736d2
EX-bug-id: EX-9678
Test-Parameters: testlist=sanity env=ONLY=84
Signed-off-by: Sebastien Buisson <sbuisson@ddn.com>
Signed-off-by: Artem Blagodarenko <ablagodarenko@ddn.com>
Signed-off-by: Marc Vef <mvef@whamcloud.com>
Change-Id: I113345cd10f41197683bfe127d373b8628364f88
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-dne-part-1 | RHEL 8.10 / x86_64 | ran 4 tests. 1 tests failed: sanity. | session |
| review-ldiskfs | RHEL 8.10 / x86_64, RHEL 9.4 / x86_64 | ran 5 tests. 1 tests failed: sanity-lnet. | session |
LU-18456 mdt: create trash dir for MDT after MDT stack setup In this patch, the trash directories corresponding to various MDTs are created in mdt_postrecov(). The trash directory should be visible in Lustre namespace by the path ".lustre/trash/MDTXXXX". Where ".lustre/trash/MDT0000" is a local trash directory for MDT0; However, for the trash directories ".lustre/trash/MDTXXXX" corresponding to MDTs other than MDT0, they are all remote directories with parent ".lustre/trash" (FID: LU_TRASH_FID) on MDT0. Moreover, the trash directory is created in an extra thread in ->o_postrecov() to avoid blocking the MDT stack setup and recovery. The reasons are as follows: - all devices belonging to the MDT stack are configured and setup properly. - The recovery precedure is finished, and the server becomes ready to handle the normal RPCs (i.e. create remote directory for a trash directory). Signed-off-by: Qian Yingjin <qian@ddn.com> Change-Id: I14830faf7ead65863a4c94f373c0df5629926afc
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-ldiskfs-dne-arm | RHEL 8.10 / x86_64, Rocky 9.5 / aarch64 | ran 5 tests. 1 tests failed: sanity. | session |
LU-18818 build: fix e2fsprogs-devel BuildRequires
We should require e2fsprogs-devel >= 1.47.3-wc2 to
make it consistent with the requirements from
config/lustre-core.m4
Change-Id: Ifbc62a3fc358fb4c92083a7752b3757e30a3d58c
Fixes: a1cec48c37 ("LU-18818 utils: covert to libext2fs for rest of ldiskfs ops")
Test-Parameters: trivial
Signed-off-by: Li Dongyang <dongyangli@ddn.com>
| unique failing test | history |
|---|---|
| replay-vbr@ldiskfs+DNE:test_5b | seen in 1 other review |
LU-15913 mdt: disable parallel rename for striped dirs
Parallel rename should not be done within striped directories to
avoid remote updates. These are like cross-directory renames.
Add tunables for parallel directory rename in case of problems.
These can be configured separately for files and directories.
mdt.*.enable_parallel_rename_dir
mdt.*.enable_parallel_rename_file
Lustre-change: https://review.whamcloud.com/47593
Lustre-commit: f238540c879dc668e18cf99cba62f117ccae64d6
Fixes: 90979ab390 ("LU-12125 mds: allow parallel directory rename")
Fixes: d76cc65d5d ("LU-12125 mds: allow parallel regular file rename")
Signed-off-by: Andreas Dilger <adilger@whamcloud.com>
Change-Id: I384976cd1c9f401169336ee7a479ba0e3dd9f4ee
Reviewed-by: Artem Blagodarenko <ablagodarenko@ddn.com>
Reviewed-by: Lai Siyao <lai.siyao@whamcloud.com>
Signed-off-by: Etienne AUJAMES <eaujames@ddn.com>
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-dne-part-4 | CentOS 7.9/x86_64 | ran 9 tests. 1 tests failed: sanity-hsm. | session |
| review-dne-part-5 | CentOS 7.9/x86_64 | ran 6 tests. 1 tests failed: sanityn. | session |
| review-ldiskfs | CentOS 7.9/x86_64 | ran 5 tests. 1 tests failed: sanity. | session |
LU-14305 ldiskfs: add parameters for mb_c123_threshold
Add mount options for /sys/fs/ldiskfs/*/mb_c[123]_threshold values
so that they can be set persistently via mount options.
The /sys/fs/ldiskfs/*/mb_c[123]_threshold values are always shown
rounded down to the next lower percentage value due to integer
division, since internal values are stored as blocks for efficiency.
Round up the values shown to the next percent to match what was
used to originally set these parameters.
Lustre-commit: c2fd5297b46c4973aeda4d4d02cbc7ca2faa0d50
Lustre-change: https://review.whamcloud.com/41193
Fixes: 95f8ae567749 ("LU-12103 ldiskfs: don't search large block range if disk full")
Signed-off-by: Andreas Dilger <adilger@whamcloud.com>
Change-Id: Ie36a6667f8bca7481aa8179ab5b97c85d449d619
Reviewed-by: Artem Blagodarenko <artem.blagodarenko@hpe.com>
Reviewed-by: Jian Yu <yujian@whamcloud.com>
Signed-off-by: Etienne AUJAMES <eaujames@ddn.com>
LU-16061 osd-ldiskfs: clear EXTENT_FL for symlink agent inode The flag should be cleared for "fast" symlinks otherwise e2fsck complains about inode correctness. New agent inodes of symlink type may have EXT4_EXTENT_FL flag set if the fs has "extent" feature and it is not cleared as in other places where "fast" symlinks are created. HPE-bug-id: LUS-10237 Lustre-change: https://review.whamcloud.com/48093 Lustre-commit: 73ac8e35e5d64d3fe4ca6c48514dc57058e3a7b8 Signed-off-by: Alexander Zarochentsev <alexander.zarochentsev@hpe.com> Change-Id: Ib7b807bb1298cc3a9fd4fdba35747b4bda6fe034 Reviewed-by: Artem Blagodarenko <ablagodarenko@ddn.com> Reviewed-by: Andreas Dilger <adilger@whamcloud.com> Signed-off-by: Etienne AUJAMES <eaujames@ddn.com>
LU-16061 osd-ldiskfs: clear EXTENT_FL for symlink agent inode The flag should be cleared for "fast" symlinks otherwise e2fsck complains about inode correctness. New agent inodes of symlink type may have EXT4_EXTENT_FL flag set if the fs has "extent" feature and it is not cleared as in other places where "fast" symlinks are created. Lustre-change: https://review.whamcloud.com/48093 Lustre-commit: 73ac8e35e5d64d3fe4ca6c48514dc57058e3a7b8 HPE-bug-id: LUS-10237 Signed-off-by: Alexander Zarochentsev <alexander.zarochentsev@hpe.com> Change-Id: Ib7b807bb1298cc3a9fd4fdba35747b4bda6fe034 Reviewed-by: Artem Blagodarenko <ablagodarenko@ddn.com> Reviewed-by: Andreas Dilger <adilger@whamcloud.com> Signed-off-by: Etienne AUJAMES <eaujames@ddn.com>
LU-16060 osd-ldiskfs: copy nul byte terminator in writelink memcpy() call in osd_ldiskfs_writelink() doesn't copy the nul terminator byte from the source buffer, leaving the space after target link name uninialized which is ok for the kernel code and debugfs but not e2fsck. HPE-bug-id: LUS-11103 Lustre-change: https://review.whamcloud.com/48092 Lustre-commit: 907dc0a2d333f2df2d654a968fc50f8cc05b779d Signed-off-by: Alexander Zarochentsev <alexander.zarochentsev@hpe.com> Change-Id: I914f2c78e1a6571bf360a23b0ede8c70502bf0df Reviewed-by: Artem Blagodarenko <ablagodarenko@ddn.com> Reviewed-by: Andrew Perepechko <andrew.perepechko@hpe.com> Reviewed-by: Andreas Dilger <adilger@whamcloud.com> Signed-off-by: Etienne AUJAMES <eaujames@ddn.com>
| failed enforced test | platform | detail | |
|---|---|---|---|
| review-dne-subtest-change failed 2× | RHEL 8.10 / x86_64 | ran 3 tests. 1 tests failed: sanityn. | session |
| review-dne-zfs-subtest-change failed 2× | RHEL 8.10 / x86_64 | ran 3 tests. 1 tests failed: sanityn. | session |
| review-ldiskfs-dne-arm | RHEL 8.10 / x86_64, Rocky 9.5 / aarch64 | ran 5 tests. 1 tests failed: sanity. | session |
LU-20116 ptlrpc: page-weighted TBF token accounting TBF assigns one token per RPC regardless of the RPC's resource footprint. A 4 MiB bulk write and a 4 KiB read consume identical tokens despite a 1000x difference in bandwidth. This makes rate limits unpredictable in terms of actual resource consumption. Add page-weighted token accounting: for I/O RPCs (OST_READ, OST_WRITE) token cost equals the number of pages in the request (sum of DIV_ROUND_UP(rnb_len / PAGE_SIZE) across all niobuf_remote entries). For non-I/O RPCs, cost remains 1. The cost model is inferred from the rate format. Bandwidth suffixes on rate= and minrate= (e.g. rate=4GiB/s, rate=500MB/s) select page-weighted accounting; the kernel converts to pages/s by dividing by PAGE_SIZE. An optional trailing "/s" is stripped before the value is passed to sysfs_memparse(), which handles both IEC binary units (K/M/G/T/P/E with optional "iB" suffix, base 1024) and SI decimal units (kB/MB/GB/TB/PB/EB, base 1000), including fractional values (e.g. rate=1.5GiB/s). Rule dumps always use explicit IEC notation with "/s" (e.g. "4GiB/s", "1536MiB/s"). Plain integers select the default rpcs model. Mixed formats (one with suffix, one without) are rejected. Page cost is computed once at nrs_tbf_req_add() (enqueue) and cached in nrq->nr_u.tbf.tr_cost. Direct lustre_msg_buf() access at REQ_REC_OFF + 2 is used instead of req_capsule_client_get() because req->rq_pill.rc_fmt is not yet initialized at NRS enqueue time. Token bucket depth is scaled for the pages model via nrs_tbf_max_depth(): PTLRPC_MAX_BRW_PAGES. Without this, the default tbf_depth=3 caps the bucket at 3 tokens, but a single 1M I/O RPC costs 256 tokens - the request can never be served. PTLRPC_MAX_BRW_PAGES (16384 at 4K pages) is the maximum cost of any single BRW RPC. Multi-token dequeue in nrs_tbf_ntoken(): when tokens < cost, the deficit-based deadline (now + deficit * nsecs_per_token - residual) ensures the timer fires at the exact moment enough tokens have accrued, rather than waking on every single-token tick. LPROCFS_NRS_RATE_MAX is raised from 1,000,000 to 1,000,000,000 to accommodate page-rate values at full server bandwidth (~4 TB/s at 4K pages). Test-Parameters: trivial Test-Parameters: testlist=sanityn,conf-sanity Test-Parameters: testlist=sanityn env=ONLY=77y,ONLY_REPEAT=30 Signed-off-by: Chris Horn <chris.horn@hpe.com> Change-Id: I13e09b74a6683149e01ef5cc254cb1e69e41117d
| unique failing test | history |
|---|---|
| sanityn@ldiskfs+DNE:test_77z | NEW unique failure for this branch in the last 30 days, and was seen 0 times across 0 other branches 0 reviews |
These parameters should all get man4 pages that describe their usage. You can run `contrib/scripts/generate-manpage.sh --param nrs_fairshare_mode` on a server that has this patch running to generate a template page.
(style) prefer `((...))` for numerical comparisons in bash (style) prefer `check || action` (style) no need for `return` after `skip`
(style) no need for `return 0` after "check || action" comparison
LU-17296 ptlrpc: fairshare prototype
Introduce a new NRS policy, "fairshare", that combines mechanisms
previously spread across three separate policies (FIFO, TBF, ORR)
into a single schedulable unit. The motivation is to remove the
"pick one" constraint NRS currently imposes on administrators:
deployments that need rate ceilings on a handful of tenants, a
minrate floor for a VIP job, and locality-aware ordering on HDD
OSTs should not have to trade one capability for another.
The policy composes four mechanisms behind one op_policy_start:
1. Dynamic statistical fair-share between entities (jobid, uid,
gid, projid, or nid). Zero-config: any entity sending RPCs
gets an equal slice. Based on ThemisIO's opportunity-fair
model rather than strict round-robin, so an idle entity does
not starve the service.
2. TBF-compatible rate/minrate rule overrides via the same
start/change/stop syntax. Rules are a per-entity refinement
of the fair-share baseline; entities without rules still
share fairly. This is a strict superset of TBF behavior for
administrators who only care about caps.
3. ORR-style intra-entity offset-sorted dispatch for BRW RPCs.
Enabled per-service via nrs_fairshare_locality. Orthogonal
to fair-share selection: pickers still choose the winner
entity as if locality were off; locality only reorders *which
request within that entity* dispatches.
4. Two-level composite hierarchy ("A_then_B_fair", e.g.
uid_then_jobid_fair). Collapses to a nested equal-partition
over the group key then the leaf; rules match the leaf key
only.
Non-obvious implementation details
* Lock and allocation discipline. fsh_lock is a per-CPT spinlock
held on every enqueue/dequeue/dispatch. Nothing on the hot path
allocates. All variable-size work (range-array rebuilds, rule
CRUD) is staged outside the lock and committed under it.
op_req_get is called under ptlrpc's scp_req_lock, which is also
a spinlock - there is no sleepable context available on the
hot path, which cascades into several design choices below.
* delta-kthread for range rebuild. A per-head kthread
(nrs_fs_delta_fn) wakes every fsh_delta_ms and rebuilds the entity
dispatch-range array if an enqueue marked the head dirty. The picker
then walks a stable array under fsh_lock. Rebuild cadence (10..10000
ms) is a tunable. kthread_stop MUST be called *before* taking fsh_lock
in nrs_fairshare_stop, because the kthread itself takes fsh_lock;
taking the lock first would deadlock.
* Opportunity fairness vs. strict rotation. When the next-in-
line entity's queue is below nrs_fairshare_opp_threshold
requests, the picker opportunistically dispatches from any
non-empty entity to keep the service busy. This trades exact
rotation for throughput; it is what prevents fair-share from
regressing against FIFO under bursty load. Set the threshold
to 0 for strict rotation.
* Token bucket math. nrs_fs_tokens_refill uses
mul_u64_u64_div_u64 with a 1-second elapsed clamp. An NRS
that sat idle for an hour would otherwise overflow the
fixed-point product at NRS_FS_TOKEN_SCALE (2^20). The clamp is
safe because a bucket that refills to its cap in 1s is already
at steady state.
* Composite mode. Collapses cleanly to an insertion-sort by
fse_key0 followed by the same equal-share partition used at
depth 1. We do not build a real transition matrix; at depth 2
the matrix degenerates to "group, then split". Rules match
the leaf field only - a uid rule under uid_then_jobid_fair
caps/floors the whole uid group, which is the intended
semantics (the rule belongs to the coarser key holder).
* Rule refcounting and entity lifetime. Rules are refcounted and
survive entity churn. An entity on create takes a ref on its
matching rule (if any). Rule updates do not walk the entity
list; the next range rebuild picks them up. Rule stop only
succeeds when refs drop to zero - in practice the entity
takes care of this via its own release path.
* Locality RB-tree. Per-entity (fse_req_tree) keyed on
(first-niobuf logical offset, monotonic fsh_seq) giving a stable
total order. Offset extraction (nrs_fs_req_extract_offset) runs
*outside* fsh_lock because req_capsule_client_get may touch
the RPC pill; the offset is stashed and consumed under lock.
Commit-phase re-pick (nrs_fs_entity_head_locked) returns the
leftmost tree entry when locality is effective-on, else the
FIFO list head. Both list_del_init and tree-remove run in the
commit path; tree-remove is a no-op when fsr_in_tree is false,
so it is safe to call unconditionally.
* Logical offsets only. Physical translation via fiemap requires
obd_get_info, which can sleep, so it cannot be used under
fsh_lock. Logical ordering captures the bulk of the rotational
win; physical refinement is a post-v1 item.
* locality=auto. Intentionally collapses to off in v1.
Rotational-media auto-detect needs an OSD->block-device bridge
that does not yet exist. The tunable still accepts "auto" and
reports "auto (effective: off)" so operators can opt into "on"
explicitly on HDD services without a schema change later.
* Live switch semantics. OFF->ON takes effect on new enqueues
only; existing requests drain via FIFO until the tree fills.
ON->OFF leaves treed entries to drain naturally - no scan-and-
unlink. This avoids any scan under fsh_lock on configuration
change.
* Per-CPT fairness. Each CPT has its own nrs_fs_head; there is
no cross-CPT coordination in v1. Under the common RPC-hashing
placement this is close to global fairness; it can skew when
a few clients' traffic collapses onto few CPTs. Cross-CPT
delta-sync is documented as a post-v1 item.
* Rule-write broadcast. Writes to nrs_fairshare_rule are
broadcast to every fairshare head on the service so rule state
stays consistent across CPTs without any shared rule table.
The command is re-parsed per head - cheap, and avoids any
cross-CPT locking.
* Stats schema stability. nrs_fairshare_stats emits one YAML
document per CPT head. The schema is declared stable with
append-only evolution; field names and meanings are preserved
across releases. Consumers can rely on key, key0, active,
queue_depth, dispatched, cost_total, opportunity, throttled,
* Union sizing. nrs_fs_req extends the per-request NRS union with
rb_node + offset + seq + bool (~48 bytes), safely below the
existing TBF/ORR union members. No change to struct
ptlrpc_request layout.
* Shared rule header. lustre_nrs_rule.h factors the enum/field
definitions that TBF and fairshare now both consume. TBF's
existing behavior is unchanged; it continues to use its own
parser. A parser-reuse refactor is deferred.
User-facing surface
-------------------
Tunables on each service's debugfs NRS directory:
nrs_fairshare_mode RW sharing key (jobid_fair,
uid_fair, gid_fair,
projid_fair, nid_fair,
A_then_B_fair composites)
nrs_fairshare_delta_ms RW rebuild cadence (10..10000)
nrs_fairshare_opp_threshold RW opportunity threshold
nrs_fairshare_rule RW rate/minrate rules
(start/change/stop syntax)
nrs_fairshare_locality RW auto | off | on
nrs_fairshare_stats RO YAML per-entity snapshot
Activation: "lctl set_param <service>.nrs_policies=fairshare".
Tests
-----
sanityn.sh tests 77aa..77ag cover:
77aa mode tunable + default fairness
77ab opportunity-fairness throughput regression vs FIFO
77ac rate ceiling enforcement
77ad minrate floor under contention
77ae composite mode round-trip + bogus rejection
77af delta_ms tunable + delta-kthread liveness
77ag locality tunable round-trip + BRW under on
A comparison-benchmark helper, lustre/tests/nrs-fairshare-bench.sh,
cycles a shared IOR workload through FIFO/TBF/ORR/fairshare/
fairshare+locality and logs per-policy throughput.
Documentation
-------------
Documentation/nrs_fairshare.txt operator guide
Documentation/nrs_fairshare_future_work.txt post-v1 roadmap
The future-work doc catalogs 12 deferred items (rotational
auto-detect, cross-CPT delta-sync, cross-server fairness, wall-clock
cost model, size-fair, deeper composites, per-opcode locality,
tree size caps, fiemap ordering, parser refactor, kunit harness,
Prometheus metrics) with enough context per item for a planner
to turn it into an implementation plan.
TBF and ORR are left untouched. The existing policies continue
to work and remain selectable; fairshare is additive.
Test-Parameters: fortestonly
Test-Parameters: testlist=sanityn
Signed-off-by: Chris Horn <chris.horn@hpe.com>
Change-Id: I6adc55873edbc70532baec53fa00fe0912e2a1ed
LU-18222 quota: add LQA directory-based record management
Add directory-based LQA initialization at MDT startup that scans for
lqa-dt-* and lqa-md-* directories in quota_master and automatically
creates in-memory LQA structures with ranges.
LQA range record format uses struct lqa_id_range from lustre_user.h
lir_start: Range start ID (inclusive)
lir_end: Range end ID (inclusive)
This structure defines the record format used to store LQA ranges
in indexed storage. Each range is stored as a separate IAM record with
the range start ID as the key and this structure as the record value.
Range indexes are stored as "ranges" IAM index in the pool directory:
quota_master/
├── lqa-dt-<lqa_name>/ # Data target pool for <lqa_name>
│ └── ranges # Binary file containing DT ranges
└── lqa-md-<lqa_name>/ # Metadata target pool for <lqa_name>
└── ranges # Binary file containing MD ranges
Signed-off-by: Artem Blagodarenko <ablagodarenko@ddn.com>
Change-Id: Ibefc9d292d5e44d6814e73cc9b48adfedaf70743
Reviewed-on: https://review.whamcloud.com/c/fs/lustre-release/+/61239
Reviewed-by: Sergey Cheremencev <scherementsev@ddn.com>
Tested-by: Andreas Dilger <adilger@thelustrecollective.com>
Reviewed-by: Oleg Drokin <green@whamcloud.com>
Tested-by: Maloo <maloo@whamcloud.com>
Reviewed-by: Andreas Dilger <adilger@thelustrecollective.com>
LU-20484 mdt: add readpage counter to md_stats mdt_readpage() handles directory listing (readdir) requests on the MDS, but unlike other MDT operations it was not tracked in md_stats. Add LPROC_MDT_READPAGE to record the latency of successful readpage calls, making readdir visible in 'lctl get_param mdt.*.md_stats'. Update sanity test_133a to verify the new "readpage" counter is incremented after a directory listing. Signed-off-by: Artem Blagodarenko <ablagodarenko@thelustrecollective.com> Change-Id: I1dc38b6f54a7802c31fb1dfc7170760bc49e1a5f Reviewed-on: https://review.whamcloud.com/c/fs/lustre-release/+/67333 Reviewed-by: Andreas Dilger <adilger@thelustrecollective.com> Reviewed-by: Oleg Drokin <green@whamcloud.com> Tested-by: jenkins <devops@whamcloud.com> Reviewed-by: Alexey Lyashkov <alexey.lyashkov@hpe.com> Tested-by: Maloo <maloo@whamcloud.com>
LU-20484 mdt: fix false error log in mdt_dir_read_on_open
mo_readpage returns bytes filled on success, so the condition
"if (rc)" triggered the "failed" CDEBUG message on every successful
read. Change to "if (rc < 0)" to log only real errors.
Fixes: 4a14a51a8e ("LU-18448 llite: read dir on open")
Signed-off-by: Artem Blagodarenko <ablagodarenko@thelustrecollective.com>
Change-Id: I552bea9c4e995111ed5cca5613c7fb52c6c70da5
Reviewed-on: https://review.whamcloud.com/c/fs/lustre-release/+/67332
Reviewed-by: Andreas Dilger <adilger@thelustrecollective.com>
Tested-by: jenkins <devops@whamcloud.com>
Reviewed-by: Oleg Drokin <green@whamcloud.com>
Tested-by: Maloo <maloo@whamcloud.com>
Reviewed-by: Alexey Lyashkov <alexey.lyashkov@hpe.com>