* [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
* [PATCH] Documentation: fix wrong value in md.txt
From: Tiezhu Yang @ 2016-06-17 1:40 UTC (permalink / raw)
To: corbet, shli; +Cc: linux-doc, linux-raid, linux-kernel
In the current Documentation/md.txt, the lower limit value of
stripe_cache_size is 16 and the default value is 128, but when
I update kernel to the latest mainline version and RAID5 array
is created by mdadm, then execute the following commands, it
shows an error and a difference respectively.
1) set stripe_cache_size to 16
[root@localhost ~]# echo 16 > /sys/block/md0/md/stripe_cache_size
bash: echo: write error: Invalid argument
2) read the default value of stripe_cache_size
[root@localhost ~]# cat /sys/block/md0/md/stripe_cache_size
256
I read drivers/md/raid5.c and find the following related code:
1) in function 'raid5_set_cache_size':
if (size <= 16 || size > 32768)
return -EINVAL;
2) #define NR_STRIPES 256
So the lower limit value of stripe_cache_size should be 17 and
the default value should be 256.
Signed-off-by: Tiezhu Yang <kernelpatch@126.com>
---
Documentation/md.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/md.txt b/Documentation/md.txt
index 1a2ada4..d6e2fcf 100644
--- a/Documentation/md.txt
+++ b/Documentation/md.txt
@@ -602,7 +602,7 @@ These currently include
stripe_cache_size (currently raid5 only)
number of entries in the stripe cache. This is writable, but
- there are upper and lower limits (32768, 16). Default is 128.
+ there are upper and lower limits (32768, 17). Default is 256.
strip_cache_active (currently raid5 only)
number of active entries in the stripe cache
preread_bypass_threshold (currently raid5 only)
--
1.8.3.1
^ permalink raw reply related
* re: dm: raid456 basic support
From: Dan Carpenter @ 2016-06-17 9:14 UTC (permalink / raw)
To: neilb; +Cc: linux-raid
[ No idea why it's only just now complaining about issues from 2011... ]
Hello NeilBrown,
The patch 9d09e663d550: "dm: raid456 basic support" from Jan 13,
2011, leads to the following static checker warning:
drivers/md/dm-raid.c:1217 parse_raid_params()
warn: no lower bound on 'value'
drivers/md/dm-raid.c
1211 return -EINVAL;
1212 }
1213 if (!value || (value > MAX_SCHEDULE_TIMEOUT)) {
value is an int. MAX_SCHEDULE_TIMEOUT is LONG_MAX. Should it be
INT_MAX? What about negatives?
1214 rs->ti->error = "daemon sleep period out of range";
1215 return -EINVAL;
1216 }
1217 rs->md.bitmap_info.daemon_sleep = value;
1218 } else if (!strcasecmp(key, dm_raid_arg_name_by_flag(CTR_FLAG_DATA_OFFSET))) {
1219 /* Userspace passes new data_offset after having extended the the data image LV */
1220 if (test_and_set_bit(__CTR_FLAG_DATA_OFFSET, &rs->ctr_flags)) {
1221 rs->ti->error = "Only one data_offset argument pair allowed";
1222 return -EINVAL;
1223 }
1224 /* Ensure sensible data offset */
1225 if (value < 0) {
1226 rs->ti->error = "Bogus data_offset value";
1227 return -EINVAL;
1228 }
1229 rs->data_offset = value;
1230 } else if (!strcasecmp(key, dm_raid_arg_name_by_flag(CTR_FLAG_DELTA_DISKS))) {
1231 /* Define the +/-# of disks to add to/remove from the given raid set */
1232 if (test_and_set_bit(__CTR_FLAG_DELTA_DISKS, &rs->ctr_flags)) {
1233 rs->ti->error = "Only one delta_disks argument pair allowed";
1234 return -EINVAL;
1235 }
1236 /* Ensure MAX_RAID_DEVICES and raid type minimal_devs! */
1237 if (!__within_range(abs(value), 1, MAX_RAID_DEVICES - rt->minimal_devs)) {
1238 rs->ti->error = "Too many delta_disk requested";
1239 return -EINVAL;
1240 }
1241
1242 rs->delta_disks = value;
1243 } else if (!strcasecmp(key, dm_raid_arg_name_by_flag(CTR_FLAG_STRIPE_CACHE))) {
1244 if (test_and_set_bit(__CTR_FLAG_STRIPE_CACHE, &rs->ctr_flags)) {
1245 rs->ti->error = "Only one stripe_cache argument pair allowed";
1246 return -EINVAL;
1247 }
1248
1249 if (!rt_is_raid456(rt)) {
1250 rs->ti->error = "Inappropriate argument: stripe_cache";
1251 return -EINVAL;
1252 }
1253
1254 rs->stripe_cache_entries = value;
1255 } else if (!strcasecmp(key, dm_raid_arg_name_by_flag(CTR_FLAG_MIN_RECOVERY_RATE))) {
1256 if (test_and_set_bit(__CTR_FLAG_MIN_RECOVERY_RATE, &rs->ctr_flags)) {
1257 rs->ti->error = "Only one min_recovery_rate argument pair allowed";
1258 return -EINVAL;
1259 }
1260 if (value > INT_MAX) {
^^^^^^^
Here we're using INT_MAX.
1261 rs->ti->error = "min_recovery_rate out of range";
1262 return -EINVAL;
1263 }
1264 rs->md.sync_speed_min = (int)value;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This looks like negatives are intentional... A few lines later as well.
drivers/md/dm-raid.c:1274 parse_raid_params() warn: no lower bound on 'value'
regards,
dan carpenter
^ 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