[Nepomuk] Review Request: Reduce number of watches created by nepomukfilewatch
Vishesh Handa
me at vhanda.in
Thu Oct 11 05:13:31 UTC 2012
> On Oct. 8, 2012, 12:15 p.m., Vishesh Handa wrote:
> > services/filewatch/kinotify.cpp, line 157
> > <http://git.reviewboard.kde.org/r/106086/diff/2/?file=81961#file81961line157>
> >
> > Why wouldn't we want to walk the directory tree in subfolders we aren't indexing?
> >
> > They could have tags/rating and other metadata which we need to preserve on file move events, and delete on a file delete event.
>
> Simeon Bird wrote:
> We still watch the directory tree for subfolders we aren't indexing - it is only for subfolders on the filter list that we do not.
>
> This is a speed optimisation (see below for my reasoning). The idea is to treat folders on the exclude list like hidden folders.
>
> Simeon Bird wrote:
> Also we're going to need this for symlinks to work properly again, I think.
How is this related to systemlink handling?
Also, as much as I like speed improvments, I don't like the idea of sacrificing correctness over speed. Specially since this may be a very common case. Eg - You index your home directory, but do not index one particular sub-folder. You then have tags/ratings in that sub-folder.
- Vishesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106086/#review20070
-----------------------------------------------------------
On Aug. 28, 2012, 5:42 a.m., Simeon Bird wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106086/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2012, 5:42 a.m.)
>
>
> Review request for Nepomuk, Vishesh Handa and Sebastian Trueg.
>
>
> Description
> -------
>
> Current master nepomukfilewatcher installs watches on all sub-folders of a watched folder.
>
> This is problematic:
>
> - It means we have to walk the entire directory tree, even for not-indexed folders.
> This is quite a lot of work if you happen to have a large complex directory structure
> mounted over a network in your $HOME (as I do)
>
> - It means we get inotify watches for directories which are on the filter list; eg, on this computer
> $HOME/build/nepomuk-core/services/filewatch/CMakeFiles/nepomukfilewatch.dir/__/fileindexer
> is watched, causing the filewatcher to go nuts every time I build something.
>
> - It means we install many more watches than we need to, vastly increasing the probability
> of hitting the inotify limit.
>
> This code instead walks the tree until it finds a folder we don't want to index and then STOPS.
> I couldn't find a way to avoid walking the whole tree with QDirIterator and QDir::Subdirectories,
> so I use QDirIterator without subdirectories, then create a new QDirIterator for each subfolder to index.
>
> I can see two objections to this change:
>
> 1) If someone moves a file into an ignored directory, they will now presumably lose their metadata.
> This is true, in my opinion not a big problem; the default configuration is to watch
> $HOME minus temporary build directories. If people are moving files into temporary
> directories they should probably lose the metadata, and if people manually add directories
> to the ignore list they probably have a good reason and expect nepomuk to ignore them.
>
> 2) I changed filterWatch from always returning true to returning true if we want to watch the
> file and false otherwise. I couldn't work out the reason for it always returning true before,
> so whatever it was, I've probably broken it.
>
> Bonus fixes:
>
> - Properly pass the return value of addWatch up the tree, so that if we run out of watches,
> we stop trying to add more.
>
> - Check for inotify on kernels that have a two-number version string, like 3.0
>
> - To find the length of event->name, qstrlen was used. If an event is returned
> for a file outside a watched directory, event->name will not be allocated, and qstrlen
> may read beyond the end of allocated memory, causing chaos, anarchy and confusion.
> Use qstrnlen instead.
>
> Thanks, let me know what you think.
>
>
> Diffs
> -----
>
> services/filewatch/kinotify.h ab12d66
> services/filewatch/kinotify.cpp 4a744d4
> services/filewatch/nepomukfilewatch.cpp 9fd5d9c
>
> Diff: http://git.reviewboard.kde.org/r/106086/diff/
>
>
> Testing
> -------
>
> Compiled, run, used for a couple of days, checked which files were actually watched, timed the filewatch service's startup.
>
>
> Thanks,
>
> Simeon Bird
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/nepomuk/attachments/20121011/72ec8679/attachment-0001.html>
More information about the Nepomuk
mailing list