Re: [PATCH -next v5 30/32] ext4: add tracepoints for EOF block zeroing and disksize-grow I/O

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Calling `i_size_read()` from the block I/O completion context (softirq) inside the `ext4_iomap_wb_disksize_pending` tracepoint class causes a hard lockup on 32-bit SMP systems.
--

--- Patch [30]: [PATCH -next v5 30/32] ext4: add tracepoints for EOF block zeroing and disksize-grow I/O ---
commit 9f6116e7039af778ef1701a210ea9b800dcc543c
Author: Zhang Yi <[email protected]>

ext4: add tracepoints for EOF block zeroing and disksize-grow I/O

Add tracepoints to track the disksize-grow-pending lifecycle in the
writeback path and the block-zero-EOF entry point.

> diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> index 4f1176b9332f2..4464eb03c9728 100644
> --- a/fs/ext4/page-io.c
> +++ b/fs/ext4/page-io.c

[ ... ]

> @@ -729,8 +735,11 @@ void ext4_iomap_end_bio(struct bio *bio)
>  	 * state set in ext4_block_zero_eof() and wake up all waiters
>  	 * that will update the inode i_disksize.
>  	 */
> -	if (io_mode == EXT4_IOMAP_IOEND_DISKSIZE_GROW_IO)
> +	if (io_mode == EXT4_IOMAP_IOEND_DISKSIZE_GROW_IO) {
> +		trace_ext4_iomap_wb_disksize_pending_complete(ioend->io_inode,
> +				ioend->io_offset, ioend->io_size);
>  		ext4_iomap_clear_disksize_pending(ioend->io_inode);
> +	}

> diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h
> index 4539ef8e5f86c..c9259c2a3e364 100644
> --- a/include/trace/events/ext4.h
> +++ b/include/trace/events/ext4.h

[ ... ]

> +/* disksize pending I/O tracepoints for iomap Buffered I/O path */
> +DECLARE_EVENT_CLASS(ext4_iomap_wb_disksize_pending,
> +	TP_PROTO(struct inode *inode, loff_t io_offset, size_t io_size),
> +	TP_ARGS(inode, io_offset, io_size),
> +	TP_STRUCT__entry(
> +		__field(dev_t, dev)
> +		__field(u64, ino)
> +		__field(loff_t, io_offset)
> +		__field(size_t, io_size)
> +		__field(loff_t, i_size)
> +		__field(loff_t, i_disksize)
> +	),
> +	TP_fast_assign(
> +		__entry->dev = inode->i_sb->s_dev;
> +		__entry->ino = inode->i_ino;
> +		__entry->io_offset = io_offset;
> +		__entry->io_size = io_size;
> +		__entry->i_size = i_size_read(inode);

[Severity: High]
Does this code introduce a deadlock on 32-bit SMP systems?

The tracepoint ext4_iomap_wb_disksize_pending_complete is called from
ext4_iomap_end_bio(), which executes in softirq context. This happens
because ext4_iomap_writeback_submit() clears the BIO_COMPLETE_IN_TASK flag.

When TP_fast_assign() evaluates i_size_read(inode), it uses a read sequence
lock on 32-bit SMP systems via read_seqcount_begin(&inode->i_size_seqcount).

If i_size_write() is executing concurrently in process context on the same
CPU, it holds the sequence lock without disabling softirqs. If the softirq
interrupts the i_size_write() section, i_size_read() will spin infinitely
waiting for the sequence count to become even.

Is there a way to avoid reading the inode size in this softirq context, or
a safer way to sample it here?

> +		__entry->i_disksize = READ_ONCE(EXT4_I(inode)->i_disksize);
> +	),
> +	TP_printk("dev %d:%d ino %llu io_offset %lld io_size %zu i_size %lld i_disksize %lld",
> +		  MAJOR(__entry->dev), MINOR(__entry->dev),
> +		  __entry->ino, __entry->io_offset, __entry->io_size,
> +		  __entry->i_size, __entry->i_disksize)
> +);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=30