KFileSystemType uses statvfs syscall that blocks when a network fs mount becomes unresponsive because of a network disconnect for instance.
The patch uses solid mount discovery to determine if the file is on a network mount.
Details
- Reviewers
dfaure - Group Reviewers
Frameworks
Diff Detail
- Repository
- R241 KIO
- Branch
- solid-network-fs-check
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 15648 Build 15666: arc lint + arc unit
src/core/kfileitem.cpp | ||
---|---|---|
767–771 | Wrong logic, when you set it to Slow, after break it will go to line 771 and became Fast again. Set it Fast before loop or check whether you change the m_slow value. |
src/core/kfileitem.cpp | ||
---|---|---|
764 | const & |
src/core/kfileitem.cpp | ||
---|---|---|
764 | Can't because of Solid::StorageAccess *storageAccess = device.as<Solid::StorageAccess>(); |
src/core/kfileitem.cpp | ||
---|---|---|
764 | Get it as const Solid::StorageAccess, you can use auto storageAccess = ... |
I'm always hesitant to use Solid but since KFilePlacesModel already queries Solid for everything, it has already loaded all its backends and queried everything, so the overhead of this during normal Dolphin use, is surprisingly negligible.
+1 good call! :)
What about symlink resolution, though?
Maybe this needs to check linkDest(), too. This might block but for our usecase in local files you have UDS_LINK_DEST populated on the kioslave side already.
src/core/kfileitem.cpp | ||
---|---|---|
766 | If you have a location /home/foo/foobar and a slow mount /home/foo/foo, it will consider it slow because "starts with" |
src/core/kfileitem.cpp | ||
---|---|---|
764 | Let clarify what David mean for (const Solid::Device& device : devices) { auto storageAccess = device.as<const Solid::StorageAccess>(); |
isSlow() didn't check linkDest before.
Even more reason to do it properly now :P
KFileSystemType uses statfs. I didn't find any documentation on whether it follows symlinks, but it is documented to return an ELOOP error code "Too many symbolic links were encountered in translating path.", so maybe it does so implicitly, which we lost now.
src/core/kfileitem.cpp | ||
---|---|---|
766 | But going via QUrl to fix this seems overkill to me. It would be much faster to do this with the usual "location==mount or location.startsWith(mount+'/')". I'm afraid this code will be called often (many KFileItems) and has to iterate over all mounts every time, so this is N*M. |
I didn't know about KMount
Gwenview uses it as we are trying here, in urlutils.cpp :
bool urlIsFastLocalFile(const QUrl &url) { if (!url.isLocalFile()) { return false; } KMountPoint::List list = KMountPoint::currentMountPoints(); KMountPoint::Ptr mountPoint = list.findByPath(url.toLocalFile()); if (!mountPoint) { // We couldn't find a mount point for the url. We are probably in a // chroot. Assume everything is fast then. return true; } return !mountPoint->probablySlow(); }
Heh, yeah, we have 3 APIs about mounted filesystems. Solid, KMountPoint, and KFileSystemType.
KFileSystemType uses the blocking statvfs, while KMountPoint relies on a properly filled in /etc/fstab -- which breaks on the FreeBSD CI these days, some weird magic with containers not needing anything in /etc/fstab.
It seems to me that we're still looking for the silver bullet solution in this area....
Solid should be quite fine. When queried for devices providing 'Solid::DeviceInterface::NetworkShare', it will only use the fstab backend, which does not block and only reparses fstab/mtab when these change.
There are some unnecessary QString constructions in the fstab backend, but these are easily solvable.
For better performance, a global static cache with the network filepath()s would be an option.
Abandoned in favor of D26407 using KMountPoint which does close to the same thing as solid.