From: Ye Bin <[email protected]>
There's issue as follows:
XFS: Assertion failed: (bmv->bmv_iflags & BMV_IF_DELALLOC) != 0, file: fs/xfs/xfs_bmap_util.c, line: 329
------------[ cut here ]------------
kernel BUG at fs/xfs/xfs_message.c:102!
invalid opcode: 0000 [#1] PREEMPT SMP KASAN
CPU: 1 PID: 14612 Comm: xfs_io Not tainted 6.3.0-rc2-next-20230315-00006-g2729d23ddb3b-dirty #422
RIP: 0010:assfail+0x96/0xa0
RSP: 0018:ffffc9000fa178c0 EFLAGS: 00010246
RAX: 0000000000000000 RBX: 0000000000000001 RCX: ffff888179a18000
RDX: 0000000000000000 RSI: ffff888179a18000 RDI: 0000000000000002
RBP: 0000000000000000 R08: ffffffff8321aab6 R09: 0000000000000000
R10: 0000000000000001 R11: ffffed1105f85139 R12: ffffffff8aacc4c0
R13: 0000000000000149 R14: ffff888269f58000 R15: 000000000000000c
FS: 00007f42f27a4740(0000) GS:ffff88882fc00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000b92388 CR3: 000000024f006000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
xfs_getbmap+0x1a5b/0x1e40
xfs_ioc_getbmap+0x1fd/0x5b0
xfs_file_ioctl+0x2cb/0x1d50
__x64_sys_ioctl+0x197/0x210
do_syscall_64+0x39/0xb0
entry_SYSCALL_64_after_hwframe+0x63/0xcd
Above issue may happen as follows:
ThreadA ThreadB
do_shared_fault
__do_fault
xfs_filemap_fault
__xfs_filemap_fault
filemap_fault
xfs_ioc_getbmap -> Without BMV_IF_DELALLOC flag
xfs_getbmap
xfs_ilock(ip, XFS_IOLOCK_SHARED);
filemap_write_and_wait
do_page_mkwrite
xfs_filemap_page_mkwrite
__xfs_filemap_fault
xfs_ilock(XFS_I(inode), XFS_MMAPLOCK_SHARED);
iomap_page_mkwrite
...
xfs_buffered_write_iomap_begin
xfs_bmapi_reserve_delalloc -> Allocate delay extent
xfs_ilock_data_map_shared(ip)
xfs_getbmap_report_one
ASSERT((bmv->bmv_iflags & BMV_IF_DELALLOC) != 0)
-> trigger BUG_ON
As xfs_filemap_page_mkwrite() only hold XFS_MMAPLOCK_SHARED lock, there's
small window mkwrite can produce delay extent after file write in xfs_getbmap().
To solve above issue, hold XFS_MMAPLOCK_EXCL lock when do xfs_getbmap(),
to prevent write operations by do_page_mkwrite().
During doing __xfs_filemap_fault() we can't hold IOLOCK lock, as it's may lead
to ABBA dealock with xfs_file_write_iter().It's very easy to reproduce when
do fsstress, lockdep will detect deadlock.
Signed-off-by: Ye Bin <[email protected]>
---
fs/xfs/xfs_bmap_util.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index a09dd2606479..f23771a0cc8d 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -463,11 +463,13 @@ xfs_getbmap(
max_len = XFS_ISIZE(ip);
break;
case XFS_DATA_FORK:
+ lock = XFS_MMAPLOCK_EXCL;
+ xfs_ilock(ip, lock);
if (!(iflags & BMV_IF_DELALLOC) &&
(ip->i_delayed_blks || XFS_ISIZE(ip) > ip->i_disk_size)) {
error = filemap_write_and_wait(VFS_I(ip)->i_mapping);
if (error)
- goto out_unlock_iolock;
+ goto out_unlock_ilock;
/*
* Even after flushing the inode, there can still be
@@ -486,7 +488,7 @@ xfs_getbmap(
else
max_len = XFS_ISIZE(ip);
- lock = xfs_ilock_data_map_shared(ip);
+ lock |= xfs_ilock_data_map_shared(ip);
break;
}
--
2.31.1
[add Christoph to cc since he added/last touched this assert, I think]
On Mon, Mar 27, 2023 at 10:02:18PM +0800, Ye Bin wrote:
> From: Ye Bin <[email protected]>
>
> There's issue as follows:
> XFS: Assertion failed: (bmv->bmv_iflags & BMV_IF_DELALLOC) != 0, file: fs/xfs/xfs_bmap_util.c, line: 329
Why not get rid of the assertion? It's not like it changes the course
of the code flow -- userspace still gets told there's a delalloc extent.
Or, if the assert does serve some purpose, then do we need to take
the mmaplock for cow fork reporting too?
--D
> ------------[ cut here ]------------
> kernel BUG at fs/xfs/xfs_message.c:102!
> invalid opcode: 0000 [#1] PREEMPT SMP KASAN
> CPU: 1 PID: 14612 Comm: xfs_io Not tainted 6.3.0-rc2-next-20230315-00006-g2729d23ddb3b-dirty #422
> RIP: 0010:assfail+0x96/0xa0
> RSP: 0018:ffffc9000fa178c0 EFLAGS: 00010246
> RAX: 0000000000000000 RBX: 0000000000000001 RCX: ffff888179a18000
> RDX: 0000000000000000 RSI: ffff888179a18000 RDI: 0000000000000002
> RBP: 0000000000000000 R08: ffffffff8321aab6 R09: 0000000000000000
> R10: 0000000000000001 R11: ffffed1105f85139 R12: ffffffff8aacc4c0
> R13: 0000000000000149 R14: ffff888269f58000 R15: 000000000000000c
> FS: 00007f42f27a4740(0000) GS:ffff88882fc00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000b92388 CR3: 000000024f006000 CR4: 00000000000006e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> <TASK>
> xfs_getbmap+0x1a5b/0x1e40
> xfs_ioc_getbmap+0x1fd/0x5b0
> xfs_file_ioctl+0x2cb/0x1d50
> __x64_sys_ioctl+0x197/0x210
> do_syscall_64+0x39/0xb0
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
> Above issue may happen as follows:
> ThreadA ThreadB
> do_shared_fault
> __do_fault
> xfs_filemap_fault
> __xfs_filemap_fault
> filemap_fault
> xfs_ioc_getbmap -> Without BMV_IF_DELALLOC flag
> xfs_getbmap
> xfs_ilock(ip, XFS_IOLOCK_SHARED);
> filemap_write_and_wait
> do_page_mkwrite
> xfs_filemap_page_mkwrite
> __xfs_filemap_fault
> xfs_ilock(XFS_I(inode), XFS_MMAPLOCK_SHARED);
> iomap_page_mkwrite
> ...
> xfs_buffered_write_iomap_begin
> xfs_bmapi_reserve_delalloc -> Allocate delay extent
> xfs_ilock_data_map_shared(ip)
> xfs_getbmap_report_one
> ASSERT((bmv->bmv_iflags & BMV_IF_DELALLOC) != 0)
> -> trigger BUG_ON
>
> As xfs_filemap_page_mkwrite() only hold XFS_MMAPLOCK_SHARED lock, there's
> small window mkwrite can produce delay extent after file write in xfs_getbmap().
> To solve above issue, hold XFS_MMAPLOCK_EXCL lock when do xfs_getbmap(),
> to prevent write operations by do_page_mkwrite().
> During doing __xfs_filemap_fault() we can't hold IOLOCK lock, as it's may lead
> to ABBA dealock with xfs_file_write_iter().It's very easy to reproduce when
> do fsstress, lockdep will detect deadlock.
>
> Signed-off-by: Ye Bin <[email protected]>
> ---
> fs/xfs/xfs_bmap_util.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index a09dd2606479..f23771a0cc8d 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -463,11 +463,13 @@ xfs_getbmap(
> max_len = XFS_ISIZE(ip);
> break;
> case XFS_DATA_FORK:
> + lock = XFS_MMAPLOCK_EXCL;
> + xfs_ilock(ip, lock);
> if (!(iflags & BMV_IF_DELALLOC) &&
> (ip->i_delayed_blks || XFS_ISIZE(ip) > ip->i_disk_size)) {
> error = filemap_write_and_wait(VFS_I(ip)->i_mapping);
> if (error)
> - goto out_unlock_iolock;
> + goto out_unlock_ilock;
>
> /*
> * Even after flushing the inode, there can still be
> @@ -486,7 +488,7 @@ xfs_getbmap(
> else
> max_len = XFS_ISIZE(ip);
>
> - lock = xfs_ilock_data_map_shared(ip);
> + lock |= xfs_ilock_data_map_shared(ip);
> break;
> }
>
> --
> 2.31.1
>
Hi Ye,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on xfs-linux/for-next]
[also build test WARNING on linus/master v6.3-rc4 next-20230327]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Ye-Bin/xfs-fix-BUG_ON-in-xfs_getbmap/20230327-220330
base: https://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git for-next
patch link: https://lore.kernel.org/r/20230327140218.4154709-1-yebin%40huaweicloud.com
patch subject: [PATCH] xfs: fix BUG_ON in xfs_getbmap()
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20230328/[email protected]/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/ff0f6481a35ee27471d1511047c34f4885e2f5aa
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Ye-Bin/xfs-fix-BUG_ON-in-xfs_getbmap/20230327-220330
git checkout ff0f6481a35ee27471d1511047c34f4885e2f5aa
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 olddefconfig
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash fs/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
fs/xfs/xfs_bmap_util.c: In function 'xfs_getbmap':
>> fs/xfs/xfs_bmap_util.c:581:1: warning: label 'out_unlock_iolock' defined but not used [-Wunused-label]
581 | out_unlock_iolock:
| ^~~~~~~~~~~~~~~~~
vim +/out_unlock_iolock +581 fs/xfs/xfs_bmap_util.c
f86f403794b144 Darrick J. Wong 2016-10-03 396
6898811459ff52 Dave Chinner 2013-08-12 397 /*
6898811459ff52 Dave Chinner 2013-08-12 398 * Get inode's extents as described in bmv, and format for output.
6898811459ff52 Dave Chinner 2013-08-12 399 * Calls formatter to fill the user's buffer until all extents
6898811459ff52 Dave Chinner 2013-08-12 400 * are mapped, until the passed-in bmv->bmv_count slots have
6898811459ff52 Dave Chinner 2013-08-12 401 * been filled, or until the formatter short-circuits the loop,
6898811459ff52 Dave Chinner 2013-08-12 402 * if it is tracking filled-in extents on its own.
6898811459ff52 Dave Chinner 2013-08-12 403 */
6898811459ff52 Dave Chinner 2013-08-12 404 int /* error code */
6898811459ff52 Dave Chinner 2013-08-12 405 xfs_getbmap(
232b51948b99df Christoph Hellwig 2017-10-17 406 struct xfs_inode *ip,
6898811459ff52 Dave Chinner 2013-08-12 407 struct getbmapx *bmv, /* user bmap structure */
232b51948b99df Christoph Hellwig 2017-10-17 408 struct kgetbmap *out)
6898811459ff52 Dave Chinner 2013-08-12 409 {
abbf9e8a450748 Christoph Hellwig 2017-10-17 410 struct xfs_mount *mp = ip->i_mount;
abbf9e8a450748 Christoph Hellwig 2017-10-17 411 int iflags = bmv->bmv_iflags;
232b51948b99df Christoph Hellwig 2017-10-17 412 int whichfork, lock, error = 0;
abbf9e8a450748 Christoph Hellwig 2017-10-17 413 int64_t bmv_end, max_len;
abbf9e8a450748 Christoph Hellwig 2017-10-17 414 xfs_fileoff_t bno, first_bno;
abbf9e8a450748 Christoph Hellwig 2017-10-17 415 struct xfs_ifork *ifp;
abbf9e8a450748 Christoph Hellwig 2017-10-17 416 struct xfs_bmbt_irec got, rec;
abbf9e8a450748 Christoph Hellwig 2017-10-17 417 xfs_filblks_t len;
b2b1712a640824 Christoph Hellwig 2017-11-03 418 struct xfs_iext_cursor icur;
6898811459ff52 Dave Chinner 2013-08-12 419
232b51948b99df Christoph Hellwig 2017-10-17 420 if (bmv->bmv_iflags & ~BMV_IF_VALID)
232b51948b99df Christoph Hellwig 2017-10-17 421 return -EINVAL;
f86f403794b144 Darrick J. Wong 2016-10-03 422 #ifndef DEBUG
f86f403794b144 Darrick J. Wong 2016-10-03 423 /* Only allow CoW fork queries if we're debugging. */
f86f403794b144 Darrick J. Wong 2016-10-03 424 if (iflags & BMV_IF_COWFORK)
f86f403794b144 Darrick J. Wong 2016-10-03 425 return -EINVAL;
f86f403794b144 Darrick J. Wong 2016-10-03 426 #endif
f86f403794b144 Darrick J. Wong 2016-10-03 427 if ((iflags & BMV_IF_ATTRFORK) && (iflags & BMV_IF_COWFORK))
f86f403794b144 Darrick J. Wong 2016-10-03 428 return -EINVAL;
f86f403794b144 Darrick J. Wong 2016-10-03 429
abbf9e8a450748 Christoph Hellwig 2017-10-17 430 if (bmv->bmv_length < -1)
abbf9e8a450748 Christoph Hellwig 2017-10-17 431 return -EINVAL;
abbf9e8a450748 Christoph Hellwig 2017-10-17 432 bmv->bmv_entries = 0;
abbf9e8a450748 Christoph Hellwig 2017-10-17 433 if (bmv->bmv_length == 0)
abbf9e8a450748 Christoph Hellwig 2017-10-17 434 return 0;
abbf9e8a450748 Christoph Hellwig 2017-10-17 435
f86f403794b144 Darrick J. Wong 2016-10-03 436 if (iflags & BMV_IF_ATTRFORK)
f86f403794b144 Darrick J. Wong 2016-10-03 437 whichfork = XFS_ATTR_FORK;
f86f403794b144 Darrick J. Wong 2016-10-03 438 else if (iflags & BMV_IF_COWFORK)
f86f403794b144 Darrick J. Wong 2016-10-03 439 whichfork = XFS_COW_FORK;
f86f403794b144 Darrick J. Wong 2016-10-03 440 else
f86f403794b144 Darrick J. Wong 2016-10-03 441 whichfork = XFS_DATA_FORK;
f86f403794b144 Darrick J. Wong 2016-10-03 442
abbf9e8a450748 Christoph Hellwig 2017-10-17 443 xfs_ilock(ip, XFS_IOLOCK_SHARED);
f86f403794b144 Darrick J. Wong 2016-10-03 444 switch (whichfork) {
f86f403794b144 Darrick J. Wong 2016-10-03 445 case XFS_ATTR_FORK:
001c179c4e26d0 ChenXiaoSong 2022-07-27 446 lock = xfs_ilock_attr_map_shared(ip);
932b42c66cb5d0 Darrick J. Wong 2022-07-09 447 if (!xfs_inode_has_attr_fork(ip))
001c179c4e26d0 ChenXiaoSong 2022-07-27 448 goto out_unlock_ilock;
6898811459ff52 Dave Chinner 2013-08-12 449
abbf9e8a450748 Christoph Hellwig 2017-10-17 450 max_len = 1LL << 32;
f86f403794b144 Darrick J. Wong 2016-10-03 451 break;
f86f403794b144 Darrick J. Wong 2016-10-03 452 case XFS_COW_FORK:
001c179c4e26d0 ChenXiaoSong 2022-07-27 453 lock = XFS_ILOCK_SHARED;
001c179c4e26d0 ChenXiaoSong 2022-07-27 454 xfs_ilock(ip, lock);
001c179c4e26d0 ChenXiaoSong 2022-07-27 455
abbf9e8a450748 Christoph Hellwig 2017-10-17 456 /* No CoW fork? Just return */
001c179c4e26d0 ChenXiaoSong 2022-07-27 457 if (!xfs_ifork_ptr(ip, whichfork))
001c179c4e26d0 ChenXiaoSong 2022-07-27 458 goto out_unlock_ilock;
f86f403794b144 Darrick J. Wong 2016-10-03 459
abbf9e8a450748 Christoph Hellwig 2017-10-17 460 if (xfs_get_cowextsz_hint(ip))
abbf9e8a450748 Christoph Hellwig 2017-10-17 461 max_len = mp->m_super->s_maxbytes;
abbf9e8a450748 Christoph Hellwig 2017-10-17 462 else
abbf9e8a450748 Christoph Hellwig 2017-10-17 463 max_len = XFS_ISIZE(ip);
f86f403794b144 Darrick J. Wong 2016-10-03 464 break;
f86f403794b144 Darrick J. Wong 2016-10-03 465 case XFS_DATA_FORK:
ff0f6481a35ee2 Ye Bin 2023-03-27 466 lock = XFS_MMAPLOCK_EXCL;
ff0f6481a35ee2 Ye Bin 2023-03-27 467 xfs_ilock(ip, lock);
efa70be1654978 Christoph Hellwig 2013-12-18 468 if (!(iflags & BMV_IF_DELALLOC) &&
13d2c10b05d8e6 Christoph Hellwig 2021-03-29 469 (ip->i_delayed_blks || XFS_ISIZE(ip) > ip->i_disk_size)) {
2451337dd04390 Dave Chinner 2014-06-25 470 error = filemap_write_and_wait(VFS_I(ip)->i_mapping);
6898811459ff52 Dave Chinner 2013-08-12 471 if (error)
ff0f6481a35ee2 Ye Bin 2023-03-27 472 goto out_unlock_ilock;
efa70be1654978 Christoph Hellwig 2013-12-18 473
6898811459ff52 Dave Chinner 2013-08-12 474 /*
efa70be1654978 Christoph Hellwig 2013-12-18 475 * Even after flushing the inode, there can still be
efa70be1654978 Christoph Hellwig 2013-12-18 476 * delalloc blocks on the inode beyond EOF due to
efa70be1654978 Christoph Hellwig 2013-12-18 477 * speculative preallocation. These are not removed
efa70be1654978 Christoph Hellwig 2013-12-18 478 * until the release function is called or the inode
efa70be1654978 Christoph Hellwig 2013-12-18 479 * is inactivated. Hence we cannot assert here that
efa70be1654978 Christoph Hellwig 2013-12-18 480 * ip->i_delayed_blks == 0.
6898811459ff52 Dave Chinner 2013-08-12 481 */
6898811459ff52 Dave Chinner 2013-08-12 482 }
6898811459ff52 Dave Chinner 2013-08-12 483
abbf9e8a450748 Christoph Hellwig 2017-10-17 484 if (xfs_get_extsz_hint(ip) ||
db07349da2f564 Christoph Hellwig 2021-03-29 485 (ip->i_diflags &
abbf9e8a450748 Christoph Hellwig 2017-10-17 486 (XFS_DIFLAG_PREALLOC | XFS_DIFLAG_APPEND)))
abbf9e8a450748 Christoph Hellwig 2017-10-17 487 max_len = mp->m_super->s_maxbytes;
abbf9e8a450748 Christoph Hellwig 2017-10-17 488 else
abbf9e8a450748 Christoph Hellwig 2017-10-17 489 max_len = XFS_ISIZE(ip);
abbf9e8a450748 Christoph Hellwig 2017-10-17 490
ff0f6481a35ee2 Ye Bin 2023-03-27 491 lock |= xfs_ilock_data_map_shared(ip);
f86f403794b144 Darrick J. Wong 2016-10-03 492 break;
efa70be1654978 Christoph Hellwig 2013-12-18 493 }
6898811459ff52 Dave Chinner 2013-08-12 494
001c179c4e26d0 ChenXiaoSong 2022-07-27 495 ifp = xfs_ifork_ptr(ip, whichfork);
001c179c4e26d0 ChenXiaoSong 2022-07-27 496
f7e67b20ecbbcb Christoph Hellwig 2020-05-18 497 switch (ifp->if_format) {
abbf9e8a450748 Christoph Hellwig 2017-10-17 498 case XFS_DINODE_FMT_EXTENTS:
abbf9e8a450748 Christoph Hellwig 2017-10-17 499 case XFS_DINODE_FMT_BTREE:
abbf9e8a450748 Christoph Hellwig 2017-10-17 500 break;
abbf9e8a450748 Christoph Hellwig 2017-10-17 501 case XFS_DINODE_FMT_LOCAL:
abbf9e8a450748 Christoph Hellwig 2017-10-17 502 /* Local format inode forks report no extents. */
6898811459ff52 Dave Chinner 2013-08-12 503 goto out_unlock_ilock;
abbf9e8a450748 Christoph Hellwig 2017-10-17 504 default:
abbf9e8a450748 Christoph Hellwig 2017-10-17 505 error = -EINVAL;
abbf9e8a450748 Christoph Hellwig 2017-10-17 506 goto out_unlock_ilock;
abbf9e8a450748 Christoph Hellwig 2017-10-17 507 }
6898811459ff52 Dave Chinner 2013-08-12 508
abbf9e8a450748 Christoph Hellwig 2017-10-17 509 if (bmv->bmv_length == -1) {
abbf9e8a450748 Christoph Hellwig 2017-10-17 510 max_len = XFS_FSB_TO_BB(mp, XFS_B_TO_FSB(mp, max_len));
abbf9e8a450748 Christoph Hellwig 2017-10-17 511 bmv->bmv_length = max(0LL, max_len - bmv->bmv_offset);
6898811459ff52 Dave Chinner 2013-08-12 512 }
6898811459ff52 Dave Chinner 2013-08-12 513
abbf9e8a450748 Christoph Hellwig 2017-10-17 514 bmv_end = bmv->bmv_offset + bmv->bmv_length;
abbf9e8a450748 Christoph Hellwig 2017-10-17 515
abbf9e8a450748 Christoph Hellwig 2017-10-17 516 first_bno = bno = XFS_BB_TO_FSBT(mp, bmv->bmv_offset);
abbf9e8a450748 Christoph Hellwig 2017-10-17 517 len = XFS_BB_TO_FSB(mp, bmv->bmv_length);
abbf9e8a450748 Christoph Hellwig 2017-10-17 518
abbf9e8a450748 Christoph Hellwig 2017-10-17 519 error = xfs_iread_extents(NULL, ip, whichfork);
6898811459ff52 Dave Chinner 2013-08-12 520 if (error)
abbf9e8a450748 Christoph Hellwig 2017-10-17 521 goto out_unlock_ilock;
6898811459ff52 Dave Chinner 2013-08-12 522
b2b1712a640824 Christoph Hellwig 2017-11-03 523 if (!xfs_iext_lookup_extent(ip, ifp, bno, &icur, &got)) {
6898811459ff52 Dave Chinner 2013-08-12 524 /*
abbf9e8a450748 Christoph Hellwig 2017-10-17 525 * Report a whole-file hole if the delalloc flag is set to
abbf9e8a450748 Christoph Hellwig 2017-10-17 526 * stay compatible with the old implementation.
6898811459ff52 Dave Chinner 2013-08-12 527 */
abbf9e8a450748 Christoph Hellwig 2017-10-17 528 if (iflags & BMV_IF_DELALLOC)
abbf9e8a450748 Christoph Hellwig 2017-10-17 529 xfs_getbmap_report_hole(ip, bmv, out, bmv_end, bno,
abbf9e8a450748 Christoph Hellwig 2017-10-17 530 XFS_B_TO_FSB(mp, XFS_ISIZE(ip)));
abbf9e8a450748 Christoph Hellwig 2017-10-17 531 goto out_unlock_ilock;
6898811459ff52 Dave Chinner 2013-08-12 532 }
6898811459ff52 Dave Chinner 2013-08-12 533
abbf9e8a450748 Christoph Hellwig 2017-10-17 534 while (!xfs_getbmap_full(bmv)) {
abbf9e8a450748 Christoph Hellwig 2017-10-17 535 xfs_trim_extent(&got, first_bno, len);
6898811459ff52 Dave Chinner 2013-08-12 536
6898811459ff52 Dave Chinner 2013-08-12 537 /*
abbf9e8a450748 Christoph Hellwig 2017-10-17 538 * Report an entry for a hole if this extent doesn't directly
abbf9e8a450748 Christoph Hellwig 2017-10-17 539 * follow the previous one.
6898811459ff52 Dave Chinner 2013-08-12 540 */
abbf9e8a450748 Christoph Hellwig 2017-10-17 541 if (got.br_startoff > bno) {
abbf9e8a450748 Christoph Hellwig 2017-10-17 542 xfs_getbmap_report_hole(ip, bmv, out, bmv_end, bno,
abbf9e8a450748 Christoph Hellwig 2017-10-17 543 got.br_startoff);
abbf9e8a450748 Christoph Hellwig 2017-10-17 544 if (xfs_getbmap_full(bmv))
abbf9e8a450748 Christoph Hellwig 2017-10-17 545 break;
6898811459ff52 Dave Chinner 2013-08-12 546 }
6898811459ff52 Dave Chinner 2013-08-12 547
c364b6d0b6cda1 Darrick J. Wong 2017-01-26 548 /*
abbf9e8a450748 Christoph Hellwig 2017-10-17 549 * In order to report shared extents accurately, we report each
abbf9e8a450748 Christoph Hellwig 2017-10-17 550 * distinct shared / unshared part of a single bmbt record with
abbf9e8a450748 Christoph Hellwig 2017-10-17 551 * an individual getbmapx record.
c364b6d0b6cda1 Darrick J. Wong 2017-01-26 552 */
abbf9e8a450748 Christoph Hellwig 2017-10-17 553 bno = got.br_startoff + got.br_blockcount;
abbf9e8a450748 Christoph Hellwig 2017-10-17 554 rec = got;
abbf9e8a450748 Christoph Hellwig 2017-10-17 555 do {
abbf9e8a450748 Christoph Hellwig 2017-10-17 556 error = xfs_getbmap_report_one(ip, bmv, out, bmv_end,
abbf9e8a450748 Christoph Hellwig 2017-10-17 557 &rec);
abbf9e8a450748 Christoph Hellwig 2017-10-17 558 if (error || xfs_getbmap_full(bmv))
abbf9e8a450748 Christoph Hellwig 2017-10-17 559 goto out_unlock_ilock;
abbf9e8a450748 Christoph Hellwig 2017-10-17 560 } while (xfs_getbmap_next_rec(&rec, bno));
abbf9e8a450748 Christoph Hellwig 2017-10-17 561
b2b1712a640824 Christoph Hellwig 2017-11-03 562 if (!xfs_iext_next_extent(ifp, &icur, &got)) {
abbf9e8a450748 Christoph Hellwig 2017-10-17 563 xfs_fileoff_t end = XFS_B_TO_FSB(mp, XFS_ISIZE(ip));
abbf9e8a450748 Christoph Hellwig 2017-10-17 564
abbf9e8a450748 Christoph Hellwig 2017-10-17 565 out[bmv->bmv_entries - 1].bmv_oflags |= BMV_OF_LAST;
abbf9e8a450748 Christoph Hellwig 2017-10-17 566
abbf9e8a450748 Christoph Hellwig 2017-10-17 567 if (whichfork != XFS_ATTR_FORK && bno < end &&
abbf9e8a450748 Christoph Hellwig 2017-10-17 568 !xfs_getbmap_full(bmv)) {
abbf9e8a450748 Christoph Hellwig 2017-10-17 569 xfs_getbmap_report_hole(ip, bmv, out, bmv_end,
abbf9e8a450748 Christoph Hellwig 2017-10-17 570 bno, end);
c364b6d0b6cda1 Darrick J. Wong 2017-01-26 571 }
abbf9e8a450748 Christoph Hellwig 2017-10-17 572 break;
abbf9e8a450748 Christoph Hellwig 2017-10-17 573 }
abbf9e8a450748 Christoph Hellwig 2017-10-17 574
abbf9e8a450748 Christoph Hellwig 2017-10-17 575 if (bno >= first_bno + len)
abbf9e8a450748 Christoph Hellwig 2017-10-17 576 break;
6898811459ff52 Dave Chinner 2013-08-12 577 }
6898811459ff52 Dave Chinner 2013-08-12 578
6898811459ff52 Dave Chinner 2013-08-12 579 out_unlock_ilock:
01f4f3277556d4 Christoph Hellwig 2013-12-06 580 xfs_iunlock(ip, lock);
6898811459ff52 Dave Chinner 2013-08-12 @581 out_unlock_iolock:
6898811459ff52 Dave Chinner 2013-08-12 582 xfs_iunlock(ip, XFS_IOLOCK_SHARED);
6898811459ff52 Dave Chinner 2013-08-12 583 return error;
6898811459ff52 Dave Chinner 2013-08-12 584 }
6898811459ff52 Dave Chinner 2013-08-12 585
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Hi Ye,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on xfs-linux/for-next]
[also build test WARNING on linus/master v6.3-rc4 next-20230327]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Ye-Bin/xfs-fix-BUG_ON-in-xfs_getbmap/20230327-220330
base: https://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git for-next
patch link: https://lore.kernel.org/r/20230327140218.4154709-1-yebin%40huaweicloud.com
patch subject: [PATCH] xfs: fix BUG_ON in xfs_getbmap()
config: s390-randconfig-r005-20230326 (https://download.01.org/0day-ci/archive/20230328/[email protected]/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install s390 cross compiling tool for clang build
# apt-get install binutils-s390x-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/ff0f6481a35ee27471d1511047c34f4885e2f5aa
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Ye-Bin/xfs-fix-BUG_ON-in-xfs_getbmap/20230327-220330
git checkout ff0f6481a35ee27471d1511047c34f4885e2f5aa
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash fs/xfs/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> fs/xfs/xfs_bmap_util.c:581:1: warning: unused label 'out_unlock_iolock' [-Wunused-label]
out_unlock_iolock:
^~~~~~~~~~~~~~~~~~
1 warning generated.
vim +/out_unlock_iolock +581 fs/xfs/xfs_bmap_util.c
f86f403794b144 Darrick J. Wong 2016-10-03 396
6898811459ff52 Dave Chinner 2013-08-12 397 /*
6898811459ff52 Dave Chinner 2013-08-12 398 * Get inode's extents as described in bmv, and format for output.
6898811459ff52 Dave Chinner 2013-08-12 399 * Calls formatter to fill the user's buffer until all extents
6898811459ff52 Dave Chinner 2013-08-12 400 * are mapped, until the passed-in bmv->bmv_count slots have
6898811459ff52 Dave Chinner 2013-08-12 401 * been filled, or until the formatter short-circuits the loop,
6898811459ff52 Dave Chinner 2013-08-12 402 * if it is tracking filled-in extents on its own.
6898811459ff52 Dave Chinner 2013-08-12 403 */
6898811459ff52 Dave Chinner 2013-08-12 404 int /* error code */
6898811459ff52 Dave Chinner 2013-08-12 405 xfs_getbmap(
232b51948b99df Christoph Hellwig 2017-10-17 406 struct xfs_inode *ip,
6898811459ff52 Dave Chinner 2013-08-12 407 struct getbmapx *bmv, /* user bmap structure */
232b51948b99df Christoph Hellwig 2017-10-17 408 struct kgetbmap *out)
6898811459ff52 Dave Chinner 2013-08-12 409 {
abbf9e8a450748 Christoph Hellwig 2017-10-17 410 struct xfs_mount *mp = ip->i_mount;
abbf9e8a450748 Christoph Hellwig 2017-10-17 411 int iflags = bmv->bmv_iflags;
232b51948b99df Christoph Hellwig 2017-10-17 412 int whichfork, lock, error = 0;
abbf9e8a450748 Christoph Hellwig 2017-10-17 413 int64_t bmv_end, max_len;
abbf9e8a450748 Christoph Hellwig 2017-10-17 414 xfs_fileoff_t bno, first_bno;
abbf9e8a450748 Christoph Hellwig 2017-10-17 415 struct xfs_ifork *ifp;
abbf9e8a450748 Christoph Hellwig 2017-10-17 416 struct xfs_bmbt_irec got, rec;
abbf9e8a450748 Christoph Hellwig 2017-10-17 417 xfs_filblks_t len;
b2b1712a640824 Christoph Hellwig 2017-11-03 418 struct xfs_iext_cursor icur;
6898811459ff52 Dave Chinner 2013-08-12 419
232b51948b99df Christoph Hellwig 2017-10-17 420 if (bmv->bmv_iflags & ~BMV_IF_VALID)
232b51948b99df Christoph Hellwig 2017-10-17 421 return -EINVAL;
f86f403794b144 Darrick J. Wong 2016-10-03 422 #ifndef DEBUG
f86f403794b144 Darrick J. Wong 2016-10-03 423 /* Only allow CoW fork queries if we're debugging. */
f86f403794b144 Darrick J. Wong 2016-10-03 424 if (iflags & BMV_IF_COWFORK)
f86f403794b144 Darrick J. Wong 2016-10-03 425 return -EINVAL;
f86f403794b144 Darrick J. Wong 2016-10-03 426 #endif
f86f403794b144 Darrick J. Wong 2016-10-03 427 if ((iflags & BMV_IF_ATTRFORK) && (iflags & BMV_IF_COWFORK))
f86f403794b144 Darrick J. Wong 2016-10-03 428 return -EINVAL;
f86f403794b144 Darrick J. Wong 2016-10-03 429
abbf9e8a450748 Christoph Hellwig 2017-10-17 430 if (bmv->bmv_length < -1)
abbf9e8a450748 Christoph Hellwig 2017-10-17 431 return -EINVAL;
abbf9e8a450748 Christoph Hellwig 2017-10-17 432 bmv->bmv_entries = 0;
abbf9e8a450748 Christoph Hellwig 2017-10-17 433 if (bmv->bmv_length == 0)
abbf9e8a450748 Christoph Hellwig 2017-10-17 434 return 0;
abbf9e8a450748 Christoph Hellwig 2017-10-17 435
f86f403794b144 Darrick J. Wong 2016-10-03 436 if (iflags & BMV_IF_ATTRFORK)
f86f403794b144 Darrick J. Wong 2016-10-03 437 whichfork = XFS_ATTR_FORK;
f86f403794b144 Darrick J. Wong 2016-10-03 438 else if (iflags & BMV_IF_COWFORK)
f86f403794b144 Darrick J. Wong 2016-10-03 439 whichfork = XFS_COW_FORK;
f86f403794b144 Darrick J. Wong 2016-10-03 440 else
f86f403794b144 Darrick J. Wong 2016-10-03 441 whichfork = XFS_DATA_FORK;
f86f403794b144 Darrick J. Wong 2016-10-03 442
abbf9e8a450748 Christoph Hellwig 2017-10-17 443 xfs_ilock(ip, XFS_IOLOCK_SHARED);
f86f403794b144 Darrick J. Wong 2016-10-03 444 switch (whichfork) {
f86f403794b144 Darrick J. Wong 2016-10-03 445 case XFS_ATTR_FORK:
001c179c4e26d0 ChenXiaoSong 2022-07-27 446 lock = xfs_ilock_attr_map_shared(ip);
932b42c66cb5d0 Darrick J. Wong 2022-07-09 447 if (!xfs_inode_has_attr_fork(ip))
001c179c4e26d0 ChenXiaoSong 2022-07-27 448 goto out_unlock_ilock;
6898811459ff52 Dave Chinner 2013-08-12 449
abbf9e8a450748 Christoph Hellwig 2017-10-17 450 max_len = 1LL << 32;
f86f403794b144 Darrick J. Wong 2016-10-03 451 break;
f86f403794b144 Darrick J. Wong 2016-10-03 452 case XFS_COW_FORK:
001c179c4e26d0 ChenXiaoSong 2022-07-27 453 lock = XFS_ILOCK_SHARED;
001c179c4e26d0 ChenXiaoSong 2022-07-27 454 xfs_ilock(ip, lock);
001c179c4e26d0 ChenXiaoSong 2022-07-27 455
abbf9e8a450748 Christoph Hellwig 2017-10-17 456 /* No CoW fork? Just return */
001c179c4e26d0 ChenXiaoSong 2022-07-27 457 if (!xfs_ifork_ptr(ip, whichfork))
001c179c4e26d0 ChenXiaoSong 2022-07-27 458 goto out_unlock_ilock;
f86f403794b144 Darrick J. Wong 2016-10-03 459
abbf9e8a450748 Christoph Hellwig 2017-10-17 460 if (xfs_get_cowextsz_hint(ip))
abbf9e8a450748 Christoph Hellwig 2017-10-17 461 max_len = mp->m_super->s_maxbytes;
abbf9e8a450748 Christoph Hellwig 2017-10-17 462 else
abbf9e8a450748 Christoph Hellwig 2017-10-17 463 max_len = XFS_ISIZE(ip);
f86f403794b144 Darrick J. Wong 2016-10-03 464 break;
f86f403794b144 Darrick J. Wong 2016-10-03 465 case XFS_DATA_FORK:
ff0f6481a35ee2 Ye Bin 2023-03-27 466 lock = XFS_MMAPLOCK_EXCL;
ff0f6481a35ee2 Ye Bin 2023-03-27 467 xfs_ilock(ip, lock);
efa70be1654978 Christoph Hellwig 2013-12-18 468 if (!(iflags & BMV_IF_DELALLOC) &&
13d2c10b05d8e6 Christoph Hellwig 2021-03-29 469 (ip->i_delayed_blks || XFS_ISIZE(ip) > ip->i_disk_size)) {
2451337dd04390 Dave Chinner 2014-06-25 470 error = filemap_write_and_wait(VFS_I(ip)->i_mapping);
6898811459ff52 Dave Chinner 2013-08-12 471 if (error)
ff0f6481a35ee2 Ye Bin 2023-03-27 472 goto out_unlock_ilock;
efa70be1654978 Christoph Hellwig 2013-12-18 473
6898811459ff52 Dave Chinner 2013-08-12 474 /*
efa70be1654978 Christoph Hellwig 2013-12-18 475 * Even after flushing the inode, there can still be
efa70be1654978 Christoph Hellwig 2013-12-18 476 * delalloc blocks on the inode beyond EOF due to
efa70be1654978 Christoph Hellwig 2013-12-18 477 * speculative preallocation. These are not removed
efa70be1654978 Christoph Hellwig 2013-12-18 478 * until the release function is called or the inode
efa70be1654978 Christoph Hellwig 2013-12-18 479 * is inactivated. Hence we cannot assert here that
efa70be1654978 Christoph Hellwig 2013-12-18 480 * ip->i_delayed_blks == 0.
6898811459ff52 Dave Chinner 2013-08-12 481 */
6898811459ff52 Dave Chinner 2013-08-12 482 }
6898811459ff52 Dave Chinner 2013-08-12 483
abbf9e8a450748 Christoph Hellwig 2017-10-17 484 if (xfs_get_extsz_hint(ip) ||
db07349da2f564 Christoph Hellwig 2021-03-29 485 (ip->i_diflags &
abbf9e8a450748 Christoph Hellwig 2017-10-17 486 (XFS_DIFLAG_PREALLOC | XFS_DIFLAG_APPEND)))
abbf9e8a450748 Christoph Hellwig 2017-10-17 487 max_len = mp->m_super->s_maxbytes;
abbf9e8a450748 Christoph Hellwig 2017-10-17 488 else
abbf9e8a450748 Christoph Hellwig 2017-10-17 489 max_len = XFS_ISIZE(ip);
abbf9e8a450748 Christoph Hellwig 2017-10-17 490
ff0f6481a35ee2 Ye Bin 2023-03-27 491 lock |= xfs_ilock_data_map_shared(ip);
f86f403794b144 Darrick J. Wong 2016-10-03 492 break;
efa70be1654978 Christoph Hellwig 2013-12-18 493 }
6898811459ff52 Dave Chinner 2013-08-12 494
001c179c4e26d0 ChenXiaoSong 2022-07-27 495 ifp = xfs_ifork_ptr(ip, whichfork);
001c179c4e26d0 ChenXiaoSong 2022-07-27 496
f7e67b20ecbbcb Christoph Hellwig 2020-05-18 497 switch (ifp->if_format) {
abbf9e8a450748 Christoph Hellwig 2017-10-17 498 case XFS_DINODE_FMT_EXTENTS:
abbf9e8a450748 Christoph Hellwig 2017-10-17 499 case XFS_DINODE_FMT_BTREE:
abbf9e8a450748 Christoph Hellwig 2017-10-17 500 break;
abbf9e8a450748 Christoph Hellwig 2017-10-17 501 case XFS_DINODE_FMT_LOCAL:
abbf9e8a450748 Christoph Hellwig 2017-10-17 502 /* Local format inode forks report no extents. */
6898811459ff52 Dave Chinner 2013-08-12 503 goto out_unlock_ilock;
abbf9e8a450748 Christoph Hellwig 2017-10-17 504 default:
abbf9e8a450748 Christoph Hellwig 2017-10-17 505 error = -EINVAL;
abbf9e8a450748 Christoph Hellwig 2017-10-17 506 goto out_unlock_ilock;
abbf9e8a450748 Christoph Hellwig 2017-10-17 507 }
6898811459ff52 Dave Chinner 2013-08-12 508
abbf9e8a450748 Christoph Hellwig 2017-10-17 509 if (bmv->bmv_length == -1) {
abbf9e8a450748 Christoph Hellwig 2017-10-17 510 max_len = XFS_FSB_TO_BB(mp, XFS_B_TO_FSB(mp, max_len));
abbf9e8a450748 Christoph Hellwig 2017-10-17 511 bmv->bmv_length = max(0LL, max_len - bmv->bmv_offset);
6898811459ff52 Dave Chinner 2013-08-12 512 }
6898811459ff52 Dave Chinner 2013-08-12 513
abbf9e8a450748 Christoph Hellwig 2017-10-17 514 bmv_end = bmv->bmv_offset + bmv->bmv_length;
abbf9e8a450748 Christoph Hellwig 2017-10-17 515
abbf9e8a450748 Christoph Hellwig 2017-10-17 516 first_bno = bno = XFS_BB_TO_FSBT(mp, bmv->bmv_offset);
abbf9e8a450748 Christoph Hellwig 2017-10-17 517 len = XFS_BB_TO_FSB(mp, bmv->bmv_length);
abbf9e8a450748 Christoph Hellwig 2017-10-17 518
abbf9e8a450748 Christoph Hellwig 2017-10-17 519 error = xfs_iread_extents(NULL, ip, whichfork);
6898811459ff52 Dave Chinner 2013-08-12 520 if (error)
abbf9e8a450748 Christoph Hellwig 2017-10-17 521 goto out_unlock_ilock;
6898811459ff52 Dave Chinner 2013-08-12 522
b2b1712a640824 Christoph Hellwig 2017-11-03 523 if (!xfs_iext_lookup_extent(ip, ifp, bno, &icur, &got)) {
6898811459ff52 Dave Chinner 2013-08-12 524 /*
abbf9e8a450748 Christoph Hellwig 2017-10-17 525 * Report a whole-file hole if the delalloc flag is set to
abbf9e8a450748 Christoph Hellwig 2017-10-17 526 * stay compatible with the old implementation.
6898811459ff52 Dave Chinner 2013-08-12 527 */
abbf9e8a450748 Christoph Hellwig 2017-10-17 528 if (iflags & BMV_IF_DELALLOC)
abbf9e8a450748 Christoph Hellwig 2017-10-17 529 xfs_getbmap_report_hole(ip, bmv, out, bmv_end, bno,
abbf9e8a450748 Christoph Hellwig 2017-10-17 530 XFS_B_TO_FSB(mp, XFS_ISIZE(ip)));
abbf9e8a450748 Christoph Hellwig 2017-10-17 531 goto out_unlock_ilock;
6898811459ff52 Dave Chinner 2013-08-12 532 }
6898811459ff52 Dave Chinner 2013-08-12 533
abbf9e8a450748 Christoph Hellwig 2017-10-17 534 while (!xfs_getbmap_full(bmv)) {
abbf9e8a450748 Christoph Hellwig 2017-10-17 535 xfs_trim_extent(&got, first_bno, len);
6898811459ff52 Dave Chinner 2013-08-12 536
6898811459ff52 Dave Chinner 2013-08-12 537 /*
abbf9e8a450748 Christoph Hellwig 2017-10-17 538 * Report an entry for a hole if this extent doesn't directly
abbf9e8a450748 Christoph Hellwig 2017-10-17 539 * follow the previous one.
6898811459ff52 Dave Chinner 2013-08-12 540 */
abbf9e8a450748 Christoph Hellwig 2017-10-17 541 if (got.br_startoff > bno) {
abbf9e8a450748 Christoph Hellwig 2017-10-17 542 xfs_getbmap_report_hole(ip, bmv, out, bmv_end, bno,
abbf9e8a450748 Christoph Hellwig 2017-10-17 543 got.br_startoff);
abbf9e8a450748 Christoph Hellwig 2017-10-17 544 if (xfs_getbmap_full(bmv))
abbf9e8a450748 Christoph Hellwig 2017-10-17 545 break;
6898811459ff52 Dave Chinner 2013-08-12 546 }
6898811459ff52 Dave Chinner 2013-08-12 547
c364b6d0b6cda1 Darrick J. Wong 2017-01-26 548 /*
abbf9e8a450748 Christoph Hellwig 2017-10-17 549 * In order to report shared extents accurately, we report each
abbf9e8a450748 Christoph Hellwig 2017-10-17 550 * distinct shared / unshared part of a single bmbt record with
abbf9e8a450748 Christoph Hellwig 2017-10-17 551 * an individual getbmapx record.
c364b6d0b6cda1 Darrick J. Wong 2017-01-26 552 */
abbf9e8a450748 Christoph Hellwig 2017-10-17 553 bno = got.br_startoff + got.br_blockcount;
abbf9e8a450748 Christoph Hellwig 2017-10-17 554 rec = got;
abbf9e8a450748 Christoph Hellwig 2017-10-17 555 do {
abbf9e8a450748 Christoph Hellwig 2017-10-17 556 error = xfs_getbmap_report_one(ip, bmv, out, bmv_end,
abbf9e8a450748 Christoph Hellwig 2017-10-17 557 &rec);
abbf9e8a450748 Christoph Hellwig 2017-10-17 558 if (error || xfs_getbmap_full(bmv))
abbf9e8a450748 Christoph Hellwig 2017-10-17 559 goto out_unlock_ilock;
abbf9e8a450748 Christoph Hellwig 2017-10-17 560 } while (xfs_getbmap_next_rec(&rec, bno));
abbf9e8a450748 Christoph Hellwig 2017-10-17 561
b2b1712a640824 Christoph Hellwig 2017-11-03 562 if (!xfs_iext_next_extent(ifp, &icur, &got)) {
abbf9e8a450748 Christoph Hellwig 2017-10-17 563 xfs_fileoff_t end = XFS_B_TO_FSB(mp, XFS_ISIZE(ip));
abbf9e8a450748 Christoph Hellwig 2017-10-17 564
abbf9e8a450748 Christoph Hellwig 2017-10-17 565 out[bmv->bmv_entries - 1].bmv_oflags |= BMV_OF_LAST;
abbf9e8a450748 Christoph Hellwig 2017-10-17 566
abbf9e8a450748 Christoph Hellwig 2017-10-17 567 if (whichfork != XFS_ATTR_FORK && bno < end &&
abbf9e8a450748 Christoph Hellwig 2017-10-17 568 !xfs_getbmap_full(bmv)) {
abbf9e8a450748 Christoph Hellwig 2017-10-17 569 xfs_getbmap_report_hole(ip, bmv, out, bmv_end,
abbf9e8a450748 Christoph Hellwig 2017-10-17 570 bno, end);
c364b6d0b6cda1 Darrick J. Wong 2017-01-26 571 }
abbf9e8a450748 Christoph Hellwig 2017-10-17 572 break;
abbf9e8a450748 Christoph Hellwig 2017-10-17 573 }
abbf9e8a450748 Christoph Hellwig 2017-10-17 574
abbf9e8a450748 Christoph Hellwig 2017-10-17 575 if (bno >= first_bno + len)
abbf9e8a450748 Christoph Hellwig 2017-10-17 576 break;
6898811459ff52 Dave Chinner 2013-08-12 577 }
6898811459ff52 Dave Chinner 2013-08-12 578
6898811459ff52 Dave Chinner 2013-08-12 579 out_unlock_ilock:
01f4f3277556d4 Christoph Hellwig 2013-12-06 580 xfs_iunlock(ip, lock);
6898811459ff52 Dave Chinner 2013-08-12 @581 out_unlock_iolock:
6898811459ff52 Dave Chinner 2013-08-12 582 xfs_iunlock(ip, XFS_IOLOCK_SHARED);
6898811459ff52 Dave Chinner 2013-08-12 583 return error;
6898811459ff52 Dave Chinner 2013-08-12 584 }
6898811459ff52 Dave Chinner 2013-08-12 585
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
On 2023/3/27 23:15, Darrick J. Wong wrote:
> [add Christoph to cc since he added/last touched this assert, I think]
>
> On Mon, Mar 27, 2023 at 10:02:18PM +0800, Ye Bin wrote:
>> From: Ye Bin <[email protected]>
>>
>> There's issue as follows:
>> XFS: Assertion failed: (bmv->bmv_iflags & BMV_IF_DELALLOC) != 0, file: fs/xfs/xfs_bmap_util.c, line: 329
> Why not get rid of the assertion? It's not like it changes the course
> of the code flow -- userspace still gets told there's a delalloc extent.
Thank you for your reply.
I think it's incorrect to return the delalloc extent to the user in this
case. Because
users expect to obtain none delalloc extent information. If there is a
delalloc extent
found at this time, there is a problem with the functionality. I even
think that here
we should return an error to the userspace instead of return an
incorrect result to
the userspace .
> Or, if the assert does serve some purpose, then do we need to take
> the mmaplock for cow fork reporting too?
Let me analyze whether it is necessary to take the mmaplock for cow fork
reporting.
> --D
>
>> ------------[ cut here ]------------
>> kernel BUG at fs/xfs/xfs_message.c:102!
>> invalid opcode: 0000 [#1] PREEMPT SMP KASAN
>> CPU: 1 PID: 14612 Comm: xfs_io Not tainted 6.3.0-rc2-next-20230315-00006-g2729d23ddb3b-dirty #422
>> RIP: 0010:assfail+0x96/0xa0
>> RSP: 0018:ffffc9000fa178c0 EFLAGS: 00010246
>> RAX: 0000000000000000 RBX: 0000000000000001 RCX: ffff888179a18000
>> RDX: 0000000000000000 RSI: ffff888179a18000 RDI: 0000000000000002
>> RBP: 0000000000000000 R08: ffffffff8321aab6 R09: 0000000000000000
>> R10: 0000000000000001 R11: ffffed1105f85139 R12: ffffffff8aacc4c0
>> R13: 0000000000000149 R14: ffff888269f58000 R15: 000000000000000c
>> FS: 00007f42f27a4740(0000) GS:ffff88882fc00000(0000) knlGS:0000000000000000
>> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: 0000000000b92388 CR3: 000000024f006000 CR4: 00000000000006e0
>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>> Call Trace:
>> <TASK>
>> xfs_getbmap+0x1a5b/0x1e40
>> xfs_ioc_getbmap+0x1fd/0x5b0
>> xfs_file_ioctl+0x2cb/0x1d50
>> __x64_sys_ioctl+0x197/0x210
>> do_syscall_64+0x39/0xb0
>> entry_SYSCALL_64_after_hwframe+0x63/0xcd
>>
>> Above issue may happen as follows:
>> ThreadA ThreadB
>> do_shared_fault
>> __do_fault
>> xfs_filemap_fault
>> __xfs_filemap_fault
>> filemap_fault
>> xfs_ioc_getbmap -> Without BMV_IF_DELALLOC flag
>> xfs_getbmap
>> xfs_ilock(ip, XFS_IOLOCK_SHARED);
>> filemap_write_and_wait
>> do_page_mkwrite
>> xfs_filemap_page_mkwrite
>> __xfs_filemap_fault
>> xfs_ilock(XFS_I(inode), XFS_MMAPLOCK_SHARED);
>> iomap_page_mkwrite
>> ...
>> xfs_buffered_write_iomap_begin
>> xfs_bmapi_reserve_delalloc -> Allocate delay extent
>> xfs_ilock_data_map_shared(ip)
>> xfs_getbmap_report_one
>> ASSERT((bmv->bmv_iflags & BMV_IF_DELALLOC) != 0)
>> -> trigger BUG_ON
>>
>> As xfs_filemap_page_mkwrite() only hold XFS_MMAPLOCK_SHARED lock, there's
>> small window mkwrite can produce delay extent after file write in xfs_getbmap().
>> To solve above issue, hold XFS_MMAPLOCK_EXCL lock when do xfs_getbmap(),
>> to prevent write operations by do_page_mkwrite().
>> During doing __xfs_filemap_fault() we can't hold IOLOCK lock, as it's may lead
>> to ABBA dealock with xfs_file_write_iter().It's very easy to reproduce when
>> do fsstress, lockdep will detect deadlock.
>>
>> Signed-off-by: Ye Bin <[email protected]>
>> ---
>> fs/xfs/xfs_bmap_util.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
>> index a09dd2606479..f23771a0cc8d 100644
>> --- a/fs/xfs/xfs_bmap_util.c
>> +++ b/fs/xfs/xfs_bmap_util.c
>> @@ -463,11 +463,13 @@ xfs_getbmap(
>> max_len = XFS_ISIZE(ip);
>> break;
>> case XFS_DATA_FORK:
>> + lock = XFS_MMAPLOCK_EXCL;
>> + xfs_ilock(ip, lock);
>> if (!(iflags & BMV_IF_DELALLOC) &&
>> (ip->i_delayed_blks || XFS_ISIZE(ip) > ip->i_disk_size)) {
>> error = filemap_write_and_wait(VFS_I(ip)->i_mapping);
>> if (error)
>> - goto out_unlock_iolock;
>> + goto out_unlock_ilock;
>>
>> /*
>> * Even after flushing the inode, there can still be
>> @@ -486,7 +488,7 @@ xfs_getbmap(
>> else
>> max_len = XFS_ISIZE(ip);
>>
>> - lock = xfs_ilock_data_map_shared(ip);
>> + lock |= xfs_ilock_data_map_shared(ip);
>> break;
>> }
>>
>> --
>> 2.31.1
>>
> .
>
On Tue, Mar 28, 2023 at 09:33:58AM +0800, yebin (H) wrote:
>
>
> On 2023/3/27 23:15, Darrick J. Wong wrote:
> > [add Christoph to cc since he added/last touched this assert, I think]
> >
> > On Mon, Mar 27, 2023 at 10:02:18PM +0800, Ye Bin wrote:
> > > From: Ye Bin <[email protected]>
> > >
> > > There's issue as follows:
> > > XFS: Assertion failed: (bmv->bmv_iflags & BMV_IF_DELALLOC) != 0, file: fs/xfs/xfs_bmap_util.c, line: 329
> > Why not get rid of the assertion? It's not like it changes the course
> > of the code flow -- userspace still gets told there's a delalloc extent.
> Thank you for your reply.
> I think it's incorrect to return the delalloc extent to the user in this
> case. Because
> users expect to obtain none delalloc extent information. If there is a
> delalloc extent
> found at this time, there is a problem with the functionality. I even think
> that here
> we should return an error to the userspace instead of return an incorrect
> result to
> the userspace .
<shrug> Seeing as the data fork mappings can change the instant the
ILOCK drops, I'm not /that/ worried about users seeing a delalloc
mapping even if the user requested a flush. The results are already
obsolete when they get to userspace, unless the application software has
found another means to lock out access to the file.
And even then, BMAP doesn't consult the page cache, so it still doesn't
provide a full picture of where all the nonzero data can be found.
--D
> > Or, if the assert does serve some purpose, then do we need to take
> > the mmaplock for cow fork reporting too?
> Let me analyze whether it is necessary to take the mmaplock for cow fork
> reporting.
> > --D
> >
> > > ------------[ cut here ]------------
> > > kernel BUG at fs/xfs/xfs_message.c:102!
> > > invalid opcode: 0000 [#1] PREEMPT SMP KASAN
> > > CPU: 1 PID: 14612 Comm: xfs_io Not tainted 6.3.0-rc2-next-20230315-00006-g2729d23ddb3b-dirty #422
> > > RIP: 0010:assfail+0x96/0xa0
> > > RSP: 0018:ffffc9000fa178c0 EFLAGS: 00010246
> > > RAX: 0000000000000000 RBX: 0000000000000001 RCX: ffff888179a18000
> > > RDX: 0000000000000000 RSI: ffff888179a18000 RDI: 0000000000000002
> > > RBP: 0000000000000000 R08: ffffffff8321aab6 R09: 0000000000000000
> > > R10: 0000000000000001 R11: ffffed1105f85139 R12: ffffffff8aacc4c0
> > > R13: 0000000000000149 R14: ffff888269f58000 R15: 000000000000000c
> > > FS: 00007f42f27a4740(0000) GS:ffff88882fc00000(0000) knlGS:0000000000000000
> > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > CR2: 0000000000b92388 CR3: 000000024f006000 CR4: 00000000000006e0
> > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > > Call Trace:
> > > <TASK>
> > > xfs_getbmap+0x1a5b/0x1e40
> > > xfs_ioc_getbmap+0x1fd/0x5b0
> > > xfs_file_ioctl+0x2cb/0x1d50
> > > __x64_sys_ioctl+0x197/0x210
> > > do_syscall_64+0x39/0xb0
> > > entry_SYSCALL_64_after_hwframe+0x63/0xcd
> > >
> > > Above issue may happen as follows:
> > > ThreadA ThreadB
> > > do_shared_fault
> > > __do_fault
> > > xfs_filemap_fault
> > > __xfs_filemap_fault
> > > filemap_fault
> > > xfs_ioc_getbmap -> Without BMV_IF_DELALLOC flag
> > > xfs_getbmap
> > > xfs_ilock(ip, XFS_IOLOCK_SHARED);
> > > filemap_write_and_wait
> > > do_page_mkwrite
> > > xfs_filemap_page_mkwrite
> > > __xfs_filemap_fault
> > > xfs_ilock(XFS_I(inode), XFS_MMAPLOCK_SHARED);
> > > iomap_page_mkwrite
> > > ...
> > > xfs_buffered_write_iomap_begin
> > > xfs_bmapi_reserve_delalloc -> Allocate delay extent
> > > xfs_ilock_data_map_shared(ip)
> > > xfs_getbmap_report_one
> > > ASSERT((bmv->bmv_iflags & BMV_IF_DELALLOC) != 0)
> > > -> trigger BUG_ON
> > >
> > > As xfs_filemap_page_mkwrite() only hold XFS_MMAPLOCK_SHARED lock, there's
> > > small window mkwrite can produce delay extent after file write in xfs_getbmap().
> > > To solve above issue, hold XFS_MMAPLOCK_EXCL lock when do xfs_getbmap(),
> > > to prevent write operations by do_page_mkwrite().
> > > During doing __xfs_filemap_fault() we can't hold IOLOCK lock, as it's may lead
> > > to ABBA dealock with xfs_file_write_iter().It's very easy to reproduce when
> > > do fsstress, lockdep will detect deadlock.
> > >
> > > Signed-off-by: Ye Bin <[email protected]>
> > > ---
> > > fs/xfs/xfs_bmap_util.c | 6 ++++--
> > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> > > index a09dd2606479..f23771a0cc8d 100644
> > > --- a/fs/xfs/xfs_bmap_util.c
> > > +++ b/fs/xfs/xfs_bmap_util.c
> > > @@ -463,11 +463,13 @@ xfs_getbmap(
> > > max_len = XFS_ISIZE(ip);
> > > break;
> > > case XFS_DATA_FORK:
> > > + lock = XFS_MMAPLOCK_EXCL;
> > > + xfs_ilock(ip, lock);
> > > if (!(iflags & BMV_IF_DELALLOC) &&
> > > (ip->i_delayed_blks || XFS_ISIZE(ip) > ip->i_disk_size)) {
> > > error = filemap_write_and_wait(VFS_I(ip)->i_mapping);
> > > if (error)
> > > - goto out_unlock_iolock;
> > > + goto out_unlock_ilock;
> > > /*
> > > * Even after flushing the inode, there can still be
> > > @@ -486,7 +488,7 @@ xfs_getbmap(
> > > else
> > > max_len = XFS_ISIZE(ip);
> > > - lock = xfs_ilock_data_map_shared(ip);
> > > + lock |= xfs_ilock_data_map_shared(ip);
> > > break;
> > > }
> > > --
> > > 2.31.1
> > >
> > .
> >
>
On Mon, Mar 27, 2023 at 08:15:24AM -0700, Darrick J. Wong wrote:
> [add Christoph to cc since he added/last touched this assert, I think]
I only really moved the ASSERT as part of an interface rewrite. I'll
try to throw in my 2 cents anyway..
On Mon, Mar 27, 2023 at 08:15:24AM -0700, Darrick J. Wong wrote:
> > There's issue as follows:
> > XFS: Assertion failed: (bmv->bmv_iflags & BMV_IF_DELALLOC) != 0, file: fs/xfs/xfs_bmap_util.c, line: 329
>
> Why not get rid of the assertion? It's not like it changes the course
> of the code flow -- userspace still gets told there's a delalloc extent.
>
> Or, if the assert does serve some purpose, then do we need to take
> the mmaplock for cow fork reporting too?
Looking at the COW fork reporting I think it's actually even more
broken as it never tried to flush data to start with. But COW
report is a DEBUG only feature, so maybe forcing or requiring
BMV_IF_DELALLOC there would make a whole lot of sense.
On Mon, Mar 27, 2023 at 06:43:28PM -0700, Darrick J. Wong wrote:
> <shrug> Seeing as the data fork mappings can change the instant the
> ILOCK drops, I'm not /that/ worried about users seeing a delalloc
> mapping even if the user requested a flush. The results are already
> obsolete when they get to userspace, unless the application software has
> found another means to lock out access to the file.
That is true, but then again the users asked to not see delalloc
mappings, so we really shouldn't report one, right?
On Mon, Mar 27, 2023 at 06:47:54PM -0700, Christoph Hellwig wrote:
> On Mon, Mar 27, 2023 at 06:43:28PM -0700, Darrick J. Wong wrote:
> > <shrug> Seeing as the data fork mappings can change the instant the
> > ILOCK drops, I'm not /that/ worried about users seeing a delalloc
> > mapping even if the user requested a flush. The results are already
> > obsolete when they get to userspace, unless the application software has
> > found another means to lock out access to the file.
>
> That is true, but then again the users asked to not see delalloc
> mappings, so we really shouldn't report one, right?
Yeah, I suppose so. I wonder how many programs there are out there that
don't pass in BMV_IF_DELALLOC /and/ can't handle that? But I suppose
taking MMAP_EXCL is good enough to shut up the obvious assertion vector.
The COW implementation probably ought to be doing the flush too.
--D
On Mon, Mar 27, 2023 at 07:03:41PM -0700, Darrick J. Wong wrote:
> On Mon, Mar 27, 2023 at 06:47:54PM -0700, Christoph Hellwig wrote:
> > On Mon, Mar 27, 2023 at 06:43:28PM -0700, Darrick J. Wong wrote:
> > > <shrug> Seeing as the data fork mappings can change the instant the
> > > ILOCK drops, I'm not /that/ worried about users seeing a delalloc
> > > mapping even if the user requested a flush. The results are already
> > > obsolete when they get to userspace, unless the application software has
> > > found another means to lock out access to the file.
> >
> > That is true, but then again the users asked to not see delalloc
> > mappings, so we really shouldn't report one, right?
>
> Yeah, I suppose so. I wonder how many programs there are out there that
> don't pass in BMV_IF_DELALLOC /and/ can't handle that? But I suppose
> taking MMAP_EXCL is good enough to shut up the obvious assertion vector.
Why not just skip it? Take the flush completion as being a
point-in-time snapshot where there are no delalloc extents, and if
any new ones have been created racily, just skip them as being
"after" the flush and so don't get reported...
> The COW implementation probably ought to be doing the flush too.
Yup, and then just skip any delalloc extents found after that, too.
Cheers,
Dave.
--
Dave Chinner
[email protected]