[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git-commits-head
Subject:    xfs: bunmapi has unnecessary AG lock ordering issues
From:       Linux Kernel Mailing List <linux-kernel () vger ! kernel ! org>
Date:       2021-05-30 4:30:27
Message-ID: git-mailbomb-linux-master-0fe0bbe00a6fb77adf75085b7d06b71a830dd6f2 () kernel ! org
[Download RAW message or body]

Commit:     0fe0bbe00a6fb77adf75085b7d06b71a830dd6f2
Parent:     991c2c5980fb97ae6194f7c46b44f9446629eb4e
Refname:    refs/heads/master
Web:        https://git.kernel.org/torvalds/c/0fe0bbe00a6fb77adf75085b7d06b71a830dd6f2
Author:     Dave Chinner <dchinner@redhat.com>
AuthorDate: Thu May 27 08:11:01 2021 -0700
Committer:  Darrick J. Wong <djwong@kernel.org>
CommitDate: Thu May 27 08:11:24 2021 -0700

    xfs: bunmapi has unnecessary AG lock ordering issues
    
    large directory block size operations are assert failing because
    xfs_bunmapi() is not completely removing fragmented directory blocks
    like so:
    
    XFS: Assertion failed: done, file: fs/xfs/libxfs/xfs_dir2.c, line: 677
    ....
    Call Trace:
     xfs_dir2_shrink_inode+0x1a8/0x210
     xfs_dir2_block_to_sf+0x2ae/0x410
     xfs_dir2_block_removename+0x21a/0x280
     xfs_dir_removename+0x195/0x1d0
     xfs_rename+0xb79/0xc50
     ? avc_has_perm+0x8d/0x1a0
     ? avc_has_perm_noaudit+0x9a/0x120
     xfs_vn_rename+0xdb/0x150
     vfs_rename+0x719/0xb50
     ? __lookup_hash+0x6a/0xa0
     do_renameat2+0x413/0x5e0
     __x64_sys_rename+0x45/0x50
     do_syscall_64+0x3a/0x70
     entry_SYSCALL_64_after_hwframe+0x44/0xae
    
    We are aborting the bunmapi() pass because of this specific chunk of
    code:
    
                    /*
                     * Make sure we don't touch multiple AGF headers out of order
                     * in a single transaction, as that could cause AB-BA deadlocks.
                     */
                    if (!wasdel && !isrt) {
                            agno = XFS_FSB_TO_AGNO(mp, del.br_startblock);
                            if (prev_agno != NULLAGNUMBER && prev_agno > agno)
                                    break;
                            prev_agno = agno;
                    }
    
    This is designed to prevent deadlocks in AGF locking when freeing
    multiple extents by ensuring that we only ever lock in increasing
    AG number order. Unfortunately, this also violates the "bunmapi will
    always succeed" semantic that some high level callers depend on,
    such as xfs_dir2_shrink_inode(), xfs_da_shrink_inode() and
    xfs_inactive_symlink_rmt().
    
    This AG lock ordering was introduced back in 2017 to fix deadlocks
    triggered by generic/299 as reported here:
    
    https://lore.kernel.org/linux-xfs/800468eb-3ded-9166-20a4-047de8018582@gmail.com/
    
    This codebase is old enough that it was before we were defering all
    AG based extent freeing from within xfs_bunmapi(). THat is, we never
    actually lock AGs in xfs_bunmapi() any more - every non-rt based
    extent free is added to the defer ops list, as is all BMBT block
    freeing. And RT extents are not RT based, so there's no lock
    ordering issues associated with them.
    
    Hence this AGF lock ordering code is both broken and dead. Let's
    just remove it so that the large directory block code works reliably
    again.
    
    Tested against xfs/538 and generic/299 which is the original test
    that exposed the deadlocks that this code fixed.
    
    Fixes: 5b094d6dac04 ("xfs: fix multi-AG deadlock in xfs_bunmapi")
    Signed-off-by: Dave Chinner <dchinner@redhat.com>
    Reviewed-by: Darrick J. Wong <djwong@kernel.org>
    Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_bmap.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 3f8b6da092611..a3e0e6f672d63 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -5349,7 +5349,6 @@ __xfs_bunmapi(
 	xfs_fsblock_t		sum;
 	xfs_filblks_t		len = *rlen;	/* length to unmap in file */
 	xfs_fileoff_t		max_len;
-	xfs_agnumber_t		prev_agno = NULLAGNUMBER, agno;
 	xfs_fileoff_t		end;
 	struct xfs_iext_cursor	icur;
 	bool			done = false;
@@ -5441,16 +5440,6 @@ __xfs_bunmapi(
 		del = got;
 		wasdel = isnullstartblock(del.br_startblock);
 
-		/*
-		 * Make sure we don't touch multiple AGF headers out of order
-		 * in a single transaction, as that could cause AB-BA deadlocks.
-		 */
-		if (!wasdel && !isrt) {
-			agno = XFS_FSB_TO_AGNO(mp, del.br_startblock);
-			if (prev_agno != NULLAGNUMBER && prev_agno > agno)
-				break;
-			prev_agno = agno;
-		}
 		if (got.br_startoff < start) {
 			del.br_startoff = start;
 			del.br_blockcount -= start - got.br_startoff;
[prev in list] [next in list] [prev in thread] [next in thread]