[PATCH 0/1] Fuse Passthrough cache issues

[PATCH 0/1] Fuse Passthrough cache issues

[Daniel Rosenberg]

I've been attempting to recreate Android's usage of Fuse Passthrough with the
version now merged in the kernel, and I've run into a couple issues. The first
one was pretty straightforward, and I've included a patch, although I'm not
convinced that it should be conditional, and it may need to do more to ensure
that the cache is up to date.

If your fuse daemon is running with writeback cache enabled, writes with
passthrough files will cause problems. Fuse will invalidate attributes on
write, but because it's in writeback cache mode, it will ignore the requested
attributes when the daemon provides them. The kernel is the source of truth in
this case, and should update the cached values during the passthrough write.

The other issue I ran into is the restriction on opening a file in passthrough
and non passthrough modes. In Android, one of our main usecases for passthrough
is location metadata redaction. Apps without the location permission get back
nulled out data when they read image metadata location. If an app has that
permission, it can open the file in passthrough mode, but otherwise the daemon
opens the file in normal fuse mode where it can do the redaction.

Currently in passthrough, this behavior is explicitly blocked. What's needed to
allow this? The page caches can contain different data, but in this case that's
a feature, not a bug. They could theoretically be backed by entirely different
things, although that would be fairly confusing. I would think the main thing
we'd need would be to invalidate areas of the cache when writing in passthrough
mode to give the daemon the opportunity to react to what's there now, and also
something in the other direction. Might make more sense as something the daemon
can opt into.

Any thoughts on these issues? And does the proposed fix make sense to you?



Daniel Rosenberg (1):
  fuse: Keep attributes consistent with Passthrough

 fs/fuse/passthrough.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)


base-commit: 73e931504f8e0d42978bfcda37b323dbbd1afc08
-- 
2.45.2.803.g4e1b14247a-goog

[PATCH 1/1] fuse: Keep attributes consistent with Passthrough

[Daniel Rosenberg]

If writeback cache is enabled, and we attempt to use fuse passthrough,
fuse will invalidate attributes on write, but will then ignore when the
server attempts to update those attributes. Since the kernel is the
arbiter of truth with writeback cache, passthrough holds the
responsibility to keep these values updated.

Signed-off-by: Daniel Rosenberg <drosen@google.com>
---
 fs/fuse/passthrough.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/fs/fuse/passthrough.c b/fs/fuse/passthrough.c
index 9666d13884ce..6f63a1a38d7c 100644
--- a/fs/fuse/passthrough.c
+++ b/fs/fuse/passthrough.c
@@ -21,8 +21,19 @@ static void fuse_file_accessed(struct file *file)
 static void fuse_file_modified(struct file *file)
 {
 	struct inode *inode = file_inode(file);
-
-	fuse_invalidate_attr_mask(inode, FUSE_STATX_MODSIZE);
+	struct fuse_conn *fc = get_fuse_conn(inode);
+	struct fuse_file *ff = file->private_data;
+	struct file *backing_file = fuse_file_passthrough(ff);
+	struct inode *backing_inode = file_inode(backing_file);
+
+	if (!fc->writeback_cache) {
+		fuse_invalidate_attr_mask(inode, FUSE_STATX_MODSIZE);
+	} else {
+		inode_set_mtime_to_ts(inode, inode_get_mtime(backing_inode));
+		inode_set_ctime_to_ts(inode, inode_get_ctime(backing_inode));
+		inode->i_blocks = backing_inode->i_blocks;
+		i_size_write(inode, i_size_read(backing_inode));
+	}
 }
 
 ssize_t fuse_passthrough_read_iter(struct kiocb *iocb, struct iov_iter *iter)
-- 
2.45.2.803.g4e1b14247a-goog

Re: [PATCH 0/1] Fuse Passthrough cache issues

[Bernd Schubert]

On 7/3/24 03:02, Daniel Rosenberg wrote:
> I've been attempting to recreate Android's usage of Fuse Passthrough with the
> version now merged in the kernel, and I've run into a couple issues. The first
> one was pretty straightforward, and I've included a patch, although I'm not
> convinced that it should be conditional, and it may need to do more to ensure
> that the cache is up to date.
> 
> If your fuse daemon is running with writeback cache enabled, writes with
> passthrough files will cause problems. Fuse will invalidate attributes on
> write, but because it's in writeback cache mode, it will ignore the requested
> attributes when the daemon provides them. The kernel is the source of truth in
> this case, and should update the cached values during the passthrough write.

Could you explain why you want to have the combination passthrough and
writeback cache?

I think Amirs intention was to have passthrough and cache writes
conflicting, see fuse_file_passthrough_open() and
fuse_file_cached_io_open().

Also in <libfuse>/example/passthrough_hp.cc in sfs_init():

    /* Passthrough and writeback cache are conflicting modes */



With that I wonder if either fc->writeback_cache should be ignored when
a file is opened in passthrough mode, or if fuse_file_io_open() should
ignore FOPEN_PASSTHROUGH when fc->writeback_cache is set. Either of both
would result in the opposite of what you are trying to achieve - which
is why I think it is important to understand what is your actual goal.

I think idea for conflicting file cached and passthrough modes is that
the backing inode can already provide a cache - why another one for fuse?


> 
> The other issue I ran into is the restriction on opening a file in passthrough
> and non passthrough modes. In Android, one of our main usecases for passthrough
> is location metadata redaction. Apps without the location permission get back
> nulled out data when they read image metadata location. If an app has that
> permission, it can open the file in passthrough mode, but otherwise the daemon
> opens the file in normal fuse mode where it can do the redaction.
> 
> Currently in passthrough, this behavior is explicitly blocked. What's needed to
> allow this? The page caches can contain different data, but in this case that's
> a feature, not a bug. They could theoretically be backed by entirely different
> things, although that would be fairly confusing. I would think the main thing
> we'd need would be to invalidate areas of the cache when writing in passthrough
> mode to give the daemon the opportunity to react to what's there now, and also
> something in the other direction. Might make more sense as something the daemon
> can opt into.
> 
> Any thoughts on these issues? And does the proposed fix make sense to you?
> 
> 
> 
> Daniel Rosenberg (1):
>   fuse: Keep attributes consistent with Passthrough
> 
>  fs/fuse/passthrough.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> 
> base-commit: 73e931504f8e0d42978bfcda37b323dbbd1afc08


Thanks,
Bernd

Re: [PATCH 0/1] Fuse Passthrough cache issues

[Amir Goldstein]

On Wed, Jul 3, 2024 at 4:27 PM Bernd Schubert
<bernd.schubert@fastmail.fm> wrote:
>
>
>
> On 7/3/24 03:02, Daniel Rosenberg wrote:
> > I've been attempting to recreate Android's usage of Fuse Passthrough with the
> > version now merged in the kernel, and I've run into a couple issues. The first
> > one was pretty straightforward, and I've included a patch, although I'm not
> > convinced that it should be conditional, and it may need to do more to ensure
> > that the cache is up to date.
> >
> > If your fuse daemon is running with writeback cache enabled, writes with
> > passthrough files will cause problems. Fuse will invalidate attributes on
> > write, but because it's in writeback cache mode, it will ignore the requested
> > attributes when the daemon provides them. The kernel is the source of truth in
> > this case, and should update the cached values during the passthrough write.
>
> Could you explain why you want to have the combination passthrough and
> writeback cache?
>
> I think Amirs intention was to have passthrough and cache writes
> conflicting, see fuse_file_passthrough_open() and
> fuse_file_cached_io_open().

Yes, this was an explicit design requirement from Miklos [1].
I also have use cases to handle some read/writes from server
and the compromise was that for the first version these cases should
use FOPEN_DIRECT_IO, which does not conflict with FOPEN_PASSTHROUGH.

I guess this is not good enough for Android applications opening photos
that need the FUSE readahead cache for performance?

In that case, a future BPF filter can decide whether to send the IO direct
to server or to backing inode.

Or a future backing inode mapping API could map part of the file to
backing inode
and the metadata portion will not be mapped to backing inode will fall back to
direct IO to server.

[1] https://lore.kernel.org/linux-fsdevel/CAJfpegtWdGVm9iHgVyXfY2mnR98XJ=6HtpaA+W83vvQea5PycQ@mail.gmail.com/

>
> Also in <libfuse>/example/passthrough_hp.cc in sfs_init():
>
>     /* Passthrough and writeback cache are conflicting modes */
>
>
>
> With that I wonder if either fc->writeback_cache should be ignored when
> a file is opened in passthrough mode, or if fuse_file_io_open() should
> ignore FOPEN_PASSTHROUGH when fc->writeback_cache is set. Either of both
> would result in the opposite of what you are trying to achieve - which
> is why I think it is important to understand what is your actual goal.
>

Is there no standard way for FUSE client to tell the server that the
INIT response is invalid?

Anyway, we already ignore  FUSE_PASSTHROUGH in INIT response
for several cases, so this could be another case.
Then FOPEN_PASSTHROUGH will fail with EIO (not be ignored).

> I think idea for conflicting file cached and passthrough modes is that
> the backing inode can already provide a cache - why another one for fuse?
>
>
> >
> > The other issue I ran into is the restriction on opening a file in passthrough
> > and non passthrough modes. In Android, one of our main usecases for passthrough
> > is location metadata redaction. Apps without the location permission get back
> > nulled out data when they read image metadata location. If an app has that
> > permission, it can open the file in passthrough mode, but otherwise the daemon
> > opens the file in normal fuse mode where it can do the redaction.
> >
> > Currently in passthrough, this behavior is explicitly blocked. What's needed to
> > allow this? The page caches can contain different data, but in this case that's
> > a feature, not a bug. They could theoretically be backed by entirely different
> > things, although that would be fairly confusing. I would think the main thing
> > we'd need would be to invalidate areas of the cache when writing in passthrough
> > mode to give the daemon the opportunity to react to what's there now, and also
> > something in the other direction. Might make more sense as something the daemon
> > can opt into.
> >
> > Any thoughts on these issues? And does the proposed fix make sense to you?

FYI, Miklos suggested that attributes will be passthrough to the "backing inode"
when inode is in passthrough mode, which requires lookup-to-forget semantics
just like in the BPF patches.

I have started a POC of this [2], but got sidetracked to other things.
I am not sure when I will be able to get back to them.
If this is interesting to you either for solving the specific attribute mismatch
issue or as a stepping stone towards FUSE BPF, do feel free to pick up
my patches.

[2] https://github.com/amir73il/linux/commits/fuse-backing-inode-wip/

Let me know if you have any questions about them.

Thanks,
Amir.

Re: [PATCH 0/1] Fuse Passthrough cache issues

[Bernd Schubert]

On 7/3/24 16:41, Amir Goldstein wrote:
> On Wed, Jul 3, 2024 at 4:27 PM Bernd Schubert
> <bernd.schubert@fastmail.fm> wrote:
>>
>>
>>
>> On 7/3/24 03:02, Daniel Rosenberg wrote:
>>> I've been attempting to recreate Android's usage of Fuse Passthrough with the
>>> version now merged in the kernel, and I've run into a couple issues. The first
>>> one was pretty straightforward, and I've included a patch, although I'm not
>>> convinced that it should be conditional, and it may need to do more to ensure
>>> that the cache is up to date.
>>>
>>> If your fuse daemon is running with writeback cache enabled, writes with
>>> passthrough files will cause problems. Fuse will invalidate attributes on
>>> write, but because it's in writeback cache mode, it will ignore the requested
>>> attributes when the daemon provides them. The kernel is the source of truth in
>>> this case, and should update the cached values during the passthrough write.
>>
>> Could you explain why you want to have the combination passthrough and
>> writeback cache?
>>
>> I think Amirs intention was to have passthrough and cache writes
>> conflicting, see fuse_file_passthrough_open() and
>> fuse_file_cached_io_open().
> 
> Yes, this was an explicit design requirement from Miklos [1].
> I also have use cases to handle some read/writes from server
> and the compromise was that for the first version these cases should
> use FOPEN_DIRECT_IO, which does not conflict with FOPEN_PASSTHROUGH.
> 
> I guess this is not good enough for Android applications opening photos
> that need the FUSE readahead cache for performance?
> 
> In that case, a future BPF filter can decide whether to send the IO direct
> to server or to backing inode.
> 
> Or a future backing inode mapping API could map part of the file to
> backing inode
> and the metadata portion will not be mapped to backing inode will fall back to
> direct IO to server.
> 
> [1] https://lore.kernel.org/linux-fsdevel/CAJfpegtWdGVm9iHgVyXfY2mnR98XJ=6HtpaA+W83vvQea5PycQ@mail.gmail.com/
> 
>>
>> Also in <libfuse>/example/passthrough_hp.cc in sfs_init():
>>
>>     /* Passthrough and writeback cache are conflicting modes */
>>
>>
>>
>> With that I wonder if either fc->writeback_cache should be ignored when
>> a file is opened in passthrough mode, or if fuse_file_io_open() should
>> ignore FOPEN_PASSTHROUGH when fc->writeback_cache is set. Either of both
>> would result in the opposite of what you are trying to achieve - which
>> is why I think it is important to understand what is your actual goal.
>>
> 
> Is there no standard way for FUSE client to tell the server that the
> INIT response is invalid?

Problem is that at FUSE_INIT time it is already mounted. process_init_reply()
can set an error state, but fuse_get_req*() will just result in ECONNREFUSED.*

> 
> Anyway, we already ignore  FUSE_PASSTHROUGH in INIT response
> for several cases, so this could be another case.
> Then FOPEN_PASSTHROUGH will fail with EIO (not be ignored).

So basically this?

diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 573550e7bbe1..36c6dcd47a53 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1327,7 +1327,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
                        if (IS_ENABLED(CONFIG_FUSE_PASSTHROUGH) &&
                            (flags & FUSE_PASSTHROUGH) &&
                            arg->max_stack_depth > 0 &&
-                           arg->max_stack_depth <= FILESYSTEM_MAX_STACK_DEPTH) {
+                           arg->max_stack_depth <= FILESYSTEM_MAX_STACK_DEPTH &&
+                           !(flags & FUSE_WRITEBACK_CACHE)) {
                                fc->passthrough = 1;
                                fc->max_stack_depth = arg->max_stack_depth;
                                fm->sb->s_stack_depth = arg->max_stack_depth;




Thanks,
Bernd

Re: [PATCH 0/1] Fuse Passthrough cache issues

[Amir Goldstein]

On Wed, Jul 3, 2024 at 7:09 PM Bernd Schubert
<bernd.schubert@fastmail.fm> wrote:
>
>
>
> On 7/3/24 16:41, Amir Goldstein wrote:
> > On Wed, Jul 3, 2024 at 4:27 PM Bernd Schubert
> > <bernd.schubert@fastmail.fm> wrote:
> >>
> >>
> >>
> >> On 7/3/24 03:02, Daniel Rosenberg wrote:
> >>> I've been attempting to recreate Android's usage of Fuse Passthrough with the
> >>> version now merged in the kernel, and I've run into a couple issues. The first
> >>> one was pretty straightforward, and I've included a patch, although I'm not
> >>> convinced that it should be conditional, and it may need to do more to ensure
> >>> that the cache is up to date.
> >>>
> >>> If your fuse daemon is running with writeback cache enabled, writes with
> >>> passthrough files will cause problems. Fuse will invalidate attributes on
> >>> write, but because it's in writeback cache mode, it will ignore the requested
> >>> attributes when the daemon provides them. The kernel is the source of truth in
> >>> this case, and should update the cached values during the passthrough write.
> >>
> >> Could you explain why you want to have the combination passthrough and
> >> writeback cache?
> >>
> >> I think Amirs intention was to have passthrough and cache writes
> >> conflicting, see fuse_file_passthrough_open() and
> >> fuse_file_cached_io_open().
> >
> > Yes, this was an explicit design requirement from Miklos [1].
> > I also have use cases to handle some read/writes from server
> > and the compromise was that for the first version these cases should
> > use FOPEN_DIRECT_IO, which does not conflict with FOPEN_PASSTHROUGH.
> >
> > I guess this is not good enough for Android applications opening photos
> > that need the FUSE readahead cache for performance?
> >
> > In that case, a future BPF filter can decide whether to send the IO direct
> > to server or to backing inode.
> >
> > Or a future backing inode mapping API could map part of the file to
> > backing inode
> > and the metadata portion will not be mapped to backing inode will fall back to
> > direct IO to server.
> >
> > [1] https://lore.kernel.org/linux-fsdevel/CAJfpegtWdGVm9iHgVyXfY2mnR98XJ=6HtpaA+W83vvQea5PycQ@mail.gmail.com/
> >
> >>
> >> Also in <libfuse>/example/passthrough_hp.cc in sfs_init():
> >>
> >>     /* Passthrough and writeback cache are conflicting modes */
> >>
> >>
> >>
> >> With that I wonder if either fc->writeback_cache should be ignored when
> >> a file is opened in passthrough mode, or if fuse_file_io_open() should
> >> ignore FOPEN_PASSTHROUGH when fc->writeback_cache is set. Either of both
> >> would result in the opposite of what you are trying to achieve - which
> >> is why I think it is important to understand what is your actual goal.
> >>
> >
> > Is there no standard way for FUSE client to tell the server that the
> > INIT response is invalid?
>
> Problem is that at FUSE_INIT time it is already mounted. process_init_reply()
> can set an error state, but fuse_get_req*() will just result in ECONNREFUSED.*
>
> >
> > Anyway, we already ignore  FUSE_PASSTHROUGH in INIT response
> > for several cases, so this could be another case.
> > Then FOPEN_PASSTHROUGH will fail with EIO (not be ignored).
>
> So basically this?
>
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 573550e7bbe1..36c6dcd47a53 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -1327,7 +1327,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
>                         if (IS_ENABLED(CONFIG_FUSE_PASSTHROUGH) &&
>                             (flags & FUSE_PASSTHROUGH) &&
>                             arg->max_stack_depth > 0 &&
> -                           arg->max_stack_depth <= FILESYSTEM_MAX_STACK_DEPTH) {
> +                           arg->max_stack_depth <= FILESYSTEM_MAX_STACK_DEPTH &&
> +                           !(flags & FUSE_WRITEBACK_CACHE)) {
>                                 fc->passthrough = 1;
>                                 fc->max_stack_depth = arg->max_stack_depth;
>                                 fm->sb->s_stack_depth = arg->max_stack_depth;
>
>

Yap.
Maybe add something to the comment, because the comment is
about max_stack_depth and someone may assume that it also refers
to FUSE_WRITEBACK_CACHE somehow.

Thanks,
Amir.