| Message ID | 20201103133108.148112-18-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/32] btrfs: extent_io: remove the extent_start/extent_len for end_bio_extent_readpage() | expand |
On 3.11.20 г. 15:30 ч., Qu Wenruo wrote: > As a preparation for subpage sector size support (allowing filesystem > with sector size smaller than page size to be mounted) if the sector > size is smaller than page size, we don't allow tree block to be read if > it crosses 64K(*) boundary. > > The 64K is selected because: > - We are only going to support 64K page size for subpage for now > - 64K is also the max node size btrfs supports > > This ensures that, tree blocks are always contained in one page for a > system with 64K page size, which can greatly simplify the handling. > > Or we need to do complex multi-page handling for tree blocks. > > Currently the only way to create such tree blocks crossing 64K boundary > is by btrfs-convert, which will get fixed soon and doesn't get > wide-spread usage. So filesystems with subpage blocksize which have been created as a result of a convert operation would eventually fail to read some block am I correct in my understanding? If that is the case then can't we simply land subpage support in userspace tools _after_ the convert has been fixed and turn this check into an assert? > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/extent_io.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 30768e49cf47..30bbaeaa129a 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -5261,6 +5261,13 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info, > btrfs_err(fs_info, "bad tree block start %llu", start); > return ERR_PTR(-EINVAL); > } > + if (btrfs_is_subpage(fs_info) && round_down(start, PAGE_SIZE) != > + round_down(start + len - 1, PAGE_SIZE)) { > + btrfs_err(fs_info, > + "tree block crosses page boundary, start %llu nodesize %lu", > + start, len); > + return ERR_PTR(-EINVAL); > + } > > eb = find_extent_buffer(fs_info, start); > if (eb) >
On 6.11.20 г. 13:54 ч., Nikolay Borisov wrote: > > > On 3.11.20 г. 15:30 ч., Qu Wenruo wrote: >> As a preparation for subpage sector size support (allowing filesystem >> with sector size smaller than page size to be mounted) if the sector >> size is smaller than page size, we don't allow tree block to be read if >> it crosses 64K(*) boundary. >> >> The 64K is selected because: >> - We are only going to support 64K page size for subpage for now >> - 64K is also the max node size btrfs supports >> >> This ensures that, tree blocks are always contained in one page for a >> system with 64K page size, which can greatly simplify the handling. >> >> Or we need to do complex multi-page handling for tree blocks. >> >> Currently the only way to create such tree blocks crossing 64K boundary >> is by btrfs-convert, which will get fixed soon and doesn't get >> wide-spread usage. > > So filesystems with subpage blocksize which have been created as a > result of a convert operation would eventually fail to read some block > am I correct in my understanding? If that is the case then can't we > simply land subpage support in userspace tools _after_ the convert has > been fixed and turn this check into an assert? > > >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/extent_io.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c >> index 30768e49cf47..30bbaeaa129a 100644 >> --- a/fs/btrfs/extent_io.c >> +++ b/fs/btrfs/extent_io.c >> @@ -5261,6 +5261,13 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info, >> btrfs_err(fs_info, "bad tree block start %llu", start); >> return ERR_PTR(-EINVAL); >> } >> + if (btrfs_is_subpage(fs_info) && round_down(start, PAGE_SIZE) != >> + round_down(start + len - 1, PAGE_SIZE)) { One more thing, instead of doing those 2 round_downs, why not: offset_in_page(start) + len > PAGE_SIZE >> + btrfs_err(fs_info, >> + "tree block crosses page boundary, start %llu nodesize %lu", >> + start, len); >> + return ERR_PTR(-EINVAL); >> + } >> >> eb = find_extent_buffer(fs_info, start); >> if (eb) >> >
On 2020/11/6 下午7:54, Nikolay Borisov wrote: > > > On 3.11.20 г. 15:30 ч., Qu Wenruo wrote: >> As a preparation for subpage sector size support (allowing filesystem >> with sector size smaller than page size to be mounted) if the sector >> size is smaller than page size, we don't allow tree block to be read if >> it crosses 64K(*) boundary. >> >> The 64K is selected because: >> - We are only going to support 64K page size for subpage for now >> - 64K is also the max node size btrfs supports >> >> This ensures that, tree blocks are always contained in one page for a >> system with 64K page size, which can greatly simplify the handling. >> >> Or we need to do complex multi-page handling for tree blocks. >> >> Currently the only way to create such tree blocks crossing 64K boundary >> is by btrfs-convert, which will get fixed soon and doesn't get >> wide-spread usage. > > So filesystems with subpage blocksize which have been created as a > result of a convert operation would eventually fail to read some block > am I correct in my understanding? If that is the case then can't we > simply land subpage support in userspace tools _after_ the convert has > been fixed and turn this check into an assert? My bad, after I checked the convert code, at least from 2016 that all free space convert can utilized is already 64K aligned. So there isn't much thing to be done in convert already. Thanks, Qu > > >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/extent_io.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c >> index 30768e49cf47..30bbaeaa129a 100644 >> --- a/fs/btrfs/extent_io.c >> +++ b/fs/btrfs/extent_io.c >> @@ -5261,6 +5261,13 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info, >> btrfs_err(fs_info, "bad tree block start %llu", start); >> return ERR_PTR(-EINVAL); >> } >> + if (btrfs_is_subpage(fs_info) && round_down(start, PAGE_SIZE) != >> + round_down(start + len - 1, PAGE_SIZE)) { >> + btrfs_err(fs_info, >> + "tree block crosses page boundary, start %llu nodesize %lu", >> + start, len); >> + return ERR_PTR(-EINVAL); >> + } >> >> eb = find_extent_buffer(fs_info, start); >> if (eb) >> >
On 6.11.20 г. 15:25 ч., Qu Wenruo wrote: > > > On 2020/11/6 下午7:54, Nikolay Borisov wrote: >> >> >> On 3.11.20 г. 15:30 ч., Qu Wenruo wrote: >>> As a preparation for subpage sector size support (allowing filesystem >>> with sector size smaller than page size to be mounted) if the sector >>> size is smaller than page size, we don't allow tree block to be read if >>> it crosses 64K(*) boundary. >>> >>> The 64K is selected because: >>> - We are only going to support 64K page size for subpage for now >>> - 64K is also the max node size btrfs supports >>> >>> This ensures that, tree blocks are always contained in one page for a >>> system with 64K page size, which can greatly simplify the handling. >>> >>> Or we need to do complex multi-page handling for tree blocks. >>> >>> Currently the only way to create such tree blocks crossing 64K boundary >>> is by btrfs-convert, which will get fixed soon and doesn't get >>> wide-spread usage. >> >> So filesystems with subpage blocksize which have been created as a >> result of a convert operation would eventually fail to read some block >> am I correct in my understanding? If that is the case then can't we >> simply land subpage support in userspace tools _after_ the convert has >> been fixed and turn this check into an assert? > > My bad, after I checked the convert code, at least from 2016 that all > free space convert can utilized is already 64K aligned. > > So there isn't much thing to be done in convert already. So remove the bit about convert and does that mean this code should really be turned into an assert? > > Thanks, > Qu > >> >> >>> >>> Signed-off-by: Qu Wenruo <wqu@suse.com> >>> --- >>> fs/btrfs/extent_io.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c >>> index 30768e49cf47..30bbaeaa129a 100644 >>> --- a/fs/btrfs/extent_io.c >>> +++ b/fs/btrfs/extent_io.c >>> @@ -5261,6 +5261,13 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info, >>> btrfs_err(fs_info, "bad tree block start %llu", start); >>> return ERR_PTR(-EINVAL); >>> } >>> + if (btrfs_is_subpage(fs_info) && round_down(start, PAGE_SIZE) != >>> + round_down(start + len - 1, PAGE_SIZE)) { >>> + btrfs_err(fs_info, >>> + "tree block crosses page boundary, start %llu nodesize %lu", >>> + start, len); >>> + return ERR_PTR(-EINVAL); >>> + } >>> >>> eb = find_extent_buffer(fs_info, start); >>> if (eb) >>> >> >
On 2020/11/6 下午10:04, Nikolay Borisov wrote: > > > On 6.11.20 г. 15:25 ч., Qu Wenruo wrote: >> >> >> On 2020/11/6 下午7:54, Nikolay Borisov wrote: >>> >>> >>> On 3.11.20 г. 15:30 ч., Qu Wenruo wrote: >>>> As a preparation for subpage sector size support (allowing filesystem >>>> with sector size smaller than page size to be mounted) if the sector >>>> size is smaller than page size, we don't allow tree block to be read if >>>> it crosses 64K(*) boundary. >>>> >>>> The 64K is selected because: >>>> - We are only going to support 64K page size for subpage for now >>>> - 64K is also the max node size btrfs supports >>>> >>>> This ensures that, tree blocks are always contained in one page for a >>>> system with 64K page size, which can greatly simplify the handling. >>>> >>>> Or we need to do complex multi-page handling for tree blocks. >>>> >>>> Currently the only way to create such tree blocks crossing 64K boundary >>>> is by btrfs-convert, which will get fixed soon and doesn't get >>>> wide-spread usage. >>> >>> So filesystems with subpage blocksize which have been created as a >>> result of a convert operation would eventually fail to read some block >>> am I correct in my understanding? If that is the case then can't we >>> simply land subpage support in userspace tools _after_ the convert has >>> been fixed and turn this check into an assert? >> >> My bad, after I checked the convert code, at least from 2016 that all >> free space convert can utilized is already 64K aligned. >> >> So there isn't much thing to be done in convert already. > > So remove the bit about convert and does that mean this code should > really be turned into an assert? I just want to be extra safe. ASSERT() can still crash the system or ignore the important check and cause crash in other locations. Thanks, Qu > >> >> Thanks, >> Qu >> >>> >>> >>>> >>>> Signed-off-by: Qu Wenruo <wqu@suse.com> >>>> --- >>>> fs/btrfs/extent_io.c | 7 +++++++ >>>> 1 file changed, 7 insertions(+) >>>> >>>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c >>>> index 30768e49cf47..30bbaeaa129a 100644 >>>> --- a/fs/btrfs/extent_io.c >>>> +++ b/fs/btrfs/extent_io.c >>>> @@ -5261,6 +5261,13 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info, >>>> btrfs_err(fs_info, "bad tree block start %llu", start); >>>> return ERR_PTR(-EINVAL); >>>> } >>>> + if (btrfs_is_subpage(fs_info) && round_down(start, PAGE_SIZE) != >>>> + round_down(start + len - 1, PAGE_SIZE)) { >>>> + btrfs_err(fs_info, >>>> + "tree block crosses page boundary, start %llu nodesize %lu", >>>> + start, len); >>>> + return ERR_PTR(-EINVAL); >>>> + } >>>> >>>> eb = find_extent_buffer(fs_info, start); >>>> if (eb) >>>> >>> >>
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 30768e49cf47..30bbaeaa129a 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -5261,6 +5261,13 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info, btrfs_err(fs_info, "bad tree block start %llu", start); return ERR_PTR(-EINVAL); } + if (btrfs_is_subpage(fs_info) && round_down(start, PAGE_SIZE) != + round_down(start + len - 1, PAGE_SIZE)) { + btrfs_err(fs_info, + "tree block crosses page boundary, start %llu nodesize %lu", + start, len); + return ERR_PTR(-EINVAL); + } eb = find_extent_buffer(fs_info, start); if (eb)
As a preparation for subpage sector size support (allowing filesystem with sector size smaller than page size to be mounted) if the sector size is smaller than page size, we don't allow tree block to be read if it crosses 64K(*) boundary. The 64K is selected because: - We are only going to support 64K page size for subpage for now - 64K is also the max node size btrfs supports This ensures that, tree blocks are always contained in one page for a system with 64K page size, which can greatly simplify the handling. Or we need to do complex multi-page handling for tree blocks. Currently the only way to create such tree blocks crossing 64K boundary is by btrfs-convert, which will get fixed soon and doesn't get wide-spread usage. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/extent_io.c | 7 +++++++ 1 file changed, 7 insertions(+)