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;
unsigned short response;
unsigned short state;
int fd;
};
struct fanotify_fh {
u8 type;
u8 len;
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;
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.
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?
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
