diff mbox series

xfs: fix BUG_ON in xfs_getbmap()

Message ID	20230327140218.4154709-1-yebin@huaweicloud.com
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; From: Ye Bin <yebin@huaweicloud.com> To: djwong@kernel.org, linux-xfs@vger.kernel.org Cc: linux-kernel@vger.kernel.org, Ye Bin <yebin10@huawei.com> Subject: [PATCH] xfs: fix BUG_ON in xfs_getbmap() Date: Mon, 27 Mar 2023 22:02:18 +0800 Message-Id: <20230327140218.4154709-1-yebin@huaweicloud.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	xfs: fix BUG_ON in xfs_getbmap() \| xfs: fix BUG_ON in xfs_getbmap()

Commit Message

Ye Bin March 27, 2023, 2:02 p.m. UTC

  From: Ye Bin <yebin10@huawei.com>

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 <yebin10@huawei.com>
---
 fs/xfs/xfs_bmap_util.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Darrick J. Wong March 27, 2023, 3:15 p.m. UTC | #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 <yebin10@huawei.com>
> 
> 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 <yebin10@huawei.com>
> ---
>  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
>

kernel test robot March 27, 2023, 5:30 p.m. UTC | #2

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/202303280146.XMC4bc3D-lkp@intel.com/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 <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303280146.XMC4bc3D-lkp@intel.com/

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

kernel test robot March 27, 2023, 8:14 p.m. UTC | #3

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/202303280351.JOvlN1HQ-lkp@intel.com/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 <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303280351.JOvlN1HQ-lkp@intel.com/

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

Ye Bin March 28, 2023, 1:33 a.m. UTC | #4

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 <yebin10@huawei.com>
>>
>> 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 <yebin10@huawei.com>
>> ---
>>   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
>>
> .
>

Darrick J. Wong March 28, 2023, 1:43 a.m. UTC | #5

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 <yebin10@huawei.com>
> > > 
> > > 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 <yebin10@huawei.com>
> > > ---
> > >   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
> > > 
> > .
> > 
>

Christoph Hellwig March 28, 2023, 1:44 a.m. UTC | #6

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..

Christoph Hellwig March 28, 2023, 1:47 a.m. UTC | #7

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?

Christoph Hellwig March 28, 2023, 1:49 a.m. UTC | #8

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.

Darrick J. Wong March 28, 2023, 2:03 a.m. UTC | #9

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

Dave Chinner March 28, 2023, 2:58 a.m. UTC | #10

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.

diff mbox series

Patch

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;
 	}