)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"22b980dcdf1087ec4eee9727354fceb4dd44cb60","unresolved":true,"context_lines":[{"line_number":7,"context_line":"Offload reads in the object server"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"An object-server worker serves all its concurrent requests on one"},{"line_number":10,"context_line":"eventlet hub. A GET read() that misses the page cache blocks the hub,"},{"line_number":11,"context_line":"stalling every other request on the worker - even cache hits. Using"},{"line_number":12,"context_line":"cooperative_period does not help: a yield cannot wake a thread stuck in"},{"line_number":13,"context_line":"read()."},{"line_number":14,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"4c953ab4_2e55ccc0","line":11,"range":{"start_line":10,"start_character":14,"end_line":11,"end_character":61},"updated":"2026-06-18 00:05:03.000000000","message":"OK, this is the key insight (for me, at least). This is also why we don\u0027t need to look at doing any comparable thing for the write path (y\u0027know, apart from the existing tpooling around fsync).\n\nBut *maybe* there\u0027s an argument that at least the `os.listdir` of the hashdir should go in the same thread pool?","commit_id":"8096eb6994f1a2c467ed11c944b84b182595c06a"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"f0f35b6437b03224f68d87d8e325ab11c660810b","unresolved":true,"context_lines":[{"line_number":7,"context_line":"Offload reads in the object server"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"An object-server worker serves all its concurrent requests on one"},{"line_number":10,"context_line":"eventlet hub. A GET read() that misses the page cache blocks the hub,"},{"line_number":11,"context_line":"stalling every other request on the worker - even cache hits. Using"},{"line_number":12,"context_line":"cooperative_period does not help: a yield cannot wake a thread stuck in"},{"line_number":13,"context_line":"read()."},{"line_number":14,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"d4e70fdb_4cbd232c","line":11,"range":{"start_line":10,"start_character":14,"end_line":11,"end_character":61},"in_reply_to":"4c953ab4_2e55ccc0","updated":"2026-08-04 10:53:37.000000000","message":"I think putting the `os.listdir` is out of scope (at least for this patch?). \n\nAn object GET looks like this:\n\n1. List the hash directory to find which file to serve using `os.listdir`\n2. Open that file and read its metadata from xattrs\n3. Read the data - what this patch is about\n\nWe only know the size after reading the metadata. Thus moving `os.listdir` into the thread pool would result in a handoff for every request, not only for the large ones.","commit_id":"8096eb6994f1a2c467ed11c944b84b182595c06a"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"05e9c715f498b4bb646968370ea76c2507f79eed","unresolved":true,"context_lines":[{"line_number":19,"context_line":"(a serial hub-side md5 would otherwise become the next bottleneck under"},{"line_number":20,"context_line":"concurrency)."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"Only reads of at least read_offload_min_size (default one burst) are"},{"line_number":23,"context_line":"offloaded; smaller reads, and small ranges of big objects, stay on the"},{"line_number":24,"context_line":"direct path, where a single sub-burst handoff would only add cost."},{"line_number":25,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"60461512_dc559273","line":22,"range":{"start_line":22,"start_character":23,"end_line":22,"end_character":44},"updated":"2026-06-29 14:58:59.000000000","message":"IIUC the config tuning trade-off is:\n\n* smaller ``read_offload_min_size`` -\u003e larger *average* GET time due to offloading relatively short GETs\n\n* larger ``read_offload_min_size`` -\u003e larger *peak* GET read time due to more GETs blocked behind slow ``read()``s","commit_id":"8096eb6994f1a2c467ed11c944b84b182595c06a"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"f0f35b6437b03224f68d87d8e325ab11c660810b","unresolved":true,"context_lines":[{"line_number":19,"context_line":"(a serial hub-side md5 would otherwise become the next bottleneck under"},{"line_number":20,"context_line":"concurrency)."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"Only reads of at least read_offload_min_size (default one burst) are"},{"line_number":23,"context_line":"offloaded; smaller reads, and small ranges of big objects, stay on the"},{"line_number":24,"context_line":"direct path, where a single sub-burst handoff would only add cost."},{"line_number":25,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"96ce2c23_929a1d8e","line":22,"range":{"start_line":22,"start_character":23,"end_line":22,"end_character":44},"in_reply_to":"60461512_dc559273","updated":"2026-08-04 10:53:37.000000000","message":"Yes, that\u0027s the tradeoff. Will update the commit msg to be a bit more detailed about this.","commit_id":"8096eb6994f1a2c467ed11c944b84b182595c06a"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"05e9c715f498b4bb646968370ea76c2507f79eed","unresolved":true,"context_lines":[{"line_number":21,"context_line":""},{"line_number":22,"context_line":"Only reads of at least read_offload_min_size (default one burst) are"},{"line_number":23,"context_line":"offloaded; smaller reads, and small ranges of big objects, stay on the"},{"line_number":24,"context_line":"direct path, where a single sub-burst handoff would only add cost."},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"Unlike the old threads_per_disk (which offloaded every 64 KiB read),"},{"line_number":27,"context_line":"each offload is a 1 MiB burst, so the handoff stays cheap even on a"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"48a73dbd_2747e23f","line":24,"updated":"2026-06-29 14:58:59.000000000","message":"ok, so this reduces the probability of a slow read() blocking other GETs, but does not eliminate them because it would be expensive to do so for short GETs.\n\nCached object GETs with 1MiB \u003c size \u003c 5 MiB may be run in a tpool thread.","commit_id":"8096eb6994f1a2c467ed11c944b84b182595c06a"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"f0f35b6437b03224f68d87d8e325ab11c660810b","unresolved":true,"context_lines":[{"line_number":21,"context_line":""},{"line_number":22,"context_line":"Only reads of at least read_offload_min_size (default one burst) are"},{"line_number":23,"context_line":"offloaded; smaller reads, and small ranges of big objects, stay on the"},{"line_number":24,"context_line":"direct path, where a single sub-burst handoff would only add cost."},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"Unlike the old threads_per_disk (which offloaded every 64 KiB read),"},{"line_number":27,"context_line":"each offload is a 1 MiB burst, so the handoff stays cheap even on a"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"4d4e381d_b48ee58a","line":24,"in_reply_to":"48a73dbd_2747e23f","updated":"2026-08-04 10:53:37.000000000","message":"Yes - though the 1MiB/5MiB size is tunable; the \"best\" option depends on how likely a cache hit is. More likely -\u003e probably better to use 5MiB `read_offload_min_size`; more unlikely cache hit -\u003e likely better to stay at 1MiB (which was the case in my tests).","commit_id":"8096eb6994f1a2c467ed11c944b84b182595c06a"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"05e9c715f498b4bb646968370ea76c2507f79eed","unresolved":true,"context_lines":[{"line_number":26,"context_line":"Unlike the old threads_per_disk (which offloaded every 64 KiB read),"},{"line_number":27,"context_line":"each offload is a 1 MiB burst, so the handoff stays cheap even on a"},{"line_number":28,"context_line":"cache hit. A per-device cap limits how many pool threads one slow disk"},{"line_number":29,"context_line":"can take from the process-global pool."},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"Defaults to off (behaviour unchanged)."},{"line_number":32,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"76c70424_c810e230","line":29,"updated":"2026-06-29 14:58:59.000000000","message":"with ``servers_per_port`` do we not end up with one disk per process-global pool?","commit_id":"8096eb6994f1a2c467ed11c944b84b182595c06a"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"f0f35b6437b03224f68d87d8e325ab11c660810b","unresolved":true,"context_lines":[{"line_number":26,"context_line":"Unlike the old threads_per_disk (which offloaded every 64 KiB read),"},{"line_number":27,"context_line":"each offload is a 1 MiB burst, so the handoff stays cheap even on a"},{"line_number":28,"context_line":"cache hit. A per-device cap limits how many pool threads one slow disk"},{"line_number":29,"context_line":"can take from the process-global pool."},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"Defaults to off (behaviour unchanged)."},{"line_number":32,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"4cbcc330_e66d3ab6","line":29,"in_reply_to":"76c70424_c810e230","updated":"2026-08-04 10:53:37.000000000","message":"Yes, that\u0027s right. Will clarify.","commit_id":"8096eb6994f1a2c467ed11c944b84b182595c06a"},{"author":{"_account_id":38767,"name":"Wael Halbawi","display_name":"Wael Halbawi","email":"whalbawi@nvidia.com","username":"whalbawi"},"change_message_id":"26eea5d33a47526e2f78f22199b87e3c5299e180","unresolved":true,"context_lines":[{"line_number":16,"context_line":"eventlet thread pool. os.pread() and hashlib release the GIL, so the"},{"line_number":17,"context_line":"hub can serve other requests at the same time. The pool thread also"},{"line_number":18,"context_line":"calculates the MD5, because on the hub, the MD5 limits the throughput."},{"line_number":19,"context_line":"It also drops each burst from the page cache. A drop from the hub can"},{"line_number":20,"context_line":"make the kernel drain the LRU caches of all CPUs."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"A burst is read_offload_burst_size bytes, 1 MiB by default. The old"},{"line_number":23,"context_line":"threads_per_disk option sent each 64 KiB read to a thread, and the"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"f32d6232_b10de39e","line":20,"range":{"start_line":19,"start_character":46,"end_line":20,"end_character":49},"updated":"2026-10-05 02:43:08.000000000","message":"Can you expand here a bit further? Which LRU cache is this sentence referring to?","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"},{"author":{"_account_id":38767,"name":"Wael Halbawi","display_name":"Wael Halbawi","email":"whalbawi@nvidia.com","username":"whalbawi"},"change_message_id":"3d16eb713e1c6e78eb534c4a671ae38591b6fdd9","unresolved":false,"context_lines":[{"line_number":16,"context_line":"eventlet thread pool. os.pread() and hashlib release the GIL, so the"},{"line_number":17,"context_line":"hub can serve other requests at the same time. The pool thread also"},{"line_number":18,"context_line":"calculates the MD5, because on the hub, the MD5 limits the throughput."},{"line_number":19,"context_line":"It also drops each burst from the page cache. A drop from the hub can"},{"line_number":20,"context_line":"make the kernel drain the LRU caches of all CPUs."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"A burst is read_offload_burst_size bytes, 1 MiB by default. The old"},{"line_number":23,"context_line":"threads_per_disk option sent each 64 KiB read to a thread, and the"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"bf0ca18e_238bd6d9","line":20,"range":{"start_line":19,"start_character":46,"end_line":20,"end_character":49},"in_reply_to":"544917e2_d0847e6b","updated":"2026-10-05 19:52:31.000000000","message":"OK I get it now. This might be appropriate motivation for establishing process/thread -\u003e CPU affinity even for the non-offloaded case.","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"7091e46e21ddf2df31f3d409e3eb6c95d685e0ad","unresolved":true,"context_lines":[{"line_number":16,"context_line":"eventlet thread pool. os.pread() and hashlib release the GIL, so the"},{"line_number":17,"context_line":"hub can serve other requests at the same time. The pool thread also"},{"line_number":18,"context_line":"calculates the MD5, because on the hub, the MD5 limits the throughput."},{"line_number":19,"context_line":"It also drops each burst from the page cache. A drop from the hub can"},{"line_number":20,"context_line":"make the kernel drain the LRU caches of all CPUs."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"A burst is read_offload_burst_size bytes, 1 MiB by default. The old"},{"line_number":23,"context_line":"threads_per_disk option sent each 64 KiB read to a thread, and the"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"544917e2_d0847e6b","line":20,"range":{"start_line":19,"start_character":46,"end_line":20,"end_character":49},"in_reply_to":"f32d6232_b10de39e","updated":"2026-10-05 13:13:36.000000000","message":"The kernel keeps a small LRU batch for each logical CPU (mm/swap.c). A read adds the new page cache pages to the batch of its own CPU. drop_buffer_cache() calls posix_fadvise() with POSIX_FADV_DONTNEED. Then the kernel drains only the batch of the CPU that makes the call. If a page is still in the batch of a different CPU, the kernel calls lru_add_drain_all(). That function drains the batches of all CPUs and waits until all of them are done.","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"fc4ffdc7e5084ec075ef4cc6391ca6cdad7590ee","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"cc36e30c_c2801d4b","updated":"2026-06-16 16:04:54.000000000","message":"recheck\n\ndevstack deployment error:  Downloading jmespath-1.1.0-py3-none-any.whl.metadata (7.6 kB)\nERROR: Could not install packages due to an OSError: (\u0027Connection broken: IncompleteRead(6389 bytes read, 1707 more expected)\u0027, IncompleteRead(6389 bytes read, 1707 more expected))","commit_id":"8096eb6994f1a2c467ed11c944b84b182595c06a"},{"author":{"_account_id":38767,"name":"Wael Halbawi","display_name":"Wael Halbawi","email":"whalbawi@nvidia.com","username":"whalbawi"},"change_message_id":"26eea5d33a47526e2f78f22199b87e3c5299e180","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"abec4680_bdcf05fa","updated":"2026-10-05 02:43:08.000000000","message":"I think the approach is fine overall. My main suggestion is to consider unifying the driver loops that schedule the IOs so that we do error-handling and cache manipulation in one place.","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"9ca58144dcb9fa86aaa6bbf4f264aede5455fe97","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"311dc7aa_0f8f63b5","updated":"2026-10-06 10:00:48.000000000","message":"I don\u0027t want to upload a new patchset at this point because of the multiple +1s, but before merging this we might want to squash this into it: 1009011: sq: keep the direct read path unchanged | https://review.opendev.org/c/openstack/swift/+/1009011\n\nIt reduces the code movements in diskfile.py quite a bit, which is both helpful when reviewing as well as later when using `git show`.\n\nHave a look at the output of `git diff HEAD~2 -- swift/obj/diskfile.py` with the sq-patch checked out.","commit_id":"db38722dc951c075dd31ea9c4cc992c42b92b41f"},{"author":{"_account_id":38767,"name":"Wael Halbawi","display_name":"Wael Halbawi","email":"whalbawi@nvidia.com","username":"whalbawi"},"change_message_id":"3d16eb713e1c6e78eb534c4a671ae38591b6fdd9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"cc40234a_2a453485","updated":"2026-10-05 19:52:31.000000000","message":"LGTM. The patch defaults offload reads to `False` and my outstanding comments are mostly concerned with code organization rather than performance/correctness.","commit_id":"db38722dc951c075dd31ea9c4cc992c42b92b41f"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"49c43d2f3a392d9d07f6d20e8330ff24f04feae8","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"f1bc4eaf_9e3f45ec","updated":"2026-10-07 14:02:08.000000000","message":"Nice work!\n\n-1 for:\n* the duplicated conf parsing: see inline diff\n\n* tpool.execute un-patching : see 1009247: test_diskfile: only mock tpool.execute when required | https://review.opendev.org/c/openstack/swift/+/1009247\n\nI would also prefer the _read_burst arg order to be made consistent with pread.\n\nI found the test that I particularly tried to debug quite confusing because the default diskfile setup uses different defaults than the DiskfileManager init (i.e. set min burst size to zero).","commit_id":"db38722dc951c075dd31ea9c4cc992c42b92b41f"},{"author":{"_account_id":38368,"name":"Christian Ohanaja","display_name":"Christian Ohanaja","email":"cohanaja@nvidia.com","username":"cohanaja"},"change_message_id":"28a13f958b3b2308346294fa977ef0b06e37ba71","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"cc79d957_40a6a134","updated":"2026-10-06 04:34:34.000000000","message":"makes sense, thanks 4 the responses! I still wish `DEFAULT_READ_OFFLOAD_MAX_INFLIGHT` matched `THREADPOOL_SIZE` but I figure that difference can be configged away in the end anyway","commit_id":"db38722dc951c075dd31ea9c4cc992c42b92b41f"}],"etc/object-server.conf-sample":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"22b980dcdf1087ec4eee9727354fceb4dd44cb60","unresolved":true,"context_lines":[{"line_number":169,"context_line":"# read_offload_min_size \u003d 1048576"},{"line_number":170,"context_line":"#"},{"line_number":171,"context_line":"# Cap on concurrent offloaded reads per device: enough to keep the device busy,"},{"line_number":172,"context_line":"# low enough that one slow disk can\u0027t exhaust the thread pool and starve fsync"},{"line_number":173,"context_line":"# or other devices. Note this bound is per-device, not node-wide: the pool is"},{"line_number":174,"context_line":"# process-global and shared with the fsync offload, so up to (cap x number of"},{"line_number":175,"context_line":"# devices) reads can contend for it at once. Size eventlet_tpool_num_threads"},{"line_number":176,"context_line":"# below (or EVENTLET_THREADPOOL_SIZE when Swift leaves eventlet\u0027s default in"}],"source_content_type":"application/octet-stream","patch_set":2,"id":"8f0208cd_258d4798","line":173,"range":{"start_line":172,"start_character":18,"end_line":173,"end_character":18},"updated":"2026-06-18 00:05:03.000000000","message":"But it\u0027s per-device, right? One slow disk affects everything *in the affected device\u0027s pool* regardless.\n\nI don\u0027t doubt that there should be *some* limit (and likely a fairly low one, looking at how we\u0027ve tuned `max_clients` for our own clusters), but I\u0027m not sold on the reasoning here yet.","commit_id":"8096eb6994f1a2c467ed11c944b84b182595c06a"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"f0f35b6437b03224f68d87d8e325ab11c660810b","unresolved":true,"context_lines":[{"line_number":169,"context_line":"# read_offload_min_size \u003d 1048576"},{"line_number":170,"context_line":"#"},{"line_number":171,"context_line":"# Cap on concurrent offloaded reads per device: enough to keep the device busy,"},{"line_number":172,"context_line":"# low enough that one slow disk can\u0027t exhaust the thread pool and starve fsync"},{"line_number":173,"context_line":"# or other devices. Note this bound is per-device, not node-wide: the pool is"},{"line_number":174,"context_line":"# process-global and shared with the fsync offload, so up to (cap x number of"},{"line_number":175,"context_line":"# devices) reads can contend for it at once. Size eventlet_tpool_num_threads"},{"line_number":176,"context_line":"# below (or EVENTLET_THREADPOOL_SIZE when Swift leaves eventlet\u0027s default in"}],"source_content_type":"application/octet-stream","patch_set":2,"id":"b36055c9_8a8850fa","line":173,"range":{"start_line":172,"start_character":18,"end_line":173,"end_character":18},"in_reply_to":"8f0208cd_258d4798","updated":"2026-08-04 10:53:37.000000000","message":"There is no per-device pool; one process-global tpool shared with fsync. The cap stops a bad disk taking all 20 threads from healthy disks. Will reword.","commit_id":"8096eb6994f1a2c467ed11c944b84b182595c06a"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"22b980dcdf1087ec4eee9727354fceb4dd44cb60","unresolved":true,"context_lines":[{"line_number":171,"context_line":"# Cap on concurrent offloaded reads per device: enough to keep the device busy,"},{"line_number":172,"context_line":"# low enough that one slow disk can\u0027t exhaust the thread pool and starve fsync"},{"line_number":173,"context_line":"# or other devices. Note this bound is per-device, not node-wide: the pool is"},{"line_number":174,"context_line":"# process-global and shared with the fsync offload, so up to (cap x number of"},{"line_number":175,"context_line":"# devices) reads can contend for it at once. Size eventlet_tpool_num_threads"},{"line_number":176,"context_line":"# below (or EVENTLET_THREADPOOL_SIZE when Swift leaves eventlet\u0027s default in"},{"line_number":177,"context_line":"# effect) for that total plus headroom for fsync, otherwise many busy disks can"},{"line_number":178,"context_line":"# collectively saturate the pool even when no single disk exceeds its cap. When"}],"source_content_type":"application/octet-stream","patch_set":2,"id":"d30447a4_a1c09956","line":175,"range":{"start_line":174,"start_character":62,"end_line":175,"end_character":9},"updated":"2026-06-18 00:05:03.000000000","message":"... x servers_per_port, right?","commit_id":"8096eb6994f1a2c467ed11c944b84b182595c06a"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"f0f35b6437b03224f68d87d8e325ab11c660810b","unresolved":true,"context_lines":[{"line_number":171,"context_line":"# Cap on concurrent offloaded reads per device: enough to keep the device busy,"},{"line_number":172,"context_line":"# low enough that one slow disk can\u0027t exhaust the thread pool and starve fsync"},{"line_number":173,"context_line":"# or other devices. Note this bound is per-device, not node-wide: the pool is"},{"line_number":174,"context_line":"# process-global and shared with the fsync offload, so up to (cap x number of"},{"line_number":175,"context_line":"# devices) reads can contend for it at once. Size eventlet_tpool_num_threads"},{"line_number":176,"context_line":"# below (or EVENTLET_THREADPOOL_SIZE when Swift leaves eventlet\u0027s default in"},{"line_number":177,"context_line":"# effect) for that total plus headroom for fsync, otherwise many busy disks can"},{"line_number":178,"context_line":"# collectively saturate the pool even when no single disk exceeds its cap. When"}],"source_content_type":"application/octet-stream","patch_set":2,"id":"73fe7ee8_7afcf70a","line":175,"range":{"start_line":174,"start_character":62,"end_line":175,"end_character":9},"in_reply_to":"d30447a4_a1c09956","updated":"2026-08-04 10:53:37.000000000","message":"Per pool it\u0027s cap x devices-served-by-that-process. Rewording.","commit_id":"8096eb6994f1a2c467ed11c944b84b182595c06a"}],"swift/obj/diskfile.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"05e9c715f498b4bb646968370ea76c2507f79eed","unresolved":true,"context_lines":[{"line_number":2237,"context_line":"    def _offload_chunk_checks(self, chunk):"},{"line_number":2238,"context_line":"        \"\"\""},{"line_number":2239,"context_line":"        Per-chunk validation that must run on the hub, not the offload thread"},{"line_number":2240,"context_line":"        (EC frag checks can quarantine). The etag is folded in by"},{"line_number":2241,"context_line":"        :meth:`_read_burst`; base does nothing."},{"line_number":2242,"context_line":"        \"\"\""},{"line_number":2243,"context_line":"        pass"}],"source_content_type":"text/x-python","patch_set":2,"id":"2a262168_5ff32afd","line":2240,"range":{"start_line":2240,"start_character":53,"end_line":2240,"end_character":62},"updated":"2026-06-29 14:58:59.000000000","message":"\"folded in\" isn\u0027t a term I\u0027m familiar with as an alternative to \"updated\"","commit_id":"8096eb6994f1a2c467ed11c944b84b182595c06a"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"f0f35b6437b03224f68d87d8e325ab11c660810b","unresolved":true,"context_lines":[{"line_number":2237,"context_line":"    def _offload_chunk_checks(self, chunk):"},{"line_number":2238,"context_line":"        \"\"\""},{"line_number":2239,"context_line":"        Per-chunk validation that must run on the hub, not the offload thread"},{"line_number":2240,"context_line":"        (EC frag checks can quarantine). The etag is folded in by"},{"line_number":2241,"context_line":"        :meth:`_read_burst`; base does nothing."},{"line_number":2242,"context_line":"        \"\"\""},{"line_number":2243,"context_line":"        pass"}],"source_content_type":"text/x-python","patch_set":2,"id":"c6c08bfd_29d3a0af","line":2240,"range":{"start_line":2240,"start_character":53,"end_line":2240,"end_character":62},"in_reply_to":"2a262168_5ff32afd","updated":"2026-08-04 10:53:37.000000000","message":"Rewording","commit_id":"8096eb6994f1a2c467ed11c944b84b182595c06a"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"05e9c715f498b4bb646968370ea76c2507f79eed","unresolved":true,"context_lines":[{"line_number":2248,"context_line":""},{"line_number":2249,"context_line":"    def _read_burst(self, fd, offset, nbytes):"},{"line_number":2250,"context_line":"        \"\"\""},{"line_number":2251,"context_line":"        Read ``nbytes`` at ``offset`` and fold them into the etag (``b\u0027\u0027`` at"},{"line_number":2252,"context_line":"        EOF). On a tpool worker: pread and md5 both release the GIL. pread (not"},{"line_number":2253,"context_line":"        read) keeps pool threads off the shared file position; bursts are"},{"line_number":2254,"context_line":"        sequential, so ``_iter_etag`` is never updated concurrently."}],"source_content_type":"text/x-python","patch_set":2,"id":"b6ddc989_1ce9d955","line":2251,"range":{"start_line":2251,"start_character":42,"end_line":2251,"end_character":56},"updated":"2026-06-29 14:58:59.000000000","message":"ditto re use of \"fold\" ?","commit_id":"8096eb6994f1a2c467ed11c944b84b182595c06a"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"f0f35b6437b03224f68d87d8e325ab11c660810b","unresolved":true,"context_lines":[{"line_number":2248,"context_line":""},{"line_number":2249,"context_line":"    def _read_burst(self, fd, offset, nbytes):"},{"line_number":2250,"context_line":"        \"\"\""},{"line_number":2251,"context_line":"        Read ``nbytes`` at ``offset`` and fold them into the etag (``b\u0027\u0027`` at"},{"line_number":2252,"context_line":"        EOF). On a tpool worker: pread and md5 both release the GIL. pread (not"},{"line_number":2253,"context_line":"        read) keeps pool threads off the shared file position; bursts are"},{"line_number":2254,"context_line":"        sequential, so ``_iter_etag`` is never updated concurrently."}],"source_content_type":"text/x-python","patch_set":2,"id":"141dd639_6069dac1","line":2251,"range":{"start_line":2251,"start_character":42,"end_line":2251,"end_character":56},"in_reply_to":"b6ddc989_1ce9d955","updated":"2026-08-04 10:53:37.000000000","message":"Ditto rewording","commit_id":"8096eb6994f1a2c467ed11c944b84b182595c06a"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"05e9c715f498b4bb646968370ea76c2507f79eed","unresolved":true,"context_lines":[{"line_number":2303,"context_line":"                    chunk \u003d buf[i:i + self._disk_chunk_size]"},{"line_number":2304,"context_line":"                    validate_chunk(chunk)"},{"line_number":2305,"context_line":"                    self._bytes_read +\u003d len(chunk)"},{"line_number":2306,"context_line":"                    if self._bytes_read - dropped_cache \u003e DROP_CACHE_WINDOW:"},{"line_number":2307,"context_line":"                        self._drop_cache("},{"line_number":2308,"context_line":"                            self._fp.fileno(), start_offset + dropped_cache,"},{"line_number":2309,"context_line":"                            self._bytes_read - dropped_cache)"}],"source_content_type":"text/x-python","patch_set":2,"id":"08677d65_5a35c073","line":2306,"updated":"2026-06-29 14:58:59.000000000","message":"note: this test is less timely than on master if ``_read_offload_burst size !\u003d DROP_CACHE_WINDOW``. \n\nIn practice, due to the batching of chunks in a burst, ``effective drop_cache_window \u003d\u003d max(DROP_CACHE_WINDOW, _read_offload_burst size)``\n\nCould we move drop_cache inside the burst iterators?","commit_id":"8096eb6994f1a2c467ed11c944b84b182595c06a"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"f0f35b6437b03224f68d87d8e325ab11c660810b","unresolved":true,"context_lines":[{"line_number":2303,"context_line":"                    chunk \u003d buf[i:i + self._disk_chunk_size]"},{"line_number":2304,"context_line":"                    validate_chunk(chunk)"},{"line_number":2305,"context_line":"                    self._bytes_read +\u003d len(chunk)"},{"line_number":2306,"context_line":"                    if self._bytes_read - dropped_cache \u003e DROP_CACHE_WINDOW:"},{"line_number":2307,"context_line":"                        self._drop_cache("},{"line_number":2308,"context_line":"                            self._fp.fileno(), start_offset + dropped_cache,"},{"line_number":2309,"context_line":"                            self._bytes_read - dropped_cache)"}],"source_content_type":"text/x-python","patch_set":2,"id":"8dd36631_5b6a27c7","line":2306,"in_reply_to":"08677d65_5a35c073","updated":"2026-08-04 10:53:37.000000000","message":"Right, will move drop_cache.","commit_id":"8096eb6994f1a2c467ed11c944b84b182595c06a"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"05e9c715f498b4bb646968370ea76c2507f79eed","unresolved":true,"context_lines":[{"line_number":2310,"context_line":"                        dropped_cache \u003d self._bytes_read"},{"line_number":2311,"context_line":"                    yield chunk"},{"line_number":2312,"context_line":"            self._drop_cache(self._fp.fileno(), start_offset + dropped_cache,"},{"line_number":2313,"context_line":"                             self._bytes_read - dropped_cache)"},{"line_number":2314,"context_line":"        finally:"},{"line_number":2315,"context_line":"            if not self._suppress_file_closing:"},{"line_number":2316,"context_line":"                self.close()"}],"source_content_type":"text/x-python","patch_set":2,"id":"575d8ef1_54e143ab","line":2313,"updated":"2026-06-29 14:58:59.000000000","message":"note to self: this extra drop_cache (here and on master) is necessary for the case when the final burst did not exceed DROP_CACHE_WINDOW","commit_id":"8096eb6994f1a2c467ed11c944b84b182595c06a"},{"author":{"_account_id":39164,"name":"Matthew Allen","display_name":"Matthew Allen","email":"maallen@nvidia.com","username":"matthewallen"},"change_message_id":"28f9fd4a3c8707c2ba7f8837951057790f93b989","unresolved":true,"context_lines":[{"line_number":3198,"context_line":"            pipe_size\u003dself._pipe_size, diskfile\u003dself, keep_cache\u003dkeep_cache,"},{"line_number":3199,"context_line":"            cooperative_period\u003dcooperative_period,"},{"line_number":3200,"context_line":"            etag_validate_frac\u003detag_validate_frac,"},{"line_number":3201,"context_line":"            read_offload\u003dself._read_offload,"},{"line_number":3202,"context_line":"            read_offload_burst_size\u003dself._read_offload_burst_size,"},{"line_number":3203,"context_line":"            read_offload_max_inflight\u003dself._read_offload_max_inflight,"},{"line_number":3204,"context_line":"            read_offload_min_size\u003dself._read_offload_min_size)"},{"line_number":3205,"context_line":"        # At this point the reader object is now responsible for closing"},{"line_number":3206,"context_line":"        # the file pointer."},{"line_number":3207,"context_line":"        self._fp \u003d None"}],"source_content_type":"text/x-python","patch_set":2,"id":"ec3c1025_4cf2baf7","line":3204,"range":{"start_line":3201,"start_character":12,"end_line":3204,"end_character":61},"updated":"2026-08-07 21:08:27.000000000","message":"Agentic review surfaced the concern that an out-of-tree reader_cls or DiskFileManager that has not updated with this patch will break due to the extra parameters passed in. Discussed with @cschwede@nvidia.com and we think it is worth documenting in the commit message.","commit_id":"8096eb6994f1a2c467ed11c944b84b182595c06a"},{"author":{"_account_id":38767,"name":"Wael Halbawi","display_name":"Wael Halbawi","email":"whalbawi@nvidia.com","username":"whalbawi"},"change_message_id":"26eea5d33a47526e2f78f22199b87e3c5299e180","unresolved":true,"context_lines":[{"line_number":93,"context_line":"_device_offload_semaphores \u003d {}"},{"line_number":94,"context_line":""},{"line_number":95,"context_line":""},{"line_number":96,"context_line":"def get_device_offload_semaphore(device_path, cap):"},{"line_number":97,"context_line":"    \"\"\"Return the shared per-device offload Semaphore, creating it once.\"\"\""},{"line_number":98,"context_line":"    sem \u003d _device_offload_semaphores.get(device_path)"},{"line_number":99,"context_line":"    if sem is None:"}],"source_content_type":"text/x-python","patch_set":7,"id":"1f600471_a6739029","line":96,"range":{"start_line":96,"start_character":4,"end_line":96,"end_character":32},"updated":"2026-10-05 02:43:08.000000000","message":"There\u0027s a dependency on call sites to ensure that each device\u0027s semaphore has the same cap, assuming we care about such enforcement.","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"},{"author":{"_account_id":38767,"name":"Wael Halbawi","display_name":"Wael Halbawi","email":"whalbawi@nvidia.com","username":"whalbawi"},"change_message_id":"3d16eb713e1c6e78eb534c4a671ae38591b6fdd9","unresolved":false,"context_lines":[{"line_number":93,"context_line":"_device_offload_semaphores \u003d {}"},{"line_number":94,"context_line":""},{"line_number":95,"context_line":""},{"line_number":96,"context_line":"def get_device_offload_semaphore(device_path, cap):"},{"line_number":97,"context_line":"    \"\"\"Return the shared per-device offload Semaphore, creating it once.\"\"\""},{"line_number":98,"context_line":"    sem \u003d _device_offload_semaphores.get(device_path)"},{"line_number":99,"context_line":"    if sem is None:"}],"source_content_type":"text/x-python","patch_set":7,"id":"f261ed3f_3d32cdfb","line":96,"range":{"start_line":96,"start_character":4,"end_line":96,"end_character":32},"in_reply_to":"1f600471_a6739029","updated":"2026-10-05 19:52:31.000000000","message":"Acknowledged","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"},{"author":{"_account_id":38767,"name":"Wael Halbawi","display_name":"Wael Halbawi","email":"whalbawi@nvidia.com","username":"whalbawi"},"change_message_id":"26eea5d33a47526e2f78f22199b87e3c5299e180","unresolved":true,"context_lines":[{"line_number":2248,"context_line":"        \"\"\""},{"line_number":2249,"context_line":"        Read ``nbytes`` at ``offset``, update the etag with them and drop them"},{"line_number":2250,"context_line":"        from the page cache (``b\u0027\u0027`` at EOF). On a tpool worker: pread and md5"},{"line_number":2251,"context_line":"        both release the GIL. pread (not read) keeps pool threads off the"},{"line_number":2252,"context_line":"        shared file position; bursts are sequential, so ``_iter_etag`` is never"},{"line_number":2253,"context_line":"        updated concurrently."},{"line_number":2254,"context_line":"        \"\"\""},{"line_number":2255,"context_line":"        buf \u003d os.pread(fd, nbytes, offset)"}],"source_content_type":"text/x-python","patch_set":7,"id":"2e8051a2_7f6fc6cb","line":2252,"range":{"start_line":2251,"start_character":53,"end_line":2252,"end_character":28},"updated":"2026-10-05 02:43:08.000000000","message":"Nice to have but doesn\u0027t seem necessary for correctness because there\u0027s never more than one burst inflight for a given file?","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"},{"author":{"_account_id":38767,"name":"Wael Halbawi","display_name":"Wael Halbawi","email":"whalbawi@nvidia.com","username":"whalbawi"},"change_message_id":"accd38651e9d33da6f2d25b3144a5a1117d10554","unresolved":true,"context_lines":[{"line_number":2248,"context_line":"        \"\"\""},{"line_number":2249,"context_line":"        Read ``nbytes`` at ``offset``, update the etag with them and drop them"},{"line_number":2250,"context_line":"        from the page cache (``b\u0027\u0027`` at EOF). On a tpool worker: pread and md5"},{"line_number":2251,"context_line":"        both release the GIL. pread (not read) keeps pool threads off the"},{"line_number":2252,"context_line":"        shared file position; bursts are sequential, so ``_iter_etag`` is never"},{"line_number":2253,"context_line":"        updated concurrently."},{"line_number":2254,"context_line":"        \"\"\""},{"line_number":2255,"context_line":"        buf \u003d os.pread(fd, nbytes, offset)"}],"source_content_type":"text/x-python","patch_set":7,"id":"2b671f4c_d3cf3ea6","line":2252,"range":{"start_line":2251,"start_character":53,"end_line":2252,"end_character":28},"in_reply_to":"1f903386_52728459","updated":"2026-10-06 16:58:00.000000000","message":"I think the explanation makes sense. I\u0027m not sure though why this behavior would depend on the scheduling context. What am I missing?","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"7091e46e21ddf2df31f3d409e3eb6c95d685e0ad","unresolved":true,"context_lines":[{"line_number":2248,"context_line":"        \"\"\""},{"line_number":2249,"context_line":"        Read ``nbytes`` at ``offset``, update the etag with them and drop them"},{"line_number":2250,"context_line":"        from the page cache (``b\u0027\u0027`` at EOF). On a tpool worker: pread and md5"},{"line_number":2251,"context_line":"        both release the GIL. pread (not read) keeps pool threads off the"},{"line_number":2252,"context_line":"        shared file position; bursts are sequential, so ``_iter_etag`` is never"},{"line_number":2253,"context_line":"        updated concurrently."},{"line_number":2254,"context_line":"        \"\"\""},{"line_number":2255,"context_line":"        buf \u003d os.pread(fd, nbytes, offset)"}],"source_content_type":"text/x-python","patch_set":7,"id":"1f903386_52728459","line":2252,"range":{"start_line":2251,"start_character":53,"end_line":2252,"end_character":28},"in_reply_to":"2e8051a2_7f6fc6cb","updated":"2026-10-05 13:13:36.000000000","message":"Correct, only one burst is in flight. But `pread` is still necessary: self._fp reads ahead and keeps the extra data in a buffer. A `seek` inside that buffer moves only the position of self._fp, not the position of the fd. Thus `read` on the fd can read from the wrong offset, for example in a multi-range GET. `pread` gets the offset in the call, so it always reads the correct bytes.","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"},{"author":{"_account_id":38767,"name":"Wael Halbawi","display_name":"Wael Halbawi","email":"whalbawi@nvidia.com","username":"whalbawi"},"change_message_id":"26eea5d33a47526e2f78f22199b87e3c5299e180","unresolved":true,"context_lines":[{"line_number":2256,"context_line":"        if buf:"},{"line_number":2257,"context_line":"            if self._iter_etag is not None:"},{"line_number":2258,"context_line":"                self._iter_etag.update(buf)"},{"line_number":2259,"context_line":"            self._drop_cache(fd, offset, len(buf))"},{"line_number":2260,"context_line":"        return buf"},{"line_number":2261,"context_line":""},{"line_number":2262,"context_line":"    def _should_offload(self):"}],"source_content_type":"text/x-python","patch_set":7,"id":"a7e2daa1_ce5a87f8","line":2259,"range":{"start_line":2259,"start_character":0,"end_line":2259,"end_character":50},"updated":"2026-10-05 02:43:08.000000000","message":"This op is not subject to the `DROP_CACHE_WINDOW` check. Is this intentional? Is it because we\u0027ll already running on a separate thread and so we\u0027re not worried about the cost of a syscall?","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"},{"author":{"_account_id":38767,"name":"Wael Halbawi","display_name":"Wael Halbawi","email":"whalbawi@nvidia.com","username":"whalbawi"},"change_message_id":"accd38651e9d33da6f2d25b3144a5a1117d10554","unresolved":true,"context_lines":[{"line_number":2256,"context_line":"        if buf:"},{"line_number":2257,"context_line":"            if self._iter_etag is not None:"},{"line_number":2258,"context_line":"                self._iter_etag.update(buf)"},{"line_number":2259,"context_line":"            self._drop_cache(fd, offset, len(buf))"},{"line_number":2260,"context_line":"        return buf"},{"line_number":2261,"context_line":""},{"line_number":2262,"context_line":"    def _should_offload(self):"}],"source_content_type":"text/x-python","patch_set":7,"id":"b0614753_786c017f","line":2259,"range":{"start_line":2259,"start_character":0,"end_line":2259,"end_character":50},"in_reply_to":"335e8b1a_b38a6230","updated":"2026-10-06 16:58:00.000000000","message":"Makes sense for the scheduling. Might be worth considering retaining the `DROP_CACHE_WINDOW` check in this patch too. Such coalescing might eliminate any potential \"gains\" we get from dropping the cache on the same thread that scheduled the read, but that\u0027s the existing behavior anyway.\n\nFor now, the defaults line up nicely (`DROP_CACHE_WINDOW` \u003d\u003d `read_offload_burst_size` \u003d\u003d 1MiB) so the behavior in this case remains the same.","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"7091e46e21ddf2df31f3d409e3eb6c95d685e0ad","unresolved":true,"context_lines":[{"line_number":2256,"context_line":"        if buf:"},{"line_number":2257,"context_line":"            if self._iter_etag is not None:"},{"line_number":2258,"context_line":"                self._iter_etag.update(buf)"},{"line_number":2259,"context_line":"            self._drop_cache(fd, offset, len(buf))"},{"line_number":2260,"context_line":"        return buf"},{"line_number":2261,"context_line":""},{"line_number":2262,"context_line":"    def _should_offload(self):"}],"source_content_type":"text/x-python","patch_set":7,"id":"335e8b1a_b38a6230","line":2259,"range":{"start_line":2259,"start_character":0,"end_line":2259,"end_character":50},"in_reply_to":"a7e2daa1_ce5a87f8","updated":"2026-10-05 13:13:36.000000000","message":"Yes, the drop must run in the thread that read the pages - see Alistairs ask in PS2 as well.","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"},{"author":{"_account_id":38767,"name":"Wael Halbawi","display_name":"Wael Halbawi","email":"whalbawi@nvidia.com","username":"whalbawi"},"change_message_id":"26eea5d33a47526e2f78f22199b87e3c5299e180","unresolved":true,"context_lines":[{"line_number":2299,"context_line":"            self._init_checks()"},{"line_number":2300,"context_line":"            for buf in bursts:"},{"line_number":2301,"context_line":"                for i in range(0, len(buf), self._disk_chunk_size):"},{"line_number":2302,"context_line":"                    chunk \u003d buf[i:i + self._disk_chunk_size]"},{"line_number":2303,"context_line":"                    validate_chunk(chunk)"},{"line_number":2304,"context_line":"                    self._bytes_read +\u003d len(chunk)"},{"line_number":2305,"context_line":"                    yield chunk"}],"source_content_type":"text/x-python","patch_set":7,"id":"6477202f_7ace414b","line":2302,"updated":"2026-10-05 02:43:08.000000000","message":"Will such slicing allocate extra memory? I\u0027m seeing the following in my REPL:\n```\n\u003e\u003e\u003e import sys\n\u003e\u003e\u003e BUF_LEN \u003d 64 \u003c\u003c 10\n\u003e\u003e\u003e a \u003d bytes(BUF_LEN)\n\u003e\u003e\u003e b \u003d a[:len(a)]\n\u003e\u003e\u003e c \u003d a[:len(a)//2]\n\u003e\u003e\u003e sys.getsizeof(a)\n65569\n\u003e\u003e\u003e sys.getsizeof(b)\n65569\n\u003e\u003e\u003e sys.getsizeof(c)\n32801\n\u003e\u003e\u003e a is b\nTrue\n\u003e\u003e\u003e a is c\nFalse\n\u003e\u003e\u003e \n```","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"},{"author":{"_account_id":38767,"name":"Wael Halbawi","display_name":"Wael Halbawi","email":"whalbawi@nvidia.com","username":"whalbawi"},"change_message_id":"3d16eb713e1c6e78eb534c4a671ae38591b6fdd9","unresolved":false,"context_lines":[{"line_number":2299,"context_line":"            self._init_checks()"},{"line_number":2300,"context_line":"            for buf in bursts:"},{"line_number":2301,"context_line":"                for i in range(0, len(buf), self._disk_chunk_size):"},{"line_number":2302,"context_line":"                    chunk \u003d buf[i:i + self._disk_chunk_size]"},{"line_number":2303,"context_line":"                    validate_chunk(chunk)"},{"line_number":2304,"context_line":"                    self._bytes_read +\u003d len(chunk)"},{"line_number":2305,"context_line":"                    yield chunk"}],"source_content_type":"text/x-python","patch_set":7,"id":"e48b08a4_a4747631","line":2302,"in_reply_to":"5e205742_d026f590","updated":"2026-10-05 19:52:31.000000000","message":"Ack. For default settings it\u0027s an addition 64KiB per 1MiB of buffer. Doesn\u0027t seem to be problematic.","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"7091e46e21ddf2df31f3d409e3eb6c95d685e0ad","unresolved":true,"context_lines":[{"line_number":2299,"context_line":"            self._init_checks()"},{"line_number":2300,"context_line":"            for buf in bursts:"},{"line_number":2301,"context_line":"                for i in range(0, len(buf), self._disk_chunk_size):"},{"line_number":2302,"context_line":"                    chunk \u003d buf[i:i + self._disk_chunk_size]"},{"line_number":2303,"context_line":"                    validate_chunk(chunk)"},{"line_number":2304,"context_line":"                    self._bytes_read +\u003d len(chunk)"},{"line_number":2305,"context_line":"                    yield chunk"}],"source_content_type":"text/x-python","patch_set":7,"id":"5e205742_d026f590","line":2302,"in_reply_to":"6477202f_7ace414b","updated":"2026-10-05 13:13:36.000000000","message":"Yes, because a slice of a bytes object is a new copy (except if it is the full object).","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"},{"author":{"_account_id":38767,"name":"Wael Halbawi","display_name":"Wael Halbawi","email":"whalbawi@nvidia.com","username":"whalbawi"},"change_message_id":"26eea5d33a47526e2f78f22199b87e3c5299e180","unresolved":true,"context_lines":[{"line_number":2338,"context_line":"                dropped_cache \u003d bytes_read"},{"line_number":2339,"context_line":"            yield chunk"},{"line_number":2340,"context_line":""},{"line_number":2341,"context_line":"    def _offload_bursts(self):"},{"line_number":2342,"context_line":"        \"\"\""},{"line_number":2343,"context_line":"        Read+hash each burst on a tpool worker; the per-device semaphore bounds"},{"line_number":2344,"context_line":"        concurrent offloads. The explicit pread offset is advanced here."}],"source_content_type":"text/x-python","patch_set":7,"id":"644e0515_e53d725a","line":2341,"updated":"2026-10-05 02:43:08.000000000","message":"What prevents us from having a single loop that drives reads and internally decide how to schedule IOs, e.g. inline vs threadpool?","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"},{"author":{"_account_id":38767,"name":"Wael Halbawi","display_name":"Wael Halbawi","email":"whalbawi@nvidia.com","username":"whalbawi"},"change_message_id":"3d16eb713e1c6e78eb534c4a671ae38591b6fdd9","unresolved":false,"context_lines":[{"line_number":2338,"context_line":"                dropped_cache \u003d bytes_read"},{"line_number":2339,"context_line":"            yield chunk"},{"line_number":2340,"context_line":""},{"line_number":2341,"context_line":"    def _offload_bursts(self):"},{"line_number":2342,"context_line":"        \"\"\""},{"line_number":2343,"context_line":"        Read+hash each burst on a tpool worker; the per-device semaphore bounds"},{"line_number":2344,"context_line":"        concurrent offloads. The explicit pread offset is advanced here."}],"source_content_type":"text/x-python","patch_set":7,"id":"8909c981_3a81adee","line":2341,"in_reply_to":"056c0edf_2cfef411","updated":"2026-10-05 19:52:31.000000000","message":"Acknowledged","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"7091e46e21ddf2df31f3d409e3eb6c95d685e0ad","unresolved":true,"context_lines":[{"line_number":2338,"context_line":"                dropped_cache \u003d bytes_read"},{"line_number":2339,"context_line":"            yield chunk"},{"line_number":2340,"context_line":""},{"line_number":2341,"context_line":"    def _offload_bursts(self):"},{"line_number":2342,"context_line":"        \"\"\""},{"line_number":2343,"context_line":"        Read+hash each burst on a tpool worker; the per-device semaphore bounds"},{"line_number":2344,"context_line":"        concurrent offloads. The explicit pread offset is advanced here."}],"source_content_type":"text/x-python","patch_set":7,"id":"056c0edf_2cfef411","line":2341,"in_reply_to":"644e0515_e53d725a","updated":"2026-10-05 13:13:36.000000000","message":"We could do this - the current way looks a bit easier to read to me, and it keeps the default path (with read_offload \u003d False) like master.","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"},{"author":{"_account_id":38767,"name":"Wael Halbawi","display_name":"Wael Halbawi","email":"whalbawi@nvidia.com","username":"whalbawi"},"change_message_id":"26eea5d33a47526e2f78f22199b87e3c5299e180","unresolved":true,"context_lines":[{"line_number":2488,"context_line":"        else:"},{"line_number":2489,"context_line":"            length \u003d None"},{"line_number":2490,"context_line":"        # bytes this read transfers, for _should_offload (None \u003d\u003d to EOF)"},{"line_number":2491,"context_line":"        self._read_length \u003d length"},{"line_number":2492,"context_line":"        try:"},{"line_number":2493,"context_line":"            for chunk in self:"},{"line_number":2494,"context_line":"                if length is not None:"}],"source_content_type":"text/x-python","patch_set":7,"id":"bdddcaef_e24fd1a3","line":2491,"updated":"2026-10-05 02:43:08.000000000","message":"perhaps `_range_length` to signify this is specific to range reads.","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"},{"author":{"_account_id":38767,"name":"Wael Halbawi","display_name":"Wael Halbawi","email":"whalbawi@nvidia.com","username":"whalbawi"},"change_message_id":"3d16eb713e1c6e78eb534c4a671ae38591b6fdd9","unresolved":false,"context_lines":[{"line_number":2488,"context_line":"        else:"},{"line_number":2489,"context_line":"            length \u003d None"},{"line_number":2490,"context_line":"        # bytes this read transfers, for _should_offload (None \u003d\u003d to EOF)"},{"line_number":2491,"context_line":"        self._read_length \u003d length"},{"line_number":2492,"context_line":"        try:"},{"line_number":2493,"context_line":"            for chunk in self:"},{"line_number":2494,"context_line":"                if length is not None:"}],"source_content_type":"text/x-python","patch_set":7,"id":"0be4aa29_ab71f417","line":2491,"in_reply_to":"4655eba7_ca6352c7","updated":"2026-10-05 19:52:31.000000000","message":"Acknowledged","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"7091e46e21ddf2df31f3d409e3eb6c95d685e0ad","unresolved":true,"context_lines":[{"line_number":2488,"context_line":"        else:"},{"line_number":2489,"context_line":"            length \u003d None"},{"line_number":2490,"context_line":"        # bytes this read transfers, for _should_offload (None \u003d\u003d to EOF)"},{"line_number":2491,"context_line":"        self._read_length \u003d length"},{"line_number":2492,"context_line":"        try:"},{"line_number":2493,"context_line":"            for chunk in self:"},{"line_number":2494,"context_line":"                if length is not None:"}],"source_content_type":"text/x-python","patch_set":7,"id":"4655eba7_ca6352c7","line":2491,"in_reply_to":"bdddcaef_e24fd1a3","updated":"2026-10-05 13:13:36.000000000","message":"Good point, I will rename it.","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"},{"author":{"_account_id":38767,"name":"Wael Halbawi","display_name":"Wael Halbawi","email":"whalbawi@nvidia.com","username":"whalbawi"},"change_message_id":"26eea5d33a47526e2f78f22199b87e3c5299e180","unresolved":true,"context_lines":[{"line_number":2502,"context_line":"            if not self._suppress_file_closing:"},{"line_number":2503,"context_line":"                self.close()"},{"line_number":2504,"context_line":""},{"line_number":2505,"context_line":"    def app_iter_ranges(self, ranges, content_type, boundary, size):"},{"line_number":2506,"context_line":"        \"\"\""},{"line_number":2507,"context_line":"        Returns an iterator over the data file for a set of ranges"},{"line_number":2508,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"6203bc4b_f062bac2","line":2505,"updated":"2026-10-05 02:43:08.000000000","message":"If I understanding this correctly, a `range` is offloaded (or not) independent of the others in `ranges`, i.e., it\u0027s not a global decision for the whole iterator.","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"},{"author":{"_account_id":38767,"name":"Wael Halbawi","display_name":"Wael Halbawi","email":"whalbawi@nvidia.com","username":"whalbawi"},"change_message_id":"3d16eb713e1c6e78eb534c4a671ae38591b6fdd9","unresolved":false,"context_lines":[{"line_number":2502,"context_line":"            if not self._suppress_file_closing:"},{"line_number":2503,"context_line":"                self.close()"},{"line_number":2504,"context_line":""},{"line_number":2505,"context_line":"    def app_iter_ranges(self, ranges, content_type, boundary, size):"},{"line_number":2506,"context_line":"        \"\"\""},{"line_number":2507,"context_line":"        Returns an iterator over the data file for a set of ranges"},{"line_number":2508,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"a6260936_bdb0a3d8","line":2505,"in_reply_to":"0e4c789d_4fdc9fe5","updated":"2026-10-05 19:52:31.000000000","message":"Acknowledged","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"7091e46e21ddf2df31f3d409e3eb6c95d685e0ad","unresolved":true,"context_lines":[{"line_number":2502,"context_line":"            if not self._suppress_file_closing:"},{"line_number":2503,"context_line":"                self.close()"},{"line_number":2504,"context_line":""},{"line_number":2505,"context_line":"    def app_iter_ranges(self, ranges, content_type, boundary, size):"},{"line_number":2506,"context_line":"        \"\"\""},{"line_number":2507,"context_line":"        Returns an iterator over the data file for a set of ranges"},{"line_number":2508,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"0e4c789d_4fdc9fe5","line":2505,"in_reply_to":"6203bc4b_f062bac2","updated":"2026-10-05 13:13:36.000000000","message":"Yes, that\u0027s tight - `_should_offload` decides on each range length, and `app_iter_ranges` is called once per range.","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"},{"author":{"_account_id":38368,"name":"Christian Ohanaja","display_name":"Christian Ohanaja","email":"cohanaja@nvidia.com","username":"cohanaja"},"change_message_id":"464ef56720191925b6d955a9c59fac963948c257","unresolved":true,"context_lines":[{"line_number":2622,"context_line":"        self._device_path \u003d device_path"},{"line_number":2623,"context_line":"        self._logger \u003d mgr.logger"},{"line_number":2624,"context_line":"        self._disk_chunk_size \u003d mgr.disk_chunk_size"},{"line_number":2625,"context_line":"        self._read_offload \u003d mgr.read_offload"},{"line_number":2626,"context_line":"        self._read_offload_burst_size \u003d mgr.read_offload_burst_size"},{"line_number":2627,"context_line":"        self._read_offload_max_inflight \u003d \\"},{"line_number":2628,"context_line":"            mgr.read_offload_max_inflight_per_device"},{"line_number":2629,"context_line":"        self._read_offload_min_size \u003d mgr.read_offload_min_size"},{"line_number":2630,"context_line":"        self._bytes_per_sync \u003d mgr.bytes_per_sync"},{"line_number":2631,"context_line":"        self._use_splice \u003d use_splice"},{"line_number":2632,"context_line":"        self._pipe_size \u003d pipe_size"}],"source_content_type":"text/x-python","patch_set":7,"id":"2686c982_9b8012a9","line":2629,"range":{"start_line":2625,"start_character":8,"end_line":2629,"end_character":63},"updated":"2026-10-05 08:34:48.000000000","message":"from what I can tell we only really use these during reader(), we could probably cut out a step","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"7091e46e21ddf2df31f3d409e3eb6c95d685e0ad","unresolved":true,"context_lines":[{"line_number":2622,"context_line":"        self._device_path \u003d device_path"},{"line_number":2623,"context_line":"        self._logger \u003d mgr.logger"},{"line_number":2624,"context_line":"        self._disk_chunk_size \u003d mgr.disk_chunk_size"},{"line_number":2625,"context_line":"        self._read_offload \u003d mgr.read_offload"},{"line_number":2626,"context_line":"        self._read_offload_burst_size \u003d mgr.read_offload_burst_size"},{"line_number":2627,"context_line":"        self._read_offload_max_inflight \u003d \\"},{"line_number":2628,"context_line":"            mgr.read_offload_max_inflight_per_device"},{"line_number":2629,"context_line":"        self._read_offload_min_size \u003d mgr.read_offload_min_size"},{"line_number":2630,"context_line":"        self._bytes_per_sync \u003d mgr.bytes_per_sync"},{"line_number":2631,"context_line":"        self._use_splice \u003d use_splice"},{"line_number":2632,"context_line":"        self._pipe_size \u003d pipe_size"}],"source_content_type":"text/x-python","patch_set":7,"id":"610fa46c_8fac7793","line":2629,"range":{"start_line":2625,"start_character":8,"end_line":2629,"end_character":63},"in_reply_to":"2686c982_9b8012a9","updated":"2026-10-05 13:13:36.000000000","message":"Agreed. Only `reader()` uses these values, so `reader()` can get them from the manager, as it already does for keep_cache_size. I will remove the copies from `__init__`.","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"},{"author":{"_account_id":38368,"name":"Christian Ohanaja","display_name":"Christian Ohanaja","email":"cohanaja@nvidia.com","username":"cohanaja"},"change_message_id":"464ef56720191925b6d955a9c59fac963948c257","unresolved":true,"context_lines":[{"line_number":3229,"context_line":"            pipe_size\u003dself._pipe_size, diskfile\u003dself, keep_cache\u003dkeep_cache,"},{"line_number":3230,"context_line":"            cooperative_period\u003dcooperative_period,"},{"line_number":3231,"context_line":"            etag_validate_frac\u003detag_validate_frac,"},{"line_number":3232,"context_line":"            read_offload\u003dself._read_offload,"},{"line_number":3233,"context_line":"            read_offload_burst_size\u003dself._read_offload_burst_size,"},{"line_number":3234,"context_line":"            read_offload_max_inflight\u003dself._read_offload_max_inflight,"},{"line_number":3235,"context_line":"            read_offload_min_size\u003dself._read_offload_min_size)"},{"line_number":3236,"context_line":"        # At this point the reader object is now responsible for closing"},{"line_number":3237,"context_line":"        # the file pointer."},{"line_number":3238,"context_line":"        self._fp \u003d None"}],"source_content_type":"text/x-python","patch_set":7,"id":"ead46a7a_5d24e0c8","line":3235,"range":{"start_line":3232,"start_character":12,"end_line":3235,"end_character":62},"updated":"2026-10-05 08:34:48.000000000","message":"cutting out a step\n\n```\ndef reader(self):\n\tmgr \u003d self._manager\n\treturn self.reader_cls(\n\t\t...\n\t\tread_offload\u003dmgr.read_offload,\n\t\tread_offload_burst_size\u003dmgr.read_offload_burst_size,\n\t\tread_offload_max_inflight\u003dmgr.read_offload_max_inflight_per_device,\n\t\tread_offload_min_size\u003dmgr.read_offload_min_size\n\t)\n```","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"49c43d2f3a392d9d07f6d20e8330ff24f04feae8","unresolved":true,"context_lines":[{"line_number":87,"context_line":"METADATA_CHECKSUM_KEY \u003d b\u0027user.swift.metadata_checksum\u0027"},{"line_number":88,"context_line":"DROP_CACHE_WINDOW \u003d 1024 * 1024"},{"line_number":89,"context_line":"DEFAULT_READ_OFFLOAD_BURST_SIZE \u003d 1024 * 1024"},{"line_number":90,"context_line":"DEFAULT_READ_OFFLOAD_MAX_INFLIGHT \u003d 8"},{"line_number":91,"context_line":""},{"line_number":92,"context_line":"# Per-device offload semaphores. Created on the hub greenlet only, so no lock."},{"line_number":93,"context_line":"_device_offload_semaphores \u003d {}"}],"source_content_type":"text/x-python","patch_set":8,"id":"1b4f2ce3_47c98785","line":90,"updated":"2026-10-07 14:02:08.000000000","message":"why 8? curious because default tpool size is 20","commit_id":"db38722dc951c075dd31ea9c4cc992c42b92b41f"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"cec3a864b1f35df869570526d7e02fc126c9b5af","unresolved":true,"context_lines":[{"line_number":87,"context_line":"METADATA_CHECKSUM_KEY \u003d b\u0027user.swift.metadata_checksum\u0027"},{"line_number":88,"context_line":"DROP_CACHE_WINDOW \u003d 1024 * 1024"},{"line_number":89,"context_line":"DEFAULT_READ_OFFLOAD_BURST_SIZE \u003d 1024 * 1024"},{"line_number":90,"context_line":"DEFAULT_READ_OFFLOAD_MAX_INFLIGHT \u003d 8"},{"line_number":91,"context_line":""},{"line_number":92,"context_line":"# Per-device offload semaphores. Created on the hub greenlet only, so no lock."},{"line_number":93,"context_line":"_device_offload_semaphores \u003d {}"}],"source_content_type":"text/x-python","patch_set":8,"id":"731e79a4_db7809dc","line":90,"in_reply_to":"1b4f2ce3_47c98785","updated":"2026-10-09 16:00:47.000000000","message":"8 is the cap for one device. With the default tpool size one (slow) device can take max 8 threads, the remaining ones stay free for other devices or the PUT fsync.","commit_id":"db38722dc951c075dd31ea9c4cc992c42b92b41f"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"49c43d2f3a392d9d07f6d20e8330ff24f04feae8","unresolved":true,"context_lines":[{"line_number":780,"context_line":"        # whole disk chunks (\u003e\u003d1) so offload yields the same chunk sizes"},{"line_number":781,"context_line":"        # as the direct path"},{"line_number":782,"context_line":"        self.read_offload_burst_size \u003d ("},{"line_number":783,"context_line":"            max(1, burst // self.disk_chunk_size) * self.disk_chunk_size)"},{"line_number":784,"context_line":"        # cap one device\u0027s share of the shared tpool"},{"line_number":785,"context_line":"        self.read_offload_max_inflight_per_device \u003d max(1, int(conf.get("},{"line_number":786,"context_line":"            \u0027read_offload_max_inflight_per_device\u0027,"}],"source_content_type":"text/x-python","patch_set":8,"id":"2244bd31_b948fa68","line":783,"updated":"2026-10-07 14:02:08.000000000","message":"oops, nothing checks that ``disk_chunk_size !\u003d 0``\n\nIt\u0027s crazy, but ought to be disallowed \"gracefully\".\n\n```\ndiff --git a/test/unit/obj/test_diskfile.py b/test/unit/obj/test_diskfile.py\nindex e695822d3..819aae74b 100644\n--- a/test/unit/obj/test_diskfile.py\n+++ b/test/unit/obj/test_diskfile.py\n@@ -1385,7 +1385,7 @@ class DiskFileManagerMixin(BaseDiskFileTestMixin):\n\n         assert_conf({}, False, 1024 * 1024, 1024 * 1024, 8)\n         # burst rounds down to whole disk chunks; min_size follows the burst\n-        conf \u003d {\u0027read_offload\u0027: \u0027true\u0027, \u0027disk_chunk_size\u0027: \u00271024\u0027,\n+        conf \u003d {\u0027read_offload\u0027: \u0027true\u0027, \u0027disk_chunk_size\u0027: \u00270\u0027,\n                 \u0027read_offload_burst_size\u0027: \u00273000\u0027}\n         assert_conf(conf, True, 2048, 2048, 8)\n         # a burst below one disk chunk becomes one chunk, never 0\n```\n\nresults in\n\n```\ntest/unit/obj/test_diskfile.py:1375 (TestDiskFileManager.test_init_read_offload)\ntest_diskfile.py:1390: in test_init_read_offload\n    assert_conf(conf, True, 2048, 2048, 8)\ntest_diskfile.py:1379: in assert_conf\n    df_mgr \u003d diskfile.DiskFileRouter(conf, self.logger)[policy]\n../../../swift/obj/diskfile.py:735: in __init__\n    policy.get_diskfile_manager(*args, **kwargs)\n../../../swift/common/storage_policy.py:423: in get_diskfile_manager\n    return dfm_cls(*args, **kwargs)\n../../../swift/obj/diskfile.py:783: in __init__\n    max(1, burst // self.disk_chunk_size) * self.disk_chunk_size)\nE   ZeroDivisionError: integer division or modulo by zero\n\n```","commit_id":"db38722dc951c075dd31ea9c4cc992c42b92b41f"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"cec3a864b1f35df869570526d7e02fc126c9b5af","unresolved":true,"context_lines":[{"line_number":780,"context_line":"        # whole disk chunks (\u003e\u003d1) so offload yields the same chunk sizes"},{"line_number":781,"context_line":"        # as the direct path"},{"line_number":782,"context_line":"        self.read_offload_burst_size \u003d ("},{"line_number":783,"context_line":"            max(1, burst // self.disk_chunk_size) * self.disk_chunk_size)"},{"line_number":784,"context_line":"        # cap one device\u0027s share of the shared tpool"},{"line_number":785,"context_line":"        self.read_offload_max_inflight_per_device \u003d max(1, int(conf.get("},{"line_number":786,"context_line":"            \u0027read_offload_max_inflight_per_device\u0027,"}],"source_content_type":"text/x-python","patch_set":8,"id":"b9cd7928_71fcc10e","line":783,"in_reply_to":"2244bd31_b948fa68","updated":"2026-10-09 16:00:47.000000000","message":"Uhh, this is actually a bug on master that popped up here! It\u0027s even worse on master: it quarantines healthy objects if set to 0. Will fix it in a separate commit.","commit_id":"db38722dc951c075dd31ea9c4cc992c42b92b41f"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"49c43d2f3a392d9d07f6d20e8330ff24f04feae8","unresolved":true,"context_lines":[{"line_number":2250,"context_line":"        return CooperativeIterator("},{"line_number":2251,"context_line":"            self._inner_iter(), period\u003dself._cooperative_period)"},{"line_number":2252,"context_line":""},{"line_number":2253,"context_line":"    def _read_burst(self, fd, offset, nbytes):"},{"line_number":2254,"context_line":"        \"\"\""},{"line_number":2255,"context_line":"        Read ``nbytes`` at ``offset``, update the etag with them and drop them"},{"line_number":2256,"context_line":"        from the page cache (``b\u0027\u0027`` at EOF). On a tpool worker: pread and md5"}],"source_content_type":"text/x-python","patch_set":8,"id":"2a5b784a_6ab7b33b","line":2253,"updated":"2026-10-07 14:02:08.000000000","message":"this method is basically wrapping a pread, and it takes the same args as pread, but in a different order. I think it would be helpful to use the same arg order as pread.","commit_id":"db38722dc951c075dd31ea9c4cc992c42b92b41f"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"cec3a864b1f35df869570526d7e02fc126c9b5af","unresolved":true,"context_lines":[{"line_number":2250,"context_line":"        return CooperativeIterator("},{"line_number":2251,"context_line":"            self._inner_iter(), period\u003dself._cooperative_period)"},{"line_number":2252,"context_line":""},{"line_number":2253,"context_line":"    def _read_burst(self, fd, offset, nbytes):"},{"line_number":2254,"context_line":"        \"\"\""},{"line_number":2255,"context_line":"        Read ``nbytes`` at ``offset``, update the etag with them and drop them"},{"line_number":2256,"context_line":"        from the page cache (``b\u0027\u0027`` at EOF). On a tpool worker: pread and md5"}],"source_content_type":"text/x-python","patch_set":8,"id":"6a00d649_aad9b880","line":2253,"in_reply_to":"2a5b784a_6ab7b33b","updated":"2026-10-09 16:00:47.000000000","message":"You\u0027re right. This will change in the next PS because I\u0027m squashing in Waels sq patch (1009142: sq? Eliminate manual offset management in offloaded reads | https://review.opendev.org/c/openstack/swift/+/1009142).","commit_id":"db38722dc951c075dd31ea9c4cc992c42b92b41f"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"49c43d2f3a392d9d07f6d20e8330ff24f04feae8","unresolved":true,"context_lines":[{"line_number":2354,"context_line":"        \"\"\""},{"line_number":2355,"context_line":"        fd \u003d self._fp.fileno()"},{"line_number":2356,"context_line":"        offset \u003d self._fp.tell()"},{"line_number":2357,"context_line":"        remaining \u003d self._range_length"},{"line_number":2358,"context_line":"        if remaining is None:"},{"line_number":2359,"context_line":"            # no range cap: read to the object\u0027s verified end"},{"line_number":2360,"context_line":"            remaining \u003d self._obj_size - offset"},{"line_number":2361,"context_line":"        while remaining \u003e 0:"},{"line_number":2362,"context_line":"            read_size \u003d min(self._read_offload_burst_size, remaining)"},{"line_number":2363,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":8,"id":"a7d551fa_b91c5bf2","line":2360,"range":{"start_line":2357,"start_character":8,"end_line":2360,"end_character":47},"updated":"2026-10-07 14:02:08.000000000","message":"these lines are same as in ``_should_offload`` - there\u0027s an opportunity to consolidate as a property/method?","commit_id":"db38722dc951c075dd31ea9c4cc992c42b92b41f"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"cec3a864b1f35df869570526d7e02fc126c9b5af","unresolved":true,"context_lines":[{"line_number":2354,"context_line":"        \"\"\""},{"line_number":2355,"context_line":"        fd \u003d self._fp.fileno()"},{"line_number":2356,"context_line":"        offset \u003d self._fp.tell()"},{"line_number":2357,"context_line":"        remaining \u003d self._range_length"},{"line_number":2358,"context_line":"        if remaining is None:"},{"line_number":2359,"context_line":"            # no range cap: read to the object\u0027s verified end"},{"line_number":2360,"context_line":"            remaining \u003d self._obj_size - offset"},{"line_number":2361,"context_line":"        while remaining \u003e 0:"},{"line_number":2362,"context_line":"            read_size \u003d min(self._read_offload_burst_size, remaining)"},{"line_number":2363,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":8,"id":"dd033005_8f7a7540","line":2360,"range":{"start_line":2357,"start_character":8,"end_line":2360,"end_character":47},"in_reply_to":"a7d551fa_b91c5bf2","updated":"2026-10-09 16:00:47.000000000","message":"Yes, good idea - will do.","commit_id":"db38722dc951c075dd31ea9c4cc992c42b92b41f"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"49c43d2f3a392d9d07f6d20e8330ff24f04feae8","unresolved":true,"context_lines":[{"line_number":2359,"context_line":"            # no range cap: read to the object\u0027s verified end"},{"line_number":2360,"context_line":"            remaining \u003d self._obj_size - offset"},{"line_number":2361,"context_line":"        while remaining \u003e 0:"},{"line_number":2362,"context_line":"            read_size \u003d min(self._read_offload_burst_size, remaining)"},{"line_number":2363,"context_line":"            try:"},{"line_number":2364,"context_line":"                with self._offload_semaphore:"},{"line_number":2365,"context_line":"                    buf \u003d tpool.execute(self._read_burst, fd, offset,"}],"source_content_type":"text/x-python","patch_set":8,"id":"da7c8af4_dd431e8d","line":2362,"range":{"start_line":2362,"start_character":12,"end_line":2362,"end_character":21},"updated":"2026-10-07 14:02:08.000000000","message":"``_read_burst`` calls this variable ``nbytes`` - I\u0027d prefer we use the same name in both methods for consistency.\n\nUnfortunately ``pread`` uses ``n`` which is not a great arg name, but I\u0027d suggest ``nbytes`` is closer to ``n`` than ``read_size``","commit_id":"db38722dc951c075dd31ea9c4cc992c42b92b41f"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"cec3a864b1f35df869570526d7e02fc126c9b5af","unresolved":false,"context_lines":[{"line_number":2359,"context_line":"            # no range cap: read to the object\u0027s verified end"},{"line_number":2360,"context_line":"            remaining \u003d self._obj_size - offset"},{"line_number":2361,"context_line":"        while remaining \u003e 0:"},{"line_number":2362,"context_line":"            read_size \u003d min(self._read_offload_burst_size, remaining)"},{"line_number":2363,"context_line":"            try:"},{"line_number":2364,"context_line":"                with self._offload_semaphore:"},{"line_number":2365,"context_line":"                    buf \u003d tpool.execute(self._read_burst, fd, offset,"}],"source_content_type":"text/x-python","patch_set":8,"id":"61ae8185_f69d9f47","line":2362,"range":{"start_line":2362,"start_character":12,"end_line":2362,"end_character":21},"in_reply_to":"da7c8af4_dd431e8d","updated":"2026-10-09 16:00:47.000000000","message":"Acknowledged","commit_id":"db38722dc951c075dd31ea9c4cc992c42b92b41f"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"49c43d2f3a392d9d07f6d20e8330ff24f04feae8","unresolved":true,"context_lines":[{"line_number":2368,"context_line":"                if e.errno \u003d\u003d errno.EIO:"},{"line_number":2369,"context_line":"                    self._quarantine(str(e))"},{"line_number":2370,"context_line":"                raise"},{"line_number":2371,"context_line":"            if not buf:"},{"line_number":2372,"context_line":"                # EOF short of _obj_size: the file shrank under us"},{"line_number":2373,"context_line":"                self._read_to_eof \u003d True"},{"line_number":2374,"context_line":"                return"}],"source_content_type":"text/x-python","patch_set":8,"id":"6bcc3692_4bf05f60","line":2371,"updated":"2026-10-07 14:02:08.000000000","message":"if I remove this conditional return, I see tests spin forever, but not fail:\n\n``test_read_offload_range_reads_match_direct``","commit_id":"db38722dc951c075dd31ea9c4cc992c42b92b41f"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"cec3a864b1f35df869570526d7e02fc126c9b5af","unresolved":true,"context_lines":[{"line_number":2368,"context_line":"                if e.errno \u003d\u003d errno.EIO:"},{"line_number":2369,"context_line":"                    self._quarantine(str(e))"},{"line_number":2370,"context_line":"                raise"},{"line_number":2371,"context_line":"            if not buf:"},{"line_number":2372,"context_line":"                # EOF short of _obj_size: the file shrank under us"},{"line_number":2373,"context_line":"                self._read_to_eof \u003d True"},{"line_number":2374,"context_line":"                return"}],"source_content_type":"text/x-python","patch_set":8,"id":"4c12d221_29bfbe8f","line":2371,"in_reply_to":"6bcc3692_4bf05f60","updated":"2026-10-09 16:00:47.000000000","message":"Good catch - will bound the test loop","commit_id":"db38722dc951c075dd31ea9c4cc992c42b92b41f"}],"swift/obj/server.py":[{"author":{"_account_id":38368,"name":"Christian Ohanaja","display_name":"Christian Ohanaja","email":"cohanaja@nvidia.com","username":"cohanaja"},"change_message_id":"57e013dcfe2c1a7435e1d4751b5a5c6b61bd667e","unresolved":true,"context_lines":[{"line_number":265,"context_line":"        if tpool_size:"},{"line_number":266,"context_line":"            tpool.set_num_threads(tpool_size)"},{"line_number":267,"context_line":""},{"line_number":268,"context_line":"        if config_true_value(conf.get(\u0027read_offload\u0027, \u0027false\u0027)):"},{"line_number":269,"context_line":"            # a pool smaller than one device\u0027s cap can\u0027t run its reads in"},{"line_number":270,"context_line":"            # parallel; they serialize and PUT fsyncs queue behind them"},{"line_number":271,"context_line":"            effective_tpool_size \u003d tpool_size or int("}],"source_content_type":"text/x-python","patch_set":7,"id":"8edc80f0_8caf940d","line":268,"range":{"start_line":268,"start_character":0,"end_line":268,"end_character":64},"updated":"2026-10-05 08:04:17.000000000","message":"You could prob remove the \u0027false\u0027 default since this\u0027ll just resolve to None which c_true_value just turns to False","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"7091e46e21ddf2df31f3d409e3eb6c95d685e0ad","unresolved":true,"context_lines":[{"line_number":265,"context_line":"        if tpool_size:"},{"line_number":266,"context_line":"            tpool.set_num_threads(tpool_size)"},{"line_number":267,"context_line":""},{"line_number":268,"context_line":"        if config_true_value(conf.get(\u0027read_offload\u0027, \u0027false\u0027)):"},{"line_number":269,"context_line":"            # a pool smaller than one device\u0027s cap can\u0027t run its reads in"},{"line_number":270,"context_line":"            # parallel; they serialize and PUT fsyncs queue behind them"},{"line_number":271,"context_line":"            effective_tpool_size \u003d tpool_size or int("}],"source_content_type":"text/x-python","patch_set":7,"id":"a4f8a899_0f6ca250","line":268,"range":{"start_line":268,"start_character":0,"end_line":268,"end_character":64},"in_reply_to":"8edc80f0_8caf940d","updated":"2026-10-05 13:13:36.000000000","message":"Yes, I could do this, but I prefer to keep it to make the default more visible.","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"},{"author":{"_account_id":38368,"name":"Christian Ohanaja","display_name":"Christian Ohanaja","email":"cohanaja@nvidia.com","username":"cohanaja"},"change_message_id":"57e013dcfe2c1a7435e1d4751b5a5c6b61bd667e","unresolved":true,"context_lines":[{"line_number":273,"context_line":"            max_inflight \u003d max(1, int(conf.get("},{"line_number":274,"context_line":"                \u0027read_offload_max_inflight_per_device\u0027,"},{"line_number":275,"context_line":"                DEFAULT_READ_OFFLOAD_MAX_INFLIGHT)))"},{"line_number":276,"context_line":"            if effective_tpool_size \u003c max_inflight:"},{"line_number":277,"context_line":"                self.logger.warning("},{"line_number":278,"context_line":"                    \u0027read_offload is enabled but the eventlet thread pool has \u0027"},{"line_number":279,"context_line":"                    \u0027only %(tpool_size)d thread(s), fewer than \u0027"}],"source_content_type":"text/x-python","patch_set":7,"id":"cfda7ea6_c93192d9","line":276,"range":{"start_line":276,"start_character":0,"end_line":276,"end_character":51},"updated":"2026-10-05 08:04:17.000000000","message":"this bothers mea wee bit in that we warn whenever the Swift operator configured max inflight thread count is less than whatever the eventlet tpool size is, \n\nwhich makes me wonder: if we prefer/nudge via warning that the operator just sets the max_inflight to whatever the threadpool size is, what\u0027s making us opt for setting DEFAULT_READ_OFFLOAD_MAX_INFLIGHT to 8 vs the default 20 for EVENTLET_THREADPOOL_SIZE?\n\nand then along that line, (though maybe I\u0027m misunderstanding the point of the max) i think the intended \"recommended\" behavior given the warning is for the operator to make tpool size to be \u003d\u003d max_inflight, so ynot surface specifically that?","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"7091e46e21ddf2df31f3d409e3eb6c95d685e0ad","unresolved":true,"context_lines":[{"line_number":273,"context_line":"            max_inflight \u003d max(1, int(conf.get("},{"line_number":274,"context_line":"                \u0027read_offload_max_inflight_per_device\u0027,"},{"line_number":275,"context_line":"                DEFAULT_READ_OFFLOAD_MAX_INFLIGHT)))"},{"line_number":276,"context_line":"            if effective_tpool_size \u003c max_inflight:"},{"line_number":277,"context_line":"                self.logger.warning("},{"line_number":278,"context_line":"                    \u0027read_offload is enabled but the eventlet thread pool has \u0027"},{"line_number":279,"context_line":"                    \u0027only %(tpool_size)d thread(s), fewer than \u0027"}],"source_content_type":"text/x-python","patch_set":7,"id":"0f6d7515_4d28bac4","line":276,"range":{"start_line":276,"start_character":0,"end_line":276,"end_character":51},"in_reply_to":"cfda7ea6_c93192d9","updated":"2026-10-05 13:13:36.000000000","message":"The warning is the other way around: when the thread pool is smaller than read_offload_max_inflight_per_device one device can use all threads. The cap has no effect then, and the offloaded reads wait behind each other and behind fsync.","commit_id":"091e131b2ddd838d74a875020068d1846b95889c"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"49c43d2f3a392d9d07f6d20e8330ff24f04feae8","unresolved":true,"context_lines":[{"line_number":269,"context_line":"            # a pool smaller than one device\u0027s cap can\u0027t run its reads in"},{"line_number":270,"context_line":"            # parallel; they serialize and PUT fsyncs queue behind them"},{"line_number":271,"context_line":"            effective_tpool_size \u003d tpool_size or int("},{"line_number":272,"context_line":"                os.environ.get(\u0027EVENTLET_THREADPOOL_SIZE\u0027, 20))"},{"line_number":273,"context_line":"            max_inflight \u003d max(1, int(conf.get("},{"line_number":274,"context_line":"                \u0027read_offload_max_inflight_per_device\u0027,"},{"line_number":275,"context_line":"                DEFAULT_READ_OFFLOAD_MAX_INFLIGHT)))"}],"source_content_type":"text/x-python","patch_set":8,"id":"866c2656_c5b82367","line":272,"range":{"start_line":272,"start_character":16,"end_line":272,"end_character":63},"updated":"2026-10-07 14:02:08.000000000","message":"Claude says: \"Hardcoded 20 for eventlet\u0027s default pool size — swift/obj/server.py:271-272 assumes eventlet\u0027s internal default. Correct today, but it\u0027s an undocumented coupling with nothing asserting it stays true. A short comment naming it as eventlet\u0027s default would help.\"\n\nWe could even create our own ``DEFAULT_EVENTLET_THREADPOOL_SIZE`` constant in concurrency.py and document it once there","commit_id":"db38722dc951c075dd31ea9c4cc992c42b92b41f"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"cec3a864b1f35df869570526d7e02fc126c9b5af","unresolved":true,"context_lines":[{"line_number":269,"context_line":"            # a pool smaller than one device\u0027s cap can\u0027t run its reads in"},{"line_number":270,"context_line":"            # parallel; they serialize and PUT fsyncs queue behind them"},{"line_number":271,"context_line":"            effective_tpool_size \u003d tpool_size or int("},{"line_number":272,"context_line":"                os.environ.get(\u0027EVENTLET_THREADPOOL_SIZE\u0027, 20))"},{"line_number":273,"context_line":"            max_inflight \u003d max(1, int(conf.get("},{"line_number":274,"context_line":"                \u0027read_offload_max_inflight_per_device\u0027,"},{"line_number":275,"context_line":"                DEFAULT_READ_OFFLOAD_MAX_INFLIGHT)))"}],"source_content_type":"text/x-python","patch_set":8,"id":"2246e625_493fc883","line":272,"range":{"start_line":272,"start_character":16,"end_line":272,"end_character":63},"in_reply_to":"866c2656_c5b82367","updated":"2026-10-09 16:00:47.000000000","message":"If we create DEFAULT_EVENTLET_THREADPOOL_SIZE this could still deviate in the future. But I think there is a better way: just use eventlets size after it got imported, that way it always uses eventlets default if not set.","commit_id":"db38722dc951c075dd31ea9c4cc992c42b92b41f"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"49c43d2f3a392d9d07f6d20e8330ff24f04feae8","unresolved":true,"context_lines":[{"line_number":281,"context_line":"                    \u0027Offloaded reads will serialize and delay the fsync of \u0027"},{"line_number":282,"context_line":"                    \u0027concurrent PUTs. Raise eventlet_tpool_num_threads.\u0027,"},{"line_number":283,"context_line":"                    {\u0027tpool_size\u0027: effective_tpool_size,"},{"line_number":284,"context_line":"                     \u0027max_inflight\u0027: max_inflight})"},{"line_number":285,"context_line":""},{"line_number":286,"context_line":"    def get_diskfile(self, device, partition, account, container, obj,"},{"line_number":287,"context_line":"                     policy, **kwargs):"}],"source_content_type":"text/x-python","patch_set":8,"id":"335d7053_83c48e8e","line":284,"updated":"2026-10-07 14:02:08.000000000","message":"It seems odd that this config check and warning is happening here in the server for config that is specific to diskfile. As a result, the defaults and forcing ``max_inflight`` to be at least 1 are repeated here and in diskfile i.e. we do not have single point of truth for config parsing.\n\nI think it may be written this way because here is where we know what tpool size, and/or to avoid multiple warnings from multiple DiskFileManagers.\n\nBut I think we could just use the attributes of the DiskFileManagers that have been instantiated, rather than parsing config again here:\n\n```\ndiff --git a/swift/obj/server.py b/swift/obj/server.py\nindex 3fcb13f38..9aa69673e 100644\n--- a/swift/obj/server.py\n+++ b/swift/obj/server.py\n@@ -61,8 +61,7 @@ from swift.common.swob import HTTPAccepted, HTTPBadRequest, HTTPCreated, \\\n     HTTPServerError, bytes_to_wsgi, wsgi_to_bytes, wsgi_to_str, \\\n     normalize_etag, HTTPServiceUnavailable\n from swift.common.wsgi import run_wsgi\n-from swift.obj.diskfile import DEFAULT_READ_OFFLOAD_MAX_INFLIGHT, \\\n-    RESERVED_DATAFILE_META, DiskFileRouter\n+from swift.obj.diskfile import RESERVED_DATAFILE_META, DiskFileRouter\n from swift.obj.expirer import build_task_obj, embed_expirer_bytes_in_ctype, \\\n     X_DELETE_TYPE\n \n@@ -265,15 +264,14 @@ class ObjectController(BaseStorageServer):\n         if tpool_size:\n             tpool.set_num_threads(tpool_size)\n \n-        if config_true_value(conf.get(\u0027read_offload\u0027, \u0027false\u0027)):\n-            # a pool smaller than one device\u0027s cap can\u0027t run its reads in\n-            # parallel; they serialize and PUT fsyncs queue behind them\n-            effective_tpool_size \u003d tpool_size or int(\n-                os.environ.get(\u0027EVENTLET_THREADPOOL_SIZE\u0027, 20))\n-            max_inflight \u003d max(1, int(conf.get(\n-                \u0027read_offload_max_inflight_per_device\u0027,\n-                DEFAULT_READ_OFFLOAD_MAX_INFLIGHT)))\n-            if effective_tpool_size \u003c max_inflight:\n+        effective_tpool_size \u003d tpool_size or int(\n+            os.environ.get(\u0027EVENTLET_THREADPOOL_SIZE\u0027, 20))\n+        for dfm in self._diskfile_router.policy_to_manager.values():\n+            if (dfm.read_offload and\n+                    effective_tpool_size \u003c\n+                    dfm.read_offload_max_inflight_per_device):\n+                # a pool smaller than one device\u0027s cap can\u0027t run its reads in\n+                # parallel; they serialize and PUT fsyncs queue behind them\n                 self.logger.warning(\n                     \u0027read_offload is enabled but the eventlet thread pool has \u0027\n                     \u0027only %(tpool_size)d thread(s), fewer than \u0027\n@@ -281,7 +279,8 @@ class ObjectController(BaseStorageServer):\n                     \u0027Offloaded reads will serialize and delay the fsync of \u0027\n                     \u0027concurrent PUTs. Raise eventlet_tpool_num_threads.\u0027,\n                     {\u0027tpool_size\u0027: effective_tpool_size,\n-                     \u0027max_inflight\u0027: max_inflight})\n+                     \u0027max_inflight\u0027: dfm.read_offload_max_inflight_per_device})\n+                break\n \n     def get_diskfile(self, device, partition, account, container, obj,\n                      policy, **kwargs):\ndiff --git a/test/unit/obj/test_server.py b/test/unit/obj/test_server.py\nindex 65192e294..8956f6bb4 100644\n--- a/test/unit/obj/test_server.py\n+++ b/test/unit/obj/test_server.py\n@@ -143,7 +143,9 @@ class TestTpoolSize(unittest.TestCase):\n         return [line for line in logger.get_lines_for_level(\u0027warning\u0027)\n                 if \u0027read_offload\u0027 in line]\n \n+    @patch_policies(test_policies)\n     def test_read_offload_undersized_tpool_warns(self):\n+        # patch in multiple policies and verify that we only see one warning;\n         # servers_per_port defaults the pool to one thread\n         warnings \u003d self._read_offload_warnings(\n             {\u0027servers_per_port\u0027: \u00273\u0027, \u0027read_offload\u0027: \u0027true\u0027})\n\n```","commit_id":"db38722dc951c075dd31ea9c4cc992c42b92b41f"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"cec3a864b1f35df869570526d7e02fc126c9b5af","unresolved":false,"context_lines":[{"line_number":281,"context_line":"                    \u0027Offloaded reads will serialize and delay the fsync of \u0027"},{"line_number":282,"context_line":"                    \u0027concurrent PUTs. Raise eventlet_tpool_num_threads.\u0027,"},{"line_number":283,"context_line":"                    {\u0027tpool_size\u0027: effective_tpool_size,"},{"line_number":284,"context_line":"                     \u0027max_inflight\u0027: max_inflight})"},{"line_number":285,"context_line":""},{"line_number":286,"context_line":"    def get_diskfile(self, device, partition, account, container, obj,"},{"line_number":287,"context_line":"                     policy, **kwargs):"}],"source_content_type":"text/x-python","patch_set":8,"id":"95c583d2_a35b47e1","line":284,"in_reply_to":"335d7053_83c48e8e","updated":"2026-10-09 16:00:47.000000000","message":"Acknowledged","commit_id":"db38722dc951c075dd31ea9c4cc992c42b92b41f"}],"test/unit/obj/test_diskfile.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"49c43d2f3a392d9d07f6d20e8330ff24f04feae8","unresolved":true,"context_lines":[{"line_number":1302,"context_line":"            mkdirs(os.path.join(self.testdir, self.existing_device2,"},{"line_number":1303,"context_line":"                                diskfile.get_tmp_dir(policy)))"},{"line_number":1304,"context_line":"        self._orig_tpool_exc \u003d tpool.execute"},{"line_number":1305,"context_line":"        tpool.execute \u003d lambda f, *args, **kwargs: f(*args, **kwargs)"},{"line_number":1306,"context_line":"        self.conf \u003d dict(devices\u003dself.testdir, mount_check\u003d\u0027false\u0027,"},{"line_number":1307,"context_line":"                         keep_cache_size\u003d2 * 1024, mb_per_sync\u003d1)"},{"line_number":1308,"context_line":"        self.logger \u003d debug_logger(\u0027test-\u0027 + self.__class__.__name__)"}],"source_content_type":"text/x-python","patch_set":8,"id":"bc366abd_a0e48d30","line":1305,"updated":"2026-10-07 14:02:08.000000000","message":"off-topic: this isn\u0027t great :( - this mocking should be undone in a cleanup that guaranteed to run whatever the test outcome is.\n\nAlso, we\u0027re now adding tests to this mixin that do not want this mocking - I wonder how many tests *do* want it?? should we just explicitly mock tpool when necessary","commit_id":"db38722dc951c075dd31ea9c4cc992c42b92b41f"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"49c43d2f3a392d9d07f6d20e8330ff24f04feae8","unresolved":true,"context_lines":[{"line_number":4649,"context_line":"        }"},{"line_number":4650,"context_line":""},{"line_number":4651,"context_line":"    @contextmanager"},{"line_number":4652,"context_line":"    def _real_tpool(self):"},{"line_number":4653,"context_line":"        # run offloads on the real eventlet thread pool; setUp stubs"},{"line_number":4654,"context_line":"        # tpool.execute to run inline, so restore that stub on exit"},{"line_number":4655,"context_line":"        tpool.execute \u003d self._orig_tpool_exc"}],"source_content_type":"text/x-python","patch_set":8,"id":"0674abfc_b1992a7b","line":4652,"updated":"2026-10-07 14:02:08.000000000","message":"I don\u0027t like this undoing of an indiscriminate monkey patch\n\nplease consider: 1009247: test_diskfile: only mock tpool.execute when required | https://review.opendev.org/c/openstack/swift/+/1009247\n\nwe could introduce ``mock_tpool_execute`` first on master if necessary","commit_id":"db38722dc951c075dd31ea9c4cc992c42b92b41f"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"49c43d2f3a392d9d07f6d20e8330ff24f04feae8","unresolved":true,"context_lines":[{"line_number":4651,"context_line":"    @contextmanager"},{"line_number":4652,"context_line":"    def _real_tpool(self):"},{"line_number":4653,"context_line":"        # run offloads on the real eventlet thread pool; setUp stubs"},{"line_number":4654,"context_line":"        # tpool.execute to run inline, so restore that stub on exit"},{"line_number":4655,"context_line":"        tpool.execute \u003d self._orig_tpool_exc"},{"line_number":4656,"context_line":"        try:"},{"line_number":4657,"context_line":"            yield"}],"source_content_type":"text/x-python","patch_set":8,"id":"282c2dfe_65e4fe68","line":4654,"range":{"start_line":4654,"start_character":39,"end_line":4654,"end_character":67},"updated":"2026-10-07 14:02:08.000000000","message":"but this is a mixin, so it\u0027s best not to assume what setUp(s) have done\n\nMight be better to put tests that do not want tpool mocked in a different mixin?","commit_id":"db38722dc951c075dd31ea9c4cc992c42b92b41f"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"49c43d2f3a392d9d07f6d20e8330ff24f04feae8","unresolved":true,"context_lines":[{"line_number":4656,"context_line":"        try:"},{"line_number":4657,"context_line":"            yield"},{"line_number":4658,"context_line":"        finally:"},{"line_number":4659,"context_line":"            tpool.execute \u003d lambda f, *a, **k: f(*a, **k)"},{"line_number":4660,"context_line":""},{"line_number":4661,"context_line":"    def _raise_dfq(self, msg):"},{"line_number":4662,"context_line":"        raise DiskFileQuarantined(msg)"}],"source_content_type":"text/x-python","patch_set":8,"id":"b8aa4538_03a2d29a","line":4659,"updated":"2026-10-07 14:02:08.000000000","message":"this *assumes* that the tpool *should* be reverted to a mock; that seems dangerous - always restore whatever was there before line 4655","commit_id":"db38722dc951c075dd31ea9c4cc992c42b92b41f"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"49c43d2f3a392d9d07f6d20e8330ff24f04feae8","unresolved":true,"context_lines":[{"line_number":4701,"context_line":"    def test_read_offload_range_reads_match_direct(self):"},{"line_number":4702,"context_line":"        # ensure ranged reads (start / middle / suffix / boundary) return the"},{"line_number":4703,"context_line":"        # same bytes under offload as on the direct path"},{"line_number":4704,"context_line":"        dcs, burst \u003d 1024, 4096"},{"line_number":4705,"context_line":"        size \u003d 5000"},{"line_number":4706,"context_line":"        data \u003d os.urandom(size)"},{"line_number":4707,"context_line":"        self._set_read_offload(False, burst, dcs)"}],"source_content_type":"text/x-python","patch_set":8,"id":"65f10f3a_412de33f","line":4704,"range":{"start_line":4704,"start_character":8,"end_line":4704,"end_character":11},"updated":"2026-10-07 14:02:08.000000000","message":"nit: ``dcs`` could be ``disk_chunk_size``\n\nwe don\u0027t need aggresively abbreviated code; bytes in source code are cheaper that the time it takes me to grok ;-)","commit_id":"db38722dc951c075dd31ea9c4cc992c42b92b41f"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"cec3a864b1f35df869570526d7e02fc126c9b5af","unresolved":false,"context_lines":[{"line_number":4701,"context_line":"    def test_read_offload_range_reads_match_direct(self):"},{"line_number":4702,"context_line":"        # ensure ranged reads (start / middle / suffix / boundary) return the"},{"line_number":4703,"context_line":"        # same bytes under offload as on the direct path"},{"line_number":4704,"context_line":"        dcs, burst \u003d 1024, 4096"},{"line_number":4705,"context_line":"        size \u003d 5000"},{"line_number":4706,"context_line":"        data \u003d os.urandom(size)"},{"line_number":4707,"context_line":"        self._set_read_offload(False, burst, dcs)"}],"source_content_type":"text/x-python","patch_set":8,"id":"704b1713_27a13deb","line":4704,"range":{"start_line":4704,"start_character":8,"end_line":4704,"end_character":11},"in_reply_to":"65f10f3a_412de33f","updated":"2026-10-09 16:00:47.000000000","message":"Acknowledged","commit_id":"db38722dc951c075dd31ea9c4cc992c42b92b41f"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"49c43d2f3a392d9d07f6d20e8330ff24f04feae8","unresolved":true,"context_lines":[{"line_number":4702,"context_line":"        # ensure ranged reads (start / middle / suffix / boundary) return the"},{"line_number":4703,"context_line":"        # same bytes under offload as on the direct path"},{"line_number":4704,"context_line":"        dcs, burst \u003d 1024, 4096"},{"line_number":4705,"context_line":"        size \u003d 5000"},{"line_number":4706,"context_line":"        data \u003d os.urandom(size)"},{"line_number":4707,"context_line":"        self._set_read_offload(False, burst, dcs)"},{"line_number":4708,"context_line":"        df, on_disk \u003d self._create_test_file(data, obj\u003d\u0027rng\u0027)"}],"source_content_type":"text/x-python","patch_set":8,"id":"1a632a3b_2a69e4c2","line":4705,"updated":"2026-10-07 14:02:08.000000000","message":"NOTE for EC tests, the diskfile made by _create_test_file is shorter (662 bytes)","commit_id":"db38722dc951c075dd31ea9c4cc992c42b92b41f"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"49c43d2f3a392d9d07f6d20e8330ff24f04feae8","unresolved":true,"context_lines":[{"line_number":4717,"context_line":"            self.assertEqual(qmsgs, [], (read_offload, start, stop))"},{"line_number":4718,"context_line":"            return body"},{"line_number":4719,"context_line":""},{"line_number":4720,"context_line":"        ranges \u003d [(0, None), (0, 100), (1000, 2000), (dcs, dcs + 10),"},{"line_number":4721,"context_line":"                  (burst, None), (burst - 5, burst + 5), (size - 10, None)]"},{"line_number":4722,"context_line":"        for start, stop in ranges:"},{"line_number":4723,"context_line":"            direct \u003d read_range(False, start, stop)"}],"source_content_type":"text/x-python","patch_set":8,"id":"b67e0aa5_81f61fc5","line":4720,"range":{"start_line":4720,"start_character":39,"end_line":4720,"end_character":51},"updated":"2026-10-07 14:02:08.000000000","message":"When this test is run via the mixin as ``TestECDiskFile.test_read_offload_range_reads_match_direct``, this scenario, and maybe others, is actually reading beyond the length of the file. In fact this scenario starts beyond the file length which is 662 (data length is 5000, but the ``_create_test_file`` helper morphs data to an ec frag). That illustrates that even when the read starts beyond the end of the file, a tpool thread is still executed with a pread that is always going to return an empty buf i.e. there is no pre-check on the start being \u003c obj_size. That seems an unnecessary use of tpool, unless we want to allow for the possibility that the file got longer??\n\nAlso, it took me a while to understand why this test was even using tpool when the ranges are much shorter than the burst size, but I eventually realised that the test setup is defaulting min burst size to zero (which is different from the way disk file manager would default the min size). IMHO it would be better if the test explicitly set the min size to zero, or even better, used defaults similar to the real world and ranges that are greater than burst size.","commit_id":"db38722dc951c075dd31ea9c4cc992c42b92b41f"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"49c43d2f3a392d9d07f6d20e8330ff24f04feae8","unresolved":true,"context_lines":[{"line_number":4719,"context_line":""},{"line_number":4720,"context_line":"        ranges \u003d [(0, None), (0, 100), (1000, 2000), (dcs, dcs + 10),"},{"line_number":4721,"context_line":"                  (burst, None), (burst - 5, burst + 5), (size - 10, None)]"},{"line_number":4722,"context_line":"        for start, stop in ranges:"},{"line_number":4723,"context_line":"            direct \u003d read_range(False, start, stop)"},{"line_number":4724,"context_line":"            offl \u003d read_range(True, start, stop)"},{"line_number":4725,"context_line":"            msg \u003d \u0027range\u003d(%s,%s)\u0027 % (start, stop)"}],"source_content_type":"text/x-python","patch_set":8,"id":"dcf64f36_0c763b3e","line":4722,"updated":"2026-10-07 14:02:08.000000000","message":"nit: I have a pet dislike for tests that iterate over test scenarios because it makes it harder to drill down and debug one scenario.\n\nFor example, I wanted to check test coverage for _offload_bursts returning if the pread returns an empty buffer, so I added a regression and this test hangs. I want to know which scenario causes it to hang, so I added a breakpoint in _offload_bursts, but the stack trace just tells me I am in this loop. I can infer which scenario by looking at the loop variables, but it\u0027s easier if the stack trace tells me what scenario is being tested.\n\nSo for the sake of a few more lines, it\u0027s easier to debug this IMHO:\n\n```\ndiff --git a/test/unit/obj/test_diskfile.py b/test/unit/obj/test_diskfile.py\nindex e695822d3..3ab9fdd28 100644\n--- a/test/unit/obj/test_diskfile.py\n+++ b/test/unit/obj/test_diskfile.py\n@@ -4717,9 +4717,7 @@ class DiskFileMixin(BaseDiskFileTestMixin):\n             self.assertEqual(qmsgs, [], (read_offload, start, stop))\n             return body\n \n-        ranges \u003d [(0, None), (0, 100), (1000, 2000), (dcs, dcs + 10),\n-                  (burst, None), (burst - 5, burst + 5), (size - 10, None)]\n-        for start, stop in ranges:\n+        def do_test(start, stop):\n             direct \u003d read_range(False, start, stop)\n             offl \u003d read_range(True, start, stop)\n             msg \u003d \u0027range\u003d(%s,%s)\u0027 % (start, stop)\n@@ -4728,6 +4726,14 @@ class DiskFileMixin(BaseDiskFileTestMixin):\n                 else on_disk[start:]\n             self.assertEqual(offl, expected, msg)\n \n+        do_test(0, None)\n+        do_test(0, 100)\n+        do_test(1000, 2000)\n+        do_test(dcs, dcs + 10)\n+        do_test(burst, None)\n+        do_test(burst - 5, burst + 5)\n+        do_test(size - 10, None)\n+\n     def test_read_offload_caps_pread_to_remaining(self):\n         # ensure reads don\u0027t issue another full-burst pread after the requested\n         # span is satisfied, and don\u0027t over-read the final partial burst.\n\n```\n\nand it is also easier to comment out scenarios when there is one scenario per line.","commit_id":"db38722dc951c075dd31ea9c4cc992c42b92b41f"}]}