On 2019/2/15 下午3:18, Qu Wenruo wrote:
> 
> 
> On 2019/1/31 下午10:36, David Sterba wrote:
>> On Thu, Jan 31, 2019 at 08:45:42AM +0800, Qu Wenruo wrote:
>>>
>>>
>>> On 2019/1/30 下午11:19, David Sterba wrote:
>>>> On Fri, Jan 25, 2019 at 01:09:17PM +0800, Qu Wenruo wrote:
>>>>> +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;
>>>>
>>>> I'm not sure if resetting epd->bio to NULL is all that needs to be done
>>>> here. With the BUG_ON the error case never happens so if all goes fine
>>>> it's also ok to set it to NULL and continue. But the callers might need
>>>> to send the flush again.
>>>
>>> If flush_write_bio() get called again on the failed one, it will just
>>> get skipped as  epd->bio is NULL, submit_one_bio() will not be triggered.
>>
>> Yes that's clear, but what is the state of the bio that went to
>> flush_write_bio and submit_one_bio failed?
>>
> 
> You got me.
> 
> In fact current simple "epd->bio = NULL" on failure will just leak epd->bio.
> 
> We need to at least call "bio_put(epd->bio);" before setting "epd->bio =
> NULL;"

Facepalm, I'm always too quick to draw a conclusion.

With proper test, the epd->bio is freed by its endio function.
So at flush_write_bio() call time, no matter whether submit_one_bio()
returns error or not, endio of epd->bio will be or has already been
triggered thus free the bio.

Either by successful bio submission, or by manual bio_endio(bio) call in
submit_bio_hook.

So in fact just setting "epd->bio = NULL" is completely fine in this
context.

Thanks,
Qu

> 
> Thanks for pointing this problem out,
> Qu
>