[PATCH] Btrfs: fix data loss of fsync

[PATCH] Btrfs: fix data loss of fsync

[Liu Bo]

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>
---
 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

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

[Filipe David Manana]

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.

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."

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

[Liu Bo]

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

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

[Filipe David Manana]

On Thu, Mar 5, 2015 at 10:23 AM, Liu Bo <bo.li.liu@oracle.com> wrote:
> 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.

IMHO, something like that is better or "check for transaction abortion
after waiting for it".

thanks

>
> 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



-- 
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."

[PATCH V2] Btrfs: catch transaction abortion after waiting for it

[Liu Bo]

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>
---
v2: Use a more generic title since it's not only for fsync, but for others.

 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

Re: [PATCH V2] Btrfs: catch transaction abortion after waiting for it

[Chris Mason]

On Thu, Mar 5, 2015 at 5:36 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>
> ---
> v2: Use a more generic title since it's not only for fsync, but for 
> others.
> 
>  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;
> +

Thanks Liu, but why are we using ACCESS_ONCE here?

-chris

Re: [PATCH V2] Btrfs: catch transaction abortion after waiting for it

[Liu Bo]

On Thu, Mar 05, 2015 at 08:59:57AM -0500, Chris Mason wrote:
> 
> 
> On Thu, Mar 5, 2015 at 5:36 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>
> >---
> >v2: Use a more generic title since it's not only for fsync, but
> >for others.
> >
> > 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;
> >+
> 
> Thanks Liu, but why are we using ACCESS_ONCE here?

It should be not necessary, I just copied it from the first check in btrfs_commit_transaction(),
not insisting in using it.

Thanks,

-liubo

[PATCH v3] Btrfs: catch transaction abortion after waiting for it

[Liu Bo]

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>
---
v3: Remove the unnecessary macro ACCESS_ONCE()
v2: Use a more generic title since it's not only for fsync, but for others. 

 fs/btrfs/transaction.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 7e80f32..d28670c 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(cur_trans->aborted))
+			ret = cur_trans->aborted;
+
 		btrfs_put_transaction(cur_trans);
 
 		return ret;
-- 
1.8.1.4