D19784: Avoid calling QT_LSTAT and accessing recent documents

[Méven Car]

meven added a comment.


  In D19784#503066 , @dfaure wrote:
  
  > 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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D19784

To: hoffmannrobert, #frameworks, dfaure, #dolphin
Cc: meven, apol, ngraham, plasma-devel, LeGast00n, jraleigh, fbampaloukas, 
GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, mart

D19784: Avoid calling QT_LSTAT and accessing recent documents

[David Faure]

dfaure added a comment.


  See KFileItem::isSlow()

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D19784

To: hoffmannrobert, #frameworks, dfaure, #dolphin
Cc: meven, apol, ngraham, plasma-devel, LeGast00n, jraleigh, fbampaloukas, 
GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, mart

D19784: Avoid calling QT_LSTAT and accessing recent documents

[Méven Car]

meven added a comment.


  Since this patch icons for folders in recent documents have been replaced 
with the application-octet-stream icon.
  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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D19784

To: hoffmannrobert, #frameworks, dfaure, #dolphin
Cc: meven, apol, ngraham, plasma-devel, LeGast00n, jraleigh, fbampaloukas, 
GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, mart

D19784: Avoid calling QT_LSTAT and accessing recent documents

[David Faure]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:408f03ca989e: Avoid calling QT_LSTAT and accessing recent 
documents (authored by hoffmannrobert, committed by dfaure).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19784?vs=55115=55142

REVISION DETAIL
  https://phabricator.kde.org/D19784

AFFECTED FILES
  applets/kicker/plugin/recentusagemodel.cpp

To: hoffmannrobert, #frameworks, dfaure, #dolphin
Cc: apol, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D19784: Avoid calling QT_LSTAT and accessing recent documents

[Robert Hoffmann]

hoffmannrobert updated this revision to Diff 55115.
hoffmannrobert added a comment.


  - Add KIO_VERSION check, use SkipMimeTypeFromContent

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19784?vs=54863=55115

BRANCH
  fix_recent_documents_kicker_hang

REVISION DETAIL
  https://phabricator.kde.org/D19784

AFFECTED FILES
  applets/kicker/plugin/recentusagemodel.cpp

To: hoffmannrobert, #frameworks, dfaure, #dolphin
Cc: apol, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D19784: Avoid calling QT_LSTAT and accessing recent documents

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  This needs a KIO version ifdef for KIO >= 5.57.
  
  `#if KIO_VERSION >= QT_VERSION_CHECK(5,57,0)`

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D19784

To: hoffmannrobert, #frameworks, dfaure, #dolphin
Cc: apol, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D19784: Avoid calling QT_LSTAT and accessing recent documents

[Robert Hoffmann]

hoffmannrobert updated this revision to Diff 54863.
hoffmannrobert added a comment.


  - Use new KFileItem::SkipMimeTypeDetermination parameter

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19784?vs=54339=54863

BRANCH
  fix_recent_documents_kicker_hang

REVISION DETAIL
  https://phabricator.kde.org/D19784

AFFECTED FILES
  applets/kicker/plugin/recentusagemodel.cpp

To: hoffmannrobert, #frameworks, dfaure, #dolphin
Cc: apol, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D19784: Avoid calling QT_LSTAT and accessing recent documents

[Robert Hoffmann]

hoffmannrobert added a comment.


  In D19784#437715 , @dfaure wrote:
  
  >
  
  
  ...
  
  > 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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D19784

To: hoffmannrobert, #frameworks, dfaure, #dolphin
Cc: apol, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D19784: Avoid calling QT_LSTAT and accessing recent documents

[David Faure]

dfaure added a comment.


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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D19784

To: hoffmannrobert, #frameworks, dfaure, #dolphin
Cc: apol, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D19784: Avoid calling QT_LSTAT and accessing recent documents

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D19784

To: hoffmannrobert, #frameworks, dfaure, #dolphin
Cc: apol, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D19784: Avoid calling QT_LSTAT and accessing recent documents

[Robert Hoffmann]

hoffmannrobert added a comment.


  In D19784#434564 , @hoffmannrobert 
wrote:
  
  > In D19784#431683 , @ngraham 
wrote:
  >
  > > 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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D19784

To: hoffmannrobert, #frameworks, dfaure, #dolphin
Cc: apol, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D19784: Avoid calling QT_LSTAT and accessing recent documents

[Robert Hoffmann]

hoffmannrobert marked an inline comment as done.
hoffmannrobert added a comment.


  In D19784#431683 , @ngraham wrote:
  
  > 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").

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D19784

To: hoffmannrobert, #frameworks, dfaure, #dolphin
Cc: apol, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D19784: Avoid calling QT_LSTAT and accessing recent documents

[Robert Hoffmann]

hoffmannrobert marked 3 inline comments as done.
hoffmannrobert added a comment.


  See https://phabricator.kde.org/D19887

INLINE COMMENTS

> dfaure wrote in recentusagemodel.cpp:261
> How do you expect isFile() and isDir() to work without a QT_LSTAT call?

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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D19784

To: hoffmannrobert, #frameworks, dfaure, #dolphin
Cc: apol, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D19784: Avoid calling QT_LSTAT and accessing recent documents

[Robert Hoffmann]

hoffmannrobert updated this revision to Diff 54339.
hoffmannrobert added a comment.


  - Use new KFileItem constructor with skipStat set to true

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19784?vs=53951=54339

BRANCH
  fix_recent_documents_kicker_hang

REVISION DETAIL
  https://phabricator.kde.org/D19784

AFFECTED FILES
  applets/kicker/plugin/recentusagemodel.cpp

To: hoffmannrobert, #frameworks, dfaure, #dolphin
Cc: apol, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D19784: Avoid calling QT_LSTAT and accessing recent documents

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> recentusagemodel.cpp:261
>  
>  if (!url.isValid() || !(fileItem.isFile() || fileItem.isDir())) {
>  return QVariant();

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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D19784

To: hoffmannrobert, #frameworks, dfaure, #dolphin
Cc: apol, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D19784: Avoid calling QT_LSTAT and accessing recent documents

[Aleix Pol Gonzalez]

apol added inline comments.

INLINE COMMENTS

> recentusagemodel.cpp:254
> +// return any useful information for our purposes here.
> +url.setScheme(QString());
> +KFileItem fileItem(url);

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?

> recentusagemodel.cpp:277
> +// We want MatchMode mode = MatchExtension
> +if (url.scheme() == "file") {
> +url.setScheme(QString());

url.isLocalFile()

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D19784

To: hoffmannrobert, #frameworks, dfaure, #dolphin
Cc: apol, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D19784: Avoid calling QT_LSTAT and accessing recent documents

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D19784

To: hoffmannrobert
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D19784: Avoid calling QT_LSTAT and accessing recent documents

[Robert Hoffmann]

hoffmannrobert created this revision.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
hoffmannrobert requested review of this revision.

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

REPOSITORY
  R119 Plasma Desktop

BRANCH
  fix_recent_documents_kicker_hang

REVISION DETAIL
  https://phabricator.kde.org/D19784

AFFECTED FILES
  applets/kicker/plugin/recentusagemodel.cpp

To: hoffmannrobert
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart