| Message ID | 20190117074855.24818-4-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: Enhancement to tree block validation | expand |
On 17.01.19 г. 9:48 ч., Qu Wenruo wrote: > This BUG_ON() is really just a crappy way to workaround the _must_check > attribute of submit_one_bio(). > > Now kill the BUG_ON() and allow flush_write_bio() to return error > number. > > Also add _must_check attribute to flush_write_bio(), and modify all > callers to handle the possible error returned. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/extent_io.c | 64 +++++++++++++++++++++++++++++++------------- > 1 file changed, 46 insertions(+), 18 deletions(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 8a2335713a2d..a773bc46badc 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -169,15 +169,15 @@ static int __must_check submit_one_bio(struct bio *bio, int mirror_num, > return blk_status_to_errno(ret); > } > > -static void flush_write_bio(struct extent_page_data *epd) > +static int __must_check flush_write_bio(struct extent_page_data *epd) > { > - if (epd->bio) { > - int ret; > + int ret = 0; > > + if (epd->bio) { > ret = submit_one_bio(epd->bio, 0, 0); > - BUG_ON(ret < 0); /* -ENOMEM */ > epd->bio = NULL; > } > + return ret; > } > > int __init extent_io_init(void) > @@ -3509,8 +3509,10 @@ lock_extent_buffer_for_io(struct extent_buffer *eb, > int ret = 0; > > if (!btrfs_try_tree_write_lock(eb)) { > + ret = flush_write_bio(epd); > + if (ret < 0) > + return ret; > flush = 1; > - flush_write_bio(epd); > btrfs_tree_lock(eb); > } > > @@ -3519,7 +3521,9 @@ lock_extent_buffer_for_io(struct extent_buffer *eb, > if (!epd->sync_io) > return 0; > if (!flush) { > - flush_write_bio(epd); > + ret = flush_write_bio(epd); > + if (ret < 0) > + return ret; > flush = 1; > } > while (1) { > @@ -3560,7 +3564,9 @@ lock_extent_buffer_for_io(struct extent_buffer *eb, > > if (!trylock_page(p)) { > if (!flush) { > - flush_write_bio(epd); > + ret = flush_write_bio(epd); > + if (ret < 0) > + return ret; Can't you end up with partially locked pages here? Are you sure that flush_write_bio will ALWAYS be executed when i = 0? If that\'s the case then I think an assert(i == 0) is in order there. > flush = 1; > } > lock_page(p); <snip>
On 2019/1/17 下午4:22, Nikolay Borisov wrote: > > > On 17.01.19 г. 9:48 ч., Qu Wenruo wrote: >> This BUG_ON() is really just a crappy way to workaround the _must_check >> attribute of submit_one_bio(). >> >> Now kill the BUG_ON() and allow flush_write_bio() to return error >> number. >> >> Also add _must_check attribute to flush_write_bio(), and modify all >> callers to handle the possible error returned. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/extent_io.c | 64 +++++++++++++++++++++++++++++++------------- >> 1 file changed, 46 insertions(+), 18 deletions(-) >> >> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c >> index 8a2335713a2d..a773bc46badc 100644 >> --- a/fs/btrfs/extent_io.c >> +++ b/fs/btrfs/extent_io.c >> @@ -169,15 +169,15 @@ static int __must_check submit_one_bio(struct bio *bio, int mirror_num, >> return blk_status_to_errno(ret); >> } >> >> -static void flush_write_bio(struct extent_page_data *epd) >> +static int __must_check flush_write_bio(struct extent_page_data *epd) >> { >> - if (epd->bio) { >> - int ret; >> + int ret = 0; >> >> + if (epd->bio) { >> ret = submit_one_bio(epd->bio, 0, 0); >> - BUG_ON(ret < 0); /* -ENOMEM */ >> epd->bio = NULL; >> } >> + return ret; >> } >> >> int __init extent_io_init(void) >> @@ -3509,8 +3509,10 @@ lock_extent_buffer_for_io(struct extent_buffer *eb, >> int ret = 0; >> >> if (!btrfs_try_tree_write_lock(eb)) { >> + ret = flush_write_bio(epd); >> + if (ret < 0) >> + return ret; >> flush = 1; >> - flush_write_bio(epd); >> btrfs_tree_lock(eb); >> } >> >> @@ -3519,7 +3521,9 @@ lock_extent_buffer_for_io(struct extent_buffer *eb, >> if (!epd->sync_io) >> return 0; >> if (!flush) { >> - flush_write_bio(epd); >> + ret = flush_write_bio(epd); >> + if (ret < 0) >> + return ret; >> flush = 1; >> } >> while (1) { >> @@ -3560,7 +3564,9 @@ lock_extent_buffer_for_io(struct extent_buffer *eb, >> >> if (!trylock_page(p)) { >> if (!flush) { >> - flush_write_bio(epd); >> + ret = flush_write_bio(epd); >> + if (ret < 0) >> + return ret; > > Can't you end up with partially locked pages here? Are you sure that > flush_write_bio will ALWAYS be executed when i = 0? If that\'s the case > then I think an assert(i == 0) is in order there. An ASSERT() indeed makes sense here. Although I could also make it better by recording the failed page number and unlock those already locked pages. I'm OK either way. Thanks, Qu > >> flush = 1; >> } >> lock_page(p); > > <snip> >
On 17.01.19 г. 10:28 ч., Qu Wenruo wrote: > > > On 2019/1/17 下午4:22, Nikolay Borisov wrote: >> >> >> On 17.01.19 г. 9:48 ч., Qu Wenruo wrote: >>> This BUG_ON() is really just a crappy way to workaround the _must_check >>> attribute of submit_one_bio(). >>> >>> Now kill the BUG_ON() and allow flush_write_bio() to return error >>> number. >>> >>> Also add _must_check attribute to flush_write_bio(), and modify all >>> callers to handle the possible error returned. >>> >>> Signed-off-by: Qu Wenruo <wqu@suse.com> >>> --- >>> fs/btrfs/extent_io.c | 64 +++++++++++++++++++++++++++++++------------- >>> 1 file changed, 46 insertions(+), 18 deletions(-) >>> >>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c >>> index 8a2335713a2d..a773bc46badc 100644 >>> --- a/fs/btrfs/extent_io.c >>> +++ b/fs/btrfs/extent_io.c >>> @@ -169,15 +169,15 @@ static int __must_check submit_one_bio(struct bio *bio, int mirror_num, >>> return blk_status_to_errno(ret); >>> } >>> >>> -static void flush_write_bio(struct extent_page_data *epd) >>> +static int __must_check flush_write_bio(struct extent_page_data *epd) >>> { >>> - if (epd->bio) { >>> - int ret; >>> + int ret = 0; >>> >>> + if (epd->bio) { >>> ret = submit_one_bio(epd->bio, 0, 0); >>> - BUG_ON(ret < 0); /* -ENOMEM */ >>> epd->bio = NULL; >>> } >>> + return ret; >>> } >>> >>> int __init extent_io_init(void) >>> @@ -3509,8 +3509,10 @@ lock_extent_buffer_for_io(struct extent_buffer *eb, >>> int ret = 0; >>> >>> if (!btrfs_try_tree_write_lock(eb)) { >>> + ret = flush_write_bio(epd); >>> + if (ret < 0) >>> + return ret; >>> flush = 1; >>> - flush_write_bio(epd); >>> btrfs_tree_lock(eb); >>> } >>> >>> @@ -3519,7 +3521,9 @@ lock_extent_buffer_for_io(struct extent_buffer *eb, >>> if (!epd->sync_io) >>> return 0; >>> if (!flush) { >>> - flush_write_bio(epd); >>> + ret = flush_write_bio(epd); >>> + if (ret < 0) >>> + return ret; >>> flush = 1; >>> } >>> while (1) { >>> @@ -3560,7 +3564,9 @@ lock_extent_buffer_for_io(struct extent_buffer *eb, >>> >>> if (!trylock_page(p)) { >>> if (!flush) { >>> - flush_write_bio(epd); >>> + ret = flush_write_bio(epd); >>> + if (ret < 0) >>> + return ret; >> >> Can't you end up with partially locked pages here? Are you sure that >> flush_write_bio will ALWAYS be executed when i = 0? If that\'s the case >> then I think an assert(i == 0) is in order there. > > An ASSERT() indeed makes sense here. > > Although I could also make it better by recording the failed page number > and unlock those already locked pages. Unlocking is the correct way to handle it, the assert would be there to prove that you won't ever need the unlocking code. If it triggers then this case must be handled. > > I'm OK either way. > > Thanks, > Qu > >> >>> flush = 1; >>> } >>> lock_page(p); >> >> <snip> >>
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 8a2335713a2d..a773bc46badc 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -169,15 +169,15 @@ static int __must_check submit_one_bio(struct bio *bio, int mirror_num, return blk_status_to_errno(ret); } -static void flush_write_bio(struct extent_page_data *epd) +static int __must_check flush_write_bio(struct extent_page_data *epd) { - if (epd->bio) { - int ret; + int ret = 0; + if (epd->bio) { ret = submit_one_bio(epd->bio, 0, 0); - BUG_ON(ret < 0); /* -ENOMEM */ epd->bio = NULL; } + return ret; } int __init extent_io_init(void) @@ -3509,8 +3509,10 @@ lock_extent_buffer_for_io(struct extent_buffer *eb, int ret = 0; if (!btrfs_try_tree_write_lock(eb)) { + ret = flush_write_bio(epd); + if (ret < 0) + return ret; flush = 1; - flush_write_bio(epd); btrfs_tree_lock(eb); } @@ -3519,7 +3521,9 @@ lock_extent_buffer_for_io(struct extent_buffer *eb, if (!epd->sync_io) return 0; if (!flush) { - flush_write_bio(epd); + ret = flush_write_bio(epd); + if (ret < 0) + return ret; flush = 1; } while (1) { @@ -3560,7 +3564,9 @@ lock_extent_buffer_for_io(struct extent_buffer *eb, if (!trylock_page(p)) { if (!flush) { - flush_write_bio(epd); + ret = flush_write_bio(epd); + if (ret < 0) + return ret; flush = 1; } lock_page(p); @@ -3751,6 +3757,7 @@ int btree_write_cache_pages(struct address_space *mapping, .sync_io = wbc->sync_mode == WB_SYNC_ALL, }; int ret = 0; + int flush_ret; int done = 0; int nr_to_write_done = 0; struct pagevec pvec; @@ -3818,6 +3825,11 @@ int btree_write_cache_pages(struct address_space *mapping, prev_eb = eb; ret = lock_extent_buffer_for_io(eb, fs_info, &epd); + if (ret < 0) { + free_extent_buffer(eb); + done = 1; + break; + } if (!ret) { free_extent_buffer(eb); continue; @@ -3850,8 +3862,10 @@ int btree_write_cache_pages(struct address_space *mapping, index = 0; goto retry; } - flush_write_bio(&epd); - return ret; + flush_ret = flush_write_bio(&epd); + if (ret) + return ret; + return flush_ret; } /** @@ -3947,7 +3961,9 @@ static int extent_write_cache_pages(struct address_space *mapping, * tmpfs file mapping */ if (!trylock_page(page)) { - flush_write_bio(epd); + ret = flush_write_bio(epd); + if (ret < 0) + break; lock_page(page); } @@ -3957,8 +3973,11 @@ static int extent_write_cache_pages(struct address_space *mapping, } if (wbc->sync_mode != WB_SYNC_NONE) { - if (PageWriteback(page)) - flush_write_bio(epd); + if (PageWriteback(page)) { + ret = flush_write_bio(epd); + if (ret < 0) + break; + } wait_on_page_writeback(page); } @@ -4019,6 +4038,7 @@ static int extent_write_cache_pages(struct address_space *mapping, int extent_write_full_page(struct page *page, struct writeback_control *wbc) { int ret; + int flush_ret; struct extent_page_data epd = { .bio = NULL, .tree = &BTRFS_I(page->mapping->host)->io_tree, @@ -4028,14 +4048,17 @@ int extent_write_full_page(struct page *page, struct writeback_control *wbc) ret = __extent_writepage(page, wbc, &epd); - flush_write_bio(&epd); - return ret; + flush_ret = flush_write_bio(&epd); + if (ret) + return ret; + return flush_ret; } int extent_write_locked_range(struct inode *inode, u64 start, u64 end, int mode) { int ret = 0; + int flush_ret; struct address_space *mapping = inode->i_mapping; struct extent_io_tree *tree = &BTRFS_I(inode)->io_tree; struct page *page; @@ -4068,14 +4091,17 @@ int extent_write_locked_range(struct inode *inode, u64 start, u64 end, start += PAGE_SIZE; } - flush_write_bio(&epd); - return ret; + flush_ret = flush_write_bio(&epd); + if (ret) + return ret; + return flush_ret; } int extent_writepages(struct address_space *mapping, struct writeback_control *wbc) { int ret = 0; + int flush_ret; struct extent_page_data epd = { .bio = NULL, .tree = &BTRFS_I(mapping->host)->io_tree, @@ -4084,8 +4110,10 @@ int extent_writepages(struct address_space *mapping, }; ret = extent_write_cache_pages(mapping, wbc, &epd); - flush_write_bio(&epd); - return ret; + flush_ret = flush_write_bio(&epd); + if (ret) + return ret; + return flush_ret; } int extent_readpages(struct address_space *mapping, struct list_head *pages,
This BUG_ON() is really just a crappy way to workaround the _must_check attribute of submit_one_bio(). Now kill the BUG_ON() and allow flush_write_bio() to return error number. Also add _must_check attribute to flush_write_bio(), and modify all callers to handle the possible error returned. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/extent_io.c | 64 +++++++++++++++++++++++++++++++------------- 1 file changed, 46 insertions(+), 18 deletions(-)