On Tue, Sep 22, 2015 at 11:06 AM, Zhao Lei <zhao...@cn.fujitsu.com> wrote:
> Hi, Filipe David Manana
>
> Thanks for review this patch.
>
>> -----Original Message-----
>> From: Filipe David Manana [mailto:fdman...@gmail.com]
>> Sent: Monday, September 21, 2015 9:27 PM
>> To: Zhao Lei <zhao...@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 <zhao...@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,054,
>> > 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
>
> 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.
Well it's a hack that besides not being very logical it's also
incomplete and unreliable: preventing the cleaner kthread (automatic
removal of unused block groups) from deleting the last block group of
a kind doesn't prevent the block group from being deleted by a
balance, just as the new regression xfstest does...
In other words, the problem existed before we had automatic removal of
unused block groups, but just less likely for someone to run into it.
So please lets have a real fix for this problem. Seems like we're
losing raid profile type somewhere when allocating a new block group.
thanks
>
> Thanks
> Zhaolei
>
>> thanks
>>
>> >
>> > Test:
>> > Test by above script, and confirmed the logic by debug output.
>> >
>> > Signed-off-by: Zhao Lei <zhao...@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 majord...@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."
>
--
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
