Re: [PATCH] ceph: Fix error handling in fill_readdir_cache()

[Matthew Wilcox <willy@infradead.org>]

On Tue, Mar 04, 2025 at 06:41:46PM +0000, Viacheslav Dubeyko wrote:
> 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@linaro.org>
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > Cc: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> > ---
> >  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.

There's no way to get a NULL pointer here.  __filemap_get_folio() always
returns a valid folio or an ERR_PTR.

> > -		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. :)

We'd get a very visible and obvious splat if we did!  But we make this
assumption all over the VFS and in other filesystems.  There's no need
to be more cautious in ceph than in other places.