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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxx> 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@xxxxxxxxx>
> > Signed-off-by: Naohiro Aota <naohiro.aota@xxxxxxx>
> > ---
> >  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.”



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux