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

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

On 2021/1/28 下午7:06, Qu Wenruo wrote:
> 
> 
> 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.

Facepalm, I should check the code before hitting send.

The WAIT_NONE case is for readahead, thus we are completely fine not to 
read the tree block and just return 0.

For real tree reads, we always have WAIT_COMPLETE.

But still, I'll add some comment on the original code to explain why 
we're safe to return 0 directly if we can't lock the page directly.

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