Re: [PATCH] Btrfs: fix data loss of fsync

[Liu Bo <bo.li.liu@oracle.com>]

On Thu, Mar 05, 2015 at 09:59:09AM +0000, Filipe David Manana wrote:
> On Thu, Mar 5, 2015 at 8:48 AM, Liu Bo <bo.li.liu@oracle.com> wrote:
> > This problem is uncovered by a test case: http://patchwork.ozlabs.org/patch/244297.
> >
> > Fsync() can report success when it actually doesn't.  When we
> > have several threads running fsync() at the same tiem and in one fsync() we
> > get a transaction abortion due to some problems(in the test case it's disk
> > failures), and other fsync()s may return successfully which makes userspace
> > programs think that data is now safely flushed into disk.
> >
> > It's because that after fsyncs() fail btrfs_sync_log() due to disk failures,
> > they get to try btrfs_commit_transaction() where it finds that there is
> > already a transaction being committed, and they'll just call wait_for_commit()
> > and return.  Note that we actually check "trans->aborted" in btrfs_end_transaction,
> > but it's likely that the error message is still not yet throwed out and only after
> > wait_for_commit() we're sure whether the transaction is committed successfully.
> >
> > This add the necessary check and it now passes the test.
> >
> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> 
> The change itself is ok but the title and associating it only with
> fsync are not quite right, since this is much more generic and not
> specific to fsync.

How about "catch errors after waiting for transaction"?

I'm open to any suggestions.

Thanks,

-liubo

> 
> thanks
> 
> > ---
> >  fs/btrfs/transaction.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> > index 7e80f32..bd7ea86 100644
> > --- a/fs/btrfs/transaction.c
> > +++ b/fs/btrfs/transaction.c
> > @@ -1814,6 +1814,9 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
> >
> >                 wait_for_commit(root, cur_trans);
> >
> > +               if (unlikely(ACCESS_ONCE(cur_trans->aborted)))
> > +                       ret = cur_trans->aborted;
> > +
> >                 btrfs_put_transaction(cur_trans);
> >
> >                 return ret;
> > --
> > 1.8.1.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 
> -- 
> Filipe David Manana,
> 
> "Reasonable men adapt themselves to the world.
>  Unreasonable men adapt the world to themselves.
>  That's why all progress depends on unreasonable men."
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html