After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 711459 - Only enumerate calls trash_watcher_rescan
Only enumerate calls trash_watcher_rescan
Status: RESOLVED FIXED
Product: gvfs
Classification: Core
Component: trash backend
git master
Other Linux
: Normal normal
: ---
Assigned To: Allison Karlitskaya (desrt)
gvfs-maint
Depends on:
Blocks: 763218 763600
 
 
Reported: 2013-11-05 07:54 UTC by Ross Lagerwall
Modified: 2016-03-31 14:37 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
trash: Rescan trash dirs before operations with files (2.02 KB, patch)
2016-03-15 11:00 UTC, Ondrej Holy
none Details | Review
trash: Check modification time to avoid rereading (2.44 KB, patch)
2016-03-15 11:01 UTC, Ondrej Holy
committed Details | Review
trash: Rescan trash dirs before operations with files (2.26 KB, patch)
2016-03-31 14:34 UTC, Ondrej Holy
committed Details | Review

Description Ross Lagerwall 2013-11-05 07:54:21 UTC
Because only enumerate() calls trash_watcher_rescan(), any operation invoked before enumerate() is first called on a file in the trash backend fails.

Eg:

$ pkill gvfs

$ gvfs-cat trash:///afile.txt

gvfs-cat: trash:///afile.txt: error opening file: No such file or directory

$ gvfs-info trash:///
display name: Trash
name: /
type: directory
uri: trash:///
attributes:
  standard::type: 2
  standard::name: /
  standard::display-name: Trash
  standard::icon: user-trash
  standard::content-type: inode/directory
  standard::symbolic-icon: user-trash-symbolic
  id::filesystem: trash:
  trash::item-count: 0 <--- note


$ gvfs-ls trash:///
afile.txt

$ gvfs-cat trash:///afile.txt
some text

$ gvfs-info trash:///
display name: Trash
name: /
type: directory
uri: trash:///
attributes:
  standard::type: 2
  standard::name: /
  standard::display-name: Trash
  standard::icon: user-trash
  standard::content-type: inode/directory
  standard::symbolic-icon: user-trash-symbolic
  id::filesystem: trash:
  trash::item-count: 1
Comment 1 Ondrej Holy 2016-03-15 11:00:13 UTC
Created attachment 323964 [details] [review]
trash: Rescan trash dirs before operations with files

Patches to use G_FILE_ATTRIBUTE_TRASH_ITEM_COUNT were recently proposed for Nautilus and GTK+. We have to be sure that the item count is valid and therefore we have to rescan the trash dirs before. Otherwise the count is 0 if enumeration isn't called before, which is obviously wrong. Similarly, trash dirs have to be rescanned before other operations (i.e. read, delete, pull), otherwise the operations may fail with G_IO_ERROR_NOT_FOUND if enumeration wasn't called before.
Comment 2 Ondrej Holy 2016-03-15 11:01:10 UTC
Created attachment 323965 [details] [review]
trash: Check modification time to avoid rereading

Rereading of trash dirs is pretty expensive and unfortunately it is performed more often with recent changes. Check modification time of each trash dir in order to avoid needless rereading, because simple query_info call is much faster than enumeration.
Comment 3 Ondrej Holy 2016-03-15 11:54:41 UTC
This is not probably the best way how to test performance, but it shows some important values:

Query info for 5000 files in the trash without any additional patches (dir monitoring is enabled):
$ time gvfs-info $(gvfs-ls -c trash:///) > /dev/null

real	0m4.285s
user	0m2.206s
sys	0m0.284s

Query info for 5000 files in the trash without any additional patches (dir monitoring is not enabled):
$ time gvfs-info $(gvfs-ls -c trash:///) > /dev/null

real	0m4.383s
user	0m2.240s
sys	0m0.260s

Query info for 5000 files in the trash with the first patch only (dir monitoring is enabled):
$ time gvfs-info $(gvfs-ls -c trash:///) > /dev/null

real	0m5.521s
user	0m2.339s
sys	0m0.265s

Query info for 5000 files in the trash with the first patch only (dir monitoring is not enabled):
$ time gvfs-info $(gvfs-ls -c trash:///) > /dev/null

real	4m24.525s
user	0m3.157s
sys	0m0.352s

Query info for 5000 files in the trash with the both patches (dir monitoring is enabled):
$ time gvfs-info $(gvfs-ls -c trash:///) > /dev/null

real	0m5.400s
user	0m2.303s
sys	0m0.262s

Query info for 5000 files in the trash with the both patches (dir monitoring is not enabled):
$ time gvfs-info $(gvfs-ls -c trash:///) > /dev/null

real	0m5.572s
user	0m2.332s
sys	0m0.256s

You can see a significant slowdown with first patch only, when dir monitoring is not enabled. It is caused by additional rescan of trash directories. You can also see that this slowdown is almost reduced by the second patch which avoids rereading if modification time is not changed...
Comment 4 Ondrej Holy 2016-03-15 11:56:13 UTC
Ryan, can you please take a look at the patches?
Comment 5 Ondrej Holy 2016-03-15 12:05:29 UTC
(In reply to Ondrej Holy from comment #1)
> Created attachment 323964 [details] [review] [review]
> trash: Rescan trash dirs before operations with files
> 
> Patches to use G_FILE_ATTRIBUTE_TRASH_ITEM_COUNT were recently proposed for
> Nautilus and GTK+. We have to be sure that the item count is valid and
> therefore we have to rescan the trash dirs before. 

Another solution might be to call enumeration once before trash monitor is established (e.g. from Nautilus and GTK+), but this patch fixes more problems than just wrong item-count...
Comment 6 Allison Karlitskaya (desrt) 2016-03-21 17:01:50 UTC
Review of attachment 323964 [details] [review]:

Do we really need all the ops here?

I think my original reasoning was that one cannot know the name of a file in the trash without first having enumerated it...

I'm definitely okay with adding this for query-info on the root object if we have not yet established a watch...
Comment 7 Ondrej Holy 2016-03-22 09:07:38 UTC
You are right that nobody can't know the name of file without enumeration, but e.g. some applications might want to reopen files from last time and it may fail in such case... maybe recent files should not include files from trash... but still we can't be sure that some application doesn't remember the files itself...
Comment 8 Ondrej Holy 2016-03-31 14:34:22 UTC
Created attachment 325089 [details] [review]
trash: Rescan trash dirs before operations with files

I improved the patch to call trash_watcher_rescan only if watch is not established yet as suggested in Comment 6. However it is called from all the operations as before due to reasons mentioned in Comment 7.
Comment 9 Ondrej Holy 2016-03-31 14:36:41 UTC
Pushed as commit be29e48 and commit 7d0ef03 since the gtk+ patch is already pushed...