* [PATCH v3 1/3] ext4: fix incorrect calculate 'reserved' in '__es_remove_extent' when enable bigalloc feature
2022-12-03 2:59 [PATCH v3 0/3] Fix two issues about bigalloc feature Ye Bin
@ 2022-12-03 2:59 ` Ye Bin
2022-12-07 23:01 ` Eric Whitney
2022-12-03 2:59 ` [PATCH v3 2/3] ext4: record error when detect abnormal 'i_reserved_data_blocks' Ye Bin
2022-12-03 2:59 ` [PATCH v3 3/3] ext4: add check pending tree when evict inode Ye Bin
2 siblings, 1 reply; 9+ messages in thread
From: Ye Bin @ 2022-12-03 2:59 UTC (permalink / raw)
To: tytso, adilger.kernel, linux-ext4
Cc: linux-kernel, jack, Ye Bin, syzbot+05a0f0ccab4a25626e38
From: Ye Bin <yebin10@huawei.com>
Syzbot report issue as follows:
EXT4-fs error (device loop0): ext4_validate_block_bitmap:398: comm rep: bg 0: block 5: invalid block bitmap
EXT4-fs (loop0): Delayed block allocation failed for inode 18 at logical offset 0 with max blocks 32 with error 28
EXT4-fs (loop0): This should not happen!! Data will be lost
EXT4-fs (loop0): Total free blocks count 0
EXT4-fs (loop0): Free/Dirty block details
EXT4-fs (loop0): free_blocks=0
EXT4-fs (loop0): dirty_blocks=32
EXT4-fs (loop0): Block reservation details
EXT4-fs (loop0): i_reserved_data_blocks=2
EXT4-fs (loop0): Inode 18 (00000000845cd634): i_reserved_data_blocks (1) not cleared!
Above issue happens as follows:
Assume:
sbi->s_cluster_ratio = 16
Step1: Insert delay block [0, 31] -> ei->i_reserved_data_blocks=2
Step2:
ext4_writepages
mpage_map_and_submit_extent -> return failed
mpage_release_unused_pages -> to release [0, 30]
ext4_es_remove_extent -> remove lblk=0 end=30
__es_remove_extent -> len1=0 len2=31-30=1
__es_remove_extent:
...
if (len2 > 0) {
...
if (len1 > 0) {
...
} else {
es->es_lblk = end + 1;
es->es_len = len2;
...
}
if (count_reserved)
count_rsvd(inode, lblk, orig_es.es_len - len1 - len2, &orig_es, &rc);
goto out; -> will return but didn't calculate 'reserved'
...
Step3: ext4_destroy_inode -> trigger "i_reserved_data_blocks (1) not cleared!"
To solve above issue if 'len2>0' call 'get_rsvd()' before goto out.
Reported-by: syzbot+05a0f0ccab4a25626e38@syzkaller.appspotmail.com
Fixes: 8fcc3a580651 ("ext4: rework reserved cluster accounting when invalidating pages")
Signed-off-by: Ye Bin <yebin10@huawei.com>
---
fs/ext4/extents_status.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
index cd0a861853e3..7ada374ff27d 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -1371,7 +1371,7 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
if (count_reserved)
count_rsvd(inode, lblk, orig_es.es_len - len1 - len2,
&orig_es, &rc);
- goto out;
+ goto out_get_reserved;
}
if (len1 > 0) {
@@ -1413,6 +1413,7 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
}
}
+out_get_reserved:
if (count_reserved)
*reserved = get_rsvd(inode, end, es, &rc);
out:
--
2.31.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/3] ext4: fix incorrect calculate 'reserved' in '__es_remove_extent' when enable bigalloc feature
2022-12-03 2:59 ` [PATCH v3 1/3] ext4: fix incorrect calculate 'reserved' in '__es_remove_extent' when enable " Ye Bin
@ 2022-12-07 23:01 ` Eric Whitney
2022-12-08 22:53 ` Eric Whitney
0 siblings, 1 reply; 9+ messages in thread
From: Eric Whitney @ 2022-12-07 23:01 UTC (permalink / raw)
To: Ye Bin
Cc: tytso, adilger.kernel, linux-ext4, linux-kernel, jack, Ye Bin,
syzbot+05a0f0ccab4a25626e38
* Ye Bin <yebin@huaweicloud.com>:
> From: Ye Bin <yebin10@huawei.com>
>
> Syzbot report issue as follows:
> EXT4-fs error (device loop0): ext4_validate_block_bitmap:398: comm rep: bg 0: block 5: invalid block bitmap
> EXT4-fs (loop0): Delayed block allocation failed for inode 18 at logical offset 0 with max blocks 32 with error 28
> EXT4-fs (loop0): This should not happen!! Data will be lost
>
> EXT4-fs (loop0): Total free blocks count 0
> EXT4-fs (loop0): Free/Dirty block details
> EXT4-fs (loop0): free_blocks=0
> EXT4-fs (loop0): dirty_blocks=32
> EXT4-fs (loop0): Block reservation details
> EXT4-fs (loop0): i_reserved_data_blocks=2
> EXT4-fs (loop0): Inode 18 (00000000845cd634): i_reserved_data_blocks (1) not cleared!
>
> Above issue happens as follows:
> Assume:
> sbi->s_cluster_ratio = 16
> Step1: Insert delay block [0, 31] -> ei->i_reserved_data_blocks=2
> Step2:
> ext4_writepages
> mpage_map_and_submit_extent -> return failed
> mpage_release_unused_pages -> to release [0, 30]
> ext4_es_remove_extent -> remove lblk=0 end=30
> __es_remove_extent -> len1=0 len2=31-30=1
> __es_remove_extent:
> ...
> if (len2 > 0) {
> ...
> if (len1 > 0) {
> ...
> } else {
> es->es_lblk = end + 1;
> es->es_len = len2;
> ...
> }
> if (count_reserved)
> count_rsvd(inode, lblk, orig_es.es_len - len1 - len2, &orig_es, &rc);
> goto out; -> will return but didn't calculate 'reserved'
> ...
> Step3: ext4_destroy_inode -> trigger "i_reserved_data_blocks (1) not cleared!"
>
> To solve above issue if 'len2>0' call 'get_rsvd()' before goto out.
>
> Reported-by: syzbot+05a0f0ccab4a25626e38@syzkaller.appspotmail.com
> Fixes: 8fcc3a580651 ("ext4: rework reserved cluster accounting when invalidating pages")
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> ---
> fs/ext4/extents_status.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> index cd0a861853e3..7ada374ff27d 100644
> --- a/fs/ext4/extents_status.c
> +++ b/fs/ext4/extents_status.c
> @@ -1371,7 +1371,7 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
> if (count_reserved)
> count_rsvd(inode, lblk, orig_es.es_len - len1 - len2,
> &orig_es, &rc);
> - goto out;
> + goto out_get_reserved;
> }
>
> if (len1 > 0) {
> @@ -1413,6 +1413,7 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
> }
> }
>
> +out_get_reserved:
> if (count_reserved)
> *reserved = get_rsvd(inode, end, es, &rc);
> out:
The length of some lines in the commit description - probably those which are
log output - is resulting in a checkpatch warning. It generally prefers lines
to be a maximum of 75 characters (and Ted usually likes them limited to 72
characters. See my comment to patch #3. I'm not sure what Ted would want here,
though I'd probably break them at 72 characters or less.
Otherwise, the patch looks good. Feel free to add:
Reviewed-by: Eric Whitney <enwlinux@gmail.com>
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/3] ext4: fix incorrect calculate 'reserved' in '__es_remove_extent' when enable bigalloc feature
2022-12-07 23:01 ` Eric Whitney
@ 2022-12-08 22:53 ` Eric Whitney
2022-12-08 22:54 ` Eric Whitney
0 siblings, 1 reply; 9+ messages in thread
From: Eric Whitney @ 2022-12-08 22:53 UTC (permalink / raw)
To: Eric Whitney
Cc: Ye Bin, tytso, adilger.kernel, linux-ext4, linux-kernel, jack,
Ye Bin, syzbot+05a0f0ccab4a25626e38
* Eric Whitney <enwlinux@gmail.com>:
> * Ye Bin <yebin@huaweicloud.com>:
> > From: Ye Bin <yebin10@huawei.com>
> >
> > Syzbot report issue as follows:
> > EXT4-fs error (device loop0): ext4_validate_block_bitmap:398: comm rep: bg 0: block 5: invalid block bitmap
> > EXT4-fs (loop0): Delayed block allocation failed for inode 18 at logical offset 0 with max blocks 32 with error 28
> > EXT4-fs (loop0): This should not happen!! Data will be lost
> >
> > EXT4-fs (loop0): Total free blocks count 0
> > EXT4-fs (loop0): Free/Dirty block details
> > EXT4-fs (loop0): free_blocks=0
> > EXT4-fs (loop0): dirty_blocks=32
> > EXT4-fs (loop0): Block reservation details
> > EXT4-fs (loop0): i_reserved_data_blocks=2
> > EXT4-fs (loop0): Inode 18 (00000000845cd634): i_reserved_data_blocks (1) not cleared!
> >
> > Above issue happens as follows:
> > Assume:
> > sbi->s_cluster_ratio = 16
> > Step1: Insert delay block [0, 31] -> ei->i_reserved_data_blocks=2
> > Step2:
> > ext4_writepages
> > mpage_map_and_submit_extent -> return failed
> > mpage_release_unused_pages -> to release [0, 30]
> > ext4_es_remove_extent -> remove lblk=0 end=30
> > __es_remove_extent -> len1=0 len2=31-30=1
> > __es_remove_extent:
> > ...
> > if (len2 > 0) {
> > ...
> > if (len1 > 0) {
> > ...
> > } else {
> > es->es_lblk = end + 1;
> > es->es_len = len2;
> > ...
> > }
> > if (count_reserved)
> > count_rsvd(inode, lblk, orig_es.es_len - len1 - len2, &orig_es, &rc);
> > goto out; -> will return but didn't calculate 'reserved'
> > ...
> > Step3: ext4_destroy_inode -> trigger "i_reserved_data_blocks (1) not cleared!"
> >
> > To solve above issue if 'len2>0' call 'get_rsvd()' before goto out.
> >
> > Reported-by: syzbot+05a0f0ccab4a25626e38@syzkaller.appspotmail.com
> > Fixes: 8fcc3a580651 ("ext4: rework reserved cluster accounting when invalidating pages")
> > Signed-off-by: Ye Bin <yebin10@huawei.com>
> > ---
> > fs/ext4/extents_status.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> > index cd0a861853e3..7ada374ff27d 100644
> > --- a/fs/ext4/extents_status.c
> > +++ b/fs/ext4/extents_status.c
> > @@ -1371,7 +1371,7 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
> > if (count_reserved)
> > count_rsvd(inode, lblk, orig_es.es_len - len1 - len2,
> > &orig_es, &rc);
> > - goto out;
> > + goto out_get_reserved;
> > }
> >
> > if (len1 > 0) {
> > @@ -1413,6 +1413,7 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
> > }
> > }
> >
> > +out_get_reserved:
> > if (count_reserved)
> > *reserved = get_rsvd(inode, end, es, &rc);
> > out:
>
> The length of some lines in the commit description - probably those which are
> log output - is resulting in a checkpatch warning. It generally prefers lines
> to be a maximum of 75 characters (and Ted usually likes them limited to 72
> characters. See my comment to patch #3. I'm not sure what Ted would want here,
> though I'd probably break them at 72 characters or less.
>
> Otherwise, the patch looks good. Feel free to add:
>
Looks good. As before, feel free to add:
> Reviewed-by: Eric Whitney <enwlinux@gmail.com>
>
> > --
> > 2.31.1
> >
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/3] ext4: fix incorrect calculate 'reserved' in '__es_remove_extent' when enable bigalloc feature
2022-12-08 22:53 ` Eric Whitney
@ 2022-12-08 22:54 ` Eric Whitney
0 siblings, 0 replies; 9+ messages in thread
From: Eric Whitney @ 2022-12-08 22:54 UTC (permalink / raw)
To: Eric Whitney
Cc: Ye Bin, tytso, adilger.kernel, linux-ext4, linux-kernel, jack,
Ye Bin, syzbot+05a0f0ccab4a25626e38
* Eric Whitney <enwlinux@gmail.com>:
> * Eric Whitney <enwlinux@gmail.com>:
> > * Ye Bin <yebin@huaweicloud.com>:
> > > From: Ye Bin <yebin10@huawei.com>
> > >
> > > Syzbot report issue as follows:
> > > EXT4-fs error (device loop0): ext4_validate_block_bitmap:398: comm rep: bg 0: block 5: invalid block bitmap
> > > EXT4-fs (loop0): Delayed block allocation failed for inode 18 at logical offset 0 with max blocks 32 with error 28
> > > EXT4-fs (loop0): This should not happen!! Data will be lost
> > >
> > > EXT4-fs (loop0): Total free blocks count 0
> > > EXT4-fs (loop0): Free/Dirty block details
> > > EXT4-fs (loop0): free_blocks=0
> > > EXT4-fs (loop0): dirty_blocks=32
> > > EXT4-fs (loop0): Block reservation details
> > > EXT4-fs (loop0): i_reserved_data_blocks=2
> > > EXT4-fs (loop0): Inode 18 (00000000845cd634): i_reserved_data_blocks (1) not cleared!
> > >
> > > Above issue happens as follows:
> > > Assume:
> > > sbi->s_cluster_ratio = 16
> > > Step1: Insert delay block [0, 31] -> ei->i_reserved_data_blocks=2
> > > Step2:
> > > ext4_writepages
> > > mpage_map_and_submit_extent -> return failed
> > > mpage_release_unused_pages -> to release [0, 30]
> > > ext4_es_remove_extent -> remove lblk=0 end=30
> > > __es_remove_extent -> len1=0 len2=31-30=1
> > > __es_remove_extent:
> > > ...
> > > if (len2 > 0) {
> > > ...
> > > if (len1 > 0) {
> > > ...
> > > } else {
> > > es->es_lblk = end + 1;
> > > es->es_len = len2;
> > > ...
> > > }
> > > if (count_reserved)
> > > count_rsvd(inode, lblk, orig_es.es_len - len1 - len2, &orig_es, &rc);
> > > goto out; -> will return but didn't calculate 'reserved'
> > > ...
> > > Step3: ext4_destroy_inode -> trigger "i_reserved_data_blocks (1) not cleared!"
> > >
> > > To solve above issue if 'len2>0' call 'get_rsvd()' before goto out.
> > >
> > > Reported-by: syzbot+05a0f0ccab4a25626e38@syzkaller.appspotmail.com
> > > Fixes: 8fcc3a580651 ("ext4: rework reserved cluster accounting when invalidating pages")
> > > Signed-off-by: Ye Bin <yebin10@huawei.com>
> > > ---
> > > fs/ext4/extents_status.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> > > index cd0a861853e3..7ada374ff27d 100644
> > > --- a/fs/ext4/extents_status.c
> > > +++ b/fs/ext4/extents_status.c
> > > @@ -1371,7 +1371,7 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
> > > if (count_reserved)
> > > count_rsvd(inode, lblk, orig_es.es_len - len1 - len2,
> > > &orig_es, &rc);
> > > - goto out;
> > > + goto out_get_reserved;
> > > }
> > >
> > > if (len1 > 0) {
> > > @@ -1413,6 +1413,7 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
> > > }
> > > }
> > >
> > > +out_get_reserved:
> > > if (count_reserved)
> > > *reserved = get_rsvd(inode, end, es, &rc);
> > > out:
> >
> > The length of some lines in the commit description - probably those which are
> > log output - is resulting in a checkpatch warning. It generally prefers lines
> > to be a maximum of 75 characters (and Ted usually likes them limited to 72
> > characters. See my comment to patch #3. I'm not sure what Ted would want here,
> > though I'd probably break them at 72 characters or less.
> >
> > Otherwise, the patch looks good. Feel free to add:
> >
>
> Looks good. As before, feel free to add:
>
> > Reviewed-by: Eric Whitney <enwlinux@gmail.com>
>
Whoops. Please disregard - wrong patch.
Eric
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 2/3] ext4: record error when detect abnormal 'i_reserved_data_blocks'
2022-12-03 2:59 [PATCH v3 0/3] Fix two issues about bigalloc feature Ye Bin
2022-12-03 2:59 ` [PATCH v3 1/3] ext4: fix incorrect calculate 'reserved' in '__es_remove_extent' when enable " Ye Bin
@ 2022-12-03 2:59 ` Ye Bin
2022-12-07 21:26 ` Eric Whitney
2022-12-03 2:59 ` [PATCH v3 3/3] ext4: add check pending tree when evict inode Ye Bin
2 siblings, 1 reply; 9+ messages in thread
From: Ye Bin @ 2022-12-03 2:59 UTC (permalink / raw)
To: tytso, adilger.kernel, linux-ext4; +Cc: linux-kernel, jack, Ye Bin
From: Ye Bin <yebin10@huawei.com>
If 'i_reserved_data_blocks' is not cleared which mean something wrong with
code, free space accounting is likely wrong, according to Jan Kara's advice
use ext4_error() to record this abnormal let fsck to repair and also we can
capture this issue.
Signed-off-by: Ye Bin <yebin10@huawei.com>
---
fs/ext4/super.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 840e0a614959..41413338c05b 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1387,10 +1387,10 @@ static void ext4_destroy_inode(struct inode *inode)
}
if (EXT4_I(inode)->i_reserved_data_blocks)
- ext4_msg(inode->i_sb, KERN_ERR,
- "Inode %lu (%p): i_reserved_data_blocks (%u) not cleared!",
- inode->i_ino, EXT4_I(inode),
- EXT4_I(inode)->i_reserved_data_blocks);
+ ext4_error(inode->i_sb,
+ "Inode %lu (%p): i_reserved_data_blocks (%u) not cleared!",
+ inode->i_ino, EXT4_I(inode),
+ EXT4_I(inode)->i_reserved_data_blocks);
}
static void init_once(void *foo)
--
2.31.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 2/3] ext4: record error when detect abnormal 'i_reserved_data_blocks'
2022-12-03 2:59 ` [PATCH v3 2/3] ext4: record error when detect abnormal 'i_reserved_data_blocks' Ye Bin
@ 2022-12-07 21:26 ` Eric Whitney
0 siblings, 0 replies; 9+ messages in thread
From: Eric Whitney @ 2022-12-07 21:26 UTC (permalink / raw)
To: Ye Bin; +Cc: tytso, adilger.kernel, linux-ext4, linux-kernel, jack, Ye Bin
* Ye Bin <yebin@huaweicloud.com>:
> From: Ye Bin <yebin10@huawei.com>
>
> If 'i_reserved_data_blocks' is not cleared which mean something wrong with
> code, free space accounting is likely wrong, according to Jan Kara's advice
> use ext4_error() to record this abnormal let fsck to repair and also we can
> capture this issue.
>
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> ---
> fs/ext4/super.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 840e0a614959..41413338c05b 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1387,10 +1387,10 @@ static void ext4_destroy_inode(struct inode *inode)
> }
>
> if (EXT4_I(inode)->i_reserved_data_blocks)
> - ext4_msg(inode->i_sb, KERN_ERR,
> - "Inode %lu (%p): i_reserved_data_blocks (%u) not cleared!",
> - inode->i_ino, EXT4_I(inode),
> - EXT4_I(inode)->i_reserved_data_blocks);
> + ext4_error(inode->i_sb,
> + "Inode %lu (%p): i_reserved_data_blocks (%u) not cleared!",
> + inode->i_ino, EXT4_I(inode),
> + EXT4_I(inode)->i_reserved_data_blocks);
It would be better if the arguments to ext4_error after the first were aligned
under "inode->i_sb", as you had in your first version. That's typical ext4
practice as seen earlier in this function, though this does pass checkpatch.
Otherwise, looks good.
That said, feel free to add:
Reviewed-by: Eric Whitney <enwlinux@gmail.com>
> }
>
> static void init_once(void *foo)
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 3/3] ext4: add check pending tree when evict inode
2022-12-03 2:59 [PATCH v3 0/3] Fix two issues about bigalloc feature Ye Bin
2022-12-03 2:59 ` [PATCH v3 1/3] ext4: fix incorrect calculate 'reserved' in '__es_remove_extent' when enable " Ye Bin
2022-12-03 2:59 ` [PATCH v3 2/3] ext4: record error when detect abnormal 'i_reserved_data_blocks' Ye Bin
@ 2022-12-03 2:59 ` Ye Bin
2022-12-07 21:59 ` Eric Whitney
2 siblings, 1 reply; 9+ messages in thread
From: Ye Bin @ 2022-12-03 2:59 UTC (permalink / raw)
To: tytso, adilger.kernel, linux-ext4
Cc: linux-kernel, jack, Ye Bin, syzbot+05a0f0ccab4a25626e38
From: Ye Bin <yebin10@huawei.com>
Syzbot found the following issue:
BUG: memory leak
unreferenced object 0xffff8881bde17420 (size 32):
comm "rep", pid 2327, jiffies 4295381963 (age 32.265s)
hex dump (first 32 bytes):
01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<00000000ac6d38f8>] __insert_pending+0x13c/0x2d0
[<00000000d717de3b>] ext4_es_insert_delayed_block+0x399/0x4e0
[<000000004be03913>] ext4_da_map_blocks.constprop.0+0x739/0xfa0
[<00000000885a832a>] ext4_da_get_block_prep+0x10c/0x440
[<0000000029b7f8ef>] __block_write_begin_int+0x28d/0x860
[<00000000e182ebc3>] ext4_da_write_inline_data_begin+0x2d1/0xf30
[<00000000ced0c8a2>] ext4_da_write_begin+0x612/0x860
[<000000008d5f27fa>] generic_perform_write+0x215/0x4d0
[<00000000552c1cde>] ext4_buffered_write_iter+0x101/0x3b0
[<0000000052177ae8>] do_iter_readv_writev+0x19f/0x340
[<000000004b9de834>] do_iter_write+0x13b/0x650
[<00000000e2401b9b>] iter_file_splice_write+0x5a5/0xab0
[<0000000023aa5d90>] direct_splice_actor+0x103/0x1e0
[<0000000089e00fc1>] splice_direct_to_actor+0x2c9/0x7b0
[<000000004386851e>] do_splice_direct+0x159/0x280
[<00000000b567e609>] do_sendfile+0x932/0x1200
Above issue fixed by 1b8f787ef547 "ext4: fix warning in 'ext4_da_release_space'"
in this scene. To make things better add check pending tree when evit inode.
According to Eric Whitney's suggestion, bigalloc + inline is still in development
so we just add test for this situation, there isn't need to add code to free
pending tree entry.
Reported-by: syzbot+05a0f0ccab4a25626e38@syzkaller.appspotmail.com
Signed-off-by: Ye Bin <yebin10@huawei.com>
---
fs/ext4/super.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 41413338c05b..2e2fbc4a832c 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1391,6 +1391,11 @@ static void ext4_destroy_inode(struct inode *inode)
"Inode %lu (%p): i_reserved_data_blocks (%u) not cleared!",
inode->i_ino, EXT4_I(inode),
EXT4_I(inode)->i_reserved_data_blocks);
+
+ if (!RB_EMPTY_ROOT(&EXT4_I(inode)->i_pending_tree.root))
+ ext4_error(inode->i_sb,
+ "Inode %lu (%p): i_pending_tree not empty!",
+ inode->i_ino, EXT4_I(inode));
}
static void init_once(void *foo)
--
2.31.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/3] ext4: add check pending tree when evict inode
2022-12-03 2:59 ` [PATCH v3 3/3] ext4: add check pending tree when evict inode Ye Bin
@ 2022-12-07 21:59 ` Eric Whitney
0 siblings, 0 replies; 9+ messages in thread
From: Eric Whitney @ 2022-12-07 21:59 UTC (permalink / raw)
To: Ye Bin
Cc: tytso, adilger.kernel, linux-ext4, linux-kernel, jack, Ye Bin,
syzbot+05a0f0ccab4a25626e38
`* Ye Bin <yebin@huaweicloud.com>:
> From: Ye Bin <yebin10@huawei.com>
>
> Syzbot found the following issue:
> BUG: memory leak
> unreferenced object 0xffff8881bde17420 (size 32):
> comm "rep", pid 2327, jiffies 4295381963 (age 32.265s)
> hex dump (first 32 bytes):
> 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> backtrace:
> [<00000000ac6d38f8>] __insert_pending+0x13c/0x2d0
> [<00000000d717de3b>] ext4_es_insert_delayed_block+0x399/0x4e0
> [<000000004be03913>] ext4_da_map_blocks.constprop.0+0x739/0xfa0
> [<00000000885a832a>] ext4_da_get_block_prep+0x10c/0x440
> [<0000000029b7f8ef>] __block_write_begin_int+0x28d/0x860
> [<00000000e182ebc3>] ext4_da_write_inline_data_begin+0x2d1/0xf30
> [<00000000ced0c8a2>] ext4_da_write_begin+0x612/0x860
> [<000000008d5f27fa>] generic_perform_write+0x215/0x4d0
> [<00000000552c1cde>] ext4_buffered_write_iter+0x101/0x3b0
> [<0000000052177ae8>] do_iter_readv_writev+0x19f/0x340
> [<000000004b9de834>] do_iter_write+0x13b/0x650
> [<00000000e2401b9b>] iter_file_splice_write+0x5a5/0xab0
> [<0000000023aa5d90>] direct_splice_actor+0x103/0x1e0
> [<0000000089e00fc1>] splice_direct_to_actor+0x2c9/0x7b0
> [<000000004386851e>] do_splice_direct+0x159/0x280
> [<00000000b567e609>] do_sendfile+0x932/0x1200
>
> Above issue fixed by 1b8f787ef547 "ext4: fix warning in 'ext4_da_release_space'"
> in this scene. To make things better add check pending tree when evit inode.
> According to Eric Whitney's suggestion, bigalloc + inline is still in development
> so we just add test for this situation, there isn't need to add code to free
> pending tree entry.
>
> Reported-by: syzbot+05a0f0ccab4a25626e38@syzkaller.appspotmail.com
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> ---
> fs/ext4/super.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 41413338c05b..2e2fbc4a832c 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1391,6 +1391,11 @@ static void ext4_destroy_inode(struct inode *inode)
> "Inode %lu (%p): i_reserved_data_blocks (%u) not cleared!",
> inode->i_ino, EXT4_I(inode),
> EXT4_I(inode)->i_reserved_data_blocks);
> +
> + if (!RB_EMPTY_ROOT(&EXT4_I(inode)->i_pending_tree.root))
> + ext4_error(inode->i_sb,
> + "Inode %lu (%p): i_pending_tree not empty!",
> + inode->i_ino, EXT4_I(inode));
> }
>
It's always a good idea to run ./scripts/checkpatch.pl on your patches before
submitting them. It's complaining that the lines in your commit description
are too long (it wants a maximum of 75 characters per line, but Ted prefers
a maximum of 72 for ext4 patches, IIRC). Also, it wants parentheses around
the title of the patch mentioned in the commit message:
ie: commit 1b8f787ef547 ("ext4: fix warning in 'ext4_da_release_space'")
Also, typical ext4 practice is to align arguments to a function on following
lines to the beginning of the first argument, as can be seen earlier in
ext4_destroy_inode. Indenting as you've done here passes checkpatch, but
it's different from most ext4 code (which also passes checkpatch).
Otherwise, it looks okay.
Eric
> static void init_once(void *foo)
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread