* [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).