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
