| Message ID | 1442801443-5132-4-git-send-email-quwenruo@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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; }
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(-)