| Message ID | 20190406031426.22436-2-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] block: Add new BLK_STS_SELFTEST status | expand |
On Sat, Apr 06, 2019 at 11:14:26AM +0800, Qu Wenruo wrote: > Since we're going to support write time tree checker, it's possible that > transaction get aborted due to tree-checker, also due to new > BLK_STS_SELFTEST bit, we can distinguish real EIO error and EUCLEAN > error. > > Now add error string for EUCLEAN too. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/super.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > index 120e4340792a..a4cbbd7861d3 100644 > --- a/fs/btrfs/super.c > +++ b/fs/btrfs/super.c > @@ -86,6 +86,9 @@ const char *btrfs_decode_error(int errno) > case -ENOENT: > errstr = "No such entry"; > break; > + case -EUCLEAN: > + errstr = "Selftest failure"; > + break; EUCLEAN usually spits out "Structure needs cleaning" in userspace (and ext4 & xfs have been using it for years to complain about corrupt data), so why diverge here? Wait... does "tree-checker" mean online metadata checking, and 'selftest failure' is what gets spit out when it finds some metadata it doesn't like? --D > } > > return errstr; > -- > 2.21.0 >
On 2019/4/6 下午12:50, Darrick J. Wong wrote: > On Sat, Apr 06, 2019 at 11:14:26AM +0800, Qu Wenruo wrote: >> Since we're going to support write time tree checker, it's possible that >> transaction get aborted due to tree-checker, also due to new >> BLK_STS_SELFTEST bit, we can distinguish real EIO error and EUCLEAN >> error. >> >> Now add error string for EUCLEAN too. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/super.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c >> index 120e4340792a..a4cbbd7861d3 100644 >> --- a/fs/btrfs/super.c >> +++ b/fs/btrfs/super.c >> @@ -86,6 +86,9 @@ const char *btrfs_decode_error(int errno) >> case -ENOENT: >> errstr = "No such entry"; >> break; >> + case -EUCLEAN: >> + errstr = "Selftest failure"; >> + break; > > EUCLEAN usually spits out "Structure needs cleaning" in userspace (and > ext4 & xfs have been using it for years to complain about corrupt data), > so why diverge here? Because the original "structure needs cleaning" doesn't really show the meaning we're using. We really means, something wrong happened, thus I prefer something like selftest failure. And, the idea of adding -EUCLEAN, is to replace the original "unknown" output, if you're sticking with the original error string, we still need that branch. > > Wait... does "tree-checker" mean online metadata checking, and 'selftest > failure' is what gets spit out when it finds some metadata it doesn't > like? Yep. Thanks, Qu > > --D > >> } >> >> return errstr; >> -- >> 2.21.0 >>
On 6.04.19 г. 8:05 ч., Qu Wenruo wrote: > > > On 2019/4/6 下午12:50, Darrick J. Wong wrote: >> On Sat, Apr 06, 2019 at 11:14:26AM +0800, Qu Wenruo wrote: >>> Since we're going to support write time tree checker, it's possible that >>> transaction get aborted due to tree-checker, also due to new >>> BLK_STS_SELFTEST bit, we can distinguish real EIO error and EUCLEAN >>> error. >>> >>> Now add error string for EUCLEAN too. >>> >>> Signed-off-by: Qu Wenruo <wqu@suse.com> >>> --- >>> fs/btrfs/super.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c >>> index 120e4340792a..a4cbbd7861d3 100644 >>> --- a/fs/btrfs/super.c >>> +++ b/fs/btrfs/super.c >>> @@ -86,6 +86,9 @@ const char *btrfs_decode_error(int errno) >>> case -ENOENT: >>> errstr = "No such entry"; >>> break; >>> + case -EUCLEAN: >>> + errstr = "Selftest failure"; >>> + break; >> >> EUCLEAN usually spits out "Structure needs cleaning" in userspace (and >> ext4 & xfs have been using it for years to complain about corrupt data), >> so why diverge here? > > Because the original "structure needs cleaning" doesn't really show the > meaning we're using. On the contrary, structure needs cleaning means the filesystem has detected a consistency error. How that detection has happened shouldn't burden the user, the important fact is it has. Selftest is somewhat cryptic, I'd suggest at least "verification error" or just go with canonical "Structure needs cleaning". > > We really means, something wrong happened, thus I prefer something like > selftest failure. > > And, the idea of adding -EUCLEAN, is to replace the original "unknown" > output, if you're sticking with the original error string, we still need > that branch. > >> >> Wait... does "tree-checker" mean online metadata checking, and 'selftest >> failure' is what gets spit out when it finds some metadata it doesn't >> like? > > Yep. > > Thanks, > Qu > >> >> --D >> >>> } >>> >>> return errstr; >>> -- >>> 2.21.0 >>> >
On 2019/4/6 下午2:20, Nikolay Borisov wrote: > > > On 6.04.19 г. 8:05 ч., Qu Wenruo wrote: >> >> >> On 2019/4/6 下午12:50, Darrick J. Wong wrote: >>> On Sat, Apr 06, 2019 at 11:14:26AM +0800, Qu Wenruo wrote: >>>> Since we're going to support write time tree checker, it's possible that >>>> transaction get aborted due to tree-checker, also due to new >>>> BLK_STS_SELFTEST bit, we can distinguish real EIO error and EUCLEAN >>>> error. >>>> >>>> Now add error string for EUCLEAN too. >>>> >>>> Signed-off-by: Qu Wenruo <wqu@suse.com> >>>> --- >>>> fs/btrfs/super.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c >>>> index 120e4340792a..a4cbbd7861d3 100644 >>>> --- a/fs/btrfs/super.c >>>> +++ b/fs/btrfs/super.c >>>> @@ -86,6 +86,9 @@ const char *btrfs_decode_error(int errno) >>>> case -ENOENT: >>>> errstr = "No such entry"; >>>> break; >>>> + case -EUCLEAN: >>>> + errstr = "Selftest failure"; >>>> + break; >>> >>> EUCLEAN usually spits out "Structure needs cleaning" in userspace (and >>> ext4 & xfs have been using it for years to complain about corrupt data), >>> so why diverge here? >> >> Because the original "structure needs cleaning" doesn't really show the >> meaning we're using. > > On the contrary, structure needs cleaning means the filesystem has > detected a consistency error. How that detection has happened shouldn't > burden the user, the important fact is it has. Selftest is somewhat > cryptic, I'd suggest at least "verification error" or just go with > canonical "Structure needs cleaning". OK, I'll go that way. Thanks for the hint, Qu > >> >> We really means, something wrong happened, thus I prefer something like >> selftest failure. >> >> And, the idea of adding -EUCLEAN, is to replace the original "unknown" >> output, if you're sticking with the original error string, we still need >> that branch. >> >>> >>> Wait... does "tree-checker" mean online metadata checking, and 'selftest >>> failure' is what gets spit out when it finds some metadata it doesn't >>> like? >> >> Yep. >> >> Thanks, >> Qu >> >>> >>> --D >>> >>>> } >>>> >>>> return errstr; >>>> -- >>>> 2.21.0 >>>> >>
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index 120e4340792a..a4cbbd7861d3 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -86,6 +86,9 @@ const char *btrfs_decode_error(int errno) case -ENOENT: errstr = "No such entry"; break; + case -EUCLEAN: + errstr = "Selftest failure"; + break; } return errstr;
Since we're going to support write time tree checker, it's possible that transaction get aborted due to tree-checker, also due to new BLK_STS_SELFTEST bit, we can distinguish real EIO error and EUCLEAN error. Now add error string for EUCLEAN too. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/super.c | 3 +++ 1 file changed, 3 insertions(+)