| Message ID | 20181008123044.13413-2-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: check: Detect invalid dev extents and device items | expand |
On 8.10.2018 15:30, Qu Wenruo wrote: > When restoring btrfs image, the total_bytes of device item is not > updated correctly. In fact total_bytes can be left 0 for restored image. > > It doesn't trigger any error because btrfs check never checks > total_bytes of dev item. > However this is going to change. > > Fix it by populating total_bytes of device item with the end position of > last dev extent to make later btrfs check happy. 'Setting it to the end position' really means "setting the total device size to the allocated space on the device". Is it more clear if stated as the second way ? In any case: Reviewed-by: Nikolay Borisov <nborisov@suse.com> > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > image/main.c | 48 +++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 45 insertions(+), 3 deletions(-) > > diff --git a/image/main.c b/image/main.c > index 351c5a256938..d5b89bc3149f 100644 > --- a/image/main.c > +++ b/image/main.c > @@ -2082,15 +2082,17 @@ 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) > + struct mdrestore_struct *mdres, int out_fd) > { > struct btrfs_trans_handle *trans; > struct btrfs_dev_item *dev_item; > + struct btrfs_dev_extent *dev_ext; > struct btrfs_path path; > struct extent_buffer *leaf; > struct btrfs_root *root = fs_info->chunk_root; > struct btrfs_key key; > u64 devid, cur_devid; > + u64 dev_size; /* Get from last dev extents */ > int ret; > > trans = btrfs_start_transaction(fs_info->tree_root, 1); > @@ -2101,16 +2103,56 @@ static int fixup_devices(struct btrfs_fs_info *fs_info, > > dev_item = &fs_info->super_copy->dev_item; > > + btrfs_init_path(&path); > devid = btrfs_stack_device_id(dev_item); > > + key.objectid = devid; > + key.type = BTRFS_DEV_EXTENT_KEY; > + key.offset = (u64)-1; > + > + ret = btrfs_search_slot(NULL, fs_info->dev_root, &key, &path, 0, 0); > + if (ret < 0) { > + error("failed to locate last dev extent of devid %llu: %s", > + devid, strerror(-ret)); > + btrfs_release_path(&path); > + return ret; > + } > + if (ret == 0) { nit: I'd prefer if this is an else if since it emphasizes the fact that the if/elseif construct works on a single value. > + error("found invalid dev extent devid %llu offset -1", > + devid); > + btrfs_release_path(&path); > + return -EUCLEAN; > + } > + ret = btrfs_previous_item(fs_info->dev_root, &path, devid, > + BTRFS_DEV_EXTENT_KEY); > + if (ret > 0) > + ret = -ENOENT; > + if (ret < 0) { ditto > + error("failed to locate last dev extent of devid %llu: %s", > + devid, strerror(-ret)); > + btrfs_release_path(&path); > + return ret; > + } > + > + btrfs_item_key_to_cpu(path.nodes[0], &key, path.slots[0]); > + dev_ext = btrfs_item_ptr(path.nodes[0], path.slots[0], > + struct btrfs_dev_extent); > + dev_size = key.offset + btrfs_dev_extent_length(path.nodes[0], dev_ext); > + btrfs_release_path(&path); > + > btrfs_set_stack_device_total_bytes(dev_item, dev_size); > btrfs_set_stack_device_bytes_used(dev_item, mdres->alloced_chunks); > + /* Don't forget to enlarge the real file */ > + ret = ftruncate64(out_fd, dev_size); > + if (ret < 0) { > + error("failed to enlarge result image: %s", strerror(errno)); > + return -errno; > + } > > key.objectid = BTRFS_DEV_ITEMS_OBJECTID; > key.type = BTRFS_DEV_ITEM_KEY; > key.offset = 0; > > - btrfs_init_path(&path); > > again: > ret = btrfs_search_slot(trans, root, &key, &path, -1, 1); > @@ -2275,7 +2317,7 @@ static int restore_metadump(const char *input, FILE *out, int old_restore, > return 1; > } > > - ret = fixup_devices(info, &mdrestore, st.st_size); > + ret = fixup_devices(info, &mdrestore, fileno(out)); > close_ctree(info->chunk_root); > if (ret) > goto out; >
On 2018/10/11 下午8:07, Nikolay Borisov wrote: > > > On 8.10.2018 15:30, Qu Wenruo wrote: >> When restoring btrfs image, the total_bytes of device item is not >> updated correctly. In fact total_bytes can be left 0 for restored image. >> >> It doesn't trigger any error because btrfs check never checks >> total_bytes of dev item. >> However this is going to change. >> >> Fix it by populating total_bytes of device item with the end position of >> last dev extent to make later btrfs check happy. > > 'Setting it to the end position' really means "setting the total device > size to the allocated space on the device". Is it more clear if stated > as the second way ? Not exactly. Don't forget that we have 0~1M reserved, and it's possible to have dev extent holes. So it's not "allocated space" but really "the end position of the last dev extent" Thanks, Qu > > In any case: > > Reviewed-by: Nikolay Borisov <nborisov@suse.com> > >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> image/main.c | 48 +++++++++++++++++++++++++++++++++++++++++++++--- >> 1 file changed, 45 insertions(+), 3 deletions(-) >> >> diff --git a/image/main.c b/image/main.c >> index 351c5a256938..d5b89bc3149f 100644 >> --- a/image/main.c >> +++ b/image/main.c >> @@ -2082,15 +2082,17 @@ 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) >> + struct mdrestore_struct *mdres, int out_fd) >> { >> struct btrfs_trans_handle *trans; >> struct btrfs_dev_item *dev_item; >> + struct btrfs_dev_extent *dev_ext; >> struct btrfs_path path; >> struct extent_buffer *leaf; >> struct btrfs_root *root = fs_info->chunk_root; >> struct btrfs_key key; >> u64 devid, cur_devid; >> + u64 dev_size; /* Get from last dev extents */ >> int ret; >> >> trans = btrfs_start_transaction(fs_info->tree_root, 1); >> @@ -2101,16 +2103,56 @@ static int fixup_devices(struct btrfs_fs_info *fs_info, >> >> dev_item = &fs_info->super_copy->dev_item; >> >> + btrfs_init_path(&path); >> devid = btrfs_stack_device_id(dev_item); >> >> + key.objectid = devid; >> + key.type = BTRFS_DEV_EXTENT_KEY; >> + key.offset = (u64)-1; >> + >> + ret = btrfs_search_slot(NULL, fs_info->dev_root, &key, &path, 0, 0); >> + if (ret < 0) { >> + error("failed to locate last dev extent of devid %llu: %s", >> + devid, strerror(-ret)); >> + btrfs_release_path(&path); >> + return ret; >> + } >> + if (ret == 0) { > > nit: I'd prefer if this is an else if since it emphasizes the fact that > the if/elseif construct works on a single value. > >> + error("found invalid dev extent devid %llu offset -1", >> + devid); >> + btrfs_release_path(&path); >> + return -EUCLEAN; >> + } >> + ret = btrfs_previous_item(fs_info->dev_root, &path, devid, >> + BTRFS_DEV_EXTENT_KEY); >> + if (ret > 0) >> + ret = -ENOENT; >> + if (ret < 0) { > > ditto > >> + error("failed to locate last dev extent of devid %llu: %s", >> + devid, strerror(-ret)); >> + btrfs_release_path(&path); >> + return ret; >> + } >> + >> + btrfs_item_key_to_cpu(path.nodes[0], &key, path.slots[0]); >> + dev_ext = btrfs_item_ptr(path.nodes[0], path.slots[0], >> + struct btrfs_dev_extent); >> + dev_size = key.offset + btrfs_dev_extent_length(path.nodes[0], dev_ext); >> + btrfs_release_path(&path); >> + >> btrfs_set_stack_device_total_bytes(dev_item, dev_size); >> btrfs_set_stack_device_bytes_used(dev_item, mdres->alloced_chunks); >> + /* Don't forget to enlarge the real file */ >> + ret = ftruncate64(out_fd, dev_size); >> + if (ret < 0) { >> + error("failed to enlarge result image: %s", strerror(errno)); >> + return -errno; >> + } >> >> key.objectid = BTRFS_DEV_ITEMS_OBJECTID; >> key.type = BTRFS_DEV_ITEM_KEY; >> key.offset = 0; >> >> - btrfs_init_path(&path); >> >> again: >> ret = btrfs_search_slot(trans, root, &key, &path, -1, 1); >> @@ -2275,7 +2317,7 @@ static int restore_metadump(const char *input, FILE *out, int old_restore, >> return 1; >> } >> >> - ret = fixup_devices(info, &mdrestore, st.st_size); >> + ret = fixup_devices(info, &mdrestore, fileno(out)); >> close_ctree(info->chunk_root); >> if (ret) >> goto out; >>
diff --git a/image/main.c b/image/main.c index 351c5a256938..d5b89bc3149f 100644 --- a/image/main.c +++ b/image/main.c @@ -2082,15 +2082,17 @@ 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) + struct mdrestore_struct *mdres, int out_fd) { struct btrfs_trans_handle *trans; struct btrfs_dev_item *dev_item; + struct btrfs_dev_extent *dev_ext; struct btrfs_path path; struct extent_buffer *leaf; struct btrfs_root *root = fs_info->chunk_root; struct btrfs_key key; u64 devid, cur_devid; + u64 dev_size; /* Get from last dev extents */ int ret; trans = btrfs_start_transaction(fs_info->tree_root, 1); @@ -2101,16 +2103,56 @@ static int fixup_devices(struct btrfs_fs_info *fs_info, dev_item = &fs_info->super_copy->dev_item; + btrfs_init_path(&path); devid = btrfs_stack_device_id(dev_item); + key.objectid = devid; + key.type = BTRFS_DEV_EXTENT_KEY; + key.offset = (u64)-1; + + ret = btrfs_search_slot(NULL, fs_info->dev_root, &key, &path, 0, 0); + if (ret < 0) { + error("failed to locate last dev extent of devid %llu: %s", + devid, strerror(-ret)); + btrfs_release_path(&path); + return ret; + } + if (ret == 0) { + error("found invalid dev extent devid %llu offset -1", + devid); + btrfs_release_path(&path); + return -EUCLEAN; + } + ret = btrfs_previous_item(fs_info->dev_root, &path, devid, + BTRFS_DEV_EXTENT_KEY); + if (ret > 0) + ret = -ENOENT; + if (ret < 0) { + error("failed to locate last dev extent of devid %llu: %s", + devid, strerror(-ret)); + btrfs_release_path(&path); + return ret; + } + + btrfs_item_key_to_cpu(path.nodes[0], &key, path.slots[0]); + dev_ext = btrfs_item_ptr(path.nodes[0], path.slots[0], + struct btrfs_dev_extent); + dev_size = key.offset + btrfs_dev_extent_length(path.nodes[0], dev_ext); + btrfs_release_path(&path); + btrfs_set_stack_device_total_bytes(dev_item, dev_size); btrfs_set_stack_device_bytes_used(dev_item, mdres->alloced_chunks); + /* Don't forget to enlarge the real file */ + ret = ftruncate64(out_fd, dev_size); + if (ret < 0) { + error("failed to enlarge result image: %s", strerror(errno)); + return -errno; + } key.objectid = BTRFS_DEV_ITEMS_OBJECTID; key.type = BTRFS_DEV_ITEM_KEY; key.offset = 0; - btrfs_init_path(&path); again: ret = btrfs_search_slot(trans, root, &key, &path, -1, 1); @@ -2275,7 +2317,7 @@ static int restore_metadump(const char *input, FILE *out, int old_restore, return 1; } - ret = fixup_devices(info, &mdrestore, st.st_size); + ret = fixup_devices(info, &mdrestore, fileno(out)); close_ctree(info->chunk_root); if (ret) goto out;
When restoring btrfs image, the total_bytes of device item is not updated correctly. In fact total_bytes can be left 0 for restored image. It doesn't trigger any error because btrfs check never checks total_bytes of dev item. However this is going to change. Fix it by populating total_bytes of device item with the end position of last dev extent to make later btrfs check happy. Signed-off-by: Qu Wenruo <wqu@suse.com> --- image/main.c | 48 +++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 45 insertions(+), 3 deletions(-)