Messages in this thread | | | Subject | Re: [PATCH -v3 7/8] fsnotify: add in inode fsnotify markings | From | Eric Paris <> | Date | Fri, 28 Nov 2008 18:43:06 -0500 |
| |
On Fri, 2008-11-28 at 05:42 +0000, Al Viro wrote: > On Tue, Nov 25, 2008 at 12:21:28PM -0500, Eric Paris wrote: > > > +void fsnotify_mark_get(struct fsnotify_mark_entry *entry) > > +{ > > + spin_lock(&entry->lock); > > + entry->refcnt++; > > + spin_unlock(&entry->lock); > > +} > > > +void fsnotify_mark_put(struct fsnotify_mark_entry *entry) > > +{ > > + spin_lock(&entry->lock); > > + entry->refcnt--; > > + /* if (!refcnt && killme) we are off both lists and nothing else can find us. */ > > + if ((!entry->refcnt) && (entry->killme)) { > > + spin_unlock(&entry->lock); > > + fsnotify_mark_kill(entry); > > + return; > > + } > > + spin_unlock(&entry->lock); > > +} > > Uh-huh... And what happens if fsnotify_mark_get() comes in the middle > of final fsnotify_mark_put()? You spin on entry->lock, gain it just before > fsnotify_mark_kill() which proceeds to kfree entry under you just as you > increment its refcnt...
fsnotify_mark_get() can only find this object through either the entry->i_list or entry->g_list. When we drop our ref to 0 and hold the spinlock we know that no other task would have been able to find us on those lists (everything that searches the i_list holds the i_fsnotify_lock and that lock was dropped since we cleared ourselves from that list and the same is true for the lock on the g_list side)
So if kill_me is set and the refcnt == 0 we are not on either list and no other task could find this to try to call mark_get(). I'll review it to make sure, but the design is that we are safe since nothing else can find us to increment the ref cnt. > > > +void fsnotify_clear_mark_group(struct fsnotify_group *group) > > +{ > > + struct fsnotify_mark_entry *entry; > > + struct inode *inode; > > + > > + mutex_lock(&group->mark_mutex); > > + while (!list_empty(&group->mark_entries)) { > > + entry = list_first_entry(&group->mark_entries, struct fsnotify_mark_entry, g_list); > > + > > + /* make sure the entry survives until it is off both lists */ > > + fsnotify_mark_get(entry); > > + > > + /* remove from g_list */ > > + list_del_init(&entry->g_list); > > + mutex_unlock(&group->mark_mutex); > > + > > + inode = entry->inode; > > + > > + spin_lock(&entry->lock); > > + entry->killme = 1; > > + spin_unlock(&entry->lock); > > > > + /* remove from i_list */ > > + spin_lock(&inode->i_fsnotify_lock); > > ... and just what would keep the inode from being freed under you here?
I'll review.
| |