[lkml]   [2011]   [Mar]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH 1/5] fsnotify: grab inode ref in add_inode_mark() instead of add_mark()
On Tue, 2011-03-01 at 19:30 +0100, Lino Sanfilippo wrote:
> 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->i.inode = inode;
> + if (mark->mask) { /* only pin if mask is set */
> + igrab(inode);
> + }
> Should i just fix it and resend the patches? Or do you have any other
> concerns?

No, but it's a rather serious concern to me. You need to make sure that
mark->i.inode is actually valid before you use it and cannot disappear
underneath you. I haven't relooked at your code (and clearly will after
you fix) but what I'm most concerned about is some place where we are
trying to delete a mark, either explicitly or by group, and then race
with the inode being evicted from cache. The inode being evicted from
cache is going to eventually hit the destroy_by_inode code. When that
function returns we cannot use mark->i.inode any more. In today's code
we prevent this situation by never dereferencing or using the
mark->i.inode outside of mark->lock. You're going to have to remember
that after a 'destroy_by_inode' call you can't ever use the inode


 \ /
  Last update: 2011-03-01 23:17    [W:0.112 / U:7.340 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site