| Message ID | 20211011090240.97-1-xieyongji@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] fuse: Avoid invalidating attrs if writeback_cache enabled | expand |
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
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
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
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
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
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
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); }
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(-)