On Wed 17-02-21 14:33:46, Amir Goldstein wrote:
> On Tue, Feb 16, 2021 at 5:02 PM Jan Kara <jack@xxxxxxx> wrote:
> > > @@ -300,10 +301,16 @@ static long inotify_ioctl(struct file *file, unsigned int cmd,
> > > switch (cmd) {
> > > case FIONREAD:
> > > spin_lock(&group->notification_lock);
> > > - list_for_each_entry(fsn_event, &group->notification_list,
> > > - list) {
> > > - send_len += sizeof(struct inotify_event);
> > > - send_len += round_event_name_len(fsn_event);
> > > + list = fsnotify_first_notification_list(group);
> > > + /*
> > > + * With multi queue, send_len will be a lower bound
> > > + * on total events size.
> > > + */
> > > + if (list) {
> > > + list_for_each_entry(fsn_event, list, list) {
> > > + send_len += sizeof(struct inotify_event);
> > > + send_len += round_event_name_len(fsn_event);
> > > + }
> >
> > As I write below IMO we should enable hashed queues also for inotify (is
> > there good reason not to?)
>
> I see your perception of inotify_merge() is the same as mine was
> when I wrote a patch to support hashed queues for inotify.
> It is only after that I realized that inotify_merge() only ever merges
> with the last event and I dropped that patch.
> I see no reason to change this long time behavior.
Ah, I even briefly looked at that code but didn't notice it merges only
with the last event. I agree that hashing for inotify doesn't make sense
then.
Hum, if the hashing and merging is specific to fanotify and as we decided
to keep the event->list for the global event list, we could easily have the
hash table just in fanotify private group data and hash->next pointer in
fanotify private part of the event? Maybe that would even result in a more
compact code?
> > > +static inline size_t fsnotify_group_size(unsigned int q_hash_bits)
> > > +{
> > > + return sizeof(struct fsnotify_group) + (sizeof(struct list_head) << q_hash_bits);
> > > +}
> > > +
> > > +static inline unsigned int fsnotify_event_bucket(struct fsnotify_group *group,
> > > + struct fsnotify_event *event)
> > > +{
> > > + /* High bits are better for hash */
> > > + return (event->key >> (32 - group->q_hash_bits)) & group->max_bucket;
> > > +}
> >
> > Why not use hash_32() here? IMHO better than just stripping bits...
>
> See hash_ptr(). There is a reason to use the highest bits.
Well, but event->key is just a 32-bit number so I don't follow how high
bits used by hash_ptr() matter?
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
