On 3/11/24 06:01, Miklos Szeredi wrote:
On Thu, 7 Mar 2024 at 16:10, Sweet Tea Dorminy
<sweettea-kernel@xxxxxxxxxx> wrote:
All calls into generic vfs functions need to make sure that the inode
attributes used by those functions are up to date, by calling
fuse_update_attributes() as appropriate.
generic_write_checks() accesses inode size in order to get the
appropriate file offset for files opened with O_APPEND. Currently, in
some cases, fuse_update_attributes() is not called before
generic_write_checks(), potentially resulting in corruption/overwrite of
previously appended data if i_size is out of date in the cached inode.
While this all sounds good, I don't think it makes sense.
Why? Because doing cached O_APPEND writes without any sort of
exclusion with remote writes is just not going to work.
Either the server ignores the current size and writes at the offset
that the kernel supplied (which will be the cached size of the file)
and executes the write at that position, or it appends the write to
the current EOF. In the former case the cache will be consistent, but
append semantics are not observed, while in the latter case the append
semantics are observed, but the cache will be inconsistent.
Solution: either exclude remote writes or don't use the cache.
Updating the file size before the write does not prevent the race,
only makes the window smaller.
Definitely agree with you.
The usecase at hand is a sort of NFS-like network filesystem, where
there's exclusion of remote writes while the file is open, but no
problem with remote writes while the file is closed.
The alternative we considered was to add a fuse_update_attributes() call
to open.
We thought about doing so during d_revalidate/lookup_fast(). But as far
as I understand, lookup_fast() is not just called during open, and will
use the cached inode if the dentry timeout hasn't expired. We tried
setting dentry timeout to 0, but that lost too much performance. So that
didn't seem to work.
But updating attributes after giving the filesystem a chance to
invalidate them during open() would work, I think?
That would also conveniently fix the issue where copy_file_range()
currently checks the size before calling into fuse at all, which I'd
been building a more elaborate changeset for.
How does that sound?
Thanks!
Sweet Tea
