On Mon, 2011-02-21 at 18:33 +0100, Lino Sanfilippo wrote: > Currently the type of a mark is checked in fsnotify_add_mark() and if its an > inode mark a ref is grabbed. Since this part is only needed for inode marks it > is shiftet to add_inode_mark(). > Beside this the fsobject is assigned to the mark outside of the fsobject lock. > > Signed-off-by: Lino Sanfilippo <LinoSanfilippo@xxxxxx> I think your patch series is making a noticeable change which I don't think I'm comfortable with. The current code does not pin inodes in core if they only have ignored masks. Under memory pressure those inodes can get eviced and the mark will get cleaned up. My glance at this code leads me to believe that all inodes with any mark is going to be pinned in core. I don't think that's a good idea for AV vendor use where they might want to watch everything on the system with mount point marks and put ignored marks on everything that comes along to cache allows. The fact that inodes might not be pinned in core is the reason for some of the stupid difficulty. There is probably some way to work it out but I think it's something I'm going to need to think about. Am I miss-reading your changes? I think they will work, just maybe not quite how I originally intended. I probably should write an fanotify stress tester that stresses both marks with real masks and with only ignored_masks... -Eric > --- > fs/notify/inode_mark.c | 12 +++++++++--- > fs/notify/mark.c | 2 -- > fs/notify/vfsmount_mark.c | 9 ++++++--- > 3 files changed, 15 insertions(+), 8 deletions(-) > > diff --git a/fs/notify/inode_mark.c b/fs/notify/inode_mark.c > index d438f11..168a242 100644 > --- a/fs/notify/inode_mark.c > +++ b/fs/notify/inode_mark.c > @@ -188,13 +188,12 @@ int fsnotify_add_inode_mark(struct fsnotify_mark *mark, > int ret = 0; > > mark->flags |= FSNOTIFY_MARK_FLAG_INODE; > + mark->i.inode = igrab(inode); > + mark->flags |= FSNOTIFY_MARK_FLAG_OBJECT_PINNED; > > assert_spin_locked(&mark->lock); > > spin_lock(&inode->i_lock); > - > - mark->i.inode = inode; > - > /* is mark the first mark? */ > if (hlist_empty(&inode->i_fsnotify_marks)) { > hlist_add_head_rcu(&mark->i.i_list, &inode->i_fsnotify_marks); > @@ -228,6 +227,13 @@ out: > fsnotify_recalc_inode_mask_locked(inode); > spin_unlock(&inode->i_lock); > > + if (ret) { > + mark->flags &= ~FSNOTIFY_MARK_FLAG_OBJECT_PINNED; > + iput(mark->i.inode); > + mark->i.inode = NULL; > + mark->flags &= ~FSNOTIFY_MARK_FLAG_INODE; > + } > + > return ret; > } > > diff --git a/fs/notify/mark.c b/fs/notify/mark.c > index 11cfedc..422de6e 100644 > --- a/fs/notify/mark.c > +++ b/fs/notify/mark.c > @@ -219,8 +219,6 @@ int fsnotify_add_mark_locked(struct fsnotify_mark *mark, > BUG(); > if (ret) > goto err; > - /* this will pin the object if appropriate */ > - fsnotify_set_mark_mask_locked(mark, mark->mask); > > spin_unlock(&mark->lock); > > diff --git a/fs/notify/vfsmount_mark.c b/fs/notify/vfsmount_mark.c > index 04f5929..c43c310 100644 > --- a/fs/notify/vfsmount_mark.c > +++ b/fs/notify/vfsmount_mark.c > @@ -145,13 +145,11 @@ int fsnotify_add_vfsmount_mark(struct fsnotify_mark *mark, > int ret = 0; > > mark->flags |= FSNOTIFY_MARK_FLAG_VFSMOUNT; > + mark->m.mnt = mnt; > > assert_spin_locked(&mark->lock); > > spin_lock(&mnt->mnt_root->d_lock); > - > - mark->m.mnt = mnt; > - > /* is mark the first mark? */ > if (hlist_empty(&mnt->mnt_fsnotify_marks)) { > hlist_add_head_rcu(&mark->m.m_list, &mnt->mnt_fsnotify_marks); > @@ -185,5 +183,10 @@ out: > fsnotify_recalc_vfsmount_mask_locked(mnt); > spin_unlock(&mnt->mnt_root->d_lock); > > + if (ret) { > + mark->m.mnt = NULL; > + mark->flags &= ~FSNOTIFY_MARK_FLAG_VFSMOUNT; > + } > + > return ret; > } -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html