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

[Jan Kara <jack@suse.cz>]

On Tue 31-10-17 13:02:21, Amir Goldstein wrote:
> On Tue, Oct 31, 2017 at 11:54 AM, Jan Kara <jack@suse.cz> wrote:
> > On Mon 30-10-17 21:18:09, Miklos Szeredi wrote:
> >> On Mon, Oct 30, 2017 at 6:27 PM, Jan Kara <jack@suse.cz> 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@redhat.com> 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@gmail.com>
> 
> 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.
>                   */

That's a good point. Actually I don't think any comment is needed after the
cleanup as the code is self-explaining then. I'll just delete the comment
as a part of the cleanup patch.

I've pulled Miklos' patches to my tree and will push them to Linus during
the merge window.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR