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

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

[Chris Mason]

"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
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.

Re: [PATCH 05/10] fuse: invalidate iomap cache after file updates

[Chris Mason]

"Darrick J. Wong" <djwong@kernel.org> wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> The kernel doesn't know what the fuse server might have done in response
> to truncate, fallocate, or ioend events.  Therefore, it must invalidate
> the mapping cache after those operations to ensure cache coherency.
> 

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 8cd55ddf79a7cd6b1c8afbe08b72d8896da67306
Author: Darrick J. Wong <djwong@kernel.org>

fuse: invalidate iomap cache after file updates

This commit fixes a cache coherency bug by adding cache invalidation
calls after file-modifying operations to prevent stale cached iomap
entries from being used.

> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c

[ ... ]

> @@ -3202,7 +3205,9 @@ struct fuse_copy_file_range_in inarg = {
>  		goto out;
>  	}
>
> -	if (!is_iomap)
> +	if (is_iomap)
> +		fuse_iomap_copied_file_range(inode_out, pos_out, outarg.size);

In __fuse_copy_file_range(), does this code pass the correct value to
fuse_iomap_copied_file_range()?

When using the FUSE_COPY_FILE_RANGE_64 API (fc->no_copy_file_range_64
is false), the response goes into 'outarg_64', not 'outarg'. The
'outarg' struct remains uninitialized in this path.

Looking at the code earlier in the function:

    if (fc->no_copy_file_range_64) {
        args.out_args[0].size = sizeof(outarg);
        args.out_args[0].value = &outarg;
    } else {
        args.out_args[0].size = sizeof(outarg_64);
        args.out_args[0].value = &outarg_64;
    }

    /* ... */

    if (args.out_args[0].value == &outarg_64)
        bytes_copied = outarg_64.size;
    else
        bytes_copied = outarg.size;

The 'bytes_copied' variable correctly handles both API versions. The
else branch (non-iomap path) already uses 'bytes_copied' for
truncate_inode_pages_range().

When the 64-bit API succeeds, could the cache invalidation use
garbage/zero data for the range instead of the actual bytes copied,
potentially leaving stale mappings in the cache?

> +	else
>  		truncate_inode_pages_range(inode_out->i_mapping,
>  				   ALIGN_DOWN(pos_out, PAGE_SIZE),
>  				   ALIGN(pos_out + bytes_copied, PAGE_SIZE) - 1);

Re: [PATCH 03/10] fuse: use the iomap cache for iomap_begin

[Chris Mason]

"Darrick J. Wong" <djwong@kernel.org> wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Look inside the iomap cache to try to satisfy iomap_begin.
> 

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 705acf20a7890a2ba0ab24fe6bf523b99c1a1f32
Author: Darrick J. Wong <djwong@kernel.org>

fuse: use the iomap cache for iomap_begin

Look inside the iomap cache to try to satisfy iomap_begin.

> diff --git a/fs/fuse/fuse_iomap.c b/fs/fuse/fuse_iomap.c
> index eade8dad8a570..cf46824dc50bf 100644
> --- a/fs/fuse/fuse_iomap.c
> +++ b/fs/fuse/fuse_iomap.c

[ ... ]

> @@ -238,9 +239,14 @@ static inline bool fuse_iomap_check_mapping(const struct inode *inode,
>  	const unsigned int blocksize = i_blocksize(inode);
>  	uint64_t end;
>
> -	/* Type and flags must be known */
> +	/*
> +	 * Type and flags must be known.  Mapping type "retry cache" doesn't
> +	 * use any of the other fields.
> +	 */
>  	if (BAD_DATA(!fuse_iomap_check_type(map->type)))
>  		return false;
> +	if (map->type == FUSE_IOMAP_TYPE_RETRY_CACHE)
> +		return true;
>  	if (BAD_DATA(!fuse_iomap_check_flags(map->flags)))
>  		return false;

[ ... ]

> @@ -271,6 +277,14 @@ static inline bool fuse_iomap_check_mapping(const struct inode *inode,
>  		if (BAD_DATA(map->addr == FUSE_IOMAP_NULL_ADDR))
>  			return false;
>  		break;
> +	case FUSE_IOMAP_TYPE_RETRY_CACHE:
> +		/*
> +		 * We only accept cache retries if we have a cache to query.
> +		 * There must not be a device addr.
> +		 */
> +		if (BAD_DATA(!fuse_inode_caches_iomaps(inode)))
> +			return false;
> +		fallthrough;
>  	case FUSE_IOMAP_TYPE_DELALLOC:
>  	case FUSE_IOMAP_TYPE_HOLE:
>  	case FUSE_IOMAP_TYPE_INLINE:

Does the validation at lines 280-287 ever execute? Looking at
fuse_iomap_check_mapping(), the early return at line 248-249 handles
FUSE_IOMAP_TYPE_RETRY_CACHE:

    if (map->type == FUSE_IOMAP_TYPE_RETRY_CACHE)
        return true;

This means the function returns before reaching the switch statement that
contains the new RETRY_CACHE case. The validation check 'if (BAD_DATA(!
fuse_inode_caches_iomaps(inode))) return false;' at lines 284-285 appears
to be dead code because RETRY_CACHE mappings are accepted without verifying
the inode has a cache, contradicting the comment "We only accept cache
retries if we have a cache to query."

Either the fuse_inode_caches_iomaps check should move to the early return
path like this:

    if (map->type == FUSE_IOMAP_TYPE_RETRY_CACHE) {
        if (BAD_DATA(!fuse_inode_caches_iomaps(inode)))
            return false;
        return true;
    }

or the early return should be removed to let RETRY_CACHE fall through to
the switch statement validation.

Re: [PATCH 03/10] fuse: use the iomap cache for iomap_begin

[Darrick J. Wong]

On Thu, Feb 05, 2026 at 10:52:04AM -0800, Chris Mason wrote:
> "Darrick J. Wong" <djwong@kernel.org> wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Look inside the iomap cache to try to satisfy iomap_begin.
> > 
> 
> 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 705acf20a7890a2ba0ab24fe6bf523b99c1a1f32
> Author: Darrick J. Wong <djwong@kernel.org>
> 
> fuse: use the iomap cache for iomap_begin
> 
> Look inside the iomap cache to try to satisfy iomap_begin.
> 
> > diff --git a/fs/fuse/fuse_iomap.c b/fs/fuse/fuse_iomap.c
> > index eade8dad8a570..cf46824dc50bf 100644
> > --- a/fs/fuse/fuse_iomap.c
> > +++ b/fs/fuse/fuse_iomap.c
> 
> [ ... ]
> 
> > @@ -238,9 +239,14 @@ static inline bool fuse_iomap_check_mapping(const struct inode *inode,
> >  	const unsigned int blocksize = i_blocksize(inode);
> >  	uint64_t end;
> >
> > -	/* Type and flags must be known */
> > +	/*
> > +	 * Type and flags must be known.  Mapping type "retry cache" doesn't
> > +	 * use any of the other fields.
> > +	 */
> >  	if (BAD_DATA(!fuse_iomap_check_type(map->type)))
> >  		return false;
> > +	if (map->type == FUSE_IOMAP_TYPE_RETRY_CACHE)
> > +		return true;
> >  	if (BAD_DATA(!fuse_iomap_check_flags(map->flags)))
> >  		return false;
> 
> [ ... ]
> 
> > @@ -271,6 +277,14 @@ static inline bool fuse_iomap_check_mapping(const struct inode *inode,
> >  		if (BAD_DATA(map->addr == FUSE_IOMAP_NULL_ADDR))
> >  			return false;
> >  		break;
> > +	case FUSE_IOMAP_TYPE_RETRY_CACHE:
> > +		/*
> > +		 * We only accept cache retries if we have a cache to query.
> > +		 * There must not be a device addr.
> > +		 */
> > +		if (BAD_DATA(!fuse_inode_caches_iomaps(inode)))
> > +			return false;
> > +		fallthrough;
> >  	case FUSE_IOMAP_TYPE_DELALLOC:
> >  	case FUSE_IOMAP_TYPE_HOLE:
> >  	case FUSE_IOMAP_TYPE_INLINE:
> 
> Does the validation at lines 280-287 ever execute? Looking at
> fuse_iomap_check_mapping(), the early return at line 248-249 handles
> FUSE_IOMAP_TYPE_RETRY_CACHE:
> 
>     if (map->type == FUSE_IOMAP_TYPE_RETRY_CACHE)
>         return true;

Indeed, this means we can't ever hit the RETRY_CACHE case in the switch
statement.

> This means the function returns before reaching the switch statement that
> contains the new RETRY_CACHE case. The validation check 'if (BAD_DATA(!
> fuse_inode_caches_iomaps(inode))) return false;' at lines 284-285 appears
> to be dead code because RETRY_CACHE mappings are accepted without verifying
> the inode has a cache, contradicting the comment "We only accept cache
> retries if we have a cache to query."
> 
> Either the fuse_inode_caches_iomaps check should move to the early return
> path like this:
> 
>     if (map->type == FUSE_IOMAP_TYPE_RETRY_CACHE) {
>         if (BAD_DATA(!fuse_inode_caches_iomaps(inode)))
>             return false;
>         return true;
>     }

That's the simplest resolution to the problem; I'll go with that.

> or the early return should be removed to let RETRY_CACHE fall through to
> the switch statement validation.

Nah, we definitely don't need to proceed with more validation.  Good
catch!

--D

Re: [PATCH 05/10] fuse: invalidate iomap cache after file updates

[Darrick J. Wong]

On Thu, Feb 05, 2026 at 10:44:40AM -0800, Chris Mason wrote:
> "Darrick J. Wong" <djwong@kernel.org> wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > The kernel doesn't know what the fuse server might have done in response
> > to truncate, fallocate, or ioend events.  Therefore, it must invalidate
> > the mapping cache after those operations to ensure cache coherency.
> > 
> 
> 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 8cd55ddf79a7cd6b1c8afbe08b72d8896da67306
> Author: Darrick J. Wong <djwong@kernel.org>
> 
> fuse: invalidate iomap cache after file updates
> 
> This commit fixes a cache coherency bug by adding cache invalidation
> calls after file-modifying operations to prevent stale cached iomap
> entries from being used.
> 
> > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > --- a/fs/fuse/file.c
> > +++ b/fs/fuse/file.c
> 
> [ ... ]
> 
> > @@ -3202,7 +3205,9 @@ struct fuse_copy_file_range_in inarg = {
> >  		goto out;
> >  	}
> >
> > -	if (!is_iomap)
> > +	if (is_iomap)
> > +		fuse_iomap_copied_file_range(inode_out, pos_out, outarg.size);
> 
> In __fuse_copy_file_range(), does this code pass the correct value to
> fuse_iomap_copied_file_range()?
> 
> When using the FUSE_COPY_FILE_RANGE_64 API (fc->no_copy_file_range_64
> is false), the response goes into 'outarg_64', not 'outarg'. The
> 'outarg' struct remains uninitialized in this path.
> 
> Looking at the code earlier in the function:
> 
>     if (fc->no_copy_file_range_64) {
>         args.out_args[0].size = sizeof(outarg);
>         args.out_args[0].value = &outarg;
>     } else {
>         args.out_args[0].size = sizeof(outarg_64);
>         args.out_args[0].value = &outarg_64;
>     }
> 
>     /* ... */
> 
>     if (args.out_args[0].value == &outarg_64)
>         bytes_copied = outarg_64.size;
>     else
>         bytes_copied = outarg.size;
> 
> The 'bytes_copied' variable correctly handles both API versions. The
> else branch (non-iomap path) already uses 'bytes_copied' for
> truncate_inode_pages_range().
> 
> When the 64-bit API succeeds, could the cache invalidation use
> garbage/zero data for the range instead of the actual bytes copied,
> potentially leaving stale mappings in the cache?

Yes, fuse_iomap_copied_file_range should have its @written parameter
changed to u64.  I forgot to make that amendment after
FUSE_COPY_FILE_RANGE_64 was introduced last cycle.  It is a very good
sign that your AI prompts can catch bitrot during development!

--D

> 
> > +	else
> >  		truncate_inode_pages_range(inode_out->i_mapping,
> >  				   ALIGN_DOWN(pos_out, PAGE_SIZE),
> >  				   ALIGN(pos_out + bytes_copied, PAGE_SIZE) - 1);
> 
> 

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

[Darrick J. Wong]

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