[PATCH 1/2] btrfs-progs: Avoid reading bad fd in case of missing device.

[PATCH 1/2] btrfs-progs: Avoid reading bad fd in case of missing device.

[Qu Wenruo]

Offline btrfs tools, like btrfs-image, will infinitely loop when there
is missing device.

The reason is, for missing device, it's fd will be set to -1, but before
we reading, we only check the fd validation by checking if it's 0.
So in that case, -1 will pass the validation check, and cause pread to
return 0, and loop to read.

Just change the validation check from "== 0" to "<= 0" to avoid such
problem.

Reported-by: Timothy Normand Miller <theosib@gmail.com>
Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 btrfs-image.c | 2 +-
 disk-io.c     | 4 ++--
 extent_io.c   | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/btrfs-image.c b/btrfs-image.c
index 3684a05..b225325 100644
--- a/btrfs-image.c
+++ b/btrfs-image.c
@@ -894,7 +894,7 @@ static int read_data_extent(struct metadump_struct *md,
 
 		device = multi->stripes[0].dev;
 
-		if (device->fd == 0) {
+		if (device->fd <= 0) {
 			fprintf(stderr,
 				"Device we need to read from is not open\n");
 			free(multi);
diff --git a/disk-io.c b/disk-io.c
index fdcfd6d..d8ca101 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -266,7 +266,7 @@ int read_whole_eb(struct btrfs_fs_info *info, struct extent_buffer *eb, int mirr
 			}
 			device = multi->stripes[0].dev;
 
-			if (device->fd == 0) {
+			if (device->fd <= 0) {
 				kfree(multi);
 				return -EIO;
 			}
@@ -385,7 +385,7 @@ int read_extent_data(struct btrfs_root *root, char *data,
 	}
 	device = multi->stripes[0].dev;
 
-	if (device->fd == 0)
+	if (device->fd <= 0)
 		goto err;
 	if (*len > max_len)
 		*len = max_len;
diff --git a/extent_io.c b/extent_io.c
index 5d49710..07695ef 100644
--- a/extent_io.c
+++ b/extent_io.c
@@ -714,7 +714,7 @@ int read_data_from_disk(struct btrfs_fs_info *info, void *buf, u64 offset,
 		device = multi->stripes[0].dev;
 
 		read_len = min(bytes_left, read_len);
-		if (device->fd == 0) {
+		if (device->fd <= 0) {
 			kfree(multi);
 			return -EIO;
 		}
@@ -790,7 +790,7 @@ int write_data_to_disk(struct btrfs_fs_info *info, void *buf, u64 offset,
 			raid_map = NULL;
 		} else while (dev_nr < multi->num_stripes) {
 			device = multi->stripes[dev_nr].dev;
-			if (device->fd == 0) {
+			if (device->fd <= 0) {
 				kfree(multi);
 				return -EIO;
 			}
-- 
2.5.0

[PATCH 2/2] btrfs-progs: Use existing facility to replace read_data_extent function

[Qu Wenruo]

Function read_data_extent() in btrfs-image.c is using manual-and-read
codes.
Replace it with existing read_extent_data() to reduce duplication.

Also, now we can use other mirror to try our best to do the dump, just
like read_tree_block().

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 btrfs-image.c | 58 +++++++++++++++++++---------------------------------------
 1 file changed, 19 insertions(+), 39 deletions(-)

diff --git a/btrfs-image.c b/btrfs-image.c
index b225325..f48d81a 100644
--- a/btrfs-image.c
+++ b/btrfs-image.c
@@ -872,54 +872,34 @@ out:
 static int read_data_extent(struct metadump_struct *md,
 			    struct async_work *async)
 {
-	struct btrfs_multi_bio *multi = NULL;
-	struct btrfs_device *device;
+	struct btrfs_root *root = md->root;
 	u64 bytes_left = async->size;
 	u64 logical = async->start;
 	u64 offset = 0;
-	u64 bytenr;
 	u64 read_len;
-	ssize_t done;
-	int fd;
+	int num_copies;
+	int cur_mirror;
 	int ret;
 
-	while (bytes_left) {
-		read_len = bytes_left;
-		ret = btrfs_map_block(&md->root->fs_info->mapping_tree, READ,
-				      logical, &read_len, &multi, 0, NULL);
-		if (ret) {
-			fprintf(stderr, "Couldn't map data block %d\n", ret);
-			return ret;
-		}
-
-		device = multi->stripes[0].dev;
-
-		if (device->fd <= 0) {
-			fprintf(stderr,
-				"Device we need to read from is not open\n");
-			free(multi);
-			return -EIO;
-		}
-		fd = device->fd;
-		bytenr = multi->stripes[0].physical;
-		free(multi);
+	num_copies = btrfs_num_copies(&root->fs_info->mapping_tree, logical,
+				      bytes_left);
 
-		read_len = min(read_len, bytes_left);
-		done = pread64(fd, async->buffer+offset, read_len, bytenr);
-		if (done < read_len) {
-			if (done < 0)
-				fprintf(stderr, "Error reading extent %d\n",
-					errno);
-			else
-				fprintf(stderr, "Short read\n");
-			return -EIO;
+	/* Try our best to read data, just like read_tree_block() */
+	for (cur_mirror = 0; cur_mirror < num_copies; cur_mirror++) {
+		while (bytes_left) {
+			read_len = bytes_left;
+			ret = read_extent_data(root,
+					(char *)(async->buffer + offset),
+					logical, &read_len, cur_mirror);
+			if (ret < 0)
+				break;
+			offset += read_len;
+			logical += read_len;
+			bytes_left -= read_len;
 		}
-
-		bytes_left -= done;
-		offset += done;
-		logical += done;
 	}
-
+	if (bytes_left)
+		return -EIO;
 	return 0;
 }
 
-- 
2.5.0

Re: [PATCH 1/2] btrfs-progs: Avoid reading bad fd in case of missing device.

[David Sterba]

On Fri, Aug 21, 2015 at 11:21:26AM +0800, Qu Wenruo wrote:
> Offline btrfs tools, like btrfs-image, will infinitely loop when there
> is missing device.
> 
> The reason is, for missing device, it's fd will be set to -1, but before
> we reading, we only check the fd validation by checking if it's 0.
> So in that case, -1 will pass the validation check, and cause pread to
> return 0, and loop to read.
> 
> Just change the validation check from "== 0" to "<= 0" to avoid such
> problem.
> 
> Reported-by: Timothy Normand Miller <theosib@gmail.com>
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>

Applied, thanks. I'll add a test for that.

Re: [PATCH 2/2] btrfs-progs: Use existing facility to replace read_data_extent function

[David Sterba]

On Fri, Aug 21, 2015 at 11:21:27AM +0800, Qu Wenruo wrote:
> Function read_data_extent() in btrfs-image.c is using manual-and-read
> codes.
> Replace it with existing read_extent_data() to reduce duplication.
> 
> Also, now we can use other mirror to try our best to do the dump, just
> like read_tree_block().
> 
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>

Applied, thanks.