Re: [bug report] btrfs: introduce read_extent_buffer_subpage()

[Qu Wenruo]

On 2021/1/28 下午6:50, Dan Carpenter wrote:

Hello Qu Wenruo,

The patch 5c60a522f1ea: "btrfs: introduce
read_extent_buffer_subpage()" from Jan 16, 2021, leads to the
following static checker warning:

        fs/btrfs/extent_io.c:5797 read_extent_buffer_subpage()
        info: return a literal instead of 'ret'

fs/btrfs/extent_io.c
   5780  static int read_extent_buffer_subpage(struct extent_buffer *eb, int 
wait,
   5781                                        int mirror_num)
   5782  {
   5783          struct btrfs_fs_info *fs_info = eb->fs_info;
   5784          struct extent_io_tree *io_tree;
   5785          struct page *page = eb->pages[0];
   5786          struct bio *bio = NULL;
   5787          int ret = 0;
   5788
   5789          ASSERT(!test_bit(EXTENT_BUFFER_UNMAPPED, &eb->bflags));
   5790          ASSERT(PagePrivate(page));
   5791          io_tree = &BTRFS_I(fs_info->btree_inode)->io_tree;
   5792
   5793          if (wait == WAIT_NONE) {
   5794                  ret = try_lock_extent(io_tree, eb->start,
   5795                                        eb->start + eb->len - 1);
   5796                  if (ret <= 0)
   5797                          return ret;

If try_lock_extent() fails to get the lock and returns 0, then is
returning zero here really the correct behavior?

This is the same behavior of read_extent_buffer_pages() for regular
sector size:

int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int
mirror_num)
{
        ...
        int ret = 0;
        ...
                num_pages = num_extent_pages(eb);
        for (i = 0; i < num_pages; i++) {
                page = eb->pages[i];
                if (wait == WAIT_NONE) {
                        if (!trylock_page(page))
                                goto unlock_exit; <<<<
        ...
unlock_exit:
        while (locked_pages > 0) {
                locked_pages--;
                page = eb->pages[locked_pages];
                unlock_page(page);
        }
        return ret;
}

Here when we hit trylock_page() == false case, we directly go
unlock_exit, and by that time, @ret is still 0.


I'm not yet confident enough to say why it's OK, but my initial guess
is, we won't have (wait == WAIT_NONE) case for metadata read.

Thank you for the hint, I'll take more time to make sure the original
behavior is correct, and if it's really (wait == WAIT_NONE) will never
be true for metadata, I'll send out cleanup for this.

Thanks,
Qu

 It feels like there
should be some documentation because this behavior is unexpected.

   5798          } else {
   5799                  ret = lock_extent(io_tree, eb->start, eb->start + 
eb->len - 1);
   5800                  if (ret < 0)
   5801                          return ret;
   5802          }
   5803
   5804          ret = 0;
   5805          if (test_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags) ||
   5806              PageUptodate(page) ||
   5807              btrfs_subpage_test_uptodate(fs_info, page, eb->start, 
eb->len)) {
   5808                  set_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags);
   5809                  unlock_extent(io_tree, eb->start, eb->start + eb->len 
- 1);
   5810                  return ret;
   5811          }

regards,
dan carpenter