| Message ID | 20211024132607.1636952-1-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuse: add FOPEN_NOFLUSH | expand |
On Sun, Oct 24, 2021 at 6:17 PM Shachar Sharon <synarete@gmail.com> wrote: > > On Sun, Oct 24, 2021 at 05:21:55PM +0300, Amir Goldstein wrote: > >On Sun, Oct 24, 2021 at 4:55 PM Shachar Sharon <synarete@gmail.com> wrote: > >> > >> On Sun, Oct 24, 2021 at 04:26:07PM +0300, Amir Goldstein wrote: > >> >Add flag returned by OPENDIR request to avoid flushing data cache > >> >on close. > >> > > >> I believe this holds not only for FUSE_OPENDIR but also to FUSE_OPEN and > >> FUSE_CREATE (see 'struct fuse_open_out'). > >> > > > >Oops that was a copy&paste typo. > >Of course this is only relevant for FUSE_OPEN and FUSE_CREATE. > IMHO it is relevant to all 3 cases (FUSE_OPEN, FUSE_CREATE, > FUSE_OPENDIR). > fuse_dir_operations have no ->flush() as there is no write cache for directories. > > > > >> >Different filesystems implement ->flush() is different ways: > >> >- Most disk filesystems do not implement ->flush() at all > >> >- Some network filesystem (e.g. nfs) flush local write cache of > >> > FMODE_WRITE file and send a "flush" command to server > >> >- Some network filesystem (e.g. cifs) flush local write cache of > >> > FMODE_WRITE file without sending an additional command to server > >> > > >> >FUSE flushes local write cache of ANY file, even non FMODE_WRITE > >> >and sends a "flush" command to server (if server implements it). > >> > > >> >The FUSE implementation of ->flush() seems over agressive and > >> >arbitrary and does not make a lot of sense when writeback caching is > >> >disabled. > >> > > >> >Instead of deciding on another arbitrary implementation that makes > >> >sense, leave the choice of per-file flush behavior in the hands of > >> >the server. > >> > > >> >Link: https://lore.kernel.org/linux-fsdevel/CAJfpegspE8e6aKd47uZtSYX8Y-1e1FWS0VL0DH2Skb9gQP5RJQ@mail.gmail.com/ > >> >Suggested-by: Miklos Szeredi <mszeredi@redhat.com> > >> >Signed-off-by: Amir Goldstein <amir73il@gmail.com> > >> >--- > >> > > >> >Hi Miklos, > >> > > >> >I've tested this manually by watching --debug-fuse prints > >> >with and without --nocache option to passthrough_hp. > >> > > >> >The libfuse+passthrough_hp patch is at: > >> >https://github.com/amir73il/libfuse/commits/fopen_noflush > >> > > >> >Thanks, > >> >Amir. > >> > > >> > fs/fuse/file.c | 3 +++ > >> > include/uapi/linux/fuse.h | 3 +++ > >> > 2 files changed, 6 insertions(+) > >> > > >> >diff --git a/fs/fuse/file.c b/fs/fuse/file.c > >> >index 11404f8c21c7..6f502a76f9ac 100644 > >> >--- a/fs/fuse/file.c > >> >+++ b/fs/fuse/file.c > >> >@@ -483,6 +483,9 @@ static int fuse_flush(struct file *file, fl_owner_t id) > >> > if (fuse_is_bad(inode)) > >> > return -EIO; > >> > > >> >+ if (ff->open_flags & FOPEN_NOFLUSH) > >> >+ return 0; > >> >+ > >> > err = write_inode_now(inode, 1); > >> > if (err) > >> > return err; > >> >diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h > >> >index 36ed092227fa..383781d1878f 100644 > >> >--- a/include/uapi/linux/fuse.h > >> >+++ b/include/uapi/linux/fuse.h > >> >@@ -184,6 +184,7 @@ > >> > * > >> > * 7.34 > >> > * - add FUSE_SYNCFS > >> Most likely you want to bump to 7.35; 7.34 is already out in the wild > >> (e.g., on my Fedora33 workstation) > >> > > > >Possibly. I wasn't sure what was the rationale behind when the version > >should be bumped. > >One argument against bumping the version is that there is not much > >harm is passing this flag to an old kernel - it just ignored the flag > >and sends the flush requests anyway. Miklos, do I need to bump the protocol version? Thanks, Amir.
On Sun, 24 Oct 2021 at 17:30, Amir Goldstein <amir73il@gmail.com> wrote: > > On Sun, Oct 24, 2021 at 6:17 PM Shachar Sharon <synarete@gmail.com> wrote: > > > > On Sun, Oct 24, 2021 at 05:21:55PM +0300, Amir Goldstein wrote: > > >On Sun, Oct 24, 2021 at 4:55 PM Shachar Sharon <synarete@gmail.com> wrote: > > >> > > >> On Sun, Oct 24, 2021 at 04:26:07PM +0300, Amir Goldstein wrote: > > >> >Add flag returned by OPENDIR request to avoid flushing data cache > > >> >on close. > > >> > > > >> I believe this holds not only for FUSE_OPENDIR but also to FUSE_OPEN and > > >> FUSE_CREATE (see 'struct fuse_open_out'). > > >> > > > > > >Oops that was a copy&paste typo. > > >Of course this is only relevant for FUSE_OPEN and FUSE_CREATE. Fixed up. > > >> > fs/fuse/file.c | 3 +++ > > >> > include/uapi/linux/fuse.h | 3 +++ > > >> > 2 files changed, 6 insertions(+) > > >> > > > >> >diff --git a/fs/fuse/file.c b/fs/fuse/file.c > > >> >index 11404f8c21c7..6f502a76f9ac 100644 > > >> >--- a/fs/fuse/file.c > > >> >+++ b/fs/fuse/file.c > > >> >@@ -483,6 +483,9 @@ static int fuse_flush(struct file *file, fl_owner_t id) > > >> > if (fuse_is_bad(inode)) > > >> > return -EIO; > > >> > > > >> >+ if (ff->open_flags & FOPEN_NOFLUSH) This needs to check for !fc->writeback_cache. Fixed up. Without this dirty pages could persist after the file is finally closed, which is undesirable for several reasons. One is that a writably open file is no longer guaranteed to exist (needed for filling fuse_write_in::fh). Another is that writing out pages from memory reclaim is deadlock prone. So even if we could omit the fh field we'd need to make sure that reclaim doesn't wait for fuse writeback. So while having dirty pages beyond last close(2) could be really useful, it's not something we can currently do. Note: while mainline currently does flush pages from ->release(), that's wrong, and removed in fuse.git#for_next. > > >> >--- a/include/uapi/linux/fuse.h > > >> >+++ b/include/uapi/linux/fuse.h > > >> >@@ -184,6 +184,7 @@ > > >> > * > > >> > * 7.34 > > >> > * - add FUSE_SYNCFS > > >> Most likely you want to bump to 7.35; 7.34 is already out in the wild > > >> (e.g., on my Fedora33 workstation) > > >> > > > > > >Possibly. I wasn't sure what was the rationale behind when the version > > >should be bumped. > > >One argument against bumping the version is that there is not much > > >harm is passing this flag to an old kernel - it just ignored the flag > > >and sends the flush requests anyway. > > Miklos, do I need to bump the protocol version? We've historically done that. But the last minor version that is actually checked by the kernel or libfuse was 23 (the fuse_init_out expansion), so nothing bad would happen if for some reason the bump was forgotten. I've fixed this up too and pushed out to #for-next. Thanks, Miklos
diff --git a/fs/fuse/file.c b/fs/fuse/file.c index 11404f8c21c7..6f502a76f9ac 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -483,6 +483,9 @@ static int fuse_flush(struct file *file, fl_owner_t id) if (fuse_is_bad(inode)) return -EIO; + if (ff->open_flags & FOPEN_NOFLUSH) + return 0; + err = write_inode_now(inode, 1); if (err) return err; diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h index 36ed092227fa..383781d1878f 100644 --- a/include/uapi/linux/fuse.h +++ b/include/uapi/linux/fuse.h @@ -184,6 +184,7 @@ * * 7.34 * - add FUSE_SYNCFS + * - add FOPEN_NOFLUSH */ #ifndef _LINUX_FUSE_H @@ -290,12 +291,14 @@ struct fuse_file_lock { * FOPEN_NONSEEKABLE: the file is not seekable * FOPEN_CACHE_DIR: allow caching this directory * FOPEN_STREAM: the file is stream-like (no file position at all) + * FOPEN_NOFLUSH: don't flush data cache on every close */ #define FOPEN_DIRECT_IO (1 << 0) #define FOPEN_KEEP_CACHE (1 << 1) #define FOPEN_NONSEEKABLE (1 << 2) #define FOPEN_CACHE_DIR (1 << 3) #define FOPEN_STREAM (1 << 4) +#define FOPEN_NOFLUSH (1 << 5) /** * INIT request/reply flags
Add flag returned by OPENDIR request to avoid flushing data cache on close. Different filesystems implement ->flush() is different ways: - Most disk filesystems do not implement ->flush() at all - Some network filesystem (e.g. nfs) flush local write cache of FMODE_WRITE file and send a "flush" command to server - Some network filesystem (e.g. cifs) flush local write cache of FMODE_WRITE file without sending an additional command to server FUSE flushes local write cache of ANY file, even non FMODE_WRITE and sends a "flush" command to server (if server implements it). The FUSE implementation of ->flush() seems over agressive and arbitrary and does not make a lot of sense when writeback caching is disabled. Instead of deciding on another arbitrary implementation that makes sense, leave the choice of per-file flush behavior in the hands of the server. Link: https://lore.kernel.org/linux-fsdevel/CAJfpegspE8e6aKd47uZtSYX8Y-1e1FWS0VL0DH2Skb9gQP5RJQ@mail.gmail.com/ Suggested-by: Miklos Szeredi <mszeredi@redhat.com> Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- Hi Miklos, I've tested this manually by watching --debug-fuse prints with and without --nocache option to passthrough_hp. The libfuse+passthrough_hp patch is at: https://github.com/amir73il/libfuse/commits/fopen_noflush Thanks, Amir. fs/fuse/file.c | 3 +++ include/uapi/linux/fuse.h | 3 +++ 2 files changed, 6 insertions(+)