Re: [PATCH v8 1/2] block: avoid long sync_blockdev() under disk->open_mutex during parallel close

[Bart Van Assche <bvanassche@acm.org>]

[-- Attachment #1: Type: text/plain, Size: 1094 bytes --]

On 10/18/26 8:26 AM, Tetsuo Handa wrote:
> Commit b849dd84b6cc ("bdev: Reduce time holding bd_mutex in sync in
> blkdev_close()") introduced a sync_blockdev() call immediately before
> holding disk->open_mutex. But this check was racy and best-effort basis.
> 
> If more than one thread concurrently called bdev_release() and saw
> atomic_read(&bdev->bd_openers) > 1, the thread which actually hit
> the atomic_dec_and_test(&bdev->bd_openers) condition will call
> sync_blockdev() from blkdev_flush_mapping() (which could take several
> minutes) with disk->open_mutex held.
> 
> Kill this race window and make sync_blockdev() always be called before
> holding disk->open_mutex, by using a counter that tracks how many threads
> are calling sync_blockdev() from bdev_release() without disk->open_mutex
> held.

An experimental LLM is not happy about this series of two patches.
Please take a look at the report produced by that LLM and let me know
whether or not you agree with the findings of that LLM. One way to view
the attached file on Linux is by using the "glow" command.

Thanks,

Bart.

[-- Attachment #2: review.md --]
[-- Type: text/markdown, Size: 14554 bytes --]

Here is a review of both patches in Tetsuo Handa's v8 series, followed by a verification of the underlying JFS and NILFS2 root causes discussed on the thread.

---

## Overall Verdict: **NACK on both patches**

1. **[PATCH v8 1/2]** breaks its own `bd_closers` invariant on partitions, allowing `sync_blockdev()` to run under `disk->open_mutex` during a concurrent partition close.
2. **[PATCH v8 2/2]** turns that partition race into **silent data loss** (truncating dirty partition pagecache pages without writeback), **still leaves windows for in-flight I/O to enter `loop` before `lo_release()`**, introduces an **unkillable (`TASK_UNINTERRUPTIBLE`) self-deadlock** with FUSE-backed loop devices, and stalls `close()` with full RCU grace periods on unbound / non-autoclearing / sibling-partition closes.
3. Most importantly, **Patch 2/2's premise is flawed**: the `syzbot-ci` NILFS2 report cited as `[3]` is a **false positive** in the `v3` debug patch, and **your original suspicion in `v6` about JFS commit `b15e4310633f` ("jfs: Set the lbmDONE flag at the end of lbmIODone") is the exact root cause** of the syzbot crashes (exact trace below).

---

## Review of [PATCH v8 1/2]: `block: avoid long sync_blockdev() under disk->open_mutex during parallel close`

### 1. Partition race breaks the `bd_openers == 1 + bd_closers` invariant ([`bdev_release()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/block/bdev.c#L1153-L1195))

When closing a partition (`part`), the patch executes:
```c
	mutex_lock(&disk->open_mutex);
	bd_early_sync(disk, bdev);
	if (bdev_is_partition(bdev))
		bd_early_sync(disk, bdev_whole(bdev));
```
Suppose **Thread 1** is the sole opener of `part` (`part->bd_openers == 1`, `whole->bd_openers == 1`):
1. `bd_early_sync(disk, part)` increments `part->bd_closers` (`0 -> 1`), drops `disk->open_mutex`, runs `sync_blockdev(part)`, re-locks `disk->open_mutex`, and **decrements `part->bd_closers` back to `0`** — while `part->bd_openers` is still `1` (not decremented until [`blkdev_put_part()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/block/bdev.c#L852-L861)).
2. Immediately afterward, `bd_early_sync(disk, whole)` drops `disk->open_mutex` **a second time** with `part->bd_openers == 1` and `part->bd_closers == 0`.
3. While `disk->open_mutex` is dropped in `sync_blockdev(whole)`, **Thread 2** opens `part` (`part->bd_openers` becomes `2`, `whole->bd_openers` becomes `2`), dirties pages in `part->bd_mapping`, and calls `bdev_release(part)`.
4. Thread 2 acquires `disk->open_mutex` and checks `bd_early_sync(disk, part)`:
   - `atomic_read(&part->bd_openers)` is `2`, while `1 + part->bd_closers` is `1` (`2 != 1`).
   - Thread 2 **skips `sync_blockdev(part)`** (and `bd_early_sync(disk, whole)` only flushes `whole->bd_mapping`, not `part->bd_mapping`).
5. Both threads then call [`blkdev_put_part()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/block/bdev.c#L852-L861) under `disk->open_mutex`; the second thread drops `part->bd_openers` to `0` and calls [`blkdev_flush_mapping(part)`](file:///usr/local/google/home/bvanassche/software/linux-kernel/block/bdev.c#L753-L759) -> `sync_blockdev(part)` **with `disk->open_mutex` held**.

### 2. Unnecessary `disk->open_mutex` round-trips
`bdev_release()` now acquires/releases `disk->open_mutex` 2 times on every final whole-disk close and **3 times** on every final partition close, even when the mapping is completely clean.

---

## Review of [PATCH v8 2/2]: `block: wait for inflight requests in bdev_release()`

### 1. Critical — Skipping `sync_blockdev()` in [`blkdev_flush_mapping()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/block/bdev.c#L753-L759) causes silent data loss

```c
static void blkdev_flush_mapping(struct block_device *bdev)
{
	WARN_ON_ONCE(bdev->bd_holders);
	/* See bd_early_sync() for this behavior. */
	if (!bdev->bd_disk->fops->sync_queue_before_release)
		sync_blockdev(bdev);
	kill_bdev(bdev);
	bdev_write_inode(bdev);
}
```
- **Partition data loss**: In the 2-thread partition sequence above (Section 1.1) on `/dev/loopXpY`, Thread 2 dirties `part->bd_mapping` and skips `bd_early_sync(disk, part)`. When `blkdev_put_part()` drops `part->bd_openers` to `0`, `blkdev_flush_mapping(part)` **skips `sync_blockdev(part)`** and immediately runs [`kill_bdev(part)`](file:///usr/local/google/home/bvanassche/software/linux-kernel/block/bdev.c#L82-L91) (`truncate_inode_pages()`), **permanently discarding dirty pages without ever writing them back**.
- **Background writeback redirty race**: Even on a whole disk, if `WB_SYNC_NONE` writeback contends on a buffer lock in [`__block_write_full_folio()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/fs/buffer.c#L1721) after `sync_blockdev()` in `bd_early_sync()` tagged folios `PAGECACHE_TAG_TOWRITE` and calls `folio_redirty_for_writepage()`, skipping `sync_blockdev()` in `blkdev_flush_mapping()` drops dirty data in `kill_bdev()`.

### 2. High — Unfreezing `bd_queue` inside `bd_early_sync()` does not prevent in-flight I/O in [`__loop_clr_fd()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/block/loop.c#L1139-L1210)

Comment `(4)` in `bd_early_sync()` claims:
> *"we freeze queue here and forbid further submission of BIOs after we unfreeze queue and re-acquires open_mutex."*

Nothing actually forbids further BIO submissions after `blk_mq_unfreeze_queue()`:
1. **Waiters blocked in `__bio_queue_enter()`**: When an autoclearing loop device (`LO_FLAGS_AUTOCLEAR`) closes, `lo->lo_state` is still `Lo_bound` throughout `bd_early_sync()`—it only transitions to `Lo_rundown` later inside [`lo_release()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/block/loop.c#L1754-L1776). Any leaked or `blk-throtl`-delayed BIO that hits `__bio_queue_enter()` while frozen sleeps on `q->mq_freeze_wq` **without holding `q->q_usage_counter`**. `blk_mq_freeze_queue()` returns as soon as `q_usage_counter == 0`, and `blk_mq_unfreeze_queue()` immediately wakes those sleepers ([`__blk_mq_unfreeze_queue()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/block/blk-mq.c#L211-L228)), allowing them to enter [`loop_queue_rq()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/block/loop.c#L1875-L1916) while `lo->lo_state == Lo_bound` and run [`lo_rw_aio()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/block/loop.c#L341-L408) concurrently with `__loop_clr_fd()`.
2. **Concurrent [`sync_bdevs(false)`](file:///usr/local/google/home/bvanassche/software/linux-kernel/block/bdev.c#L1339-L1389)**: Between `blk_mq_unfreeze_queue()` and `mutex_lock(&disk->open_mutex)`, `bdev->bd_openers` is still `>= 1`. If dirty folios remain in `bdev->bd_mapping` (for instance after `-EIO` in `sync_blockdev()`), a concurrent `sync()` acquires `disk->open_mutex`, sees `bd_openers != 0`, and calls `filemap_fdatawrite()`, submitting fresh async write BIOs right before `lo_release()`.
3. **`blk_mq_freeze_queue()` does not wait for `loop_handle_cmd()` / `loop_process_work()` teardown**: In [`lo_rw_aio_do_completion()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/block/loop.c#L319-L331), `blk_mq_complete_request(rq)` runs *inside* `do_req_filebacked()`, dropping `q->q_usage_counter` to `0` **before** [`loop_handle_cmd()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/block/loop.c#L1949-L1968) and [`loop_process_work()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/block/loop.c#L1970-L2002) finish on `lo->workqueue` (and without quiescing ongoing `loop_queue_rq()` dispatches).

### 3. High — Unkillable (`TASK_UNINTERRUPTIBLE`) self-deadlock on FUSE-backed loop devices

A single process `P` (including unprivileged userspace in a user+mount namespace or `syzbot`) can trigger a permanent unkillable hang:
1. `P` opens `/dev/fuse` (`fd = 4`), mounts a FUSE filesystem, and binds a file on it to `/dev/loop0` (`fd = 3`, even **without** `LO_FLAGS_AUTOCLEAR`).
2. `P` calls non-blocking `readahead(3, 0, 4096)` without servicing `fd = 4`, leaving `lo_rw_aio()` blocked inside `fuse_simple_request()` with `q->q_usage_counter > 0`.
3. `P` calls `close(3)` -> `bdev_release()` -> [`blk_mq_freeze_queue_wait()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/block/blk-mq.c#L189-L192), which sleeps in **`TASK_UNINTERRUPTIBLE`** (`wait_event()`).
4. Even **`SIGKILL` cannot wake `P`** to enter `do_exit()` and close `fd = 4`, so `fuse_dev_release()` never aborts the FUSE connection, `lo_rw_aio()` never completes, and `khungtaskd` fires `task hung in blk_mq_freeze_queue_wait`.

### 4. Medium — Spurious RCU grace-period freezes on unbound, persistent, and partitioned loop devices

`blk_mq_freeze_queue()` + `blk_mq_unfreeze_queue()` incurs a full RCU grace period (`percpu_ref_kill()`):
- It runs on **every close of an unbound `/dev/loopN`** (e.g., `udev`, `blkid`, `losetup -f` scanning loop devices) and on **bound devices that are not clearing** (`!(lo->lo_flags & LO_FLAGS_AUTOCLEAR)`).
- Because all partitions share `disk->queue` ([`bdev_alloc()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/block/bdev.c#L520)), closing `/dev/loop0p1` while `/dev/loop0p2` is active freezes the entire disk queue and stalls I/O on `/dev/loop0p2`; closing the last open partition freezes and unfreezes the same queue **twice back-to-back** (`bd_early_sync(part)` then `bd_early_sync(whole)`).

---

## Root-Cause Verification (For the Mailing List Thread)

### A. Why the `syzbot-ci` NILFS2 readahead report (`[3]`) is a false positive

Patch 2/2 claims that `nilfs_search_super_root() -> __breadahead()` leaks read BIOs past `lo_release()`. That is **not happening**; `v3` of the debug patch checked the wrong condition inside `loop_handle_cmd()`:
```c
if (unlikely(data_race(READ_ONCE(lo->lo_state)) != Lo_bound &&
             disk_openers(lo->lo_disk) == 0))
```
Look at [`blkdev_put_whole()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/block/bdev.c#L761-L767):
```c
static void blkdev_put_whole(struct block_device *bdev)
{
	if (atomic_dec_and_test(&bdev->bd_openers))
		blkdev_flush_mapping(bdev);
	if (bdev->bd_disk->fops->release)
		bdev->bd_disk->fops->release(bdev->bd_disk);
}
```
1. After `nilfs_get_tree()` queued `__breadahead()` on `sb->s_bdev` (`bdev->bd_mapping`) and failed mount while another `fd` still held `/dev/loop0`, `ioctl(fd, LOOP_CLR_FD)` set `lo->lo_state = Lo_rundown` ([`loop_clr_fd()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/block/loop.c#L1239-L1240)).
2. On `close(fd)`, `atomic_dec_and_test(&bdev->bd_openers)` decremented `disk_openers(lo->lo_disk)` to **`0` on line 763 — *before* `blkdev_flush_mapping(bdev)` and *before* `lo_release()`**.
3. On line 764, `blkdev_flush_mapping(bdev)` called [`kill_bdev(bdev)`](file:///usr/local/google/home/bvanassche/software/linux-kernel/block/bdev.c#L82-L91) -> [`truncate_inode_pages_range()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/mm/truncate.c#L400-L466) -> [`block_invalidate_folio()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/fs/buffer.c#L1516-L1562) -> `discard_buffer(bh)` -> **`lock_buffer(bh)`** (and `folio_lock()` on non-buffer-head paths), which **synchronously waited** for every in-flight `__breadahead()` BIO to complete **before calling `lo_release()` on line 766**.
4. While `kill_bdev()` was actively waiting for `loop_handle_cmd()` to complete those exact read BIOs, `loop_handle_cmd()` saw `lo_state == Lo_rundown` and `disk_openers == 0` and falsely printed `BUG: ... loop0 is no longer bound`.

### B. Exact proof that JFS commit `b15e4310633f` is the real root cause

Your diagnosis in `v6` was spot-on. Look at [`lbmIODone()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/fs/jfs/jfs_logmgr.c#L2281-L2302) after commit `b15e4310633f` ("jfs: Set the lbmDONE flag at the end of lbmIODone"):

```c
	/*
	 *	Group Commit pageout:
	 */
	else if (bp->l_flag & lbmGC) {
		LCACHE_UNLOCK(flags);
		lmPostGC(bp);
		LCACHE_LOCK(flags);		/* disable+lock */
	}
...
out:
	bp->l_flag |= lbmDONE;
	LCACHE_UNLOCK(flags);
```

Here is the exact race that leaks a live JFS log write past unmount:
1. **CPU 1 (`lbmIODone` for `bio1`)**: A partial-page group commit completes on `bp = log->bp` (`bp->l_flag == (lbmWRITE | lbmGC)`, without `lbmRELEASE`, so `bp` stays at `log->bp`). At line 2282, `lbmIODone()` **drops `LCACHE_LOCK`** and calls `lmPostGC(bp)`, which clears `logGC_PAGEOUT` and wakes `tblk->gcwait` in [`jfs_flush_journal()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/fs/jfs/jfs_logmgr.c#L1555-L1564). CPU 1 is delayed right before line 2284 (`LCACHE_LOCK`).
2. **CPU 0 ([`lmLogShutdown()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/fs/jfs/jfs_logmgr.c#L1640-L1658))**: Wakes from `jfs_flush_journal(log, 2)`, writes the final `LOG_SYNCPT` record into that **same partial `bp = log->bp`**, calls [`lbmWrite(log, log->bp, lbmWRITE | lbmRELEASE | lbmSYNC, 0)`](file:///usr/local/google/home/bvanassche/software/linux-kernel/fs/jfs/jfs_logmgr.c#L2025-L2062) (resetting `bp->l_flag = lbmWRITE | lbmRELEASE | lbmSYNC` and submitting **`bio2`** into `lo->workqueue`), and enters [`lbmIOWait(log->bp, lbmFREE)`](file:///usr/local/google/home/bvanassche/software/linux-kernel/fs/jfs/jfs_logmgr.c#L2140-L2160) waiting for `bp->l_flag & lbmDONE`.
3. **CPU 1 (`lbmIODone` for `bio1` resumes)**: Acquires `LCACHE_LOCK` at line 2284 and executes line 2300: **`bp->l_flag |= lbmDONE`** — clobbering `bp->l_flag` for **`bio2`**, which is still sitting unprocessed in `lo->workqueue`!
4. **CPU 0 (`lmLogShutdown()`)**: Sees `lbmDONE` set immediately, frees `bp` and `bp->l_page` in [`lbmLogShutdown()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/fs/jfs/jfs_logmgr.c#L1852-L1865), finishes `jfs_umount()`, and closes `/dev/loop0` -> `__loop_clr_fd()` sets `lo->lo_backing_file = NULL` before `lo->workqueue` processes `bio2` in `lo_rw_aio()`.

Fixing [`lbmIODone()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/fs/jfs/jfs_logmgr.c#L2281-L2302) so `bp->l_flag |= lbmDONE` is set **before** `LCACHE_UNLOCK(flags); lmPostGC(bp);` (and returning directly without re-locking just to set `lbmDONE` after `lmPostGC()` has handed `bp` off) fixes the JFS UAF/leak at its source without touching core block-device close semantics.