| Message ID | 20200908075230.86856-14-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: add read-only support for subpage sector size | expand |
On 8.09.20 г. 10:52 ч., Qu Wenruo wrote: > If we're reading partial page, btrfs will warn about this as our > read/write are always done in sector size, which equals page size. > > But for the incoming subpage RO support, our data read is only aligned > to sectorsize, which can be smaller than page size. > > Thus here we change the warning condition to check it against > sectorsize, thus the behavior is not changed for regular sectorsize == > PAGE_SIZE case, while won't report error for subpage read. > > Also, pass the proper start/end with bv_offset for check_data_csum() to > handle. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/extent_io.c | 19 ++++++++++++------- > 1 file changed, 12 insertions(+), 7 deletions(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 81e43d99feda..a83b63ecc5f8 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -2819,6 +2819,7 @@ static void end_bio_extent_readpage(struct bio *bio) > struct page *page = bvec->bv_page; > struct inode *inode = page->mapping->host; > struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb); > + u32 sectorsize = fs_info->sectorsize; > bool data_inode = btrfs_ino(BTRFS_I(inode)) > != BTRFS_BTREE_INODE_OBJECTID; > > @@ -2829,13 +2830,17 @@ static void end_bio_extent_readpage(struct bio *bio) > tree = &BTRFS_I(inode)->io_tree; > failure_tree = &BTRFS_I(inode)->io_failure_tree; > > - /* We always issue full-page reads, but if some block > + /* > + * We always issue full-sector reads, but if some block > * in a page fails to read, blk_update_request() will > * advance bv_offset and adjust bv_len to compensate. > - * Print a warning for nonzero offsets, and an error > - * if they don't add up to a full page. */ > - if (bvec->bv_offset || bvec->bv_len != PAGE_SIZE) { > - if (bvec->bv_offset + bvec->bv_len != PAGE_SIZE) > + * Print a warning for unaligned offsets, and an error > + * if they don't add up to a full sector. > + */ > + if (!IS_ALIGNED(bvec->bv_offset, sectorsize) || > + !IS_ALIGNED(bvec->bv_offset + bvec->bv_len, sectorsize)) { > + if (!IS_ALIGNED(bvec->bv_offset + bvec->bv_len, > + sectorsize)) Duplicated check ... > btrfs_err(fs_info, > "partial page read in btrfs with offset %u and length %u", > bvec->bv_offset, bvec->bv_len); > @@ -2845,8 +2850,8 @@ static void end_bio_extent_readpage(struct bio *bio) > bvec->bv_offset, bvec->bv_len); > } > > - start = page_offset(page); > - end = start + bvec->bv_offset + bvec->bv_len - 1; > + start = page_offset(page) + bvec->bv_offset; > + end = start + bvec->bv_len - 1; nit: 'start' and 'end' must really be renamed - to file_offset and file_end because they represent values in the logical namespace of the file. And given the context they are used i.e endio handler where we also deal with extent starts and physical offsets such a rename is long over due. Perhaps you can create a separate patch when you are resending the series alternatively I'll make a sweep across those low-level functions to clean that up. > len = bvec->bv_len; > > mirror = io_bio->mirror_num; >
On 2020/9/11 下午9:55, Nikolay Borisov wrote: > > > On 8.09.20 г. 10:52 ч., Qu Wenruo wrote: >> If we're reading partial page, btrfs will warn about this as our >> read/write are always done in sector size, which equals page size. >> >> But for the incoming subpage RO support, our data read is only aligned >> to sectorsize, which can be smaller than page size. >> >> Thus here we change the warning condition to check it against >> sectorsize, thus the behavior is not changed for regular sectorsize == >> PAGE_SIZE case, while won't report error for subpage read. >> >> Also, pass the proper start/end with bv_offset for check_data_csum() to >> handle. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/extent_io.c | 19 ++++++++++++------- >> 1 file changed, 12 insertions(+), 7 deletions(-) >> >> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c >> index 81e43d99feda..a83b63ecc5f8 100644 >> --- a/fs/btrfs/extent_io.c >> +++ b/fs/btrfs/extent_io.c >> @@ -2819,6 +2819,7 @@ static void end_bio_extent_readpage(struct bio *bio) >> struct page *page = bvec->bv_page; >> struct inode *inode = page->mapping->host; >> struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb); >> + u32 sectorsize = fs_info->sectorsize; >> bool data_inode = btrfs_ino(BTRFS_I(inode)) >> != BTRFS_BTREE_INODE_OBJECTID; >> >> @@ -2829,13 +2830,17 @@ static void end_bio_extent_readpage(struct bio *bio) >> tree = &BTRFS_I(inode)->io_tree; >> failure_tree = &BTRFS_I(inode)->io_failure_tree; >> >> - /* We always issue full-page reads, but if some block >> + /* >> + * We always issue full-sector reads, but if some block >> * in a page fails to read, blk_update_request() will >> * advance bv_offset and adjust bv_len to compensate. >> - * Print a warning for nonzero offsets, and an error >> - * if they don't add up to a full page. */ >> - if (bvec->bv_offset || bvec->bv_len != PAGE_SIZE) { >> - if (bvec->bv_offset + bvec->bv_len != PAGE_SIZE) >> + * Print a warning for unaligned offsets, and an error >> + * if they don't add up to a full sector. >> + */ >> + if (!IS_ALIGNED(bvec->bv_offset, sectorsize) || >> + !IS_ALIGNED(bvec->bv_offset + bvec->bv_len, sectorsize)) { >> + if (!IS_ALIGNED(bvec->bv_offset + bvec->bv_len, >> + sectorsize)) > > Duplicated check ... BTW, this is not duplicated, it's to distinguish two different error patterns... One for read request which doesn't end at sector boundary, and the other one for which doesn't start at sector boundary. > >> btrfs_err(fs_info, >> "partial page read in btrfs with offset %u and length %u", >> bvec->bv_offset, bvec->bv_len); >> @@ -2845,8 +2850,8 @@ static void end_bio_extent_readpage(struct bio *bio) >> bvec->bv_offset, bvec->bv_len); >> } >> >> - start = page_offset(page); >> - end = start + bvec->bv_offset + bvec->bv_len - 1; >> + start = page_offset(page) + bvec->bv_offset; >> + end = start + bvec->bv_len - 1; > > nit: 'start' and 'end' must really be renamed - to file_offset and > file_end because they represent values in the logical namespace of the > file. And given the context they are used i.e endio handler where we > also deal with extent starts and physical offsets such a rename is long > over due. Perhaps you can create a separate patch when you are > resending the series alternatively I'll make a sweep across those > low-level functions to clean that up. I guess we could do that in another patchset. The naming is really aweful, but there are tons of other similar situations across the code base. It may be a big batch of work to properly unify the naming. And the naming itself will take some time to mature. We have a lot of different terms which share the similar meanings but still slightly different: - bytenr btrfs logical bytenr - file_offset the offset inside a file And in this particular case, for btree inode, bytenr == file_offset, which may make things more complex. While for regualr file inodes, file_offset is different from the extent bytenr. So we really need to come out with a proper term table for this... Thanks, Qu > >> len = bvec->bv_len; >> >> mirror = io_bio->mirror_num; >> >
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 81e43d99feda..a83b63ecc5f8 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -2819,6 +2819,7 @@ static void end_bio_extent_readpage(struct bio *bio) struct page *page = bvec->bv_page; struct inode *inode = page->mapping->host; struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb); + u32 sectorsize = fs_info->sectorsize; bool data_inode = btrfs_ino(BTRFS_I(inode)) != BTRFS_BTREE_INODE_OBJECTID; @@ -2829,13 +2830,17 @@ static void end_bio_extent_readpage(struct bio *bio) tree = &BTRFS_I(inode)->io_tree; failure_tree = &BTRFS_I(inode)->io_failure_tree; - /* We always issue full-page reads, but if some block + /* + * We always issue full-sector reads, but if some block * in a page fails to read, blk_update_request() will * advance bv_offset and adjust bv_len to compensate. - * Print a warning for nonzero offsets, and an error - * if they don't add up to a full page. */ - if (bvec->bv_offset || bvec->bv_len != PAGE_SIZE) { - if (bvec->bv_offset + bvec->bv_len != PAGE_SIZE) + * Print a warning for unaligned offsets, and an error + * if they don't add up to a full sector. + */ + if (!IS_ALIGNED(bvec->bv_offset, sectorsize) || + !IS_ALIGNED(bvec->bv_offset + bvec->bv_len, sectorsize)) { + if (!IS_ALIGNED(bvec->bv_offset + bvec->bv_len, + sectorsize)) btrfs_err(fs_info, "partial page read in btrfs with offset %u and length %u", bvec->bv_offset, bvec->bv_len); @@ -2845,8 +2850,8 @@ static void end_bio_extent_readpage(struct bio *bio) bvec->bv_offset, bvec->bv_len); } - start = page_offset(page); - end = start + bvec->bv_offset + bvec->bv_len - 1; + start = page_offset(page) + bvec->bv_offset; + end = start + bvec->bv_len - 1; len = bvec->bv_len; mirror = io_bio->mirror_num;
If we're reading partial page, btrfs will warn about this as our read/write are always done in sector size, which equals page size. But for the incoming subpage RO support, our data read is only aligned to sectorsize, which can be smaller than page size. Thus here we change the warning condition to check it against sectorsize, thus the behavior is not changed for regular sectorsize == PAGE_SIZE case, while won't report error for subpage read. Also, pass the proper start/end with bv_offset for check_data_csum() to handle. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/extent_io.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)