| Message ID | 20240703010215.2013266-1-drosen@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fuse Passthrough cache issues | expand |
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
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.
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
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.