It was introduced by commit 73bb49da50cd460bb3ba31250ed2e7fbf2115edf In function ext4_read_merkle_tree_page and f2fs_read_merkle_tree_page, parameter index was added by a offset and then passed to page_cache_readahead_unbounded. For now, index is passed to DEFINE_READAHEAD directly with wrong value. Possible fix is not changing the value of parameter index and defining a new local variable ra_index to replace all the other reference to index. Like: static struct page *ext4_read_merkle_tree_page(struct inode *inode, pgoff_t index, unsigned long num_ra_pages) { pgoff_t ra_index = index + ext4_verity_metadata_pos(inode) >> PAGE_SHIFT; DEFINE_READAHEAD(ractl, NULL, NULL, inode->i_mapping, ra_index); struct page *page; page = find_get_page_flags(inode->i_mapping, ra_index, FGP_ACCESSED); if (!page || !PageUptodate(page)) { if (page) put_page(page); else if (num_ra_pages > 1) page_cache_ra_unbounded(&ractl, num_ra_pages, 0); page = read_mapping_page(inode->i_mapping, ra_index, NULL); } return page; } static struct page *f2fs_read_merkle_tree_page(struct inode *inode, pgoff_t index, unsigned long num_ra_pages) { pgoff_t ra_index = index + f2fs_verity_metadata_pos(inode) >> PAGE_SHIFT; DEFINE_READAHEAD(ractl, NULL, NULL, inode->i_mapping, ra_index); struct page *page; page = find_get_page_flags(inode->i_mapping, ra_index, FGP_ACCESSED); if (!page || !PageUptodate(page)) { if (page) put_page(page); else if (num_ra_pages > 1) page_cache_ra_unbounded(&ractl, num_ra_pages, 0); page = read_mapping_page(inode->i_mapping, ra_index, NULL); } return page; }
static struct page *ext4_read_merkle_tree_page(struct inode *inode, pgoff_t index, unsigned long num_ra_pages) { pgoff_t ra_index = index + (ext4_verity_metadata_pos(inode) >> PAGE_SHIFT); DEFINE_READAHEAD(ractl, NULL, NULL, inode->i_mapping, ra_index); struct page *page; page = find_get_page_flags(inode->i_mapping, ra_index, FGP_ACCESSED); if (!page || !PageUptodate(page)) { if (page) put_page(page); else if (num_ra_pages > 1) page_cache_ra_unbounded(&ractl, num_ra_pages, 0); page = read_mapping_page(inode->i_mapping, ra_index, NULL); } return page; } static struct page *f2fs_read_merkle_tree_page(struct inode *inode, pgoff_t index, unsigned long num_ra_pages) { pgoff_t ra_index = index + (f2fs_verity_metadata_pos(inode) >> PAGE_SHIFT); DEFINE_READAHEAD(ractl, NULL, NULL, inode->i_mapping, ra_index); struct page *page; page = find_get_page_flags(inode->i_mapping, ra_index, FGP_ACCESSED); if (!page || !PageUptodate(page)) { if (page) put_page(page); else if (num_ra_pages > 1) page_cache_ra_unbounded(&ractl, num_ra_pages, 0); page = read_mapping_page(inode->i_mapping, ra_index, NULL); } return page; } Add parentheses to make the precedence right.
(switched to email. Please respond via emailed reply-to-all, not via the bugzilla web interface). On Tue, 11 Oct 2022 06:52:49 +0000 bugzilla-daemon@kernel.org wrote: > https://bugzilla.kernel.org/show_bug.cgi?id=216570 > > Bug ID: 216570 > Summary: Pass wrong index value to DEFINE_READAHEAD which leads > to almost no readahead of fsverity data > Product: Memory Management > Version: 2.5 > Kernel Version: 6.0 > Hardware: All > OS: Linux > Tree: Mainline > Status: NEW > Severity: low > Priority: P1 > Component: Other > Assignee: akpm@linux-foundation.org > Reporter: nicememory@gmail.com > Regression: No > > It was introduced by commit 73bb49da50cd460bb3ba31250ed2e7fbf2115edf > > In function ext4_read_merkle_tree_page and f2fs_read_merkle_tree_page, > parameter index was added by a offset and then passed to > page_cache_readahead_unbounded. > For now, index is passed to DEFINE_READAHEAD directly with wrong value. oops, yes, thanks. Matthew, could you please take a look? > > Possible fix is not changing the value of parameter index and defining a new > local variable ra_index to replace all the other reference to index. Like: > > static struct page *ext4_read_merkle_tree_page(struct inode *inode, > pgoff_t index, > unsigned long num_ra_pages) > { > pgoff_t ra_index = index + ext4_verity_metadata_pos(inode) >> > PAGE_SHIFT; > DEFINE_READAHEAD(ractl, NULL, NULL, inode->i_mapping, ra_index); > struct page *page; > > page = find_get_page_flags(inode->i_mapping, ra_index, FGP_ACCESSED); > if (!page || !PageUptodate(page)) { > if (page) > put_page(page); > else if (num_ra_pages > 1) > page_cache_ra_unbounded(&ractl, num_ra_pages, 0); > page = read_mapping_page(inode->i_mapping, ra_index, NULL); > } > return page; > } > > > static struct page *f2fs_read_merkle_tree_page(struct inode *inode, > pgoff_t index, > unsigned long num_ra_pages) > { > pgoff_t ra_index = index + f2fs_verity_metadata_pos(inode) >> > PAGE_SHIFT; > DEFINE_READAHEAD(ractl, NULL, NULL, inode->i_mapping, ra_index); > struct page *page; > > page = find_get_page_flags(inode->i_mapping, ra_index, FGP_ACCESSED); > if (!page || !PageUptodate(page)) { > if (page) > put_page(page); > else if (num_ra_pages > 1) > page_cache_ra_unbounded(&ractl, num_ra_pages, 0); > page = read_mapping_page(inode->i_mapping, ra_index, NULL); > } > return page; > } >
On Tue, Oct 11, 2022 at 05:21:19PM -0700, Andrew Morton wrote: > > It was introduced by commit 73bb49da50cd460bb3ba31250ed2e7fbf2115edf > > > > In function ext4_read_merkle_tree_page and f2fs_read_merkle_tree_page, > > parameter index was added by a offset and then passed to > > page_cache_readahead_unbounded. > > For now, index is passed to DEFINE_READAHEAD directly with wrong value. > > oops, yes, thanks. Matthew, could you please take a look? Oh, yes, oops indeed. I'll send a fix in the morning.