Re: [f2fs-dev] [PATCH 07/16] fsverity: don't issue readahead for non-ENOENT errors from __filemap_get_folio
On Tue, Jan 27, 2026 at 07:38:09AM +0100, Christoph Hellwig wrote: > PTR_ERR(ptr) == -EFOO checks if ptr is an error pointer for the errno > value -EFOO. To reiterate (again): when ptr may or may not be an error pointer, it should be written as ptr == ERR_PTR(-EFOO), as is normally done. Otherwise an error code is being extracted from something that doesn't have an error code, which is nonsense, even if it works by accident. - Eric ___ Linux-f2fs-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 07/16] fsverity: don't issue readahead for non-ENOENT errors from __filemap_get_folio
On Mon, Jan 26, 2026 at 10:28:49PM -0800, Darrick J. Wong wrote: > > Yes, it's really just a cast, and 'PTR_ERR(folio) == -ENOENT' actually > > still works when folio isn't necessarily an error pointer. But normally > > it would be written as a pointer comparison as I suggested. > > How does one know that a pointer is an error pointer? Oughtn't there be > some kind of obvious marker, or is IS_ERR the only tool we've got? IS_ERR(ptr) is the interface to check is a pointer is an error pointer or not. PTR_ERR(ptr) == -EFOO checks if ptr is an error pointer for the errno value -EFOO. ___ Linux-f2fs-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 07/16] fsverity: don't issue readahead for non-ENOENT errors from __filemap_get_folio
On Mon, Jan 26, 2026 at 10:20:55PM -0800, Eric Biggers wrote:
> > That's new to me, and I can't find anything in the documentation or
> > implementation suggesting that. Your example code above also does
> > this as does plenty of code in the kernel elsewhere.
>
> Not sure why this is controversial.
It wasn't controversial until you came up with that claim.
> The documentation for PTR_ERR() is
> clear that it's for error pointers:
Yes, but anything that stores an ERR_PTR is an error pointer. There
never has been any explicit requirement to first call IS_ERR.
One very common pattern is to extract it first an then check
for errors like:
error = PTR_ERR(ptr);
if (IS_ERR(ptr)))
goto handler_error;
one could come up with arguments that this is special, because error
is not used until after the branch. But there's plenty of other code
like:
type = alg_get_type(sa->salg_type);
if (PTR_ERR(type) == -ENOENT) {
request_module("algif-%s", sa->salg_type);
type = alg_get_type(sa->salg_type);
}
if (IS_ERR(type))
return PTR_ERR(type);
> * PTR_ERR - Extract the error code from an error pointer.
> * @ptr: An error pointer.
> * Return: The error code within @ptr.
> */
> static inline long __must_check PTR_ERR(__force const void *ptr)
> {
> return (long) ptr;
> }
>
> Yes, it's really just a cast, and 'PTR_ERR(folio) == -ENOENT' actually
> still works when folio isn't necessarily an error pointer. But normally
> it would be written as a pointer comparison as I suggested.
You suggestion is using PTR_ERR before checking, to quote from the
previous mail:
> Or as a diff from this series:
>
> - if (PTR_ERR(folio) == -ENOENT ||
> - !(IS_ERR(folio) && !folio_test_uptodate(folio))) {
> + if (folio == ERR_PTR(-ENOENT) ||
> + (!IS_ERR(folio) && !folio_test_uptodate(folio))) {
___
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 07/16] fsverity: don't issue readahead for non-ENOENT errors from __filemap_get_folio
On Mon, Jan 26, 2026 at 10:20:55PM -0800, Eric Biggers wrote:
> On Tue, Jan 27, 2026 at 07:00:39AM +0100, Christoph Hellwig wrote:
> > > - if (PTR_ERR(folio) == -ENOENT ||
> > > - !(IS_ERR(folio) && !folio_test_uptodate(folio))) {
> > > + if (folio == ERR_PTR(-ENOENT) ||
> > > + (!IS_ERR(folio) && !folio_test_uptodate(folio))) {
> > >
> > > (Note that PTR_ERR() shouldn't be used before it's known that the
> > > pointer is an error pointer.)
> >
> > That's new to me, and I can't find anything in the documentation or
> > implementation suggesting that. Your example code above also does
> > this as does plenty of code in the kernel elsewhere.
>
> Not sure why this is controversial. The documentation for PTR_ERR() is
> clear that it's for error pointers:
>
> /**
> * PTR_ERR - Extract the error code from an error pointer.
> * @ptr: An error pointer.
> * Return: The error code within @ptr.
> */
> static inline long __must_check PTR_ERR(__force const void *ptr)
> {
> return (long) ptr;
> }
>
> Yes, it's really just a cast, and 'PTR_ERR(folio) == -ENOENT' actually
> still works when folio isn't necessarily an error pointer. But normally
> it would be written as a pointer comparison as I suggested.
How does one know that a pointer is an error pointer? Oughtn't there be
some kind of obvious marker, or is IS_ERR the only tool we've got?
--D
> - Eric
___
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 07/16] fsverity: don't issue readahead for non-ENOENT errors from __filemap_get_folio
On Tue, Jan 27, 2026 at 07:00:39AM +0100, Christoph Hellwig wrote:
> > - if (PTR_ERR(folio) == -ENOENT ||
> > - !(IS_ERR(folio) && !folio_test_uptodate(folio))) {
> > + if (folio == ERR_PTR(-ENOENT) ||
> > + (!IS_ERR(folio) && !folio_test_uptodate(folio))) {
> >
> > (Note that PTR_ERR() shouldn't be used before it's known that the
> > pointer is an error pointer.)
>
> That's new to me, and I can't find anything in the documentation or
> implementation suggesting that. Your example code above also does
> this as does plenty of code in the kernel elsewhere.
Not sure why this is controversial. The documentation for PTR_ERR() is
clear that it's for error pointers:
/**
* PTR_ERR - Extract the error code from an error pointer.
* @ptr: An error pointer.
* Return: The error code within @ptr.
*/
static inline long __must_check PTR_ERR(__force const void *ptr)
{
return (long) ptr;
}
Yes, it's really just a cast, and 'PTR_ERR(folio) == -ENOENT' actually
still works when folio isn't necessarily an error pointer. But normally
it would be written as a pointer comparison as I suggested.
- Eric
___
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 07/16] fsverity: don't issue readahead for non-ENOENT errors from __filemap_get_folio
On Mon, Jan 26, 2026 at 12:53:01PM -0800, Eric Biggers wrote:
> Then for the final version in generic_readahead_merkle_tree(), one
> option would be:
>
> struct folio *folio;
>
> folio = __filemap_get_folio(inode->i_mapping, index, FGP_ACCESSED, 0);
> if (folio == ERR_PTR(-ENOENT) ||
> (!IS_ERR(folio) && !folio_test_uptodate(folio))) {
> DEFINE_READAHEAD(ractl, NULL, NULL, inode->i_mapping, index);
>
> page_cache_ra_unbounded(&ractl, nr_pages, 0);
> }
> if (!IS_ERR(folio))
> folio_put(folio);
>
> Or as a diff from this series:
I ended up doing the second version (which is what I intended to do
anyway, but messed up the brace placement) in this patch. It then
automatically carries over to the readahead split.
>
> - if (PTR_ERR(folio) == -ENOENT ||
> - !(IS_ERR(folio) && !folio_test_uptodate(folio))) {
> + if (folio == ERR_PTR(-ENOENT) ||
> + (!IS_ERR(folio) && !folio_test_uptodate(folio))) {
>
> (Note that PTR_ERR() shouldn't be used before it's known that the
> pointer is an error pointer.)
That's new to me, and I can't find anything in the documentation or
implementation suggesting that. Your example code above also does
this as does plenty of code in the kernel elsewhere.
___
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 07/16] fsverity: don't issue readahead for non-ENOENT errors from __filemap_get_folio
On Mon, Jan 26, 2026 at 11:11:02AM -0800, Darrick J. Wong wrote:
> On Mon, Jan 26, 2026 at 05:50:53AM +0100, Christoph Hellwig wrote:
> > Issuing more reads on errors is not a good idea, especially when the
> > most common error here is -ENOMEM.
> >
> > Signed-off-by: Christoph Hellwig
> > ---
> > fs/verity/pagecache.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/verity/pagecache.c b/fs/verity/pagecache.c
> > index 1efcdde20b73..63393f0f5834 100644
> > --- a/fs/verity/pagecache.c
> > +++ b/fs/verity/pagecache.c
> > @@ -22,7 +22,8 @@ struct page *generic_read_merkle_tree_page(struct inode
> > *inode, pgoff_t index,
> > 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 ||
> > + !(IS_ERR(folio) && !folio_test_uptodate(folio))) {
>
> I don't understand this logic at all. If @folio is actually an
> ERR_PTR, then we dereference the non-folio to see if it's not uptodate?
>
> I think (given the previous revisions) that what you want is to initiate
> readahead if either there's no folio at all (ENOENT) or if there is a
> folio but it's not uptodate? But not if there's some other error
> (ENOMEM, EL3HLT, EFSCORRUPTED, etc)?
>
> So maybe you want:
>
> folio = __filemap_get_folio(...);
> if (!IS_ERR(folio)) {
> if (folio_test_uptodate(folio))
> return folio_file_page(folio);
> folio_put(folio);
> } else if (PTR_ERR(folio) == -ENOENT) {
> return ERR_CAST(folio);
> }
>
> if (num_ra_pages > 1)
> page_cache_ra_unbounded(&ractl, num_ra_pages, 0);
> folio = read_mapping_folio(inode->i_mapping, index, NULL);
> if (IS_ERR(folio))
> return ERR_CAST(folio);
>
> return folio_file_page(folio);
>
>
That version is wrong too: the condition 'PTR_ERR(folio) == -ENOENT' is
backwards.
This code gets replaced later in the series anyway. For this patch, we
could simply insert two lines:
folio = __filemap_get_folio(inode->i_mapping, index, FGP_ACCESSED, 0);
+ if (IS_ERR(folio) && folio != ERR_PTR(-ENOENT))
+ return folio;
Then for the final version in generic_readahead_merkle_tree(), one
option would be:
struct folio *folio;
folio = __filemap_get_folio(inode->i_mapping, index, FGP_ACCESSED, 0);
if (folio == ERR_PTR(-ENOENT) ||
(!IS_ERR(folio) && !folio_test_uptodate(folio))) {
DEFINE_READAHEAD(ractl, NULL, NULL, inode->i_mapping, index);
page_cache_ra_unbounded(&ractl, nr_pages, 0);
}
if (!IS_ERR(folio))
folio_put(folio);
Or as a diff from this series:
- if (PTR_ERR(folio) == -ENOENT ||
- !(IS_ERR(folio) && !folio_test_uptodate(folio))) {
+ if (folio == ERR_PTR(-ENOENT) ||
+ (!IS_ERR(folio) && !folio_test_uptodate(folio))) {
(Note that PTR_ERR() shouldn't be used before it's known that the
pointer is an error pointer.)
- Eric
___
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 07/16] fsverity: don't issue readahead for non-ENOENT errors from __filemap_get_folio
On Mon, Jan 26, 2026 at 05:50:53AM +0100, Christoph Hellwig wrote:
> Issuing more reads on errors is not a good idea, especially when the
> most common error here is -ENOMEM.
>
> Signed-off-by: Christoph Hellwig
> ---
> fs/verity/pagecache.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/verity/pagecache.c b/fs/verity/pagecache.c
> index 1efcdde20b73..63393f0f5834 100644
> --- a/fs/verity/pagecache.c
> +++ b/fs/verity/pagecache.c
> @@ -22,7 +22,8 @@ struct page *generic_read_merkle_tree_page(struct inode
> *inode, pgoff_t index,
> 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 ||
> + !(IS_ERR(folio) && !folio_test_uptodate(folio))) {
I don't understand this logic at all. If @folio is actually an
ERR_PTR, then we dereference the non-folio to see if it's not uptodate?
I think (given the previous revisions) that what you want is to initiate
readahead if either there's no folio at all (ENOENT) or if there is a
folio but it's not uptodate? But not if there's some other error
(ENOMEM, EL3HLT, EFSCORRUPTED, etc)?
So maybe you want:
folio = __filemap_get_folio(...);
if (!IS_ERR(folio)) {
if (folio_test_uptodate(folio))
return folio_file_page(folio);
folio_put(folio);
} else if (PTR_ERR(folio) == -ENOENT) {
return ERR_CAST(folio);
}
if (num_ra_pages > 1)
page_cache_ra_unbounded(&ractl, num_ra_pages, 0);
folio = read_mapping_folio(inode->i_mapping, index, NULL);
if (IS_ERR(folio))
return ERR_CAST(folio);
return folio_file_page(folio);
--D
> DEFINE_READAHEAD(ractl, NULL, NULL, inode->i_mapping, index);
>
> if (!IS_ERR(folio))
> --
> 2.47.3
>
>
___
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
