Re: [GIT PULL] Fsnotify cleanups

[Amir Goldstein <amir73il@gmail.com>]

On Wed, Jun 13, 2018 at 4:21 PM, Jan Kara <jack@suse.cz> wrote:
> On Mon 11-06-18 22:51:13, Amir Goldstein wrote:
>> On Mon, Jun 11, 2018 at 7:38 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> > On Mon, Jun 11, 2018 at 7:03 PM, Jan Kara <jack@suse.cz> wrote:
>> >> On Mon 11-06-18 16:58:14, Amir Goldstein wrote:
>> >>> On Mon, Jun 11, 2018 at 4:36 PM, Jan Kara <jack@suse.cz> 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.