* [bug report] btrfs: introduce read_extent_buffer_subpage()
@ 2021-01-28 10:50 Dan Carpenter
2021-01-28 11:06 ` Qu Wenruo
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2021-01-28 10:50 UTC (permalink / raw)
To: wqu; +Cc: linux-btrfs
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? 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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] btrfs: introduce read_extent_buffer_subpage() 2021-01-28 10:50 [bug report] btrfs: introduce read_extent_buffer_subpage() Dan Carpenter @ 2021-01-28 11:06 ` Qu Wenruo 2021-01-28 11:10 ` Qu Wenruo 0 siblings, 1 reply; 3+ messages in thread From: Qu Wenruo @ 2021-01-28 11:06 UTC (permalink / raw) To: Dan Carpenter, wqu; +Cc: linux-btrfs 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 > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] btrfs: introduce read_extent_buffer_subpage() 2021-01-28 11:06 ` Qu Wenruo @ 2021-01-28 11:10 ` Qu Wenruo 0 siblings, 0 replies; 3+ messages in thread From: Qu Wenruo @ 2021-01-28 11:10 UTC (permalink / raw) To: Dan Carpenter, wqu; +Cc: linux-btrfs 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 >> ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-01-28 11:12 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-01-28 10:50 [bug report] btrfs: introduce read_extent_buffer_subpage() Dan Carpenter 2021-01-28 11:06 ` Qu Wenruo 2021-01-28 11:10 ` Qu Wenruo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox