* Re: Failure propagation of concatenated raids ?
From: Benjamin ESTRABAUD @ 2016-06-15 9:29 UTC (permalink / raw)
To: Nicolas Noble, John Stoffel; +Cc: linux-raid
In-Reply-To: <CAAkR8+vfXJOtYUBvkZXxQhXwh-e2t6yGuarLW4MGS8B+YKtUZg@mail.gmail.com>
On 15/06/16 10:18, Nicolas Noble wrote:
>> it
>> *might* make sense to look at ceph or some other distributed
>> filesystem.
>
> I was trying to avoid that, mainly because that doesn't seem to be as
> supported as a more straightforward raids+lvm2 scenario. But I might
> be willing to reconsider my position in light of such data losses.
>
>> no filesystem I know handles that without either going
>> readonly, or totally locking up.
>
> Which, to be fair, is exactly what I'm looking for. I'd rather see the
> filesystem lock itself up, until a human tries to restore the failed
> raid back online. But my recent experience and experiments show me
> that the filesystems actually don't lock themselves up, and don't go
> read only for quite some time, and heavy heavy data corruption will
> then happen. I'd be much more happy if the behavior was that the
> filesystem locks itself up instead of self destroying over time.
Hi Nicolas,
I have limited experience in that domain but I've usually observed that
if the filesystem (say xfs) is unable to read or write its superblock it
immediately goes into read only mode. MD will remain online and provide
"best service" whenever possible, but as you pointed out this can be
risky if you still think your RAID offers parity protection while
degraded. I think in your case you're better off stopping an array that
has less than parity drives than it should, either using a udev rule or
using mdadm --monitor.
Regards,
Ben.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: Failure propagation of concatenated raids ?
From: Nicolas Noble @ 2016-06-15 9:49 UTC (permalink / raw)
To: Benjamin ESTRABAUD; +Cc: John Stoffel, linux-raid
In-Reply-To: <57612012.9080902@mpstor.com>
> I
> think in your case you're better off stopping an array that has less than
> parity drives than it should, either using a udev rule or using mdadm
> --monitor.
I actually have been unsuccessful in these attempts so far. What
happens is that you very quickly get processes that get indefinitely
stuck (indefinitely as in 'waiting on a very very long kernel
timeout') trying to write something, so that the ext4fs layer becomes
unresponsive on these threads, or take a very long time. Killing the
processes takes a very long time because they are stuck in a kernel
operation. And if potentially more processes can spawn back up, the
automated script starts an interesting game of whack-a-mole in order
to unmount the filesystem.
And you can't stop the underlying arrays without first stopping the
whole chain (umount, stop the lvm volume, etc...), otherwise you
simply get "device is busy" errors, hence the whack-a-mole process
killing. The only working method I've managed to successfully
implement is to programatically loop over the list of all the drives
involved in the filesystem, on all the raids involved, and flag all of
them as failed drives. This way, you get to really put "emergency
brakes" on. I find that to be a very, very scary method however.
^ permalink raw reply
* [PATCH] mdadm: protecting sys_name overflow
From: Nikhil Kshirsagar @ 2016-06-15 10:09 UTC (permalink / raw)
To: linux-raid
[-- Attachment #1: Type: text/plain, Size: 249 bytes --]
Hello,
Devices with names larger than 31 bytes will overflow the sys_name array.
This patch enables mdadm to fail and log a message if a long device name
is going to cause a buffer overflow.
Signed-off-by: Nikhil Kshirsagar <nkshirsa@redhat.com>
[-- Attachment #2: 0001-Protecting-overflow-of-sys_name.-If-a-long-device-na.patch --]
[-- Type: text/x-patch, Size: 1046 bytes --]
From 705aec84c6abf5b09c4202aec7cade9824ca7f12 Mon Sep 17 00:00:00 2001
From: root <root@nkshirsa.pnq.csb>
Date: Wed, 15 Jun 2016 15:23:12 +0530
Subject: [PATCH] Protecting overflow of sys_name. If a long device name is
going to cause a buffer overflow, we fail with a log message.
---
sysfs.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/sysfs.c b/sysfs.c
index 8379ca8..68b8b95 100644
--- a/sysfs.c
+++ b/sysfs.c
@@ -283,6 +283,15 @@ struct mdinfo *sysfs_read(int fd, char *devnm, unsigned long options)
}
}
+ /* strlen computes length of string *not* including the terminating null character. */
+
+ if(strlen(de->d_name) >= sizeof(dev->sys_name))
+ {
+ pr_err("Device name %s larger than currently supported by mdadm\n",de->d_name);
+ free(dev);
+ goto abort;
+
+ }
strcpy(dev->sys_name, de->d_name);
dev->disk.raid_disk = strtoul(buf, &ep, 10);
if (*ep) dev->disk.raid_disk = -1;
--
1.8.3.1
^ permalink raw reply related
* Re: Failure propagation of concatenated raids ?
From: Benjamin ESTRABAUD @ 2016-06-15 14:45 UTC (permalink / raw)
To: Nicolas Noble; +Cc: John Stoffel, linux-raid
In-Reply-To: <CAAkR8+tkvwZneqz2XpwMEqKCXZk=HERXPu9XC0pNX0OZ-AQ8ZA@mail.gmail.com>
On 15/06/16 10:49, Nicolas Noble wrote:
>> I
>> think in your case you're better off stopping an array that has less than
>> parity drives than it should, either using a udev rule or using mdadm
>> --monitor.
>
> I actually have been unsuccessful in these attempts so far. What
> happens is that you very quickly get processes that get indefinitely
> stuck (indefinitely as in 'waiting on a very very long kernel
> timeout') trying to write something, so that the ext4fs layer becomes
> unresponsive on these threads, or take a very long time. Killing the
> processes takes a very long time because they are stuck in a kernel
> operation. And if potentially more processes can spawn back up, the
> automated script starts an interesting game of whack-a-mole in order
> to unmount the filesystem.
>
> And you can't stop the underlying arrays without first stopping the
> whole chain (umount, stop the lvm volume, etc...), otherwise you
> simply get "device is busy" errors, hence the whack-a-mole process
> killing. The only working method I've managed to successfully
> implement is to programatically loop over the list of all the drives
> involved in the filesystem, on all the raids involved, and flag all of
> them as failed drives. This way, you get to really put "emergency
> brakes" on. I find that to be a very, very scary method however.
>
I understand your concern, but I remember a thread where it was
explained that a RAID0 or linear one basically behaves like a hard drive
would: since there is no parity and the data is distributed, if say half
of the devices of the RAID0 are unavailable the LBAs on the other half
of that RAID will work fine, like if you had a SSD with half of its
cells broken. So your issue seems to be more related with dealing with
IO errors than anything. I would imagine that if the filesystem's
superblock was to become unread/writeable (if it was on the missing
RAID) then the filesystem would "fail" (be remounted as readonly). Other
than that there's not much to be done apart from instructing your
program to stop the IOs and/or fiddling with timeouts to speed up the
failure of the process.
The "emergency brake" as you put it would work similarly to a RAID5
losing more than it can: the array will error every write sent to it.
Alternatively you could disconnect the drives from Linux using the
"delete" sysfs property. If you use a journalled filesystem you
shouldn't lose any data over any of this anyways, so that seems safe.
HTH,
Regards,
Ben.
^ permalink raw reply
* Re: Failure propagation of concatenated raids ?
From: John Stoffel @ 2016-06-15 14:56 UTC (permalink / raw)
To: Nicolas Noble; +Cc: John Stoffel, linux-raid
In-Reply-To: <CAAkR8+vfXJOtYUBvkZXxQhXwh-e2t6yGuarLW4MGS8B+YKtUZg@mail.gmail.com>
>>>>> "Nicolas" == Nicolas Noble <nicolas@nobis-crew.org> writes:
>> it
>> *might* make sense to look at ceph or some other distributed
>> filesystem.
Nicolas> I was trying to avoid that, mainly because that doesn't seem
Nicolas> to be as supported as a more straightforward raids+lvm2
Nicolas> scenario. But I might be willing to reconsider my position in
Nicolas> light of such data losses.
If you are building multiple RAID sets, and then stripping across them
using LVM and then putting filesystems on top of them, you should be
ok if your underlying RAID is robust.
By that I mean splitting members across controllers, so as to avoid
single points of failure. You would also use RAID6 with hot spares as
well. Once you have a robust foundation, then the filesystem layered
on top doesn't have to worry as much about part of the storage going
away.
But if you're not willing, or can't afford the cost of true no single
point of failure, then you have to take your chances. This is why I
tend to mirror my system at home and even do triple mirrors at points
for data I really care about.
>> no filesystem I know handles that without either going
>> readonly, or totally locking up.
Nicolas> Which, to be fair, is exactly what I'm looking for. I'd
Nicolas> rather see the filesystem lock itself up, until a human tries
Nicolas> to restore the failed raid back online. But my recent
Nicolas> experience and experiments show me that the filesystems
Nicolas> actually don't lock themselves up, and don't go read only for
Nicolas> quite some time, and heavy heavy data corruption will then
Nicolas> happen. I'd be much more happy if the behavior was that the
Nicolas> filesystem locks itself up instead of self destroying over
Nicolas> time.
Part of the problem is that if the filesystem isn't writing to that
section of the device, it might not know about the failure in time,
esp if they're seperate devices. Now I would think that LVM would
notice that a PV in a VG has gone away, but it then needs to
percolate up to check the LV(s) on that PV which then needs to notify
the filesystem.
I agree it should work, and should be more robust, and it might
actually be possible to tweak the system to be more hair trigger about
going into lock down mode.
Of course the other option is for you to shard your data across
multiple filesystems, and pot the resiliency into your application, so
that if some of the data can't be found, it just keeps going. But
that's a different sort of complexity as well.
John
^ permalink raw reply
* Re: Failure propagation of concatenated raids ?
From: John Stoffel @ 2016-06-15 14:59 UTC (permalink / raw)
To: Nicolas Noble; +Cc: Benjamin ESTRABAUD, John Stoffel, linux-raid
In-Reply-To: <CAAkR8+tkvwZneqz2XpwMEqKCXZk=HERXPu9XC0pNX0OZ-AQ8ZA@mail.gmail.com>
>>>>> "Nicolas" == Nicolas Noble <nicolas@nobis-crew.org> writes:
>> I
>> think in your case you're better off stopping an array that has less than
>> parity drives than it should, either using a udev rule or using mdadm
>> --monitor.
Nicolas> I actually have been unsuccessful in these attempts so far. What
Nicolas> happens is that you very quickly get processes that get indefinitely
Nicolas> stuck (indefinitely as in 'waiting on a very very long kernel
Nicolas> timeout') trying to write something, so that the ext4fs layer becomes
Nicolas> unresponsive on these threads, or take a very long time. Killing the
Nicolas> processes takes a very long time because they are stuck in a kernel
Nicolas> operation. And if potentially more processes can spawn back up, the
Nicolas> automated script starts an interesting game of whack-a-mole in order
Nicolas> to unmount the filesystem.
Nicolas> And you can't stop the underlying arrays without first
Nicolas> stopping the whole chain (umount, stop the lvm volume,
Nicolas> etc...), otherwise you simply get "device is busy" errors,
Nicolas> hence the whack-a-mole process killing. The only working
Nicolas> method I've managed to successfully implement is to
Nicolas> programatically loop over the list of all the drives involved
Nicolas> in the filesystem, on all the raids involved, and flag all of
Nicolas> them as failed drives. This way, you get to really put
Nicolas> "emergency brakes" on. I find that to be a very, very scary
Nicolas> method however.
I think this is the wrong idea. You do want MD to re-try errors on
underlying devices, because some drives will return an error, and if
MD has long enough timeouts, it can recover and try to re-write the
bad sector(s) on the drive, which early on will let the bad block be
mapped out and new block put in place.
But you're looking for a solution when one device in a stripped RAID0
goes away, what happens to the filesystem then. And in that case your
shit out of luck. No filesystem is designed to cope with that type of
failure.
So there might be ext4 or xfs or jfs options which will help you in
this case, but it's not a simple thing to program around. Esp once
the size of the volume gets really big.
John
^ permalink raw reply
* Re: BLKZEROOUT not zeroing md dev on VMDK
From: Arvind Kumar @ 2016-06-15 18:17 UTC (permalink / raw)
To: Sitsofe Wheeler, Tom Yan
Cc: Darrick J. Wong, Shaohua Li, Jens Axboe, VMware PV-Drivers,
linux-raid@vger.kernel.org, linux-scsi@vger.kernel.org,
linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
Petr Vandrovec
In-Reply-To: <CALjAwxgdr1zSgZZzNA9BQ8Ta+UuutJ6iuBnqZfmV-BTCDL2PbA@mail.gmail.com>
It is possibly some race. We saw a WRITE SAME related issue in past for which Petr sent out a patch but looks like the patch didn't make it. :(
https://groups.google.com/forum/#!topic/linux.kernel/1WGDSlyY0y0
Thanks!
Arvind
________________________________________
From: Sitsofe Wheeler <sitsofe@gmail.com>
Sent: Tuesday, May 31, 2016 10:04 PM
To: Tom Yan
Cc: Darrick J. Wong; Shaohua Li; Jens Axboe; Arvind Kumar; VMware PV-Drivers; linux-raid@vger.kernel.org; linux-scsi@vger.kernel.org; linux-block@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: BLKZEROOUT not zeroing md dev on VMDK
On 27 May 2016 at 10:30, Tom Yan <tom.ty89@gmail.com> wrote:
> There seems to be some sort of race condition between
> blkdev_issue_zeroout() and the scsi disk driver (disabling write same
> after an illegal request). On my UAS drive, sometimes `blkdiscard -z
> /dev/sdX` will return right away, even though if I then check
> `write_same_max_bytes` it has turned 0. Sometimes it will just write
> zero with SCSI WRITE even if `write_same_max_bytes` is 33553920 before
> I issue `blkdiscard -z` (`write_same_max_bytes` also turned 0, as
> expected).
>
> Not sure if it is directly related to the case here though.
I'm not aware of hitting that particular problem myself directly on
the underlying "SCSI" device but the patch on
https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_9137311_&d=CwIBaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=bUMaNc7nC9xbXtaMJrOvIIPNpPH0chY2kdRsskQn6GY&m=rx_5ntfhkt2GOpfjpiQjoCb5n4gCY7jKznXO0gKYcVI&s=W1F45VBu8NDxu2ImcbKM5b3d6UnUCLGgH8xEM9e6JQk&e= should be able to resolve
that issue. Could you test it and follow up on
https://urldefense.proofpoint.com/v2/url?u=http-3A__permalink.gmane.org_gmane.linux.kernel_2229377&d=CwIBaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=bUMaNc7nC9xbXtaMJrOvIIPNpPH0chY2kdRsskQn6GY&m=rx_5ntfhkt2GOpfjpiQjoCb5n4gCY7jKznXO0gKYcVI&s=9ekqmTk18vzcwcY0SSMF8AZnJ_lWezFIM8tDvQqeDHI&e= ? I'm hoping
more testing reports will lead to the patch being reviewed and
accepted sooner rather than later as it's currently stalled...
--
Sitsofe | https://urldefense.proofpoint.com/v2/url?u=http-3A__sucs.org_-7Esits_&d=CwIBaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=bUMaNc7nC9xbXtaMJrOvIIPNpPH0chY2kdRsskQn6GY&m=rx_5ntfhkt2GOpfjpiQjoCb5n4gCY7jKznXO0gKYcVI&s=arwniVbdl5KJZfyreWLhq-WUlgvKAf_eW1i6D2GbFGQ&e=
^ permalink raw reply
* Re: [PATCH] mdadm: protecting sys_name overflow
From: Jes Sorensen @ 2016-06-15 18:40 UTC (permalink / raw)
To: Nikhil Kshirsagar; +Cc: linux-raid
In-Reply-To: <57612976.6030801@redhat.com>
Nikhil Kshirsagar <nkshirsa@redhat.com> writes:
> Hello,
>
> Devices with names larger than 31 bytes will overflow the sys_name array.
>
> This patch enables mdadm to fail and log a message if a long device name
> is going to cause a buffer overflow.
>
> Signed-off-by: Nikhil Kshirsagar <nkshirsa@redhat.com>
>
> From 705aec84c6abf5b09c4202aec7cade9824ca7f12 Mon Sep 17 00:00:00 2001
> From: root <root@nkshirsa.pnq.csb>
> Date: Wed, 15 Jun 2016 15:23:12 +0530
> Subject: [PATCH] Protecting overflow of sys_name. If a long device name is
> going to cause a buffer overflow, we fail with a log message.
>
> ---
> sysfs.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/sysfs.c b/sysfs.c
> index 8379ca8..68b8b95 100644
> --- a/sysfs.c
> +++ b/sysfs.c
> @@ -283,6 +283,15 @@ struct mdinfo *sysfs_read(int fd, char *devnm, unsigned long options)
> }
>
> }
> + /* strlen computes length of string *not* including the terminating null character. */
> +
> + if(strlen(de->d_name) >= sizeof(dev->sys_name))
> + {
> + pr_err("Device name %s larger than currently supported by mdadm\n",de->d_name);
> + free(dev);
> + goto abort;
> +
> + }
Nikhil,
Please respect the coding style of mdadm. Code indents at 8 characters
using a tab, not whitespace, and brackets should be placed correctly.
Just like with kernel code.
Jes
^ permalink raw reply
* Re: [PATCH 1/1] MDADM:Check mdinfo->reshape_active more times before calling Grow_continue
From: Jes Sorensen @ 2016-06-15 20:27 UTC (permalink / raw)
To: Xiao Ni; +Cc: linux-raid
In-Reply-To: <1465955832-3781-1-git-send-email-xni@redhat.com>
Xiao Ni <xni@redhat.com> writes:
> Hi Jes
>
> Thanks for reminding me. This is the patch after modifying.
>
> When reshaping a 3 drives raid5 to 4 drives raid5, there is a chance that
> it can't start the reshape. If the disks are not enough to have spaces for
> relocating the data_offset, it needs to call start_reshape and then run
> mdadm --grow --continue by systemd. But mdadm --grow --continue fails
> because it checkes that info->reshape_active is 0.
>
> The info->reshape_active is got from the superblock of underlying devices.
> Function start_reshape write reshape to /sys/../sync_action. Before writing
> latest superblock to underlying devices, mdadm --grow --continue is called.
> There is a chance info->reshape_active is 0. We should wait for superblock
> updating more time before calling Grow_continue.
>
> Regards
> Xiao
Xiao,
Sorry but this version doesn't compile:
Grow.c: In function 'Grow_continue_command':
Grow.c:4838:12: error: 'cnt' undeclared (first use in this function)
} while (cnt-- > 0);
^
Also please try to separate the email comments from the patch message. I
normally save the message and use git am to integrate it.
Thanks,
Jes
^ permalink raw reply
* Re: BLKZEROOUT not zeroing md dev on VMDK
From: Sitsofe Wheeler @ 2016-06-15 21:33 UTC (permalink / raw)
To: Arvind Kumar
Cc: Tom Yan, Darrick J. Wong, Shaohua Li, Jens Axboe,
VMware PV-Drivers, linux-raid@vger.kernel.org,
linux-scsi@vger.kernel.org, linux-block@vger.kernel.org,
linux-kernel@vger.kernel.org, Petr Vandrovec
In-Reply-To: <1466014718999.6976@vmware.com>
On Wed, Jun 15, 2016 at 06:17:37PM +0000, Arvind Kumar wrote:
> It is possibly some race. We saw a WRITE SAME related issue in past
> for which Petr sent out a patch but looks like the patch didn't make
> it. :(
>
> https://groups.google.com/forum/#!topic/linux.kernel/1WGDSlyY0y0
Indeed - the investigation you folks did is linked to within the
upstream Bugzilla bug (see
https://bugzilla.kernel.org/show_bug.cgi?id=118581#c2 ). Hopefully this
issue will be resolved but there's still some debate over on
http://thread.gmane.org/gmane.linux.kernel/2236800 . The problem is that
it is causing real problems in stable kernels (data not being correctly
zero'd) today...
--
Sitsofe | http://sucs.org/~sits/
^ permalink raw reply
* [PATCH 1/1] MDADM:Check mdinfo->reshape_active more times before calling Grow_continue
From: Xiao Ni @ 2016-06-16 1:41 UTC (permalink / raw)
To: Jes.Sorensen; +Cc: linux-raid
When reshaping a 3 drives raid5 to 4 drives raid5, there is a chance that
it can't start the reshape. If the disks are not enough to have spaces for
relocating the data_offset, it needs to call start_reshape and then run
mdadm --grow --continue by systemd. But mdadm --grow --continue fails
because it checkes that info->reshape_active is 0.
The info->reshape_active is got from the superblock of underlying devices.
Function start_reshape write reshape to /sys/../sync_action. Before writing
latest superblock to underlying devices, mdadm --grow --continue is called.
There is a chance info->reshape_active is 0. We should wait for superblock
updating more time before calling Grow_continue.
Signed-off-by: Xiao Ni <xni@redhat.com>
---
Grow.c | 67 ++++++++++++++++++++++++++++++++++++------------------------------
1 file changed, 37 insertions(+), 30 deletions(-)
diff --git a/Grow.c b/Grow.c
index f184d9c..628f0e7 100755
--- a/Grow.c
+++ b/Grow.c
@@ -4788,6 +4788,7 @@ int Grow_continue_command(char *devname, int fd,
dprintf("Grow continue is run for ");
if (st->ss->external == 0) {
int d;
+ int cnt = 5;
dprintf_cont("native array (%s)\n", devname);
if (ioctl(fd, GET_ARRAY_INFO, &array.array) < 0) {
pr_err("%s is not an active md array - aborting\n", devname);
@@ -4799,36 +4800,42 @@ int Grow_continue_command(char *devname, int fd,
* FIXME we should really get what we need from
* sysfs
*/
- for (d = 0; d < MAX_DISKS; d++) {
- mdu_disk_info_t disk;
- char *dv;
- int err;
- disk.number = d;
- if (ioctl(fd, GET_DISK_INFO, &disk) < 0)
- continue;
- if (disk.major == 0 && disk.minor == 0)
- continue;
- if ((disk.state & (1 << MD_DISK_ACTIVE)) == 0)
- continue;
- dv = map_dev(disk.major, disk.minor, 1);
- if (!dv)
- continue;
- fd2 = dev_open(dv, O_RDONLY);
- if (fd2 < 0)
- continue;
- err = st->ss->load_super(st, fd2, NULL);
- close(fd2);
- if (err)
- continue;
- break;
- }
- if (d == MAX_DISKS) {
- pr_err("Unable to load metadata for %s\n",
- devname);
- ret_val = 1;
- goto Grow_continue_command_exit;
- }
- st->ss->getinfo_super(st, content, NULL);
+ do {
+ for (d = 0; d < MAX_DISKS; d++) {
+ mdu_disk_info_t disk;
+ char *dv;
+ int err;
+ disk.number = d;
+ if (ioctl(fd, GET_DISK_INFO, &disk) < 0)
+ continue;
+ if (disk.major == 0 && disk.minor == 0)
+ continue;
+ if ((disk.state & (1 << MD_DISK_ACTIVE)) == 0)
+ continue;
+ dv = map_dev(disk.major, disk.minor, 1);
+ if (!dv)
+ continue;
+ fd2 = dev_open(dv, O_RDONLY);
+ if (fd2 < 0)
+ continue;
+ err = st->ss->load_super(st, fd2, NULL);
+ close(fd2);
+ if (err)
+ continue;
+ break;
+ }
+ if (d == MAX_DISKS) {
+ pr_err("Unable to load metadata for %s\n",
+ devname);
+ ret_val = 1;
+ goto Grow_continue_command_exit;
+ }
+ st->ss->getinfo_super(st, content, NULL);
+ if (!content->reshape_active)
+ sleep(3);
+ else
+ break;
+ } while (cnt-- > 0);
} else {
char *container;
--
2.4.3
^ permalink raw reply related
* Re: [PATCH] mdadm: protecting sys_name overflow
From: Nikhil Kshirsagar @ 2016-06-16 3:56 UTC (permalink / raw)
To: Jes Sorensen; +Cc: linux-raid
In-Reply-To: <wrfjeg7yuwho.fsf@redhat.com>
Hi Jes,
Apologies for the wrong indentation. I will re-post the patch.
Thanks,
nikhil.
On 06/16/2016 12:10 AM, Jes Sorensen wrote:
> Nikhil Kshirsagar <nkshirsa@redhat.com> writes:
>> Hello,
>>
>> Devices with names larger than 31 bytes will overflow the sys_name array.
>>
>> This patch enables mdadm to fail and log a message if a long device name
>> is going to cause a buffer overflow.
>>
>> Signed-off-by: Nikhil Kshirsagar <nkshirsa@redhat.com>
>>
>> From 705aec84c6abf5b09c4202aec7cade9824ca7f12 Mon Sep 17 00:00:00 2001
>> From: root <root@nkshirsa.pnq.csb>
>> Date: Wed, 15 Jun 2016 15:23:12 +0530
>> Subject: [PATCH] Protecting overflow of sys_name. If a long device name is
>> going to cause a buffer overflow, we fail with a log message.
>>
>> ---
>> sysfs.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/sysfs.c b/sysfs.c
>> index 8379ca8..68b8b95 100644
>> --- a/sysfs.c
>> +++ b/sysfs.c
>> @@ -283,6 +283,15 @@ struct mdinfo *sysfs_read(int fd, char *devnm, unsigned long options)
>> }
>>
>> }
>> + /* strlen computes length of string *not* including the terminating null character. */
>> +
>> + if(strlen(de->d_name) >= sizeof(dev->sys_name))
>> + {
>> + pr_err("Device name %s larger than currently supported by mdadm\n",de->d_name);
>> + free(dev);
>> + goto abort;
>> +
>> + }
> Nikhil,
>
> Please respect the coding style of mdadm. Code indents at 8 characters
> using a tab, not whitespace, and brackets should be placed correctly.
>
> Just like with kernel code.
>
> Jes
^ permalink raw reply
* [PATCH] mdadm: protecting sys_name overflow
From: Nikhil Kshirsagar @ 2016-06-16 3:59 UTC (permalink / raw)
To: linux-raid
[-- Attachment #1: Type: text/plain, Size: 322 bytes --]
Hello,
(Corrected indentation and code formatting, and re-posting this patch.)
Devices with names larger than 31 bytes will overflow the sys_name array.
This patch enables mdadm to fail and log a message if a long device name
is going to cause a buffer overflow.
Signed-off-by: Nikhil Kshirsagar <nkshirsa@redhat.com>
[-- Attachment #2: 0001-Protecting-overflow-of-sys_name.-If-a-long-device-na.patch --]
[-- Type: text/x-patch, Size: 950 bytes --]
From 8198c463c3199c8207dd16cefac23197b16d8a09 Mon Sep 17 00:00:00 2001
From: Nikhil Kshirsagar <nkshirsa@redhat.com>
Date: Thu, 16 Jun 2016 09:25:07 +0530
Subject: [PATCH] Protecting overflow of sys_name. If a long device name is
going to cause a buffer overflow, we fail with a log message.
---
sysfs.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/sysfs.c b/sysfs.c
index 8379ca8..d346fe9 100644
--- a/sysfs.c
+++ b/sysfs.c
@@ -283,6 +283,13 @@ struct mdinfo *sysfs_read(int fd, char *devnm, unsigned long options)
}
}
+
+ /* strlen computes length of string *not* including the terminating null character. */
+ if (strlen(de->d_name) >= sizeof(dev->sys_name)) {
+ pr_err("Device name %s larger than currently supported by mdadm\n",de->d_name);
+ free(dev);
+ goto abort;
+ }
strcpy(dev->sys_name, de->d_name);
dev->disk.raid_disk = strtoul(buf, &ep, 10);
if (*ep) dev->disk.raid_disk = -1;
--
1.8.3.1
^ permalink raw reply related
* Re: RAID1: deadlock between freeze_array and blk plug?
From: Alexander Lyakas @ 2016-06-16 7:48 UTC (permalink / raw)
To: Lawrence, Joe, linux-raid
In-Reply-To: <CY1PR0801MB2252D339C65DF97375E3949C98560@CY1PR0801MB2252.namprd08.prod.outlook.com>
Hello Joe,
I think the commit you mention is related to handling read errors, in
which case freeze_array is called, and it may hang due to incorrect
accounting of IO requests. Also, this commit is only relevant since
kernel 4.3. For example, in kernel 3.18 there is no "bio_end_io_list"
at all.
Looking more at this issue, I don't think this is related to the new
freeze_array code using array_frozen since
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/md/raid1.c?id=b364e3d048e49b1d177eb7ee7853e77aa0560464
Because the same plugging infrastructure already existed, for example,
in kernel 3.8, but we did not observe similar deadlocks. I will have
to dig more to understand how this deadlock is avoided.
I am more worried now about the freeze_array deadlock I reported in
http://www.spinics.net/lists/raid/msg52678.html
This is a real deadlock that we see now.
Thanks,
Alex.
On Thu, Jun 16, 2016 at 6:38 AM, Lawrence, Joe <Joe.Lawrence@stratus.com> wrote:
> Hi Alexander,
>
> Any chance this was handled by commit "raid1: include bio_end_io_list in
> nr_queued to prevent freeze_array hang" [1]
>
> [1]
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/md/raid1.c?id=ccfc7bf1f09d6190ef86693ddc761d5fe3fa47cb
> ________________________________
> From: linux-raid-owner@vger.kernel.org <linux-raid-owner@vger.kernel.org> on
> behalf of Alexander Lyakas <alex.bolshoy@gmail.com>
> Sent: Monday, June 13, 2016 7:02:38 AM
> To: Neil Brown; Jes Sorensen; linux-raid
> Subject: RAID1: deadlock between freeze_array and blk plug?
>
> Hello Neil, Jes,
>
> I wonder if the following deadlock is possible:
>
> - Caller calls blk_start_plug and wants to submit two WRITE bios
>
> - First bio successfully calls wait_barrier() and is appended to
> plug->pending list
>
> - Now somebody does freeze_array()
>
> - freeze_array() unconditionally sets:
> conf->array_frozen = 1;
> and starts waiting for conf->nr_pending to go down
>
> - Second WRITE bio calls wait_barrier, but it will wait for
> "!conf->array_frozen" until it can proceed
>
> - Now we have a deadlock: first bio will not be submitted because it
> sits on the plug list of the caller, and caller is stuck in
> wait_barrier, so it cannot do blk_finish_plug.
>
> I am about to try to reproduce it on kernel 3.18, but looking at the
> latest Linus tree, I don't see something preventing this from
> happening either. Am I missing something?
>
> Thanks,
> Alex.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH] dm raid: don't use 'const' in function return
From: Arnd Bergmann @ 2016-06-16 9:03 UTC (permalink / raw)
To: Mike Snitzer
Cc: Arnd Bergmann, Alasdair Kergon, dm-devel, Shaohua Li,
Heinz Mauelshagen, linux-raid, linux-kernel
A newly introduced function has 'const int' as the return type,
but as "make W=1" reports, that has no meaning:
drivers/md/dm-raid.c:510:18: error: type qualifiers ignored on function return type [-Werror=ignored-qualifiers]
This changes the return type to plain 'int'.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: 33e53f06850f ("dm raid: introduce extended superblock and new raid types to support takeover/reshaping")
---
drivers/md/dm-raid.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
index 96bcff7a0b94..55d25ac87bda 100644
--- a/drivers/md/dm-raid.c
+++ b/drivers/md/dm-raid.c
@@ -507,7 +507,7 @@ static const char *raid10_md_layout_to_format(int layout)
}
/* Return md raid10 algorithm for @name */
-static const int raid10_name_to_format(const char *name)
+static int raid10_name_to_format(const char *name)
{
if (!strcasecmp(name, "near"))
return ALGORITHM_RAID10_NEAR;
--
2.9.0
^ permalink raw reply related
* [PATCH] monitor: Make sure that last_checkpoint is set to 0 after sync
From: Pawel Baldysiak @ 2016-06-16 9:12 UTC (permalink / raw)
To: jes.sorensen
Cc: linux-raid, artur.paszkiewicz, aleksey.obitotskiy,
tomasz.majchrzak
In a case of successful completion of a resync (in the last step)
- read_and_act sometimes still reads sync_action as "resync"
but sync_completed already is set to component_size.
When this race occurs, sync operation is
marked as finished, but last_checkpoint is
overwritten with sync_completed. It will cause next
sync operation (ie. reshape) to be reported as complete immediately
after start - mdmon will write successful completion of the reshape
to metadata. This patch sets last_checkpoint to 0 once the sync
is completed to stop it happening.
Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com>
---
monitor.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/monitor.c b/monitor.c
index 870cc1a..4c79ce2 100644
--- a/monitor.c
+++ b/monitor.c
@@ -420,6 +420,9 @@ static int read_and_act(struct active_array *a)
if (sync_completed > a->last_checkpoint)
a->last_checkpoint = sync_completed;
+ if (sync_completed >= a->info.component_size)
+ a->last_checkpoint = 0;
+
a->container->ss->sync_metadata(a->container);
dprintf("(%d): state:%s action:%s next(", a->info.container_member,
array_states[a->curr_state], sync_actions[a->curr_action]);
^ permalink raw reply related
* [PATCH 0/2] imsm: add extra handling of sync_action and sync_completed
From: Alexey Obitotskiy @ 2016-06-16 9:31 UTC (permalink / raw)
To: jes.sorensen; +Cc: linux-raid
This patchset fixes issues with IMSM reshape completion
or interruption.
After sync completion sync_action set to 'idle' and must be treated
as normal wait_for_reshape_imsm termination. During sync or after
sync finished sync_completed can be set to 'disabled' or 'none' and
must be handled respectively.
Alexey Obitotskiy (2):
imsm: add handling of sync_action is equal to 'idle'
imsm: properly handle values of sync_completed
super-intel.c | 40 +++++++++++++++++++++++++++++++++++++---
1 file changed, 37 insertions(+), 3 deletions(-)
--
2.7.4
^ permalink raw reply
* [PATCH 1/2] imsm: add handling of sync_action is equal to 'idle'
From: Alexey Obitotskiy @ 2016-06-16 9:31 UTC (permalink / raw)
To: jes.sorensen; +Cc: linux-raid
In-Reply-To: <1466069497-29503-1-git-send-email-aleksey.obitotskiy@intel.com>
After resync is stopped sync_action value become 'idle'.
We treat this case as normal termination of waiting, not as error.
Signed-off-by: Alexey Obitotskiy <aleksey.obitotskiy@intel.com>
Reviewed-by: Pawel Baldysiak <pawel.baldysiak@intel.com>
---
super-intel.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/super-intel.c b/super-intel.c
index 7e2860c..7950bef 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -10423,6 +10423,8 @@ int wait_for_reshape_imsm(struct mdinfo *sra, int ndata)
if (sysfs_get_str(sra, NULL, "sync_action",
action, 20) > 0 &&
strncmp(action, "reshape", 7) != 0) {
+ if (strncmp(action, "idle", 4) == 0)
+ break;
close(fd);
return -1;
}
@@ -10432,9 +10434,9 @@ int wait_for_reshape_imsm(struct mdinfo *sra, int ndata)
return 1;
}
} while (completed < position_to_set);
+
close(fd);
return 0;
-
}
/*******************************************************************************
--
2.7.4
^ permalink raw reply related
* [PATCH 2/2] imsm: properly handle values of sync_completed
From: Alexey Obitotskiy @ 2016-06-16 9:31 UTC (permalink / raw)
To: jes.sorensen; +Cc: linux-raid
In-Reply-To: <1466069497-29503-1-git-send-email-aleksey.obitotskiy@intel.com>
The sync_completed can be set to such values:
- two numbers of processed sectors and total during synchronization,
separated with '/';
- 'none' if synchronization process is stopped;
- 'delayed' if synchronization process is delayed.
Handle value of sync_completed not only as numbers but
also check for 'none' and 'delayed'.
Signed-off-by: Alexey Obitotskiy <aleksey.obitotskiy@intel.com>
Reviewed-by: Pawel Baldysiak <pawel.baldysiak@intel.com>
---
super-intel.c | 36 ++++++++++++++++++++++++++++++++++--
1 file changed, 34 insertions(+), 2 deletions(-)
diff --git a/super-intel.c b/super-intel.c
index 7950bef..b401f3c 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -10363,6 +10363,33 @@ exit_imsm_reshape_super:
return ret_val;
}
+#define COMPLETED_OK 0
+#define COMPLETED_NONE 1
+#define COMPLETED_DELAYED 2
+
+static int read_completed(int fd, unsigned long long *val)
+{
+ int ret;
+ char buf[50];
+
+ ret = sysfs_fd_get_str(fd, buf, 50);
+ if (ret < 0)
+ return ret;
+
+ ret = COMPLETED_OK;
+ if (strncmp(buf, "none", 4) == 0) {
+ ret = COMPLETED_NONE;
+ } else if (strncmp(buf, "delayed", 7) == 0) {
+ ret = COMPLETED_DELAYED;
+ } else {
+ char *ep;
+ *val = strtoull(buf, &ep, 0);
+ if (ep == buf || (*ep != 0 && *ep != '\n' && *ep != ' '))
+ ret = -1;
+ }
+ return ret;
+}
+
/*******************************************************************************
* Function: wait_for_reshape_imsm
* Description: Function writes new sync_max value and waits until
@@ -10417,7 +10444,9 @@ int wait_for_reshape_imsm(struct mdinfo *sra, int ndata)
}
do {
+ int rc;
char action[20];
+
int timeout = 3000;
sysfs_wait(fd, &timeout);
if (sysfs_get_str(sra, NULL, "sync_action",
@@ -10428,11 +10457,14 @@ int wait_for_reshape_imsm(struct mdinfo *sra, int ndata)
close(fd);
return -1;
}
- if (sysfs_fd_get_ll(fd, &completed) < 0) {
+
+ rc = read_completed(fd, &completed);
+ if (rc < 0) {
dprintf("cannot read reshape_position (in loop)\n");
close(fd);
return 1;
- }
+ } else if (rc == COMPLETED_NONE)
+ break;
} while (completed < position_to_set);
close(fd);
--
2.7.4
^ permalink raw reply related
* Re: RAID1: deadlock between freeze_array and blk plug?
From: Alexander Lyakas @ 2016-06-16 15:01 UTC (permalink / raw)
To: Lawrence, Joe, linux-raid
In-Reply-To: <CAGRgLy7nsB7affa--6DK0hXzyoLmRxZuMkgLuxr4CktvAiBAzw@mail.gmail.com>
Hello,
By further analysis I found out that this deadlock is not possible.
Reason is that when wait_barrier goes into waiting by calling
schedule(), then it will call sched_submit_work(), and it will do:
/*
* If we are going to sleep and we have plugged IO queued,
* make sure to submit it to avoid deadlocks.
*/
if (blk_needs_flush_plug(tsk))
blk_schedule_flush_plug(tsk);
So it will flush all the plugged WRITEs, and they will go into
conf->pending_bio_list. And freeze_array will call
flush_pending_writes, so eventually these writes will complete, and
freeze_array will also complete.
So this problem does not exist, but the problems I mentioned in
http://www.spinics.net/lists/raid/msg52678.html
are real.
Thanks,
Alex.
On Thu, Jun 16, 2016 at 10:48 AM, Alexander Lyakas
<alex.bolshoy@gmail.com> wrote:
> Hello Joe,
>
> I think the commit you mention is related to handling read errors, in
> which case freeze_array is called, and it may hang due to incorrect
> accounting of IO requests. Also, this commit is only relevant since
> kernel 4.3. For example, in kernel 3.18 there is no "bio_end_io_list"
> at all.
>
> Looking more at this issue, I don't think this is related to the new
> freeze_array code using array_frozen since
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/md/raid1.c?id=b364e3d048e49b1d177eb7ee7853e77aa0560464
>
> Because the same plugging infrastructure already existed, for example,
> in kernel 3.8, but we did not observe similar deadlocks. I will have
> to dig more to understand how this deadlock is avoided.
>
> I am more worried now about the freeze_array deadlock I reported in
> http://www.spinics.net/lists/raid/msg52678.html
>
> This is a real deadlock that we see now.
>
> Thanks,
> Alex.
>
>
>
> On Thu, Jun 16, 2016 at 6:38 AM, Lawrence, Joe <Joe.Lawrence@stratus.com> wrote:
>> Hi Alexander,
>>
>> Any chance this was handled by commit "raid1: include bio_end_io_list in
>> nr_queued to prevent freeze_array hang" [1]
>>
>> [1]
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/md/raid1.c?id=ccfc7bf1f09d6190ef86693ddc761d5fe3fa47cb
>> ________________________________
>> From: linux-raid-owner@vger.kernel.org <linux-raid-owner@vger.kernel.org> on
>> behalf of Alexander Lyakas <alex.bolshoy@gmail.com>
>> Sent: Monday, June 13, 2016 7:02:38 AM
>> To: Neil Brown; Jes Sorensen; linux-raid
>> Subject: RAID1: deadlock between freeze_array and blk plug?
>>
>> Hello Neil, Jes,
>>
>> I wonder if the following deadlock is possible:
>>
>> - Caller calls blk_start_plug and wants to submit two WRITE bios
>>
>> - First bio successfully calls wait_barrier() and is appended to
>> plug->pending list
>>
>> - Now somebody does freeze_array()
>>
>> - freeze_array() unconditionally sets:
>> conf->array_frozen = 1;
>> and starts waiting for conf->nr_pending to go down
>>
>> - Second WRITE bio calls wait_barrier, but it will wait for
>> "!conf->array_frozen" until it can proceed
>>
>> - Now we have a deadlock: first bio will not be submitted because it
>> sits on the plug list of the caller, and caller is stuck in
>> wait_barrier, so it cannot do blk_finish_plug.
>>
>> I am about to try to reproduce it on kernel 3.18, but looking at the
>> latest Linus tree, I don't see something preventing this from
>> happening either. Am I missing something?
>>
>> Thanks,
>> Alex.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] dm raid: don't use 'const' in function return
From: Heinz Mauelshagen @ 2016-06-16 16:21 UTC (permalink / raw)
To: Arnd Bergmann, Mike Snitzer
Cc: Alasdair Kergon, dm-devel, Shaohua Li, linux-raid, linux-kernel
In-Reply-To: <20160616090359.984712-1-arnd@arndb.de>
Thanks Arnd.
On 06/16/2016 11:03 AM, Arnd Bergmann wrote:
> A newly introduced function has 'const int' as the return type,
> but as "make W=1" reports, that has no meaning:
>
> drivers/md/dm-raid.c:510:18: error: type qualifiers ignored on function return type [-Werror=ignored-qualifiers]
>
> This changes the return type to plain 'int'.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: 33e53f06850f ("dm raid: introduce extended superblock and new raid types to support takeover/reshaping")
> ---
> drivers/md/dm-raid.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
> index 96bcff7a0b94..55d25ac87bda 100644
> --- a/drivers/md/dm-raid.c
> +++ b/drivers/md/dm-raid.c
> @@ -507,7 +507,7 @@ static const char *raid10_md_layout_to_format(int layout)
> }
>
> /* Return md raid10 algorithm for @name */
> -static const int raid10_name_to_format(const char *name)
> +static int raid10_name_to_format(const char *name)
> {
> if (!strcasecmp(name, "near"))
> return ALGORITHM_RAID10_NEAR;
^ permalink raw reply
* Re: [PATCH 1/1] MDADM:Check mdinfo->reshape_active more times before calling Grow_continue
From: Jes Sorensen @ 2016-06-16 17:54 UTC (permalink / raw)
To: Xiao Ni; +Cc: linux-raid
In-Reply-To: <1466041262-12785-1-git-send-email-xni@redhat.com>
Xiao Ni <xni@redhat.com> writes:
> When reshaping a 3 drives raid5 to 4 drives raid5, there is a chance that
> it can't start the reshape. If the disks are not enough to have spaces for
> relocating the data_offset, it needs to call start_reshape and then run
> mdadm --grow --continue by systemd. But mdadm --grow --continue fails
> because it checkes that info->reshape_active is 0.
>
> The info->reshape_active is got from the superblock of underlying devices.
> Function start_reshape write reshape to /sys/../sync_action. Before writing
> latest superblock to underlying devices, mdadm --grow --continue is called.
> There is a chance info->reshape_active is 0. We should wait for superblock
> updating more time before calling Grow_continue.
>
> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
> Grow.c | 67 ++++++++++++++++++++++++++++++++++++------------------------------
> 1 file changed, 37 insertions(+), 30 deletions(-)
Applied!
Thanks,
Jes
^ permalink raw reply
* Re: [PATCH] monitor: Make sure that last_checkpoint is set to 0 after sync
From: Jes Sorensen @ 2016-06-16 17:55 UTC (permalink / raw)
To: Pawel Baldysiak
Cc: linux-raid, artur.paszkiewicz, aleksey.obitotskiy,
tomasz.majchrzak
In-Reply-To: <146606834045.17835.18148510717600494913.stgit@gklab-154-222.intel.com>
Pawel Baldysiak <pawel.baldysiak@intel.com> writes:
> In a case of successful completion of a resync (in the last step)
> - read_and_act sometimes still reads sync_action as "resync"
> but sync_completed already is set to component_size.
> When this race occurs, sync operation is
> marked as finished, but last_checkpoint is
> overwritten with sync_completed. It will cause next
> sync operation (ie. reshape) to be reported as complete immediately
> after start - mdmon will write successful completion of the reshape
> to metadata. This patch sets last_checkpoint to 0 once the sync
> is completed to stop it happening.
>
> Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com>
> ---
> monitor.c | 3 +++
> 1 file changed, 3 insertions(+)
Applied!
Thanks,
Jes
^ permalink raw reply
* Re: [PATCH 1/2] imsm: add handling of sync_action is equal to 'idle'
From: Jes Sorensen @ 2016-06-16 17:59 UTC (permalink / raw)
To: Alexey Obitotskiy; +Cc: linux-raid
In-Reply-To: <1466069497-29503-2-git-send-email-aleksey.obitotskiy@intel.com>
Alexey Obitotskiy <aleksey.obitotskiy@intel.com> writes:
> After resync is stopped sync_action value become 'idle'.
> We treat this case as normal termination of waiting, not as error.
>
> Signed-off-by: Alexey Obitotskiy <aleksey.obitotskiy@intel.com>
> Reviewed-by: Pawel Baldysiak <pawel.baldysiak@intel.com>
> ---
> super-intel.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
Applied!
Thanks,
Jes
^ permalink raw reply
* Re: [PATCH 2/2] imsm: properly handle values of sync_completed
From: Jes Sorensen @ 2016-06-16 17:59 UTC (permalink / raw)
To: Alexey Obitotskiy; +Cc: linux-raid
In-Reply-To: <1466069497-29503-3-git-send-email-aleksey.obitotskiy@intel.com>
Alexey Obitotskiy <aleksey.obitotskiy@intel.com> writes:
> The sync_completed can be set to such values:
> - two numbers of processed sectors and total during synchronization,
> separated with '/';
> - 'none' if synchronization process is stopped;
> - 'delayed' if synchronization process is delayed.
> Handle value of sync_completed not only as numbers but
> also check for 'none' and 'delayed'.
>
> Signed-off-by: Alexey Obitotskiy <aleksey.obitotskiy@intel.com>
> Reviewed-by: Pawel Baldysiak <pawel.baldysiak@intel.com>
> ---
> super-intel.c | 36 ++++++++++++++++++++++++++++++++++--
> 1 file changed, 34 insertions(+), 2 deletions(-)
>
> diff --git a/super-intel.c b/super-intel.c
> index 7950bef..b401f3c 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -10363,6 +10363,33 @@ exit_imsm_reshape_super:
> return ret_val;
> }
>
> +#define COMPLETED_OK 0
> +#define COMPLETED_NONE 1
> +#define COMPLETED_DELAYED 2
> +
> +static int read_completed(int fd, unsigned long long *val)
> +{
> + int ret;
> + char buf[50];
> +
> + ret = sysfs_fd_get_str(fd, buf, 50);
> + if (ret < 0)
> + return ret;
> +
> + ret = COMPLETED_OK;
> + if (strncmp(buf, "none", 4) == 0) {
> + ret = COMPLETED_NONE;
> + } else if (strncmp(buf, "delayed", 7) == 0) {
> + ret = COMPLETED_DELAYED;
> + } else {
> + char *ep;
> + *val = strtoull(buf, &ep, 0);
> + if (ep == buf || (*ep != 0 && *ep != '\n' && *ep != ' '))
> + ret = -1;
> + }
> + return ret;
> +}
> +
> /*******************************************************************************
> * Function: wait_for_reshape_imsm
> * Description: Function writes new sync_max value and waits until
> @@ -10417,7 +10444,9 @@ int wait_for_reshape_imsm(struct mdinfo *sra, int ndata)
> }
>
> do {
> + int rc;
> char action[20];
> +
> int timeout = 3000;
> sysfs_wait(fd, &timeout);
> if (sysfs_get_str(sra, NULL, "sync_action",
Nit - you shouldn't have a blank line between variable declaration, but
rather put it between the declarations and the code itself. I fixed it
up for you.
Applied!
Cheers,
Jes
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox