On Wed, 25 Sept 2024 at 16:07, Bernd Schubert <bernd.schubert@xxxxxxxxxxx> wrote: > > Hi Miklos, > > On 9/25/24 14:20, Miklos Szeredi wrote: > > On Thu, 15 Aug 2024 at 16:45, Laura Promberger <laura.promberger@xxxxxxx> wrote: > > > >> - But for corrupted symlinks `fuse_change_attributes()` exits before `fuse_change_attributes_common()` is called and as such the length stays the old one. > > > > The reason is that the attr_version check fails. The trace logs show > > a zero attr_version value, which suggests that the check can not fail. > > But we know that fuse_dentry_revalidate() supplies a non-zero > > attr_version to fuse_change_attributes() and if there's a racing > > fuse_reverse_inval_inode() which updates the fuse_inode's > > attr_version, then it would result in fuse_change_attributes() exiting > > before updating the cached attributes, which is what you observe. > > > I'm a bit confused by this, especially due to "fuse_reverse_inval_inode()", > isn't this about FUSE_NOTIFY_INVAL_ENTRY and the additional flag > FUSE_EXPIRE_ONLY? I.e. the used code path is fuse_reverse_inval_entry()? > And that path doesn't change the attr_version? Which I'm also confused > about. The trace does have several fuse_reverse_inval_inode() calls, which made me conclude that this was the cause. > > This is probably okay, as the cached attributes remain invalid and the > > next call to fuse_change_attributes() will likely update the inode > > with the correct values. > > > > The reason this causes problems is that cached symlinks will be > > returned through page_get_link(), which truncates the symlink to > > inode->i_size. This is correct for filesystems that don't mutate > > symlinks, but for cvmfs it causes problems. > > > > My proposed solution would be to just remove this truncation. This > > can cause a regression in a filesystem that relies on supplying a > > symlink larger than the file size, but this is unlikely. If that > > happens we'd need to make this behavior conditional. > > I wonder if we can just repeat operations if we detect changes in the > middle. Hard started to work on a patch, but got distracted and I > first would like to create a passthrough reproducer. I think in this case it's much cleaner to just ignore the file size. Old, non-cached readlink code never did anything with i_size, why should the cached one care about it? Thanks, Miklos
