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 <[email protected]>
> ---
> 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>
