From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:50385 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751025AbeFHN1j (ORCPT ); Fri, 8 Jun 2018 09:27:39 -0400 Date: Fri, 8 Jun 2018 15:27:37 +0200 From: Jan Kara To: Linus Torvalds Cc: Jan Kara , linux-fsdevel , Amir Goldstein Subject: Re: [GIT PULL] Fsnotify cleanups Message-ID: <20180608132737.etbkpqg77yz3vhp7@quack2.suse.cz> References: <20180607150217.jq757ncqopuimbkd@quack2.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: Added Amir to CC as the author of those changes. On Thu 07-06-18 09:34:53, Linus Torvalds wrote: > On Thu, Jun 7, 2018 at 8:02 AM Jan Kara wrote: > > > > several fsnotify cleanups unifying handling of different watch > > types. > > Grr. > > Why is this growing things like "fsnotify_obj_inode()" helpers in ? > > It has nothing to do with generic fs code. The only things that can > possibly use that already have to include fsnotify-specific headers, > where things like this belong. Fair point. When I was merging Amir's patches I've actually tried to put this function in fsnotify-specific headers but compilation was failing for some reason I don't remember and so I left it in fs.h. Anyway, this is easy to fix. > It also adds a "struct fsnotify_obj i_fsnotify" to the struct inode, > and marks it "packed", so now architectures that have issues with > alignment might have issues depending on random changes to 'struct > inode'. Yeah, "packed" is ugly but I wanted to avoid growing of struct inode just because of unnecessary padding on 64-bit archs... And I missed the fact that the packed structure is not properly aligned as a whole. Thanks for catching this! The alignment could be fixed by having struct inode { ... struct fsnotify_obj i_fsnotify __attribute__ ((aligned(sizeof(void *)))); ... } but that's too ugly even for my twisted taste. And before someone asks, adding aligned attribute to the definition of fsnotify_obj structure makes it grow the padding at the end again. I'll ponder a bit more about this. I guess I'll try just moving the 'mask' field from struct inode to struct fsnotify_connector. That avoids all the alignment / size issues, makes struct inode smaller, and allows simplification of fsnotify code as well. Just we'll have to benchmark how much the additional dereference to access the mask is visible. > Plus it (again) causes more disturbance to a core header file that > fsnotify shouldn't touch. We had a forward declaration and a pointer. > > So no. I'm not pulling this. I don't think it's a "cleanup". Maybe it > cleans up the fsnotify code, but it uglifies code that is much more > important. Understood. Honza -- Jan Kara SUSE Labs, CR