[3/5] btrfs: Do per-chunk degraded check for remount
diff mbox

Message ID 1442801443-5132-4-git-send-email-quwenruo@cn.fujitsu.com
State New
Headers show

Commit Message

Qu Wenruo Sept. 21, 2015, 2:10 a.m. UTC
Just the same for mount time check, use new btrfs_check_degraded() to do
per chunk check.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 fs/btrfs/super.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Anand Jain Sept. 25, 2015, 6:54 a.m. UTC | #1
On 09/21/2015 10:10 AM, Qu Wenruo wrote:
> Just the same for mount time check, use new btrfs_check_degraded() to do
> per chunk check.
>
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> ---
>   fs/btrfs/super.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index c389c13..720c044 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -1681,11 +1681,14 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
>   			goto restore;
>   		}
>
> -		if (fs_info->fs_devices->missing_devices >
> -		     fs_info->num_tolerated_disk_barrier_failures &&
> -		    !(*flags & MS_RDONLY)) {
> +		ret = btrfs_check_degradable(fs_info, *flags);
> +		if (ret < 0) {
> +			btrfs_error(fs_info, ret,
> +				    "degraded writable remount failed");

btrfs_erorr() which is an error handling routine, isn't appropriate 
here, mainly because as we are in the remount context, I am not sure if 
you meant to change the fs state to readonly (on error) in the remount 
context ? or Instead btrfs_err() which is an error reporting/logging 
would be appropriate.

btrfs_erorr() and btrfs_err() are way different in action but very easy 
have an oversight and use the wrong one. the below patch changed it..

    Btrfs: consolidate btrfs_error() to btrfs_std_error()

Thanks, Anand


> +			goto restore;
> +		} else if (ret > 0 && !btrfs_test_opt(root, DEGRADED)) {
>   			btrfs_warn(fs_info,
> -				"too many missing devices, writeable remount is not allowed");
> +				"some device missing, but still degraded mountable, please remount with -o degraded option");
>   			ret = -EACCES;
>   			goto restore;
>   		}
>
--
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
Qu Wenruo Sept. 25, 2015, 8:08 a.m. UTC | #2
Anand Jain wrote on 2015/09/25 14:54 +0800:
>
>
> On 09/21/2015 10:10 AM, Qu Wenruo wrote:
>> Just the same for mount time check, use new btrfs_check_degraded() to do
>> per chunk check.
>>
>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>> ---
>>   fs/btrfs/super.c | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>> index c389c13..720c044 100644
>> --- a/fs/btrfs/super.c
>> +++ b/fs/btrfs/super.c
>> @@ -1681,11 +1681,14 @@ static int btrfs_remount(struct super_block
>> *sb, int *flags, char *data)
>>               goto restore;
>>           }
>>
>> -        if (fs_info->fs_devices->missing_devices >
>> -             fs_info->num_tolerated_disk_barrier_failures &&
>> -            !(*flags & MS_RDONLY)) {
>> +        ret = btrfs_check_degradable(fs_info, *flags);
>> +        if (ret < 0) {
>> +            btrfs_error(fs_info, ret,
>> +                    "degraded writable remount failed");
>
> btrfs_erorr() which is an error handling routine, isn't appropriate
> here, mainly because as we are in the remount context, I am not sure if
> you meant to change the fs state to readonly (on error) in the remount
> context ? or Instead btrfs_err() which is an error reporting/logging
> would be appropriate.
>
> btrfs_erorr() and btrfs_err() are way different in action but very easy
> have an oversight and use the wrong one. the below patch changed it..
>
>     Btrfs: consolidate btrfs_error() to btrfs_std_error()
>
> Thanks, Anand

Thanks for pointing this out.

I was quite unsure about using btrfs_info/warn/error.

In this case, I just wan't to output a dmesg info to let user know 
exactly what caused the mount failed.
Original code output nothing but "failed to open chunk tree", which is 
quite confusing for end user.

I was planning to use btrfs_info, but at least this is really an error 
message, but only to info user the real cause.

Maybe btrfs_warn will be a better choice?

Thanks,
Qu

>
>
>> +            goto restore;
>> +        } else if (ret > 0 && !btrfs_test_opt(root, DEGRADED)) {
>>               btrfs_warn(fs_info,
>> -                "too many missing devices, writeable remount is not
>> allowed");
>> +                "some device missing, but still degraded mountable,
>> please remount with -o degraded option");
>>               ret = -EACCES;
>>               goto restore;
>>           }
>>
--
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
Anand Jain Sept. 25, 2015, 8:30 a.m. UTC | #3
Qu,

Strictly speaking IMO it should be reported to the user on the cli 
terminal, and no logging in required. since its not that easy to get 
that at this point, I am ok with logging it as error. Since we are 
failing the task(mount), error is better.

I have made that change this on top of the patch

   [PATCH 1/1] Btrfs: consolidate btrfs_error() to btrfs_std_error()

and sent them both.

Thanks, Anand


> Thanks for pointing this out.
>
> I was quite unsure about using btrfs_info/warn/error.
>
> In this case, I just wan't to output a dmesg info to let user know
> exactly what caused the mount failed.
> Original code output nothing but "failed to open chunk tree", which is
> quite confusing for end user.
>
> I was planning to use btrfs_info, but at least this is really an error
> message, but only to info user the real cause.
>
> Maybe btrfs_warn will be a better choice?
>
> Thanks,
> Qu


--
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
Qu Wenruo Sept. 25, 2015, 8:34 a.m. UTC | #4
Thanks Anand,

I'm OK with both new patches.

Thanks for the modification.
Qu

Anand Jain wrote on 2015/09/25 16:30 +0800:
> Qu,
>
> Strictly speaking IMO it should be reported to the user on the cli
> terminal, and no logging in required. since its not that easy to get
> that at this point, I am ok with logging it as error. Since we are
> failing the task(mount), error is better.
>
> I have made that change this on top of the patch
>
>    [PATCH 1/1] Btrfs: consolidate btrfs_error() to btrfs_std_error()
>
> and sent them both.
>
> Thanks, Anand
>
>
>> Thanks for pointing this out.
>>
>> I was quite unsure about using btrfs_info/warn/error.
>>
>> In this case, I just wan't to output a dmesg info to let user know
>> exactly what caused the mount failed.
>> Original code output nothing but "failed to open chunk tree", which is
>> quite confusing for end user.
>>
>> I was planning to use btrfs_info, but at least this is really an error
>> message, but only to info user the real cause.
>>
>> Maybe btrfs_warn will be a better choice?
>>
>> Thanks,
>> Qu
>
>
--
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

Patch
diff mbox

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index c389c13..720c044 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1681,11 +1681,14 @@  static int btrfs_remount(struct super_block *sb, int *flags, char *data)
 			goto restore;
 		}
 
-		if (fs_info->fs_devices->missing_devices >
-		     fs_info->num_tolerated_disk_barrier_failures &&
-		    !(*flags & MS_RDONLY)) {
+		ret = btrfs_check_degradable(fs_info, *flags);
+		if (ret < 0) {
+			btrfs_error(fs_info, ret,
+				    "degraded writable remount failed");
+			goto restore;
+		} else if (ret > 0 && !btrfs_test_opt(root, DEGRADED)) {
 			btrfs_warn(fs_info,
-				"too many missing devices, writeable remount is not allowed");
+				"some device missing, but still degraded mountable, please remount with -o degraded option");
 			ret = -EACCES;
 			goto restore;
 		}