Re: [PATCH -next] ext4: Fix warning in ext4_da_release_space

yebin <yebin10@xxxxxxxxxx> · Wed, 15 Jun 2022 12:03:41 +0800

On 2022/6/15 8:53, Eric Whitney wrote:
* yebin <yebin10@xxxxxxxxxx>:
ping...

On 2022/5/20 10:55, Ye Bin wrote:
We got issue as follows:
WARNING: CPU: 2 PID: 1936 at fs/ext4/inode.c:1511 ext4_da_release_space+0x1b9/0x266
Modules linked in:
CPU: 2 PID: 1936 Comm: dd Not tainted 5.10.0+ #344
RIP: 0010:ext4_da_release_space+0x1b9/0x266
RSP: 0018:ffff888127307848 EFLAGS: 00010292
RAX: 0000000000000000 RBX: 0000000000000001 RCX: ffffffff843f67cc
RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffed1024e60ed9
RBP: ffff888124dc8140 R08: 0000000000000083 R09: ffffed1075da6d23
R10: ffff8883aed36917 R11: ffffed1075da6d22 R12: ffff888124dc83f0
R13: ffff888124dc844c R14: ffff888124dc8168 R15: 000000000000000c
FS:  00007f6b7247d740(0000) GS:ffff8883aed00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007ffc1a0b7dd8 CR3: 00000001065ce000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
   ext4_es_remove_extent+0x187/0x230
   mpage_release_unused_pages+0x3af/0x470
   ext4_writepages+0xb9b/0x1160
   do_writepages+0xbb/0x1e0
   __filemap_fdatawrite_range+0x1b1/0x1f0
   file_write_and_wait_range+0x80/0xe0
   ext4_sync_file+0x13d/0x800
   vfs_fsync_range+0x75/0x140
   do_fsync+0x4d/0x90
   __x64_sys_fsync+0x1d/0x30
   do_syscall_64+0x33/0x40
   entry_SYSCALL_64_after_hwframe+0x44/0xa9

Above issue may happens as follows:
	process1                        process2
ext4_da_write_begin
    ext4_da_reserve_space
      ext4_es_insert_delayed_block[1/1]
                                      ext4_da_write_begin
				      ext4_es_insert_delayed_block[0/1]
ext4_writepages
    ****Delayed block allocation failed****
    mpage_release_unused_pages
      ext4_es_remove_extent[1/1]
        ext4_da_release_space [reserved 0]

ext4_da_write_begin
    ext4_es_scan_clu(inode, &ext4_es_is_delonly, lblk)
     ->As there exist [0, 1] extent, so will return true
                                     ext4_writepages
				   ****Delayed block allocation failed****
                                       mpage_release_unused_pages
				       ext4_es_remove_extent[0/1]
				         ext4_da_release_space [reserved 1]
					   ei->i_reserved_data_blocks [1->0]

    ext4_es_insert_delayed_block[1/1]

ext4_writepages
    ****Delayed block allocation failed****
    mpage_release_unused_pages
    ext4_es_remove_extent[1/1]
     ext4_da_release_space [reserved 1]
      ei->i_reserved_data_blocks[0, -1]
      ->As ei->i_reserved_data_blocks already is zero but to_free is 1,
      will trigger warning.

To solve above issue, introduce i_clu_lock to protect insert delayed
block and remove block under cluster delay allocate mode.

Signed-off-by: Ye Bin <yebin10@xxxxxxxxxx>
---
   fs/ext4/ext4.h           |  3 +++
   fs/ext4/extents_status.c |  5 +++++
   fs/ext4/inode.c          | 11 +++++++++--
   fs/ext4/super.c          |  1 +
   4 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index bcd3b9bf8069..47c88ac4d4a8 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1169,6 +1169,9 @@ struct ext4_inode_info {
   	__u32 i_csum_seed;
   	kprojid_t i_projid;
+
+	/* Protect concurrent add cluster delayed block and remove block */
+	struct mutex i_clu_lock;
   };
   /*
diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
index 9a3a8996aacf..dd679014db98 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -1433,6 +1433,7 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
   int ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
   			  ext4_lblk_t len)
   {
+	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
   	ext4_lblk_t end;
   	int err = 0;
   	int reserved = 0;
@@ -1455,9 +1456,13 @@ int ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
   	 * so that we are sure __es_shrink() is done with the inode before it
   	 * is reclaimed.
   	 */
+	if (sbi->s_cluster_ratio != 1)
+		mutex_lock(&EXT4_I(inode)->i_clu_lock);
   	write_lock(&EXT4_I(inode)->i_es_lock);
   	err = __es_remove_extent(inode, lblk, end, &reserved);
   	write_unlock(&EXT4_I(inode)->i_es_lock);
+	if (sbi->s_cluster_ratio != 1)
+		mutex_unlock(&EXT4_I(inode)->i_clu_lock);
   	ext4_es_print_tree(inode);
   	ext4_da_release_space(inode, reserved);
   	return err;
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 01c9e4f743ba..1109d77ad60b 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1649,17 +1649,22 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
   			goto errout;
   		reserved = true;
   	} else {   /* bigalloc */
+		mutex_lock(&EXT4_I(inode)->i_clu_lock);
   		if (!ext4_es_scan_clu(inode, &ext4_es_is_delonly, lblk)) {
   			if (!ext4_es_scan_clu(inode,
   					      &ext4_es_is_mapped, lblk)) {
   				ret = ext4_clu_mapped(inode,
   						      EXT4_B2C(sbi, lblk));
-				if (ret < 0)
+				if (ret < 0) {
+					mutex_unlock(&EXT4_I(inode)->i_clu_lock);
   					goto errout;
+				}
   				if (ret == 0) {
   					ret = ext4_da_reserve_space(inode);
-					if (ret != 0)   /* ENOSPC */
+					if (ret != 0) {   /* ENOSPC */
+						mutex_unlock(&EXT4_I(inode)->i_clu_lock);
   						goto errout;
+					}
   					reserved = true;
   				} else {
   					allocated = true;
@@ -1671,6 +1676,8 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
   	}
   	ret = ext4_es_insert_delayed_block(inode, lblk, allocated);
+	if (sbi->s_cluster_ratio != 1)
+		mutex_unlock(&EXT4_I(inode)->i_clu_lock);
   	if (ret && reserved)
   		ext4_da_release_space(inode, 1);
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index c5021ca0a28a..aa6f2a68bf41 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1347,6 +1347,7 @@ static struct inode *ext4_alloc_inode(struct super_block *sb)
   	INIT_WORK(&ei->i_rsv_conversion_work, ext4_end_io_rsv_work);
   	ext4_fc_init_inode(&ei->vfs_inode);
   	mutex_init(&ei->i_fc_lock);
+	mutex_init(&ei->i_clu_lock);
   	return &ei->vfs_inode;
   }
As you have reported, it looks like there is potential for a race between
mpage_release_unused_pages and ext4_insert_delayed_block when running on a
bigalloc file system in rare circumstances.  There should have been locking
in mpage_release_unused_pages to avoid this.  The locking was missed when new
code was added to ext4_insert_delayed_block to handle bigalloc delayed block
accounting.

I have a solution that addresses that omission and is simpler than your
proposed fix.  It does not require adding a new lock.  I'm putting that
patch through the usual testing cycles, plus new stress testing to verify that
no lock dependency problems exist.  It looks good so far, and I expect to post
it tomorrow.  If you would be willing to test that patch, we would welcome
it.  As you have not chosen to explain how you've triggered the race, you
are in a better position to determine whether the patch will successfully
address it.
I've provided a way to reproduce.
It's important to understand that the delayed block allocation failures
you have noted in your call graphs are far more important events than the
block accounting errors that occur due to the race.  The delayed block
allocation failures represent data loss for the user, and they should really
never happen.  That's why we'd like to understand how you triggered the race,
whether by fuzzing or some other method.  Depending on what was done, there
may be a need to harden the code.

Eric
.








