* [GIT PULL] Fsnotify cleanups @ 2018-06-07 15:02 Jan Kara 2018-06-07 16:34 ` Linus Torvalds 0 siblings, 1 reply; 22+ messages in thread From: Jan Kara @ 2018-06-07 15:02 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-fsdevel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [GIT PULL] Fsnotify cleanups 2018-06-07 15:02 [GIT PULL] Fsnotify cleanups Jan Kara @ 2018-06-07 16:34 ` Linus Torvalds 2018-06-08 13:27 ` Jan Kara 0 siblings, 1 reply; 22+ messages in thread From: Linus Torvalds @ 2018-06-07 16:34 UTC (permalink / raw) To: Jan Kara; +Cc: linux-fsdevel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [GIT PULL] Fsnotify cleanups 2018-06-07 16:34 ` Linus Torvalds @ 2018-06-08 13:27 ` Jan Kara 2018-06-08 20:34 ` Linus Torvalds 0 siblings, 1 reply; 22+ messages in thread From: Jan Kara @ 2018-06-08 13:27 UTC (permalink / raw) To: Linus Torvalds; +Cc: Jan Kara, linux-fsdevel, Amir Goldstein 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [GIT PULL] Fsnotify cleanups 2018-06-08 13:27 ` Jan Kara @ 2018-06-08 20:34 ` Linus Torvalds 2018-06-09 6:57 ` Amir Goldstein 2018-06-11 11:08 ` Jan Kara 0 siblings, 2 replies; 22+ messages in thread From: Linus Torvalds @ 2018-06-08 20:34 UTC (permalink / raw) To: Jan Kara; +Cc: linux-fsdevel, Amir Goldstein 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [GIT PULL] Fsnotify cleanups 2018-06-08 20:34 ` Linus Torvalds @ 2018-06-09 6:57 ` Amir Goldstein 2018-06-09 8:00 ` Amir Goldstein 2018-06-09 17:30 ` Linus Torvalds 2018-06-11 11:08 ` Jan Kara 1 sibling, 2 replies; 22+ messages in thread From: Amir Goldstein @ 2018-06-09 6:57 UTC (permalink / raw) To: Linus Torvalds; +Cc: Jan Kara, linux-fsdevel 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [GIT PULL] Fsnotify cleanups 2018-06-09 6:57 ` Amir Goldstein @ 2018-06-09 8:00 ` Amir Goldstein 2018-06-11 16:12 ` Jan Kara 2018-06-09 17:30 ` Linus Torvalds 1 sibling, 1 reply; 22+ messages in thread From: Amir Goldstein @ 2018-06-09 8:00 UTC (permalink / raw) To: Linus Torvalds; +Cc: Jan Kara, linux-fsdevel 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [GIT PULL] Fsnotify cleanups 2018-06-09 8:00 ` Amir Goldstein @ 2018-06-11 16:12 ` Jan Kara 2018-06-11 16:31 ` Amir Goldstein 0 siblings, 1 reply; 22+ messages in thread From: Jan Kara @ 2018-06-11 16:12 UTC (permalink / raw) To: Amir Goldstein; +Cc: Linus Torvalds, Jan Kara, linux-fsdevel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [GIT PULL] Fsnotify cleanups 2018-06-11 16:12 ` Jan Kara @ 2018-06-11 16:31 ` Amir Goldstein 0 siblings, 0 replies; 22+ messages in thread From: Amir Goldstein @ 2018-06-11 16:31 UTC (permalink / raw) To: Jan Kara; +Cc: Linus Torvalds, linux-fsdevel 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [GIT PULL] Fsnotify cleanups 2018-06-09 6:57 ` Amir Goldstein 2018-06-09 8:00 ` Amir Goldstein @ 2018-06-09 17:30 ` Linus Torvalds 2018-06-09 18:46 ` Amir Goldstein 1 sibling, 1 reply; 22+ messages in thread From: Linus Torvalds @ 2018-06-09 17:30 UTC (permalink / raw) To: Amir Goldstein; +Cc: Jan Kara, linux-fsdevel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [GIT PULL] Fsnotify cleanups 2018-06-09 17:30 ` Linus Torvalds @ 2018-06-09 18:46 ` Amir Goldstein 2018-06-10 17:49 ` Amir Goldstein 0 siblings, 1 reply; 22+ messages in thread From: Amir Goldstein @ 2018-06-09 18:46 UTC (permalink / raw) To: Linus Torvalds; +Cc: Jan Kara, linux-fsdevel 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [GIT PULL] Fsnotify cleanups 2018-06-09 18:46 ` Amir Goldstein @ 2018-06-10 17:49 ` Amir Goldstein 2018-06-11 13:36 ` Jan Kara 0 siblings, 1 reply; 22+ messages in thread From: Amir Goldstein @ 2018-06-10 17:49 UTC (permalink / raw) To: Jan Kara; +Cc: linux-fsdevel, Linus Torvalds 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [GIT PULL] Fsnotify cleanups 2018-06-10 17:49 ` Amir Goldstein @ 2018-06-11 13:36 ` Jan Kara 2018-06-11 13:58 ` Amir Goldstein 0 siblings, 1 reply; 22+ messages in thread From: Jan Kara @ 2018-06-11 13:36 UTC (permalink / raw) To: Amir Goldstein; +Cc: Jan Kara, linux-fsdevel, Linus Torvalds 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [GIT PULL] Fsnotify cleanups 2018-06-11 13:36 ` Jan Kara @ 2018-06-11 13:58 ` Amir Goldstein 2018-06-11 16:03 ` Jan Kara 0 siblings, 1 reply; 22+ messages in thread From: Amir Goldstein @ 2018-06-11 13:58 UTC (permalink / raw) To: Jan Kara; +Cc: linux-fsdevel, Linus Torvalds 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [GIT PULL] Fsnotify cleanups 2018-06-11 13:58 ` Amir Goldstein @ 2018-06-11 16:03 ` Jan Kara 2018-06-11 16:38 ` Amir Goldstein 0 siblings, 1 reply; 22+ messages in thread From: Jan Kara @ 2018-06-11 16:03 UTC (permalink / raw) To: Amir Goldstein; +Cc: Jan Kara, linux-fsdevel, Linus Torvalds 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [GIT PULL] Fsnotify cleanups 2018-06-11 16:03 ` Jan Kara @ 2018-06-11 16:38 ` Amir Goldstein 2018-06-11 19:51 ` Amir Goldstein 0 siblings, 1 reply; 22+ messages in thread From: Amir Goldstein @ 2018-06-11 16:38 UTC (permalink / raw) To: Jan Kara; +Cc: linux-fsdevel, Linus Torvalds 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [GIT PULL] Fsnotify cleanups 2018-06-11 16:38 ` Amir Goldstein @ 2018-06-11 19:51 ` Amir Goldstein 2018-06-13 13:21 ` Jan Kara 0 siblings, 1 reply; 22+ messages in thread From: Amir Goldstein @ 2018-06-11 19:51 UTC (permalink / raw) To: Jan Kara; +Cc: linux-fsdevel, Linus Torvalds 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [GIT PULL] Fsnotify cleanups 2018-06-11 19:51 ` Amir Goldstein @ 2018-06-13 13:21 ` Jan Kara 2018-06-13 13:56 ` Amir Goldstein 0 siblings, 1 reply; 22+ messages in thread From: Jan Kara @ 2018-06-13 13:21 UTC (permalink / raw) To: Amir Goldstein; +Cc: Jan Kara, linux-fsdevel, Linus Torvalds 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [GIT PULL] Fsnotify cleanups 2018-06-13 13:21 ` Jan Kara @ 2018-06-13 13:56 ` Amir Goldstein 2018-06-13 22:17 ` Amir Goldstein 0 siblings, 1 reply; 22+ messages in thread From: Amir Goldstein @ 2018-06-13 13:56 UTC (permalink / raw) To: Jan Kara; +Cc: linux-fsdevel, Linus Torvalds 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [GIT PULL] Fsnotify cleanups 2018-06-13 13:56 ` Amir Goldstein @ 2018-06-13 22:17 ` Amir Goldstein 2018-06-22 16:44 ` Jan Kara 0 siblings, 1 reply; 22+ messages in thread From: Amir Goldstein @ 2018-06-13 22:17 UTC (permalink / raw) To: Jan Kara; +Cc: linux-fsdevel 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [GIT PULL] Fsnotify cleanups 2018-06-13 22:17 ` Amir Goldstein @ 2018-06-22 16:44 ` Jan Kara 2018-06-23 7:42 ` Amir Goldstein 0 siblings, 1 reply; 22+ messages in thread From: Jan Kara @ 2018-06-22 16:44 UTC (permalink / raw) To: Amir Goldstein; +Cc: Jan Kara, linux-fsdevel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [GIT PULL] Fsnotify cleanups 2018-06-22 16:44 ` Jan Kara @ 2018-06-23 7:42 ` Amir Goldstein 0 siblings, 0 replies; 22+ messages in thread From: Amir Goldstein @ 2018-06-23 7:42 UTC (permalink / raw) To: Jan Kara; +Cc: linux-fsdevel 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [GIT PULL] Fsnotify cleanups 2018-06-08 20:34 ` Linus Torvalds 2018-06-09 6:57 ` Amir Goldstein @ 2018-06-11 11:08 ` Jan Kara 1 sibling, 0 replies; 22+ messages in thread From: Jan Kara @ 2018-06-11 11:08 UTC (permalink / raw) To: Linus Torvalds; +Cc: Jan Kara, linux-fsdevel, Amir Goldstein 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2018-06-23 7:42 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-07 15:02 [GIT PULL] Fsnotify cleanups Jan Kara 2018-06-07 16:34 ` Linus Torvalds 2018-06-08 13:27 ` Jan Kara 2018-06-08 20:34 ` Linus Torvalds 2018-06-09 6:57 ` Amir Goldstein 2018-06-09 8:00 ` Amir Goldstein 2018-06-11 16:12 ` Jan Kara 2018-06-11 16:31 ` Amir Goldstein 2018-06-09 17:30 ` Linus Torvalds 2018-06-09 18:46 ` Amir Goldstein 2018-06-10 17:49 ` Amir Goldstein 2018-06-11 13:36 ` Jan Kara 2018-06-11 13:58 ` Amir Goldstein 2018-06-11 16:03 ` Jan Kara 2018-06-11 16:38 ` Amir Goldstein 2018-06-11 19:51 ` Amir Goldstein 2018-06-13 13:21 ` Jan Kara 2018-06-13 13:56 ` Amir Goldstein 2018-06-13 22:17 ` Amir Goldstein 2018-06-22 16:44 ` Jan Kara 2018-06-23 7:42 ` Amir Goldstein 2018-06-11 11:08 ` Jan Kara
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.