Re: [PATCH 07/10] fuse: enable iomap cache management

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Feb 05, 2026 at 10:33:26AM -0800, Chris Mason wrote:
> "Darrick J. Wong" <djwong@kernel.org> wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Provide a means for the fuse server to upload iomappings to the kernel
> > and invalidate them.  This is how we enable iomap caching for better
> > performance.  This is also required for correct synchronization between
> > pagecache writes and writeback.
> > 
> 
> Hi everyone,
> 
> I'm trying out my AI review prompts on a few more trees, and I ran it
> on the fuse-iomap-cache branch:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=fuse-iomap-cache
> 
> This was the latest email lore could find, apologies if I grabbed the wrong one.
> 
> Hopefully the review below is useful, please let me know if there are places it
> has gone wildly wrong.  I kept the header to make it obvious exactly which
> commit I'd run through:
> 
> commit 84c726dd312b652c0fd2d4f43d65eefd16bfa901
> Author: Darrick J. Wong <djwong@kernel.org>
> 
> fuse: enable iomap cache management
> 
> This commit provides a means for the fuse server to upload iomappings to
> the kernel and invalidate them, enabling iomap caching for better
> performance and ensuring correct synchronization between pagecache writes
> and writeback.
> 
> > diff --git a/fs/fuse/fuse_iomap.c b/fs/fuse/fuse_iomap.c
> > index b56718dd256a6..dcadd66f6ec95 100644
> > --- a/fs/fuse/fuse_iomap.c
> > +++ b/fs/fuse/fuse_iomap.c
> 
> [ ... ]
> 
> > +int fuse_iomap_inval(struct fuse_conn *fc,
> > +		     const struct fuse_iomap_inval_out *outarg)
> > +{
> > +	struct inode *inode;
> > +	struct fuse_inode *fi;
> > +	int ret = 0, ret2 = 0;
> > +
> > +	if (!fc->iomap)
> > +		return -EINVAL;
> > +
> > +	down_read(&fc->killsb);
> > +	inode = fuse_ilookup(fc, outarg->nodeid, NULL);
> > +	if (!inode) {
> > +		ret = -ESTALE;
> > +		goto out_sb;
> > +	}
> > +
> > +	fi = get_fuse_inode(inode);
> > +	if (BAD_DATA(fi->orig_ino != outarg->attr_ino)) {
> > +		ret = -EINVAL;
> > +		goto out_inode;
> > +	}
> > +
> > +	if (fuse_is_bad(inode)) {
> > +		ret = -EIO;
> > +		goto out_inode;
> > +	}
> > +
> > +	if (!fuse_iomap_inval_validate(inode, outarg->write_offset,
> > +				       outarg->write_length)) {
> > +		ret = -EFSCORRUPTED;
> > +		goto out_inode;
> > +	}
> > +
> > +	if (!fuse_iomap_inval_validate(inode, outarg->read_offset,
> > +				       outarg->read_length)) {
> > +		ret = -EFSCORRUPTED;
> > +		goto out_inode;
> > +	}
> > +
> > +	fuse_iomap_cache_lock(inode);
> > +	if (outarg->read_length)
> > +		ret2 = fuse_iomap_cache_remove(inode, READ_MAPPING,
> > +					       outarg->read_offset,
> > +					       outarg->read_length);
> > +	if (outarg->write_length)
> > +		ret = fuse_iomap_cache_remove(inode, WRITE_MAPPING,
> > +					      outarg->write_offset,
> > +					      outarg->write_length);
> > +	fuse_iomap_cache_unlock(inode);
> >                    ^^^^
> 
> Can fuse_iomap_cache_lock() dereference a NULL pointer here? Looking at

Yes.  For those of you reading the parent message you might be confused
because the 29 Oct posting embedded the iomap cache directly in struct
fuse_inode so there was no possibility of a NULL pointer dereference.

Based on feedback from Joanne I changed fuse_inode to point to a
fuse_iomap_cache object, which the AI has now caught.  Excellent!

> fuse_iomap_upsert() above, there's a check for fuse_inode_caches_iomaps()
> before accessing the cache and allocating if necessary. In fuse_iomap_inval(),
> there's no such check before calling fuse_iomap_cache_lock().
> 
> fuse_iomap_cache_lock() accesses fi->cache and calls down_write(&ic->ic_lock),
> which will crash if fi->cache is NULL. This could happen when a fuse server
> sends FUSE_NOTIFY_IOMAP_INVAL for an inode that never had mappings upserted
> to it.
> 
> Should fuse_iomap_inval() check fuse_inode_caches_iomaps(inode) before
> calling fuse_iomap_cache_lock()? If the cache doesn't exist, there's
> nothing to invalidate, so returning early (perhaps with 0 or -EINVAL)
> would be appropriate.

I'll have it return 0 because no cache means cache invalidation is a
nop.  Thanks for your review help, Chris!

--D