Re: [PATCH V5 11/13] Btrfs: Clean pte corresponding to page straddling i_size

[Chandan Rajendra <chandan@linux.vnet.ibm.com>]

On Thursday 01 Oct 2015 10:57:52 Josef Bacik wrote:
> On 09/30/2015 06:28 AM, Chandan Rajendra wrote:
> > When extending a file by either "truncate up" or by writing beyond i_size,
> > the page which had i_size needs to be marked "read only" so that future
> > writes to the page via mmap interface causes btrfs_page_mkwrite() to be
> > invoked. If not, a write performed after extending the file via the mmap
> > interface will find the page to be writaeable and continue writing to the
> > page without invoking btrfs_page_mkwrite() i.e. we end up writing to a
> > file without reserving disk space.
> > 
> > Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>
> > ---
> > 
> >   fs/btrfs/file.c  | 12 ++++++++++--
> >   fs/btrfs/inode.c |  2 +-
> >   2 files changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> > index 360d56d..5715e29 100644
> > --- a/fs/btrfs/file.c
> > +++ b/fs/btrfs/file.c
> > @@ -1757,6 +1757,8 @@ static ssize_t btrfs_file_write_iter(struct kiocb
> > *iocb,> 
> >   	ssize_t err;
> >   	loff_t pos;
> >   	size_t count;
> > 
> > +	loff_t oldsize;
> > +	int clean_page = 0;
> > 
> >   	mutex_lock(&inode->i_mutex);
> >   	err = generic_write_checks(iocb, from);
> > 
> > @@ -1795,14 +1797,17 @@ static ssize_t btrfs_file_write_iter(struct kiocb
> > *iocb,> 
> >   	pos = iocb->ki_pos;
> >   	count = iov_iter_count(from);
> >   	start_pos = round_down(pos, root->sectorsize);
> > 
> > -	if (start_pos > i_size_read(inode)) {
> > +	oldsize = i_size_read(inode);
> > +	if (start_pos > oldsize) {
> > 
> >   		/* Expand hole size to cover write data, preventing empty gap 
*/
> >   		end_pos = round_up(pos + count, root->sectorsize);
> > 
> > -		err = btrfs_cont_expand(inode, i_size_read(inode), end_pos);
> > +		err = btrfs_cont_expand(inode, oldsize, end_pos);
> > 
> >   		if (err) {
> >   		
> >   			mutex_unlock(&inode->i_mutex);
> >   			goto out;
> >   		
> >   		}
> > 
> > +		if (start_pos > round_up(oldsize, root->sectorsize))
> > +			clean_page = 1;
> > 
> >   	}
> >   	
> >   	if (sync)
> > 
> > @@ -1814,6 +1819,9 @@ static ssize_t btrfs_file_write_iter(struct kiocb
> > *iocb,> 
> >   		num_written = __btrfs_buffered_write(file, from, pos);
> >   		if (num_written > 0)
> >   		
> >   			iocb->ki_pos = pos + num_written;
> > 
> > +		if (clean_page)
> > +			pagecache_isize_extended(inode, oldsize,
> > +						i_size_read(inode));
> > 
> >   	}
> >   	
> >   	mutex_unlock(&inode->i_mutex);
> > 
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index c937357..f31da87 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -4853,7 +4853,6 @@ static int btrfs_setsize(struct inode *inode, struct
> > iattr *attr)> 
> >   	}
> >   	
> >   	if (newsize > oldsize) {
> > 
> > -		truncate_pagecache(inode, newsize);
> 
> So I don't understand why we are dropping this bit here, could you
> explain?  Otherwise the patch looks fine to me.  Thanks,
>
Josef, As per our previous discussion on IRC we found that the
"truncate_pagecache(inode, newsize)" statement to be a relic of the
past. During the test runs, I haven't seen any sort of failure caused by the
removal of this statement.

-- 
chandan