| Message ID | 20201210063905.75727-3-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: add read-only support for subpage sector size | expand |
On 10.12.20 г. 8:38 ч., Qu Wenruo wrote: > The refactor involves the following modifications: > - iosize alignment > In fact we don't really need to manually do alignment at all. > All extent maps should already be aligned, thus basic ASSERT() check > would be enough. > > - redundant variables > We have extra variable like blocksize/pg_offset/end. > They are all unnecessary. > > @blocksize can be replaced by sectorsize size directly, and it's only > used to verify the em start/size is aligned. > > @pg_offset can be easily calculated using @cur and page_offset(page). > > @end is just assigned to @page_end and never modified, use @page_end > to replace it. > > - remove some BUG_ON()s > The BUG_ON()s are for extent map, which we have tree-checker to check > on-disk extent data item and runtime check. > ASSERT() should be enough. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/extent_io.c | 37 +++++++++++++++++-------------------- > 1 file changed, 17 insertions(+), 20 deletions(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 2650e8720394..612fe60b367e 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -3515,17 +3515,14 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode, > unsigned long nr_written, > int *nr_ret) > { > + struct btrfs_fs_info *fs_info = inode->root->fs_info; > struct extent_io_tree *tree = &inode->io_tree; > u64 start = page_offset(page); > u64 page_end = start + PAGE_SIZE - 1; nit: page_end should be renamed to end because start now points to the logical logical byte offset, i.e having "page" in the name is misleading. <snip>
On 2020/12/10 下午8:12, Nikolay Borisov wrote: > > > On 10.12.20 г. 8:38 ч., Qu Wenruo wrote: >> The refactor involves the following modifications: >> - iosize alignment >> In fact we don't really need to manually do alignment at all. >> All extent maps should already be aligned, thus basic ASSERT() check >> would be enough. >> >> - redundant variables >> We have extra variable like blocksize/pg_offset/end. >> They are all unnecessary. >> >> @blocksize can be replaced by sectorsize size directly, and it's only >> used to verify the em start/size is aligned. >> >> @pg_offset can be easily calculated using @cur and page_offset(page). >> >> @end is just assigned to @page_end and never modified, use @page_end >> to replace it. >> >> - remove some BUG_ON()s >> The BUG_ON()s are for extent map, which we have tree-checker to check >> on-disk extent data item and runtime check. >> ASSERT() should be enough. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/extent_io.c | 37 +++++++++++++++++-------------------- >> 1 file changed, 17 insertions(+), 20 deletions(-) >> >> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c >> index 2650e8720394..612fe60b367e 100644 >> --- a/fs/btrfs/extent_io.c >> +++ b/fs/btrfs/extent_io.c >> @@ -3515,17 +3515,14 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode, >> unsigned long nr_written, >> int *nr_ret) >> { >> + struct btrfs_fs_info *fs_info = inode->root->fs_info; >> struct extent_io_tree *tree = &inode->io_tree; >> u64 start = page_offset(page); >> u64 page_end = start + PAGE_SIZE - 1; > > nit: page_end should be renamed to end because start now points to the > logical logical byte offset, i.e having "page" in the name is misleading. But page_offset() along page_end is still logical bytenr, thus I didn't see much confusion here... Thanks, Qu > > <snip> >
On 10.12.20 г. 14:53 ч., Qu Wenruo wrote: > > > On 2020/12/10 下午8:12, Nikolay Borisov wrote: >> >> >> On 10.12.20 г. 8:38 ч., Qu Wenruo wrote: >>> The refactor involves the following modifications: >>> - iosize alignment >>> In fact we don't really need to manually do alignment at all. >>> All extent maps should already be aligned, thus basic ASSERT() check >>> would be enough. >>> >>> - redundant variables >>> We have extra variable like blocksize/pg_offset/end. >>> They are all unnecessary. >>> >>> @blocksize can be replaced by sectorsize size directly, and it's only >>> used to verify the em start/size is aligned. >>> >>> @pg_offset can be easily calculated using @cur and page_offset(page). >>> >>> @end is just assigned to @page_end and never modified, use @page_end >>> to replace it. >>> >>> - remove some BUG_ON()s >>> The BUG_ON()s are for extent map, which we have tree-checker to check >>> on-disk extent data item and runtime check. >>> ASSERT() should be enough. >>> >>> Signed-off-by: Qu Wenruo <wqu@suse.com> >>> --- >>> fs/btrfs/extent_io.c | 37 +++++++++++++++++-------------------- >>> 1 file changed, 17 insertions(+), 20 deletions(-) >>> >>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c >>> index 2650e8720394..612fe60b367e 100644 >>> --- a/fs/btrfs/extent_io.c >>> +++ b/fs/btrfs/extent_io.c >>> @@ -3515,17 +3515,14 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode, >>> unsigned long nr_written, >>> int *nr_ret) >>> { >>> + struct btrfs_fs_info *fs_info = inode->root->fs_info; >>> struct extent_io_tree *tree = &inode->io_tree; >>> u64 start = page_offset(page); >>> u64 page_end = start + PAGE_SIZE - 1; >> >> nit: page_end should be renamed to end because start now points to the >> logical logical byte offset, i.e having "page" in the name is misleading. > > But page_offset() along page_end is still logical bytenr, thus I didn't > see much confusion here... Exactly page_offset converts the page index to a logical bytenr and that point we no longer care about the physical page but the logical range which is PAGE_SIZE. 'page_end' is really some logical affset which spans a PAGE_SIZE region > > Thanks, > Qu >> >> <snip> >> >
On 12/10/20 1:38 AM, Qu Wenruo wrote: > The refactor involves the following modifications: > - iosize alignment > In fact we don't really need to manually do alignment at all. > All extent maps should already be aligned, thus basic ASSERT() check > would be enough. > > - redundant variables > We have extra variable like blocksize/pg_offset/end. > They are all unnecessary. > > @blocksize can be replaced by sectorsize size directly, and it's only > used to verify the em start/size is aligned. > > @pg_offset can be easily calculated using @cur and page_offset(page). > > @end is just assigned to @page_end and never modified, use @page_end > to replace it. > > - remove some BUG_ON()s > The BUG_ON()s are for extent map, which we have tree-checker to check > on-disk extent data item and runtime check. > ASSERT() should be enough. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/extent_io.c | 37 +++++++++++++++++-------------------- > 1 file changed, 17 insertions(+), 20 deletions(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 2650e8720394..612fe60b367e 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -3515,17 +3515,14 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode, > unsigned long nr_written, > int *nr_ret) > { > + struct btrfs_fs_info *fs_info = inode->root->fs_info; > struct extent_io_tree *tree = &inode->io_tree; > u64 start = page_offset(page); > u64 page_end = start + PAGE_SIZE - 1; > - u64 end; > u64 cur = start; > u64 extent_offset; > u64 block_start; > - u64 iosize; > struct extent_map *em; > - size_t pg_offset = 0; > - size_t blocksize; > int ret = 0; > int nr = 0; > const unsigned int write_flags = wbc_to_write_flags(wbc); > @@ -3546,19 +3543,17 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode, > */ > update_nr_written(wbc, nr_written + 1); > > - end = page_end; > - blocksize = inode->vfs_inode.i_sb->s_blocksize; > - > - while (cur <= end) { > + while (cur <= page_end) { > u64 disk_bytenr; > u64 em_end; > + u32 iosize; > > if (cur >= i_size) { > btrfs_writepage_endio_finish_ordered(page, cur, > page_end, 1); > break; > } > - em = btrfs_get_extent(inode, NULL, 0, cur, end - cur + 1); > + em = btrfs_get_extent(inode, NULL, 0, cur, page_end - cur + 1); > if (IS_ERR_OR_NULL(em)) { > SetPageError(page); > ret = PTR_ERR_OR_ZERO(em); > @@ -3567,16 +3562,20 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode, > > extent_offset = cur - em->start; > em_end = extent_map_end(em); > - BUG_ON(em_end <= cur); > - BUG_ON(end < cur); > - iosize = min(em_end - cur, end - cur + 1); > - iosize = ALIGN(iosize, blocksize); > - disk_bytenr = em->block_start + extent_offset; > + ASSERT(cur <= em_end); > + ASSERT(cur < page_end); > + ASSERT(IS_ALIGNED(em->start, fs_info->sectorsize)); > + ASSERT(IS_ALIGNED(em->len, fs_info->sectorsize)); > block_start = em->block_start; > compressed = test_bit(EXTENT_FLAG_COMPRESSED, &em->flags); > + disk_bytenr = em->block_start + extent_offset; > + > + /* Note that em_end from extent_map_end() is exclusive */ > + iosize = min(em_end, page_end + 1) - cur; > free_extent_map(em); > em = NULL; > > + Random extra whitespace. Once you fix you can add Reviewed-by: Josef Bacik <josef@toxicpanda.com> Thanks, Josef
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 2650e8720394..612fe60b367e 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -3515,17 +3515,14 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode, unsigned long nr_written, int *nr_ret) { + struct btrfs_fs_info *fs_info = inode->root->fs_info; struct extent_io_tree *tree = &inode->io_tree; u64 start = page_offset(page); u64 page_end = start + PAGE_SIZE - 1; - u64 end; u64 cur = start; u64 extent_offset; u64 block_start; - u64 iosize; struct extent_map *em; - size_t pg_offset = 0; - size_t blocksize; int ret = 0; int nr = 0; const unsigned int write_flags = wbc_to_write_flags(wbc); @@ -3546,19 +3543,17 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode, */ update_nr_written(wbc, nr_written + 1); - end = page_end; - blocksize = inode->vfs_inode.i_sb->s_blocksize; - - while (cur <= end) { + while (cur <= page_end) { u64 disk_bytenr; u64 em_end; + u32 iosize; if (cur >= i_size) { btrfs_writepage_endio_finish_ordered(page, cur, page_end, 1); break; } - em = btrfs_get_extent(inode, NULL, 0, cur, end - cur + 1); + em = btrfs_get_extent(inode, NULL, 0, cur, page_end - cur + 1); if (IS_ERR_OR_NULL(em)) { SetPageError(page); ret = PTR_ERR_OR_ZERO(em); @@ -3567,16 +3562,20 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode, extent_offset = cur - em->start; em_end = extent_map_end(em); - BUG_ON(em_end <= cur); - BUG_ON(end < cur); - iosize = min(em_end - cur, end - cur + 1); - iosize = ALIGN(iosize, blocksize); - disk_bytenr = em->block_start + extent_offset; + ASSERT(cur <= em_end); + ASSERT(cur < page_end); + ASSERT(IS_ALIGNED(em->start, fs_info->sectorsize)); + ASSERT(IS_ALIGNED(em->len, fs_info->sectorsize)); block_start = em->block_start; compressed = test_bit(EXTENT_FLAG_COMPRESSED, &em->flags); + disk_bytenr = em->block_start + extent_offset; + + /* Note that em_end from extent_map_end() is exclusive */ + iosize = min(em_end, page_end + 1) - cur; free_extent_map(em); em = NULL; + /* * compressed and inline extents are written through other * paths in the FS @@ -3589,7 +3588,6 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode, btrfs_writepage_endio_finish_ordered(page, cur, cur + iosize - 1, 1); cur += iosize; - pg_offset += iosize; continue; } @@ -3597,12 +3595,12 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode, if (!PageWriteback(page)) { btrfs_err(inode->root->fs_info, "page %lu not writeback, cur %llu end %llu", - page->index, cur, end); + page->index, cur, page_end); } ret = submit_extent_page(REQ_OP_WRITE | write_flags, wbc, - page, disk_bytenr, iosize, pg_offset, - &epd->bio, + page, disk_bytenr, iosize, + cur - page_offset(page), &epd->bio, end_bio_extent_writepage, 0, 0, 0, false); if (ret) { @@ -3611,8 +3609,7 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode, end_page_writeback(page); } - cur = cur + iosize; - pg_offset += iosize; + cur += iosize; nr++; } *nr_ret = nr;
The refactor involves the following modifications: - iosize alignment In fact we don't really need to manually do alignment at all. All extent maps should already be aligned, thus basic ASSERT() check would be enough. - redundant variables We have extra variable like blocksize/pg_offset/end. They are all unnecessary. @blocksize can be replaced by sectorsize size directly, and it's only used to verify the em start/size is aligned. @pg_offset can be easily calculated using @cur and page_offset(page). @end is just assigned to @page_end and never modified, use @page_end to replace it. - remove some BUG_ON()s The BUG_ON()s are for extent map, which we have tree-checker to check on-disk extent data item and runtime check. ASSERT() should be enough. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/extent_io.c | 37 +++++++++++++++++-------------------- 1 file changed, 17 insertions(+), 20 deletions(-)