Re: Race between vmtruncate and mapped areas?

[Andrea Arcangeli]

On Wed, May 14, 2003 at 10:36:23PM -0400, Rik van Riel wrote:
> On Thu, 15 May 2003, Andrea Arcangeli wrote:
> 
> > --- x/include/linux/fs.h.~1~	2003-05-14 23:26:19.000000000 +0200
> > +++ x/include/linux/fs.h	2003-05-15 02:35:57.000000000 +0200
> > @@ -421,6 +421,8 @@ struct address_space {
> >  	struct vm_area_struct	*i_mmap;	/* list of private mappings */
> >  	struct vm_area_struct	*i_mmap_shared; /* list of shared mappings */
> >  	spinlock_t		i_shared_lock;  /* and spinlock protecting it */
> > +	int			truncate_sequence1; /* serialize ->nopage against truncate */
> > +	int			truncate_sequence2; /* serialize ->nopage against truncate */
> 
> How about calling them truncate_start and truncate_end ?

Normally we use start/end for ranges, this is not a range, so I wouldn't
suggest it, but I don't care about names.

> > --- x/mm/vmscan.c.~1~	2003-05-14 23:26:12.000000000 +0200
> > +++ x/mm/vmscan.c	2003-05-15 00:22:57.000000000 +0200
> > @@ -165,11 +165,10 @@ drop_pte:
> >  		goto drop_pte;
> >  
> >  	/*
> > -	 * Anonymous buffercache pages can be left behind by
> > +	 * Anonymous buffercache pages can't be left behind by
> >  	 * concurrent truncate and pagefault.
> >  	 */
> > -	if (page->buffers)
> > -		goto preserve;
> > +	BUG_ON(page->buffers);
> 
> I wonder if there is nothing else that can leave behind
> buffers in this way.

that's why I left the BUG_ON, if there's anything else I want to know,
there shouldn't be anything else as the comment also suggest. I recall
when we discussed this single check with Andrew and that was the only
reason we left it AFIK.

> > +	mb(); /* spin_lock has inclusive semantics */
> > +	if (unlikely(truncate_sequence != mapping->truncate_sequence1)) {
> > +		struct inode *inode;
> 
> This code looks like it should work, but IMHO it is very subtle
> so it should really get some documentation.

Andrea