[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [RFC][PATCH] Inotify kernel API



Some comments below...

On Tuesday ٢٣ August ٢٠٠٥ ١٥:٣٧, Amy Griffis wrote:
> diff -r ٨ecff٩٣e٧٠٤a -r ٥٨e١٣٠١e٩٦٦١ fs/inotify.c
> --- a/fs/inotify.c      Thu Aug ١٨ ١٩:٥٣:٥٩ ٢٠٠٥
> +++ b/fs/inotify.c      Thu Aug ١٨ ٢٣:١٩:٥٢ ٢٠٠٥
> @@ -٨٣,١٤ +٨٣,١٨ @@
>         wait_queue_head_t       wq;             /* wait queue for i/o */
>         struct idr              idr;            /* idr mapping wd -> watch */
>         struct semaphore        sem;            /* protects this bad boy */
> -       struct list_head        events;         /* list of queued events */
>         struct list_head        watches;        /* list of watches */
>         atomic_t                count;          /* reference count */
> +       u٣٢                     last_wd;        /* the last wd allocated */
> +       /* userland consumer API */
> +       struct list_head        events;         /* list of queued events */
>         struct user_struct      *user;          /* user who opened this dev */
>         unsigned int            queue_size;     /* size of the queue (bytes) */
>         unsigned int            event_count;    /* number of pending events */
>         unsigned int            max_events;     /* maximum number of events */
> -       u٣٢                     last_wd;        /* the last wd allocated */
> +       /* kernel consumer API */
> +       void (*callback)(struct inotify_event *, const char *,
> +                        void *);               /* event callback */
>  };
>  
>  /*
> @@ -١٢٢,٦ +١٢٦,٧ @@
>         struct inode            *inode; /* associated inode */
>         s٣٢                     wd;     /* watch descriptor */
>         u٣٢                     mask;   /* event mask for this watch */
> +       void            *callback_arg;  /* callback argument - kernel API */
>  };
>  
>  #ifdef CONFIG_SYSCTL
> @@ -١٧٣,٨ +١٧٨,١٠ @@
>  static inline void put_inotify_dev(struct inotify_device *dev)
>  {
>         if (atomic_dec_and_test(&dev->count)) {
> -               atomic_dec(&dev->user->inotify_devs);
> -               free_uid(dev->user);
> +               if (dev->user) {
> +                       atomic_dec(&dev->user->inotify_devs);
> +                       free_uid(dev->user);
> +               }
>                 kfree(dev);
>         }
>  }
> @@ -٣٤١,٦ +٣٤٨,٢٣ @@
>  }
>  
>  /*
> + * inotify_callback_event - notify kernel consumers of events
> + */
> +static void inotify_callback_event(struct inotify_device *dev, 
> +                                 struct inotify_watch *watch, 
> +                                 u٣٢ mask, u٣٢ cookie, const char *name)
> +{
> +       struct inotify_event event;
> +
> +       event.wd = watch->wd;
> +       event.mask = mask;
> +       event.cookie = cookie;
> +       event.len = ٠; /* kernel consumers don't need length */
> +
> +       dev->callback(&event, name, watch->callback_arg);
> +}
> +
> +/*
>   * inotify_dev_get_wd - returns the next WD for use by the given dev
>   *
>   * Callers must hold dev->sem.  This function can sleep.
> @@ -٣٨٣,١٢ +٤٠٧,١٣ @@
>   * Both 'dev' and 'inode' (by way of nameidata) need to be pinned.
>   */
>  static struct inotify_watch *create_watch(struct inotify_device *dev,
> -                                         u٣٢ mask, struct inode *inode)
> +                                         u٣٢ mask, struct inode *inode,
> +                                         void *callback_arg)
>  {
>         struct inotify_watch *watch;
>         int ret;
>  
> -       if (atomic_read(&dev->user->inotify_watches) >=
> +       if (dev->user && atomic_read(&dev->user->inotify_watches) >=
>                         inotify_max_user_watches)
>                 return ERR_PTR(-ENOSPC);
>  
> @@ -٤٠٤,٦ +٤٢٩,٧ @@
>  
>         dev->last_wd = watch->wd;
>         watch->mask = mask;
> +       watch->callback_arg = callback_arg;
>         atomic_set(&watch->count, ٠);
>         INIT_LIST_HEAD(&watch->d_list);
>         INIT_LIST_HEAD(&watch->i_list);
> @@ -٤٢١,٧ +٤٤٧,٨ @@
>         /* bump our own count, corresponding to our entry in dev->watches */
>         get_inotify_watch(watch);
>  
> -       atomic_inc(&dev->user->inotify_watches);
> +       if (dev->user)
> +               atomic_inc(&dev->user->inotify_watches);
>  
>         return watch;
>  }
> @@ -٤٥٣,٧ +٤٨٠,٨ @@
>         list_del(&watch->i_list);
>         list_del(&watch->d_list);
>  
> -       atomic_dec(&dev->user->inotify_watches);
> +       if (dev->user)
> +               atomic_dec(&dev->user->inotify_watches);
>         idr_remove(&dev->idr, watch->wd);
>         put_inotify_watch(watch);
>  }
> @@ -٤٧١,٧ +٤٩٩,١٠ @@
>   */
>  static void remove_watch(struct inotify_watch *watch,struct inotify_device *dev)
>  {
> -       inotify_dev_queue_event(dev, watch, IN_IGNORED, ٠, NULL);
> +       if (dev->callback)
> +               inotify_callback_event(dev, watch, IN_IGNORED, ٠, NULL);
> +       else
> +               inotify_dev_queue_event(dev, watch, IN_IGNORED, ٠, NULL);
>         remove_watch_no_event(watch, dev);
>  }
>  
> @@ -٤٨٤,٧ +٥١٥,١٧٢ @@
>         return !list_empty(&inode->inotify_watches);
>  }
>  
> -/* Kernel API */
> +/* Kernel consumer API */
> +
> +/*
> + * inotify_init - allocates and initializes an inotify device.
> + */
> +struct inotify_device * 
> +inotify_init(void (*callback)(struct inotify_event *, const char *, void *))
> +{
> +       struct inotify_device *dev;
> +
> +       dev = kmalloc(sizeof(struct inotify_device), GFP_KERNEL);
> +       if (unlikely(!dev))
> +               return NULL;
> +
> +       idr_init(&dev->idr);
> +       INIT_LIST_HEAD(&dev->events);
> +       INIT_LIST_HEAD(&dev->watches);
> +       init_waitqueue_head(&dev->wq);
> +       sema_init(&dev->sem, ١);
> +       dev->event_count = ٠;
> +       dev->queue_size = ٠;
> +       dev->max_events = inotify_max_queued_events;
> +       dev->user = NULL;       /* set in sys_inotify_init */
> +       dev->last_wd = ٠;
> +       dev->callback = callback;
> +       atomic_set(&dev->count, ٠);
> +       get_inotify_dev(dev);
> +
> +       return dev;
> +}
> +EXPORT_SYMBOL_GPL(inotify_init);
> +
> +/*
> + * inotify_free - clean up and free an inotify device.
> + */
> +int inotify_free(struct inotify_device *dev)
> +{
> +       /*
> +        * Destroy all of the watches on this device.  Unfortunately, not very
> +        * pretty.  We cannot do a simple iteration over the list, because we
> +        * do not know the inode until we iterate to the watch.  But we need to
> +        * hold inode->inotify_sem before dev->sem.  The following works.
> +        */
> +       while (١) {
> +               struct inotify_watch *watch;
> +               struct list_head *watches;
> +               struct inode *inode;
> +
> +               down(&dev->sem);
> +               watches = &dev->watches;
> +               if (list_empty(watches)) {
> +                       up(&dev->sem);
> +                       break;
> +               }
> +               watch = list_entry(watches->next, struct inotify_watch, d_list);
> +               get_inotify_watch(watch);
> +               up(&dev->sem);
> +
> +               inode = watch->inode;
> +               down(&inode->inotify_sem);
> +               down(&dev->sem);
> +               remove_watch_no_event(watch, dev);
> +               up(&dev->sem);
> +               up(&inode->inotify_sem);
> +               put_inotify_watch(watch);
> +       }
> +
> +       /* destroy all of the events on this device */
> +       down(&dev->sem);
> +       while (!list_empty(&dev->events))
> +               inotify_dev_event_dequeue(dev);
> +       up(&dev->sem);
> +
> +       /* free this device: the put matching the get in inotify_init() */
> +       put_inotify_dev(dev);
> +
> +       return ٠;
> +}
> +EXPORT_SYMBOL_GPL(inotify_free);
> +
> +/*
> + * inotify_add_watch - add a watch to this inotify instance.
> + */
> +s٣٢ inotify_add_watch(struct inotify_device *dev, struct inode *inode, 
> +                     u٣٢ mask, void *callback_arg)
> +{
> +       struct inotify_watch *watch, *old;
> +       int ret;
> +
> +       down(&inode->inotify_sem);

What happens if the inode is deleted when attempting to hold down the semaphore?
Perhaps a comment reminding the caller of this function they must hold the proper
inode locks around this function?.

> +       down(&dev->sem);
> +
> +       /* don't let consumers set invalid bits: we don't want flags set */
> +       mask &= IN_ALL_EVENTS;
> +       if (unlikely(!mask)) {
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +
> +       /*
> +        * Handle the case of re-adding a watch on an (inode,dev) pair that we
> +        * are already watching.  We just update the mask and callback_arg and
> +        * return its wd.
> +        */
> +       old = inode_find_dev(inode, dev);
> +       if (unlikely(old)) {
> +               old->mask = mask;
> +               old->callback_arg = callback_arg;

You can leak memory here, right?  Maybe there needs to be a old->callback_free
field that, when set, knows how to clean up old->callback_arg, before pointing it
else where.  Not sure we can assume that the callback_arg is always going to be
statically allocated.

> +               ret = old->wd;
> +               goto out;
> +       }
> +
> +       watch = create_watch(dev, mask, inode, callback_arg);
> +       if (unlikely(IS_ERR(watch))) {
> +               ret = PTR_ERR(watch);
> +               goto out;
> +       }
> +
> +       /* Add the watch to the device's and the inode's list */
> +       list_add(&watch->d_list, &dev->watches);
> +       list_add(&watch->i_list, &inode->inotify_watches);
> +       ret = watch->wd;
> +out:
> +       up(&dev->sem);
> +       up(&inode->inotify_sem);
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(inotify_add_watch);
> +
> +/*
> + * inotify_ignore - remove a given wd from this inotify instance.
> + *
> + * Can sleep.
> + */
> +int inotify_ignore(struct inotify_device *dev, s٣٢ wd)
> +{
> +       struct inotify_watch *watch;
> +       struct inode *inode;
> +
> +       down(&dev->sem);
> +       watch = idr_find(&dev->idr, wd);
> +       if (unlikely(!watch)) {
> +               up(&dev->sem);
> +               return -EINVAL;
> +       }
> +       get_inotify_watch(watch);
> +       inode = watch->inode;
> +       up(&dev->sem);
> +
> +       down(&inode->inotify_sem);
> +       down(&dev->sem);
> +
> +       /* make sure that we did not race */
> +       watch = idr_find(&dev->idr, wd);
> +       if (likely(watch))
> +               remove_watch(watch, dev);
> +
> +       up(&dev->sem);
> +       up(&inode->inotify_sem);
> +       put_inotify_watch(watch);
> +
> +       return ٠;
> +}
> +EXPORT_SYMBOL_GPL(inotify_ignore);
> +
> +/* Kernel producer API */
>  
>  /**
>   * inotify_inode_queue_event - queue an event to all watches on this inode

Since the purpose of this patch is to add a kernel API to Inotify, I think it should
be as clean as possible.   With this in mind, I think that since we _might_ be calling
inotify_callback_event that the calling function not be inotify_inode_queue_event;
it's not all that intuitive.  Instead, I think this function should be renamed to 
something like, inotify_inode_handle_event...

> @@ -٥٠٨,٧ +٧٠٤,١٢ @@
>                         struct inotify_device *dev = watch->dev;
>                         get_inotify_watch(watch);
>                         down(&dev->sem);
> -                       inotify_dev_queue_event(dev, watch, mask, cookie, name);
> +                       if (dev->callback)
> +                               inotify_callback_event(dev, watch, mask, 
> +                                                      cookie, name);
> +                       else
> +                               inotify_dev_queue_event(dev, watch, mask,
> +                                                       cookie, name);
>                         if (watch_mask & IN_ONESHOT)
>                                 remove_watch_no_event(watch, dev);
>                         up(&dev->sem);
> @@ -٦٢٢,٧ +٨٢٣,١٢ @@
>                 list_for_each_entry_safe(watch, next_w, watches, i_list) {
>                         struct inotify_device *dev = watch->dev;
>                         down(&dev->sem);
> -                       inotify_dev_queue_event(dev, watch, IN_UNMOUNT,٠,NULL);
> +                       if (dev->callback)
> +                               inotify_callback_event(dev, watch, IN_UNMOUNT, 
> +                                                      ٠, NULL);
> +                       else
> +                               inotify_dev_queue_event(dev, watch, IN_UNMOUNT,
> +                                                       ٠, NULL);
>                         remove_watch(watch, dev);
>                         up(&dev->sem);
>                 }
> @@ -٧٤٨,٨٣ +٩٥٤,٧ @@
>  
>  static int inotify_release(struct inode *ignored, struct file *file)
>  {
> -       struct inotify_device *dev = file->private_data;
> -
> -       /*
> -        * Destroy all of the watches on this device.  Unfortunately, not very
> -        * pretty.  We cannot do a simple iteration over the list, because we
> -        * do not know the inode until we iterate to the watch.  But we need to
> -        * hold inode->inotify_sem before dev->sem.  The following works.
> -        */
> -       while (١) {
> -               struct inotify_watch *watch;
> -               struct list_head *watches;
> -               struct inode *inode;
> -
> -               down(&dev->sem);
> -               watches = &dev->watches;
> -               if (list_empty(watches)) {
> -                       up(&dev->sem);
> -                       break;
> -               }
> -               watch = list_entry(watches->next, struct inotify_watch, d_list);
> -               get_inotify_watch(watch);
> -               up(&dev->sem);
> -
> -               inode = watch->inode;
> -               down(&inode->inotify_sem);
> -               down(&dev->sem);
> -               remove_watch_no_event(watch, dev);
> -               up(&dev->sem);
> -               up(&inode->inotify_sem);
> -               put_inotify_watch(watch);
> -       }
> -
> -       /* destroy all of the events on this device */
> -       down(&dev->sem);
> -       while (!list_empty(&dev->events))
> -               inotify_dev_event_dequeue(dev);
> -       up(&dev->sem);
> -
> -       /* free this device: the put matching the get in inotify_init() */
> -       put_inotify_dev(dev);
> -
> -       return ٠;
> -}
> -
> -/*
> - * inotify_ignore - remove a given wd from this inotify instance.
> - *
> - * Can sleep.
> - */
> -static int inotify_ignore(struct inotify_device *dev, s٣٢ wd)
> -{
> -       struct inotify_watch *watch;
> -       struct inode *inode;
> -
> -       down(&dev->sem);
> -       watch = idr_find(&dev->idr, wd);
> -       if (unlikely(!watch)) {
> -               up(&dev->sem);
> -               return -EINVAL;
> -       }
> -       get_inotify_watch(watch);
> -       inode = watch->inode;
> -       up(&dev->sem);
> -
> -       down(&inode->inotify_sem);
> -       down(&dev->sem);
> -
> -       /* make sure that we did not race */
> -       watch = idr_find(&dev->idr, wd);
> -       if (likely(watch))
> -               remove_watch(watch, dev);
> -
> -       up(&dev->sem);
> -       up(&inode->inotify_sem);
> -       put_inotify_watch(watch);
> -
> -       return ٠;
> +       return inotify_free(file->private_data);
>  }
>  
>  static long inotify_ioctl(struct file *file, unsigned int cmd,
> @@ -٨٧٨,١١ +١٠٠٨,١٤ @@
>                 goto out_free_uid;
>         }
>  
> -       dev = kmalloc(sizeof(struct inotify_device), GFP_KERNEL);
> +       dev = inotify_init(NULL);
>         if (unlikely(!dev)) {
>                 ret = -ENOMEM;
>                 goto out_free_uid;
>         }
> +
> +       dev->user = user;
> +       atomic_inc(&user->inotify_devs);
>  
>         filp->f_op = &inotify_fops;
>         filp->f_vfsmnt = mntget(inotify_mnt);
> @@ -٨٩٢,٢٠ +١٠٢٥,٦ @@
>         filp->f_flags = O_RDONLY;
>         filp->private_data = dev;
>  
> -       idr_init(&dev->idr);
> -       INIT_LIST_HEAD(&dev->events);
> -       INIT_LIST_HEAD(&dev->watches);
> -       init_waitqueue_head(&dev->wq);
> -       sema_init(&dev->sem, ١);
> -       dev->event_count = ٠;
> -       dev->queue_size = ٠;
> -       dev->max_events = inotify_max_queued_events;
> -       dev->user = user;
> -       dev->last_wd = ٠;
> -       atomic_set(&dev->count, ٠);
> -
> -       get_inotify_dev(dev);
> -       atomic_inc(&user->inotify_devs);
>         fd_install(fd, filp);
>  
>         return fd;
> @@ -٩١٩,٧ +١٠٣٨,٦ @@
>  
>  asmlinkage long sys_inotify_add_watch(int fd, const char __user *path, u٣٢ mask)
>  {
> -       struct inotify_watch *watch, *old;
>         struct inode *inode;
>         struct inotify_device *dev;
>         struct nameidata nd;
> @@ -٩٣٨,٤٦ +١٠٥٦,١٤ @@
>  
>         ret = find_inode(path, &nd);
>         if (unlikely(ret))
> -               goto fput_and_out;
> +               goto out;
>  
>         /* inode held in place by reference to nd; dev by fget on fd */
>         inode = nd.dentry->d_inode;
>         dev = filp->private_data;
>  
> -       down(&inode->inotify_sem);
> -       down(&dev->sem);
> -
> -       /* don't let user-space set invalid bits: we don't want flags set */
> -       mask &= IN_ALL_EVENTS;
> -       if (unlikely(!mask)) {
> -               ret = -EINVAL;
> -               goto out;
> -       }
> -
> -       /*
> -        * Handle the case of re-adding a watch on an (inode,dev) pair that we
> -        * are already watching.  We just update the mask and return its wd.
> -        */
> -       old = inode_find_dev(inode, dev);
> -       if (unlikely(old)) {
> -               old->mask = mask;
> -               ret = old->wd;
> -               goto out;
> -       }
> -
> -       watch = create_watch(dev, mask, inode);
> -       if (unlikely(IS_ERR(watch))) {
> -               ret = PTR_ERR(watch);
> -               goto out;
> -       }
> -
> -       /* Add the watch to the device's and the inode's list */
> -       list_add(&watch->d_list, &dev->watches);
> -       list_add(&watch->i_list, &inode->inotify_watches);
> -       ret = watch->wd;
> +       ret = inotify_add_watch(dev, inode, mask, NULL);
>  out:
> -       up(&dev->sem);
> -       up(&inode->inotify_sem);
>         path_release(&nd);
>  fput_and_out:
>         fput_light(filp, fput_needed);
> diff -r ٨ecff٩٣e٧٠٤a -r ٥٨e١٣٠١e٩٦٦١ include/linux/inotify.h
> --- a/include/linux/inotify.h   Thu Aug ١٨ ١٩:٥٣:٥٩ ٢٠٠٥
> +++ b/include/linux/inotify.h   Thu Aug ١٨ ٢٣:١٩:٥٢ ٢٠٠٥
> @@ -١٤,٦ +١٤,٩ @@
>   *
>   * When you are watching a directory, you will receive the filename for events
>   * such as IN_CREATE, IN_DELETE, IN_OPEN, IN_CLOSE, ..., relative to the wd.
> + *
> + * When using inotify from the kernel, len will always be zero.  Instead you
> + * should check the path for non-NULL in your callback.
>   */
>  struct inotify_event {
>         __s٣٢           wd;             /* watch descriptor */
> @@ -٦٨,٦ +٧١,١٧ @@
>  
>  #ifdef CONFIG_INOTIFY
>  
> +/* Kernel consumer API */
> +
> +extern struct inotify_device *inotify_init(void (*)(struct inotify_event *, 
> +                                                   const char *, void *));
> +extern int inotify_free(struct inotify_device *);
> +extern __s٣٢ inotify_add_watch(struct inotify_device *, struct inode *, __u٣٢,
> +                              void *);
> +extern int inotify_ignore(struct inotify_device *, __s٣٢);
> +
> +/* Kernel producer API */
> +
>  extern void inotify_inode_queue_event(struct inode *, __u٣٢, __u٣٢,
>                                       const char *);
>  extern void inotify_dentry_parent_queue_event(struct dentry *, __u٣٢, __u٣٢,
> @@ -٧٧,٦ +٩١,٨ @@
>  extern u٣٢ inotify_get_cookie(void);
>  
>  #else
> +
> +/* Kernel producer API stubs */
>  
>  static inline void inotify_inode_queue_event(struct inode *inode,
>                                              __u٣٢ mask, __u٣٢ cookie,


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]