RE: [PATCH] btrfs: Fix no space bug caused by removing bg

[Zhao Lei <zhaolei@cn.fujitsu.com>]

Hi, Filipe David Manana

> -----Original Message-----
> From: linux-btrfs-owner@vger.kernel.org
> [mailto:linux-btrfs-owner@vger.kernel.org] On Behalf Of Zhao Lei
> Sent: Tuesday, September 22, 2015 6:06 PM
> To: fdmanana@gmail.com
> Cc: linux-btrfs@vger.kernel.org
> Subject: RE: [PATCH] btrfs: Fix no space bug caused by removing bg
> 
> Hi, Filipe David Manana
> 
> Thanks for review this patch.
> 
> > -----Original Message-----
> > From: Filipe David Manana [mailto:fdmanana@gmail.com]
> > Sent: Monday, September 21, 2015 9:27 PM
> > To: Zhao Lei <zhaolei@cn.fujitsu.com>
> > Cc: linux-btrfs@vger.kernel.org
> > Subject: Re: [PATCH] btrfs: Fix no space bug caused by removing bg
> >
> > On Mon, Sep 21, 2015 at 1:59 PM, Zhao Lei <zhaolei@cn.fujitsu.com> wrote:
> > > btrfs in v4.3-rc1 failed many xfstests items with '-o nospace_cache'
> > > mount option.
> > >
> > > Failed cases are:
> > >
> > > btrfs/008,016,019,020,026,027,028,029,031,041,046,048,050,051,053,05
> > > 4,
> > >  077,083,084,087,092,094
> >
> > Hi Zhao,
> >
> > So far I tried a few of those against Chris' integration-4.3 branch
> > (same btrfs code as 4.3-rc1):
> >
> > MOUNT_OPTIONS="-o nospace_cache" ./check btrfs/008 btrfs/016 btrfs/019
> > btrfs/020
> > FSTYP         -- btrfs
> > PLATFORM      -- Linux/x86_64 debian3 4.2.0-rc5-btrfs-next-12+
> > MKFS_OPTIONS  -- /dev/sdc
> > MOUNT_OPTIONS -- -o nospace_cache /dev/sdc
> > /home/fdmanana/btrfs-tests/scratch_1
> >
> > btrfs/008 2s ... 1s
> > btrfs/016 4s ... 3s
> > btrfs/019 4s ... 2s
> > btrfs/020 2s ... 1s
> > Ran: btrfs/008 btrfs/016 btrfs/019 btrfs/020 Passed all 4 tests
> >
> > And none of the tests failed...
> >
> Sorry I hadn't paste detail of my test command.
> 
> It is from a coincidence operation which is some different with standard
> steps(as yours), I mount fs with -o no_space_cache manually without set
> MOUNT_OPT, then xfstests entered into a special path, and triggered the bug:
>   export TEST_DEV='/dev/sdb5'
>   export TEST_DIR='/var/ltf/tester/mnt'
>   mkdir -p '/var/ltf/tester/mnt'
> 
>   export SCRATCH_DEV_POOL='/dev/sdb6 /dev/sdb7 /dev/sdb8 /dev/sdb9
> /dev/sdb10 /dev/sdb11'
>   export SCRATCH_MNT='/var/ltf/tester/scratch_mnt'
>   mkdir -p '/var/ltf/tester/scratch_mnt'
> 
>   export DIFF_LENGTH=0
> 
>   mkfs.btrfs -f "$TEST_DEV"
>   mount -o nospace_cache "$TEST_DEV" "$TEST_DIR"
> 
>   ./check generic/014
> 
> Result:
>   FSTYP         -- btrfs
>   PLATFORM      -- Linux/x86_64 lenovo
> 4.3.0-rc2_HEAD_1f93e4a96c9109378204c147b3eec0d0e8100fde_
>   MKFS_OPTIONS  -- /dev/sdb6
>   MOUNT_OPTIONS -- /dev/sdb6 /var/ltf/tester/scratch_mnt
> 
>   generic/014 0s ... - output mismatch (see
> /var/lib/xfstests/results//generic/014.out.bad)
>       --- tests/generic/014.out   2015-09-22 17:46:13.855391451 +0800
>       +++ /var/lib/xfstests/results//generic/014.out.bad  2015-09-22
> 17:57:06.446095748 +0800
>       @@ -3,4 +3,5 @@
>        ------
>        test 1
>        ------
>       -OK
>       +truncfile returned 1 : "write: No space left on device
>       +Seed = 1442915826 (use "-s 1442915826" to re-execute this test)"
>   Ran: generic/014
>   Failures: generic/014
>   Failed 1 of 1 tests
> 

Plus, by retest, the xfstests fail also happened in standard steps in my node:
(with newest xfstests)

  # btrfs --version
  btrfs-progs v4.2
  # uname -a
  Linux lenovo 4.3.0-rc2_HEAD_1f93e4a96c9109378204c147b3eec0d0e8100fde_ #1 SMP Mon Sep 21 06:34:49 CST 2015 x86_64 x86_64 x86_64 GNU/Linux
  # MOUNT_OPTIONS="-o nospace_cache" ./check btrfs/008
  FSTYP         -- btrfs
  PLATFORM      -- Linux/x86_64 lenovo 4.3.0-rc2_HEAD_1f93e4a96c9109378204c147b3eec0d0e8100fde_
  MKFS_OPTIONS  -- /dev/sdb6
  MOUNT_OPTIONS -- -o nospace_cache /dev/sdb6 /var/ltf/tester/scratch_mnt

  btrfs/008 1s ... [failed, exit status 1] - output mismatch (see /var/lib/xfstests/results//btrfs/008.out.bad)
      --- tests/btrfs/008.out     2015-09-22 17:46:12.530391386 +0800
      +++ /var/lib/xfstests/results//btrfs/008.out.bad    2015-09-22 18:17:24.699154708 +0800
      @@ -1,2 +1,3 @@
       QA output created by 008
      -Silence is golden
      +send failed
      +(see /var/lib/xfstests/results//btrfs/008.full for details)
  Ran: btrfs/008
  Failures: btrfs/008
  Failed 1 of 1 tests

Maybe there are some different with our nodes, but I think it is no relationship
with this bug, and need not investigate the detail reason.

Thanks
Zhaolei

> And following script is from trace result of above test.
> Maybe I can remove the xfstest description because it is not standard steps.
> 
> > >
> > > generic/004,010,014,023,024,074,075,080,086,087,089,091,092,100,112,
> > > 12
> > > 3,
> > > 124,125,126,127,131,133,192,193,198,207,208,209,213,214,215,228,239,
> > > 24
> > > 0,
> > >  246,247,248,255,263,285,306,313,316,323
> > >
> > > We can reproduce this bug with following simple command:
> > >  TEST_DEV=/dev/vdh
> > >  TEST_DIR=/mnt/tmp
> > >
> > >  umount "$TEST_DEV" >/dev/null
> > >  mkfs.btrfs -f "$TEST_DEV"
> > >  mount "$TEST_DEV" "$TEST_DIR"
> > >
> > >  umount "$TEST_DEV"
> > >  mount "$TEST_DEV" "$TEST_DIR"
> > >
> > >  cp /bin/bash $TEST_DIR
> > >
> > > Result is:
> > >  (omit previous commands)
> > >  # cp /bin/bash $TEST_DIR
> > >  cp: writing `/mnt/tmp/bash': No space left on device
> > >
> > > By bisect, we can see it is triggered by patch titled:
> > >  commit e44163e17796
> > >  ("btrfs: explictly delete unused block groups in close_ctree and
> > > ro-remount")
> > >
> > > But the wrong code is not in above patch, btrfs delete all chunks if
> > > no data in filesystem, and above patch just make it obviously.
> > >
> > > Detail reason:
> > >  1: mkfs a blank filesystem, or delete everything in filesystem
> > >  2: umount fs
> > >     (current code will delete all data chunks)
> > >  3: mount fs
> > >     Because no any data chunks, data's space_cache have no chance
> > >     to init, it means: space_info->total_bytes == 0, and
> > >     space_info->full == 1.
> >
> > Right, and that's the problem. When the space_info is initialized it
> > should never be flagged as full, otherwise any buffered write attempts
> > fail immediately with enospc instead of trying to allocate a data
> > block group (at extent-tree.c:btrfs_check_data_free_space()).
> >
> > That was fixed recently by:
> >
> > https://patchwork.kernel.org/patch/7133451/
> >
> > (with a respective test too,
> > https://patchwork.kernel.org/patch/7133471/)
> >
> 
> It can fix problem in mount, but can not fix problem of "raid-level change",
> please see below.
> 
> > >  4: do some write
> > >     Current code will ignore chunk allocate because space_info->full,
> > >     and return -ENOSPC.
> > >
> > > Fix:
> > >  Don't auto-delete last blockgroup for a raid type.
> > >  If we delete all blockgroup for a raidtype, it not only cause above
> > > bug,  but also may change filesystem to all-single in some case.
> >
> > I don't get this. Can you mention in which cases that happens and why
> > (in the commit message)?
> >
> > It isn't clear when reading the patch why we need to keep at least one
> > block of each type/profile, and seems to be a workaround for a different
> problem.
> >
> Simply speaking, if we run following command after apply your patch:
> 
>   TEST_DEV=(/dev/vdg /dev/vdh)
>   TEST_DIR=/mnt/tmp
> 
>   umount "$TEST_DEV" >/dev/null
>   mkfs.btrfs -f -d raid1 "${TEST_DEV[@]}"
>   mount -o nospace_cache "$TEST_DEV" "$TEST_DIR"
> 
>   umount "$TEST_DEV"
>   mount -o nospace_cache "$TEST_DEV" "$TEST_DIR"
> 
>   btrfs filesystem usage $TEST_DIR
> 
> The result is:
>   # btrfs filesystem usage $TEST_DIR
>   (omit)
>   Data,single: Size:8.00MiB, Used:0.00B
>      /dev/vdg        8.00MiB
>   ...
> 
> We can see data chunk is changed from raid1 to single, because if we delete all
> data chunks before mount, there are raid-type information in filesystem, and
> btrfs will use raid-type of "0x0" for new data chunk after your patch.
> 
> So, leave at least one data chunk is a simple workaround for above two bug.
> 
> Thanks
> Zhaolei
> 
> > thanks
> >
> > >
> > > Test:
> > >  Test by above script, and confirmed the logic by debug output.
> > >
> > > Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
> > > ---
> > >  fs/btrfs/extent-tree.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index
> > > 5411f0a..35cf7eb 100644
> > > --- a/fs/btrfs/extent-tree.c
> > > +++ b/fs/btrfs/extent-tree.c
> > > @@ -10012,7 +10012,8 @@ void btrfs_delete_unused_bgs(struct
> > btrfs_fs_info *fs_info)
> > >                                                bg_list);
> > >                 space_info = block_group->space_info;
> > >                 list_del_init(&block_group->bg_list);
> > > -               if (ret || btrfs_mixed_space_info(space_info)) {
> > > +               if (ret || btrfs_mixed_space_info(space_info) ||
> > > +                   block_group->list.next ==
> > > + block_group->list.prev) {
> > >                         btrfs_put_block_group(block_group);
> > >                         continue;
> > >                 }
> > > --
> > > 1.8.5.1
> > >
> > > --
> > > 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