Re: [PATCH 1/5] fsnotify: grab inode ref in add_inode_mark()instead of add_mark()

[Lino Sanfilippo]

On Fri, Feb 25, 2011 at 01:36:35PM -0500, Eric Paris wrote:
> 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.

Youre right, the inode is now also pinned if there is no mask set. This is
a change i did not on purpose. Whether the inode is pinned or not does not 
affect the original purpose of the patch series, which was the decoupling 
of the mark lock and the fsobject lock. I simply forgot to check the mask.
I could replace that part with something like

-       mark->i.inode = igrab(inode);
-       mark->flags |= FSNOTIFY_MARK_FLAG_OBJECT_PINNED;
+       mark->i.inode = inode;
+       if (mark->mask) {       /* only pin if mask is set */   
+               igrab(inode);
+               mark->flags |= FSNOTIFY_MARK_FLAG_OBJECT_PINNED;
+       }

Should i just fix it and resend the patches? Or do you have any other
concerns?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/