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

2021年1月28日 03:14:06 -0800 

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

Reply via email to