On Wed, Jan 4, 2017 at 10:28 AM, Jan Kara <jack@xxxxxxx> wrote:
> Hi,
>
> On Tue 27-12-16 21:32:24, Amir Goldstein wrote:
>> I thought this would turn out simpler, so you may be able to use it
>> for your work, but I'm afraid that's not the case.
>>
>> Anyway, since I am leaving for new year's vacation, I am posting
>> what I have in case you want to use any of it.
>>
>> It passed some initial tests I ran, but when I wanted to test the
>> corner case referred to in patch 1, I found that my test program
>> hangs open() syscalls with kernel 4.10-rc1 before any of my changes.
>>
>> This is the mark setup I was testing [1]:
>> fanotify_mark(fd, FAN_MARK_ADD,
>> FAN_OPEN_PERM | FAN_EVENT_ON_CHILD, AT_FDCWD,
>> path);
>> fanotify_mark(fd, FAN_MARK_ADD | \
>> FAN_MARK_IGNORED_SURV_MODIFY | FAN_MARK_IGNORED_MASK
>> FAN_OPEN_PERM | FAN_EVENT_ON_CHILD, AT_FDCWD,
>> FAN_CLOSE_WRITE, AT_FDCWD,
>> path);
>> fanotify_mark(fd, FAN_MARK_ADD | FAN_MARK_MOUNT,
>> FAN_OPEN_PERM | FAN_CLOSE_WRITE, AT_FDCWD,
>> path);
>>
>> Without FAN_EVENT_ON_CHILD it works fine, but with FAN_EVENT_ON_CHILD,
>> something bad is going on and I did not have time to look into it.
>
> I had a look at the patches and the result does not look simpler than what
> we had before AFAICT. Sure we don't have to pass both marks into
> ->handle_event but is that really such a big win? And actually my patches
> for dropping SRCU lock when waiting for userspace response to fanotify
> permission event need both marks in ->handle_event because they both need
> to be protected against freeing when SRCU lock is dropped... So I don't
> think this is really viable path.
>
Sure.
> However one thing that may be worth cleaning up is that
> fanotify_should_send_event() needlessly checks the masks - send to group
> already did this. So I'd move the check for FS_EVENT_ON_CHILD from
> fanotify_should_send_event() to send_to_group() - arguably it belongs there
> - and then just completely drop checking of the masks from
> fanotify_should_send_event(). What do you think?
>
Right, so patch [1/4] plus deduplicating the tests in
fanotify_should_send_event().
In principle it makes sense. However, you probably noticed that the logic used
by fanotify_should_send_event() for FS_EVENT_ON_CHILD is different from
the generic logic in send_to_group().
The test in fanotify_should_send_event() is skipped if both inode and vfsmount
marks are present. My patch [1/4] changes this logic, because I thought it was
a bug, but my tests indicate that a bug related to FS_EVENT_ON_CHILD exists
before AND after my change.
So first, I need to isolate and analyse the bug. When I propose a fix, I will
make sure the FS_EVENT_ON_CHILD test ends up only in send_to_group().
>> In general, I would like to start working on an fsnotify testsuite,
>> so if you have any plans wrt writing extra tests or ideas about specific
>> missing tests, please let me know about them.
>
> That would be certainly worthwhile. Actually when I find some useful
> testcase I add it to LTP under the
> testcases/kernel/syscalls/{fanotify|inotify}. So please extend that if you
> have some more ideas for testcases.
>
Yes, I am aware of those testcases.
I find LTP quite heavy to build, so I though I would spin a dedicated
testsuite that will contain your testcases, but will also include
infrastructure for stress testing and profiling.
Keeping track of performance regressions is clearly a major aspect
of maintaining fsnotify.
Amir.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
