On 2021/7/22 下午10:18, David Sterba wrote:

On Thu, Jul 22, 2021 at 05:18:19AM +0800, Qu Wenruo wrote:

quoted

quoted

quoted

For subpage case, we can have multiple sectors inside a page, this makes
it possible for __extent_writepage() to have part of its page submitted
before returning.

In btrfs/160, we are using dm-dust to emulate write error, this means
for certain pages, we could have everything running fine, but at the end
of __extent_writepage(), one of the submitted bios fails due to dm-dust.

Then the page is marked Error, and we change @ret from 0 to -EIO.

This makes the caller extent_write_cache_pages() to error out, without
submitting the remaining pages.

Furthermore, since we're erroring out for free space cache, it doesn't
really care about the error and will update the inode and retry the
writeback.

Then we re-run the delalloc range, and will try to insert the same
delalloc range while previous delalloc range is still hanging there,
triggering the above error.

This seems like the real bug.

That's true.

quoted

   We should do the proper cleanup for the
range in this case, not ignore errors during writeback.  Thanks,

But if you change the view point, __extent_writepage() is only
submitting the pages to bio.
It shouldn't bother the bio error, but only care the error that affects
the bio submission.

This PageError() check makes the behavior different between subpage and
regular page size, thus this can be considered as a quick fix for subpage.

For a proper fix for both subpage and non-subpage case, I'm trying a
completely different way, and will send out a RFC for comment later.

I tend to agree with Josef, the change is conunter intuitive. The proper
fix would be probably bigger than just a line removing the 'ret'
updates, so at least a comment explaining what's going on should be
there incase we decie to take this patch first.

Right, the commit message and missing comment is indeed concerning.

I assume that for non-subpage case it's working as before.

Ironically, for non-subpage case, that branch never get executed for bio
error.

For non-subpage case, a page will only be *added* to current bio, but
submission never happens for current page.

Thus that if (PageError()) branch only get executed for fatal errors
like failure to allocate memory.

Only for subpage case that we can have part of the page submitted while
we're still at that page.

Thanks,
Qu