GNOME Bugzilla – Bug 779777
patch: new kqueue() backend for file monitoring
Last modified: 2018-03-13 12:39:58 UTC
Created attachment 347513 [details] [review] new kqueue() file monitoring backend kqueue() file monitoring backend Original kqueue() file monitor uses polling to monitor files in /mnt and all path that can be unmounted. On my system thunar eat 100% CPU after short time browsing sshfs, smbfs shares. But on Apple MAC (where defined O_EVTONLY) kqueue() always used. My code based on FAM backend ( https://github.com/GNOME/glib/blob/master/gio/fam/gfamfilemonitor.c ): - no additional threads created - no hash tables used - lock() only in few placed: on init and on kqueue() events process (may be this can be removed, I m not sure how glib process read events) - small and simple code: ~500 lines, that easy to understand and debug without glib - add/remove file/dir monitoring is async. If you have some sshfs/smb mount point and some one (not from your host) change files there - no notifications will be received: kqueue() does not handle it. Original code use polling. Im not sure does inotify, fam, kqueue() on apple mac and others handle it. I can add polling by timer but it may take many CPU resources, like glib based polling in original kqueue backend.
Created attachment 349427 [details] [review] readdir_r() -> getdirentries() After some system/ports updates during this months apps start crash on standard open dialog. I fail to reproduce this in demo app, so I decide replace readdir_r() to getdirentries() and this helps.
Even if the current kqueue backend for GIO is not optimal, your version is not portable at all and re-implement some libc internals using some of FreeBSD specific code.
I dont see any portable issues and no one report about it. If some one report about issues on other BSD systems I will try to fix it. But, I m use FreeBSD and have no time to check this on all other BSD systems. getdirentries() is not FreeBSD specific. readdir_r() -> getdirentries() has fix some strange crash inside readdir_r() and save a little CPU resources (no sort, no additional mem allocations, etc...). I can add some small C program to debug my kqueue code: kqueue_fnm.h and kqueue_fnm.c can work in any program, without glib. I done this patch for myself because current implementation was very annoying me with hight CPU usage and Thunar crashes last year. And some time before, I live half year on my desktop without any fam because this ugly code was broken and disabled. Two or more year this code annoying me and no one fix it. So, please stop telling me about current fam kqueue backend - this is piece of shit and I dont want see it more on all my systems. It cant be fixed, it must be replaced at all. But you may continue spent time to fix bugs in current implementation, its your time.
Your code do not even compile on !FreeBSD system, how is that portable? getdirentries() is a deprecated interface. redefining reallocarray(3) here is wrong. DT_WHT is a FreeBSD internal. Your code does not respect the coding style of glib. If you can get your code work on NetBSD and OpenBSD and clean it, it might become a better alternative that what currently exist. I agree that the current code is not good in many way. But your diff still need some love. Cheers.
Hello. > I done this patch for myself because current implementation was very > annoying me with hight CPU usage and Thunar crashes last year. That is true. The current implementation is not optimal. > And some time before, I live half year on my desktop without any fam because > this ugly code was broken and disabled. > Two or more year this code annoying me and no one fix it. > > So, please stop telling me about current fam kqueue backend - this is piece I'm only jumping in because I'm the OpenBSD GLib maintainer and we've experienced a long standing gio-kqueue bug which FreeBSD also had. I will not compare the different implementations because I am not competent enough but I feel like I have to answer because your tone is very rude and selfish. Today Martin just fixed a very long standing but in this area. I haven't seen the FAM backend mentioned anywhere so I am very surprised you are putting it here. I am not sure about FreeBSD but on OpenBSD we stopped using the FAM backend more than 5 years ago. > of shit and I dont want see it more on all my systems. > It cant be fixed, it must be replaced at all. I don't disagree. But putting code out there that is not better than the original is not an improvement. It's just different. It's awesome that you want to contribute but I would advise you to read the GNOME Code of Conduct: https://wiki.gnome.org/action/show/Foundation/CodeOfConduct > But you may continue spent time to fix bugs in current implementation, its > your time. Yes it is. We (FreeBSD and OpenBSD) have been working with GNOME developers for many years and we all spent a lot of time fixing bugs for the benefit of many. Your patronizing does not encourage this. That said, thank you for trying to improve things.
(In reply to Martin Pieuchot from comment #4) > Your code do not even compile on !FreeBSD system, how is that portable? Can you show error messages? Did you try https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=214338 ? > getdirentries() is a deprecated interface. Only on Mac OS X 10.6. > redefining reallocarray(3) here is wrong. Not redefining. This done for FreeBSD 10.x compatible. This should be adopted for other BSD systems. > DT_WHT is a FreeBSD internal. #ifndef DT_WHT #define DT_WHT DT_UNKNOWN #endif should fix it, or may be more proper if ( #ifdef DT_WHT DT_WHT == de_cur->d_type || #endif IS_NAME_DOTS(de_cur->d_name)) goto retry; > Your code does not respect the coding style of glib. Yes, it is true. > If you can get your code work on NetBSD and OpenBSD and clean it, it might > become a better alternative that what currently exist. If some one who use NetBSD and OpenBSD join and help with test then I will do this. IMHO code need some cosmetic changes to build and work on other BSD systems. > I agree that the current code is not good in many way. But your diff still > need some love. Yes, it is not ideal. This patch a bit old, latest version in FreeBSD bug tracker.
(In reply to rozhuk.im from comment #6) > (In reply to Martin Pieuchot from comment #4) > > Your code do not even compile on !FreeBSD system, how is that portable? > > Can you show error messages? Can't you try yourself? > Did you try https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=214338 ? Are you kidding? This is a FreeBSD port issue which have nothing to do with any other OS or GNOME at all. > > getdirentries() is a deprecated interface. > > Only on Mac OS X 10.6. And only NetBSD. And the function no longer exist on OpenBSD. Plus I don't understand why you don't use getdents(2) instead. > > redefining reallocarray(3) here is wrong. > > Not redefining. > This done for FreeBSD 10.x compatible. > This should be adopted for other BSD systems. This has nothing to do inside a GIO kqueue backend. > > DT_WHT is a FreeBSD internal. > > #ifndef DT_WHT > #define DT_WHT DT_UNKNOWN > #endif > > should fix it, or may be more proper > if ( > #ifdef DT_WHT > DT_WHT == de_cur->d_type || > #endif > IS_NAME_DOTS(de_cur->d_name)) > goto retry; This is incorrect. > > If you can get your code work on NetBSD and OpenBSD and clean it, it might > > become a better alternative that what currently exist. > > If some one who use NetBSD and OpenBSD join and help with test then I will > do this. > IMHO code need some cosmetic changes to build and work on other BSD systems. Trust me, it's more than that. I only looked at the top of the iceberg, but there are more issues. > > I agree that the current code is not good in many way. But your diff still > > need some love. > > Yes, it is not ideal. > This patch a bit old, latest version in FreeBSD bug tracker. Sorry but this is a GNOME bug tracker and the kqueue backend is not a FreeBSD only code. That said, I'd be happy if you'd take the time to improve your backend to make it portable and work on other BSD variants.
(In reply to Antoine Jacoutot from comment #5) > Hello. Hello, Antoine Jacoutot. > ...I feel like I have to answer because your tone is very rude and selfish. > Today Martin just fixed a very long standing but in this area. I haven't > seen the FAM backend mentioned anywhere so I am very surprised you are > putting it here. I am not sure about FreeBSD but on OpenBSD we stopped using > the FAM backend more than 5 years ago. Sorry for the tone, but this code daily did not make me comfortable for at least two years, until my patience was over and I did not rewrite it. FAM may be wrong name, GFileMonitor / gio-kqueue. It used by Thunar, some xfce4 staff, PcManFM... I see fix, but you wait it too much time, like all other fixes for this backend. > > of shit and I dont want see it more on all my systems. > > It cant be fixed, it must be replaced at all. > > I don't disagree. But putting code out there that is not better than the > original is not an improvement. It's just different. It's awesome that you > want to contribute but I would advise you to read the GNOME Code of Conduct: > https://wiki.gnome.org/action/show/Foundation/CodeOfConduct I do not have a bad attitude here to anyone, only to the code about which I spoke earlier. I do not insist that my code is better in everything, but it solved my problems and I'm happy. Thunar still periodically crashes, but does not load the CPU and crashes is not directly related to gio-kqueue. PcManFM more stable and not load CPU any more. > > But you may continue spent time to fix bugs in current implementation, its > > your time. > > Yes it is. We (FreeBSD and OpenBSD) have been working with GNOME developers > for many years and we all spent a lot of time fixing bugs for the benefit of > many. Your patronizing does not encourage this. That said, thank you for > trying to improve things. I offer my code and my help with it. Accept or not - decide for yourself, in any case I will continue to use my code and update the patch at least in the FreeBSD bug tracker.
(In reply to Martin Pieuchot from comment #7) > Can't you try yourself? Ok, I will try. > > Did you try https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=214338 ? > > Are you kidding? This is a FreeBSD port issue which have nothing to do > with any other OS or GNOME at all. This is almost same patch, but port specific. > > > getdirentries() is a deprecated interface. > > > > Only on Mac OS X 10.6. > > And only NetBSD. And the function no longer exist on OpenBSD. Plus I don't > understand why you don't use getdents(2) instead. I take this from FreeBSD libc. Ok, getdents(2) will be in next patch. > > > redefining reallocarray(3) here is wrong. > > > > Not redefining. > > This done for FreeBSD 10.x compatible. > > This should be adopted for other BSD systems. > > This has nothing to do inside a GIO kqueue backend. And where I should get reallocarray(3)? Or I should use something from GLIB world? > > > DT_WHT is a FreeBSD internal. > > > > #ifndef DT_WHT > > #define DT_WHT DT_UNKNOWN > > #endif > > > > should fix it, or may be more proper > > if ( > > #ifdef DT_WHT > > DT_WHT == de_cur->d_type || > > #endif > > IS_NAME_DOTS(de_cur->d_name)) > > goto retry; > > This is incorrect. Very informative remark. > > If some one who use NetBSD and OpenBSD join and help with test then I will > > do this. > > IMHO code need some cosmetic changes to build and work on other BSD systems. > > Trust me, it's more than that. I only looked at the top of the iceberg, but > there are more issues. Should this scare or stop me?) > > This patch a bit old, latest version in FreeBSD bug tracker. > > Sorry but this is a GNOME bug tracker and the kqueue backend is not a FreeBSD > only code. That said, I'd be happy if you'd take the time to improve your > backend to make it portable and work on other BSD variants. Ok but I need feedback, because I do not have so mush time to setup all BSD OS`es, build env and test it.
(In reply to rozhuk.im from comment #9) > (In reply to Martin Pieuchot from comment #7) > [...] > > > > redefining reallocarray(3) here is wrong. > > > > > > Not redefining. > > > This done for FreeBSD 10.x compatible. > > > This should be adopted for other BSD systems. > > > > This has nothing to do inside a GIO kqueue backend. > > And where I should get reallocarray(3)? > Or I should use something from GLIB world? I don't know, look at which allocators are used inside the glib and pick the appropriate one. This is not the place to redefine functions found in standard libraries. If you do so, programs linking with the glib might have symbol problems. It's also not the place to redefine O_NOATIME or O_EVTONLY. If you need to do that, then your code is not portable. Plus I doubt that you need to pass all these flags to your open(2) calls. > > > If some one who use NetBSD and OpenBSD join and help with test then I will > > > do this. > > > IMHO code need some cosmetic changes to build and work on other BSD systems. > > > > Trust me, it's more than that. I only looked at the top of the iceberg, but > > there are more issues. > > Should this scare or stop me?) I should encourage you to continue. The actual solution is not good. Your code has a lot of potential but it has some portability problems. If you can clean it up and make it work on OpenBSD and/or NetBSD, I'll review it.
reallocarray(..., a, b) is good way to replace realloc(..., (a * b)). reallocarray() was defined only for old FreeBSD witch not have it. I prefer to avoid using GLIB staff to keep code work without glib at least for debug purposes. #ifndef O_NOATIME # define O_NOATIME 0 #endif #ifndef O_EVTONLY # define O_EVTONLY O_RDONLY #endif This is not redefining. This is module/local defining of undefined staff. This cant affect any code outside file where it placed.
Created attachment 351399 [details] [review] Latest changes This should build with current glib.
Created attachment 364369 [details] [review] update patch
Created attachment 364370 [details] tool to test patch tool to test patch Tool to test code. Place in folder with latest kqueue_fnm.h and kqueue_fnm.c, edit const char *paths build: cc gkqueuefilemonitor.c kqueue_fnm.c -O0 -DDEBUG -lpthread -o gkqueuefilemonitor run: ./gkqueuefilemonitor
Created attachment 368463 [details] tool to test
Created attachment 368464 [details] [review] patch Add: - adaptive rate limit - dir objects count limit - tunable to disable monitor local dir subfiles changes - tunable to disable monitor local dir subdirs changes - tunable to disable monitor for subfiles and subdirs on non local filesystems Fix: - descriptors leak on procfs, may cause use mem after free - add workaround for procfs and tmpfs, where st_ino may != d_fileno Also done some test with valgring and cppcheck.
Bug #739424 just landed a rewrite of the kqueue backend, so this one is obsolete. At the moment, I would prefer any more improvements to the kqueue backend to be incremental. If you have any patches to make small, incremental, self-contained changes like that, please attach them to new bug reports. Thanks.