On Wed, Apr 3, 2024 at 12:18 AM Bernd Schubert
<bernd.schubert@xxxxxxxxxxx> wrote:
>
>
>
> On 4/2/24 22:13, Sweet Tea Dorminy wrote:
> >
> >
> > On 2/6/24 09:24, Amir Goldstein wrote:
> >> After passthrough read/write, we invalidate a/c/mtime/size attributes
> >> if the backing inode attributes differ from FUSE inode attributes.
> >>
> >> Do the same in fuse_getattr() and after detach of backing inode, so that
> >> passthrough mmap read/write changes to a/c/mtime/size attribute of the
> >> backing inode will be propagated to the FUSE inode.
> >>
> >> The rules of invalidating a/c/mtime/size attributes with writeback cache
> >> are more complicated, so for now, writeback cache and passthrough cannot
> >> be enabled on the same filesystem.
> >>
> >> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> >> ---
> >> fs/fuse/dir.c | 4 ++++
> >> fs/fuse/fuse_i.h | 2 ++
> >> fs/fuse/inode.c | 4 ++++
> >> fs/fuse/iomode.c | 5 +++-
> >> fs/fuse/passthrough.c | 55 ++++++++++++++++++++++++++++++++++++-------
> >> 5 files changed, 61 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> >> index 95330c2ca3d8..7f9d002b8f23 100644
> >> --- a/fs/fuse/dir.c
> >> +++ b/fs/fuse/dir.c
> >> @@ -2118,6 +2118,10 @@ static int fuse_getattr(struct mnt_idmap *idmap,
> >> return -EACCES;
> >> }
> >> + /* Maybe update/invalidate attributes from backing inode */
> >> + if (fuse_inode_backing(get_fuse_inode(inode)))
> >> + fuse_backing_update_attr_mask(inode, request_mask);
> >> +
> >> return fuse_update_get_attr(inode, NULL, stat, request_mask,
> >> flags);
> >> }
> >> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> >> index 98f878a52af1..4b011d31012f 100644
> >> --- a/fs/fuse/fuse_i.h
> >> +++ b/fs/fuse/fuse_i.h
> >> @@ -1456,6 +1456,8 @@ void fuse_backing_files_init(struct fuse_conn *fc);
> >> void fuse_backing_files_free(struct fuse_conn *fc);
> >> int fuse_backing_open(struct fuse_conn *fc, struct fuse_backing_map
> >> *map);
> >> int fuse_backing_close(struct fuse_conn *fc, int backing_id);
> >> +void fuse_backing_update_attr(struct inode *inode, struct
> >> fuse_backing *fb);
> >> +void fuse_backing_update_attr_mask(struct inode *inode, u32
> >> request_mask);
> >> struct fuse_backing *fuse_passthrough_open(struct file *file,
> >> struct inode *inode,
> >> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> >> index c26a84439934..c68f005b6e86 100644
> >> --- a/fs/fuse/inode.c
> >> +++ b/fs/fuse/inode.c
> >> @@ -1302,9 +1302,13 @@ static void process_init_reply(struct
> >> fuse_mount *fm, struct fuse_args *args,
> >> * on a stacked fs (e.g. overlayfs) themselves and with
> >> * max_stack_depth == 1, FUSE fs can be stacked as the
> >> * underlying fs of a stacked fs (e.g. overlayfs).
> >> + *
> >> + * For now, writeback cache and passthrough cannot be
> >> + * enabled on the same filesystem.
> >> */
> >> if (IS_ENABLED(CONFIG_FUSE_PASSTHROUGH) &&
> >> (flags & FUSE_PASSTHROUGH) &&
> >> + !fc->writeback_cache &&
> >> arg->max_stack_depth > 0 &&
> >> arg->max_stack_depth <= FILESYSTEM_MAX_STACK_DEPTH) {
> >> fc->passthrough = 1;
> >> diff --git a/fs/fuse/iomode.c b/fs/fuse/iomode.c
> >> index c545058a01e1..96eb311fe7bd 100644
> >> --- a/fs/fuse/iomode.c
> >> +++ b/fs/fuse/iomode.c
> >> @@ -157,8 +157,11 @@ void fuse_file_uncached_io_end(struct inode *inode)
> >> spin_unlock(&fi->lock);
> >> if (!uncached_io)
> >> wake_up(&fi->direct_io_waitq);
> >> - if (oldfb)
> >> + if (oldfb) {
> >> + /* Maybe update attributes after detaching backing inode */
> >> + fuse_backing_update_attr(inode, oldfb);
> >> fuse_backing_put(oldfb);
> >> + }
> >> }
> >> /*
> >> diff --git a/fs/fuse/passthrough.c b/fs/fuse/passthrough.c
> >> index 260e76fc72d5..c1bb80a6e536 100644
> >> --- a/fs/fuse/passthrough.c
> >> +++ b/fs/fuse/passthrough.c
> >> @@ -11,11 +11,8 @@
> >> #include <linux/backing-file.h>
> >> #include <linux/splice.h>
> >> -static void fuse_file_accessed(struct file *file)
> >> +static void fuse_backing_accessed(struct inode *inode, struct
> >> fuse_backing *fb)
> >> {
> >> - struct inode *inode = file_inode(file);
> >> - struct fuse_inode *fi = get_fuse_inode(inode);
> >> - struct fuse_backing *fb = fuse_inode_backing(fi);
> >> struct inode *backing_inode = file_inode(fb->file);
> >> struct timespec64 atime = inode_get_atime(inode);
> >> struct timespec64 batime = inode_get_atime(backing_inode);
> >> @@ -25,11 +22,8 @@ static void fuse_file_accessed(struct file *file)
> >> fuse_invalidate_atime(inode);
> >> }
> >> -static void fuse_file_modified(struct file *file)
> >> +static void fuse_backing_modified(struct inode *inode, struct
> >> fuse_backing *fb)
> >> {
> >> - struct inode *inode = file_inode(file);
> >> - struct fuse_inode *fi = get_fuse_inode(inode);
> >> - struct fuse_backing *fb = fuse_inode_backing(fi);
> >> struct inode *backing_inode = file_inode(fb->file);
> >> struct timespec64 ctime = inode_get_ctime(inode);
> >> struct timespec64 mtime = inode_get_mtime(inode);
> >> @@ -42,6 +36,51 @@ static void fuse_file_modified(struct file *file)
> >> fuse_invalidate_attr_mask(inode, FUSE_STATX_MODSIZE);
> >> }
> >> +/* Called from fuse_file_uncached_io_end() after detach of backing
> >> inode */
> >> +void fuse_backing_update_attr(struct inode *inode, struct
> >> fuse_backing *fb)
> >> +{
> >> + fuse_backing_modified(inode, fb);
> >> + fuse_backing_accessed(inode, fb);
> >> +}
> >> +
> >> +/* Called from fuse_getattr() - may race with detach of backing inode */
> >> +void fuse_backing_update_attr_mask(struct inode *inode, u32
> >> request_mask)
> >> +{
> >> + struct fuse_inode *fi = get_fuse_inode(inode);
> >> + struct fuse_backing *fb;
> >> +
> >> + rcu_read_lock();
> >> + fb = fuse_backing_get(fuse_inode_backing(fi));
> >> + rcu_read_unlock();
> >> + if (!fb)
> >> + return;
> >> +
> >> + if (request_mask & FUSE_STATX_MODSIZE)
> >> + fuse_backing_modified(inode, fb);
> >> + if (request_mask & STATX_ATIME)
> >> + fuse_backing_accessed(inode, fb);
> >> +
> >> + fuse_backing_put(fb);
> >> +}
> >> +
> >> +static void fuse_file_accessed(struct file *file)
> >> +{
> >> + struct inode *inode = file_inode(file);
> >> + struct fuse_inode *fi = get_fuse_inode(inode);
> >> + struct fuse_backing *fb = fuse_inode_backing(fi);
> >> +
> >> + fuse_backing_accessed(inode, fb);
> >> +}
> >> +
> >> +static void fuse_file_modified(struct file *file)
> >> +{
> >> + struct inode *inode = file_inode(file);
> >> + struct fuse_inode *fi = get_fuse_inode(inode);
> >> + struct fuse_backing *fb = fuse_inode_backing(fi);
> >> +
> >> + fuse_backing_modified(inode, fb);
> >> +}
> >> +
> >> ssize_t fuse_passthrough_read_iter(struct kiocb *iocb, struct
> >> iov_iter *iter)
> >> {
> >> struct file *file = iocb->ki_filp;
> >
> > I noticed this patch doesn't seem to have made it into 6.9 like the rest
> > of these passthrough patches -- may I ask why it didn't make it? I think
> > it still makes sense but I might be missing some change between what's
> > in 6.9 and this version of the patches.
> >
> > Thanks!
> >
> > Sweet Tea
>
> See here please
> https://lore.kernel.org/all/CAOQ4uxj8Az6VEZ-Ky5gs33gc0N9hjv4XqL6XC_kc+vsVpaBCOg@xxxxxxxxxxxxxx/
>
>
FWIW, I intend to take a swing at getattr() passthrough "as soon as I can".
Sweet Tea,
Can you please explain the workload where you find that this patch is needed?
Is your workload using mmap writes? requires a long attribute cache timeout?
Does your workload involve mixing passthrough IO and direct/cached IO
on the same inode at different times or by different open fd's?
I would like to know, so I can tell you if getattr() passthrough design is
going to help your use case.
For example, my current getattr() passthrough design (in my head)
will not allow opening the inode in cached IO mode from lookup time
until evict/forget, unlike the current read/write passthrough, which is
from first open to last close.
Thanks,
Amir.
