Re: [RFC][PATCH] inotify 0.10.0

Robert Love
2004-09-27T20:57:01Z

On Sun, 2004-09-26 at 21:17 -0700, Andrew Morton wrote:

> Please raise patches against current kernels from
> ftp://ftp.kernel.org/pub/linux/kernel/v2.6/snapshots. This kernel is six
> weeks old.

I have patches against newer kernels at

http://www.kernel.org/pub/linux/kernel/people/rml/inotify

Anyhow, the tidal flood of commentary is appreciated. I have addressed
all of your issues ...

> > +#define INOTIFY_VERSION "0.10.0"
>
> You should plan to remove this - it becomes meainingless once the code
> is merged up, and nobody ever updates it as they patch things.

Agreed. Patch sent.

> +#define INOTIFY_DEV_TIMER_TIME (jiffies + (HZ/4))
>
> ick. Don't hide the logic in a #define.
>
> static inline arm_dev_timer(struct inotify_device *dev)
> {
> mod_timer(&dev->timer, jiffies + HZ/4);
> }
>
> is nicer.

It is used in more than one place, but OK. Patch sent.

> > +/* For debugging */
> > +static int event_object_count;
> > +static int watcher_object_count;
> > +static int inode_ref_count;
>
> OK. These are accessed racily. Either make them atomic_t's or remove them.

They are just statistics. I'd prefer to remove them entirely (I don't
personally think that the debugging code, statistics, etc. should go
into the mainline kernel).

> > +static int find_inode(const char __user *dirname, struct inode **inode)
>
> This can just return an inode*, or an IS_ERR() errno, I think?

Yes, it can. Done, patch sent.

> > +struct inotify_kernel_event *kernel_event(int wd, int mask,
> > + const char *filename)
> > +{
> > + struct inotify_kernel_event *kevent;
> > +
> > + kevent = kmem_cache_alloc(kevent_cache, GFP_ATOMIC);
>
> Try to rearrange things so the allocation mode here can become GFP_KERNEL.

Hrmph.

> Are there ever enough of these objects in flight to justify a standalone
> slab cache?

Yes. There are up to 256*8 possible events and (more importantly than
the net number, I think) they come and go constantly.

> > + watcher = kmem_cache_alloc(watcher_cache, GFP_KERNEL);

Yes. There can be a lot of watches... one of the intended uses of this
is automatic indexing of a user's homedir (think Apple Spotlight). Alan
Cox has also mentioned virus checking, etc.

There are structures are _not_ used as much, and we do not slab cache
them.

> i_lock is documented as an "innermost" lock.
>
> But here, dev->lock is nesting inside it. Not necessarily a bug per-se,
> but it changes and complicates kernel locking rules, and invalidates
> commentary elsewhere.
>
> If possible, please try to avoid using i_lock. Use i_sem instead.
>
> A bug, I think. What happens if another CPU comes in and tries to take these
> two locks in the opposite order?
>
> The same problem applies if you switch to i_sem. The standard fix is to
> take the lowest-addressed lock first. See d_move() for an example.

I'll work on the locking.

> I'll be merging invalidate_inodes-speedup.patch once the 2.6.10 stream
> opens. That will make the above code simpler, faster and quite different.

Sweet.

> > + add_wait_queue(&dev->wait, &wait);
> > +repeat:
> > + if (signal_pending(current)) {
> > + spin_unlock(&dev->lock);
> > + out = -ERESTARTSYS;
> > + set_current_state(TASK_RUNNING);
> > + remove_wait_queue(&dev->wait, &wait);
> > + goto out;
> > + }
> > + set_current_state(TASK_INTERRUPTIBLE);
> > + if (!inotify_dev_has_events(dev)) {
> > + spin_unlock(&dev->lock);
> > + schedule();
> > + spin_lock(&dev->lock);
> > + goto repeat;
> > + }
> > +
> > + set_current_state(TASK_RUNNING);
> > + remove_wait_queue(&dev->wait, &wait);
>
> The above seems a bit clumsy.

John is reworking inotify_read(), which should take care of the various
issues you raised here.

> > +static int inotify_open(struct inode *inode, struct file *file)
> > +{
> > + struct inotify_device *dev;
> > +
> > + if (atomic_read(&watcher_count) == MAX_INOTIFY_DEVS)
> > + return -ENODEV;
> > +
> > + atomic_inc(&watcher_count);
> > +
> > + dev = kmalloc(sizeof(struct inotify_device), GFP_KERNEL);
> > + if (!dev)
> > + return -ENOMEM;
> > +
> > + memset(dev->bitmask, 0,
> > + sizeof(unsigned long) * MAX_INOTIFY_DEV_WATCHERS / BITS_PER_LONG);
>
> What purpose does this bitmask serve, anyway??

Bitmask of allocated/unallocated watcher descriptors.

Patch sent to add a comment. Also sent a patch to use the bitmap.h
functions instead of this open-coded memset().

> > +static void inotify_release_all_watchers(struct inotify_device *dev)
> > +{
> > + struct inotify_watcher *watcher,*next;
> > +
> > + list_for_each_entry_safe(watcher, next, &dev->watchers, d_list)
> > + ignore_helper(watcher, 0);
> > +}
>
> Locking?
>
> > +
> > +static int inotify_release(struct inode *inode, struct file *file)
> > +{
> > + if (file->private_data) {
>
> Why test ->private_data here?

I'm not sure why. I don't think we ought to - release functions are
only called once, when the last ref on the file dies. I asked John and
sent a patch to remove it.

> If it indeed needs testing here, shouldn't it be zeroed out as well, with
> appropriate locking?
>
> > + struct inotify_device *dev;
> > +
> > + dev = (struct inotify_device *) file->private_data;
>
> Please don't typecast when assigning to and from void*'s

Nod. Patch sent.

> > + del_timer_sync(&dev->timer);
> > + inotify_release_all_watchers(dev);
> > + inotify_release_all_events(dev);
> > + kfree(dev);
> > + }
> > +
> > + printk(KERN_ALERT "inotify device released\n");
> > +
> > + atomic_dec(&watcher_count);
>
> If file->private_data was zero, we shouldn't have decremented this?

I don't think we should test file->private_data at all, so after fixing
that, this is fixed.

> +static int inotify_watch(struct inotify_device *dev,
> + struct inotify_watch_request *request)
> > +{
> > ...
> > + spin_lock(&dev->lock);
> > + spin_lock(&inode->i_lock);
>
> Lock ranking.
>
> > +static int inotify_ioctl(struct inode *ip, struct file *fp,
> > + unsigned int cmd, unsigned long arg) {
>
> An errant brace!

Patch sent.

> > +
> > + if (_IOC_DIR(cmd) & _IOC_READ)
> > + err = !access_ok(VERIFY_READ, (void *) arg, _IOC_SIZE(cmd));
> > +
> > + if (err)
> > + err = -EFAULT;
> > + goto out;
> > +
>
> eh? The above is missing braces, and cannot possibly have worked.

That is my fault, just introduced in the latest revision.

> > + if (_IOC_DIR(cmd) & _IOC_WRITE)
> > + err = !access_ok(VERIFY_WRITE, (void *)arg, _IOC_SIZE(cmd));
> > +
> > + if (err) {
> > + err = -EFAULT;
> > + goto out;
> > + }
>
> Why are these access_ok() checks here? I think they can (should) go away.

They should. We should just use copy_{to,from}_user. Will fix.

> We often do:
>
> switch (cmd) {
> case INOTIFY_WATCH:
>
> to save a tabstop.

Yah. I thought I included that in my coding style cleanup. Patch sent.

> > +struct miscdevice inotify_device = {
> > + .minor = MISC_DYNAMIC_MINOR,
> > + .name = "inotify",
> > + .fops = &inotify_fops,
> > +};
>
> Please update devices.txt

Documentation/devices.txt doesn't really have provisions for dynamically
allocated devices, which this is.

We _could_ take a fixed minor...

> > +struct inotify_event {
> > + int wd;
> > + int mask;
> > + int cookie;
> > + char filename[INOTIFY_FILENAME_MAX];
> > +};
>
> yeah, that's not very nice. Better to kmalloc the pathname.

That is the structure that we communicate with to user-space.

We could kmalloc() filename, but it makes the user-space use a bit more
complicated (and right now it is trivial and wonderfully simple).

We've been debating the pros and cons.

> Please add CONFIG_INOTIFY and make all this:
>
> [...]
>
> go away if the user doesn't want inotify. And remember to test with
> CONFIG_INOTIFY=n!

Done. Patch sent.

> > + INIT_LIST_HEAD (&inode->watchers);
>
> Please review the entire patch and ensure that all macros and function
> calls have no space between the identifier and the opening parenthesis.

I thought I caught them all - guess not, patch sent.

Thanks for the feedback.

Robert Love