Messages in this thread | | | From | Martijn Coenen <> | Date | Fri, 22 May 2020 17:23:30 +0200 | Subject | Re: Writeback bug causing writeback stalls |
| |
[ dropped android-storage-core@google.com from CC: since that list can't receive emails from outside google.com - sorry about that ]
Hi Jan,
On Fri, May 22, 2020 at 4:41 PM Jan Kara <jack@suse.cz> wrote: > > The easiest way to fix this, I think, is to call requeue_inode() at the end of > > writeback_single_inode(), much like it is called from writeback_sb_inodes(). > > However, requeue_inode() has the following ominous warning: > > > > /* > > * Find proper writeback list for the inode depending on its current state and > > * possibly also change of its state while we were doing writeback. Here we > > * handle things such as livelock prevention or fairness of writeback among > > * inodes. This function can be called only by flusher thread - noone else > > * processes all inodes in writeback lists and requeueing inodes behind flusher > > * thread's back can have unexpected consequences. > > */ > > > > Obviously this is very critical code both from a correctness and a performance > > point of view, so I wanted to run this by the maintainers and folks who have > > contributed to this code first. > > Sadly, the fix won't be so easy. The main problem with calling > requeue_inode() from writeback_single_inode() is that if there's parallel > sync(2) call, inode->i_io_list is used to track all inodes that need writing > before sync(2) can complete. So requeueing inodes in parallel while sync(2) > runs can result in breaking data integrity guarantees of it.
Ah, makes sense.
> But I agree > we need to find some mechanism to safely move inode to appropriate dirty > list reasonably quickly. > > Probably I'd add an inode state flag telling that inode is queued for > writeback by flush worker and we won't touch dirty lists in that case, > otherwise we are safe to update current writeback list as needed. I'll work > on fixing this as when I was reading the code I've noticed there are other > quirks in the code as well. Thanks for the report!
Thanks! While looking at the code I also saw some other paths that appeared to be racy, though I haven't worked them out in detail to confirm that - the locking around the inode and writeback lists is tricky. What's the best way to follow up on those? Happy to post them to this same thread after I spend a bit more time looking at the code.
Thanks, Martijn
> > Honza > > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR
| |