Re: [PATCH 2/2] btrfs: Fix lost-data-profile caused by balance bg

[Filipe Manana <fdmanana@gmail.com>]

On Wed, Sep 30, 2015 at 9:26 AM, Zhao Lei <zhaolei@cn.fujitsu.com> wrote:
> Hi, Filipe Manana
>
>> -----Original Message-----
>> From: Filipe Manana [mailto:fdmanana@gmail.com]
>> Sent: Wednesday, September 30, 2015 3:41 PM
>> To: Zhao Lei <zhaolei@cn.fujitsu.com>
>> Cc: linux-btrfs@vger.kernel.org
>> Subject: Re: [PATCH 2/2] btrfs: Fix lost-data-profile caused by balance bg
>>
>> On Wed, Sep 30, 2015 at 5:20 AM, Zhao Lei <zhaolei@cn.fujitsu.com> wrote:
>> > Hi, Filipe Manana
>> >
>> > Thanks for reviewing.
>> >
>> >> -----Original Message-----
>> >> From: Filipe Manana [mailto:fdmanana@gmail.com]
>> >> Sent: Tuesday, September 29, 2015 11:48 PM
>> >> To: Zhao Lei <zhaolei@cn.fujitsu.com>
>> >> Cc: linux-btrfs@vger.kernel.org
>> >> Subject: Re: [PATCH 2/2] btrfs: Fix lost-data-profile caused by
>> >> balance bg
>> >>
>> >> On Tue, Sep 29, 2015 at 2:51 PM, Zhao Lei <zhaolei@cn.fujitsu.com> 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"
>> >> >  btrfs balance start -dusage=0 $TEST_DIR  btrfs filesystem usage
>> >> > $TEST_DIR
>> >> >
>> >> >  dd if=/dev/zero of="$TEST_DIR"/file count=100  btrfs filesystem
>> >> > usage $TEST_DIR
>> >> >
>> >> > Result:
>> >> >  We can see "no data chunk" in first "btrfs filesystem usage":
>> >> >  # btrfs filesystem usage $TEST_DIR
>> >> >  Overall:
>> >> >     ...
>> >> >  Metadata,single: Size:8.00MiB, Used:0.00B
>> >> >     /dev/vdg        8.00MiB
>> >> >  Metadata,RAID1: Size:122.88MiB, Used:112.00KiB
>> >> >     /dev/vdg      122.88MiB
>> >> >     /dev/vdh      122.88MiB
>> >> >  System,single: Size:4.00MiB, Used:0.00B
>> >> >     /dev/vdg        4.00MiB
>> >> >  System,RAID1: Size:8.00MiB, Used:16.00KiB
>> >> >     /dev/vdg        8.00MiB
>> >> >     /dev/vdh        8.00MiB
>> >> >  Unallocated:
>> >> >     /dev/vdg        1.06GiB
>> >> >     /dev/vdh        1.07GiB
>> >> >
>> >> >  And "data chunks changed from raid1 to single" in second  "btrfs
>> >> > filesystem usage":
>> >> >  # btrfs filesystem usage $TEST_DIR
>> >> >  Overall:
>> >> >     ...
>> >> >  Data,single: Size:256.00MiB, Used:0.00B
>> >> >     /dev/vdh      256.00MiB
>> >> >  Metadata,single: Size:8.00MiB, Used:0.00B
>> >> >     /dev/vdg        8.00MiB
>> >> >  Metadata,RAID1: Size:122.88MiB, Used:112.00KiB
>> >> >     /dev/vdg      122.88MiB
>> >> >     /dev/vdh      122.88MiB
>> >> >  System,single: Size:4.00MiB, Used:0.00B
>> >> >     /dev/vdg        4.00MiB
>> >> >  System,RAID1: Size:8.00MiB, Used:16.00KiB
>> >> >     /dev/vdg        8.00MiB
>> >> >     /dev/vdh        8.00MiB
>> >> >  Unallocated:
>> >> >     /dev/vdg        1.06GiB
>> >> >     /dev/vdh      841.92MiB
>> >> >
>> >> > Reason:
>> >> >  btrfs balance delete last data chunk in case of no data in  the
>> >> > filesystem, then we can see "no data chunk" by "fi usage"
>> >> >  command.
>> >> >
>> >> >  And when we do write operation to fs, the only available data
>> >> > profile is 0x0, result is all new chunks are allocated single type.
>> >> >
>> >> > Fix:
>> >> >  Allocate a data chunk explicitly in balance operation, to reserve
>> >> > at least one data chunk in the filesystem.
>> >>
>> >> Allocate a data chunk explicitly to ensure we don't lose the raid profile for
>> data.
>> >>
>> >
>> > Thanks, will change in v2.
>> >
>> >> >
>> >> > Test:
>> >> >  Test by above script, and confirmed the logic by debug output.
>> >> >
>> >> > Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
>> >> > ---
>> >> >  fs/btrfs/volumes.c | 19 +++++++++++++++++++
>> >> >  1 file changed, 19 insertions(+)
>> >> >
>> >> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index
>> >> > 6fc73586..3d5e41e 100644
>> >> > --- a/fs/btrfs/volumes.c
>> >> > +++ b/fs/btrfs/volumes.c
>> >> > @@ -3277,6 +3277,7 @@ static int __btrfs_balance(struct
>> >> > btrfs_fs_info
>> >> *fs_info)
>> >> >         u64 limit_data = bctl->data.limit;
>> >> >         u64 limit_meta = bctl->meta.limit;
>> >> >         u64 limit_sys = bctl->sys.limit;
>> >> > +       int chunk_reserved = 0;
>> >> >
>> >> >         /* step one make some room on all the devices */
>> >> >         devices = &fs_info->fs_devices->devices; @@ -3387,6
>> >> > +3388,24 @@ again:
>> >> >                         goto loop;
>> >> >                 }
>> >> >
>> >> > +               if (!chunk_reserved) {
>> >> > +                       trans = btrfs_start_transaction(chunk_root,
>> 0);
>> >> > +                       if (IS_ERR(trans)) {
>> >> > +
>> >> mutex_unlock(&fs_info->delete_unused_bgs_mutex);
>> >> > +                               ret = PTR_ERR(trans);
>> >> > +                               goto error;
>> >> > +                       }
>> >> > +
>> >> > +                       ret = btrfs_force_chunk_alloc(trans,
>> >> > + chunk_root, 1);
>> >>
>> >> Can we please use the symbol BTRFS_BLOCK_GROUP_DATA instead of 1?
>> >>
>> > Thanks, will change in v2.
>> >
>> >
>> >> > +                       if (ret < 0) {
>> >> > +
>> >> mutex_unlock(&fs_info->delete_unused_bgs_mutex);
>> >> > +                               goto error;
>> >> > +                       }
>> >> > +
>> >> > +                       btrfs_end_transaction(trans, chunk_root);
>> >> > +                       chunk_reserved = 1;
>> >> > +               }
>> >>
>> >> Can we do this logic only if the block group is a data one? I.e. no
>> >> point allocating a data block group if our balance only touches
>> >> metadata ones (due to some filter for e.g.).
>> >>
>> > Thanks for point out it, will change in v2.
>>
>> I find it somewhat awkward that we always allocate a new data block group no
>> matter what. Some balance operations that used to succeed in the past may
>> now fail with -ENOSPC for example...
>>
>> How about making this simpler and not so special for data block groups, like the
>> following (compile tested only):
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 644e070..067b1eb
>> 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -2774,6 +2774,8 @@ static int btrfs_relocate_chunk(struct btrfs_root
>> *root, u64 chunk_offset)
>>         struct btrfs_root *extent_root;
>>         struct btrfs_trans_handle *trans;
>>         int ret;
>> +       struct btrfs_block_group_cache *bg;
>> +       bool remove = true;
>>
>>         root = root->fs_info->chunk_root;
>>         extent_root = root->fs_info->extent_root; @@ -2803,6 +2805,23
>> @@ static int btrfs_relocate_chunk(struct btrfs_root *root, u64 chunk_offset)
>>         if (ret)
>>                 return ret;
>>
>> +       bg = btrfs_lookup_block_group(root->fs_info, chunk_offset);
>> +       ASSERT(bg);
>> +       down_read(&bg->space_info->groups_sem);
>> +       /*
>> +        * Do not remove the last block group of a kind to prevent losing
>> +        * raid profile information for future chunk allocations.
>> +        */
>> +       if (bg->list.next == bg->list.prev)
>> +               remove = false;
>> +       up_read(&bg->space_info->groups_sem);
>> +       if (!remove)
>> +               btrfs_dec_block_group_ro(extent_root, bg);
>> +       btrfs_put_block_group(bg);
>> +
>> +       if (!remove)
>> +               return 0;
>> +
>>         trans = btrfs_start_transaction(root, 0);
>>         if (IS_ERR(trans)) {
>>                 ret = PTR_ERR(trans);
>>
>
> Thanks for your detailed review!
>
> Reason of creating new bg is:
> Balance operation may be used to change raid-profile of the filesystem.
> If we want to change filesystem form raid1 to raid5, we need:
> 1: delete all raid1 bgs
> 2: move all data to raid5 bg if data exist in filesystem
> 3: reserve only one blank raid5 bg if no data in filesystem

I see. Thanks.

>
> If we use similar operation as patch 1/2(not delete lattest blockgroup),
> we'll have following problem:
> 1: the old raid1 blockgroups are not all-deleted
> 2: if no data in filesystem, the profile will not changed from raid1 to raid5.
>
> I understand your worry of "NO_SPACE" when create additional bg,
> and I also considered it in making patch, but I choose to use this way because:
> 1: balance operation had check free space before operation
>   (In front of __btrfs_balance)
> 2: for filesystem with data, we have to create target-chunk in balance operation,
>   this patch only make "creating-chunk" earlier, and the created chunk will be used
>   to save data in balance operation, and reduce one chunk-allocate in balance operation.
> 3: for a blank filesystem, it is necessary to create a blank chunk with of target raid profile.
>   the old code hadn't create it, and this patch created. It is right operation even if
>   cause no-space. Actually, create a chunk in empty filesystem rarely cause no-space,
>   plus we have free-space check in 1.
> 4: 1~3 ensure this patch rarely cause additional no-space problem in logic.
>   And if it really caused additional no-space(if out of my consideration),
>   We can change code of 1 to avoid it.
>
> Thanks
> Zhaolei
>
>> (also at https://friendpaste.com/5IeAIIzBv3oKhureKfvjwm)
>>
>> thanks
>>
>>
>> >
>> >> Also, can't this be ineffective when the data block group we allocate
>> >> ends up with a key  in the chunk_root that is lower than the key we
>> >> found in the current iteration? I.e., key found in current iteration
>> >> has object id B, we allocate a new block group which gets a  key with
>> >> object id A, where A < B and the next iteration of our loop sees key
>> >> A, deletes the respective block group A if it's empty. In the end we
>> >> have no data block groups, assuming that before A there are no other
>> non-empty data block groups.
>> >> Your example works because there's no free space before the offset
>> >> where the chunk starts in the device.
>> >>
>> > New chunk will always use increased logic address, even if there are
>> > "hole" before, so A's logic address will always >B.
>> > And current code of balance also use this feature to avoid balance
>> > new-created chunks(which was created by balance operation itself).
>> >
>> > Thanks
>> > Zhaolei
>> >
>> >> thanks
>> >>
>> >> > +
>> >> >                 ret = btrfs_relocate_chunk(chunk_root,
>> >> >                                            found_key.offset);
>> >> >                 mutex_unlock(&fs_info->delete_unused_bgs_mutex);
>> >> > --
>> >> > 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."
>> >
>>
>>
>>
>> --
>> 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."