Re: [PATCH] 2.6.14 patch for supporting madvise(MADV_REMOVE)

[Badari Pulavarty <pbadari@us.ibm.com>]

On Fri, 2005-11-11 at 16:25 -0800, Andrew Morton wrote:
> Badari Pulavarty <pbadari@us.ibm.com> wrote:
> >
> > +/*
> >  + * Application wants to free up the pages and associated backing store. 
> >  + * This is effectively punching a hole into the middle of a file.
> >  + *
> >  + * NOTE: Currently, only shmfs/tmpfs is supported for this operation.
> >  + * Other filesystems return -ENOSYS.
> >  + */
> >  +static long madvise_remove(struct vm_area_struct * vma,
> >  +			     unsigned long start, unsigned long end)
> >  +{
> >  +	struct address_space *mapping;
> >  +        loff_t offset, endoff;
> >  +
> >  +	if (vma->vm_flags & (VM_LOCKED|VM_NONLINEAR|VM_HUGETLB)) 
> >  +		return -EINVAL;
> >  +
> >  +	if (!vma->vm_file || !vma->vm_file->f_mapping 
> >  +		|| !vma->vm_file->f_mapping->host) {
> >  +			return -EINVAL;
> >  +	}
> >  +
> >  +	mapping = vma->vm_file->f_mapping;
> >  +	if (mapping == &swapper_space) {
> >  +		return -EINVAL;
> >  +	}
> >  +
> >  +	offset = (loff_t)(start - vma->vm_start) 
> >  +			+ (vma->vm_pgoff << PAGE_SHIFT);
> >  +	endoff = (loff_t)(end - vma->vm_start - 1) 
> >  +			+ (vma->vm_pgoff << PAGE_SHIFT);
> >  +	return  vmtruncate_range(mapping->host, offset, endoff);
> >  +}
> >  +
> 
> I'm suspecting you tested this on a 64-bit machine, yes?  On 32-bit that
> vm_pgoff shift is going to overflow.  

Yes. I have moved to all 64-bit (amd64, em64t, ppc64) machines. My bad.

> 
> Fixes-thus-far below.   Please rerun all tests on x86?
> 

I will verify. Thanks.

> Why does madvise_remove() have an explicit check for swapper_space?

I really don't remember (I yanked code from some other kernel routine
vmtruncate()). If you think its unnecessary, I can take it out.

> In your testing, how are you determining that the code is successfully
> removing the correct number of pages, from the correct file offset?

I verified with test programs, added debug printk + looked through live
"crash" session + verified with UML testcases.

> 
> diff -puN mm/madvise.c~madvise-remove-remove-pages-from-tmpfs-shm-backing-store-tidy mm/madvise.c
> --- devel/mm/madvise.c~madvise-remove-remove-pages-from-tmpfs-shm-backing-store-tidy	2005-11-11 16:12:43.000000000 -0800
> +++ devel-akpm/mm/madvise.c	2005-11-11 16:16:50.000000000 -0800
> @@ -147,8 +147,8 @@ static long madvise_dontneed(struct vm_a
>   * NOTE: Currently, only shmfs/tmpfs is supported for this operation.
>   * Other filesystems return -ENOSYS.
>   */
> -static long madvise_remove(struct vm_area_struct * vma,
> -			     unsigned long start, unsigned long end)
> +static long madvise_remove(struct vm_area_struct *vma,
> +				unsigned long start, unsigned long end)
>  {
>  	struct address_space *mapping;
>          loff_t offset, endoff;
> @@ -162,14 +162,13 @@ static long madvise_remove(struct vm_are
>  	}
>  
>  	mapping = vma->vm_file->f_mapping;
> -	if (mapping == &swapper_space) {
> +	if (mapping == &swapper_space)
>  		return -EINVAL;
> -	}
>  
>  	offset = (loff_t)(start - vma->vm_start)
> -			+ (vma->vm_pgoff << PAGE_SHIFT);
> +			+ ((loff_t)vma->vm_pgoff << PAGE_SHIFT);
>  	endoff = (loff_t)(end - vma->vm_start - 1)
> -			+ (vma->vm_pgoff << PAGE_SHIFT);
> +			+ ((loff_t)vma->vm_pgoff << PAGE_SHIFT);
>  	return  vmtruncate_range(mapping->host, offset, endoff);
>  }
>  
> diff -puN mm/memory.c~madvise-remove-remove-pages-from-tmpfs-shm-backing-store-tidy mm/memory.c
> --- devel/mm/memory.c~madvise-remove-remove-pages-from-tmpfs-shm-backing-store-tidy	2005-11-11 16:16:54.000000000 -0800
> +++ devel-akpm/mm/memory.c	2005-11-11 16:17:59.000000000 -0800
> @@ -1608,10 +1608,9 @@ out_big:
>  out_busy:
>  	return -ETXTBSY;
>  }
> -
>  EXPORT_SYMBOL(vmtruncate);
>  
> -int vmtruncate_range(struct inode * inode, loff_t offset, loff_t end)
> +int vmtruncate_range(struct inode *inode, loff_t offset, loff_t end)
>  {
>  	struct address_space *mapping = inode->i_mapping;
>  
> @@ -1634,7 +1633,6 @@ int vmtruncate_range(struct inode * inod
>  
>  	return 0;
>  }
> -
>  EXPORT_SYMBOL(vmtruncate_range);
>  
>  /* 
> _
> 
>