On Sat, Jan 24, 2026 at 12:53:29PM -0800, Eric Biggers wrote:
> > +void generic_readahead_merkle_tree(struct inode *inode, pgoff_t index,
> > + unsigned long nr_pages)
> > {
> > struct folio *folio;
> >
> > folio = __filemap_get_folio(inode->i_mapping, index, FGP_ACCESSED, 0);
> > - if (IS_ERR(folio) || !folio_test_uptodate(folio)) {
> > + if (PTR_ERR(folio) == -ENOENT || !folio_test_uptodate(folio)) {
>
> This dereferences an ERR_PTR() when __filemap_get_folio() returns an
> error other than -ENOENT.
Yes. I've fixed that and split the change in error handling into a
separate well described prep patch while at it.
> I think the correct thing to do here would be the following:
>
> if (inode->i_sb->s_vop->readahead_merkle_tree)
> inode->i_sb->s_vop->readahead_merkle_tree(inode, index,
> last_index - index +
> 1);
>
> Then __fsverity_readahead() can be folded into fsverity_readahead().
I've done that, and also added a little comment.
>
> > +void __fsverity_readahead(struct inode *inode, const struct fsverity_info
> > *vi,
> > + loff_t data_start_pos, unsigned long nr_pages)
> > +{
> > + const struct merkle_tree_params *params = &vi->tree_params;
> > + u64 start_hidx = data_start_pos >> params->log_blocksize;
> > + u64 end_hidx = (data_start_pos + ((nr_pages - 1) << PAGE_SHIFT)) >>
> > + params->log_blocksize;
>
> (nr_pages - 1) << PAGE_SHIFT can overflow an 'unsigned long'.
> (nr_pages - 1) needs to be cast to u64 before doing the shift.
>
> But also it would make more sense to pass
> (pgoff_t start_index, unsigned long nr_pages) instead of
> (loff_t data_start_pos, unsigned long nr_pages),
> so that the two numbers have the same units.
>
> start_idx and end_hidx could then be computed as follows:
>
> u64 start_hidx = (u64)start_index << params->log_blocks_per_page;
> u64 end_hidx = (((u64)start_index + nr_pages) <<
> params->log_blocks_per_page) - 1;
>
> Note that fsverity_readahead() derives the position from the index. If
> it just used the index directly, that would be more direct.
Yes, I've updated that. Having proper types and/or conversion helpers
for the fsverity specific addressing would be really nice as well,
but I've not touched that for now.
> > + int level;
> > +
> > + if (!inode->i_sb->s_vop->readahead_merkle_tree)
> > + return;
> > + if (unlikely(data_start_pos >= inode->i_size))
> > + return;
>
> The check against i_size shouldn't be necessary: the caller should just
> call this only for data it's actually going to read.
This check is based on / copied from the check in verify_data_block.
Now we only kick off readahead now and and don't actually do anything
with the read blocks, so I'll take your word that this can be removed.
> > + for (level = 0; level < params->num_levels; level++) {
> > + unsigned long level_start = params->level_start[level];
> > + unsigned long next_start_hidx = start_hidx >> params->log_arity;
> > + unsigned long next_end_hidx = end_hidx >> params->log_arity;
> > + unsigned long start_idx = (level_start + next_start_hidx) >>
> > + params->log_blocks_per_page;
> > + unsigned long end_idx = (level_start + next_end_hidx) >>
> > + params->log_blocks_per_page;
>
> start_idx and end_idx should have type pgoff_t to make it clear that
> they're page indices.
Fixed.
> > +/**
> > + * fsverity_readahead() - kick off readahead on fsverity hashes
> > + * @folio: first folio that is being read
>
> folio => file data folio
>
> Otherwise it can be confused with the Merkle tree.
I've incoroporate the various suggested documentation improvements.
Thanks!
_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
