Re: [PATCH] mm/fadvise: discard partial pages iff endbyte is also eof

[Mel Gorman]

On Wed, Jan 03, 2018 at 04:17:53PM -0800, Andrew Morton wrote:
> : invalidate_mapping_pages() takes start/end, but fadvise is currently passing
> : it start/len.
> : 
> : 
> : 
> :  mm/fadvise.c |    8 ++++++--
> :  1 files changed, 6 insertions(+), 2 deletions(-)
> : 
> : diff -puN mm/fadvise.c~fadvise-fix mm/fadvise.c
> : --- 25/mm/fadvise.c~fadvise-fix	2003-08-14 18:16:12.000000000 -0700
> : +++ 25-akpm/mm/fadvise.c	2003-08-14 18:16:12.000000000 -0700
> : @@ -26,6 +26,8 @@ long sys_fadvise64(int fd, loff_t offset
> :  	struct inode *inode;
> :  	struct address_space *mapping;
> :  	struct backing_dev_info *bdi;
> : +	pgoff_t start_index;
> : +	pgoff_t end_index;
> :  	int ret = 0;
> :  
> :  	if (!file)
> : @@ -65,8 +67,10 @@ long sys_fadvise64(int fd, loff_t offset
> :  	case POSIX_FADV_DONTNEED:
> :  		if (!bdi_write_congested(mapping->backing_dev_info))
> :  			filemap_flush(mapping);
> : -		invalidate_mapping_pages(mapping, offset >> PAGE_CACHE_SHIFT,
> : -				(len >> PAGE_CACHE_SHIFT) + 1);
> : +		start_index = offset >> PAGE_CACHE_SHIFT;
> : +		end_index = (offset + len + PAGE_CACHE_SIZE - 1) >>
> : +						PAGE_CACHE_SHIFT;
> : +		invalidate_mapping_pages(mapping, start_index, end_index);
> :  		break;
> :  	default:
> :  		ret = -EINVAL;
> : 
> 
> So I'm not sure that the whole "don't discard partial pages" thing is
> well-founded and I see no reason why we cannot alter it.
> 
> So, thinking caps on: why not just discard them?  After all, that's
> what userspace asked us to do.
> 

We could, it just means that any application that accidentally discards
hot data due to an unaligned fadvise will incur more IO. We've no idea
how many, if any applications, do this.

-- 
Mel Gorman
SUSE Labs