From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:54512 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751886AbeFKNga (ORCPT ); Mon, 11 Jun 2018 09:36:30 -0400 Date: Mon, 11 Jun 2018 15:36:28 +0200 From: Jan Kara To: Amir Goldstein Cc: Jan Kara , linux-fsdevel , Linus Torvalds Subject: Re: [GIT PULL] Fsnotify cleanups Message-ID: <20180611133628.e35npuuv425n2425@quack2.suse.cz> References: <20180607150217.jq757ncqopuimbkd@quack2.suse.cz> <20180608132737.etbkpqg77yz3vhp7@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: On Sun 10-06-18 20:49:16, Amir Goldstein wrote: > On Sat, Jun 9, 2018 at 9:46 PM, Amir Goldstein wrote: > > On Sat, Jun 9, 2018 at 8:30 PM, Linus Torvalds > > wrote: > >> On Fri, Jun 8, 2018 at 11:57 PM Amir Goldstein wrote: > >>> > >>> We embed fsnotify_obj in struct inode and fsnotify_obj_mask in struct mount. > >> > >> So I'd *really* like to see just a pointer, not an embedded struct. > >> > >> Yes, if you get rid of the mask from the embedded struct (so that it > >> only contains a pointer), you do get rid of the odd alignment issues > >> and the need for "packed". > >> > >> But from previous experience, once you embed a structure, that > >> structure tends to grow. Because it can, and it's so convenient. > >> Suddently it has a spinlock in it too etc. > >> > > > > Fair enough. > > > >> So if you can make do with just the pointer, it would actually be > >> nicer to expose it as such. Then you can also avoid the header file > >> dependency chain, because you can just pre-declare the structure (like > >> it does now) and > >> > >> struct fsnotify_mark_connector; > >> .. > >> struct fsnotify_mark_connector __rcu *i_fsnotify_marks; > >> > >> in the inode. That way the core header files don't need to worry about > >> the fsnotify details, and don't need to include fsnotify headers. > >> > >> And we can do inode packing without having to know (not that it > >> happens all that often - everybody would *love* to shrink the inode > >> structure, but it's just hard. Because everybody also wants to put > >> their own data into the inode ..) > >> > >> Can't the generalization code just take a pointer to a __rcu pointer > >> to fsnotify_mark_connector, obviating the need for the fsnotify_obj > >> structure definition? > >> > > > > Jan, > > I reworked the cleanup patches to get rid of fsnotify_obj and pushed to: > https://github.com/amir73il/linux.git fsnotify-cleanup Thanks! > Only last 5 patches from fsnotify_for_v4.18-rc1 have been modified > and I removed your S-O-B from the modified patches. > > This leaves struct inode unchanged, in fact no changes to code outside > fsnotify/audit at all. > > mask is now a member of connector for the purpose of generalizing > add/remove mark, but struct inode/mount still have a copy of the mask > for the purpose of the VFS optimizations. Looking through those patches, is it really beneficial to add mask to connector when you keep it in inode / vfsmount? A helper function to get mask from connector would make the same refactoring possible as well, won't it? And adding a helper function to set mask given connector would get rid of the remaining checks for connector type due to mask manipulations... Honza -- Jan Kara SUSE Labs, CR