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@xxxxxxxx>
>> ---
>> 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>
>
