On Tue, 2025-03-04 at 15:48 +0000, Matthew Wilcox (Oracle) wrote:
> __filemap_get_folio() returns an ERR_PTR, not NULL. There are extensive
> assumptions that ctl->folio is NULL, not an error pointer, so it seems
> better to fix this one place rather than change all the places which
> check ctl->folio.
>
> Fixes: baff9740bc8f ("ceph: Convert ceph_readdir_cache_control to store a folio")
> Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@xxxxxxxxxxxxx>
> Cc: Viacheslav Dubeyko <Slava.Dubeyko@xxxxxxx>
> ---
> fs/ceph/inode.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index c15970fa240f..6ac2bd555e86 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -1870,9 +1870,12 @@ static int fill_readdir_cache(struct inode *dir, struct dentry *dn,
>
> ctl->folio = __filemap_get_folio(&dir->i_data, pgoff,
> fgf, mapping_gfp_mask(&dir->i_data));
Could we expect to receive NULL here somehow? I assume we should receive valid
pointer or ERR_PTR always here.
> - if (!ctl->folio) {
> + if (IS_ERR(ctl->folio)) {
> + int err = PTR_ERR(ctl->folio);
> +
> + ctl->folio = NULL;
> ctl->index = -1;
> - return idx == 0 ? -ENOMEM : 0;
> + return idx == 0 ? err : 0;
> }
> /* reading/filling the cache are serialized by
> * i_rwsem, no need to use folio lock */
But I prefer to check on NULL anyway, because we try to unlock the folio here:
/* reading/filling the cache are serialized by
* i_rwsem, no need to use folio lock */
folio_unlock(ctl->folio);
And absence of check on NULL makes me slightly nervous. :)
Thanks,
Slava.
