Re: [PATCH 10/10] send events using read

[Eric Paris <eparis@redhat.com>]

On Tue, 2009-11-03 at 16:59 -0700, Jonathan Corbet wrote:
> Hi, Eric,
> 
> This is not a full review, but I did notice a problem as I was trying to
> figure out the new API...

This is a rip off of inotify which was introduced in commit 3632dee2f8b
from Vegard Nossum back in January.  I can't seem to find any discussion
of this before it went into Linus' tree, so if someone knows how this
patch got to Linus and what was said about it, I'd like to know.  Thanks
for unwittingly finding and inotify bug!

I looked back over it and it looks to me like it will work although
there may be a race like situation if there are multiple things trying
to read events.  I can see how with perfect timing and precision Task A
might try to get the mutex and it is being held by task B.  Task B drops
the mutex and Task A gets it, this causes Task A to be TASK_RUNNABLE.
Lets assume Task B runs back around the loop and tries to get it while
Task A still holds it.  Task A will drop the mutex and Task B gets it,
now B is runnable.  Repeat until infinity with perfect timing!  It's not
really a live-lock, if either of them ever gets the mutex uncontested or
if an event ever arrives they are going to sleep and/or break the cycle.

Certainly I'll take a look at it.

> > +static ssize_t fanotify_read(struct file *file, char __user *buf,
> > +			     size_t count, loff_t *pos)
> > +{
> > +	struct fsnotify_group *group;
> > +	struct fsnotify_event *kevent;
> > +	char __user *start;
> > +	int ret;
> > +	DEFINE_WAIT(wait);
> > +
> > +	start = buf;
> > +	group = file->private_data;
> > +
> > +	pr_debug("%s: group=%p\n", __func__, group);
> > +
> > +	while (1) {
> > +		prepare_to_wait(&group->notification_waitq, &wait, TASK_INTERRUPTIBLE);
> > +
> > +		mutex_lock(&group->notification_mutex);
> > +		kevent = get_one_event(group, count);
> > +		mutex_unlock(&group->notification_mutex);
> 
> prepare_to_wait(), among other things, sets the task state.  But then you
> go into various sleeping calls (mutex_lock(), for starters); that will undo
> what prepare_to_wait has done.  You'll be back in TASK_RUNNING at this
> point, so you'll never sleep.