Hi Dan,
On Tue, Oct 27, 2020 at 1:10 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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
