On Thu, Dec 14, 2023 at 01:38:45PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@xxxxxxxxxx>
>
> Overall, this function tries to find and invalidate all buffers for a
> given extent of space on the data device. The inner for loop in this
> function tries to find all xfs_bufs for a given daddr. The lengths of
> all possible cached buffers range from 1 fsblock to the largest needed
> to contain a 64k xattr value (~17fsb). The scan is capped to avoid
> looking at anything buffer going past the given extent.
>
> Unfortunately, the loop continuation test is wrong -- max_fsbs is the
> largest size we want to scan, not one past that. Put another way, this
> loop is actually 1-indexed, not 0-indexed. Therefore, the continuation
> test should use <=, not <.
>
> As a result, online repairs of btree blocks fails to stale any buffers
> for btrees that are being torn down, which causes later assertions in
> the buffer cache when another thread creates a different-sized buffer.
> This happens in xfs/709 when allocating an inode cluster buffer:
>
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 3346128 at fs/xfs/xfs_message.c:104 assfail+0x3a/0x40 [xfs]
> CPU: 0 PID: 3346128 Comm: fsstress Not tainted 6.7.0-rc4-djwx #rc4
> RIP: 0010:assfail+0x3a/0x40 [xfs]
> Call Trace:
> <TASK>
> _xfs_buf_obj_cmp+0x4a/0x50
> xfs_buf_get_map+0x191/0xba0
> xfs_trans_get_buf_map+0x136/0x280
> xfs_ialloc_inode_init+0x186/0x340
> xfs_ialloc_ag_alloc+0x254/0x720
> xfs_dialloc+0x21f/0x870
> xfs_create_tmpfile+0x1a9/0x2f0
> xfs_rename+0x369/0xfd0
> xfs_vn_rename+0xfa/0x170
> vfs_rename+0x5fb/0xc30
> do_renameat2+0x52d/0x6e0
> __x64_sys_renameat2+0x4b/0x60
> do_syscall_64+0x3b/0xe0
> entry_SYSCALL_64_after_hwframe+0x46/0x4e
>
> A later refactoring patch in the online repair series fixed this by
> accident, which is why I didn't notice this until I started testing only
> the patches that are likely to end up in 6.8.
>
> Fixes: 1c7ce115e521 ("xfs: reap large AG metadata extents when possible")
> Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> ---
> fs/xfs/scrub/reap.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/xfs/scrub/reap.c b/fs/xfs/scrub/reap.c
> index 9b6c919db522..f99eca799809 100644
> --- a/fs/xfs/scrub/reap.c
> +++ b/fs/xfs/scrub/reap.c
> @@ -251,7 +251,7 @@ xreap_agextent_binval(
> max_fsbs = min_t(xfs_agblock_t, agbno_next - bno,
> xfs_attr3_rmt_blocks(mp, XFS_XATTR_SIZE_MAX));
>
> - for (fsbcount = 1; fsbcount < max_fsbs; fsbcount++) {
> + for (fsbcount = 1; fsbcount <= max_fsbs; fsbcount++) {
> struct xfs_buf *bp = NULL;
> xfs_daddr_t daddr;
> int error;
Looks good.
Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>
--
Dave Chinner
david@xxxxxxxxxxxxx
