| Message ID | 20241011135326.667781-1-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fuse: update inode size after extending passthrough write | expand |
On Fri, 11 Oct 2024 at 15:53, Amir Goldstein <amir73il@gmail.com> wrote: > @@ -20,9 +20,18 @@ static void fuse_file_accessed(struct file *file) > > static void fuse_file_modified(struct file *file) > { > + struct fuse_file *ff = file->private_data; > + struct file *backing_file = fuse_file_passthrough(ff); > struct inode *inode = file_inode(file); > - > - fuse_invalidate_attr_mask(inode, FUSE_STATX_MODSIZE); > + loff_t size = i_size_read(file_inode(backing_file)); What about passing iocb and res to ->end_write() instead of just the file, so that we don't need to touch the underlying inode? Thanks, Miklos
On Fri, Oct 11, 2024 at 4:25 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > On Fri, 11 Oct 2024 at 15:53, Amir Goldstein <amir73il@gmail.com> wrote: > > > @@ -20,9 +20,18 @@ static void fuse_file_accessed(struct file *file) > > > > static void fuse_file_modified(struct file *file) > > { > > + struct fuse_file *ff = file->private_data; > > + struct file *backing_file = fuse_file_passthrough(ff); > > struct inode *inode = file_inode(file); > > - > > - fuse_invalidate_attr_mask(inode, FUSE_STATX_MODSIZE); > > + loff_t size = i_size_read(file_inode(backing_file)); > > What about passing iocb and res to ->end_write() instead of just the > file, so that we don't need to touch the underlying inode? > I considered that. It was like this in one of my older versions. But why do we want to avoid copying attributes from the underlying inode? If anything, I thought that we would want to get closer to ovl_file_modified() for backing inodes in some situations like this one. I understand that brute copy of attributes is a problem, but I don't see the problem with i_size = max(i_size, i_backing_size) Can you explain the problem? Thanks, Amir.
On Mon, Oct 14, 2024 at 8:30 AM yangyun <yangyun50@huawei.com> wrote: > > On Fri, Oct 11, 2024 at 03:53:26PM +0200, Amir Goldstein wrote: > > yangyun reported that libfuse test test_copy_file_range() copies zero > > bytes from a newly written file when fuse passthrough is enabled. > > > > The reason is that extending passthrough write is not updating the fuse > > inode size and when vfs_copy_file_range() observes a zero size inode, > > it returns without calling the filesystem copy_file_range() method. > > > > Extend the fuse inode size to the size of the backing inode after every > > passthrough write if the backing inode size is larger. > > > > This does not yet provide cache coherency of fuse inode attributes and > > backing inode attributes, but it should prevent situations where fuse > > inode size is too small, causing read/copy to be wrongly shortened. > > > > Reported-by: yangyun <yangyun50@huawei.com> > > Closes: https://github.com/libfuse/libfuse/issues/1048 > > Fixes: 57e1176e6086 ("fuse: implement read/write passthrough") > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > > > Doh! force of habbit - fixed subject s/ovl/fuse > > > > Thanks, > > Amir. > > > > fs/fuse/passthrough.c | 13 +++++++++++-- > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > diff --git a/fs/fuse/passthrough.c b/fs/fuse/passthrough.c > > index ba3207f6c4ce..d3047a4bc40e 100644 > > --- a/fs/fuse/passthrough.c > > +++ b/fs/fuse/passthrough.c > > @@ -20,9 +20,18 @@ static void fuse_file_accessed(struct file *file) > > > > static void fuse_file_modified(struct file *file) > > { > > + struct fuse_file *ff = file->private_data; > > + struct file *backing_file = fuse_file_passthrough(ff); > > struct inode *inode = file_inode(file); > > - > > - fuse_invalidate_attr_mask(inode, FUSE_STATX_MODSIZE); > > + loff_t size = i_size_read(file_inode(backing_file)); > > + > > + /* > > + * Most of the time we will be holding inode_lock(), but even if we are > > + * called from async io completion without inode_lock(), the last write > > + * will update fuse inode size to the size of the backing inode, even if > > + * the last write was not the extending write. > > + */ > > + fuse_write_update_attr(inode, size, size); > > } > > > > ssize_t fuse_passthrough_read_iter(struct kiocb *iocb, struct iov_iter *iter) > > -- > > 2.34.1 > > Sorry for a late reply. Because I have spent some time on figuring out whether we > need some FUSE_I_SIZE_UNSTABLE bit operations before and after the write which are > provided in your v14 patch of fuse passthrough, just like in fuse_perform_write. > > The conclusion is not, IMO. The FUSE_I_SIZE_UNSTABLE bit is provided in > commit 06a7c3c2781(fuse: hotfix truncate_pagecache() issue) for the > races between i_size updates and truncate_pagecache() in > fuse_change_attributes. Because the pagecache operations of fuse inode > is not allowed in fuse passthrough mode, this FUSE_I_SIZE_UNSTABLE bit is useless. > And we just need the minimum fix for extending writes by now. > That was my conclusion as well. In general, too large i_size is less of a problem IMO. Shortening i_size on short read/truncate is an optimization. > I have also tested this patch with xfstests (using ./check -fuse -b) and libfuse > test. In xfstest, this patch does not import new failed tests compared with pre-this > patch. And in libfuse test, this patch can solve the problem test_copy_file_range(). Thanks for testing! Amir.
On Fri, 11 Oct 2024 at 19:57, Amir Goldstein <amir73il@gmail.com> wrote:
> But why do we want to avoid copying attributes from the underlying inode?
Because that's just a special case. The general case is that backing
data is mapped into fuse file data, possibly using more than one
extent and not necessarily starting at zero offset. In this case
using the backing file's size doesn't make sense generally.
And because it's easy to avoid, I don't see why we'd need to force
using the backing inode attributes at this point.
Your work on directory tree passthrough is related, but I think it's
separate enough to not mix their traits. When that is finalized we
can possibly add back mirroring of i_size on write, but I think the
general case shouldn't have that.
Thanks,
Miklos
On Mon, Oct 14, 2024 at 10:40 AM Miklos Szeredi <miklos@szeredi.hu> wrote: > > On Fri, 11 Oct 2024 at 19:57, Amir Goldstein <amir73il@gmail.com> wrote: > > > But why do we want to avoid copying attributes from the underlying inode? > > Because that's just a special case. The general case is that backing > data is mapped into fuse file data, possibly using more than one > extent and not necessarily starting at zero offset. In this case > using the backing file's size doesn't make sense generally. > I see. > And because it's easy to avoid, I don't see why we'd need to force > using the backing inode attributes at this point. > > Your work on directory tree passthrough is related, but I think it's > separate enough to not mix their traits. When that is finalized we > can possibly add back mirroring of i_size on write, but I think the > general case shouldn't have that. > OK. I'll make it generic. Thanks, Amir.
On Fri, Oct 11, 2024 at 03:53:26PM +0200, Amir Goldstein wrote: > yangyun reported that libfuse test test_copy_file_range() copies zero > bytes from a newly written file when fuse passthrough is enabled. > > The reason is that extending passthrough write is not updating the fuse > inode size and when vfs_copy_file_range() observes a zero size inode, > it returns without calling the filesystem copy_file_range() method. > > Extend the fuse inode size to the size of the backing inode after every > passthrough write if the backing inode size is larger. > > This does not yet provide cache coherency of fuse inode attributes and > backing inode attributes, but it should prevent situations where fuse > inode size is too small, causing read/copy to be wrongly shortened. > > Reported-by: yangyun <yangyun50@huawei.com> > Closes: https://github.com/libfuse/libfuse/issues/1048 > Fixes: 57e1176e6086 ("fuse: implement read/write passthrough") > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > > Doh! force of habbit - fixed subject s/ovl/fuse > > Thanks, > Amir. > > fs/fuse/passthrough.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/fs/fuse/passthrough.c b/fs/fuse/passthrough.c > index ba3207f6c4ce..d3047a4bc40e 100644 > --- a/fs/fuse/passthrough.c > +++ b/fs/fuse/passthrough.c > @@ -20,9 +20,18 @@ static void fuse_file_accessed(struct file *file) > > static void fuse_file_modified(struct file *file) > { > + struct fuse_file *ff = file->private_data; > + struct file *backing_file = fuse_file_passthrough(ff); > struct inode *inode = file_inode(file); > - > - fuse_invalidate_attr_mask(inode, FUSE_STATX_MODSIZE); > + loff_t size = i_size_read(file_inode(backing_file)); > + > + /* > + * Most of the time we will be holding inode_lock(), but even if we are > + * called from async io completion without inode_lock(), the last write > + * will update fuse inode size to the size of the backing inode, even if > + * the last write was not the extending write. > + */ > + fuse_write_update_attr(inode, size, size); > } > > ssize_t fuse_passthrough_read_iter(struct kiocb *iocb, struct iov_iter *iter) > -- > 2.34.1 Sorry for a late reply. Because I have spent some time on figuring out whether we need some FUSE_I_SIZE_UNSTABLE bit operations before and after the write which are provided in your v14 patch of fuse passthrough, just like in fuse_perform_write. The conclusion is not, IMO. The FUSE_I_SIZE_UNSTABLE bit is provided in commit 06a7c3c2781(fuse: hotfix truncate_pagecache() issue) for the races between i_size updates and truncate_pagecache() in fuse_change_attributes. Because the pagecache operations of fuse inode is not allowed in fuse passthrough mode, this FUSE_I_SIZE_UNSTABLE bit is useless. And we just need the minimum fix for extending writes by now. I have also tested this patch with xfstests (using ./check -fuse -b) and libfuse test. In xfstest, this patch does not import new failed tests compared with pre-this patch. And in libfuse test, this patch can solve the problem test_copy_file_range().
diff --git a/fs/fuse/passthrough.c b/fs/fuse/passthrough.c index ba3207f6c4ce..d3047a4bc40e 100644 --- a/fs/fuse/passthrough.c +++ b/fs/fuse/passthrough.c @@ -20,9 +20,18 @@ static void fuse_file_accessed(struct file *file) static void fuse_file_modified(struct file *file) { + struct fuse_file *ff = file->private_data; + struct file *backing_file = fuse_file_passthrough(ff); struct inode *inode = file_inode(file); - - fuse_invalidate_attr_mask(inode, FUSE_STATX_MODSIZE); + loff_t size = i_size_read(file_inode(backing_file)); + + /* + * Most of the time we will be holding inode_lock(), but even if we are + * called from async io completion without inode_lock(), the last write + * will update fuse inode size to the size of the backing inode, even if + * the last write was not the extending write. + */ + fuse_write_update_attr(inode, size, size); } ssize_t fuse_passthrough_read_iter(struct kiocb *iocb, struct iov_iter *iter)
yangyun reported that libfuse test test_copy_file_range() copies zero bytes from a newly written file when fuse passthrough is enabled. The reason is that extending passthrough write is not updating the fuse inode size and when vfs_copy_file_range() observes a zero size inode, it returns without calling the filesystem copy_file_range() method. Extend the fuse inode size to the size of the backing inode after every passthrough write if the backing inode size is larger. This does not yet provide cache coherency of fuse inode attributes and backing inode attributes, but it should prevent situations where fuse inode size is too small, causing read/copy to be wrongly shortened. Reported-by: yangyun <yangyun50@huawei.com> Closes: https://github.com/libfuse/libfuse/issues/1048 Fixes: 57e1176e6086 ("fuse: implement read/write passthrough") Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- Doh! force of habbit - fixed subject s/ovl/fuse Thanks, Amir. fs/fuse/passthrough.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)