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

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

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
>