Re: [PATCH v2 1/5] btrfs-progs: image: Refactor fixup_devices() to fixup_chunks_and_devices()
On 2018/12/4 下午6:20, David Sterba wrote:
> On Tue, Nov 27, 2018 at 04:38:24PM +0800, Qu Wenruo wrote:
>> +error:
>> +error(
>> +"failed to fix chunks and devices mapping, the fs may not be mountable: %s",
>> +strerror(-ret));
>
> Recently the sterror(error code) have been converted to %m, so I changed
> this to
>
> errno = -ret
> error("... %m");
>
Thanks for mentioning this.
I'll use this format for later patches.
Thanks,
Qu
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v2 1/5] btrfs-progs: image: Refactor fixup_devices() to fixup_chunks_and_devices()
On 2018/12/4 下午6:18, David Sterba wrote:
> On Tue, Nov 27, 2018 at 04:50:57PM +0800, Qu Wenruo wrote:
-static int fixup_devices(struct btrfs_fs_info *fs_info,
- struct mdrestore_struct *mdres, off_t dev_size)
+static int fixup_device_size(struct btrfs_trans_handle *trans,
+ struct mdrestore_struct *mdres,
+ off_t dev_size)
{
- struct btrfs_trans_handle *trans;
+ struct btrfs_fs_info *fs_info = trans->fs_info;
struct btrfs_dev_item *dev_item;
struct btrfs_path path;
- struct extent_buffer *leaf;
struct btrfs_root *root = fs_info->chunk_root;
struct btrfs_key key;
+ struct extent_buffer *leaf;
>>>
>>> nit: Unnecessary change
>>
>> Doesn't it look better when all btrfs_ prefix get batched together? :)
>
> Please don't do unrelated changes like that.
Understood, the github version has that @leaf reverted to the original
location.
Thanks,
Qu
Re: [PATCH v2 1/5] btrfs-progs: image: Refactor fixup_devices() to fixup_chunks_and_devices()
On Tue, Nov 27, 2018 at 04:38:24PM +0800, Qu Wenruo wrote:
> +error:
> + error(
> +"failed to fix chunks and devices mapping, the fs may not be mountable: %s",
> + strerror(-ret));
Recently the sterror(error code) have been converted to %m, so I changed
this to
errno = -ret
error("... %m");
Re: [PATCH v2 1/5] btrfs-progs: image: Refactor fixup_devices() to fixup_chunks_and_devices()
On Tue, Nov 27, 2018 at 04:50:57PM +0800, Qu Wenruo wrote:
> >> -static int fixup_devices(struct btrfs_fs_info *fs_info,
> >> - struct mdrestore_struct *mdres, off_t dev_size)
> >> +static int fixup_device_size(struct btrfs_trans_handle *trans,
> >> + struct mdrestore_struct *mdres,
> >> + off_t dev_size)
> >> {
> >> - struct btrfs_trans_handle *trans;
> >> + struct btrfs_fs_info *fs_info = trans->fs_info;
> >>struct btrfs_dev_item *dev_item;
> >>struct btrfs_path path;
> >> - struct extent_buffer *leaf;
> >>struct btrfs_root *root = fs_info->chunk_root;
> >>struct btrfs_key key;
> >> + struct extent_buffer *leaf;
> >
> > nit: Unnecessary change
>
> Doesn't it look better when all btrfs_ prefix get batched together? :)
Please don't do unrelated changes like that.
Re: [PATCH v2 1/5] btrfs-progs: image: Refactor fixup_devices() to fixup_chunks_and_devices()
On 27.11.18 г. 10:50 ч., Qu Wenruo wrote:
>
>
> On 2018/11/27 下午4:46, Nikolay Borisov wrote:
>>
>>
>> On 27.11.18 г. 10:38 ч., Qu Wenruo wrote:
>>> Current fixup_devices() will only remove DEV_ITEMs and reset DEV_ITEM
>>> size.
>>> Later we need to do more fixup works, so change the name to
>>> fixup_chunks_and_devices() and refactor the original device size fixup
>>> to fixup_device_size().
>>>
>>> Signed-off-by: Qu Wenruo
>>
>> Reviewed-by: Nikolay Borisov
>>
>> However, one minor nit below.
>>
>>> ---
>>> image/main.c | 52
>>> 1 file changed, 36 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/image/main.c b/image/main.c
>>> index c680ab19de6c..bbfcf8f19298 100644
>>> --- a/image/main.c
>>> +++ b/image/main.c
>>> @@ -2084,28 +2084,19 @@ static void remap_overlapping_chunks(struct
>>> mdrestore_struct *mdres)
>>> }
>>> }
>>>
>>> -static int fixup_devices(struct btrfs_fs_info *fs_info,
>>> -struct mdrestore_struct *mdres, off_t dev_size)
>>> +static int fixup_device_size(struct btrfs_trans_handle *trans,
>>> +struct mdrestore_struct *mdres,
>>> +off_t dev_size)
>>> {
>>> - struct btrfs_trans_handle *trans;
>>> + struct btrfs_fs_info *fs_info = trans->fs_info;
>>> struct btrfs_dev_item *dev_item;
>>> struct btrfs_path path;
>>> - struct extent_buffer *leaf;
>>> struct btrfs_root *root = fs_info->chunk_root;
>>> struct btrfs_key key;
>>> + struct extent_buffer *leaf;
>>
>> nit: Unnecessary change
>
> Doesn't it look better when all btrfs_ prefix get batched together? :)
Didn't even realize this was the intended effect. IMO doesn't make much
difference, what does, though, is probably reverse christmas tree, ie
longer variable names
come before shorter
ones
But I guess this is a matter of taste, no need to resend.
>
> Thanks,
> Qu
>
>>
>>> u64 devid, cur_devid;
>>> int ret;
>>>
>>> - if (btrfs_super_log_root(fs_info->super_copy)) {
>>> - warning(
>>> - "log tree detected, its generation will not match superblock");
>>> - }
>>> - trans = btrfs_start_transaction(fs_info->tree_root, 1);
>>> - if (IS_ERR(trans)) {
>>> - error("cannot starting transaction %ld", PTR_ERR(trans));
>>> - return PTR_ERR(trans);
>>> - }
>>> -
>>> dev_item = _info->super_copy->dev_item;
>>>
>>> devid = btrfs_stack_device_id(dev_item);
>>> @@ -2123,7 +2114,7 @@ again:
>>> ret = btrfs_search_slot(trans, root, , , -1, 1);
>>> if (ret < 0) {
>>> error("search failed: %d", ret);
>>> - exit(1);
>>> + return ret;
>>> }
>>>
>>> while (1) {
>>> @@ -2170,12 +2161,41 @@ again:
>>> }
>>>
>>> btrfs_release_path();
>>> + return 0;
>>> +}
>>> +
>>> +static int fixup_chunks_and_devices(struct btrfs_fs_info *fs_info,
>>> +struct mdrestore_struct *mdres, off_t dev_size)
>>> +{
>>> + struct btrfs_trans_handle *trans;
>>> + int ret;
>>> +
>>> + if (btrfs_super_log_root(fs_info->super_copy)) {
>>> + warning(
>>> + "log tree detected, its generation will not match superblock");
>>> + }
>>> + trans = btrfs_start_transaction(fs_info->tree_root, 1);
>>> + if (IS_ERR(trans)) {
>>> + error("cannot starting transaction %ld", PTR_ERR(trans));
>>> + return PTR_ERR(trans);
>>> + }
>>> +
>>> + ret = fixup_device_size(trans, mdres, dev_size);
>>> + if (ret < 0)
>>> + goto error;
>>> +
>>> ret = btrfs_commit_transaction(trans, fs_info->tree_root);
>>> if (ret) {
>>> error("unable to commit transaction: %d", ret);
>>> return ret;
>>> }
>>> return 0;
>>> +error:
>>> + error(
>>> +"failed to fix chunks and devices mapping, the fs may not be mountable:
>>> %s",
>>> + strerror(-ret));
>>> + btrfs_abort_transaction(trans, ret);
>>> + return ret;
>>> }
>>>
>>> static int restore_metadump(const char *input, FILE *out, int old_restore,
>>> @@ -2282,7 +2302,7 @@ static int restore_metadump(const char *input, FILE
>>> *out, int old_restore,
>>> return 1;
>>> }
>>>
>>> - ret = fixup_devices(info, , st.st_size);
>>> + ret = fixup_chunks_and_devices(info, , st.st_size);
>>> close_ctree(info->chunk_root);
>>> if (ret)
>>> goto out;
>>>
>
Re: [PATCH v2 1/5] btrfs-progs: image: Refactor fixup_devices() to fixup_chunks_and_devices()
On 2018/11/27 下午4:46, Nikolay Borisov wrote:
>
>
> On 27.11.18 г. 10:38 ч., Qu Wenruo wrote:
>> Current fixup_devices() will only remove DEV_ITEMs and reset DEV_ITEM
>> size.
>> Later we need to do more fixup works, so change the name to
>> fixup_chunks_and_devices() and refactor the original device size fixup
>> to fixup_device_size().
>>
>> Signed-off-by: Qu Wenruo
>
> Reviewed-by: Nikolay Borisov
>
> However, one minor nit below.
>
>> ---
>> image/main.c | 52
>> 1 file changed, 36 insertions(+), 16 deletions(-)
>>
>> diff --git a/image/main.c b/image/main.c
>> index c680ab19de6c..bbfcf8f19298 100644
>> --- a/image/main.c
>> +++ b/image/main.c
>> @@ -2084,28 +2084,19 @@ static void remap_overlapping_chunks(struct
>> mdrestore_struct *mdres)
>> }
>> }
>>
>> -static int fixup_devices(struct btrfs_fs_info *fs_info,
>> - struct mdrestore_struct *mdres, off_t dev_size)
>> +static int fixup_device_size(struct btrfs_trans_handle *trans,
>> + struct mdrestore_struct *mdres,
>> + off_t dev_size)
>> {
>> -struct btrfs_trans_handle *trans;
>> +struct btrfs_fs_info *fs_info = trans->fs_info;
>> struct btrfs_dev_item *dev_item;
>> struct btrfs_path path;
>> -struct extent_buffer *leaf;
>> struct btrfs_root *root = fs_info->chunk_root;
>> struct btrfs_key key;
>> +struct extent_buffer *leaf;
>
> nit: Unnecessary change
Doesn't it look better when all btrfs_ prefix get batched together? :)
Thanks,
Qu
>
>> u64 devid, cur_devid;
>> int ret;
>>
>> -if (btrfs_super_log_root(fs_info->super_copy)) {
>> -warning(
>> -"log tree detected, its generation will not match superblock");
>> -}
>> -trans = btrfs_start_transaction(fs_info->tree_root, 1);
>> -if (IS_ERR(trans)) {
>> -error("cannot starting transaction %ld", PTR_ERR(trans));
>> -return PTR_ERR(trans);
>> -}
>> -
>> dev_item = _info->super_copy->dev_item;
>>
>> devid = btrfs_stack_device_id(dev_item);
>> @@ -2123,7 +2114,7 @@ again:
>> ret = btrfs_search_slot(trans, root, , , -1, 1);
>> if (ret < 0) {
>> error("search failed: %d", ret);
>> -exit(1);
>> +return ret;
>> }
>>
>> while (1) {
>> @@ -2170,12 +2161,41 @@ again:
>> }
>>
>> btrfs_release_path();
>> +return 0;
>> +}
>> +
>> +static int fixup_chunks_and_devices(struct btrfs_fs_info *fs_info,
>> + struct mdrestore_struct *mdres, off_t dev_size)
>> +{
>> +struct btrfs_trans_handle *trans;
>> +int ret;
>> +
>> +if (btrfs_super_log_root(fs_info->super_copy)) {
>> +warning(
>> +"log tree detected, its generation will not match superblock");
>> +}
>> +trans = btrfs_start_transaction(fs_info->tree_root, 1);
>> +if (IS_ERR(trans)) {
>> +error("cannot starting transaction %ld", PTR_ERR(trans));
>> +return PTR_ERR(trans);
>> +}
>> +
>> +ret = fixup_device_size(trans, mdres, dev_size);
>> +if (ret < 0)
>> +goto error;
>> +
>> ret = btrfs_commit_transaction(trans, fs_info->tree_root);
>> if (ret) {
>> error("unable to commit transaction: %d", ret);
>> return ret;
>> }
>> return 0;
>> +error:
>> +error(
>> +"failed to fix chunks and devices mapping, the fs may not be mountable: %s",
>> +strerror(-ret));
>> +btrfs_abort_transaction(trans, ret);
>> +return ret;
>> }
>>
>> static int restore_metadump(const char *input, FILE *out, int old_restore,
>> @@ -2282,7 +2302,7 @@ static int restore_metadump(const char *input, FILE
>> *out, int old_restore,
>> return 1;
>> }
>>
>> -ret = fixup_devices(info, , st.st_size);
>> +ret = fixup_chunks_and_devices(info, , st.st_size);
>> close_ctree(info->chunk_root);
>> if (ret)
>> goto out;
>>
Re: [PATCH v2 1/5] btrfs-progs: image: Refactor fixup_devices() to fixup_chunks_and_devices()
On 27.11.18 г. 10:38 ч., Qu Wenruo wrote:
> Current fixup_devices() will only remove DEV_ITEMs and reset DEV_ITEM
> size.
> Later we need to do more fixup works, so change the name to
> fixup_chunks_and_devices() and refactor the original device size fixup
> to fixup_device_size().
>
> Signed-off-by: Qu Wenruo
Reviewed-by: Nikolay Borisov
However, one minor nit below.
> ---
> image/main.c | 52
> 1 file changed, 36 insertions(+), 16 deletions(-)
>
> diff --git a/image/main.c b/image/main.c
> index c680ab19de6c..bbfcf8f19298 100644
> --- a/image/main.c
> +++ b/image/main.c
> @@ -2084,28 +2084,19 @@ static void remap_overlapping_chunks(struct
> mdrestore_struct *mdres)
> }
> }
>
> -static int fixup_devices(struct btrfs_fs_info *fs_info,
> - struct mdrestore_struct *mdres, off_t dev_size)
> +static int fixup_device_size(struct btrfs_trans_handle *trans,
> + struct mdrestore_struct *mdres,
> + off_t dev_size)
> {
> - struct btrfs_trans_handle *trans;
> + struct btrfs_fs_info *fs_info = trans->fs_info;
> struct btrfs_dev_item *dev_item;
> struct btrfs_path path;
> - struct extent_buffer *leaf;
> struct btrfs_root *root = fs_info->chunk_root;
> struct btrfs_key key;
> + struct extent_buffer *leaf;
nit: Unnecessary change
> u64 devid, cur_devid;
> int ret;
>
> - if (btrfs_super_log_root(fs_info->super_copy)) {
> - warning(
> - "log tree detected, its generation will not match superblock");
> - }
> - trans = btrfs_start_transaction(fs_info->tree_root, 1);
> - if (IS_ERR(trans)) {
> - error("cannot starting transaction %ld", PTR_ERR(trans));
> - return PTR_ERR(trans);
> - }
> -
> dev_item = _info->super_copy->dev_item;
>
> devid = btrfs_stack_device_id(dev_item);
> @@ -2123,7 +2114,7 @@ again:
> ret = btrfs_search_slot(trans, root, , , -1, 1);
> if (ret < 0) {
> error("search failed: %d", ret);
> - exit(1);
> + return ret;
> }
>
> while (1) {
> @@ -2170,12 +2161,41 @@ again:
> }
>
> btrfs_release_path();
> + return 0;
> +}
> +
> +static int fixup_chunks_and_devices(struct btrfs_fs_info *fs_info,
> + struct mdrestore_struct *mdres, off_t dev_size)
> +{
> + struct btrfs_trans_handle *trans;
> + int ret;
> +
> + if (btrfs_super_log_root(fs_info->super_copy)) {
> + warning(
> + "log tree detected, its generation will not match superblock");
> + }
> + trans = btrfs_start_transaction(fs_info->tree_root, 1);
> + if (IS_ERR(trans)) {
> + error("cannot starting transaction %ld", PTR_ERR(trans));
> + return PTR_ERR(trans);
> + }
> +
> + ret = fixup_device_size(trans, mdres, dev_size);
> + if (ret < 0)
> + goto error;
> +
> ret = btrfs_commit_transaction(trans, fs_info->tree_root);
> if (ret) {
> error("unable to commit transaction: %d", ret);
> return ret;
> }
> return 0;
> +error:
> + error(
> +"failed to fix chunks and devices mapping, the fs may not be mountable: %s",
> + strerror(-ret));
> + btrfs_abort_transaction(trans, ret);
> + return ret;
> }
>
> static int restore_metadump(const char *input, FILE *out, int old_restore,
> @@ -2282,7 +2302,7 @@ static int restore_metadump(const char *input, FILE
> *out, int old_restore,
> return 1;
> }
>
> - ret = fixup_devices(info, , st.st_size);
> + ret = fixup_chunks_and_devices(info, , st.st_size);
> close_ctree(info->chunk_root);
> if (ret)
> goto out;
>
