On 2021/8/9 下午8:13, Qu Wenruo wrote:

On 2021/8/9 下午7:32, Dan Carpenter wrote:

quoted

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 <redacted>
Reported-by: Dan Carpenter <redacted>

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.

quoted

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;

quoted

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.

quoted

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

quoted

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.

quoted

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

quoted

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