From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753648AbdJaQ2I (ORCPT ); Tue, 31 Oct 2017 12:28:08 -0400 Received: from mx2.suse.de ([195.135.220.15]:42872 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753572AbdJaQ2G (ORCPT ); Tue, 31 Oct 2017 12:28:06 -0400 Date: Tue, 31 Oct 2017 17:27:59 +0100 From: Jan Kara To: Amir Goldstein Cc: Jan Kara , Miklos Szeredi , linux-fsdevel , Xiong Zhou , lkml Subject: Re: [PATCH v2 0/7] fix fanotify issues with the series in v4.12 Message-ID: <20171031162759.GF26128@quack2.suse.cz> References: <1508920899-8115-1-git-send-email-mszeredi@redhat.com> <20171027115320.GJ31161@quack2.suse.cz> <20171030172756.GO23278@quack2.suse.cz> <20171031095417.GC8989@quack2.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue 31-10-17 13:02:21, Amir Goldstein wrote: > On Tue, Oct 31, 2017 at 11:54 AM, Jan Kara wrote: > > On Mon 30-10-17 21:18:09, Miklos Szeredi wrote: > >> On Mon, Oct 30, 2017 at 6:27 PM, Jan Kara 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 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 > > 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 SUSE Labs, CR