On Wed, Feb 26, 2020 at 7:07 PM Jan Kara <jack@xxxxxxx> wrote: > > On Wed 26-02-20 13:53:06, Amir Goldstein wrote: > > On Wed, Feb 26, 2020 at 12:23 PM Jan Kara <jack@xxxxxxx> wrote: > > > > > > > + __kernel_fsid_t fsid; > > > > union { > > > > /* > > > > * We hold ref to this path so it may be dereferenced at any > > > > > > Here I disagree. IMO 'fsid' should be still part of the union below because > > > the "object identification" is either struct path or (fsid + fh). I > > > understand that you want to reuse fsid for the other file handle. But then > > > I believe it should rather be done like: > > > > > > struct fanotify_fh { > > > union { > > > unsigned char fh[FANOTIFY_INLINE_FH_LEN]; > > > unsigned char *ext_fh; > > > }; > > > }; > > > > > > > This I will do. > > > > > struct fanotify_fid { > > > __kernel_fsid_t fsid; > > > struct fanotify_fh object; > > > struct fanotify_fh dir; > > > } > > > > > > > object and dir do not end up in the same struct. > > Right, ok. > > > object is in fanotify_event > > dir is in the extended fanotify_name_event, but I can do: > > > > struct fanotify_fid { > > __kernel_fsid_t fsid; > > struct fanotify_fh fh; > > } > > > > struct fanotify_event { > > struct fsnotify_event fse; > > u32 mask; > > struct fanotify_fid_hdr fh; > > struct fanotify_fid_hdr dfh; > > union { > > struct path path; > > struct fanotify_fid object; > > }; > > struct pid *pid; > > }; > > > > struct fanotify_name_event { > > struct fanotify_event fae; > > struct fanotify_fh dir; > > struct qstr name; > > unsigned char inline_name[FANOTIFY_INLINE_NAME_LEN]; > > }; > > Looking at this I'm not quite happy either :-| E.g. 'dfh' contents here > somewhat magically tells that this is not fanotify_event but > fanotify_name_event. Also I agree that fsid hidden in 'object' is not ideal > although I still dislike having it directly in fanotify_event as for path > events it will not be filled and that can lead to confusion. > > I understand this is so convoluted because there are several constraints: > 1) We don't want to grow event size unnecessarily. > 2) We prefer allocating from dedicated slab cache > 3) We have events of several types needing to store different kind of > information. > > But seeing how things evolve I think we should consider relaxing some of > the constraints to make the code easier to follow. How about having > something like: > > struct fanotify_event { > struct fsnotify_event fse; > u32 mask; > enum fanotify_event_type type; > struct pid *pid; > }; > > where type would identify what kind of event we have. Then we would have > > struct fanotify_path_event { > struct fanotify_event fae; > struct path path; > }; > > struct fanotify_perm_path_event { > struct fanotify_event fae; > struct path path; Any reason not to "inherit" from fanotify_path_event? There is code that is generic to permission and non-permission path events that accesses event->path and I wouldn't want to make that code two cases instead of just one. > unsigned short response; > unsigned short state; > int fd; > }; > > struct fanotify_fh { > u8 type; > u8 len; That's a 6 bytes hole! and then there are two of those in object_fh and dir_fh. That is why I stored the header in separate from the fh itself so that two headers could pack up nicely and yes, I also used the headers as an event type indication. > union { > unsigned char fh[FANOTIFY_INLINE_FH_LEN]; > unsigned char *ext_fh; > }; > }; > > struct fanotify_fid_event { > struct fanotify_event fae; > __kernel_fsid_t fsid; > struct fanotify_fh object_fh; > }; > > struct fanofify_name_event { > struct fanotify_event fae; > __kernel_fsid_t fsid; > struct fanotify_fh object_fh; Again, any reason not to "inherit" from fanotify_fid_event? There is plenty of code that is common to fid and name events because name events are also fid events. > struct fanotify_fh dir_fh; > u8 name_len; > char name[0]; > }; > > WRT size, this would grow fanotify_fid_event by 1 long on 64-bits, > fanotify_path_event would be actually smaller by 1 long, fanofify_name_event > would be smaller but that's not really comparable because you chose a > solution with fixed-inline length while I'd just go with allocating from > kmalloc when we have to store the name. OK. Same an inotify. I guess I started with the name_snapshot thing that was really fixed-size event and then reused the same construct without the snapshot, but I guess we can do away with the inline name. > > In terms of kmalloc caches, we would need three: for path, perm_path, fid > events, I'd allocate name events from generic kmalloc caches. > > So overall I think this would be better. The question is whether the > resulting code will really be more readable. I hope so because the > structures are definitely nicer this way and things belonging logically > together are now together. But you never know until you convert the code... > Would you be willing to try this refactoring? Yes, but I would like to know what you think about the two 6 byte holes Just let that space be wasted for the sake of nicer abstraction? It seems like too much to me. Thanks, Amir.