On Wed, Sep 30, 2015 at 11:06 AM, Zhao Lei <[email protected]> wrote:
> Hi, Filipe Manana
>
>> -----Original Message-----
>> From: [email protected]
>> [mailto:[email protected]] On Behalf Of Filipe Manana
>> Sent: Wednesday, September 30, 2015 3:43 PM
>> To: Zhao Lei <[email protected]>
>> Cc: [email protected]
>> Subject: Re: [PATCH 1/2] btrfs: Fix lost-data-profile caused by auto
>> removing bg
>>
>> On Tue, Sep 29, 2015 at 2:51 PM, Zhao Lei <[email protected]> wrote:
>> > Reproduce:
>> > (In integration-4.3 branch)
>> >
>> > 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
>> >
>> > We can see the data chunk changed from raid1 to single:
>> > # btrfs filesystem usage $TEST_DIR
>> > Data,single: Size:8.00MiB, Used:0.00B
>> > /dev/vdg 8.00MiB
>> > #
>> >
>> > Reason:
>> > When a empty filesystem mount with -o nospace_cache, the last data
>> > blockgroup will be auto-removed in umount.
>> >
>> > Then if we mount it again, there is no data chunk in the filesystem,
>> > so the only available data profile is 0x0, result is all new chunks
>> > are created as single type.
>> >
>> > Fix:
>> > Don't auto-delete last blockgroup for a raid type.
>> >
>> > Test:
>> > Test by above script, and confirmed the logic by debug output.
>> >
>> > Signed-off-by: Zhao Lei <[email protected]>
>> > ---
>> > 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
>> > 79a5bd9..3505649 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)
>> > + {
>>
>> This isn't race free. The list block_group->list is protected by the
>> groups_sem
>> semaphore. Need to take before doing this check.
>
> Thanks for pointing out this.
>
>> You can do that in the "if" statement below this one, where we're holding
>> &space_info->groups_sem [1]
>>
> It is hard to do check in btrfs_remove_block_group(), because it is common
> function used by both balance and auto-remove bg.
I didn't say to move it to or touch btrfs_remove_block_group() at all.
What I meant is to do check still inside btrfs_delete_unused_bgs but
in the other "if" statement that is done while holding the groups
semaphore, i.e.:
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 9f96042..627ac3b 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -10022,7 +10022,8 @@ void btrfs_delete_unused_bgs(struct
btrfs_fs_info *fs_info)
spin_lock(&block_group->lock);
if (block_group->reserved ||
btrfs_block_group_used(&block_group->item) ||
- block_group->ro) {
+ block_group->ro ||
+ lock_group->list.next == block_group->list.prev) {
/*
* We want to bail if we made new allocations or have
* outstanding allocations in this block group. We do
>
> For balance operation, we can remove lattest bg in some case, or we need
> add additional argument to separate these two operation(complex).
>
> So I decided to take groups_sem semaphore in place of checking.
Not sure I follow. The problem I was pointing is doing the check
without holding the groups semaphore.
> Thanks for notice this lock problem.
>
> btw, could I add your signed-off-by or reviewed-by in patch 2/2?
Yeah, once I see the updated version I can add a reviewed-by tag.
thanks
>
> Thanks
> Zhaolei
>
>> thanks
>>
>> [1]
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/fs/btrfs/exte
>> nt-tree.c?id=refs/tags/v4.3-rc3#n10021
>>
>> > 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 [email protected] 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 [email protected] 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
