Avoid calling QT_LSTAT and accessing recent documents
ClosedPublic

Authored by hoffmannrobert on Mar 15 2019, 1:01 PM.

Details

Summary

Document entries pointing to unavailable network shares freeze
the application menu, so avoid calling QT_LSTAT (lstat64())
three times for each list item and reading the first 16k of
each document when showing the recent documents list in application
menu.

The current behaviour of accessing each document in the list
just by browsing the application menu is inacceptable in large
enterprise environments where thousands of users work
with documents located on mounted network shares. This induces
additional load on filers and network, slows down working
with the menu and makes its functioning dependent on network
and remote filesystems.

Test Plan
  1. Open a document on a mounted network share, e.g. a text document on a CIFS share with Libreoffice Writer or Kate.
  2. Open the application menu (kicker), look at Recent Documents, the document's filename should be there.
  3. Pull out the network plug, open the application menu, try to open Recent Documents. Without this patch, the application menu doesn't work anymore and just hangs. With this patch applied, the menu continues working.

Diff Detail

Repository
R119 Plasma Desktop
Branch
fix_recent_documents_kicker_hang
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10155
Build 10173: arc lint + arc unit
hoffmannrobert created this revision.Mar 15 2019, 1:01 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 15 2019, 1:01 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
hoffmannrobert requested review of this revision.Mar 15 2019, 1:01 PM

Nice, kinda sounds like this fixes https://bugs.kde.org/show_bug.cgi?id=373352. Can you confirm?

apol added a subscriber: apol.Mar 15 2019, 6:47 PM
apol added inline comments.
applets/kicker/plugin/recentusagemodel.cpp
254

This looks very much like a workaround. How about adding an argument to the KFileItem to skip the stat if it's a desirable behaviour?

267

url.isLocalFile()

dfaure requested changes to this revision.Mar 15 2019, 7:38 PM
dfaure added inline comments.
applets/kicker/plugin/recentusagemodel.cpp
251

How do you expect isFile() and isDir() to work without a QT_LSTAT call?

This revision now requires changes to proceed.Mar 15 2019, 7:38 PM
  • Use new KFileItem constructor with skipStat set to true
hoffmannrobert marked 3 inline comments as done.Mar 19 2019, 4:45 PM
applets/kicker/plugin/recentusagemodel.cpp
251

A document should always be a file, so not needed.

hoffmannrobert marked an inline comment as done.Mar 19 2019, 5:00 PM

Nice, kinda sounds like this fixes https://bugs.kde.org/show_bug.cgi?id=373352. Can you confirm?

No, unfortunately it doesn't fix this. If the network isn't available, the file dialog waits for KCoreDirLister ("Iterating over dirs").

Nice, kinda sounds like this fixes https://bugs.kde.org/show_bug.cgi?id=373352. Can you confirm?

No, unfortunately it doesn't fix this. If the network isn't available, the file dialog waits for KCoreDirLister ("Iterating over dirs").

After debugging opening a file dialog in kate and firefox, I found it's not KCoreDirLister but the Recent Files[$e] entries in .config/katerc and .config/kdialogrc which cause the hang.
If they point to files on a network drive, and the network or the drive is not responding, KUrlComboBox::setUrls() is trying to access them at if (u.isLocalFile() && !QFile::exists(u.toLocalFile())) {.... The QFile::exists() calls QFileInfo::exists()which is hanging at a __xstat64() call.

dfaure requested changes to this revision.Mar 24 2019, 10:33 PM

OK so this is about KFileItem::text() and KFileItem::iconName().

Indeed this doesn't need the stat() done by KFileItem's init(). This means the right solution is indeed for KFileItem to do that stat() on demand, and this code doesn't need any changes.

This revision now requires changes to proceed.Mar 24 2019, 10:33 PM

If they point to files on a network drive, and the network or the drive is not responding

Well that's exactly the problem with network mounts, and the reason they are a sucky technical solution.
KIO's async jobs never have that problem.

You will never be able to remove all uses of synchronous local file APIs in all of Qt and KDE-made software -- or IMHO any other large toolkit or application.

At best, the kernel should offer better solutions for users to get rid of non-responding mounts more easily.

...

Indeed this doesn't need the stat() done by KFileItem's init(). This means the right solution is indeed for KFileItem to do that stat() on demand, and this code doesn't need any changes.

Wait, my comment about recent files only refers to this bug: https://bugs.kde.org/show_bug.cgi?id=373352

The original problem about the application menu not responding can still be solved by not doing the stat() in the KFileItemPrivate::init() AND not doing the mime type determination by content.

So, as you say in https://phabricator.kde.org/D19887 the init() (and the stat()) should be done only on demand and the new KFileItem constructor with the SkipMimetypeDetermination parameter should be called from here.

  • Use new KFileItem::SkipMimeTypeDetermination parameter
dfaure accepted this revision.Mar 27 2019, 8:27 AM
This revision is now accepted and ready to land.Mar 27 2019, 8:27 AM
dfaure requested changes to this revision.EditedMar 30 2019, 2:36 PM

This needs a KIO version ifdef for KIO >= 5.57.

#if KIO_VERSION >= QT_VERSION_CHECK(5,57,0)

(with #include <kio_version.h>)

This revision now requires changes to proceed.Mar 30 2019, 2:36 PM
  • Add KIO_VERSION check, use SkipMimeTypeFromContent
dfaure accepted this revision.Mar 31 2019, 5:35 PM
This revision is now accepted and ready to land.Mar 31 2019, 5:35 PM
This revision was automatically updated to reflect the committed changes.
meven added a subscriber: meven.EditedJul 26 2019, 8:10 AM

Since this patch icons for folders in recent documents have been replaced with the application-octet-stream icon as the default as their mime-type is not checked anymore.
SkipMimeTypeFromContent should be avoided for local directories.

We need a better way to detect if the folder is a network folder before making assumptions.
isLocalFile is probably not enough given mounted drives can have urls such as file://mount.
Could we for instance use solid to detect if a folder is a network one before skipping mime type detection ?

CCBUG: 401579

See KFileItem::isSlow()

meven added a comment.Jul 27 2019, 8:22 PM

See KFileItem::isSlow()

Interesting but the initial issue happened when a drive is not mounted and isSlow implementation uses statfs that gives information about ... mounted filesystem.
We need the same sort of information but with "cold" unmounted filesystem.