* [bug report] ext4: fast commit recovery path
@ 2020-10-27 8:09 Dan Carpenter
2020-10-27 18:00 ` harshad shirwadkar
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2020-10-27 8:09 UTC (permalink / raw)
To: harshadshirwadkar; +Cc: linux-ext4
Hello Harshad Shirwadkar,
The patch 8016e29f4362: "ext4: fast commit recovery path" from Oct
15, 2020, leads to the following static checker warning:
fs/ext4/fast_commit.c:1620 ext4_fc_replay_add_range()
warn: 'path' is an error pointer or valid
fs/ext4/fast_commit.c
1600 cur = start;
1601 remaining = len;
1602 jbd_debug(1, "ADD_RANGE, lblk %d, pblk %lld, len %d, unwritten %d, inode %ld\n",
1603 start, start_pblk, len, ext4_ext_is_unwritten(ex),
1604 inode->i_ino);
1605
1606 while (remaining > 0) {
1607 map.m_lblk = cur;
1608 map.m_len = remaining;
1609 map.m_pblk = 0;
1610 ret = ext4_map_blocks(NULL, inode, &map, 0);
1611
1612 if (ret < 0) {
1613 iput(inode);
1614 return 0;
1615 }
1616
1617 if (ret == 0) {
1618 /* Range is not mapped */
1619 path = ext4_find_extent(inode, cur, NULL, 0);
1620 if (!path)
1621 continue;
^^^^^^^^^^^^^^^^
"path" can't be NULL, this should be an IS_ERR() test. It's sort of
surprising to me that we continue here instead of returning an error.
1622 memset(&newex, 0, sizeof(newex));
1623 newex.ee_block = cpu_to_le32(cur);
1624 ext4_ext_store_pblock(
1625 &newex, start_pblk + cur - start);
1626 newex.ee_len = cpu_to_le16(map.m_len);
1627 if (ext4_ext_is_unwritten(ex))
1628 ext4_ext_mark_unwritten(&newex);
1629 down_write(&EXT4_I(inode)->i_data_sem);
1630 ret = ext4_ext_insert_extent(
1631 NULL, inode, &path, &newex, 0);
1632 up_write((&EXT4_I(inode)->i_data_sem));
1633 ext4_ext_drop_refs(path);
1634 kfree(path);
1635 if (ret) {
1636 iput(inode);
1637 return 0;
1638 }
1639 goto next;
1640 }
1641
1642 if (start_pblk + cur - start != map.m_pblk) {
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] ext4: fast commit recovery path
2020-10-27 8:09 [bug report] ext4: fast commit recovery path Dan Carpenter
@ 2020-10-27 18:00 ` harshad shirwadkar
0 siblings, 0 replies; 4+ messages in thread
From: harshad shirwadkar @ 2020-10-27 18:00 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Ext4 Developers List
Hi Dan,
On Tue, Oct 27, 2020 at 1:10 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Hello Harshad Shirwadkar,
>
> The patch 8016e29f4362: "ext4: fast commit recovery path" from Oct
> 15, 2020, leads to the following static checker warning:
>
> fs/ext4/fast_commit.c:1620 ext4_fc_replay_add_range()
> warn: 'path' is an error pointer or valid
>
> fs/ext4/fast_commit.c
> 1600 cur = start;
> 1601 remaining = len;
> 1602 jbd_debug(1, "ADD_RANGE, lblk %d, pblk %lld, len %d, unwritten %d, inode %ld\n",
> 1603 start, start_pblk, len, ext4_ext_is_unwritten(ex),
> 1604 inode->i_ino);
> 1605
> 1606 while (remaining > 0) {
> 1607 map.m_lblk = cur;
> 1608 map.m_len = remaining;
> 1609 map.m_pblk = 0;
> 1610 ret = ext4_map_blocks(NULL, inode, &map, 0);
> 1611
> 1612 if (ret < 0) {
> 1613 iput(inode);
> 1614 return 0;
> 1615 }
> 1616
> 1617 if (ret == 0) {
> 1618 /* Range is not mapped */
> 1619 path = ext4_find_extent(inode, cur, NULL, 0);
> 1620 if (!path)
> 1621 continue;
> ^^^^^^^^^^^^^^^^
> "path" can't be NULL, this should be an IS_ERR() test. It's sort of
> surprising to me that we continue here instead of returning an error.
Thanks for pointing this out. We should check using IS_ERR() here.
Also, I agree that instead of "continue" we should be returning an
error. If not we'd be stuck in this loop forever.
Thanks,
Harshad
>
> 1622 memset(&newex, 0, sizeof(newex));
> 1623 newex.ee_block = cpu_to_le32(cur);
> 1624 ext4_ext_store_pblock(
> 1625 &newex, start_pblk + cur - start);
> 1626 newex.ee_len = cpu_to_le16(map.m_len);
> 1627 if (ext4_ext_is_unwritten(ex))
> 1628 ext4_ext_mark_unwritten(&newex);
> 1629 down_write(&EXT4_I(inode)->i_data_sem);
> 1630 ret = ext4_ext_insert_extent(
> 1631 NULL, inode, &path, &newex, 0);
> 1632 up_write((&EXT4_I(inode)->i_data_sem));
> 1633 ext4_ext_drop_refs(path);
> 1634 kfree(path);
> 1635 if (ret) {
> 1636 iput(inode);
> 1637 return 0;
> 1638 }
> 1639 goto next;
> 1640 }
> 1641
> 1642 if (start_pblk + cur - start != map.m_pblk) {
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* [bug report] ext4: fast commit recovery path
@ 2021-11-30 9:40 Dan Carpenter
2021-12-01 16:35 ` harshad shirwadkar
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2021-11-30 9:40 UTC (permalink / raw)
To: harshadshirwadkar; +Cc: linux-ext4
Hello Harshad Shirwadkar,
The patch 8016e29f4362: "ext4: fast commit recovery path" from Oct
15, 2020, leads to the following Smatch static checker warnings:
fs/ext4/inode.c:4533 __ext4_get_inode_loc_noinmem()
error: uninitialized symbol 'err_blk'.
fs/ext4/inode.c:4548 ext4_get_inode_loc()
error: uninitialized symbol 'err_blk'.
fs/ext4/inode.c
4523 static int __ext4_get_inode_loc_noinmem(struct inode *inode,
4524 struct ext4_iloc *iloc)
4525 {
4526 ext4_fsblk_t err_blk;
4527 int ret;
4528
4529 ret = __ext4_get_inode_loc(inode->i_sb, inode->i_ino, NULL, iloc,
4530 &err_blk);
4531
4532 if (ret == -EIO)
--> 4533 ext4_error_inode_block(inode, err_blk, EIO,
Only the last return -EIO sets err_blk. The first return -EIO leaves it
uninitialized.
4534 "unable to read itable block");
4535
4536 return ret;
4537 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] ext4: fast commit recovery path
2021-11-30 9:40 Dan Carpenter
@ 2021-12-01 16:35 ` harshad shirwadkar
0 siblings, 0 replies; 4+ messages in thread
From: harshad shirwadkar @ 2021-12-01 16:35 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Ext4 Developers List
Thanks for the report Dan. This patch -
https://patchwork.ozlabs.org/project/linux-ext4/patch/20211201163421.2631661-1-harshads@google.com/
should take care of this report.
- Harshad
On Tue, Nov 30, 2021 at 1:40 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Hello Harshad Shirwadkar,
>
> The patch 8016e29f4362: "ext4: fast commit recovery path" from Oct
> 15, 2020, leads to the following Smatch static checker warnings:
>
> fs/ext4/inode.c:4533 __ext4_get_inode_loc_noinmem()
> error: uninitialized symbol 'err_blk'.
>
> fs/ext4/inode.c:4548 ext4_get_inode_loc()
> error: uninitialized symbol 'err_blk'.
>
> fs/ext4/inode.c
> 4523 static int __ext4_get_inode_loc_noinmem(struct inode *inode,
> 4524 struct ext4_iloc *iloc)
> 4525 {
> 4526 ext4_fsblk_t err_blk;
> 4527 int ret;
> 4528
> 4529 ret = __ext4_get_inode_loc(inode->i_sb, inode->i_ino, NULL, iloc,
> 4530 &err_blk);
> 4531
> 4532 if (ret == -EIO)
> --> 4533 ext4_error_inode_block(inode, err_blk, EIO,
>
> Only the last return -EIO sets err_blk. The first return -EIO leaves it
> uninitialized.
>
> 4534 "unable to read itable block");
> 4535
> 4536 return ret;
> 4537 }
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-12-01 16:35 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-10-27 8:09 [bug report] ext4: fast commit recovery path Dan Carpenter
2020-10-27 18:00 ` harshad shirwadkar
-- strict thread matches above, loose matches on Subject: below --
2021-11-30 9:40 Dan Carpenter
2021-12-01 16:35 ` harshad shirwadkar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox