RE: Hugepage regression

[Hugh Dickins <hugh@veritas.com>]

On Tue, 10 Oct 2006, Chen, Kenneth W wrote:
> Hugh Dickins wrote on Tuesday, October 10, 2006 1:10 PM
> > > can we reverse that order (call unmap_region
> > > and then nulls out vma->vmfile and fput)?
> > 
> > I'm pretty sure we cannot: the ordering is quite intentional, that if
> > a driver ->mmap failed, then it'd be wrong to call down to driver in
> > the unmap_region (if a driver is nicely behaved, that unmap_region
> > shouldn't be unnecessary; but some do rely on us clearing ptes there).

Looking at it again, my explanation seems wrong: I can't see any danger
of calling down to the _driver_ there (there's no remove_vma): rather,
it's __remove_shared_vm_struct we're avoiding by setting vm_file NULL.

> 
> Even not something like the following?  I believe you that nullifying
> vma->vm_file can not be done after unmap_region(),

Yet in your patch below, you do nullify vm_file _after_ unmap_region.

> I just want to make sure we are talking the same thing.

So I'm not sure if we are!

> It looks OK to me to defer the fput in the do_mmap_pgoff clean up path.

Yes, it would be quite okay to defer the fput until after the
unmap_region; but there's no point in making that change, is there?

Hugh (sorry, I was off sick for a couple of days)

> 
> 
> --- ./mm/mmap.c.orig	2006-10-10 15:58:17.000000000 -0700
> +++ ./mm/mmap.c	2006-10-10 15:59:02.000000000 -0700
> @@ -1159,11 +1159,12 @@ out:	
>  unmap_and_free_vma:
>  	if (correct_wcount)
>  		atomic_inc(&inode->i_writecount);
> -	vma->vm_file = NULL;
> -	fput(file);
>  
>  	/* Undo any partial mapping done by a device driver. */
>  	unmap_region(mm, vma, prev, vma->vm_start, vma->vm_end);
> +
> +	vma->vm_file = NULL;
> +	fput(file);
>  	charged = 0;
>  free_vma:
>  	kmem_cache_free(vm_area_cachep, vma);
>