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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2021/8/9 下午8:13, Qu Wenruo wrote:
> 
> 
> 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-support-sector-perfect-defrag/20210806-161501 
>>
>> 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@intel.com>
>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>>
>> 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.

BTW, here the proper way to calculate last_byte should be:

round_up(last_byte + 1, sectorsize) - 1;

So that we are ensured that rounded up last_byte will never be smaller 
than range->start.

I have fixed both uninitialized @ret and this @last_byte problem in my 
github repo already.

Thanks,
Qu
> 
>> 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
>>