[PATCH 0/2] Bad block notification

[PATCH 0/2] Bad block notification

[Tomasz Majchrzak]

At the moment there is no way to be notified that bad blocks have been found on
a disk. It is only possible to check manually with 'mdadm --examine-badblocks'.
User might not be aware there is a bad block for a long period. If another disk
in the array fails, data is lost.

These patches add a new event to the kernel and mdadm in order to send
notification on the first bad block on a disk. I have chosen to do it only for
first bad block as I think it's sufficient indication that the drive requires
replacement.

Tomasz Majchrzak (1):
  md: add bad block flag to disk state

 drivers/md/md.c                | 2 ++
 include/uapi/linux/raid/md_p.h | 1 +
 2 files changed, 3 insertions(+)

Tomasz Majchrzak (1):
  Monitor: add new event BadBlocks

 Monitor.c  | 14 +++++++++-----
 md_p.h     |  1 + 
 mdadm.8.in | 10 ++++++++--
 3 files changed, 18 insertions(+), 7 deletions(-)

-- 
1.8.3.1

[PATCH 1/2] md: add bad block flag to disk state

[Tomasz Majchrzak]

Add a new flag to report that bad blocks are present on a disk. It will
allow userspace to notify the user of the problem.

Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
---
 drivers/md/md.c                | 2 ++
 include/uapi/linux/raid/md_p.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 0abb147..1a807ec 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -6034,6 +6034,8 @@ static int get_disk_info(struct mddev *mddev, void __user * arg)
 			info.state |= (1<<MD_DISK_WRITEMOSTLY);
 		if (test_bit(FailFast, &rdev->flags))
 			info.state |= (1<<MD_DISK_FAILFAST);
+		if (rdev->badblocks.count)
+			info.state |= (1<<MD_DISK_BB_PRESENT);
 	} else {
 		info.major = info.minor = 0;
 		info.raid_disk = -1;
diff --git a/include/uapi/linux/raid/md_p.h b/include/uapi/linux/raid/md_p.h
index 9930f3e..b151e93 100644
--- a/include/uapi/linux/raid/md_p.h
+++ b/include/uapi/linux/raid/md_p.h
@@ -93,6 +93,7 @@
 				   * read requests will only be sent here in
 				   * dire need
 				   */
+#define MD_DISK_BB_PRESENT	11 /* disk has bad blocks */
 #define MD_DISK_JOURNAL		18 /* disk is used as the write journal in RAID-5/6 */
 
 #define MD_DISK_ROLE_SPARE	0xffff
-- 
1.8.3.1

[PATCH 2/2] Monitor: add new event BadBlocks

[Tomasz Majchrzak]

Add new event BadBlocks to notify the user when bad blocks are found on a
disk. Send an email (if configured) and write it to syslog as a warning.

Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
---
 Monitor.c  | 14 +++++++++-----
 md_p.h     |  1 +
 mdadm.8.in | 10 ++++++++--
 3 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/Monitor.c b/Monitor.c
index 802a9d9..cb92a23 100644
--- a/Monitor.c
+++ b/Monitor.c
@@ -363,10 +363,11 @@ static void alert(char *event, char *dev, char *disc, struct alert_info *info)
 		}
 	}
 	if (info->mailaddr &&
-	    (strncmp(event, "Fail", 4)==0 ||
-	     strncmp(event, "Test", 4)==0 ||
-	     strncmp(event, "Spares", 6)==0 ||
-	     strncmp(event, "Degrade", 7)==0)) {
+	    (strncmp(event, "Fail", 4) == 0 ||
+	     strncmp(event, "Test", 4) == 0 ||
+	     strncmp(event, "Spares", 6) == 0 ||
+	     strncmp(event, "Degrade", 7) == 0 ||
+	     strncmp(event, "BadBlocks", 9) == 0)) {
 		FILE *mp = popen(Sendmail, "w");
 		if (mp) {
 			FILE *mdstat;
@@ -422,7 +423,8 @@ static void alert(char *event, char *dev, char *disc, struct alert_info *info)
 		/* Good to know about, but are not failures: */
 		else if (strncmp(event, "Rebuild", 7)==0 ||
 			 strncmp(event, "MoveSpare", 9)==0 ||
-			 strncmp(event, "Spares", 6) != 0)
+			 strncmp(event, "Spares", 6) != 0 ||
+			 strncmp(event, "BadBlocks", 9) != 0)
 			priority = LOG_WARNING;
 		/* Everything else: */
 		else
@@ -668,6 +670,8 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
 				alert("FailSpare", dev, dv, ainfo);
 			else if ((newstate&change)&(1<<MD_DISK_SYNC))
 				alert("SpareActive", dev, dv, ainfo);
+			else if ((newstate&change)&(1<<MD_DISK_BB_PRESENT))
+				alert("BadBlocks", dev, dv, ainfo);
 		}
 		st->devstate[i] = newstate;
 		st->devid[i] = makedev(disc.major, disc.minor);
diff --git a/md_p.h b/md_p.h
index dc9fec1..39884be 100644
--- a/md_p.h
+++ b/md_p.h
@@ -90,6 +90,7 @@
 				   * dire need
 				   */
 #define	MD_DISK_FAILFAST	10 /* Fewer retries, more failures */
+#define MD_DISK_BB_PRESENT	11 /* disk has bad blocks */
 
 #define MD_DISK_REPLACEMENT	17
 #define MD_DISK_JOURNAL		18 /* disk is used as the write journal in RAID-5/6 */
diff --git a/mdadm.8.in b/mdadm.8.in
index 1e4f91d..7b89a8a 100644
--- a/mdadm.8.in
+++ b/mdadm.8.in
@@ -2552,6 +2552,10 @@ message.
 (syslog priority: Warning)
 
 .TP
+.B BadBlocks
+Bad blocks have been found on the device. (syslog priority: Warning)
+
+.TP
 .B TestMessage
 An array was found at startup, and the
 .B \-\-test
@@ -2563,7 +2567,8 @@ Only
 .B Fail,
 .B FailSpare,
 .B DegradedArray,
-.B SparesMissing
+.B SparesMissing,
+.B BadBlocks
 and
 .B TestMessage
 cause Email to be sent.  All events cause the program to be run.
@@ -2575,8 +2580,9 @@ Each event has an associated array device (e.g.
 and possibly a second device.  For
 .BR Fail ,
 .BR FailSpare ,
+.BR SpareActive
 and
-.B SpareActive
+.BR BadBlocks
 the second device is the relevant component device.
 For
 .B MoveSpare
-- 
1.8.3.1

Re: [PATCH 0/2] Bad block notification

[jes.sorensen]

Tomasz Majchrzak <tomasz.majchrzak@intel.com> writes:
> At the moment there is no way to be notified that bad blocks have been found on
> a disk. It is only possible to check manually with 'mdadm --examine-badblocks'.
> User might not be aware there is a bad block for a long period. If another disk
> in the array fails, data is lost.
>
> These patches add a new event to the kernel and mdadm in order to send
> notification on the first bad block on a disk. I have chosen to do it only for
> first bad block as I think it's sufficient indication that the drive requires
> replacement.

Tomasz,

Looks reasonable to me - I'll wait for Shaohua to respond on the kernel
part before I apply the mdadm part.

Cheers,
Jes

Re: [PATCH 1/2] md: add bad block flag to disk state

[Shaohua Li]

On Tue, Jan 24, 2017 at 01:03:38PM +0100, Tomasz Majchrzak wrote:
> Add a new flag to report that bad blocks are present on a disk. It will
> allow userspace to notify the user of the problem.
> 
> Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
> ---
>  drivers/md/md.c                | 2 ++
>  include/uapi/linux/raid/md_p.h | 1 +
>  2 files changed, 3 insertions(+)
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 0abb147..1a807ec 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -6034,6 +6034,8 @@ static int get_disk_info(struct mddev *mddev, void __user * arg)
>  			info.state |= (1<<MD_DISK_WRITEMOSTLY);
>  		if (test_bit(FailFast, &rdev->flags))
>  			info.state |= (1<<MD_DISK_FAILFAST);
> +		if (rdev->badblocks.count)
> +			info.state |= (1<<MD_DISK_BB_PRESENT);

Userspace can find if a disk has badblocks by reading the bad_blocks sysfs
file. Why adds another interface?

Thanks,
Shaohua

>  	} else {
>  		info.major = info.minor = 0;
>  		info.raid_disk = -1;
> diff --git a/include/uapi/linux/raid/md_p.h b/include/uapi/linux/raid/md_p.h
> index 9930f3e..b151e93 100644
> --- a/include/uapi/linux/raid/md_p.h
> +++ b/include/uapi/linux/raid/md_p.h
> @@ -93,6 +93,7 @@
>  				   * read requests will only be sent here in
>  				   * dire need
>  				   */
> +#define MD_DISK_BB_PRESENT	11 /* disk has bad blocks */
>  #define MD_DISK_JOURNAL		18 /* disk is used as the write journal in RAID-5/6 */
>  
>  #define MD_DISK_ROLE_SPARE	0xffff
> -- 
> 1.8.3.1
> 

Re: [PATCH 1/2] md: add bad block flag to disk state

[Tomasz Majchrzak]

On Mon, Jan 30, 2017 at 03:33:41PM -0800, Shaohua Li wrote:
> On Tue, Jan 24, 2017 at 01:03:38PM +0100, Tomasz Majchrzak wrote:
> > Add a new flag to report that bad blocks are present on a disk. It will
> > allow userspace to notify the user of the problem.
> > 
> > Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
> > ---
> >  drivers/md/md.c                | 2 ++
> >  include/uapi/linux/raid/md_p.h | 1 +
> >  2 files changed, 3 insertions(+)
> > 
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index 0abb147..1a807ec 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -6034,6 +6034,8 @@ static int get_disk_info(struct mddev *mddev, void __user * arg)
> >  			info.state |= (1<<MD_DISK_WRITEMOSTLY);
> >  		if (test_bit(FailFast, &rdev->flags))
> >  			info.state |= (1<<MD_DISK_FAILFAST);
> > +		if (rdev->badblocks.count)
> > +			info.state |= (1<<MD_DISK_BB_PRESENT);
> 
> Userspace can find if a disk has badblocks by reading the bad_blocks sysfs
> file. Why adds another interface?
> 
> Thanks,
> Shaohua

Yes, indeed, it can. I have chosen to do it this way to keep it consistent
with mdadm which uses GET_DISK_INFO ioctl to get disk information. All data
provided in this ioctl is also available in sysfs file (rdev state), however
ioctl is still used (legacy). The same applies for details subcommand of
mdadm. To answer your question - yes, I could avoid new flag but it would
make mdadm side of my improvement much more complicated.

Tomek

> >  	} else {
> >  		info.major = info.minor = 0;
> >  		info.raid_disk = -1;
> > diff --git a/include/uapi/linux/raid/md_p.h b/include/uapi/linux/raid/md_p.h
> > index 9930f3e..b151e93 100644
> > --- a/include/uapi/linux/raid/md_p.h
> > +++ b/include/uapi/linux/raid/md_p.h
> > @@ -93,6 +93,7 @@
> >  				   * read requests will only be sent here in
> >  				   * dire need
> >  				   */
> > +#define MD_DISK_BB_PRESENT	11 /* disk has bad blocks */
> >  #define MD_DISK_JOURNAL		18 /* disk is used as the write journal in RAID-5/6 */
> >  
> >  #define MD_DISK_ROLE_SPARE	0xffff
> > -- 
> > 1.8.3.1
> > 

Re: [PATCH 1/2] md: add bad block flag to disk state

[Shaohua Li]

On Wed, Feb 01, 2017 at 10:53:52AM +0100, Tomasz Majchrzak wrote:
> On Mon, Jan 30, 2017 at 03:33:41PM -0800, Shaohua Li wrote:
> > On Tue, Jan 24, 2017 at 01:03:38PM +0100, Tomasz Majchrzak wrote:
> > > Add a new flag to report that bad blocks are present on a disk. It will
> > > allow userspace to notify the user of the problem.
> > > 
> > > Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
> > > ---
> > >  drivers/md/md.c                | 2 ++
> > >  include/uapi/linux/raid/md_p.h | 1 +
> > >  2 files changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > > index 0abb147..1a807ec 100644
> > > --- a/drivers/md/md.c
> > > +++ b/drivers/md/md.c
> > > @@ -6034,6 +6034,8 @@ static int get_disk_info(struct mddev *mddev, void __user * arg)
> > >  			info.state |= (1<<MD_DISK_WRITEMOSTLY);
> > >  		if (test_bit(FailFast, &rdev->flags))
> > >  			info.state |= (1<<MD_DISK_FAILFAST);
> > > +		if (rdev->badblocks.count)
> > > +			info.state |= (1<<MD_DISK_BB_PRESENT);
> > 
> > Userspace can find if a disk has badblocks by reading the bad_blocks sysfs
> > file. Why adds another interface?
> > 
> > Thanks,
> > Shaohua
> 
> Yes, indeed, it can. I have chosen to do it this way to keep it consistent
> with mdadm which uses GET_DISK_INFO ioctl to get disk information. All data
> provided in this ioctl is also available in sysfs file (rdev state), however
> ioctl is still used (legacy). The same applies for details subcommand of
> mdadm. To answer your question - yes, I could avoid new flag but it would
> make mdadm side of my improvement much more complicated.

I intended to avoid adding new user interface if possible. Not sure about this
case though. How complicated in the mdadm side if we use the bad_block sysfs
file?

Jes, how do you think from the mdadm side?

Thanks,
Shaohua

Re: [PATCH 1/2] md: add bad block flag to disk state

[jes.sorensen]

Shaohua Li <shli@kernel.org> writes:
> On Wed, Feb 01, 2017 at 10:53:52AM +0100, Tomasz Majchrzak wrote:
>> On Mon, Jan 30, 2017 at 03:33:41PM -0800, Shaohua Li wrote:
>> > On Tue, Jan 24, 2017 at 01:03:38PM +0100, Tomasz Majchrzak wrote:
>> > > Add a new flag to report that bad blocks are present on a disk. It will
>> > > allow userspace to notify the user of the problem.
>> > > 
>> > > Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
>> > > ---
>> > >  drivers/md/md.c                | 2 ++
>> > >  include/uapi/linux/raid/md_p.h | 1 +
>> > >  2 files changed, 3 insertions(+)
>> > > 
>> > > diff --git a/drivers/md/md.c b/drivers/md/md.c
>> > > index 0abb147..1a807ec 100644
>> > > --- a/drivers/md/md.c
>> > > +++ b/drivers/md/md.c
>> > > @@ -6034,6 +6034,8 @@ static int get_disk_info(struct mddev *mddev, void __user * arg)
>> > >  			info.state |= (1<<MD_DISK_WRITEMOSTLY);
>> > >  		if (test_bit(FailFast, &rdev->flags))
>> > >  			info.state |= (1<<MD_DISK_FAILFAST);
>> > > +		if (rdev->badblocks.count)
>> > > +			info.state |= (1<<MD_DISK_BB_PRESENT);
>> > 
>> > Userspace can find if a disk has badblocks by reading the bad_blocks sysfs
>> > file. Why adds another interface?
>> > 
>> > Thanks,
>> > Shaohua
>> 
>> Yes, indeed, it can. I have chosen to do it this way to keep it consistent
>> with mdadm which uses GET_DISK_INFO ioctl to get disk information. All data
>> provided in this ioctl is also available in sysfs file (rdev state), however
>> ioctl is still used (legacy). The same applies for details subcommand of
>> mdadm. To answer your question - yes, I could avoid new flag but it would
>> make mdadm side of my improvement much more complicated.
>
> I intended to avoid adding new user interface if possible. Not sure about this
> case though. How complicated in the mdadm side if we use the bad_block sysfs
> file?
>
> Jes, how do you think from the mdadm side?

Hi,

Sorry for being so late getting back on this, I am just getting back to
this now.

I am really split on this - if we have spare flags available, I guess
it's not the end of the World. On the other hand there is something to
be said for not adding any more using the old interface.

Right now mdadm relies heavily on the ioctl interfaces. In order to
migrate away from that, we need to spend a fair amount of time to
rewriting the general interface first. Something I think we should
invest some time into doing, but having to handle both in parallel seems
a bad idea to me.

Cheers,
Jes

Re: [PATCH 1/2] md: add bad block flag to disk state

[Hannes Reinecke]

On 03/06/2017 09:23 PM, jes.sorensen@gmail.com wrote:
> Shaohua Li <shli@kernel.org> writes:
>> On Wed, Feb 01, 2017 at 10:53:52AM +0100, Tomasz Majchrzak wrote:
>>> On Mon, Jan 30, 2017 at 03:33:41PM -0800, Shaohua Li wrote:
>>>> On Tue, Jan 24, 2017 at 01:03:38PM +0100, Tomasz Majchrzak wrote:
>>>>> Add a new flag to report that bad blocks are present on a disk. It will
>>>>> allow userspace to notify the user of the problem.
>>>>>
>>>>> Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
>>>>> ---
>>>>>  drivers/md/md.c                | 2 ++
>>>>>  include/uapi/linux/raid/md_p.h | 1 +
>>>>>  2 files changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>>>>> index 0abb147..1a807ec 100644
>>>>> --- a/drivers/md/md.c
>>>>> +++ b/drivers/md/md.c
>>>>> @@ -6034,6 +6034,8 @@ static int get_disk_info(struct mddev *mddev, void __user * arg)
>>>>>  			info.state |= (1<<MD_DISK_WRITEMOSTLY);
>>>>>  		if (test_bit(FailFast, &rdev->flags))
>>>>>  			info.state |= (1<<MD_DISK_FAILFAST);
>>>>> +		if (rdev->badblocks.count)
>>>>> +			info.state |= (1<<MD_DISK_BB_PRESENT);
>>>>
>>>> Userspace can find if a disk has badblocks by reading the bad_blocks sysfs
>>>> file. Why adds another interface?
>>>>
>>>> Thanks,
>>>> Shaohua
>>>
>>> Yes, indeed, it can. I have chosen to do it this way to keep it consistent
>>> with mdadm which uses GET_DISK_INFO ioctl to get disk information. All data
>>> provided in this ioctl is also available in sysfs file (rdev state), however
>>> ioctl is still used (legacy). The same applies for details subcommand of
>>> mdadm. To answer your question - yes, I could avoid new flag but it would
>>> make mdadm side of my improvement much more complicated.
>>
>> I intended to avoid adding new user interface if possible. Not sure about this
>> case though. How complicated in the mdadm side if we use the bad_block sysfs
>> file?
>>
>> Jes, how do you think from the mdadm side?
> 
> Hi,
> 
> Sorry for being so late getting back on this, I am just getting back to
> this now.
> 
> I am really split on this - if we have spare flags available, I guess
> it's not the end of the World. On the other hand there is something to
> be said for not adding any more using the old interface.
> 
> Right now mdadm relies heavily on the ioctl interfaces. In order to
> migrate away from that, we need to spend a fair amount of time to
> rewriting the general interface first. Something I think we should
> invest some time into doing, but having to handle both in parallel seems
> a bad idea to me.
> 
Personally I would positively _love_ a consistent interface.
ATM we have about 3 (or was it 4?) different interfaces, all of which
providing _different_ information.
(Not to mention the odd stall when reading that information...)

Coalescing all of that into _one_ set, or, better still, make sure that
one interface (sysfs?) provides all information would be a massive bonus.
And would finally enable us to provide an API for library usage.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)