Re: [PATCH v15 43/43] btrfs: zoned: deal with holes writing out tree-log pages

2021-02-05 Thread Filipe Manana
On Fri, Feb 5, 2021 at 12:55 PM Naohiro Aota  wrote:
>
> On Fri, Feb 05, 2021 at 11:49:05AM +, Filipe Manana wrote:
> > On Fri, Feb 5, 2021 at 9:26 AM Naohiro Aota  wrote:
> > >
> > > Since the zoned filesystem requires sequential write out of metadata, we
> > > cannot proceed with a hole in tree-log pages. When such a hole exists,
> > > btree_write_cache_pages() will return -EAGAIN. We cannot wait for the 
> > > range
> > > to be written, because it will cause a deadlock. So, let's bail out to a
> > > full commit in this case.
> > >
> > > Cc: Filipe Manana 
> > > Signed-off-by: Naohiro Aota 
> > > ---
> > >  fs/btrfs/tree-log.c | 19 ++-
> > >  1 file changed, 18 insertions(+), 1 deletion(-)
> > >
> > > This patch solves a regression introduced by fixing patch 40. I'm
> > > sorry for the confusing patch numbering.
> >
> > Hum, how does patch 40 can cause this?
> > And is it before the fixup or after?
>
> With pre-5.10 code base + zoned series at that time, it passed
> xfstests without this patch.
>
> With current code base + zoned series without the fixup for patch 40,
> it also passed the tests, because we are mostly bailing out to a full
> commit.
>
> The fixup now stressed the new fsync code on zoned mode and revealed
> an issue to have -EAGAIN from btrfs_write_marked_extents(). This error
> happens when a concurrent transaction commit is writing a dirty extent
> in this tree-log commit. This issue didn't occur previously because of
> a longer critical section, I guess.

Ok, if I understand you correctly, the problem is a transaction commit
and an fsync both allocating metadata extents at the same time.

>
> >
> > >
> > > diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> > > index 4e72794342c0..629e605cd62d 100644
> > > --- a/fs/btrfs/tree-log.c
> > > +++ b/fs/btrfs/tree-log.c
> > > @@ -3120,6 +3120,14 @@ int btrfs_sync_log(struct btrfs_trans_handle 
> > > *trans,
> > >  */
> > > blk_start_plug(&plug);
> > > ret = btrfs_write_marked_extents(fs_info, &log->dirty_log_pages, 
> > > mark);
> > > +   /*
> > > +* There is a hole writing out the extents and cannot proceed it 
> > > on
> > > +* zoned filesystem, which require sequential writing. We can
> >
> > require -> requires
> >
> > > +* ignore the error for now, since we don't wait for completion 
> > > for
> > > +* now.
> >
> > So why can we ignore the error for now?
> > Why not just bail out here and mark the log for full commit? (without
> > a transaction abort)
>
> As described above, -EAGAIN happens when a concurrent process writes
> out an extent buffer of this tree-log commit. This concurrent write
> out will fill a hole for us, so the next write out might
> succeed. Indeed we can bail out here, but I opted to try the next
> write.

Ok, if I understand you correctly, you mean it will be fine if after
this point no one allocates metadata extents from the hole?

I think such a clear explanation would fit nicely in the comment.

Thanks.

>
> > > +*/
> > > +   if (ret == -EAGAIN)
> > > +   ret = 0;
> > > if (ret) {
> > > blk_finish_plug(&plug);
> > > btrfs_abort_transaction(trans, ret);
> > > @@ -3229,7 +3237,16 @@ int btrfs_sync_log(struct btrfs_trans_handle 
> > > *trans,
> > >  &log_root_tree->dirty_log_pages,
> > >  EXTENT_DIRTY | EXTENT_NEW);
> > > blk_finish_plug(&plug);
> > > -   if (ret) {
> > > +   /*
> > > +* There is a hole in the extents, and failed to sequential write
> > > +* on zoned filesystem. We cannot wait for this write outs, sinc 
> > > it
> >
> > this -> these
> >
> > > +* cause a deadlock. Bail out to the full commit, instead.
> > > +*/
> > > +   if (ret == -EAGAIN) {
> > > +   btrfs_wait_tree_log_extents(log, mark);
> > > +   mutex_unlock(&log_root_tree->log_mutex);
> > > +   goto out_wake_log_root;
> >
> > Must also call btrfs_set_log_full_commit(trans);
>
> Oops, I missed this one.
>
> > Thanks.
> >
> > > +   } else if (ret) {
> > > btrfs_set_log_full_commit(trans);
> > > btrfs_abort_transaction(trans, ret);
> > > mutex_unlock(&log_root_tree->log_mutex);
> > > --
> > > 2.30.0
> > >
> >
> >
> > --
> > Filipe David Manana,
> >
> > “Whether you think you can, or you think you can't — you're right.”



-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”


Re: [PATCH v15 43/43] btrfs: zoned: deal with holes writing out tree-log pages

2021-02-05 Thread Filipe Manana
On Fri, Feb 5, 2021 at 11:49 AM Filipe Manana  wrote:
>
> On Fri, Feb 5, 2021 at 9:26 AM Naohiro Aota  wrote:
> >
> > Since the zoned filesystem requires sequential write out of metadata, we
> > cannot proceed with a hole in tree-log pages. When such a hole exists,
> > btree_write_cache_pages() will return -EAGAIN. We cannot wait for the range
> > to be written, because it will cause a deadlock. So, let's bail out to a
> > full commit in this case.
> >
> > Cc: Filipe Manana 
> > Signed-off-by: Naohiro Aota 
> > ---
> >  fs/btrfs/tree-log.c | 19 ++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> >
> > This patch solves a regression introduced by fixing patch 40. I'm
> > sorry for the confusing patch numbering.
>
> Hum, how does patch 40 can cause this?
> And is it before the fixup or after?
>
> >
> > diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> > index 4e72794342c0..629e605cd62d 100644
> > --- a/fs/btrfs/tree-log.c
> > +++ b/fs/btrfs/tree-log.c
> > @@ -3120,6 +3120,14 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
> >  */
> > blk_start_plug(&plug);
> > ret = btrfs_write_marked_extents(fs_info, &log->dirty_log_pages, 
> > mark);
> > +   /*
> > +* There is a hole writing out the extents and cannot proceed it on
> > +* zoned filesystem, which require sequential writing. We can
>
> require -> requires
>
> > +* ignore the error for now, since we don't wait for completion for
> > +* now.
>
> So why can we ignore the error for now?
> Why not just bail out here and mark the log for full commit? (without
> a transaction abort)
>
> > +*/
> > +   if (ret == -EAGAIN)
> > +   ret = 0;

Thinking again about this, it would be safer, and self-documenting to
check here that we are in zoned mode:

if (ret == -EAGAIN && is_zoned)
ret = 0;

Because if we start to get -EAGAIN here one day, from non-zoned code,
we risk not writing out some extent buffer and getting a corrupt log,
which may be very hard to find.
With that additional check in place, we'll end up aborting the
transaction with -EAGAIN and notice the problem much sooner.

> > if (ret) {
> > blk_finish_plug(&plug);
> > btrfs_abort_transaction(trans, ret);
> > @@ -3229,7 +3237,16 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
> >  &log_root_tree->dirty_log_pages,
> >  EXTENT_DIRTY | EXTENT_NEW);
> > blk_finish_plug(&plug);
> > -   if (ret) {
> > +   /*
> > +* There is a hole in the extents, and failed to sequential write
> > +* on zoned filesystem. We cannot wait for this write outs, sinc it
>
> this -> these
>
> > +* cause a deadlock. Bail out to the full commit, instead.
> > +*/
> > +   if (ret == -EAGAIN) {

I would add "&& is_zoned" here too.

Thanks.


> > +   btrfs_wait_tree_log_extents(log, mark);
> > +   mutex_unlock(&log_root_tree->log_mutex);
> > +   goto out_wake_log_root;
>
> Must also call btrfs_set_log_full_commit(trans);
>
> Thanks.
>
> > +   } else if (ret) {
> > btrfs_set_log_full_commit(trans);
> > btrfs_abort_transaction(trans, ret);
> > mutex_unlock(&log_root_tree->log_mutex);
> > --
> > 2.30.0
> >
>
>
> --
> Filipe David Manana,
>
> “Whether you think you can, or you think you can't — you're right.”



--
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”


Re: [PATCH v15 43/43] btrfs: zoned: deal with holes writing out tree-log pages

2021-02-05 Thread Naohiro Aota
On Fri, Feb 05, 2021 at 02:19:50PM +, Filipe Manana wrote:
> On Fri, Feb 5, 2021 at 11:49 AM Filipe Manana  wrote:
> >
> > On Fri, Feb 5, 2021 at 9:26 AM Naohiro Aota  wrote:
> > >
> > > Since the zoned filesystem requires sequential write out of metadata, we
> > > cannot proceed with a hole in tree-log pages. When such a hole exists,
> > > btree_write_cache_pages() will return -EAGAIN. We cannot wait for the 
> > > range
> > > to be written, because it will cause a deadlock. So, let's bail out to a
> > > full commit in this case.
> > >
> > > Cc: Filipe Manana 
> > > Signed-off-by: Naohiro Aota 
> > > ---
> > >  fs/btrfs/tree-log.c | 19 ++-
> > >  1 file changed, 18 insertions(+), 1 deletion(-)
> > >
> > > This patch solves a regression introduced by fixing patch 40. I'm
> > > sorry for the confusing patch numbering.
> >
> > Hum, how does patch 40 can cause this?
> > And is it before the fixup or after?
> >
> > >
> > > diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> > > index 4e72794342c0..629e605cd62d 100644
> > > --- a/fs/btrfs/tree-log.c
> > > +++ b/fs/btrfs/tree-log.c
> > > @@ -3120,6 +3120,14 @@ int btrfs_sync_log(struct btrfs_trans_handle 
> > > *trans,
> > >  */
> > > blk_start_plug(&plug);
> > > ret = btrfs_write_marked_extents(fs_info, &log->dirty_log_pages, 
> > > mark);
> > > +   /*
> > > +* There is a hole writing out the extents and cannot proceed it 
> > > on
> > > +* zoned filesystem, which require sequential writing. We can
> >
> > require -> requires
> >
> > > +* ignore the error for now, since we don't wait for completion 
> > > for
> > > +* now.
> >
> > So why can we ignore the error for now?
> > Why not just bail out here and mark the log for full commit? (without
> > a transaction abort)
> >
> > > +*/
> > > +   if (ret == -EAGAIN)
> > > +   ret = 0;
> 
> Thinking again about this, it would be safer, and self-documenting to
> check here that we are in zoned mode:
> 
> if (ret == -EAGAIN && is_zoned)
> ret = 0;
> 
> Because if we start to get -EAGAIN here one day, from non-zoned code,
> we risk not writing out some extent buffer and getting a corrupt log,
> which may be very hard to find.
> With that additional check in place, we'll end up aborting the
> transaction with -EAGAIN and notice the problem much sooner.

Yeah, I agree.

I'll post a new version with the comments revised and using "if (ret
== -EAGAIN && btrfs_is_zoned(fs_info))".

> > > if (ret) {
> > > blk_finish_plug(&plug);
> > > btrfs_abort_transaction(trans, ret);
> > > @@ -3229,7 +3237,16 @@ int btrfs_sync_log(struct btrfs_trans_handle 
> > > *trans,
> > >  &log_root_tree->dirty_log_pages,
> > >  EXTENT_DIRTY | EXTENT_NEW);
> > > blk_finish_plug(&plug);
> > > -   if (ret) {
> > > +   /*
> > > +* There is a hole in the extents, and failed to sequential write
> > > +* on zoned filesystem. We cannot wait for this write outs, sinc 
> > > it
> >
> > this -> these
> >
> > > +* cause a deadlock. Bail out to the full commit, instead.
> > > +*/
> > > +   if (ret == -EAGAIN) {
> 
> I would add "&& is_zoned" here too.
> 
> Thanks.
>
> 
> > > +   btrfs_wait_tree_log_extents(log, mark);
> > > +   mutex_unlock(&log_root_tree->log_mutex);
> > > +   goto out_wake_log_root;
> >
> > Must also call btrfs_set_log_full_commit(trans);
> >
> > Thanks.
> >
> > > +   } else if (ret) {
> > > btrfs_set_log_full_commit(trans);
> > > btrfs_abort_transaction(trans, ret);
> > > mutex_unlock(&log_root_tree->log_mutex);
> > > --
> > > 2.30.0
> > >
> >
> >
> > --
> > Filipe David Manana,
> >
> > “Whether you think you can, or you think you can't — you're right.”
> 
> 
> 
> --
> Filipe David Manana,
> 
> “Whether you think you can, or you think you can't — you're right.”


Re: [PATCH v15 43/43] btrfs: zoned: deal with holes writing out tree-log pages

2021-02-05 Thread Naohiro Aota
On Fri, Feb 05, 2021 at 11:49:05AM +, Filipe Manana wrote:
> On Fri, Feb 5, 2021 at 9:26 AM Naohiro Aota  wrote:
> >
> > Since the zoned filesystem requires sequential write out of metadata, we
> > cannot proceed with a hole in tree-log pages. When such a hole exists,
> > btree_write_cache_pages() will return -EAGAIN. We cannot wait for the range
> > to be written, because it will cause a deadlock. So, let's bail out to a
> > full commit in this case.
> >
> > Cc: Filipe Manana 
> > Signed-off-by: Naohiro Aota 
> > ---
> >  fs/btrfs/tree-log.c | 19 ++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> >
> > This patch solves a regression introduced by fixing patch 40. I'm
> > sorry for the confusing patch numbering.
> 
> Hum, how does patch 40 can cause this?
> And is it before the fixup or after?

With pre-5.10 code base + zoned series at that time, it passed
xfstests without this patch.

With current code base + zoned series without the fixup for patch 40,
it also passed the tests, because we are mostly bailing out to a full
commit.

The fixup now stressed the new fsync code on zoned mode and revealed
an issue to have -EAGAIN from btrfs_write_marked_extents(). This error
happens when a concurrent transaction commit is writing a dirty extent
in this tree-log commit. This issue didn't occur previously because of
a longer critical section, I guess.

> 
> >
> > diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> > index 4e72794342c0..629e605cd62d 100644
> > --- a/fs/btrfs/tree-log.c
> > +++ b/fs/btrfs/tree-log.c
> > @@ -3120,6 +3120,14 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
> >  */
> > blk_start_plug(&plug);
> > ret = btrfs_write_marked_extents(fs_info, &log->dirty_log_pages, 
> > mark);
> > +   /*
> > +* There is a hole writing out the extents and cannot proceed it on
> > +* zoned filesystem, which require sequential writing. We can
> 
> require -> requires
> 
> > +* ignore the error for now, since we don't wait for completion for
> > +* now.
> 
> So why can we ignore the error for now?
> Why not just bail out here and mark the log for full commit? (without
> a transaction abort)

As described above, -EAGAIN happens when a concurrent process writes
out an extent buffer of this tree-log commit. This concurrent write
out will fill a hole for us, so the next write out might
succeed. Indeed we can bail out here, but I opted to try the next
write.

> > +*/
> > +   if (ret == -EAGAIN)
> > +   ret = 0;
> > if (ret) {
> > blk_finish_plug(&plug);
> > btrfs_abort_transaction(trans, ret);
> > @@ -3229,7 +3237,16 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
> >  &log_root_tree->dirty_log_pages,
> >  EXTENT_DIRTY | EXTENT_NEW);
> > blk_finish_plug(&plug);
> > -   if (ret) {
> > +   /*
> > +* There is a hole in the extents, and failed to sequential write
> > +* on zoned filesystem. We cannot wait for this write outs, sinc it
> 
> this -> these
> 
> > +* cause a deadlock. Bail out to the full commit, instead.
> > +*/
> > +   if (ret == -EAGAIN) {
> > +   btrfs_wait_tree_log_extents(log, mark);
> > +   mutex_unlock(&log_root_tree->log_mutex);
> > +   goto out_wake_log_root;
> 
> Must also call btrfs_set_log_full_commit(trans);

Oops, I missed this one.

> Thanks.
> 
> > +   } else if (ret) {
> > btrfs_set_log_full_commit(trans);
> > btrfs_abort_transaction(trans, ret);
> > mutex_unlock(&log_root_tree->log_mutex);
> > --
> > 2.30.0
> >
> 
> 
> -- 
> Filipe David Manana,
> 
> “Whether you think you can, or you think you can't — you're right.”


Re: [PATCH v15 43/43] btrfs: zoned: deal with holes writing out tree-log pages

2021-02-05 Thread Filipe Manana
On Fri, Feb 5, 2021 at 9:26 AM Naohiro Aota  wrote:
>
> Since the zoned filesystem requires sequential write out of metadata, we
> cannot proceed with a hole in tree-log pages. When such a hole exists,
> btree_write_cache_pages() will return -EAGAIN. We cannot wait for the range
> to be written, because it will cause a deadlock. So, let's bail out to a
> full commit in this case.
>
> Cc: Filipe Manana 
> Signed-off-by: Naohiro Aota 
> ---
>  fs/btrfs/tree-log.c | 19 ++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
>
> This patch solves a regression introduced by fixing patch 40. I'm
> sorry for the confusing patch numbering.

Hum, how does patch 40 can cause this?
And is it before the fixup or after?

>
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 4e72794342c0..629e605cd62d 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -3120,6 +3120,14 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
>  */
> blk_start_plug(&plug);
> ret = btrfs_write_marked_extents(fs_info, &log->dirty_log_pages, 
> mark);
> +   /*
> +* There is a hole writing out the extents and cannot proceed it on
> +* zoned filesystem, which require sequential writing. We can

require -> requires

> +* ignore the error for now, since we don't wait for completion for
> +* now.

So why can we ignore the error for now?
Why not just bail out here and mark the log for full commit? (without
a transaction abort)

> +*/
> +   if (ret == -EAGAIN)
> +   ret = 0;
> if (ret) {
> blk_finish_plug(&plug);
> btrfs_abort_transaction(trans, ret);
> @@ -3229,7 +3237,16 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
>  &log_root_tree->dirty_log_pages,
>  EXTENT_DIRTY | EXTENT_NEW);
> blk_finish_plug(&plug);
> -   if (ret) {
> +   /*
> +* There is a hole in the extents, and failed to sequential write
> +* on zoned filesystem. We cannot wait for this write outs, sinc it

this -> these

> +* cause a deadlock. Bail out to the full commit, instead.
> +*/
> +   if (ret == -EAGAIN) {
> +   btrfs_wait_tree_log_extents(log, mark);
> +   mutex_unlock(&log_root_tree->log_mutex);
> +   goto out_wake_log_root;

Must also call btrfs_set_log_full_commit(trans);

Thanks.

> +   } else if (ret) {
> btrfs_set_log_full_commit(trans);
> btrfs_abort_transaction(trans, ret);
> mutex_unlock(&log_root_tree->log_mutex);
> --
> 2.30.0
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”


[PATCH v15 43/43] btrfs: zoned: deal with holes writing out tree-log pages

2021-02-05 Thread Naohiro Aota
Since the zoned filesystem requires sequential write out of metadata, we
cannot proceed with a hole in tree-log pages. When such a hole exists,
btree_write_cache_pages() will return -EAGAIN. We cannot wait for the range
to be written, because it will cause a deadlock. So, let's bail out to a
full commit in this case.

Cc: Filipe Manana 
Signed-off-by: Naohiro Aota 
---
 fs/btrfs/tree-log.c | 19 ++-
 1 file changed, 18 insertions(+), 1 deletion(-)

This patch solves a regression introduced by fixing patch 40. I'm
sorry for the confusing patch numbering.

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 4e72794342c0..629e605cd62d 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -3120,6 +3120,14 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 */
blk_start_plug(&plug);
ret = btrfs_write_marked_extents(fs_info, &log->dirty_log_pages, mark);
+   /*
+* There is a hole writing out the extents and cannot proceed it on
+* zoned filesystem, which require sequential writing. We can
+* ignore the error for now, since we don't wait for completion for
+* now.
+*/
+   if (ret == -EAGAIN)
+   ret = 0;
if (ret) {
blk_finish_plug(&plug);
btrfs_abort_transaction(trans, ret);
@@ -3229,7 +3237,16 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 &log_root_tree->dirty_log_pages,
 EXTENT_DIRTY | EXTENT_NEW);
blk_finish_plug(&plug);
-   if (ret) {
+   /*
+* There is a hole in the extents, and failed to sequential write
+* on zoned filesystem. We cannot wait for this write outs, sinc it
+* cause a deadlock. Bail out to the full commit, instead.
+*/
+   if (ret == -EAGAIN) {
+   btrfs_wait_tree_log_extents(log, mark);
+   mutex_unlock(&log_root_tree->log_mutex);
+   goto out_wake_log_root;
+   } else if (ret) {
btrfs_set_log_full_commit(trans);
btrfs_abort_transaction(trans, ret);
mutex_unlock(&log_root_tree->log_mutex);
-- 
2.30.0