| Message ID | 20210205092635.i6w3c7brawlv6pgs@naota-xeon (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: zoned block device support | expand |
On Fri, Feb 5, 2021 at 9:26 AM Naohiro Aota <naohiro.aota@wdc.com> 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 <fdmanana@gmail.com> > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> > --- > 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 >
On Fri, Feb 05, 2021 at 11:49:05AM +0000, Filipe Manana wrote: > On Fri, Feb 5, 2021 at 9:26 AM Naohiro Aota <naohiro.aota@wdc.com> 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 <fdmanana@gmail.com> > > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> > > --- > > 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.”
On Fri, Feb 5, 2021 at 12:55 PM Naohiro Aota <naohiro.aota@wdc.com> wrote: > > On Fri, Feb 05, 2021 at 11:49:05AM +0000, Filipe Manana wrote: > > On Fri, Feb 5, 2021 at 9:26 AM Naohiro Aota <naohiro.aota@wdc.com> 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 <fdmanana@gmail.com> > > > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> > > > --- > > > 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.”
On Fri, Feb 5, 2021 at 11:49 AM Filipe Manana <fdmanana@gmail.com> wrote: > > On Fri, Feb 5, 2021 at 9:26 AM Naohiro Aota <naohiro.aota@wdc.com> 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 <fdmanana@gmail.com> > > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> > > --- > > 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.”
On Fri, Feb 05, 2021 at 02:19:50PM +0000, Filipe Manana wrote: > On Fri, Feb 5, 2021 at 11:49 AM Filipe Manana <fdmanana@gmail.com> wrote: > > > > On Fri, Feb 5, 2021 at 9:26 AM Naohiro Aota <naohiro.aota@wdc.com> 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 <fdmanana@gmail.com> > > > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> > > > --- > > > 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.”
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);
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 <fdmanana@gmail.com> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> --- 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.