[GIT PULL] Fsnotify cleanups

[GIT PULL] Fsnotify cleanups

[Jan Kara]

  Hello Linus,

  could you please pull from

git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify_for_v4.18-rc1

to get several fsnotify cleanups unifying handling of different watch
types.

Top of the tree is 3be4aaf4e2d3. The full shortlog is:

Amir Goldstein (12):
      fsnotify: use type id to identify connector object type
      fsnotify: remove redundant arguments to handle_event()
      fsnotify: introduce marks iteration helpers
      fsnotify: generalize iteration of marks by object type
      fsnotify: generalize send_to_group()
      fanotify: generalize fanotify_should_send_event()
      fsnotify: add fsnotify_add_inode_mark() wrappers
      fsnotify: introduce prototype struct fsnotify_obj
      fsnotify: pass fsnotify_obj instead of **connp argument
      fsnotify: pass object and object type to fsnotify_add_mark()
      fsnotify: let connector point to abstract fsnotify_obj
      fanotify: factor out helpers to add/remove mark

The diffstat is

 fs/inode.c                           |   2 +-
 fs/mount.h                           |  11 ++-
 fs/notify/dnotify/dnotify.c          |  16 ++--
 fs/notify/fanotify/fanotify.c        |  46 +++++-----
 fs/notify/fanotify/fanotify_user.c   |  98 ++++++++-------------
 fs/notify/fdinfo.c                   |  12 +--
 fs/notify/fsnotify.c                 | 160 ++++++++++++++++++++---------------
 fs/notify/fsnotify.h                 |  14 +--
 fs/notify/group.c                    |   2 +-
 fs/notify/inotify/inotify.h          |   2 -
 fs/notify/inotify/inotify_fsnotify.c |   6 +-
 fs/notify/inotify/inotify_user.c     |  16 ++--
 fs/notify/mark.c                     | 140 ++++++++++++++----------------
 include/linux/fs.h                   |  20 +++--
 include/linux/fsnotify_backend.h     | 109 +++++++++++++++++++-----
 include/linux/fsnotify_obj.h         |  18 ++++
 kernel/audit_fsnotify.c              |   5 +-
 kernel/audit_tree.c                  |  22 +++--
 kernel/audit_watch.c                 |   7 +-
 kernel/auditsc.c                     |   4 +-
 20 files changed, 396 insertions(+), 314 deletions(-)
 create mode 100644 include/linux/fsnotify_obj.h

							Thanks
								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [GIT PULL] Fsnotify cleanups

[Linus Torvalds]

On Thu, Jun 7, 2018 at 8:02 AM Jan Kara <jack@suse.cz> wrote:
>
> several fsnotify cleanups unifying handling of different watch
> types.

Grr.

Why is this growing things like "fsnotify_obj_inode()" helpers in <linux/fs.h>?

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.

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'.

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.

                Linus

Re: [GIT PULL] Fsnotify cleanups

[Jan Kara]

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 <jack@suse.cz> wrote:
> >
> > several fsnotify cleanups unifying handling of different watch
> > types.
> 
> Grr.
> 
> Why is this growing things like "fsnotify_obj_inode()" helpers in <linux/fs.h>?
> 
> 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 <jack@suse.com>
SUSE Labs, CR

Re: [GIT PULL] Fsnotify cleanups

[Linus Torvalds]

On Fri, Jun 8, 2018 at 6:27 AM Jan Kara <jack@suse.cz> wrote:
>
> 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.

Yeah, I don't think there's any way to say "this can't be an array
member, don't pad the size out to the alignment".

I do wonder if perhaps "struct fsnotify_obj" could actually have a
mask and a 32-bit object ID instead, which would (a) avoid the
alignment issue and (b) actually shrink the structure onm 64-bit.

Obviously you'd then have to look up the pointer from the object ID
(presumably using a hash, but maybe it would use the radix tree of
idr_alloc() or something).

I haven't looked at how often those pointers actually get looked up.
If the 'mask' makes it fairly rare, then the extra indirection might
be ok. I suspect it's not. But I thought I'd mention it as a possible
approach.

The other approach is - as you say - to move 'mask' to be behind the
pointer. But if that causes a lot of extra pointer chasing that mask
would normally avoid, that could be really expensive too. We see that
with the security objects: the cost of looking up whatever is behind
i_security is really nasty, because every inode gets allocated a
private security structure, so it's absolutely horrendous for cache
effects (no sharing).

But I suspect that the behavior of i_fsnotify_marks is very different
from i_security, in that most inodes probably don't have marks at all.
No? Yes? So you might not have the nasty case of "almost all inode
access ends up following that pointer and it's horrible in the cache".

                  Linus

Re: [GIT PULL] Fsnotify cleanups

[Amir Goldstein]

On Fri, Jun 8, 2018 at 11:34 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Jun 8, 2018 at 6:27 AM Jan Kara <jack@suse.cz> wrote:
>>
>> 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.
>
> Yeah, I don't think there's any way to say "this can't be an array
> member, don't pad the size out to the alignment".
>
> I do wonder if perhaps "struct fsnotify_obj" could actually have a
> mask and a 32-bit object ID instead, which would (a) avoid the
> alignment issue and (b) actually shrink the structure onm 64-bit.
>
> Obviously you'd then have to look up the pointer from the object ID
> (presumably using a hash, but maybe it would use the radix tree of
> idr_alloc() or something).
>
> I haven't looked at how often those pointers actually get looked up.
> If the 'mask' makes it fairly rare, then the extra indirection might
> be ok. I suspect it's not. But I thought I'd mention it as a possible
> approach.

As a matter of fact, the pointer is checked to be non zero before
accessing the mask:

7c49b8616460 fs/notify: optimize inotify/fsnotify code for unwatched files

So the mask is not really needed to optimize away unwatched inodes.
I guess the mask is needed to optimize away use cases like frequent
fsnotify_access() calls on inodes that are watched for modification only,
but not sure that is worth optimizing.

>
> The other approach is - as you say - to move 'mask' to be behind the
> pointer. But if that causes a lot of extra pointer chasing that mask
> would normally avoid, that could be really expensive too. We see that
> with the security objects: the cost of looking up whatever is behind
> i_security is really nasty, because every inode gets allocated a
> private security structure, so it's absolutely horrendous for cache
> effects (no sharing).

Besides pointer chasing, there is also srcu_read_lock.

>
> But I suspect that the behavior of i_fsnotify_marks is very different
> from i_security, in that most inodes probably don't have marks at all.
> No? Yes? So you might not have the nasty case of "almost all inode
> access ends up following that pointer and it's horrible in the cache".
>

Yes and No :-)
Yes, most inodes don't have marks at all,
but, every fsnotify() call may need to follow mnt->mnt_fsnotify.marks
if mount is watched and there is no mnt->mnt_fsnotify.mask in fsnotify_obj.

Sure, we don't need to worry about 32bits in struct mount, so we could
have a different attach method for inodes and mounts, but the whole
purpose of this cleanup is to make the code more generic in handing
and attaching of marks to inode/mounts.

How about if we move mask into connector, but also leave a shadow mask,
in an "inherited" struct i.e.:

/* struct to embed in objects, which marks can be attached to */
struct fsnotify_obj {
       struct fsnotify_mark_connector __rcu *marks;
};

/* struct to embed in objects, which marks can be attached to with
shadow mask */
struct fsnotify_obj_mask {
       struct fsnotify_obj obj;
       /* all events this object cares about */
       __u32 mask;
};

We embed fsnotify_obj in struct inode and fsnotify_obj_mask in struct mount.

The only code that needs to test the shadow mask before following into
connector is the optimization code in start of fsnotify() which was not
generalized anyway and where the mask optimization is more important
for mount watches.

The only code that sets the mask is __fsnotify_recalc_mask(), needs to
set the shadow mask, if conn->type != FSNOTIFY_OBJ_TYPE_INODE.
We already special case FSNOTIFY_OBJ_TYPE_INODE for pinning inode
in several places, so there is nothing new here.

Thanks,
Amir.

Re: [GIT PULL] Fsnotify cleanups

[Amir Goldstein]

On Sat, Jun 9, 2018 at 9:57 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Fri, Jun 8, 2018 at 11:34 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>> On Fri, Jun 8, 2018 at 6:27 AM Jan Kara <jack@suse.cz> wrote:
>>>
>>> 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.
>>
>> Yeah, I don't think there's any way to say "this can't be an array
>> member, don't pad the size out to the alignment".
>>
>> I do wonder if perhaps "struct fsnotify_obj" could actually have a
>> mask and a 32-bit object ID instead, which would (a) avoid the
>> alignment issue and (b) actually shrink the structure onm 64-bit.
>>
>> Obviously you'd then have to look up the pointer from the object ID
>> (presumably using a hash, but maybe it would use the radix tree of
>> idr_alloc() or something).
>>
>> I haven't looked at how often those pointers actually get looked up.
>> If the 'mask' makes it fairly rare, then the extra indirection might
>> be ok. I suspect it's not. But I thought I'd mention it as a possible
>> approach.
>
> As a matter of fact, the pointer is checked to be non zero before
> accessing the mask:
>
> 7c49b8616460 fs/notify: optimize inotify/fsnotify code for unwatched files
>
> So the mask is not really needed to optimize away unwatched inodes.
> I guess the mask is needed to optimize away use cases like frequent
> fsnotify_access() calls on inodes that are watched for modification only,
> but not sure that is worth optimizing.
>
>>
>> The other approach is - as you say - to move 'mask' to be behind the
>> pointer. But if that causes a lot of extra pointer chasing that mask
>> would normally avoid, that could be really expensive too. We see that
>> with the security objects: the cost of looking up whatever is behind
>> i_security is really nasty, because every inode gets allocated a
>> private security structure, so it's absolutely horrendous for cache
>> effects (no sharing).
>
> Besides pointer chasing, there is also srcu_read_lock.
>
>>
>> But I suspect that the behavior of i_fsnotify_marks is very different
>> from i_security, in that most inodes probably don't have marks at all.
>> No? Yes? So you might not have the nasty case of "almost all inode
>> access ends up following that pointer and it's horrible in the cache".
>>
>
> Yes and No :-)
> Yes, most inodes don't have marks at all,
> but, every fsnotify() call may need to follow mnt->mnt_fsnotify.marks
> if mount is watched and there is no mnt->mnt_fsnotify.mask in fsnotify_obj.
>
> Sure, we don't need to worry about 32bits in struct mount, so we could
> have a different attach method for inodes and mounts, but the whole
> purpose of this cleanup is to make the code more generic in handing
> and attaching of marks to inode/mounts.
>
> How about if we move mask into connector, but also leave a shadow mask,
> in an "inherited" struct i.e.:
>
> /* struct to embed in objects, which marks can be attached to */
> struct fsnotify_obj {
>        struct fsnotify_mark_connector __rcu *marks;
> };
>
> /* struct to embed in objects, which marks can be attached to with
> shadow mask */
> struct fsnotify_obj_mask {
>        struct fsnotify_obj obj;
>        /* all events this object cares about */
>        __u32 mask;
> };
>
> We embed fsnotify_obj in struct inode and fsnotify_obj_mask in struct mount.
>
> The only code that needs to test the shadow mask before following into
> connector is the optimization code in start of fsnotify() which was not
> generalized anyway and where the mask optimization is more important
> for mount watches.
>

No. That's not true, there is also fsnotify_inode_watches_children() and
__fsnotify_parent(). But I think the only bits in the mask that really matter
for optimizing inode watches are FS_ACCESS and FS_EVENT_ON_CHILD
and those could be encoded in lower bits of connector pointer.
connector is pointer is always dereferenced via fsnotify_grab_connector(),
so it's easy to mask out those bits.

> The only code that sets the mask is __fsnotify_recalc_mask(), needs to
> set the shadow mask, if conn->type != FSNOTIFY_OBJ_TYPE_INODE.
> We already special case FSNOTIFY_OBJ_TYPE_INODE for pinning inode
> in several places, so there is nothing new here.
>

So instead, __fsnotify_recalc_mask() needs to set the bits on connector
pointer in FSNOTIFY_OBJ_TYPE_INODE case.

I'll try to sketch some patches.

Thanks,
Amir.

Re: [GIT PULL] Fsnotify cleanups

[Linus Torvalds]

On Fri, Jun 8, 2018 at 11:57 PM Amir Goldstein <amir73il@gmail.com> 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.

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?

             Linus

Re: [GIT PULL] Fsnotify cleanups

[Amir Goldstein]

On Sat, Jun 9, 2018 at 8:30 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Jun 8, 2018 at 11:57 PM Amir Goldstein <amir73il@gmail.com> 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?
>

Well, if Jan agrees with me that using 2 bits for FS_ACCESS and
FS_EVENT_ON_CHILD on pointer is enough for the purpose of
optimizing pointer dereference, then we can have generalized code
with just the pointer in struct inode/mount.

Thanks,
Amir.

Re: [GIT PULL] Fsnotify cleanups

[Amir Goldstein]

On Sat, Jun 9, 2018 at 9:46 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Sat, Jun 9, 2018 at 8:30 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>> On Fri, Jun 8, 2018 at 11:57 PM Amir Goldstein <amir73il@gmail.com> 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

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.

I have a POC patch that removes mask from struct inode and
uses 2 bits in connector pointer for VFS optimizations, but that patch
requires more testing, so for another time.

Thanks,
Amir.

Re: [GIT PULL] Fsnotify cleanups

[Jan Kara]

On Fri 08-06-18 13:34:04, Linus Torvalds wrote:
> On Fri, Jun 8, 2018 at 6:27 AM Jan Kara <jack@suse.cz> wrote:
> >
> > 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.
> 
> Yeah, I don't think there's any way to say "this can't be an array
> member, don't pad the size out to the alignment".
> 
> I do wonder if perhaps "struct fsnotify_obj" could actually have a
> mask and a 32-bit object ID instead, which would (a) avoid the
> alignment issue and (b) actually shrink the structure onm 64-bit.
> 
> Obviously you'd then have to look up the pointer from the object ID
> (presumably using a hash, but maybe it would use the radix tree of
> idr_alloc() or something).
> 
> I haven't looked at how often those pointers actually get looked up.
> If the 'mask' makes it fairly rare, then the extra indirection might
> be ok. I suspect it's not. But I thought I'd mention it as a possible
> approach.

The most critical is the mask check in fs/notify/fsnotify.c: fsnotify().
There the mask matches whenever an event should be delivered to someone
watching inode / vfsmount. And we already know someone is watching due to
previous checks in there, we just don't know whether they are interested in
what is currently happening. How frequent is the mask match is difficult to
tell - it heavily depends on the way fsnotify is used on the system - and
there are definitely heavy fsnotify users where I suspect the additional
idr / hash lookup for each event would be visible. So I suppose we are
better off with a simple approach of grabbing srcu_read_lock and
dereference to access the mask. That is going to visibly hit the cases
where someone is watching the inode / vfsmount but not for event currently
happening (Amir already referenced the commit 7c49b8616460e "fs/notify:
optimize inotify/fsnotify code for unwatched files" which gives an idea of
the impact). Not great but the least of evils I suppose.

> The other approach is - as you say - to move 'mask' to be behind the
> pointer. But if that causes a lot of extra pointer chasing that mask
> would normally avoid, that could be really expensive too. We see that
> with the security objects: the cost of looking up whatever is behind
> i_security is really nasty, because every inode gets allocated a
> private security structure, so it's absolutely horrendous for cache
> effects (no sharing).
> 
> But I suspect that the behavior of i_fsnotify_marks is very different
> from i_security, in that most inodes probably don't have marks at all.
> No? Yes? So you might not have the nasty case of "almost all inode
> access ends up following that pointer and it's horrible in the cache".

Yes, I don't expect most inodes to have any marks attached. Directories
tend to be more likely to have them but still usually only a small fraction
of them.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [GIT PULL] Fsnotify cleanups

[Jan Kara]

On Sun 10-06-18 20:49:16, Amir Goldstein wrote:
> On Sat, Jun 9, 2018 at 9:46 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> > On Sat, Jun 9, 2018 at 8:30 PM, Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> >> On Fri, Jun 8, 2018 at 11:57 PM Amir Goldstein <amir73il@gmail.com> 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 <jack@suse.com>
SUSE Labs, CR

Re: [GIT PULL] Fsnotify cleanups

[Amir Goldstein]

On Mon, Jun 11, 2018 at 4:36 PM, Jan Kara <jack@suse.cz> wrote:
>
> On Sun 10-06-18 20:49:16, Amir Goldstein wrote:
> > On Sat, Jun 9, 2018 at 9:46 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> > > On Sat, Jun 9, 2018 at 8:30 PM, Linus Torvalds
> > > <torvalds@linux-foundation.org> wrote:
> > >> On Fri, Jun 8, 2018 at 11:57 PM Amir Goldstein <amir73il@gmail.com> 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...
>

By moving the checks for object type into the helper?
Maybe I am missing your point.

Anyway, my thinking was:

What do we have to loose from keeping the mask also inside the connector?

Not much. We didn't change the size of connector struct
and it hardly adds any complexity / performance cost.

What do we have to gain from keeping the mask also inside the connector?

We can later get rid of the copy of mask in inode struct as I wrote.
I will follow up on that.

Thanks,
Amir.

Re: [GIT PULL] Fsnotify cleanups

[Jan Kara]

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.

> Anyway, my thinking was:
> 
> What do we have to loose from keeping the mask also inside the connector?
> 
> Not much. We didn't change the size of connector struct
> and it hardly adds any complexity / performance cost.

You've actually grown the connector by 1 long on x86_64 - spinlock_t is
just 4 bytes there. Also it seems a bit stupid to me to have the mask in
two places (connector & object) just to save ifs in two helper functions.

> What do we have to gain from keeping the mask also inside the connector?
> 
> We can later get rid of the copy of mask in inode struct as I wrote.
> I will follow up on that.

If we can get rid of the mask in inode, I'm definitely fine with moving the
mask to the connector.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [GIT PULL] Fsnotify cleanups

[Jan Kara]

On Sat 09-06-18 11:00:17, Amir Goldstein wrote:
> On Sat, Jun 9, 2018 at 9:57 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> > The only code that needs to test the shadow mask before following into
> > connector is the optimization code in start of fsnotify() which was not
> > generalized anyway and where the mask optimization is more important
> > for mount watches.
> >
> 
> No. That's not true, there is also fsnotify_inode_watches_children() and
> __fsnotify_parent(). But I think the only bits in the mask that really matter
> for optimizing inode watches are FS_ACCESS and FS_EVENT_ON_CHILD
> and those could be encoded in lower bits of connector pointer.

Actually for children events, we have a dentry flag
DCACHE_FSNOTIFY_PARENT_WATCHED and that should catch 99% of situation.
FS_EVENT_ON_CHILD is used only to verify event really should be delivered
since the DCACHE_FSNOTIFY_PARENT_WATCHED flag is cleared lazily. Or where
do you think the performance of FS_EVENT_ON_CHILD checking matters?

Also I'm not 100% sure just having FS_ACCESS checked quickly is going to
buy us that much. We have quite a few event types and if someone is
watching for a different event type than the current event (just not
FS_ACCESS), we'll just have to pay the cost of srcu_read_lock and
indirection. So it just depends whether we consider such loads worthwhile
or not...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [GIT PULL] Fsnotify cleanups

[Amir Goldstein]

On Mon, Jun 11, 2018 at 7:12 PM, Jan Kara <jack@suse.cz> wrote:
> On Sat 09-06-18 11:00:17, Amir Goldstein wrote:
>> On Sat, Jun 9, 2018 at 9:57 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>> > The only code that needs to test the shadow mask before following into
>> > connector is the optimization code in start of fsnotify() which was not
>> > generalized anyway and where the mask optimization is more important
>> > for mount watches.
>> >
>>
>> No. That's not true, there is also fsnotify_inode_watches_children() and
>> __fsnotify_parent(). But I think the only bits in the mask that really matter
>> for optimizing inode watches are FS_ACCESS and FS_EVENT_ON_CHILD
>> and those could be encoded in lower bits of connector pointer.
>
> Actually for children events, we have a dentry flag
> DCACHE_FSNOTIFY_PARENT_WATCHED and that should catch 99% of situation.
> FS_EVENT_ON_CHILD is used only to verify event really should be delivered
> since the DCACHE_FSNOTIFY_PARENT_WATCHED flag is cleared lazily. Or where
> do you think the performance of FS_EVENT_ON_CHILD checking matters?
>

I thought that dereferencing the connector from
fsnotify_inode_watches_children()
is not a good idea, so we can set that bit  in connector pointer during
fsnotify_recalc_mask().

> Also I'm not 100% sure just having FS_ACCESS checked quickly is going to
> buy us that much. We have quite a few event types and if someone is
> watching for a different event type than the current event (just not
> FS_ACCESS), we'll just have to pay the cost of srcu_read_lock and
> indirection. So it just depends whether we consider such loads worthwhile
> or not...

I just though user could generate FS_ACCESS callbacks on an inode
at higher rate than any other event types, so for other event types
optimization is going to be less noticeable. Not sure that is true.

Thanks,
Amir.

Re: [GIT PULL] Fsnotify cleanups

[Amir Goldstein]

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.
>
>> Anyway, my thinking was:
>>
>> What do we have to loose from keeping the mask also inside the connector?
>>
>> Not much. We didn't change the size of connector struct
>> and it hardly adds any complexity / performance cost.
>
> You've actually grown the connector by 1 long on x86_64 - spinlock_t is
> just 4 bytes there. Also it seems a bit stupid to me to have the mask in
> two places (connector & object) just to save ifs in two helper functions.
>

Oh! I had CONFIG_DEBUG_LOCK_ALLOC on so hadn't noticed that.

>> What do we have to gain from keeping the mask also inside the connector?
>>
>> We can later get rid of the copy of mask in inode struct as I wrote.
>> I will follow up on that.
>
> If we can get rid of the mask in inode, I'm definitely fine with moving the
> mask to the connector.
>

I'll rework mask back out of connector and will bring that back later if that
makes sense.

BTW, found a way to shave 8 bytes from struct inode as well as from
struct file on x86_64 by shrinking _write_hint to u8 and moving it around.
Will test and send an RFC patch.

Thanks,
Amir.

Re: [GIT PULL] Fsnotify cleanups

[Amir Goldstein]

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.

Thanks!
Amir.

Re: [GIT PULL] Fsnotify cleanups

[Jan Kara]

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.

"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).

Otherwise patches look good to me.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [GIT PULL] Fsnotify cleanups

[Amir Goldstein]

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.

Re: [GIT PULL] Fsnotify cleanups

[Amir Goldstein]

On Wed, Jun 13, 2018 at 4:56 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Wed, Jun 13, 2018 at 4:21 PM, Jan Kara <jack@suse.cz> wrote:

>> 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.

Two places I couldn't resist keeping 'obj':
1. connector->obj
2. fsnotify_obj_{inode,mount}

The first one because conn->connp is horrible.
The second one because most call sites pass conn->obj as argument.

Force pushed result to fsnotify-cleanup.

See if you find it acceptable.

Thanks,
Amir.

Re: [GIT PULL] Fsnotify cleanups

[Jan Kara]

On Thu 14-06-18 01:17:38, Amir Goldstein wrote:
> On Wed, Jun 13, 2018 at 4:56 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> > On Wed, Jun 13, 2018 at 4:21 PM, Jan Kara <jack@suse.cz> wrote:
> 
> >> 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.
> 
> Two places I couldn't resist keeping 'obj':
> 1. connector->obj
> 2. fsnotify_obj_{inode,mount}
> 
> The first one because conn->connp is horrible.
> The second one because most call sites pass conn->obj as argument.
> 
> Force pushed result to fsnotify-cleanup.
> 
> See if you find it acceptable.

Thanks. I like the patches. Just one suggestion for improvement:
Maybe we could call fsnotify_obj_inode() fsnotify_conn_inode()
and let it take connector as an argument. Everybody does conn->obj anyway
unnecessarily. Similarly for mount. And then fsnotify_connector_mask()
could be fsnotify_conn_mask() for consistency?

fsnotify_connector_inode() (or mount) is already a bit too long for my
taste ;).

If you update this, please post the patches to fsdevel with me on CC so
that we have an official posting and I'll pick them up to my tree.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [GIT PULL] Fsnotify cleanups

[Amir Goldstein]

On Fri, Jun 22, 2018 at 7:44 PM, Jan Kara <jack@suse.cz> wrote:
> On Thu 14-06-18 01:17:38, Amir Goldstein wrote:
>> On Wed, Jun 13, 2018 at 4:56 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> > On Wed, Jun 13, 2018 at 4:21 PM, Jan Kara <jack@suse.cz> wrote:
>>
>> >> 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.
>>
>> Two places I couldn't resist keeping 'obj':
>> 1. connector->obj
>> 2. fsnotify_obj_{inode,mount}
>>
>> The first one because conn->connp is horrible.
>> The second one because most call sites pass conn->obj as argument.
>>
>> Force pushed result to fsnotify-cleanup.
>>
>> See if you find it acceptable.
>
> Thanks. I like the patches. Just one suggestion for improvement:
> Maybe we could call fsnotify_obj_inode() fsnotify_conn_inode()
> and let it take connector as an argument. Everybody does conn->obj anyway
> unnecessarily. Similarly for mount. And then fsnotify_connector_mask()
> could be fsnotify_conn_mask() for consistency?

Sure.

>
> fsnotify_connector_inode() (or mount) is already a bit too long for my
> taste ;).

Mine as well.

>
> If you update this, please post the patches to fsdevel with me on CC so
> that we have an official posting and I'll pick them up to my tree.
>

Will do.

Thanks,
Amir.