From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-yw0-f196.google.com ([209.85.161.196]:37395 "EHLO mail-yw0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S935183AbeFMN5A (ORCPT ); Wed, 13 Jun 2018 09:57:00 -0400 Received: by mail-yw0-f196.google.com with SMTP id j134-v6so903076ywg.4 for ; Wed, 13 Jun 2018 06:57:00 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20180613132107.pto3j7ujt2wacgii@quack2.suse.cz> References: <20180611133628.e35npuuv425n2425@quack2.suse.cz> <20180611160311.dsehobeuvwiq2j6c@quack2.suse.cz> <20180613132107.pto3j7ujt2wacgii@quack2.suse.cz> From: Amir Goldstein Date: Wed, 13 Jun 2018 16:56:59 +0300 Message-ID: Subject: Re: [GIT PULL] Fsnotify cleanups To: Jan Kara Cc: linux-fsdevel , Linus Torvalds Content-Type: text/plain; charset="UTF-8" Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Wed, Jun 13, 2018 at 4:21 PM, Jan Kara wrote: > On Mon 11-06-18 22:51:13, Amir Goldstein wrote: >> On Mon, Jun 11, 2018 at 7:38 PM, Amir Goldstein wrote: >> > On Mon, Jun 11, 2018 at 7:03 PM, Jan Kara wrote: >> >> On Mon 11-06-18 16:58:14, Amir Goldstein wrote: >> >>> On Mon, Jun 11, 2018 at 4:36 PM, Jan Kara wrote: >> >>> > > 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... >> >>> > >> >>> >> >>> By moving the checks for object type into the helper? >> >> >> >> Yes, that's what I meant. >> >> >> >> Force pushed cleaner cleanup with helper to same branch. >> >> Naturally, that one also passes the LTP tests. > > Going through patches: > > Regarding "fsnotify: use abstract fsnotify_obj_t * instead of **connp > argument" - I agree "struct fsnotify_mark_connector __rcu *" is quite > verbose but given how things evolved I don't think "fsnotify_obj_t" is a > great name. How about "fsnotify_connp_t" and keep parameter names as > "connp" instead of renaming them to "obj"? Because abstraction (like > pretending this is some kind of object when it is actually just a pointer) > that does not actually abstract anything is just obfuscation... So let's be > direct and admit this is just a shortcut name for connector pointer. > I though you'd say that and I agree. will rework after pull request. > "fsnotify: pass object and object type to fsnotify_add_mark()" looks good, > just please wrap lines with fanotify_add_new_mark() that became longer than > 80 chars. > > "fsnotify: add helper to get mask from connector" - why do you modify > inotify_update_existing_watch()? I don't see much benefit in changes there > (but I may be missing some consistency you are trying to pursue). > It was preparation for removal of i_fsnotyf_mask. We don't need that if we don't want to pursue that effort. Thanks, Amir.