ext4: fast commit recovery path

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

Hi Dan,

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