| Summary: | [PATCH] KDirwatch doesn't detect remote file changes on NFS | ||
|---|---|---|---|
| Product: | [Unmaintained] kio | Reporter: | Travers Carter <tcarter> |
| Component: | general | Assignee: | David Faure <faure> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | flavio |
| Priority: | NOR | ||
| Version: | 4.1 | ||
| Target Milestone: | --- | ||
| Platform: | Fedora RPMs | ||
| OS: | Linux | ||
| Latest Commit: | Version Fixed In: | ||
| Sentry Crash Report: | |||
| Attachments: |
Add nfsPreferredMethod config key to KDirwatch
Add nfsPreferredMethod config key to KDirwatch |
||
|
Description
Travers Carter
2008-12-16 11:23:06 UTC
Created attachment 29374 [details]
Add nfsPreferredMethod config key to KDirwatch
Nice patch, thanks. Are you sure about this change, though? - if (useINotify(e)) return; + if (useINotify(e)); return; While I'm asking about that, can I also suggest a helper function (file-static) for converting a string to a WatchMethod enum, in order to reduce the duplication between the two "read from config" bits of code? Thanks. Created attachment 29389 [details]
Add nfsPreferredMethod config key to KDirwatch
Yes, your are right that iNotify change shouldn't be there, I've fixed it in the new version that I just attached. I've also added a static method methodFromString for the common bit of the config mapping, my C++ is a little rusty, but just declaring the function static limits it to the file scope right? is that what you had in mind? Yep, the static function looks good (should take a const QString& though).
I almost committed this, but I realized I have one more question :-)
I noticed that the default is "same method for nfs as elsewhere", which doesn't really fix the bug (it still requires users to read this code and add a key to a config file), right?
If NFS mounts don't work with inotify, shouldn't there be logic like
if (slowmounted) {
if (nfspreferredmethod was set in config file)
use that
else if (m_preferredMethod == inotify)
use stat
else
use m_preferredMethod
}
?
Changing the default order for NFS was something I was considered, but the situation is a bit more complex: iNotify will detect changes on NFS when they originate from the local system - which makes it a reasonable choice if the NFS filesystem is only expected to be modified from a single client in general, for example when user home directories are NFS mounted. FAM (the original one), supports a client-server mode where it can run a watch daemon on the server and propagate events to the client, but gamin which Fedora (and I believe most current Linux distos) now use instead of FAM doesn't support this feature. I'm not sure other platforms though. Stat is the only method that is guaranteed to detect remote changes on NFS, but it has a fairly high overhead, can take up to 5 seconds (by default) to detect changes and from reading some gamin bug reports (gamin polls on NFS by default too) can end up placing a significant load on NFS servers with a large number of clients. I guess the question is which is the best trade off for an NFS default, and I'm not sure of the answer, which is why I took the approach of making it configurable, in my particular case I have less than 20 users who need to see changes on shared NFS mounts, and don't have a FAM server, so Stat is the best choice for me. Good point about NFS-mounted homes, a default value of inotify makes sense indeed. We just need to document this setting on userbase.kde.org, then. SVN commit 898321 by dfaure: Add nfsPreferredMethod config key to KDirwatch, patch by Travers Carter. This allows to configure a different method for watching NFS-mounted directories, but the default is to simply use the same method as normally, so this patch is a safe no-op by default. BUG: 177892 M +48 -40 kdirwatch.cpp M +1 -1 kdirwatch_p.h WebSVN link: http://websvn.kde.org/?view=rev&revision=898321 Committed, I'll let you add something to the userbase wiki so that others can figure this out :-) |