Re: [PATCH] block: stop the timeout timer when releasing a never added disk

[Chris S]

Hi Jan, I'm currently working on the changes we discussed.  Based on
vfs.all now.  Four things came out of writing it that I would rather
resolve before posting.

1. mark_buffer_write_io_error() is not safe for jbd2's buffers yet.  It
dereferences bh->b_folio->mapping directly (fs/buffer.c:1057), and
jbd2_journal_write_metadata_buffer() points the temporary bh at
virt_to_folio(jh->b_frozen_data), which is slab-backed.
buffer_set_crypto_ctx() right next to it already uses folio_mapping() for
exactly this reason.  So I would send a prerequisite converting
mark_buffer_write_io_error() to folio_mapping() before the jbd2 change.
Also worth noting the consequence of A2: in ordered mode the temp bh
inherits the source folio, so the error lands on the bdev mapping and
mapping_set_error() errseq_set()s s_wb_err - a later syncfs() on an
unrelated fd of the same fs can then return EIO.

2. gfs2 already has what we are building: gfs2_end_log_write_bh()
(fs/gfs2/lops.c:171) calls mark_buffer_write_io_error() and does not clear
uptodate.  Which means fs/gfs2/log.c:110 and :324 are blind to log write
errors today, and converting them is not behaviour-preserving - it makes
them start catching those.  I think that is right, but say if you would
rather it were separate.

3. On the sticky BH_Write_EIO: I am adding it to BUFFER_FLAGS_DISCARD as
you suggested.  That covers discard_buffer(), but there are four other
clear_buffer_req() sites (fs/buffer.c:1687, fs/gfs2/aops.c:600,
fs/jbd2/commit.c:1035, fs/jbd2/transaction.c:2462).  Three of them also
clear_buffer_mapped() and NULL b_bdev, so __bh_submit()'s
BUG_ON(!buffer_mapped(bh)) makes them unreachable.  Only
clean_bdev_aliases() leaves the buffer writable, and I could not construct
a workload that reaches it.  So I am not sending the ungating patch.  The
one place it would matter is ocfs2: fs/ocfs2/journal.c:691 is nested
inside an if (!buffer_uptodate(bh)) at :675 that goes dead once the clear
is gone, so :691 has to be hoisted out, and it then becomes a pre-use
check on a sticky flag rather than a post-write one. I will Cc ocfs2-devel
on that patch.

4. fs/jbd2/transaction.c:923, J_EXPECT_JH(jh, buffer_uptodate(bh)) in
jbd2_freeze_jh_data(): after the series a failed write leaves the buffer
uptodate, so this stops firing for write errors.  Arguably correct - the
in-memory copy being frozen is still valid - but it is your assert.  Leave
it, or convert it?  I instrumented it and ran ext4 with data=journal under
injected write errors, forcing copy-out; it never saw a non-uptodate
buffer, so I have no evidence either way.

One thing I did settle by testing.  fs/ext4/ext4_jbd2.c:416 open-codes
buffer_req(bh) && !buffer_uptodate(bh) after sync_dirty_buffer(), which
already returns -EIO.  On an instrumented kernel (ext4 without a journal,
-o sync, injected write errors) the two agreed on all 40 occurrences and
never diverged, with BH_Write_EIO set every time.  So that site just
consumes the return value and the buffer_req() question goes away.

Best,
Chao

On Wed, Jul 29, 2026 at 7:23 AM Jens Axboe <axboe@xxxxxxxxx> wrote:
>
>
> On Mon, 27 Jul 2026 16:12:57 -0400, Chao Shi wrote:
> > disk_release() undoes blk_mq_init_allocated_queue() for a disk whose
> > probe failed before add_disk(), but it only calls blk_mq_exit_queue().
> > Nothing there stops q->timeout, and that timer rolls forward: it stays
> > pending until it next expires, not until the last request completes.
> > So if the driver issued any I/O before adding the disk, the
> > request_queue is freed while still linked into a timer wheel bucket.
> >
> > [...]
>
> Applied, thanks!
>
> [1/1] block: stop the timeout timer when releasing a never added disk
>       commit: 26cb8ebbfaf713c82e142d08828d4d765057633b
>
> Best regards,
> --
> Jens Axboe
>
>
>