Re: [PATCH] btrfs: Avoid getting stuck during cyclic writebacks

[Tejun Heo <tj@kernel.org>]

Hello,

On Tue, Oct 08, 2019 at 04:23:22PM +0200, David Sterba wrote:
> > 1. There is a single file which has accumulated enough dirty pages to
> >    trigger balance_dirty_pages() and the writer appending to the file
> >    with a series of short writes.
> > 
> > 2. bdp kicks in, wakes up background writeback and sleeps.
> 
> What does 'bdp' refer to?

Oh, Sorry about that.  balance_dirty_pages().

> > IOW, as long as it's not EOF and there's budget, the code never
> > retries writing back the same page.  Only when a page happens to be
> > the last page of a particular run, we end up retrying the page, which
> > can't possibly guarantee anything data integrity related.  Besides,
> > cyclic writes are only used for non-syncing writebacks meaning that
> > there's no data integrity implication to begin with.
> 
> The code was added in a91326679f2a0a4c239 ("Btrfs: make
> mapping->writeback_index point to the last written page") after a user
> report in https://www.spinics.net/lists/linux-btrfs/msg52628.html , slow
> appends that caused fragmentation

Ah, okay, that makes sense.  That prolly warrants some comments.

> What you describe as the cause is similar, but you're partially
> reverting the fix that was supposed to fix it. As there's more code
> added by the original patch, the revert won't probably bring back the
> bug.
> 
> The whole function and states are hard to follow, I agree with your
> reasoning about the check being bogus and overall I'd rather see fewer
> special cases in the function.
> 
> Also the removed comment mentions media errors but this was not the
> problem for the original report and is not a common scenario either. So
> as long as the fallback in such case is sane (ie. set done = 1 and
> exit), I don't see futher problems.
> 
> > Fix it by always setting done_index past the current page being
> > processed.
> > 
> > Note that this problem exists in other writepages too.
> 
> I can see that write_cache_pages does the same done_index updates.  So
> it makes sense that the page walking and writeback index tracking
> behaviour is consistent, unless extent_write_cache_pages has diverged
> too much.
> 
> I'll add the patch to misc-next. Thanks.

So, in case trying to write the last page is still needed, the thing
necessary to fix this deadlock is avoiding repeating on the last page
too many times / indefinitely as that's what ends up blocking forward
progress - almost all of dirty data is behind the cyclic cursor and
the cursor can't wrap back to get to them.

Thanks.

-- 
tejun