Message ID | 20201210063905.75727-8-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: > For subpage case, grab_extent_buffer_from_page() can't really get an > extent buffer just from btrfs_subpage. > > Although we have btrfs_subpage::tree_block_bitmap, which can be used to > grab the bytenr of an existing extent buffer, and can then go radix tree > search to grab that existing eb. > > However we are still doing radix tree insert check in > alloc_extent_buffer(), thus we don't really need to do the extra hassle, > just let alloc_extent_buffer() to handle existing eb in radix tree. > > So for grab_extent_buffer_from_page(), just always return NULL for > subpage case. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/extent_io.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 51dd7ec3c2b3..b99bd0402130 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -5278,10 +5278,19 @@ struct extent_buffer *alloc_test_extent_buffer(struct btrfs_fs_info *fs_info, > } > #endif > > -static struct extent_buffer *grab_extent_buffer_from_page(struct page *page) > +static struct extent_buffer *grab_extent_buffer_from_page( > + struct btrfs_fs_info *fs_info, struct page *page) > { > struct extent_buffer *exists; > > + /* > + * For subpage case, we completely rely on radix tree to ensure we > + * don't try to insert two eb for the same bytenr. > + * So here we alwasy return NULL and just continue. > + */ > + if (fs_info->sectorsize < PAGE_SIZE) > + return NULL; > + Instead of hiding this in the function, just open-code it in the only caller. It would look like: diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index b99bd0402130..440dab207944 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -5370,8 +5370,9 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info, } spin_lock(&mapping->private_lock); - exists = grab_extent_buffer_from_page(fs_info, p); - if (exists) { + if (fs_info->sectorsize == PAGE_SIZE && + (exists = grab_extent_buffer_from_page(fs_info, p))); + { spin_unlock(&mapping->private_lock); unlock_page(p); put_page(p); Admittedly that exist = ... in the if condition is a bit of an anti-pattern but given it's used in only 1 place and makes the flow of code more linear I'd say it's a win. But would like to hear David's opinion. > /* Page not yet attached to an extent buffer */ > if (!PagePrivate(page)) > return NULL; > @@ -5361,7 +5370,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info, > } > > spin_lock(&mapping->private_lock); > - exists = grab_extent_buffer_from_page(p); > + exists = grab_extent_buffer_from_page(fs_info, p); > if (exists) { > spin_unlock(&mapping->private_lock); > unlock_page(p); >
On 2020/12/10 下午11:39, Nikolay Borisov wrote: > > > On 10.12.20 г. 8:38 ч., Qu Wenruo wrote: >> For subpage case, grab_extent_buffer_from_page() can't really get an >> extent buffer just from btrfs_subpage. >> >> Although we have btrfs_subpage::tree_block_bitmap, which can be used to >> grab the bytenr of an existing extent buffer, and can then go radix tree >> search to grab that existing eb. >> >> However we are still doing radix tree insert check in >> alloc_extent_buffer(), thus we don't really need to do the extra hassle, >> just let alloc_extent_buffer() to handle existing eb in radix tree. >> >> So for grab_extent_buffer_from_page(), just always return NULL for >> subpage case. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/extent_io.c | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c >> index 51dd7ec3c2b3..b99bd0402130 100644 >> --- a/fs/btrfs/extent_io.c >> +++ b/fs/btrfs/extent_io.c >> @@ -5278,10 +5278,19 @@ struct extent_buffer *alloc_test_extent_buffer(struct btrfs_fs_info *fs_info, >> } >> #endif >> >> -static struct extent_buffer *grab_extent_buffer_from_page(struct page *page) >> +static struct extent_buffer *grab_extent_buffer_from_page( >> + struct btrfs_fs_info *fs_info, struct page *page) >> { >> struct extent_buffer *exists; >> >> + /* >> + * For subpage case, we completely rely on radix tree to ensure we >> + * don't try to insert two eb for the same bytenr. >> + * So here we alwasy return NULL and just continue. >> + */ >> + if (fs_info->sectorsize < PAGE_SIZE) >> + return NULL; >> + > > Instead of hiding this in the function, just open-code it in the only caller. It would look like: > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index b99bd0402130..440dab207944 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -5370,8 +5370,9 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info, > } > > spin_lock(&mapping->private_lock); > - exists = grab_extent_buffer_from_page(fs_info, p); > - if (exists) { > + if (fs_info->sectorsize == PAGE_SIZE && > + (exists = grab_extent_buffer_from_page(fs_info, p))); > + { > spin_unlock(&mapping->private_lock); > unlock_page(p); > put_page(p); > > > Admittedly that exist = ... in the if condition is a bit of an anti-pattern but given it's used in only 1 place > and makes the flow of code more linear I'd say it's a win. But would like to hear David's opinion. Personally speaking, the (exists == *) inside the condition really looks ugly and hard to grasp. And since grab_extent_buffer_from_page() is only called once, the generated code shouldn't be that much different anyway as the compiler would mostly just inline it. So I still prefer the current code, not to mention it also provides extra space for the comment. Thanks, Qu > >> /* Page not yet attached to an extent buffer */ >> if (!PagePrivate(page)) >> return NULL; >> @@ -5361,7 +5370,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info, >> } >> >> spin_lock(&mapping->private_lock); >> - exists = grab_extent_buffer_from_page(p); >> + exists = grab_extent_buffer_from_page(fs_info, p); >> if (exists) { >> spin_unlock(&mapping->private_lock); >> unlock_page(p); >>
On 12/10/20 1:38 AM, Qu Wenruo wrote: > For subpage case, grab_extent_buffer_from_page() can't really get an > extent buffer just from btrfs_subpage. > > Although we have btrfs_subpage::tree_block_bitmap, which can be used to > grab the bytenr of an existing extent buffer, and can then go radix tree > search to grab that existing eb. > > However we are still doing radix tree insert check in > alloc_extent_buffer(), thus we don't really need to do the extra hassle, > just let alloc_extent_buffer() to handle existing eb in radix tree. > > So for grab_extent_buffer_from_page(), just always return NULL for > subpage case. This is fundamentally flawed. The extent buffer radix tree look up is done _after_ the pages are init'ed. This is why there's that complicated dance of checking for existing extent buffers attached to to a page, because we can race at the initialization stage and attach an EB to a page before it's in the radix tree. What you'll end up doing here is overwriting your existing subpage stuff anytime there's a race, and it'll end very badly. Thanks, Josef
On 2020/12/18 上午12:02, Josef Bacik wrote: > On 12/10/20 1:38 AM, Qu Wenruo wrote: >> For subpage case, grab_extent_buffer_from_page() can't really get an >> extent buffer just from btrfs_subpage. >> >> Although we have btrfs_subpage::tree_block_bitmap, which can be used to >> grab the bytenr of an existing extent buffer, and can then go radix tree >> search to grab that existing eb. >> >> However we are still doing radix tree insert check in >> alloc_extent_buffer(), thus we don't really need to do the extra hassle, >> just let alloc_extent_buffer() to handle existing eb in radix tree. >> >> So for grab_extent_buffer_from_page(), just always return NULL for >> subpage case. > > This is fundamentally flawed. The extent buffer radix tree look up is > done _after_ the pages are init'ed. This is why there's that > complicated dance of checking for existing extent buffers attached to to > a page, because we can race at the initialization stage and attach an EB > to a page before it's in the radix tree. What you'll end up doing here > is overwriting your existing subpage stuff anytime there's a race, and > it'll end very badly. Thanks, We have page lock preventing two eb getting the same page. And btrfs_attach_subpage() won't overwrite the existing page::private, thus it's safe. Thanks, Qu > > Josef
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 51dd7ec3c2b3..b99bd0402130 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -5278,10 +5278,19 @@ struct extent_buffer *alloc_test_extent_buffer(struct btrfs_fs_info *fs_info, } #endif -static struct extent_buffer *grab_extent_buffer_from_page(struct page *page) +static struct extent_buffer *grab_extent_buffer_from_page( + struct btrfs_fs_info *fs_info, struct page *page) { struct extent_buffer *exists; + /* + * For subpage case, we completely rely on radix tree to ensure we + * don't try to insert two eb for the same bytenr. + * So here we alwasy return NULL and just continue. + */ + if (fs_info->sectorsize < PAGE_SIZE) + return NULL; + /* Page not yet attached to an extent buffer */ if (!PagePrivate(page)) return NULL; @@ -5361,7 +5370,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info, } spin_lock(&mapping->private_lock); - exists = grab_extent_buffer_from_page(p); + exists = grab_extent_buffer_from_page(fs_info, p); if (exists) { spin_unlock(&mapping->private_lock); unlock_page(p);
For subpage case, grab_extent_buffer_from_page() can't really get an extent buffer just from btrfs_subpage. Although we have btrfs_subpage::tree_block_bitmap, which can be used to grab the bytenr of an existing extent buffer, and can then go radix tree search to grab that existing eb. However we are still doing radix tree insert check in alloc_extent_buffer(), thus we don't really need to do the extra hassle, just let alloc_extent_buffer() to handle existing eb in radix tree. So for grab_extent_buffer_from_page(), just always return NULL for subpage case. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/extent_io.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)