[Nepomuk] Review Request: Reduce number of watches created by nepomukfilewatch

[Simeon Bird]

> On Oct. 8, 2012, 12:15 p.m., Vishesh Handa wrote:
> > services/filewatch/nepomukfilewatch.cpp, line 107
> > <http://git.reviewboard.kde.org/r/106086/diff/2/?file=81962#file81962line107>
> >
> >     I'm a little confused about this.
> >     
> >     Shouldn't the shouldFolderBeIndexed on line 100 be enough? Why do we have additional checks to see if the file should be indexed? Specially since the relevant code is there in the shouldFolderByIndexed method.
> >     
> >     Also, returning false would make us not add any more watches for any of the subdirectories, which would result in problems. Cause we *always* require the watches, even when there is no indexed metadata. (Tags & Ratings)

I'm sorry, this is a bit confusing. 

The idea here is that (with this patch) folders can have three states: 

1) watched.
2) A watch installed which does not watch any events
3) No watch.

1) Corresponds to an indexed directory.
2) Corresponds to a directory which is not indexed.
3) Corresponds to a directory which is on the exclude filter list.

So the checks at line 107 are a subset of those done in shouldFolderBeIndexed; I guess a clearer way of writing it would be to add a function "IsFolderFiltered" or "shouldFolderBeWatched", which is called both in shouldFolderBeIndexed and in filterWatch.

Now, as you rightly point out, folders in state 3) cannot have metadata (tags and ratings), and if files are moved into these folders and out again, they may lose their metadata. By default the things on the exclude filter list are temporary build files and folders connected to version control systems: CMakeFiles, autom4te, CVS... which can't meaningfully have tags and ratings (they are temporary or hidden files). Installing many temporary watches on CMakeFiles slowed builds down noticeably for me (because nepomuk started watching CMakeFiles/file.o). So I am telling nepomuk "treat folders on the exclude filter list as if they were hidden, even though they do not start with a '.'"

So with the default configuration, all this does is restore the assumption that temporary build directories have no tags (which was present in 4.9). But the main benefit is that it allows the user to specify extra directories that will certainly have no tags in either, and the user doesn't want watched, for whatever reason.

One particular case I wanted nepomuk to ignore was remote filesystems. I often use sshfs to mount (very complex) remote filesystems over ~/sshfs. Installing watches on every single subdirectory - in fact just walking the directory tree - is extremely slow and generates quite a bit of network traffic. There can't be any tags on this filesystem, because I will mount completely different remote machines at the same place. I know this, but nepomuk cannot. Probably the correct way to handle this is to add a PRUNEFS feature to nepomuk, like that of mlocate, but at the moment I just add the directory to the filter list by hand. There are probably other scenarios like this we haven't thought of, so I figured the user needs a way to do a manual override.

Does that make it clearer? Does it make sense at all? :) 


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

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


-----------------------------------------------------------
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/20121008/9fc7205b/attachment-0001.html>