* Re: [dm-devel] [PATCH 0/3] md raid: enhancements to support the device mapper dm-raid target
From: NeilBrown @ 2015-02-24 22:12 UTC (permalink / raw)
To: Heinz Mauelshagen
Cc: device-mapper development, jbras >> Brassow Jonathan,
linux RAID
In-Reply-To: <54EB13AC.4010006@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 321 bytes --]
On Mon, 23 Feb 2015 12:49:00 +0100 Heinz Mauelshagen <heinzm@redhat.com>
wrote:
> Sure.
> I'd like to see the raid0 conditonal request queue patch though.
>
ok, I've applied that one. It should turn up in my -next by the end of the
week.
I'll look forward to your other results.
Thanks,
NeilBrown
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply
* Re: [PATCH 2/5] Check return of stat() to avoid covscan complaining
From: Jes Sorensen @ 2015-02-25 0:13 UTC (permalink / raw)
To: NeilBrown; +Cc: artur.paszkiewicz, linux-raid
In-Reply-To: <20150225090301.5158921d@notabene.brown>
NeilBrown <neilb@suse.de> writes:
> On Tue, 24 Feb 2015 16:56:29 -0500 Jes Sorensen <Jes.Sorensen@redhat.com>
> wrote:
>
>> NeilBrown <neilb@suse.de> writes:
>> > On Tue, 24 Feb 2015 16:00:37 -0500 Jes.Sorensen@redhat.com wrote:
>> >
>> >> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>> >>
>> >> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>> >> ---
>> >> Assemble.c | 6 +++++-
>> >> 1 file changed, 5 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/Assemble.c b/Assemble.c
>> >> index 131f871..b392214 100644
>> >> --- a/Assemble.c
>> >> +++ b/Assemble.c
>> >> @@ -688,7 +688,11 @@ static int load_devices(struct devs *devices, char *devmap,
>> >> close(dfd);
>> >> }
>> >>
>> >> - stat(devname, &stb);
>> >> + if (stat(devname, &stb)) {
>> >> + pr_err("Unsable to stat(%s) - skipping device.\n",
>> >> + devname);
>> >> + continue;
>> >> + }
>> >>
>> >> if (c->verbose > 0)
>> >> pr_err("%s is identified as a member of %s, slot %d%s.\n",
>> >
>> > I've applied the other 4. I think I'd rather this one was fixed by changing
>> > stat(devname,
>> > to
>> > fstat(dfd,
>> > and keep dfd open a bit longer.
>> >
>> > Does this look OK to you?
>>
>> I got the warning from covscan because we ignored the return value from
>> stat, so I think you still need to check the return value from fstat()
>> as well.
>
> I hope not.
> You can only get errors from fstat if you do something stupid like passing
> NULL as the stat pointer, or passing a non-open file descriptior.
> So if covscan complains, then covscan is broken.
>
> In contrast, stat can certainly given an error, such a ENOENT, which cannot
> possibly be avoided by not being stupid.
Hmmm OK you got a point, even if the file is removed, it shouldn't
disappear until all users close it.
Cheers,
Jes
^ permalink raw reply
* Re: [dm-devel] [PATCH 0/3] md raid: enhancements to support the device mapper dm-raid target
From: Heinz Mauelshagen @ 2015-02-25 10:10 UTC (permalink / raw)
To: NeilBrown
Cc: device-mapper development, jbras >> Brassow Jonathan,
linux RAID
In-Reply-To: <20150225091243.33fdd5f6@notabene.brown>
On 02/24/2015 11:12 PM, NeilBrown wrote:
> On Mon, 23 Feb 2015 12:49:00 +0100 Heinz Mauelshagen <heinzm@redhat.com>
> wrote:
>
>> Sure.
>> I'd like to see the raid0 conditonal request queue patch though.
>>
>
> ok, I've applied that one. It should turn up in my -next by the end of the
> week.
Thanks for the heads up.
>
> I'll look forward to your other results.
Ok, will inform when I have something working.
Cheers,
Heinz
>
> Thanks,
> NeilBrown
>
^ permalink raw reply
* Re: [PATCH 3/5] add_orom(): Compare content of struct imsm_orom rather than pointers to it
From: Artur Paszkiewicz @ 2015-02-25 10:51 UTC (permalink / raw)
To: Jes.Sorensen, neilb; +Cc: linux-raid
In-Reply-To: <1424811640-26569-4-git-send-email-Jes.Sorensen@redhat.com>
On 02/24/2015 10:00 PM, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> This avoids adding the same orom entry to the oroms list multiple
> times, as the comparison of pointers is never going to succeed, in
> particular when '*orom' points to a local stack variable in the
> calling function.
>
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
> platform-intel.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/platform-intel.c b/platform-intel.c
> index 37274da..a4ffa9f 100644
> --- a/platform-intel.c
> +++ b/platform-intel.c
> @@ -255,8 +255,8 @@ static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
> int i;
>
> for (i = 0; i < SYS_DEV_MAX; i++) {
> - if (&oroms[i].orom == orom)
> - return orom;
> + if (!memcmp(&oroms[i].orom, orom, sizeof(struct imsm_orom)))
> + return &oroms[i].orom;
> if (oroms[i].orom.signature[0] == 0) {
> oroms[i].orom = *orom;
> return &oroms[i].orom;
>
Hi Jes,
You are right that this can add the same entry multiple times, but this
is how it is supposed to work. The oroms list should contain all the
platform's oroms and they can be the same, this is why memcmp() should
not be used here. We don't want to compare the contents of the
structure, just its address. Sorry if it's not clear.
Artur
^ permalink raw reply
* Re: [PATCH 3/5] add_orom(): Compare content of struct imsm_orom rather than pointers to it
From: Jes Sorensen @ 2015-02-25 12:29 UTC (permalink / raw)
To: Artur Paszkiewicz; +Cc: neilb, linux-raid
In-Reply-To: <54EDA91E.5050005@intel.com>
Artur Paszkiewicz <artur.paszkiewicz@intel.com> writes:
> On 02/24/2015 10:00 PM, Jes.Sorensen@redhat.com wrote:
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>
>> This avoids adding the same orom entry to the oroms list multiple
>> times, as the comparison of pointers is never going to succeed, in
>> particular when '*orom' points to a local stack variable in the
>> calling function.
>>
>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>> ---
>> platform-intel.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/platform-intel.c b/platform-intel.c
>> index 37274da..a4ffa9f 100644
>> --- a/platform-intel.c
>> +++ b/platform-intel.c
>> @@ -255,8 +255,8 @@ static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
>> int i;
>>
>> for (i = 0; i < SYS_DEV_MAX; i++) {
>> - if (&oroms[i].orom == orom)
>> - return orom;
>> + if (!memcmp(&oroms[i].orom, orom, sizeof(struct imsm_orom)))
>> + return &oroms[i].orom;
>> if (oroms[i].orom.signature[0] == 0) {
>> oroms[i].orom = *orom;
>> return &oroms[i].orom;
>>
>
> Hi Jes,
>
> You are right that this can add the same entry multiple times, but this
> is how it is supposed to work. The oroms list should contain all the
> platform's oroms and they can be the same, this is why memcmp() should
> not be used here. We don't want to compare the contents of the
> structure, just its address. Sorry if it's not clear.
Artur,
Then the code is fundamentally broken, since you end up comparing a
stack variable against the oroms array when you call it from
find_imsm_efi(). Worse you can end up returning the local stack variable
declared in find_imsm_efi() to the calling function - there is no way
that can be correct.
Look at this:
static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
{
int i;
for (i = 0; i < SYS_DEV_MAX; i++) {
if (&oroms[i].orom == orom)
return orom;
if (oroms[i].orom.signature[0] == 0) {
oroms[i].orom = *orom;
return &oroms[i].orom;
}
}
return NULL;
}
const struct imsm_orom *find_imsm_efi(struct sys_dev *hba)
{
struct imsm_orom orom;
const struct imsm_orom *ret;
int err;
....
ret = add_orom(&orom);
add_orom_device_id(ret, hba->dev_id);
return ret;
}
Cheers,
Jes
^ permalink raw reply
* Re: udev rules and scripts (erc timeout fix)
From: Chris @ 2015-02-25 14:37 UTC (permalink / raw)
To: linux-raid
In-Reply-To: <20150222112309.7fa20ad0@smtp.arcor.de>
[-- Attachment #1: Type: text/plain, Size: 2379 bytes --]
Hello,
managed to test and bugfix the udev rules.
Simply dropping all files into /etc/udev/rules.d/ is enough, and
an "udevadm test /block/<drive_dev>/<partition_dev>" (dryrun) for a
raid member seems to run ok and tells me it would execute the
appropriate script.
However, for me, un- and re-plugging a device still does not seem to
execute the script. The _possibly-redundant-disk.sh currently logs
into /tmp/timeout-tst, but udev events don't appear there.
If you have the possiblity to test this in your /etc/udev/rules.d/
setup, it would be much appreciated.
Cheers,
Chris
# /etc/udev/rules.d/99-test_mdadm_smartctl-timeouts.rules
SUBSYSTEM!="block", GOTO="md_inc_end"
# handle potential components of arrays (the ones supported by md)
ENV{ID_FS_TYPE}=="linux_raid_member", GOTO="md_inc"
# "noiswmd" on kernel command line stops mdadm from handling
# "isw" (aka IMSM - Intel RAID).
# "nodmraid" on kernel command line stops mdadm from handling
# "isw" or "ddf".
IMPORT{cmdline}="noiswmd"
IMPORT{cmdline}="nodmraid"
ENV{nodmraid}=="?*", GOTO="md_inc_end"
ENV{ID_FS_TYPE}=="ddf_raid_member", GOTO="md_inc"
ENV{noiswmd}=="?*", GOTO="md_inc_end"
ENV{ID_FS_TYPE}=="isw_raid_member", GOTO="md_inc"
GOTO="md_inc_end"
LABEL="md_inc"
# initialize redundancy possibility status
# (only the kernel module could set actual run-time state, and may in the future
# set a dynamic FASTFAIL kernel device property instead of calling smartctl-timeout scripts)
IMPORT{program}="/sbin/mdadm --examine --export $tempnode"
ENV{MD_LEVEL}=="raid[1-9]*", ENV{REDUNDANT_DEV}="possibly"
ENV{MD_LEVEL}=="raid0", ENV{REDUNDANT_DEV}="false"
LABEL="md_inc_end"
# call initial HDD error correction timeouts adjustment
ENV{DEVTYPE}=="partition", ENV{REDUNDANT_DEV}=="possibly", TEST=="/usr/sbin/smartctl", RUN+="/etc/udev/rules.d/smartctl-timeouts_possibly-redundant-partition.sh $parent"
ENV{DEVTYPE}=="partition", ENV{REDUNDANT_DEV}=="false|", TEST=="/usr/sbin/smartctl", RUN+="/etc/udev/rules.d/smartctl-timeouts_non-redundant-partition.sh $parent"
ENV{DEVTYPE}=="disk", ENV{REDUNDANT_DEV}=="possibly", TEST=="/usr/sbin/smartctl", RUN+="/etc/udev/rules.d/smartctl-timeouts_posibly-redundant-disk.sh $devnode"
ENV{DEVTYPE}=="disk", ENV{REDUNDANT_DEV}=="false", TEST=="/usr/sbin/smartctl", RUN+="/etc/udev/rules.d/smartctl-timeouts_non-redundant-disk.sh $devnode"
[-- Attachment #2: smartctl-timeouts_udevadm-test.tar.gz --]
[-- Type: application/x-gzip, Size: 5448 bytes --]
^ permalink raw reply
* Re: [PATCH 4/5] IncrementalScan(): Make sure 'st' is valid before dereferencing it
From: John Stoffel @ 2015-02-25 15:00 UTC (permalink / raw)
To: Jes.Sorensen; +Cc: neilb, artur.paszkiewicz, linux-raid
In-Reply-To: <1424811640-26569-5-git-send-email-Jes.Sorensen@redhat.com>
>>>>> "Jes" == Jes Sorensen <Jes.Sorensen@redhat.com> writes:
Jes> From: Jes Sorensen <Jes.Sorensen@redhat.com>
Jes> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
Jes> ---
Jes> Incremental.c | 2 +-
Jes> 1 file changed, 1 insertion(+), 1 deletion(-)
Jes> diff --git a/Incremental.c b/Incremental.c
Jes> index 87d9114..33c0d7f 100644
Jes> --- a/Incremental.c
Jes> +++ b/Incremental.c
Jes> @@ -1354,7 +1354,7 @@ restart:
Jes> if (st && st->ss->load_container)
Jes> ret = st->ss->load_container(st, mdfd, NULL);
Jes> close(mdfd);
Jes> - if (!ret && st->ss->container_content) {
Jes> + if (!ret && st && st->ss->container_content) {
Jes> if (map_lock(&map))
Jes> pr_err("failed to get exclusive lock on mapfile\n");
Jes> ret = Incremental_container(st, me->path, c, only);
Jes> --
Jes> 2.1.0
Forgive my stupidity, but how does this really help anything? You
already did the check above for a valid 'st', and now you're just
repeating it. Maybe if needs to be more of:
if (st) {
if (st->ss->load_container)
ret = st->ss->load_container(st,mdfd, NULL);
close(mdfd);
if (!ret && st->ss->container_content) {
.....
}
}
but maybe I'm just missing something here.
John
^ permalink raw reply
* Re: [PATCH 4/5] IncrementalScan(): Make sure 'st' is valid before dereferencing it
From: Jes Sorensen @ 2015-02-25 15:37 UTC (permalink / raw)
To: John Stoffel; +Cc: neilb, artur.paszkiewicz, linux-raid
In-Reply-To: <21741.58242.604155.228473@quad.stoffel.home>
"John Stoffel" <john@stoffel.org> writes:
>>>>>> "Jes" == Jes Sorensen <Jes.Sorensen@redhat.com> writes:
>
> Jes> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> Jes> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> Jes> ---
> Jes> Incremental.c | 2 +-
> Jes> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> Jes> diff --git a/Incremental.c b/Incremental.c
> Jes> index 87d9114..33c0d7f 100644
> Jes> --- a/Incremental.c
> Jes> +++ b/Incremental.c
> Jes> @@ -1354,7 +1354,7 @@ restart:
> Jes> if (st && st->ss->load_container)
> Jes> ret = st->ss->load_container(st, mdfd, NULL);
> Jes> close(mdfd);
> Jes> - if (!ret && st->ss->container_content) {
> Jes> + if (!ret && st && st->ss->container_content) {
> Jes> if (map_lock(&map))
> Jes> pr_err("failed to get exclusive lock on mapfile\n");
> Jes> ret = Incremental_container(st, me->path, c, only);
> Jes> --
> Jes> 2.1.0
>
> Forgive my stupidity, but how does this really help anything? You
> already did the check above for a valid 'st', and now you're just
> repeating it. Maybe if needs to be more of:
>
> if (st) {
> if (st->ss->load_container)
> ret = st->ss->load_container(st,mdfd, NULL);
> close(mdfd);
> if (!ret && st->ss->container_content) {
> .....
> }
> }
>
> but maybe I'm just missing something here.
Please look more carefully - the checks above are in place so 'st' is
only dereferenced if 'st is valid. The code does not bail out if
st = NULL.
Your example results in mdfd not getting closed if st == NULL.
Jes
^ permalink raw reply
* Re: [PATCH 4/5] IncrementalScan(): Make sure 'st' is valid before dereferencing it
From: John Stoffel @ 2015-02-25 15:42 UTC (permalink / raw)
To: Jes Sorensen; +Cc: John Stoffel, neilb, artur.paszkiewicz, linux-raid
In-Reply-To: <wrfjioeq2e9p.fsf@redhat.com>
>>>>> "Jes" == Jes Sorensen <Jes.Sorensen@redhat.com> writes:
Jes> "John Stoffel" <john@stoffel.org> writes:
>>>>>>> "Jes" == Jes Sorensen <Jes.Sorensen@redhat.com> writes:
>>
Jes> From: Jes Sorensen <Jes.Sorensen@redhat.com>
Jes> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
Jes> ---
Jes> Incremental.c | 2 +-
Jes> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
Jes> diff --git a/Incremental.c b/Incremental.c
Jes> index 87d9114..33c0d7f 100644
Jes> --- a/Incremental.c
Jes> +++ b/Incremental.c
Jes> @@ -1354,7 +1354,7 @@ restart:
Jes> if (st && st->ss->load_container)
Jes> ret = st->ss->load_container(st, mdfd, NULL);
Jes> close(mdfd);
Jes> - if (!ret && st->ss->container_content) {
Jes> + if (!ret && st && st->ss->container_content) {
Jes> if (map_lock(&map))
Jes> pr_err("failed to get exclusive lock on mapfile\n");
Jes> ret = Incremental_container(st, me->path, c, only);
Jes> --
Jes> 2.1.0
>>
>> Forgive my stupidity, but how does this really help anything? You
>> already did the check above for a valid 'st', and now you're just
>> repeating it. Maybe if needs to be more of:
>>
>> if (st) {
>> if (st->ss->load_container)
>> ret = st->ss->load_container(st,mdfd, NULL);
>> close(mdfd);
>> if (!ret && st->ss->container_content) {
>> .....
>> }
>> }
>>
>> but maybe I'm just missing something here.
Jes> Please look more carefully - the checks above are in place so 'st' is
Jes> only dereferenced if 'st is valid. The code does not bail out if
Jes> st = NULL.
Jes> Your example results in mdfd not getting closed if st == NULL.
Right, I agree my example isn't perfect either, but I was just
commenting more that I think the check for st not being NULL should be
performed once, instead of multiple times. The closing the mdfd is
just a detail of the structure of the code. It's a nitpick I agree...
^ permalink raw reply
* Re: [PATCH 3/5] add_orom(): Compare content of struct imsm_orom rather than pointers to it
From: Artur Paszkiewicz @ 2015-02-25 16:32 UTC (permalink / raw)
To: Jes Sorensen; +Cc: neilb, linux-raid
In-Reply-To: <wrfjvbiq2mzo.fsf@redhat.com>
On 02/25/2015 01:29 PM, Jes Sorensen wrote:
> Artur Paszkiewicz <artur.paszkiewicz@intel.com> writes:
>> On 02/24/2015 10:00 PM, Jes.Sorensen@redhat.com wrote:
>>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>>
>>> This avoids adding the same orom entry to the oroms list multiple
>>> times, as the comparison of pointers is never going to succeed, in
>>> particular when '*orom' points to a local stack variable in the
>>> calling function.
>>>
>>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>>> ---
>>> platform-intel.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/platform-intel.c b/platform-intel.c
>>> index 37274da..a4ffa9f 100644
>>> --- a/platform-intel.c
>>> +++ b/platform-intel.c
>>> @@ -255,8 +255,8 @@ static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
>>> int i;
>>>
>>> for (i = 0; i < SYS_DEV_MAX; i++) {
>>> - if (&oroms[i].orom == orom)
>>> - return orom;
>>> + if (!memcmp(&oroms[i].orom, orom, sizeof(struct imsm_orom)))
>>> + return &oroms[i].orom;
>>> if (oroms[i].orom.signature[0] == 0) {
>>> oroms[i].orom = *orom;
>>> return &oroms[i].orom;
>>>
>>
>> Hi Jes,
>>
>> You are right that this can add the same entry multiple times, but this
>> is how it is supposed to work. The oroms list should contain all the
>> platform's oroms and they can be the same, this is why memcmp() should
>> not be used here. We don't want to compare the contents of the
>> structure, just its address. Sorry if it's not clear.
>
> Artur,
>
> Then the code is fundamentally broken, since you end up comparing a
> stack variable against the oroms array when you call it from
> find_imsm_efi(). Worse you can end up returning the local stack variable
> declared in find_imsm_efi() to the calling function - there is no way
> that can be correct.
>
> Look at this:
>
> static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
> {
> int i;
>
> for (i = 0; i < SYS_DEV_MAX; i++) {
> if (&oroms[i].orom == orom)
> return orom;
> if (oroms[i].orom.signature[0] == 0) {
> oroms[i].orom = *orom;
> return &oroms[i].orom;
> }
> }
> return NULL;
> }
>
> const struct imsm_orom *find_imsm_efi(struct sys_dev *hba)
> {
> struct imsm_orom orom;
> const struct imsm_orom *ret;
> int err;
>
> ....
>
> ret = add_orom(&orom);
> add_orom_device_id(ret, hba->dev_id);
>
> return ret;
> }
I can't see how this can lead to returning a stack variable. The oroms
array is global and add_orom() will always return a pointer to a struct
in this array. This comparison will always fail when we pass a pointer
to a stack variable to add_orom():
if (&oroms[i].orom == orom)
return orom;
This was meant to prevent adding an orom again like this:
ret = add_orom(&orom);
add_orom(ret);
Maybe it would be more appropriate to return NULL to indicate that
nothing was added instead of returning back the same pointer. I can do a
patch for this. What do you think?
Regards,
Artur
^ permalink raw reply
* Re: [PATCH 3/5] add_orom(): Compare content of struct imsm_orom rather than pointers to it
From: Jes Sorensen @ 2015-02-25 17:15 UTC (permalink / raw)
To: Artur Paszkiewicz; +Cc: neilb, linux-raid
In-Reply-To: <54EDF91F.40200@intel.com>
Artur Paszkiewicz <artur.paszkiewicz@intel.com> writes:
> On 02/25/2015 01:29 PM, Jes Sorensen wrote:
>> Artur Paszkiewicz <artur.paszkiewicz@intel.com> writes:
>>> On 02/24/2015 10:00 PM, Jes.Sorensen@redhat.com wrote:
>>>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>>>
>>>> This avoids adding the same orom entry to the oroms list multiple
>>>> times, as the comparison of pointers is never going to succeed, in
>>>> particular when '*orom' points to a local stack variable in the
>>>> calling function.
>>>>
>>>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>>>> ---
>>>> platform-intel.c | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/platform-intel.c b/platform-intel.c
>>>> index 37274da..a4ffa9f 100644
>>>> --- a/platform-intel.c
>>>> +++ b/platform-intel.c
>>>> @@ -255,8 +255,8 @@ static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
>>>> int i;
>>>>
>>>> for (i = 0; i < SYS_DEV_MAX; i++) {
>>>> - if (&oroms[i].orom == orom)
>>>> - return orom;
>>>> + if (!memcmp(&oroms[i].orom, orom, sizeof(struct imsm_orom)))
>>>> + return &oroms[i].orom;
>>>> if (oroms[i].orom.signature[0] == 0) {
>>>> oroms[i].orom = *orom;
>>>> return &oroms[i].orom;
>>>>
>>>
>>> Hi Jes,
>>>
>>> You are right that this can add the same entry multiple times, but this
>>> is how it is supposed to work. The oroms list should contain all the
>>> platform's oroms and they can be the same, this is why memcmp() should
>>> not be used here. We don't want to compare the contents of the
>>> structure, just its address. Sorry if it's not clear.
>>
>> Artur,
>>
>> Then the code is fundamentally broken, since you end up comparing a
>> stack variable against the oroms array when you call it from
>> find_imsm_efi(). Worse you can end up returning the local stack variable
>> declared in find_imsm_efi() to the calling function - there is no way
>> that can be correct.
>>
>> Look at this:
>>
>> static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
>> {
>> int i;
>>
>> for (i = 0; i < SYS_DEV_MAX; i++) {
>> if (&oroms[i].orom == orom)
>> return orom;
>> if (oroms[i].orom.signature[0] == 0) {
>> oroms[i].orom = *orom;
>> return &oroms[i].orom;
>> }
>> }
>> return NULL;
>> }
>>
>> const struct imsm_orom *find_imsm_efi(struct sys_dev *hba)
>> {
>> struct imsm_orom orom;
>> const struct imsm_orom *ret;
>> int err;
>>
>> ....
>>
>> ret = add_orom(&orom);
>> add_orom_device_id(ret, hba->dev_id);
>>
>> return ret;
>> }
>
> I can't see how this can lead to returning a stack variable. The oroms
> array is global and add_orom() will always return a pointer to a struct
> in this array. This comparison will always fail when we pass a pointer
> to a stack variable to add_orom():
>
> if (&oroms[i].orom == orom)
> return orom;
>
> This was meant to prevent adding an orom again like this:
>
> ret = add_orom(&orom);
> add_orom(ret);
>
> Maybe it would be more appropriate to return NULL to indicate that
> nothing was added instead of returning back the same pointer. I can do a
> patch for this. What do you think?
It will fail because we know we're comparing a stack pointer, but it
raises red flags with tools like coverity and it is really bad coding
practice to rely on hacks like this.
I also don't understand why you want to keep a table of identical
entries in the orom structure if multiple identical entries are found.
Each entry ought to match onto a specific physical controller, unless I
get something wrong?
Cheers,
Jes
^ permalink raw reply
* Re: RAID6 write I/O amplification?
From: Alireza Haghdoost @ 2015-02-26 0:40 UTC (permalink / raw)
To: Markus Stockhausen; +Cc: Roman Mamedov, linux-raid@vger.kernel.org
In-Reply-To: <12EF8D94C6F8734FB2FF37B9FBEDD1735F9E168E@EXCHANGE.collogia.de>
On Tue, Feb 24, 2015 at 12:29 AM, Markus Stockhausen
<stockhausen@collogia.de> wrote:
>> Von: linux-raid-owner@vger.kernel.org [linux-raid-owner@vger.kernel.org]" im Auftrag von "Roman Mamedov [rm@romanrm.net]
>> Gesendet: Dienstag, 24. Februar 2015 00:58
>> An: linux-raid@vger.kernel.org
>> Betreff: RAID6 write I/O amplification?
>>
>> Hello,
>>
>> Got a bit of a "how does it actually work" question...
>>
>> Suppose I have an MD RAID6 of 8 drives, with 64KB chunk size.
>>
>> I am rewriting a 4KB filesystem sector somewhere on that RAID (not crossing
>> the stripe boundary).
>>
>> What's the amount of disk I/O in total this will result in?
>>
>> I assume the RAID will need to read data from all drives, recompute parity,
>> then write to the data stripe where the updated piece happened to be, and also
>> write to two parity stripes.
>>
>> Is this done at a stripe granularity, so 6x64KB reads, 3x64KB writes?
>> Or down to individual sectors (pages), i.e. 6x4KB reads, 3x4KB writes?
>> Or am I describing this algorithm correctly at all?
>
> Implementation will work on "internal" stripe granularity and that is 4K
> So your case will be 6x4KB read + 3x4KB write.
Having said that, does it mean that following description of "chunk
size" is wrong:
'[chunk size] is the smallest "atomic" mass of data that can be
written to the devices'
since in this case chunk size is 64KB but 4KB is written atomically (?).
I have find it in the kernel.org wiki page [1]
---
Alireza
1. https://raid.wiki.kernel.org/index.php/RAID_setup
^ permalink raw reply
* Re: RAID6 write I/O amplification?
From: NeilBrown @ 2015-02-26 0:55 UTC (permalink / raw)
To: Alireza Haghdoost
Cc: Markus Stockhausen, Roman Mamedov, linux-raid@vger.kernel.org
In-Reply-To: <CAB-428mPydqGoku-RnhZUDVyHVnzb73Yz=5bL7DOd+G88siDtg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2226 bytes --]
On Wed, 25 Feb 2015 18:40:46 -0600 Alireza Haghdoost <alireza@cs.umn.edu>
wrote:
> On Tue, Feb 24, 2015 at 12:29 AM, Markus Stockhausen
> <stockhausen@collogia.de> wrote:
> >> Von: linux-raid-owner@vger.kernel.org [linux-raid-owner@vger.kernel.org]" im Auftrag von "Roman Mamedov [rm@romanrm.net]
> >> Gesendet: Dienstag, 24. Februar 2015 00:58
> >> An: linux-raid@vger.kernel.org
> >> Betreff: RAID6 write I/O amplification?
> >>
> >> Hello,
> >>
> >> Got a bit of a "how does it actually work" question...
> >>
> >> Suppose I have an MD RAID6 of 8 drives, with 64KB chunk size.
> >>
> >> I am rewriting a 4KB filesystem sector somewhere on that RAID (not crossing
> >> the stripe boundary).
> >>
> >> What's the amount of disk I/O in total this will result in?
> >>
> >> I assume the RAID will need to read data from all drives, recompute parity,
> >> then write to the data stripe where the updated piece happened to be, and also
> >> write to two parity stripes.
> >>
> >> Is this done at a stripe granularity, so 6x64KB reads, 3x64KB writes?
> >> Or down to individual sectors (pages), i.e. 6x4KB reads, 3x4KB writes?
> >> Or am I describing this algorithm correctly at all?
> >
> > Implementation will work on "internal" stripe granularity and that is 4K
> > So your case will be 6x4KB read + 3x4KB write.
>
> Having said that, does it mean that following description of "chunk
> size" is wrong:
> '[chunk size] is the smallest "atomic" mass of data that can be
> written to the devices'
> since in this case chunk size is 64KB but 4KB is written atomically (?).
> I have find it in the kernel.org wiki page [1]
I think that when it says "atomic" it means in space, not time.
i.e. one (properly aligned) chunk of data will not be split up and
written to different devices, it will all be written to one device.
If you write more than a chunk, it will be split up and parts of if written
to different devices.
You can still write less than a chunk.
So the intent is correct I think, but the word "atomic" doesn't really convey
the right meaning. Probably it should be re-written to avoid that term and
just spell out what is happening.
NeilBrown
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply
* smartctl-timeouts_v1.0 (erc timeout fix)
From: Chris @ 2015-02-27 12:31 UTC (permalink / raw)
To: linux-raid
[-- Attachment #1: Type: text/plain, Size: 381 bytes --]
Hi,
here they are now. The smartctl-timeouts scripts fix commonly
mismatching defaults with drives that have no error recovery timeout
configured, which has often lead to data loss.
To test, extract the files to /etc/udev/rules.d/ and
*reboot*. For me, the rules somehow had no effect without
rebooting.
Please test, report, and then ship it with smartctl/mdadm.
Cheers,
Chris
[-- Attachment #2: smartctl-timeouts_v1.0.zip --]
[-- Type: application/zip, Size: 10355 bytes --]
^ permalink raw reply
* Re: [PATCH 3/5] add_orom(): Compare content of struct imsm_orom rather than pointers to it
From: Artur Paszkiewicz @ 2015-02-27 13:39 UTC (permalink / raw)
To: Jes Sorensen; +Cc: neilb, linux-raid
In-Reply-To: <wrfj1tld3oa9.fsf@redhat.com>
On 02/25/2015 06:15 PM, Jes Sorensen wrote:
> Artur Paszkiewicz <artur.paszkiewicz@intel.com> writes:
>> On 02/25/2015 01:29 PM, Jes Sorensen wrote:
>>> Artur Paszkiewicz <artur.paszkiewicz@intel.com> writes:
>>>> On 02/24/2015 10:00 PM, Jes.Sorensen@redhat.com wrote:
>>>>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>>>>
>>>>> This avoids adding the same orom entry to the oroms list multiple
>>>>> times, as the comparison of pointers is never going to succeed, in
>>>>> particular when '*orom' points to a local stack variable in the
>>>>> calling function.
>>>>>
>>>>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>>>>> ---
>>>>> platform-intel.c | 4 ++--
>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/platform-intel.c b/platform-intel.c
>>>>> index 37274da..a4ffa9f 100644
>>>>> --- a/platform-intel.c
>>>>> +++ b/platform-intel.c
>>>>> @@ -255,8 +255,8 @@ static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
>>>>> int i;
>>>>>
>>>>> for (i = 0; i < SYS_DEV_MAX; i++) {
>>>>> - if (&oroms[i].orom == orom)
>>>>> - return orom;
>>>>> + if (!memcmp(&oroms[i].orom, orom, sizeof(struct imsm_orom)))
>>>>> + return &oroms[i].orom;
>>>>> if (oroms[i].orom.signature[0] == 0) {
>>>>> oroms[i].orom = *orom;
>>>>> return &oroms[i].orom;
>>>>>
>>>>
>>>> Hi Jes,
>>>>
>>>> You are right that this can add the same entry multiple times, but this
>>>> is how it is supposed to work. The oroms list should contain all the
>>>> platform's oroms and they can be the same, this is why memcmp() should
>>>> not be used here. We don't want to compare the contents of the
>>>> structure, just its address. Sorry if it's not clear.
>>>
>>> Artur,
>>>
>>> Then the code is fundamentally broken, since you end up comparing a
>>> stack variable against the oroms array when you call it from
>>> find_imsm_efi(). Worse you can end up returning the local stack variable
>>> declared in find_imsm_efi() to the calling function - there is no way
>>> that can be correct.
>>>
>>> Look at this:
>>>
>>> static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
>>> {
>>> int i;
>>>
>>> for (i = 0; i < SYS_DEV_MAX; i++) {
>>> if (&oroms[i].orom == orom)
>>> return orom;
>>> if (oroms[i].orom.signature[0] == 0) {
>>> oroms[i].orom = *orom;
>>> return &oroms[i].orom;
>>> }
>>> }
>>> return NULL;
>>> }
>>>
>>> const struct imsm_orom *find_imsm_efi(struct sys_dev *hba)
>>> {
>>> struct imsm_orom orom;
>>> const struct imsm_orom *ret;
>>> int err;
>>>
>>> ....
>>>
>>> ret = add_orom(&orom);
>>> add_orom_device_id(ret, hba->dev_id);
>>>
>>> return ret;
>>> }
>>
>> I can't see how this can lead to returning a stack variable. The oroms
>> array is global and add_orom() will always return a pointer to a struct
>> in this array. This comparison will always fail when we pass a pointer
>> to a stack variable to add_orom():
>>
>> if (&oroms[i].orom == orom)
>> return orom;
>>
>> This was meant to prevent adding an orom again like this:
>>
>> ret = add_orom(&orom);
>> add_orom(ret);
>>
>> Maybe it would be more appropriate to return NULL to indicate that
>> nothing was added instead of returning back the same pointer. I can do a
>> patch for this. What do you think?
>
> It will fail because we know we're comparing a stack pointer, but it
> raises red flags with tools like coverity and it is really bad coding
> practice to rely on hacks like this.
>
> I also don't understand why you want to keep a table of identical
> entries in the orom structure if multiple identical entries are found.
> Each entry ought to match onto a specific physical controller, unless I
> get something wrong?
>
OK, you're right, it is a hack. I thought it over and redesigned those
orom functions. This should make it simpler and more consistent.
Thanks,
Artur
From 673ecf1c0539f0050cc5934203af6d79cd68234d Mon Sep 17 00:00:00 2001
From: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
Date: Fri, 27 Feb 2015 10:34:20 +0100
Subject: [PATCH] imsm: simplified multiple OROMs support
Replaced oroms array with list, add_orom() now only appends to this list
and add_orom_device_id() only appends devid_list node to an orom_entry.
Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
---
platform-intel.c | 96 +++++++++++++++++++++++++++-----------------------------
platform-intel.h | 4 ++-
super-intel.c | 18 +++++------
3 files changed, 57 insertions(+), 61 deletions(-)
diff --git a/platform-intel.c b/platform-intel.c
index 37274da..9c89c20 100644
--- a/platform-intel.c
+++ b/platform-intel.c
@@ -229,65 +229,61 @@ struct pciExpDataStructFormat {
__u16 devListOffset;
} __attribute__ ((packed));
-static struct orom_entry oroms[SYS_DEV_MAX];
-
-const struct orom_entry *get_oroms(void)
-{
- return (const struct orom_entry *)&oroms;
-}
+struct orom_entry *orom_entries;
const struct imsm_orom *get_orom_by_device_id(__u16 dev_id)
{
- int i;
- struct devid_list *list;
+ struct orom_entry *entry;
+ struct devid_list *devid;
- for (i = 0; i < SYS_DEV_MAX; i++) {
- for (list = oroms[i].devid_list; list; list = list->next) {
- if (list->devid == dev_id)
- return &oroms[i].orom;
+ for (entry = orom_entries; entry; entry = entry->next) {
+ for (devid = entry->devid_list; devid; devid = devid->next) {
+ if (devid->devid == dev_id)
+ return &entry->orom;
}
}
+
return NULL;
}
-static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
+static struct orom_entry *add_orom(const struct imsm_orom *orom)
{
- int i;
-
- for (i = 0; i < SYS_DEV_MAX; i++) {
- if (&oroms[i].orom == orom)
- return orom;
- if (oroms[i].orom.signature[0] == 0) {
- oroms[i].orom = *orom;
- return &oroms[i].orom;
- }
- }
- return NULL;
+ struct orom_entry *list;
+ struct orom_entry *prev = NULL;
+
+ for (list = orom_entries; list; prev = list, list = list->next)
+ ;
+
+ list = xmalloc(sizeof(struct orom_entry));
+ list->orom = *orom;
+ list->devid_list = NULL;
+ list->next = NULL;
+
+ if (prev == NULL)
+ orom_entries = list;
+ else
+ prev->next = list;
+
+ return list;
}
-static void add_orom_device_id(const struct imsm_orom *orom, __u16 dev_id)
+static void add_orom_device_id(struct orom_entry *entry, __u16 dev_id)
{
- int i;
struct devid_list *list;
struct devid_list *prev = NULL;
- for (i = 0; i < SYS_DEV_MAX; i++) {
- if (&oroms[i].orom == orom) {
- for (list = oroms[i].devid_list; list; prev = list, list = list->next) {
- if (list->devid == dev_id)
- return;
- }
- list = xmalloc(sizeof(struct devid_list));
- list->devid = dev_id;
- list->next = NULL;
-
- if (prev == NULL)
- oroms[i].devid_list = list;
- else
- prev->next = list;
+ for (list = entry->devid_list; list; prev = list, list = list->next) {
+ if (list->devid == dev_id)
return;
- }
}
+ list = xmalloc(sizeof(struct devid_list));
+ list->devid = dev_id;
+ list->next = NULL;
+
+ if (prev == NULL)
+ entry->devid_list = list;
+ else
+ prev->next = list;
}
static int scan(const void *start, const void *end, const void *data)
@@ -321,7 +317,7 @@ static int scan(const void *start, const void *end, const void *data)
if (!imsm_mem)
return 0;
- const struct imsm_orom *orom = add_orom(imsm_mem);
+ struct orom_entry *orom = add_orom(imsm_mem);
if (ptr->devListOffset) {
const __u16 *dev_list = (void *)ptr + ptr->devListOffset;
@@ -367,11 +363,11 @@ const struct imsm_orom *imsm_platform_test(struct sys_dev *hba)
IMSM_OROM_RLC_RAID10;
}
- const struct imsm_orom *ret = add_orom(&orom);
+ struct orom_entry *ret = add_orom(&orom);
add_orom_device_id(ret, hba->dev_id);
- return ret;
+ return &ret->orom;
}
static const struct imsm_orom *find_imsm_hba_orom(struct sys_dev *hba)
@@ -508,7 +504,7 @@ static int read_efi_variable(void *buffer, ssize_t buf_size, char *variable_name
const struct imsm_orom *find_imsm_efi(struct sys_dev *hba)
{
struct imsm_orom orom;
- const struct imsm_orom *ret;
+ struct orom_entry *ret;
int err;
if (check_env("IMSM_TEST_AHCI_EFI") || check_env("IMSM_TEST_SCU_EFI"))
@@ -529,14 +525,14 @@ const struct imsm_orom *find_imsm_efi(struct sys_dev *hba)
/* try to read variable for combined AHCI controllers */
if (err && hba->type == SYS_DEV_SATA) {
- static const struct imsm_orom *csata;
+ static struct orom_entry *csata;
err = read_efi_variable(&orom, sizeof(orom), AHCI_CSATA_PROP, VENDOR_GUID);
if (!err) {
if (!csata)
csata = add_orom(&orom);
add_orom_device_id(csata, hba->dev_id);
- return csata;
+ return &csata->orom;
}
}
@@ -546,12 +542,12 @@ const struct imsm_orom *find_imsm_efi(struct sys_dev *hba)
ret = add_orom(&orom);
add_orom_device_id(ret, hba->dev_id);
- return ret;
+ return &ret->orom;
}
const struct imsm_orom *find_imsm_nvme(struct sys_dev *hba)
{
- static const struct imsm_orom *nvme_orom;
+ static struct orom_entry *nvme_orom;
if (hba->type != SYS_DEV_NVME)
return NULL;
@@ -574,7 +570,7 @@ const struct imsm_orom *find_imsm_nvme(struct sys_dev *hba)
nvme_orom = add_orom(&nvme_orom_compat);
}
add_orom_device_id(nvme_orom, hba->dev_id);
- return nvme_orom;
+ return &nvme_orom->orom;
}
const struct imsm_orom *find_imsm_capability(struct sys_dev *hba)
diff --git a/platform-intel.h b/platform-intel.h
index 2ead431..631fa76 100644
--- a/platform-intel.h
+++ b/platform-intel.h
@@ -213,8 +213,11 @@ struct devid_list {
struct orom_entry {
struct imsm_orom orom;
struct devid_list *devid_list;
+ struct orom_entry *next;
};
+extern struct orom_entry *orom_entries;
+
static inline char *guid_str(char *buf, struct efi_guid guid)
{
sprintf(buf, "%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x",
@@ -235,6 +238,5 @@ int devt_attached_to_hba(dev_t dev, const char *hba_path);
char *devt_to_devpath(dev_t dev);
int path_attached_to_hba(const char *disk_path, const char *hba_path);
const char *get_sys_dev_type(enum sys_dev_type);
-const struct orom_entry * get_oroms(void);
const struct imsm_orom *get_orom_by_device_id(__u16 device_id);
struct sys_dev *device_by_id(__u16 device_id);
diff --git a/super-intel.c b/super-intel.c
index 819e0da..53269fd 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -1948,13 +1948,12 @@ static int detail_platform_imsm(int verbose, int enumerate_only, char *controlle
return result;
}
- const struct orom_entry *oroms = get_oroms();
- int i;
+ const struct orom_entry *entry;
- for (i = 0; i < SYS_DEV_MAX && oroms[i].devid_list; i++) {
- print_imsm_capability(&oroms[i].orom);
+ for (entry = orom_entries; entry; entry = entry->next) {
+ print_imsm_capability(&entry->orom);
- if (imsm_orom_is_nvme(&oroms[i].orom)) {
+ if (imsm_orom_is_nvme(&entry->orom)) {
for (hba = list; hba; hba = hba->next) {
if (hba->type == SYS_DEV_NVME)
printf(" NVMe Device : %s\n", hba->path);
@@ -1963,7 +1962,7 @@ static int detail_platform_imsm(int verbose, int enumerate_only, char *controlle
}
struct devid_list *devid;
- for (devid = oroms[i].devid_list; devid; devid = devid->next) {
+ for (devid = entry->devid_list; devid; devid = devid->next) {
hba = device_by_id(devid->devid);
if (!hba)
continue;
@@ -2007,11 +2006,10 @@ static int export_detail_platform_imsm(int verbose, char *controller_path)
result = 0;
}
- const struct orom_entry *oroms = get_oroms();
- int i;
+ const struct orom_entry *entry;
- for (i = 0; i < SYS_DEV_MAX && oroms[i].devid_list; i++)
- print_imsm_capability_export(&oroms[i].orom);
+ for (entry = orom_entries; entry; entry = entry->next)
+ print_imsm_capability_export(&entry->orom);
return result;
}
--
2.1.4
^ permalink raw reply related
* [PATCH] IMSM-orom: make sure, that device list is supported
From: Pawel Baldysiak @ 2015-02-27 14:45 UTC (permalink / raw)
To: neilb; +Cc: linux-raid, pawel.baldysiak, artur.paszkiewicz
Devices list in PCI Data Structure is supported only in
3 and above revision. Make sure that this is checked.
Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com>
---
platform-intel.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/platform-intel.c b/platform-intel.c
index 37274da..c6a28e8 100644
--- a/platform-intel.c
+++ b/platform-intel.c
@@ -227,6 +227,8 @@ struct pciExpDataStructFormat {
__u16 vendorID;
__u16 deviceID;
__u16 devListOffset;
+ __u16 pciDataStructLen;
+ __u8 pciDataStructRev;
} __attribute__ ((packed));
static struct orom_entry oroms[SYS_DEV_MAX];
@@ -323,7 +325,8 @@ static int scan(const void *start, const void *end, const void *data)
const struct imsm_orom *orom = add_orom(imsm_mem);
- if (ptr->devListOffset) {
+ /* only PciDataStructure with revision 3 and above supports devices list. */
+ if (ptr->pciDataStructRev >= 3 && ptr->devListOffset) {
const __u16 *dev_list = (void *)ptr + ptr->devListOffset;
int i;
^ permalink raw reply related
* [PATCH] IncRemove: Set "auto-read" only after successful excl open.
From: Pawel Baldysiak @ 2015-02-27 14:47 UTC (permalink / raw)
To: neilb; +Cc: linux-raid, pawel.baldysiak, artur.paszkiewicz
"mdadm -If" - triggered from udev rules when disk is removed from OS -
tries to set array in auto-read-only mode. This can interrupt rebuild
process which is started automatically, e.g. if array is mounted and
spare disk is available (I/O error is detected faster than removing
failed disk by mdadm).
This patch prevents "mdadm -If" from setting array into "auto-read-only",
by requiring exclusive open to succeed.
Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com>
---
Incremental.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/Incremental.c b/Incremental.c
index 87d9114..b12f144 100644
--- a/Incremental.c
+++ b/Incremental.c
@@ -1712,12 +1712,16 @@ int IncrementalRemove(char *devname, char *id_path, int verbose)
return 1;
}
sysfs_init(&mdi, -1, ent->devnm);
- if (sysfs_get_str(&mdi, NULL, "array_state",
- buf, sizeof(buf)) > 0) {
- if (strncmp(buf, "active", 6) == 0 ||
- strncmp(buf, "clean", 5) == 0)
- sysfs_set_str(&mdi, NULL,
- "array_state", "read-auto");
+ mdfd = open_dev_excl(ent->devnm);
+ if (mdfd > 0) {
+ close(mdfd);
+ if (sysfs_get_str(&mdi, NULL, "array_state",
+ buf, sizeof(buf)) > 0) {
+ if (strncmp(buf, "active", 6) == 0 ||
+ strncmp(buf, "clean", 5) == 0)
+ sysfs_set_str(&mdi, NULL,
+ "array_state", "read-auto");
+ }
}
mdfd = open_dev(ent->devnm);
if (mdfd < 0) {
^ permalink raw reply related
* Re: [PATCH 3/5] add_orom(): Compare content of struct imsm_orom rather than pointers to it
From: Jes Sorensen @ 2015-02-27 20:51 UTC (permalink / raw)
To: Artur Paszkiewicz; +Cc: neilb, linux-raid
In-Reply-To: <54F0739E.50207@intel.com>
Artur Paszkiewicz <artur.paszkiewicz@intel.com> writes:
> On 02/25/2015 06:15 PM, Jes Sorensen wrote:
>> Artur Paszkiewicz <artur.paszkiewicz@intel.com> writes:
>>> On 02/25/2015 01:29 PM, Jes Sorensen wrote:
>>>> Artur Paszkiewicz <artur.paszkiewicz@intel.com> writes:
>>>>> On 02/24/2015 10:00 PM, Jes.Sorensen@redhat.com wrote:
>>>>>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>>>>>
>>>>>> This avoids adding the same orom entry to the oroms list multiple
>>>>>> times, as the comparison of pointers is never going to succeed, in
>>>>>> particular when '*orom' points to a local stack variable in the
>>>>>> calling function.
>>>>>>
>>>>>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>>>>>> ---
>>>>>> platform-intel.c | 4 ++--
>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/platform-intel.c b/platform-intel.c
>>>>>> index 37274da..a4ffa9f 100644
>>>>>> --- a/platform-intel.c
>>>>>> +++ b/platform-intel.c
>>>>>> @@ -255,8 +255,8 @@ static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
>>>>>> int i;
>>>>>>
>>>>>> for (i = 0; i < SYS_DEV_MAX; i++) {
>>>>>> - if (&oroms[i].orom == orom)
>>>>>> - return orom;
>>>>>> + if (!memcmp(&oroms[i].orom, orom, sizeof(struct imsm_orom)))
>>>>>> + return &oroms[i].orom;
>>>>>> if (oroms[i].orom.signature[0] == 0) {
>>>>>> oroms[i].orom = *orom;
>>>>>> return &oroms[i].orom;
>>>>>>
>>>>>
>>>>> Hi Jes,
>>>>>
>>>>> You are right that this can add the same entry multiple times, but this
>>>>> is how it is supposed to work. The oroms list should contain all the
>>>>> platform's oroms and they can be the same, this is why memcmp() should
>>>>> not be used here. We don't want to compare the contents of the
>>>>> structure, just its address. Sorry if it's not clear.
>>>>
>>>> Artur,
>>>>
>>>> Then the code is fundamentally broken, since you end up comparing a
>>>> stack variable against the oroms array when you call it from
>>>> find_imsm_efi(). Worse you can end up returning the local stack variable
>>>> declared in find_imsm_efi() to the calling function - there is no way
>>>> that can be correct.
>>>>
>>>> Look at this:
>>>>
>>>> static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
>>>> {
>>>> int i;
>>>>
>>>> for (i = 0; i < SYS_DEV_MAX; i++) {
>>>> if (&oroms[i].orom == orom)
>>>> return orom;
>>>> if (oroms[i].orom.signature[0] == 0) {
>>>> oroms[i].orom = *orom;
>>>> return &oroms[i].orom;
>>>> }
>>>> }
>>>> return NULL;
>>>> }
>>>>
>>>> const struct imsm_orom *find_imsm_efi(struct sys_dev *hba)
>>>> {
>>>> struct imsm_orom orom;
>>>> const struct imsm_orom *ret;
>>>> int err;
>>>>
>>>> ....
>>>>
>>>> ret = add_orom(&orom);
>>>> add_orom_device_id(ret, hba->dev_id);
>>>>
>>>> return ret;
>>>> }
>>>
>>> I can't see how this can lead to returning a stack variable. The oroms
>>> array is global and add_orom() will always return a pointer to a struct
>>> in this array. This comparison will always fail when we pass a pointer
>>> to a stack variable to add_orom():
>>>
>>> if (&oroms[i].orom == orom)
>>> return orom;
>>>
>>> This was meant to prevent adding an orom again like this:
>>>
>>> ret = add_orom(&orom);
>>> add_orom(ret);
>>>
>>> Maybe it would be more appropriate to return NULL to indicate that
>>> nothing was added instead of returning back the same pointer. I can do a
>>> patch for this. What do you think?
>>
>> It will fail because we know we're comparing a stack pointer, but it
>> raises red flags with tools like coverity and it is really bad coding
>> practice to rely on hacks like this.
>>
>> I also don't understand why you want to keep a table of identical
>> entries in the orom structure if multiple identical entries are found.
>> Each entry ought to match onto a specific physical controller, unless I
>> get something wrong?
>>
>
> OK, you're right, it is a hack. I thought it over and redesigned those
> orom functions. This should make it simpler and more consistent.
Looks a lot nicer to me :)
Jes
> Thanks,
> Artur
>
> From 673ecf1c0539f0050cc5934203af6d79cd68234d Mon Sep 17 00:00:00 2001
> From: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
> Date: Fri, 27 Feb 2015 10:34:20 +0100
> Subject: [PATCH] imsm: simplified multiple OROMs support
>
> Replaced oroms array with list, add_orom() now only appends to this list
> and add_orom_device_id() only appends devid_list node to an orom_entry.
>
> Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
> ---
> platform-intel.c | 96 +++++++++++++++++++++++++++-----------------------------
> platform-intel.h | 4 ++-
> super-intel.c | 18 +++++------
> 3 files changed, 57 insertions(+), 61 deletions(-)
>
> diff --git a/platform-intel.c b/platform-intel.c
> index 37274da..9c89c20 100644
> --- a/platform-intel.c
> +++ b/platform-intel.c
> @@ -229,65 +229,61 @@ struct pciExpDataStructFormat {
> __u16 devListOffset;
> } __attribute__ ((packed));
>
> -static struct orom_entry oroms[SYS_DEV_MAX];
> -
> -const struct orom_entry *get_oroms(void)
> -{
> - return (const struct orom_entry *)&oroms;
> -}
> +struct orom_entry *orom_entries;
>
> const struct imsm_orom *get_orom_by_device_id(__u16 dev_id)
> {
> - int i;
> - struct devid_list *list;
> + struct orom_entry *entry;
> + struct devid_list *devid;
>
> - for (i = 0; i < SYS_DEV_MAX; i++) {
> - for (list = oroms[i].devid_list; list; list = list->next) {
> - if (list->devid == dev_id)
> - return &oroms[i].orom;
> + for (entry = orom_entries; entry; entry = entry->next) {
> + for (devid = entry->devid_list; devid; devid = devid->next) {
> + if (devid->devid == dev_id)
> + return &entry->orom;
> }
> }
> +
> return NULL;
> }
>
> -static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
> +static struct orom_entry *add_orom(const struct imsm_orom *orom)
> {
> - int i;
> -
> - for (i = 0; i < SYS_DEV_MAX; i++) {
> - if (&oroms[i].orom == orom)
> - return orom;
> - if (oroms[i].orom.signature[0] == 0) {
> - oroms[i].orom = *orom;
> - return &oroms[i].orom;
> - }
> - }
> - return NULL;
> + struct orom_entry *list;
> + struct orom_entry *prev = NULL;
> +
> + for (list = orom_entries; list; prev = list, list = list->next)
> + ;
> +
> + list = xmalloc(sizeof(struct orom_entry));
> + list->orom = *orom;
> + list->devid_list = NULL;
> + list->next = NULL;
> +
> + if (prev == NULL)
> + orom_entries = list;
> + else
> + prev->next = list;
> +
> + return list;
> }
>
> -static void add_orom_device_id(const struct imsm_orom *orom, __u16 dev_id)
> +static void add_orom_device_id(struct orom_entry *entry, __u16 dev_id)
> {
> - int i;
> struct devid_list *list;
> struct devid_list *prev = NULL;
>
> - for (i = 0; i < SYS_DEV_MAX; i++) {
> - if (&oroms[i].orom == orom) {
> - for (list = oroms[i].devid_list; list; prev = list, list = list->next) {
> - if (list->devid == dev_id)
> - return;
> - }
> - list = xmalloc(sizeof(struct devid_list));
> - list->devid = dev_id;
> - list->next = NULL;
> -
> - if (prev == NULL)
> - oroms[i].devid_list = list;
> - else
> - prev->next = list;
> + for (list = entry->devid_list; list; prev = list, list = list->next) {
> + if (list->devid == dev_id)
> return;
> - }
> }
> + list = xmalloc(sizeof(struct devid_list));
> + list->devid = dev_id;
> + list->next = NULL;
> +
> + if (prev == NULL)
> + entry->devid_list = list;
> + else
> + prev->next = list;
> }
>
> static int scan(const void *start, const void *end, const void *data)
> @@ -321,7 +317,7 @@ static int scan(const void *start, const void *end, const void *data)
> if (!imsm_mem)
> return 0;
>
> - const struct imsm_orom *orom = add_orom(imsm_mem);
> + struct orom_entry *orom = add_orom(imsm_mem);
>
> if (ptr->devListOffset) {
> const __u16 *dev_list = (void *)ptr + ptr->devListOffset;
> @@ -367,11 +363,11 @@ const struct imsm_orom *imsm_platform_test(struct sys_dev *hba)
> IMSM_OROM_RLC_RAID10;
> }
>
> - const struct imsm_orom *ret = add_orom(&orom);
> + struct orom_entry *ret = add_orom(&orom);
>
> add_orom_device_id(ret, hba->dev_id);
>
> - return ret;
> + return &ret->orom;
> }
>
> static const struct imsm_orom *find_imsm_hba_orom(struct sys_dev *hba)
> @@ -508,7 +504,7 @@ static int read_efi_variable(void *buffer, ssize_t buf_size, char *variable_name
> const struct imsm_orom *find_imsm_efi(struct sys_dev *hba)
> {
> struct imsm_orom orom;
> - const struct imsm_orom *ret;
> + struct orom_entry *ret;
> int err;
>
> if (check_env("IMSM_TEST_AHCI_EFI") || check_env("IMSM_TEST_SCU_EFI"))
> @@ -529,14 +525,14 @@ const struct imsm_orom *find_imsm_efi(struct sys_dev *hba)
>
> /* try to read variable for combined AHCI controllers */
> if (err && hba->type == SYS_DEV_SATA) {
> - static const struct imsm_orom *csata;
> + static struct orom_entry *csata;
>
> err = read_efi_variable(&orom, sizeof(orom), AHCI_CSATA_PROP, VENDOR_GUID);
> if (!err) {
> if (!csata)
> csata = add_orom(&orom);
> add_orom_device_id(csata, hba->dev_id);
> - return csata;
> + return &csata->orom;
> }
> }
>
> @@ -546,12 +542,12 @@ const struct imsm_orom *find_imsm_efi(struct sys_dev *hba)
> ret = add_orom(&orom);
> add_orom_device_id(ret, hba->dev_id);
>
> - return ret;
> + return &ret->orom;
> }
>
> const struct imsm_orom *find_imsm_nvme(struct sys_dev *hba)
> {
> - static const struct imsm_orom *nvme_orom;
> + static struct orom_entry *nvme_orom;
>
> if (hba->type != SYS_DEV_NVME)
> return NULL;
> @@ -574,7 +570,7 @@ const struct imsm_orom *find_imsm_nvme(struct sys_dev *hba)
> nvme_orom = add_orom(&nvme_orom_compat);
> }
> add_orom_device_id(nvme_orom, hba->dev_id);
> - return nvme_orom;
> + return &nvme_orom->orom;
> }
>
> const struct imsm_orom *find_imsm_capability(struct sys_dev *hba)
> diff --git a/platform-intel.h b/platform-intel.h
> index 2ead431..631fa76 100644
> --- a/platform-intel.h
> +++ b/platform-intel.h
> @@ -213,8 +213,11 @@ struct devid_list {
> struct orom_entry {
> struct imsm_orom orom;
> struct devid_list *devid_list;
> + struct orom_entry *next;
> };
>
> +extern struct orom_entry *orom_entries;
> +
> static inline char *guid_str(char *buf, struct efi_guid guid)
> {
> sprintf(buf, "%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x",
> @@ -235,6 +238,5 @@ int devt_attached_to_hba(dev_t dev, const char *hba_path);
> char *devt_to_devpath(dev_t dev);
> int path_attached_to_hba(const char *disk_path, const char *hba_path);
> const char *get_sys_dev_type(enum sys_dev_type);
> -const struct orom_entry * get_oroms(void);
> const struct imsm_orom *get_orom_by_device_id(__u16 device_id);
> struct sys_dev *device_by_id(__u16 device_id);
> diff --git a/super-intel.c b/super-intel.c
> index 819e0da..53269fd 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -1948,13 +1948,12 @@ static int detail_platform_imsm(int verbose, int enumerate_only, char *controlle
> return result;
> }
>
> - const struct orom_entry *oroms = get_oroms();
> - int i;
> + const struct orom_entry *entry;
>
> - for (i = 0; i < SYS_DEV_MAX && oroms[i].devid_list; i++) {
> - print_imsm_capability(&oroms[i].orom);
> + for (entry = orom_entries; entry; entry = entry->next) {
> + print_imsm_capability(&entry->orom);
>
> - if (imsm_orom_is_nvme(&oroms[i].orom)) {
> + if (imsm_orom_is_nvme(&entry->orom)) {
> for (hba = list; hba; hba = hba->next) {
> if (hba->type == SYS_DEV_NVME)
> printf(" NVMe Device : %s\n", hba->path);
> @@ -1963,7 +1962,7 @@ static int detail_platform_imsm(int verbose, int enumerate_only, char *controlle
> }
>
> struct devid_list *devid;
> - for (devid = oroms[i].devid_list; devid; devid = devid->next) {
> + for (devid = entry->devid_list; devid; devid = devid->next) {
> hba = device_by_id(devid->devid);
> if (!hba)
> continue;
> @@ -2007,11 +2006,10 @@ static int export_detail_platform_imsm(int verbose, char *controller_path)
> result = 0;
> }
>
> - const struct orom_entry *oroms = get_oroms();
> - int i;
> + const struct orom_entry *entry;
>
> - for (i = 0; i < SYS_DEV_MAX && oroms[i].devid_list; i++)
> - print_imsm_capability_export(&oroms[i].orom);
> + for (entry = orom_entries; entry; entry = entry->next)
> + print_imsm_capability_export(&entry->orom);
>
> return result;
> }
^ permalink raw reply
* raid0: "clean" state on drive failure/removal
From: Sushma Gurram @ 2015-02-27 20:54 UTC (permalink / raw)
To: linux-raid@vger.kernel.org
Hi,
A basic question on raid0 behavior.
When a drive fails/removed in a raid0 array, “mdadm –detail” reports the array
state as clean instead of failed.
It appears that the drive is whacked out of the array, but that array slot
continues to show “active sync” state.
I/O to the failed drive errors out. However, it appears that I/O to other drives
in the raid0 array would succeed. How would the user know about the array state
and potential data loss? Should the array information be cached, drive
failure/removals be monitored and checked against the cached information?
Could anyone please explain the reason for this behavior of not reporting a
raid0 array as failed?
Thanks,
Sushma
________________________________
PLEASE NOTE: The information contained in this electronic mail message is intended only for the use of the designated recipient(s) named above. If the reader of this message is not the intended recipient, you are hereby notified that you have received this message in error and that any review, dissemination, distribution, or copying of this message is strictly prohibited. If you have received this communication in error, please notify the sender by telephone or e-mail (as shown above) immediately and destroy any and all copies of this message in your possession (whether hard copies or electronically stored copies).
--
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: raid0: "clean" state on drive failure/removal
From: NeilBrown @ 2015-02-27 21:56 UTC (permalink / raw)
To: Sushma Gurram; +Cc: linux-raid@vger.kernel.org
In-Reply-To: <B9FF37F8E2D7894A9493A5314D0816FC242755A9@SACMBXIP01.sdcorp.global.sandisk.com>
[-- Attachment #1: Type: text/plain, Size: 2351 bytes --]
On Fri, 27 Feb 2015 20:54:32 +0000 Sushma Gurram <Sushma.Gurram@sandisk.com>
wrote:
> Hi,
>
> A basic question on raid0 behavior.
>
> When a drive fails/removed in a raid0 array, “mdadm –detail” reports the array
> state as clean instead of failed.
>
> It appears that the drive is whacked out of the array, but that array slot
> continues to show “active sync” state.
>
> I/O to the failed drive errors out. However, it appears that I/O to other drives
> in the raid0 array would succeed. How would the user know about the array state
> and potential data loss? Should the array information be cached, drive
> failure/removals be monitored and checked against the cached information?
The user would know about potential data loss in *exactly* they same way as
if half the sectors on a single hard-drive stopped working.
i.e. the filesystem would complain.
>
> Could anyone please explain the reason for this behavior of not reporting a
> raid0 array as failed?
RAID0 is not really RAID - there is no redundancy.
So it isn't treated like RAID.
RAID0 is simply a mapping off linear addresses onto multiple devices.
There is no sense in which the array as a whole "fails", or any value in
marking a device as "failed".
If there is something wrong, then IO requests will fail. Not necessarily all
IO requests, but some. Just like a normal drive that has partially failed.
NeilBrown
>
> Thanks,
> Sushma
>
> ________________________________
>
> PLEASE NOTE: The information contained in this electronic mail message is intended only for the use of the designated recipient(s) named above. If the reader of this message is not the intended recipient, you are hereby notified that you have received this message in error and that any review, dissemination, distribution, or copying of this message is strictly prohibited. If you have received this communication in error, please notify the sender by telephone or e-mail (as shown above) immediately and destroy any and all copies of this message in your possession (whether hard copies or electronically stored copies).
>
> --
> 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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply
* [PATCH md] recover_bitmaps() can be static
From: kbuild test robot @ 2015-02-27 23:04 UTC (permalink / raw)
To: Goldwyn Rodrigues; +Cc: kbuild-all, Neil Brown, linux-raid, linux-kernel
In-Reply-To: <201502280727.kOhvF4kI%fengguang.wu@intel.com>
drivers/md/md-cluster.c:190:6: sparse: symbol 'recover_bitmaps' was not declared. Should it be static?
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
md-cluster.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
index 1f82d0d..c71217a 100644
--- a/drivers/md/md-cluster.c
+++ b/drivers/md/md-cluster.c
@@ -187,7 +187,7 @@ out:
return s;
}
-void recover_bitmaps(struct md_thread *thread)
+static void recover_bitmaps(struct md_thread *thread)
{
struct mddev *mddev = thread->mddev;
struct md_cluster_info *cinfo = mddev->cluster_info;
^ permalink raw reply related
* [md:for-next 14/48] drivers/md/md-cluster.c:190:6: sparse: symbol 'recover_bitmaps' was not declared. Should it be static?
From: kbuild test robot @ 2015-02-27 23:04 UTC (permalink / raw)
To: Goldwyn Rodrigues; +Cc: kbuild-all, Neil Brown, linux-raid, linux-kernel
tree: git://neil.brown.name/md for-next
head: 5d98513a2e546337c0deb93d772eb0c988d47c1d
commit: e94987db2ed983aea4e45d22db9e17c6bbf2a623 [14/48] Initiate recovery on node failure
reproduce:
# apt-get install sparse
git checkout e94987db2ed983aea4e45d22db9e17c6bbf2a623
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
drivers/md/md-cluster.c:178:15: sparse: restricted __le64 degrades to integer
>> drivers/md/md-cluster.c:190:6: sparse: symbol 'recover_bitmaps' was not declared. Should it be static?
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
http://lists.01.org/mailman/listinfo/kbuild Intel Corporation
^ permalink raw reply
* [PATCH] Communication Framework: fix semicolon.cocci warnings
From: kbuild test robot @ 2015-02-28 1:16 UTC (permalink / raw)
To: Goldwyn Rodrigues
Cc: kbuild-all, Lidong Zhong, Neil Brown, linux-raid, linux-kernel
In-Reply-To: <201502280906.22NYH7vD%fengguang.wu@intel.com>
drivers/md/md-cluster.c:328:2-3: Unneeded semicolon
Removes unneeded semicolon.
Generated by: scripts/coccinelle/misc/semicolon.cocci
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
md-cluster.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/drivers/md/md-cluster.c
+++ b/drivers/md/md-cluster.c
@@ -325,7 +325,7 @@ static void process_recvd_msg(struct mdd
pr_info("%s: %d Received message: RESYNCING from %d\n",
__func__, __LINE__, msg->slot);
break;
- };
+ }
}
/*
^ permalink raw reply
* block devices loosing state after resume: trigger udev rules to re-apply settings
From: Chris @ 2015-02-28 8:30 UTC (permalink / raw)
To: 779412; +Cc: 725284, linux-raid, smartmontools-support
(http://bugs.debian.org/779412 explanation)
There is a general problem with non-permanent block devices settings
(hard disks, optical disks, usb storage, ...), that are not restored
when resuming from suspend (instead using factory defaults and
loosing all pre-suspend settings).
And as long as the ata/scsi command set drivers can not save and
restore every state register a device may have (impossible?),
systemd may ship a viable workaround for this:
A systemd unit file could trigger an udev change action upon resume for
block devices. This way the same udev rules that set up the devices when
they are first plugged, will re-apply their settings after resume.
Providing this centrally with the systemd package could avoid that
multiple packages ship their own files, resulting in multiple change
events triggerd on each resume.
Examples for very important (non-permanent) settings are with
hdparm (i.e. the important -B hard disk wear settings)
https://bugs.debian.org/725284
smartctl/mdadm/lvm/btrfs/zfs/... (i.e. set error recovery timeouts to
prevent controller resets and data loss)
http://sourceforge.net/p/smartmontools/mailman/message/33501936/
A draft for such a central systemd unit file:
[Unit]
Description=Trigger all block device udev rules on resume, to re-apply all non-permanent device settings (e.g. smartctl and hdparm rules).
After=suspend.target After=hibernate.target
After=hybrid-sleep.target
[Service]
Type=oneshot
ExecStart=/sbin/udevadm trigger --action=change --subsystem-match=block
[Install]
WantedBy=suspend.target
WantedBy=hibernate.target
WantedBy=hybrid-sleep.target
^ permalink raw reply
* Bug#725284: Bug#779412: block devices loosing state after resume: trigger udev rules to re-apply settings
From: Michael Biebl @ 2015-02-28 8:38 UTC (permalink / raw)
To: Chris, 779412; +Cc: 725284, linux-raid, smartmontools-support
In-Reply-To: <20150228093023.14e30d9a@smtp.arcor.de>
[-- Attachment #1: Type: text/plain, Size: 1693 bytes --]
Am 28.02.2015 um 09:30 schrieb Chris:
> (http://bugs.debian.org/779412 explanation)
>
> There is a general problem with non-permanent block devices settings
> (hard disks, optical disks, usb storage, ...), that are not restored
> when resuming from suspend (instead using factory defaults and
> loosing all pre-suspend settings).
>
> And as long as the ata/scsi command set drivers can not save and
> restore every state register a device may have (impossible?),
> systemd may ship a viable workaround for this:
>
> A systemd unit file could trigger an udev change action upon resume for
> block devices. This way the same udev rules that set up the devices when
> they are first plugged, will re-apply their settings after resume.
>
> Providing this centrally with the systemd package could avoid that
> multiple packages ship their own files, resulting in multiple change
> events triggerd on each resume.
>
> Examples for very important (non-permanent) settings are with
> hdparm (i.e. the important -B hard disk wear settings)
> https://bugs.debian.org/725284
> smartctl/mdadm/lvm/btrfs/zfs/... (i.e. set error recovery timeouts to
> prevent controller resets and data loss)
> http://sourceforge.net/p/smartmontools/mailman/message/33501936/
>
I don't think working around this in udev/systemd is a good idea.
After all, most of those custom settings aren't applied via udev rules
anyway. This should be fixed in the kernel properly (or the individual
services) and not be papered over in systemd.
Marco, what do you think?
--
Why is it that all of the instruments seeking intelligent life in the
universe are pointed away from Earth?
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ 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