Re: [PATCH v2 0/7] fix fanotify issues with the series in v4.12

[Amir Goldstein]

On Tue, Oct 31, 2017 at 11:54 AM, Jan Kara <jack@xxxxxxx> wrote:
> On Mon 30-10-17 21:18:09, Miklos Szeredi wrote:
>> On Mon, Oct 30, 2017 at 6:27 PM, Jan Kara <jack@xxxxxxx> wrote:
>> > On Fri 27-10-17 13:53:20, Jan Kara wrote:
>> >> On Wed 25-10-17 16:31:39, Miklos Szeredi wrote:
>> >> > On Wed, Oct 25, 2017 at 10:41 AM, Miklos Szeredi <mszeredi@xxxxxxxxxx> wrote:
>> >> > > We discovered some problems in the latest fsnotify/fanotify codebase with
>> >> > > the help of a stress test (Xiong Zhou is working on upstreaming it to
>> >> > > fstests).
>> >> > >
>> >> > > This series attempts to fix these.  With the patch applied the stress test
>> >> > > passes.
>> >> > >
>> >> > > Please review/test.
>> >> > >
>> >> > > Changes in v2 (only cosmetic fixes, no functional change):
>> >> > >  - split first patch into 3 parts to make it more readable
>> >> > >  - checkpatch fixes
>> >> > >  - added cleanup patch for fanotify
>> >> >
>> >> > Pushed v3 (again with just cosmetics) to:
>> >> >
>> >> > git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git fsnotify-fixes-v3
>> >>
>> >> Sorry, I was at OSS Europe this week so I didn't find time to have a close
>> >> look. I'll try to check it early next week and pick it up to my tree.
>> >> Also thanks Amir for reviewing Miklos' patches!
>> >
>> > So I went through all patches and commented on those where I had some
>> > questions / suggestions. Once those are addressed, I can pick this up to my
>> > tree.
>>
>> Pushed v5 to:
>>
>>   git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git
>> fsnotify-fixes-v5
>>
>> Changes from v4:
>> - address comments for fanotify cleanup patch
>
> Thanks! I went through all the patches and they look fine to me. Amir, will
> you have time to check the final version of patches so that I can add your
> Reviewed-by tag?

You can add for the v5 series:
Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx>

My only concern is regarding the comment that you requested
("Need to protect both marks against freeing...")
from "pin both inode and vfsmount mark" patch.

IMO its a bit hard for a bypassing reader to understand why the
comment is there and what exactly it refers to.
Then the "cleanup fsnotify()" patch leaves this comment dangling in
a place that makes this even worse.

I suggest to move the comment into the comment above mark iteration
loop and mention iter_info explicitly, something like this:

                  * That's why this traversal is so complicated...
+                * Need to protect both marks against freeing so that we can
+                * continue iteration from this place, so pass both marks on
+                * iter_info to send_to_group(), regardless of which mark
+                * we actually happen to send an event for.
                  */

Amir.