* [RFC] fuse: Avoid invalidating attrs if writeback_cache enabled
@ 2021-10-11 9:02 Xie Yongji
2021-10-11 13:21 ` Miklos Szeredi
0 siblings, 1 reply; 7+ messages in thread
From: Xie Yongji @ 2021-10-11 9:02 UTC (permalink / raw)
To: miklos; +Cc: linux-fsdevel, zhangjiachen.jaycee
Recently we found the performance of small direct writes is bad
when writeback_cache enabled. This is because we need to get
attrs from userspace in fuse_update_get_attr() on each write.
The timeout for the attributes doesn't work since every direct write
will invalidate the attrs in fuse_direct_IO().
To fix it, this patch tries to avoid invalidating attrs if writeback_cache
is enabled since we should trust local size/ctime/mtime in this case.
Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
fs/fuse/file.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 11404f8c21c7..5561d4cc735c 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -2868,8 +2868,11 @@ fuse_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
}
if (iov_iter_rw(iter) == WRITE) {
+ struct fuse_conn *fc = get_fuse_conn(inode);
+
ret = fuse_direct_io(io, iter, &pos, FUSE_DIO_WRITE);
- fuse_invalidate_attr(inode);
+ if (!fc->writeback_cache)
+ fuse_invalidate_attr(inode);
} else {
ret = __fuse_direct_read(io, iter, &pos);
}
--
2.11.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC] fuse: Avoid invalidating attrs if writeback_cache enabled
2021-10-11 9:02 [RFC] fuse: Avoid invalidating attrs if writeback_cache enabled Xie Yongji
@ 2021-10-11 13:21 ` Miklos Szeredi
2021-10-11 14:45 ` Yongji Xie
0 siblings, 1 reply; 7+ messages in thread
From: Miklos Szeredi @ 2021-10-11 13:21 UTC (permalink / raw)
To: Xie Yongji; +Cc: linux-fsdevel, zhangjiachen.jaycee
On Mon, 11 Oct 2021 at 11:07, Xie Yongji <xieyongji@bytedance.com> wrote:
>
> Recently we found the performance of small direct writes is bad
> when writeback_cache enabled. This is because we need to get
> attrs from userspace in fuse_update_get_attr() on each write.
> The timeout for the attributes doesn't work since every direct write
> will invalidate the attrs in fuse_direct_IO().
>
> To fix it, this patch tries to avoid invalidating attrs if writeback_cache
> is enabled since we should trust local size/ctime/mtime in this case.
Hi,
Thanks for the patch.
Just pushed an update to
git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.gitt#for-next
(9ca3f8697158 ("fuse: selective attribute invalidation")) that should
fix this behavior.
Could you please test?
Thanks,
Miklos
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] fuse: Avoid invalidating attrs if writeback_cache enabled
2021-10-11 13:21 ` Miklos Szeredi
@ 2021-10-11 14:45 ` Yongji Xie
2021-10-13 13:52 ` Miklos Szeredi
0 siblings, 1 reply; 7+ messages in thread
From: Yongji Xie @ 2021-10-11 14:45 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: linux-fsdevel, 张佳辰
On Mon, Oct 11, 2021 at 9:21 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Mon, 11 Oct 2021 at 11:07, Xie Yongji <xieyongji@bytedance.com> wrote:
> >
> > Recently we found the performance of small direct writes is bad
> > when writeback_cache enabled. This is because we need to get
> > attrs from userspace in fuse_update_get_attr() on each write.
> > The timeout for the attributes doesn't work since every direct write
> > will invalidate the attrs in fuse_direct_IO().
> >
> > To fix it, this patch tries to avoid invalidating attrs if writeback_cache
> > is enabled since we should trust local size/ctime/mtime in this case.
>
> Hi,
>
> Thanks for the patch.
>
> Just pushed an update to
> git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.gitt#for-next
> (9ca3f8697158 ("fuse: selective attribute invalidation")) that should
> fix this behavior.
>
Looks like fuse_update_get_attr() will still get attrs from userspace
each time with this commit applied.
> Could you please test?
>
I applied the commit 9ca3f8697158 ("fuse: selective attribute
invalidation") and tested it. But the issue still exists.
Thanks,
Yongji
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] fuse: Avoid invalidating attrs if writeback_cache enabled
2021-10-11 14:45 ` Yongji Xie
@ 2021-10-13 13:52 ` Miklos Szeredi
2021-10-18 11:25 ` Yongji Xie
0 siblings, 1 reply; 7+ messages in thread
From: Miklos Szeredi @ 2021-10-13 13:52 UTC (permalink / raw)
To: Yongji Xie; +Cc: linux-fsdevel, 张佳辰
On Mon, 11 Oct 2021 at 16:45, Yongji Xie <xieyongji@bytedance.com> wrote:
>
> On Mon, Oct 11, 2021 at 9:21 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Mon, 11 Oct 2021 at 11:07, Xie Yongji <xieyongji@bytedance.com> wrote:
> > >
> > > Recently we found the performance of small direct writes is bad
> > > when writeback_cache enabled. This is because we need to get
> > > attrs from userspace in fuse_update_get_attr() on each write.
> > > The timeout for the attributes doesn't work since every direct write
> > > will invalidate the attrs in fuse_direct_IO().
> > >
> > > To fix it, this patch tries to avoid invalidating attrs if writeback_cache
> > > is enabled since we should trust local size/ctime/mtime in this case.
> >
> > Hi,
> >
> > Thanks for the patch.
> >
> > Just pushed an update to
> > git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.gitt#for-next
> > (9ca3f8697158 ("fuse: selective attribute invalidation")) that should
> > fix this behavior.
> >
>
> Looks like fuse_update_get_attr() will still get attrs from userspace
> each time with this commit applied.
>
> > Could you please test?
> >
>
> I applied the commit 9ca3f8697158 ("fuse: selective attribute
> invalidation") and tested it. But the issue still exists.
Yeah, my bad. Pushed a more complete set of fixes to #for-next ending with
e15a9a5fca6c ("fuse: take cache_mask into account in getattr")
You should pull or cherry pick the complete branch.
Thanks,
Miklos
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] fuse: Avoid invalidating attrs if writeback_cache enabled
2021-10-13 13:52 ` Miklos Szeredi
@ 2021-10-18 11:25 ` Yongji Xie
2021-10-18 11:45 ` Miklos Szeredi
0 siblings, 1 reply; 7+ messages in thread
From: Yongji Xie @ 2021-10-18 11:25 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: linux-fsdevel, 张佳辰
On Wed, Oct 13, 2021 at 9:52 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Mon, 11 Oct 2021 at 16:45, Yongji Xie <xieyongji@bytedance.com> wrote:
> >
> > On Mon, Oct 11, 2021 at 9:21 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Mon, 11 Oct 2021 at 11:07, Xie Yongji <xieyongji@bytedance.com> wrote:
> > > >
> > > > Recently we found the performance of small direct writes is bad
> > > > when writeback_cache enabled. This is because we need to get
> > > > attrs from userspace in fuse_update_get_attr() on each write.
> > > > The timeout for the attributes doesn't work since every direct write
> > > > will invalidate the attrs in fuse_direct_IO().
> > > >
> > > > To fix it, this patch tries to avoid invalidating attrs if writeback_cache
> > > > is enabled since we should trust local size/ctime/mtime in this case.
> > >
> > > Hi,
> > >
> > > Thanks for the patch.
> > >
> > > Just pushed an update to
> > > git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.gitt#for-next
> > > (9ca3f8697158 ("fuse: selective attribute invalidation")) that should
> > > fix this behavior.
> > >
> >
> > Looks like fuse_update_get_attr() will still get attrs from userspace
> > each time with this commit applied.
> >
> > > Could you please test?
> > >
> >
> > I applied the commit 9ca3f8697158 ("fuse: selective attribute
> > invalidation") and tested it. But the issue still exists.
>
> Yeah, my bad. Pushed a more complete set of fixes to #for-next ending with
>
> e15a9a5fca6c ("fuse: take cache_mask into account in getattr")
>
> You should pull or cherry pick the complete branch.
>
I tested this branch, but it still doesn't fix this issue. The
inval_mask = 0x6C0 and cache_mask = 0x2C0, so we still need to get
attrs from userspace. Should we add STATX_BLOCKS to cache_mask?
Thanks,
Yongji
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] fuse: Avoid invalidating attrs if writeback_cache enabled
2021-10-18 11:25 ` Yongji Xie
@ 2021-10-18 11:45 ` Miklos Szeredi
2021-10-18 13:08 ` Yongji Xie
0 siblings, 1 reply; 7+ messages in thread
From: Miklos Szeredi @ 2021-10-18 11:45 UTC (permalink / raw)
To: Yongji Xie; +Cc: linux-fsdevel, 张佳辰
[-- Attachment #1: Type: text/plain, Size: 2101 bytes --]
On Mon, 18 Oct 2021 at 13:25, Yongji Xie <xieyongji@bytedance.com> wrote:
>
> On Wed, Oct 13, 2021 at 9:52 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Mon, 11 Oct 2021 at 16:45, Yongji Xie <xieyongji@bytedance.com> wrote:
> > >
> > > On Mon, Oct 11, 2021 at 9:21 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > >
> > > > On Mon, 11 Oct 2021 at 11:07, Xie Yongji <xieyongji@bytedance.com> wrote:
> > > > >
> > > > > Recently we found the performance of small direct writes is bad
> > > > > when writeback_cache enabled. This is because we need to get
> > > > > attrs from userspace in fuse_update_get_attr() on each write.
> > > > > The timeout for the attributes doesn't work since every direct write
> > > > > will invalidate the attrs in fuse_direct_IO().
> > > > >
> > > > > To fix it, this patch tries to avoid invalidating attrs if writeback_cache
> > > > > is enabled since we should trust local size/ctime/mtime in this case.
> > > >
> > > > Hi,
> > > >
> > > > Thanks for the patch.
> > > >
> > > > Just pushed an update to
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.gitt#for-next
> > > > (9ca3f8697158 ("fuse: selective attribute invalidation")) that should
> > > > fix this behavior.
> > > >
> > >
> > > Looks like fuse_update_get_attr() will still get attrs from userspace
> > > each time with this commit applied.
> > >
> > > > Could you please test?
> > > >
> > >
> > > I applied the commit 9ca3f8697158 ("fuse: selective attribute
> > > invalidation") and tested it. But the issue still exists.
> >
> > Yeah, my bad. Pushed a more complete set of fixes to #for-next ending with
> >
> > e15a9a5fca6c ("fuse: take cache_mask into account in getattr")
> >
> > You should pull or cherry pick the complete branch.
> >
>
> I tested this branch, but it still doesn't fix this issue. The
> inval_mask = 0x6C0 and cache_mask = 0x2C0, so we still need to get
> attrs from userspace. Should we add STATX_BLOCKS to cache_mask?
Does the attach incremental ~/gupatch solve this? Or is the
fuse_update_get_attr() coming from a stat* syscall?
Thanks,
Miklos
[-- Attachment #2: fuse-only-update-necessary-attributes.patch --]
[-- Type: text/x-patch, Size: 3387 bytes --]
Index: linux/fs/fuse/dir.c
===================================================================
--- linux.orig/fs/fuse/dir.c 2021-10-18 13:40:27.381801032 +0200
+++ linux/fs/fuse/dir.c 2021-10-18 13:37:26.798569496 +0200
@@ -1055,11 +1055,9 @@ static int fuse_update_get_attr(struct i
return err;
}
-int fuse_update_attributes(struct inode *inode, struct file *file)
+int fuse_update_attributes(struct inode *inode, struct file *file, u32 mask)
{
- /* Do *not* need to get atime for internal purposes */
- return fuse_update_get_attr(inode, file, NULL,
- STATX_BASIC_STATS & ~STATX_ATIME, 0);
+ return fuse_update_get_attr(inode, file, NULL, mask, 0);
}
int fuse_reverse_inval_entry(struct fuse_conn *fc, u64 parent_nodeid,
Index: linux/fs/fuse/file.c
===================================================================
--- linux.orig/fs/fuse/file.c 2021-10-18 13:40:27.382801044 +0200
+++ linux/fs/fuse/file.c 2021-10-18 13:40:14.504641904 +0200
@@ -996,7 +996,7 @@ static ssize_t fuse_cache_read_iter(stru
if (fc->auto_inval_data ||
(iocb->ki_pos + iov_iter_count(to) > i_size_read(inode))) {
int err;
- err = fuse_update_attributes(inode, iocb->ki_filp);
+ err = fuse_update_attributes(inode, iocb->ki_filp, STATX_SIZE);
if (err)
return err;
}
@@ -1282,7 +1282,8 @@ static ssize_t fuse_cache_write_iter(str
if (fc->writeback_cache) {
/* Update size (EOF optimization) and mode (SUID clearing) */
- err = fuse_update_attributes(mapping->host, file);
+ err = fuse_update_attributes(mapping->host, file,
+ STATX_SIZE | STATX_MODE);
if (err)
return err;
@@ -2633,7 +2634,7 @@ static loff_t fuse_lseek(struct file *fi
return vfs_setpos(file, outarg.offset, inode->i_sb->s_maxbytes);
fallback:
- err = fuse_update_attributes(inode, file);
+ err = fuse_update_attributes(inode, file, STATX_SIZE);
if (!err)
return generic_file_llseek(file, offset, whence);
else
@@ -2653,7 +2654,7 @@ static loff_t fuse_file_llseek(struct fi
break;
case SEEK_END:
inode_lock(inode);
- retval = fuse_update_attributes(inode, file);
+ retval = fuse_update_attributes(inode, file, STATX_SIZE);
if (!retval)
retval = generic_file_llseek(file, offset, whence);
inode_unlock(inode);
Index: linux/fs/fuse/fuse_i.h
===================================================================
--- linux.orig/fs/fuse/fuse_i.h 2021-10-18 13:40:27.382801044 +0200
+++ linux/fs/fuse/fuse_i.h 2021-10-18 13:37:43.778779327 +0200
@@ -1161,7 +1161,7 @@ u64 fuse_lock_owner_id(struct fuse_conn
void fuse_flush_time_update(struct inode *inode);
void fuse_update_ctime(struct inode *inode);
-int fuse_update_attributes(struct inode *inode, struct file *file);
+int fuse_update_attributes(struct inode *inode, struct file *file, u32 mask);
void fuse_flush_writepages(struct inode *inode);
Index: linux/fs/fuse/readdir.c
===================================================================
--- linux.orig/fs/fuse/readdir.c 2021-10-18 13:41:02.365233336 +0200
+++ linux/fs/fuse/readdir.c 2021-10-18 13:38:03.413021954 +0200
@@ -454,7 +454,7 @@ static int fuse_readdir_cached(struct fi
* cache; both cases require an up-to-date mtime value.
*/
if (!ctx->pos && fc->auto_inval_data) {
- int err = fuse_update_attributes(inode, file);
+ int err = fuse_update_attributes(inode, file, STATX_MTIME);
if (err)
return err;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] fuse: Avoid invalidating attrs if writeback_cache enabled
2021-10-18 11:45 ` Miklos Szeredi
@ 2021-10-18 13:08 ` Yongji Xie
0 siblings, 0 replies; 7+ messages in thread
From: Yongji Xie @ 2021-10-18 13:08 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: linux-fsdevel, 张佳辰
On Mon, Oct 18, 2021 at 7:45 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Mon, 18 Oct 2021 at 13:25, Yongji Xie <xieyongji@bytedance.com> wrote:
> >
> > On Wed, Oct 13, 2021 at 9:52 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Mon, 11 Oct 2021 at 16:45, Yongji Xie <xieyongji@bytedance.com> wrote:
> > > >
> > > > On Mon, Oct 11, 2021 at 9:21 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > > >
> > > > > On Mon, 11 Oct 2021 at 11:07, Xie Yongji <xieyongji@bytedance.com> wrote:
> > > > > >
> > > > > > Recently we found the performance of small direct writes is bad
> > > > > > when writeback_cache enabled. This is because we need to get
> > > > > > attrs from userspace in fuse_update_get_attr() on each write.
> > > > > > The timeout for the attributes doesn't work since every direct write
> > > > > > will invalidate the attrs in fuse_direct_IO().
> > > > > >
> > > > > > To fix it, this patch tries to avoid invalidating attrs if writeback_cache
> > > > > > is enabled since we should trust local size/ctime/mtime in this case.
> > > > >
> > > > > Hi,
> > > > >
> > > > > Thanks for the patch.
> > > > >
> > > > > Just pushed an update to
> > > > > git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.gitt#for-next
> > > > > (9ca3f8697158 ("fuse: selective attribute invalidation")) that should
> > > > > fix this behavior.
> > > > >
> > > >
> > > > Looks like fuse_update_get_attr() will still get attrs from userspace
> > > > each time with this commit applied.
> > > >
> > > > > Could you please test?
> > > > >
> > > >
> > > > I applied the commit 9ca3f8697158 ("fuse: selective attribute
> > > > invalidation") and tested it. But the issue still exists.
> > >
> > > Yeah, my bad. Pushed a more complete set of fixes to #for-next ending with
> > >
> > > e15a9a5fca6c ("fuse: take cache_mask into account in getattr")
> > >
> > > You should pull or cherry pick the complete branch.
> > >
> >
> > I tested this branch, but it still doesn't fix this issue. The
> > inval_mask = 0x6C0 and cache_mask = 0x2C0, so we still need to get
> > attrs from userspace. Should we add STATX_BLOCKS to cache_mask?
>
> Does the attach incremental ~/gupatch solve this?
Yes, this patch solves it.
Thanks,
Yongji
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-10-18 13:09 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-11 9:02 [RFC] fuse: Avoid invalidating attrs if writeback_cache enabled Xie Yongji
2021-10-11 13:21 ` Miklos Szeredi
2021-10-11 14:45 ` Yongji Xie
2021-10-13 13:52 ` Miklos Szeredi
2021-10-18 11:25 ` Yongji Xie
2021-10-18 11:45 ` Miklos Szeredi
2021-10-18 13:08 ` Yongji Xie
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.