On Mon, Feb 19, 2018 at 3:50 PM, Jan Kara <jack@xxxxxxx> wrote: [...] > For fanotify without FAN_UNLIMITED_QUEUE the situation is similar as for > inotify - IMO low practical impact, apps should generally handle queue > overflow so I don't see a need for any opt in (more accurate memcg charging > takes precedense over possibly broken apps). > > For fanotify with FAN_UNLIMITED_QUEUE the situation is somewhat different - > firstly there is a practical impact (memory consumption is not limited by > anything else) and secondly there are higher chances of the application > breaking (no queue overflow expected) and also that this breakage won't be > completely harmless (e.g., the application participates in securing the > system). I've been thinking about this "conflict of interests" for some > time and currently I think that the best handling of this is that by > default events for FAN_UNLIMITED_QUEUE groups will get allocated with > GFP_NOFAIL - such groups can be created only by global CAP_SYS_ADMIN anyway > so it is reasonably safe against misuse (and since the allocations are > small it is in fact equivalent to current status quo, just more explicit). > That way application won't see unexpected queue overflow. The process > generating event may be looping in the allocator but that is the case > currently as well. Also the memcg with the consumer of events will have > higher chances of triggering oom-kill if events consume too much memory but > I don't see how this is not a good thing by default - and if such reaction > is not desirable, there's memcg's oom_control to tune the OOM behavior > which has capabilities far beyond of what we could invent for fanotify... > > What do you think Amir? > If I followed all your reasoning correctly, you propose to change behavior to always account events to group memcg and never fail event allocation, without any change of API and without opting-in for new behavior? I think it makes sense. I can't point at any expected breakage, so overall, this would be a good change. I just feel sorry about passing an opportunity to improve functionality. The fact that fanotify does not have a way for defining the events queue size is a deficiency IMO, one which I had to work around in the past. I find that assigning group to memgc and configure memcg to desired memory limit and getting Q_OVERFLOW on failure to allocate event is going to be a proper way of addressing this deficiency. But if you don't think we should bind these 2 things together, I'll let Shakeel decide if he want to pursue the Q_OVERFLOW change or not. Thanks, Amir.
