Re: [PATCH] Btrfs: fix list_add corruption and soft lockups in fsync

[David Sterba <dsterba@suse.cz>]

On Mon, Nov 20, 2017 at 10:37:25AM -0700, Liu Bo wrote:
> > > [1]: https://www.spinics.net/lists/linux-btrfs/msg65308.html
> > > "btrfs list corruption and soft lockups while testing writeback error handling"
> > > 
> > 
> > Fixes: 8407f553268a4611f254 ("Btrfs: fix data corruption after fast fsync and writeback error")
> > 
> > you can also add the commit author to CC.
> 
> Hmm, somehow I thought ctx->list hasn't been added at that time, but
> looks like you're right.

The commit added

+       if (ordered_io_err) {
+               ctx->io_err = -EIO;
+               return 0;
+       }
+

which you fix.

> > > --- a/fs/btrfs/file.c
> > > +++ b/fs/btrfs/file.c
> > > @@ -2241,6 +2241,7 @@ int btrfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
> > >  	if (ctx.io_err) {
> > >  		btrfs_end_transaction(trans);
> > >  		ret = ctx.io_err;
> > > +		ASSERT(list_empty(&ctx.list));
> > 
> > Please move that to the label 'out', so all exit paths can catch the
> > problem.
> >
> 
> 'out' can't be used as we can goto 'out' before ctx is initialized,
> I'll add a new label 'out_ctx' in a separate patch (Feel free to fold
> them into one if you prefer).

Would be better to initialize ctx->list at the beginning and then always do the
list_empty check, instead of selectively jumping to out or out_ctx.