lkml.org 
[lkml]   [2004]   [Sep]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RFC][PATCH] inotify 0.10.0
    From
    Date
    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


    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2005-03-22 14:06    [W:3.186 / U:0.092 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site