| Message ID | 20200909094914.29721-4-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Cleanup metadata page reading path | expand |
On 2020/9/9 下午5:49, Nikolay Borisov wrote: > Metadata pages currently use __do_readpage to read metadata pages, > unfortunately this function is also used to deal with ordinary data > pages. This makes the metadata pages reading code to go through multiple > hoops in order to adhere to __do_readpage invariants. Most of these are > necessary for data pages which could be compressed. For metadata it's > enough to simply build a bio and submit it. > > To this effect simply call submit_extent_page directly from > read_extent_buffer_pages which is the only callpath used to populate > extent_buffers with data. This in turn enables further cleanups. This is awesome!!! And the code also looks pretty good to me. Reviewed-by: Qu Wenruo <wqu@suse.com> Just a note for further enhancement inlined below. > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> > --- > fs/btrfs/extent_io.c | 18 ++++++------------ > 1 file changed, 6 insertions(+), 12 deletions(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index ac92c0ab1402..1789a7931312 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -5575,20 +5575,14 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num) > } > > ClearPageError(page); > - err = __extent_read_full_page(page, > - btree_get_extent, &bio, > - mirror_num, &bio_flags, > - REQ_META); > + err = submit_extent_page(REQ_OP_READ | REQ_META, NULL, > + page, page_offset(page), PAGE_SIZE, 0, It would be better to enhance the comment for submit_extent_page() of @offset. It's in fact btrfs logical bytenr. For metadata, page_offset(page) is also the btrfs logical bytenr so it's completely fine. But it can be different for data inodes, so it's better to make it more clear in the comment. Thanks, Qu > + &bio, end_bio_extent_readpage, > + mirror_num, 0, 0, false); > if (err) { > ret = err; > - /* > - * We use &bio in above __extent_read_full_page, > - * so we ensure that if it returns error, the > - * current page fails to add itself to bio and > - * it's been unlocked. > - * > - * We must dec io_pages by ourselves. > - */ > + SetPageError(page); > + unlock_page(page); > atomic_dec(&eb->io_pages); > } > } else { >
On 9/9/20 5:49 AM, Nikolay Borisov wrote: > Metadata pages currently use __do_readpage to read metadata pages, > unfortunately this function is also used to deal with ordinary data > pages. This makes the metadata pages reading code to go through multiple > hoops in order to adhere to __do_readpage invariants. Most of these are > necessary for data pages which could be compressed. For metadata it's > enough to simply build a bio and submit it. > > To this effect simply call submit_extent_page directly from > read_extent_buffer_pages which is the only callpath used to populate > extent_buffers with data. This in turn enables further cleanups. > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> > --- > fs/btrfs/extent_io.c | 18 ++++++------------ > 1 file changed, 6 insertions(+), 12 deletions(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index ac92c0ab1402..1789a7931312 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -5575,20 +5575,14 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num) > } > > ClearPageError(page); > - err = __extent_read_full_page(page, > - btree_get_extent, &bio, > - mirror_num, &bio_flags, > - REQ_META); > + err = submit_extent_page(REQ_OP_READ | REQ_META, NULL, > + page, page_offset(page), PAGE_SIZE, 0, > + &bio, end_bio_extent_readpage, > + mirror_num, 0, 0, false); > if (err) { > ret = err; > - /* > - * We use &bio in above __extent_read_full_page, > - * so we ensure that if it returns error, the > - * current page fails to add itself to bio and > - * it's been unlocked. > - * > - * We must dec io_pages by ourselves. > - */ I'd rather change the comment to indicate that we failed to submit the bio, thus the page is our responsibility to clean up, and thus we need to do the error/unlock and io_pages dance. Thanks, Josef
On 9.09.20 г. 14:20 ч., Qu Wenruo wrote: > > > On 2020/9/9 下午5:49, Nikolay Borisov wrote: >> Metadata pages currently use __do_readpage to read metadata pages, >> unfortunately this function is also used to deal with ordinary data >> pages. This makes the metadata pages reading code to go through multiple >> hoops in order to adhere to __do_readpage invariants. Most of these are >> necessary for data pages which could be compressed. For metadata it's >> enough to simply build a bio and submit it. >> >> To this effect simply call submit_extent_page directly from >> read_extent_buffer_pages which is the only callpath used to populate >> extent_buffers with data. This in turn enables further cleanups. > > This is awesome!!! > > And the code also looks pretty good to me. > > Reviewed-by: Qu Wenruo <wqu@suse.com> > > Just a note for further enhancement inlined below. > >> >> Signed-off-by: Nikolay Borisov <nborisov@suse.com> >> --- >> fs/btrfs/extent_io.c | 18 ++++++------------ >> 1 file changed, 6 insertions(+), 12 deletions(-) >> >> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c >> index ac92c0ab1402..1789a7931312 100644 >> --- a/fs/btrfs/extent_io.c >> +++ b/fs/btrfs/extent_io.c >> @@ -5575,20 +5575,14 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num) >> } >> >> ClearPageError(page); >> - err = __extent_read_full_page(page, >> - btree_get_extent, &bio, >> - mirror_num, &bio_flags, >> - REQ_META); >> + err = submit_extent_page(REQ_OP_READ | REQ_META, NULL, >> + page, page_offset(page), PAGE_SIZE, 0, > > It would be better to enhance the comment for submit_extent_page() of > @offset. > It's in fact btrfs logical bytenr. > > For metadata, page_offset(page) is also the btrfs logical bytenr so it's > completely fine. > But it can be different for data inodes, so it's better to make it more > clear in the comment. How can it be different for data node? page_offset is always page->index << PAGE_SHIFT, meaning it's the offset in the file that this page refers to. I.e page 5 would refer to 20k. > > Thanks, > Qu > >> + &bio, end_bio_extent_readpage, >> + mirror_num, 0, 0, false); >> if (err) { >> ret = err; >> - /* >> - * We use &bio in above __extent_read_full_page, >> - * so we ensure that if it returns error, the >> - * current page fails to add itself to bio and >> - * it's been unlocked. >> - * >> - * We must dec io_pages by ourselves. >> - */ >> + SetPageError(page); >> + unlock_page(page); >> atomic_dec(&eb->io_pages); >> } >> } else { >> >
On 2020/9/14 下午4:08, Nikolay Borisov wrote: > > > On 9.09.20 г. 14:20 ч., Qu Wenruo wrote: >> >> >> On 2020/9/9 下午5:49, Nikolay Borisov wrote: >>> Metadata pages currently use __do_readpage to read metadata pages, >>> unfortunately this function is also used to deal with ordinary data >>> pages. This makes the metadata pages reading code to go through multiple >>> hoops in order to adhere to __do_readpage invariants. Most of these are >>> necessary for data pages which could be compressed. For metadata it's >>> enough to simply build a bio and submit it. >>> >>> To this effect simply call submit_extent_page directly from >>> read_extent_buffer_pages which is the only callpath used to populate >>> extent_buffers with data. This in turn enables further cleanups. >> >> This is awesome!!! >> >> And the code also looks pretty good to me. >> >> Reviewed-by: Qu Wenruo <wqu@suse.com> >> >> Just a note for further enhancement inlined below. >> >>> >>> Signed-off-by: Nikolay Borisov <nborisov@suse.com> >>> --- >>> fs/btrfs/extent_io.c | 18 ++++++------------ >>> 1 file changed, 6 insertions(+), 12 deletions(-) >>> >>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c >>> index ac92c0ab1402..1789a7931312 100644 >>> --- a/fs/btrfs/extent_io.c >>> +++ b/fs/btrfs/extent_io.c >>> @@ -5575,20 +5575,14 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num) >>> } >>> >>> ClearPageError(page); >>> - err = __extent_read_full_page(page, >>> - btree_get_extent, &bio, >>> - mirror_num, &bio_flags, >>> - REQ_META); >>> + err = submit_extent_page(REQ_OP_READ | REQ_META, NULL, >>> + page, page_offset(page), PAGE_SIZE, 0, >> >> It would be better to enhance the comment for submit_extent_page() of >> @offset. >> It's in fact btrfs logical bytenr. >> >> For metadata, page_offset(page) is also the btrfs logical bytenr so it's >> completely fine. >> But it can be different for data inodes, so it's better to make it more >> clear in the comment. > > How can it be different for data node? page_offset is always page->index > << PAGE_SHIFT, meaning it's the offset in the file that this page refers > to. I.e page 5 would refer to 20k. For data inode, its page_offset() is the file offset, not logical bytenr. Thanks, Qu > >> >> Thanks, >> Qu >> >>> + &bio, end_bio_extent_readpage, >>> + mirror_num, 0, 0, false); >>> if (err) { >>> ret = err; >>> - /* >>> - * We use &bio in above __extent_read_full_page, >>> - * so we ensure that if it returns error, the >>> - * current page fails to add itself to bio and >>> - * it's been unlocked. >>> - * >>> - * We must dec io_pages by ourselves. >>> - */ >>> + SetPageError(page); >>> + unlock_page(page); >>> atomic_dec(&eb->io_pages); >>> } >>> } else { >>> >>
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index ac92c0ab1402..1789a7931312 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -5575,20 +5575,14 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num) } ClearPageError(page); - err = __extent_read_full_page(page, - btree_get_extent, &bio, - mirror_num, &bio_flags, - REQ_META); + err = submit_extent_page(REQ_OP_READ | REQ_META, NULL, + page, page_offset(page), PAGE_SIZE, 0, + &bio, end_bio_extent_readpage, + mirror_num, 0, 0, false); if (err) { ret = err; - /* - * We use &bio in above __extent_read_full_page, - * so we ensure that if it returns error, the - * current page fails to add itself to bio and - * it's been unlocked. - * - * We must dec io_pages by ourselves. - */ + SetPageError(page); + unlock_page(page); atomic_dec(&eb->io_pages); } } else {
Metadata pages currently use __do_readpage to read metadata pages, unfortunately this function is also used to deal with ordinary data pages. This makes the metadata pages reading code to go through multiple hoops in order to adhere to __do_readpage invariants. Most of these are necessary for data pages which could be compressed. For metadata it's enough to simply build a bio and submit it. To this effect simply call submit_extent_page directly from read_extent_buffer_pages which is the only callpath used to populate extent_buffers with data. This in turn enables further cleanups. Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- fs/btrfs/extent_io.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-)