Re: [PATCH 1/2] Btrfs: fix unexpected file hole after disk errors

[Liu Bo <bo.li.liu@oracle.com>]

On Wed, Mar 01, 2017 at 10:44:53AM +0800, Qu Wenruo wrote:
> 
> 
> At 03/01/2017 09:04 AM, Liu Bo wrote:
> > Btrfs creates hole extents to cover any unwritten section right before
> > doing buffer writes after commit 3ac0d7b96a26 ("btrfs: Change the expanding
> > write sequence to fix snapshot related bug.").
> > 
> > However, that takes the start position of the buffered write to compare
> > against the current EOF, hole extents would be created only if (EOF <
> > start).
> > 
> > If the EOF is at the middle of the buffered write, no hole extents will be
> > created and a file hole without a hole extent is left in this file.
> > 
> > This bug was revealed by generic/019 in fstests.  'fsstress' in this test
> > may create the above situation and the test then fails all requests
> > including writes, so the buffer write which is supposed to cover the
> > hole (without the hole extent) couldn't make it on disk.  Running fsck
> > against such btrfs ends up with detecting file extent holes.
> > 
> > Things could be more serious, some stale data would be exposed to
> > userspace if files with this kind of hole are truncated to a position of
> > the hole, because the on-disk inode size is beyond the last extent in the
> > file.
> > 
> > This fixes the bug by comparing the end position against the EOF.
> > 
> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> 
> Patch looks good to me.
> Reviewed-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> 
> > ---
> >  fs/btrfs/file.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> > index b5c5da2..0be837b 100644
> > --- a/fs/btrfs/file.c
> > +++ b/fs/btrfs/file.c
> > @@ -1861,11 +1861,10 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
> >  	pos = iocb->ki_pos;
> >  	count = iov_iter_count(from);
> >  	start_pos = round_down(pos, fs_info->sectorsize);
> > +	end_pos = round_up(pos + count, fs_info->sectorsize);
> >  	oldsize = i_size_read(inode);
> > -	if (start_pos > oldsize) {
> > +	if (end_pos > oldsize) {
> >  		/* Expand hole size to cover write data, preventing empty gap */
> 
> The comment still makes sense here, but it could be better to explain why to
> insert the hole to cover the whole write range (in case write fails)
>

Sounds good, I'll update.

Thanks,

-liubo

> Thanks,
> Qu
> 
> > -		end_pos = round_up(pos + count,
> > -				   fs_info->sectorsize);
> >  		err = btrfs_cont_expand(inode, oldsize, end_pos);
> >  		if (err) {
> >  			inode_unlock(inode);
> > 
> 
>