Re: [PATCH v2] fs: fsnotify: account fsnotify metadata to kmemcg

On Wed, Feb 14, 2018 at 12:20 AM, Shakeel Butt [off-list ref] wrote:

On Tue, Feb 13, 2018 at 1:54 PM, Amir Goldstein [off-list ref] wrote:

quoted

On Tue, Feb 13, 2018 at 11:10 PM, Shakeel Butt [off-list ref] wrote:

[...]

quoted

quoted

quoted

The question is, do we need the user to also explicitly opt-in for
Q_OVERFLOW on ENOMEM with FAN_Q_ERR mark mask?
Should these 2 new APIs be coupled or independent?

Are there any error which are not related to queue overflows? I see
the mention of ENODEV and EOVERFLOW in the discussion. If there are
such errors and might be interesting to the listener then we should
have 2 independent APIs.

These are indeed 2 different use cases.
A Q_OVERFLOW event is only expected one of ENOMEM or
EOVERFLOW in event->fd, but other events (like open of special device
file) can have ENODEV in event->fd.

But I am not convinced that those require 2 independent APIs.
Specifying FAN_Q_ERR means that the user expects to reads errors
from event->fd.

Can you please explain what you mean by 2 independent APIs? I thought
"no independent APIs" means FAN_Q_ERR can only be used with
FAN_Q_OVERFLOW and without FAN_Q_OVERFLOW, FAN_Q_ERR is ignored. Is
that right or I misunderstood?

What I initially meant to say was, we actually consider several
behavior changes:
1. Charge event allocations to memcg of listener
2. Queue a Q_OVERFLOW event on ENOMEM of event allocation
3. Report the error to user on metadata->fd (instead of FAN_NOFD)
4. Allow non Q_OVERFLOW event to have negative metadata->fd.

#3 is applicable both to Q_OVERFLOW event and other events that
can't provide and open file descriptor for some reason (i.e. ENODEV).

#1 and #2 could be independent, but they both make sense together.
When enabling #1 user increases the chance of ENOMEM and therefore
#2 is desired. So if we are going to let distro/admin/programmer to
opt-in for what we believe to be a "change of behavior for the best", then
we could consider that opting-in  for #1 will also imply opting-in for #2
and #3 (as the means to report Q_OVERFLOW due to ENOMEM).

I guess we will need to allow user to opt-in to #4 and #3 by FAN_Q_ERR
mask flag to cover the ENODEV case independently from opting-in to
charging memcg.

How was I doing in the balance of adding clarity vs. adding confusion?

Thanks,
Amir.