[PATCH 1/2] btrfs: fix read_one_chunk() return error code

[PATCH 1/2] btrfs: fix read_one_chunk() return error code

[Anand Jain]

EIO is for IO (RW) failures, use correct error return
codes at two locations with in read_one_chunk(). Also
adds a small cleanup.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/volumes.c | 17 ++++++++++-------
 fs/btrfs/volumes.h |  3 +--
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 0e8f16c305df..f4b659b6ef4c 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6249,7 +6249,7 @@ static struct btrfs_device *add_missing_dev(struct btrfs_fs_devices *fs_devices,
 
 	device = btrfs_alloc_device(NULL, &devid, dev_uuid);
 	if (IS_ERR(device))
-		return NULL;
+		return device;
 
 	list_add(&device->dev_list, &fs_devices->devices);
 	device->fs_devices = fs_devices;
@@ -6448,15 +6448,18 @@ static int read_one_chunk(struct btrfs_fs_info *fs_info, struct btrfs_key *key,
 		    !btrfs_test_opt(fs_info, DEGRADED)) {
 			free_extent_map(em);
 			btrfs_report_missing_device(fs_info, devid, uuid);
-			return -EIO;
+			return -EINVAL;
 		}
 		if (!map->stripes[i].dev) {
 			map->stripes[i].dev =
 				add_missing_dev(fs_info->fs_devices, devid,
 						uuid);
-			if (!map->stripes[i].dev) {
+			if (IS_ERR(map->stripes[i].dev)) {
 				free_extent_map(em);
-				return -EIO;
+				btrfs_err(fs_info,
+				"failed to init missing dev %llu %ld",
+					devid, PTR_ERR(map->stripes[i].dev));
+				return PTR_ERR(map->stripes[i].dev);
 			}
 			btrfs_report_missing_device(fs_info, devid, uuid);
 		}
@@ -6578,12 +6581,12 @@ static int read_one_dev(struct btrfs_fs_info *fs_info,
 	if (!device) {
 		if (!btrfs_test_opt(fs_info, DEGRADED)) {
 			btrfs_report_missing_device(fs_info, devid, dev_uuid);
-			return -EIO;
+			return -EINVAL;
 		}
 
 		device = add_missing_dev(fs_devices, devid, dev_uuid);
-		if (!device)
-			return -ENOMEM;
+		if (IS_ERR(device))
+			return PTR_ERR(device);
 		btrfs_report_missing_device(fs_info, devid, dev_uuid);
 	} else {
 		if (!device->bdev) {
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 6108fdfec67f..6c807a1c0531 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -432,8 +432,7 @@ int btrfs_find_device_by_devspec(struct btrfs_fs_info *fs_info, u64 devid,
 					 const char *devpath,
 					 struct btrfs_device **device);
 struct btrfs_device *btrfs_alloc_device(struct btrfs_fs_info *fs_info,
-					const u64 *devid,
-					const u8 *uuid);
+					const u64 *devid, const u8 *uuid);
 int btrfs_rm_device(struct btrfs_fs_info *fs_info,
 		    const char *device_path, u64 devid);
 void btrfs_cleanup_fs_uuids(void);
-- 
2.13.1

[PATCH 2/2] btrfs: clean up btrfs_report_missing_device() usage

[Anand Jain]

This patch makes the following changes..
When -o degraded flag is not set and one of the device
is missing then we should log an error instead of warning.
So move the check for DEGRADED flag into the same function
and rename it. Also does code cleanup around this, including
making this function as static. And to print warning, just
open code it.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/volumes.c | 41 +++++++++++++++++++++++------------------
 fs/btrfs/volumes.h |  3 ---
 2 files changed, 23 insertions(+), 21 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index f4b659b6ef4c..317d6d1b58d0 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6377,6 +6377,22 @@ static int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
 	return 0;
 }
 
+static int check_report_degraded(struct btrfs_fs_info *fs_info,
+					u64 devid, u8 *uuid, int warn)
+{
+	if (!btrfs_test_opt(fs_info, DEGRADED)) {
+		btrfs_err_rl(fs_info, "devid %llu uuid %pU is missing",
+				devid, uuid);
+		return 1;
+	}
+
+	if (warn)
+		btrfs_warn_rl(fs_info, "devid %llu uuid %pU is missing",
+				devid, uuid);
+	return 0;
+}
+
+
 static int read_one_chunk(struct btrfs_fs_info *fs_info, struct btrfs_key *key,
 			  struct extent_buffer *leaf,
 			  struct btrfs_chunk *chunk)
@@ -6445,9 +6461,8 @@ static int read_one_chunk(struct btrfs_fs_info *fs_info, struct btrfs_key *key,
 		map->stripes[i].dev = btrfs_find_device(fs_info, devid,
 							uuid, NULL);
 		if (!map->stripes[i].dev &&
-		    !btrfs_test_opt(fs_info, DEGRADED)) {
+			check_report_degraded(fs_info, devid, uuid, 0)) {
 			free_extent_map(em);
-			btrfs_report_missing_device(fs_info, devid, uuid);
 			return -EINVAL;
 		}
 		if (!map->stripes[i].dev) {
@@ -6461,7 +6476,7 @@ static int read_one_chunk(struct btrfs_fs_info *fs_info, struct btrfs_key *key,
 					devid, PTR_ERR(map->stripes[i].dev));
 				return PTR_ERR(map->stripes[i].dev);
 			}
-			btrfs_report_missing_device(fs_info, devid, uuid);
+			check_report_degraded(fs_info, devid, uuid, 1);
 		}
 		map->stripes[i].dev->in_fs_metadata = 1;
 	}
@@ -6579,21 +6594,17 @@ static int read_one_dev(struct btrfs_fs_info *fs_info,
 
 	device = btrfs_find_device(fs_info, devid, dev_uuid, fs_uuid);
 	if (!device) {
-		if (!btrfs_test_opt(fs_info, DEGRADED)) {
-			btrfs_report_missing_device(fs_info, devid, dev_uuid);
+		if (check_report_degraded(fs_info, devid, dev_uuid, 0))
 			return -EINVAL;
-		}
 
 		device = add_missing_dev(fs_devices, devid, dev_uuid);
 		if (IS_ERR(device))
 			return PTR_ERR(device);
-		btrfs_report_missing_device(fs_info, devid, dev_uuid);
+		check_report_degraded(fs_info, devid, dev_uuid, 1);
 	} else {
-		if (!device->bdev) {
-			btrfs_report_missing_device(fs_info, devid, dev_uuid);
-			if (!btrfs_test_opt(fs_info, DEGRADED))
-				return -EIO;
-		}
+		if (!device->bdev &&
+			check_report_degraded(fs_info, devid, dev_uuid, 1))
+			return -EINVAL;
 
 		if(!device->bdev && !device->missing) {
 			/*
@@ -6759,12 +6770,6 @@ int btrfs_read_sys_array(struct btrfs_fs_info *fs_info)
 	return -EIO;
 }
 
-void btrfs_report_missing_device(struct btrfs_fs_info *fs_info, u64 devid,
-				 u8 *uuid)
-{
-	btrfs_warn_rl(fs_info, "devid %llu uuid %pU is missing", devid, uuid);
-}
-
 /*
  * Check if all chunks in the fs are OK for read-write degraded mount
  *
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 6c807a1c0531..e40b6f207533 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -541,7 +541,4 @@ void btrfs_set_fs_info_ptr(struct btrfs_fs_info *fs_info);
 void btrfs_reset_fs_info_ptr(struct btrfs_fs_info *fs_info);
 
 bool btrfs_check_rw_degradable(struct btrfs_fs_info *fs_info);
-void btrfs_report_missing_device(struct btrfs_fs_info *fs_info, u64 devid,
-				 u8 *uuid);
-
 #endif
-- 
2.13.1

Re: [PATCH 1/2] btrfs: fix read_one_chunk() return error code

[David Sterba]

On Fri, Oct 06, 2017 at 06:04:42PM +0800, Anand Jain wrote:
> EIO is for IO (RW) failures, use correct error return
> codes at two locations with in read_one_chunk(). Also
> adds a small cleanup.

This patch does 3 things, please split it, and drop the cleanup part.

1) switch add_missing_dev to ERR_PTR/IS_ERR return value and update
   callers
2) change return value from EIO to EINVAL in case a device is missing
   and mount is not degraded

I'm not sure that EINVAL is the right error code, as it's used when the
mount options are unrecognized or wrong. I agree that EIO is not the
right code either, as it's reserved for "true" IO errors. Possibly it
could be ENOENT or EAGAIN followed by the syslog message.