Use solid to check if a KFileItem is located on a network mount
Needs RevisionPublic

Authored by meven on Aug 25 2019, 8:32 AM.

Details

Reviewers
dfaure
Group Reviewers
Frameworks
Summary

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.

Diff Detail

Repository
R241 KIO
Branch
arcpatch-D23420
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 15687
Build 15705: arc lint + arc unit
meven created this revision.Aug 25 2019, 8:32 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptAug 25 2019, 8:32 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
meven requested review of this revision.Aug 25 2019, 8:32 AM
anthonyfieroni added inline comments.
src/core/kfileitem.cpp
766–770

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.

meven updated this revision to Diff 64538.Aug 25 2019, 8:48 AM

Fix logic

meven marked an inline comment as done.Aug 25 2019, 8:48 AM
dfaure accepted this revision.Aug 25 2019, 10:29 AM
dfaure added inline comments.
src/core/kfileitem.cpp
763

const &

This revision is now accepted and ready to land.Aug 25 2019, 10:29 AM
meven marked an inline comment as done.Aug 26 2019, 5:43 AM
meven added inline comments.
src/core/kfileitem.cpp
763

Can't because of

Solid::StorageAccess *storageAccess = device.as<Solid::StorageAccess>();
anthonyfieroni added inline comments.Aug 26 2019, 6:47 AM
src/core/kfileitem.cpp
763

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! :)

meven updated this revision to Diff 64645.Aug 26 2019, 8:37 AM
meven marked an inline comment as done.

Add a const

meven marked 2 inline comments as done.Aug 26 2019, 8:38 AM

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
765

If you have a location /home/foo/foobar and a slow mount /home/foo/foo, it will consider it slow because "starts with"

ngraham retitled this revision from Use solid to check if a a KFileItem is located on a network mount to Use solid to check if a KFileItem is located on a network mount.Aug 26 2019, 2:05 PM
meven updated this revision to Diff 64715.Aug 27 2019, 6:59 AM

better check file path

meven marked an inline comment as done.Aug 27 2019, 6:59 AM

Now we still need the linkDest check :)

anthonyfieroni added inline comments.Aug 27 2019, 7:19 AM
src/core/kfileitem.cpp
763

Let clarify what David mean

for (const Solid::Device& device : devices) {
    auto storageAccess = device.as<const Solid::StorageAccess>();
meven updated this revision to Diff 64716.Aug 27 2019, 7:43 AM

Add a const& clean unused variable

meven marked an inline comment as done.Aug 27 2019, 7:44 AM

Now we still need the linkDest check :)

isSlow() didn't check linkDest before.

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.

dfaure requested changes to this revision.Aug 28 2019, 7:11 AM
dfaure added inline comments.
src/core/kfileitem.cpp
765

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.

This revision now requires changes to proceed.Aug 28 2019, 7:11 AM
meven added a comment.EditedSep 5 2019, 3:23 PM

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....

bruns added a subscriber: bruns.Sep 10 2019, 6:25 PM

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.