Re: [PATCH 05/11] fsverity: kick off hash readahead at data I/O submission time

[Christoph Hellwig <hch@lst.de>]

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!