[kbuild-all] Re: [PATCH v5 09/11] btrfs: defrag: use defrag_one_cluster() to implement btrfs_defrag_file()

On 2021/8/9 下午7:32, Dan Carpenter wrote: > Hi Qu, > > url: > https://github.com/0day-ci/linux/commits/Qu-Wenruo/btrfs-defrag-rework-to... > > base: > https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next > config: h8300-randconfig-m031-20210804 (attached as .config) > compiler: h8300-linux-gcc (GCC) 10.3.0 > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp(a)intel.com&gt; > Reported-by: Dan Carpenter <dan.carpenter(a)oracle.com&gt; > > New smatch warnings: > fs/btrfs/ioctl.c:1869 btrfs_defrag_file() error: uninitialized symbol > 'ret'. OK, it's indeed a possible case, and I even find a special case where we don't need to go through the branch reported by the static checker. > > vim +/ret +1869 fs/btrfs/ioctl.c > > fe90d1614439a8 Qu Wenruo                 2021-08-06  1757  int > btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra, > 4cb5300bc839b8 Chris Mason               2011-05-24 > 1758                struct btrfs_ioctl_defrag_range_args *range, > 4cb5300bc839b8 Chris Mason               2011-05-24 > 1759                u64 newer_than, unsigned long max_to_defrag) > 4cb5300bc839b8 Chris Mason               2011-05-24  1760  { > 0b246afa62b0cf Jeff Mahoney              2016-06-22  1761      struct > btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb); > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1762 > unsigned long sectors_defragged = 0; > 151a31b25e5c94 Li Zefan                  2011-09-02  1763      u64 > isize = i_size_read(inode); > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1764      u64 cur; > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1765      u64 > last_byte; > 1e2ef46d89ee41 David Sterba              2017-07-17  1766      bool > do_compress = range->flags & BTRFS_DEFRAG_RANGE_COMPRESS; > fe90d1614439a8 Qu Wenruo                 2021-08-06  1767      bool > ra_allocated = false; > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1768      int > compress_type = BTRFS_COMPRESS_ZLIB; > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1769      int ret; > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1770      u32 > extent_thresh = range->extent_thresh; > 4cb5300bc839b8 Chris Mason               2011-05-24  1771 > 0abd5b17249ea5 Liu Bo                    2013-04-16  1772      if > (isize == 0) > 0abd5b17249ea5 Liu Bo                    2013-04-16  1773 > return 0; > 0abd5b17249ea5 Liu Bo                    2013-04-16  1774 > 0abd5b17249ea5 Liu Bo                    2013-04-16  1775      if > (range->start >= isize) > 0abd5b17249ea5 Liu Bo                    2013-04-16  1776 > return -EINVAL; Firstly, we skip several invalid cases, like empty file or range beyond isize. Notice that now range->start < isize; AKA range->start <= isize - 1; > 1a419d85a76853 Li Zefan                  2010-10-25  1777 > 1e2ef46d89ee41 David Sterba              2017-07-17  1778      if > (do_compress) { > ce96b7ffd11e26 Chengguang Xu             2019-10-10  1779          if > (range->compress_type >= BTRFS_NR_COMPRESS_TYPES) > 1a419d85a76853 Li Zefan                  2010-10-25  1780 > return -EINVAL; > 1a419d85a76853 Li Zefan                  2010-10-25  1781          if > (range->compress_type) > 1a419d85a76853 Li Zefan                  2010-10-25  1782 > compress_type = range->compress_type; > 1a419d85a76853 Li Zefan                  2010-10-25  1783      } > f46b5a66b3316e Christoph Hellwig         2008-06-11  1784 > 0abd5b17249ea5 Liu Bo                    2013-04-16  1785      if > (extent_thresh == 0) > ee22184b53c823 Byongho Lee               2015-12-15  1786 > extent_thresh = SZ_256K; > 940100a4a7b78b Chris Mason               2010-03-10  1787 > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1788      if > (range->start + range->len > range->start) { > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1789          /* > Got a specific range */ > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1790 > last_byte = min(isize, range->start + range->len) - 1; > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1791      } else { > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1792          /* > Defrag until file end */ > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1793 > last_byte = isize - 1; > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1794      } No matter the range->len, last_byte <= isize - 1; Also start->range <= isize - 1; Thus we can have a worst case where start->range == last_byte. > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1795 > 4cb5300bc839b8 Chris Mason               2011-05-24  1796      /* > fe90d1614439a8 Qu Wenruo                 2021-08-06  1797       * If > we were not given a ra, allocate a readahead context. As > 0a52d108089f33 David Sterba              2017-06-22  1798       * > readahead is just an optimization, defrag will work without it so > 0a52d108089f33 David Sterba              2017-06-22  1799       * we > don't error out. > 4cb5300bc839b8 Chris Mason               2011-05-24  1800       */ > fe90d1614439a8 Qu Wenruo                 2021-08-06  1801      if (!ra) { > fe90d1614439a8 Qu Wenruo                 2021-08-06  1802 > ra_allocated = true; > 63e727ecd238be David Sterba              2017-06-22  1803          ra > = kzalloc(sizeof(*ra), GFP_KERNEL); > 0a52d108089f33 David Sterba              2017-06-22  1804          if > (ra) > 4cb5300bc839b8 Chris Mason               2011-05-24  1805 > file_ra_state_init(ra, inode->i_mapping); > 4cb5300bc839b8 Chris Mason               2011-05-24  1806      } > 4cb5300bc839b8 Chris Mason               2011-05-24  1807 > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1808      /* > Align the range */ > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1809      cur = > round_down(range->start, fs_info->sectorsize); > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1810 > last_byte = round_up(last_byte, fs_info->sectorsize) - 1; Even for the worst case, range->start == last_byte. If range->start = last_byte = 4K (aka isize = 4K + 1), then: cur = 4K; last_byte = 4K - 1; Now we don't need to go through the while() loop at all.

> 4cb5300bc839b8 Chris Mason               2011-05-24  1811 > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1812      while > (cur < last_byte) { > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1813          u64 > cluster_end; > 1e701a3292e25a Chris Mason               2010-03-11  1814 > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1815          /* > The cluster size 256K should always be page aligned */ > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1816 > BUILD_BUG_ON(!IS_ALIGNED(CLUSTER_SIZE, PAGE_SIZE)); > 008873eafbc77d Li Zefan                  2011-09-02  1817 > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1818          /* > We want the cluster ends at page boundary when possible */ > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1819 > cluster_end = (((cur >> PAGE_SHIFT) + > d0b928ff1ed56a Qu Wenruo                 2021-08-06 > 1820                     (SZ_256K >> PAGE_SHIFT)) << PAGE_SHIFT) - 1; > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1821 > cluster_end = min(cluster_end, last_byte); > 940100a4a7b78b Chris Mason               2010-03-10  1822 > 64708539cd23b3 Josef Bacik               2021-02-10  1823 > btrfs_inode_lock(inode, 0); > eede2bf34f4fa8 Omar Sandoval             2016-11-03  1824          if > (IS_SWAPFILE(inode)) { > eede2bf34f4fa8 Omar Sandoval             2016-11-03  1825 > ret = -ETXTBSY; > 64708539cd23b3 Josef Bacik               2021-02-10  1826 > btrfs_inode_unlock(inode, 0); > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1827 > break; > ecb8bea87d05fd Liu Bo                    2012-03-29  1828          } > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1829          if > (!(inode->i_sb->s_flags & SB_ACTIVE)) { > 64708539cd23b3 Josef Bacik               2021-02-10  1830 > btrfs_inode_unlock(inode, 0); > 4cb5300bc839b8 Chris Mason               2011-05-24  1831 > break; > > Can we hit this break statement on the first iteration through the loop? > > 3eaa2885276fd6 Chris Mason               2008-07-24  1832          } > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1833          if > (do_compress) > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1834 > BTRFS_I(inode)->defrag_compress = compress_type; > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1835          ret > = defrag_one_cluster(BTRFS_I(inode), ra, cur, > d0b928ff1ed56a Qu Wenruo                 2021-08-06 > 1836                  cluster_end + 1 - cur, extent_thresh, > d0b928ff1ed56a Qu Wenruo                 2021-08-06 > 1837                  newer_than, do_compress, > d0b928ff1ed56a Qu Wenruo                 2021-08-06 > 1838                  &sectors_defragged, max_to_defrag); > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1839 > btrfs_inode_unlock(inode, 0); > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1840          if > (ret < 0) > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1841 > break; > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1842          cur > = cluster_end + 1; > 4cb5300bc839b8 Chris Mason               2011-05-24  1843      } > f46b5a66b3316e Christoph Hellwig         2008-06-11  1844 > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1845      if > (ra_allocated) > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1846 > kfree(ra); > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1847      if > (sectors_defragged) { > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1848          /* > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1849           * > We have defragged some sectors, for compression case > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1850           * > they need to be written back immediately. > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1851           */ > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1852          if > (range->flags & BTRFS_DEFRAG_RANGE_START_IO) { > 1e701a3292e25a Chris Mason               2010-03-11  1853 > filemap_flush(inode->i_mapping); > dec8ef90552f7b Filipe Manana             2014-03-01  1854 > if (test_bit(BTRFS_INODE_HAS_ASYNC_EXTENT, > dec8ef90552f7b Filipe Manana             2014-03-01 > 1855                       &BTRFS_I(inode)->runtime_flags)) > 1e701a3292e25a Chris Mason               2010-03-11 > 1856                  filemap_flush(inode->i_mapping); > dec8ef90552f7b Filipe Manana             2014-03-01  1857          } > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1858          if > (range->compress_type == BTRFS_COMPRESS_LZO) > 0b246afa62b0cf Jeff Mahoney              2016-06-22  1859 > btrfs_set_fs_incompat(fs_info, COMPRESS_LZO); > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1860 > else if (range->compress_type == BTRFS_COMPRESS_ZSTD) > 5c1aab1dd5445e Nick Terrell              2017-08-09  1861 > btrfs_set_fs_incompat(fs_info, COMPRESS_ZSTD); > d0b928ff1ed56a Qu Wenruo                 2021-08-06  1862          ret > = sectors_defragged; > 1a419d85a76853 Li Zefan                  2010-10-25  1863      } We also skip above (sectors_defraged) branch. > 1e2ef46d89ee41 David Sterba              2017-07-17  1864      if > (do_compress) { > 64708539cd23b3 Josef Bacik               2021-02-10  1865 > btrfs_inode_lock(inode, 0); > eec63c65dcbeb1 David Sterba              2017-07-17  1866 > BTRFS_I(inode)->defrag_compress = BTRFS_COMPRESS_NONE; > 64708539cd23b3 Josef Bacik               2021-02-10  1867 > btrfs_inode_unlock(inode, 0); > 633085c79c84c3 Filipe David Borba Manana 2013-08-16  1868      } > 940100a4a7b78b Chris Mason               2010-03-10 @1869      return > ret; And @ret is indeed uninitialized. Will fix it in next update. Thanks, Qu > f46b5a66b3316e Christoph Hellwig         2008-06-11  1870  } > > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org >