| Message ID | 1488330280-22678-1-git-send-email-bo.li.liu@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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) Thanks, Qu > - end_pos = round_up(pos + count, > - fs_info->sectorsize); > err = btrfs_cont_expand(inode, oldsize, end_pos); > if (err) { > inode_unlock(inode); > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
It seems that my previous mail doesn't reach mail list. So send again. 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) Thanks, Qu > - end_pos = round_up(pos + count, > - fs_info->sectorsize); > err = btrfs_cont_expand(inode, oldsize, end_pos); > if (err) { > inode_unlock(inode); > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 */ - end_pos = round_up(pos + count, - fs_info->sectorsize); err = btrfs_cont_expand(inode, oldsize, end_pos); if (err) { inode_unlock(inode);
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> --- fs/btrfs/file.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)