GNOME Bugzilla – Bug 688371
add support for GKqueueDirectoryMonitor
Last modified: 2012-11-16 07:21:37 UTC
Created attachment 229027 [details] [review] tracker-monitor: add support for GKqueueDirectoryMonitor Hi. kqueue(2) support for BSD systems was recently introduced in GLib. Make tracker aware of this new backend. See GLib commit: http://git.gnome.org/browse/glib/commit/?id=a335fd1de8fc2ab4b26c5fe6055014ea15043fb9 I don't know if the ifdef parts are OK from your point of view, I could always rework it to better suit your expectations if needed. Thanks.
Hello Antoine, Thank you for the patch. My comments: 1. Can we remove this? +/* getrlimit(2) */ 2. The TRACKER_MONITOR_KQUEUE stuff for the strcmp() shouldn't be necessary. At least this is a run-time check and if the backend is there, then it is there. This really depends on what GLib provides in the end, not where Tracker is built (i.e. on BSD). 3. Why do we initially set limit to 400 why not another number? I ask because in terms of directories to monitor, 400 is a really low number. 8k seems to be used in a lot of places on Ubuntu and Fedora I think for inotify. People even then still complain they run out of inotify watches. I am just worried that 400 is too low if it ends up being used. + guint limit = 400; 4. Small coding style thing, please use spaces between operators like * and / to make the code more easily understood or read by others. On the other hand, we should use this approach for our inotify upper limit instead of x - 500 :) + limit = (rl.rlim_cur*90)/100; 5. Why limit this? You have a comment there about preventing too many FDs being open, but shouldn't that be limited anyway by ulimit? Setting an arbitrary number here might be counter productive. Regarding my earlier comment of 8k with inotify, I wonder if this would be too low anyway? + if (limit > 2048) -- Overall, it's a great patch. The only thing I am not sure about is if we really need the TRACKER_MONITOR_KQUEUE work in the first place. It adds a small maintenance overhead especially if we end up with this API on other unix systems at some point too. I actually don't know if that's a possibility either (it being available on other unix systems). Thoughts?
(In reply to comment #1) > Hello Antoine, > > Thank you for the patch. My comments: Hi Martin. Well thanks for the review :-) > 1. Can we remove this? > > +/* getrlimit(2) */ Certainly. > > 2. The TRACKER_MONITOR_KQUEUE stuff for the strcmp() shouldn't be necessary. At > least this is a run-time check and if the backend is there, then it is there. > This really depends on what GLib provides in the end, not where Tracker is > built (i.e. on BSD). Yes you're right. It was a leftover from a previous version of this diff. I'll remove it. > 3. Why do we initially set limit to 400 why not another number? I ask because > in terms of directories to monitor, 400 is a really low number. 8k seems to be > used in a lot of places on Ubuntu and Fedora I think for inotify. People even > then still complain they run out of inotify watches. I am just worried that 400 > is too low if it ends up being used. Because of this: $ ulimit -n 512 These are the default limits on OpenBSD... I think that if the call fails, then the fallback limit should be within a range that would work anywhere, whatever default ulimit is. 400 comes from the Fam/Gamin monitor thing below. > + guint limit = 400; > > 4. Small coding style thing, please use spaces between operators like * and / > to make the code more easily understood or read by others. On the other hand, > we should use this approach for our inotify upper limit instead of x - 500 :) > > + limit = (rl.rlim_cur*90)/100; Sure :-) > > 5. Why limit this? You have a comment there about preventing too many FDs being > open, but shouldn't that be limited anyway by ulimit? Setting an arbitrary > number here might be counter productive. Regarding my earlier comment of 8k > with inotify, I wonder if this would be too low anyway? > > + if (limit > 2048) The issue really is that if you start using an insane amount of FDs, then the machine will start crawling. Also the total max open files on OpenBSD is "7030". So if for e.g. you setup your user to have a max nofiles of 5120, kqueue will eat most of the system-wide available openfiles. I guess that for the sake of being more generic between all BSDs I could remove that check... yeah actually it'd make more sense. > Overall, it's a great patch. The only thing I am not sure about is if we really > need the TRACKER_MONITOR_KQUEUE work in the first place. It adds a small > maintenance overhead especially if we end up with this API on other unix > systems at some point too. I doubt Linux will ever have this. So far, all BSD implement it. I guess I should add Mac OS to the list as well actually. > I actually don't know if that's a possibility either (it being available on > other unix systems). Thoughts? Let's keep TRACKER_MONITOR_KQUEUE with the updated patch according to your comments for now. We can always revisit this later I'd say. I'll post the updated patch asap, I'm not in a position do to it right now. Cheers!
(In reply to comment #2) > (In reply to comment #1) > > 1. Can we remove this? > > > > +/* getrlimit(2) */ > > Certainly. Thanks. > > 3. Why do we initially set limit to 400 why not another number? I ask because > > in terms of directories to monitor, 400 is a really low number. 8k seems to be > > used in a lot of places on Ubuntu and Fedora I think for inotify. People even > > then still complain they run out of inotify watches. I am just worried that 400 > > is too low if it ends up being used. > > Because of this: > $ ulimit -n > 512 I see, on my Ubuntu box (12.10) I have: $ ulimit -n 1024 > These are the default limits on OpenBSD... My main concern here is coding in arbitrary limits which are set by the OS not the technology itself. > I think that if the call fails, then the fallback limit should be within a > range that would work anywhere, whatever default ulimit is. > 400 comes from the Fam/Gamin monitor thing below. Yea, that's rather old and been there for some time. So I am not sure how useful it is to base numbers on those. Let's keep things as they are, if we get complaints we can always fix up the patch :) > > 5. Why limit this? You have a comment there about preventing too many FDs being > > open, but shouldn't that be limited anyway by ulimit? Setting an arbitrary > > number here might be counter productive. Regarding my earlier comment of 8k > > with inotify, I wonder if this would be too low anyway? > > > > + if (limit > 2048) > > The issue really is that if you start using an insane amount of FDs, then the > machine will start crawling. Also the total max open files on OpenBSD is > "7030". > So if for e.g. you setup your user to have a max nofiles of 5120, kqueue will > eat most of the system-wide available openfiles. > I guess that for the sake of being more generic between all BSDs I could remove > that check... yeah actually it'd make more sense. Indeed. It's good to have limit checks and we do this with inotify because otherwise smaller apps who only use the API for simple file changes in one or two places would not be able to operate. So I think it's fine to have, I just would rather not use a number which is so easily changeable in the system in the code we build. > > Overall, it's a great patch. The only thing I am not sure about is if we really > > need the TRACKER_MONITOR_KQUEUE work in the first place. It adds a small > > maintenance overhead especially if we end up with this API on other unix > > systems at some point too. > > I doubt Linux will ever have this. So far, all BSD implement it. I guess I > should add Mac OS to the list as well actually. OK, thanks for the info. > > I actually don't know if that's a possibility either (it being available on > > other unix systems). Thoughts? > > Let's keep TRACKER_MONITOR_KQUEUE with the updated patch according to your > comments for now. We can always revisit this later I'd say. > > I'll post the updated patch asap, I'm not in a position do to it right now. > Cheers! Great, commit when ready ;) thanks again.
> Great, commit when ready ;) thanks again. Hi Martin. Before pushing the patch, I'd like you to have one last look as I made several modifications to it and it should now be way simpler. What do you think?
Created attachment 229077 [details] [review] tracker-monitor: add support for GKqueueDirectoryMonitor v2
Comment on attachment 229077 [details] [review] tracker-monitor: add support for GKqueueDirectoryMonitor v2 Patch looks fine, thanks for the updated version, please go ahead and commit.
It's in Martyn: 0fbb71a, thanks. (and sorry for mistyping your name in my previous answer...).