* [mdadm PATCH 1/4] Grow_continue_command: ensure 'content' is properly initialised.
From: NeilBrown @ 2017-04-20 2:40 UTC (permalink / raw)
To: Jes Sorensen; +Cc: linux-raid
In-Reply-To: <149265560315.31004.3851231165281498425.stgit@noble>
Grow_continue_command() call verify_reshape_position(), which assumes
that info->sys_name is initialised.
'info' in verify_reshape_position() is 'content' in Grow_continue_command().
In the st->ss->external != 0 branch of that function, sysfs_init() is called
to initialize content->sys_name.
In the st->ss->external == 0 branch, ->sys_name is not initialized so
verify_reshape_position() will not do the right thing.
Signed-off-by: NeilBrown <neilb@suse.com>
---
Grow.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/Grow.c b/Grow.c
index 15f4ed1253bf..c6967ed1c9c7 100755
--- a/Grow.c
+++ b/Grow.c
@@ -5002,6 +5002,7 @@ int Grow_continue_command(char *devname, int fd,
goto Grow_continue_command_exit;
}
content = &array;
+ sysfs_init(content, fd, NULL);
/* Need to load a superblock.
* FIXME we should really get what we need from
* sysfs
^ permalink raw reply related
* [mdadm PATCH 0/4] Assorted mdadm patches
From: NeilBrown @ 2017-04-20 2:40 UTC (permalink / raw)
To: Jes Sorensen; +Cc: linux-raid
These are 4 unrelated patches that I've gathered
over the last few weeks.
The last one is probably the most interesting and one
that you should probably think carefully before apply
(but I hope you'll decide in favour).
When you --assemble -or --create an array, udev immediately has a look
at the new device and might act on that content. e.g. tell udisks it
can mount a filesystem, or tell mdadm that there is part of a RAID
array in here.
When you --assemble an array you want that to happen.
When you --create it, you don't.
udev cannot distinguish 'assemble' from 'create'.
This can be annoying.
The way I have found to tell udev about the distinction to create a
/run/udev/rules.d/01-mdadm.rules file which marks any newly appearing
md array as SYSTEMD_READY==0. This is created before the array, and
removed once the array exisits (and hopefully has been handled by
udev).
Most udev rules which might process a newly appearing device will
avoid doing so i SYSTEMD_READY is set to 0.
Thanks,
NeilBrown
---
NeilBrown (4):
Grow_continue_command: ensure 'content' is properly initialised.
systemd/mdadm-last-resort: use ConditionPathExists instead of Conflicts
Detail: ensure --export names are acceptable as shell variables.
Create: tell udev device is not ready when first created.
Create.c | 9 +++++++++
Detail.c | 12 +++++++++---
Grow.c | 1 +
lib.c | 22 ++++++++++++++++++++++
mdadm.h | 2 ++
systemd/mdadm-last-resort@.service | 2 +-
6 files changed, 44 insertions(+), 4 deletions(-)
--
Signature
^ permalink raw reply
* [mdadm PATCH 2/4] systemd/mdadm-last-resort: use ConditionPathExists instead of Conflicts
From: NeilBrown @ 2017-04-20 2:40 UTC (permalink / raw)
To: Jes Sorensen; +Cc: linux-raid
In-Reply-To: <149265560315.31004.3851231165281498425.stgit@noble>
Commit cec72c071bbe ("systemd/mdadm-last-resort: add Conflicts to .service file.")
added a 'Conflicts' directive to the mdadm-last-resort@.service file in
the hope that this would make sure the service didn't run after the device
was active, even if the timer managed to get started, which is possible in
race conditions.
This seemed to work is testing, but it isn't clear why, and it is known
to cause problems.
If systemd happens to know that the mentioned device is a dependency of a
mount point, the Conflicts can unmount that mountpoint, which is certainly
not wanted.
So remove the "Conflicts" and instead use
ConditionPathExists=!/sys/devices/virtual/block/%i/md/sync_action
The "sync_action" file exists for any array which requires last-resort
handling, and only appears when the array is activated. So it is safe
to rely on it to determine if the last-resort is really needed.
Fixes: cec72c071bbe ("systemd/mdadm-last-resort: add Conflicts to .service file.")
Signed-off-by: NeilBrown <neilb@suse.com>
---
systemd/mdadm-last-resort@.service | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/systemd/mdadm-last-resort@.service b/systemd/mdadm-last-resort@.service
index e93d72b2b45e..f9d4d12738a3 100644
--- a/systemd/mdadm-last-resort@.service
+++ b/systemd/mdadm-last-resort@.service
@@ -1,7 +1,7 @@
[Unit]
Description=Activate md array even though degraded
DefaultDependencies=no
-Conflicts=sys-devices-virtual-block-%i.device
+ConditionPathExists=!/sys/devices/virtual/block/%i/md/sync_action
[Service]
Type=oneshot
^ permalink raw reply related
* [mdadm PATCH 3/4] Detail: ensure --export names are acceptable as shell variables.
From: NeilBrown @ 2017-04-20 2:40 UTC (permalink / raw)
To: Jes Sorensen; +Cc: linux-raid
In-Reply-To: <149265560315.31004.3851231165281498425.stgit@noble>
If an array contains a device which has a name that
contains something other than alphnumerics and underscores,
then some values reported by "mdadm --detail --export" will
not be valid as variable assignment of the shell.
This particularly affects dm devices.
e.g.
MD_DEVICE_dm-4_ROLE=1
MD_DEVICE_dm-4_DEV=/dev/dm-4
As it is particularly useful to be able to work with these
in a shell script, and as the precise name is not important,
change all non-alphanumerics to '_'.
MD_DEVICE_dm_4_ROLE=1
MD_DEVICE_dm_4_DEV=/dev/dm-4
Signed-off-by: NeilBrown <neilb@suse.com>
---
Detail.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/Detail.c b/Detail.c
index d4e620438f93..7c3dac8ba8ec 100644
--- a/Detail.c
+++ b/Detail.c
@@ -25,6 +25,7 @@
#include "mdadm.h"
#include "md_p.h"
#include "md_u.h"
+#include <ctype.h>
#include <dirent.h>
static int cmpstringp(const void *p1, const void *p2)
@@ -276,17 +277,22 @@ int Detail(char *dev, struct context *c)
char *path =
map_dev(mdi->disk.major,
mdi->disk.minor, 0);
+ char *sysdev = xstrdup(mdi->sys_name + 1);
+ char *cp;
+ for (cp = sysdev; *cp; cp++)
+ if (!isalnum(*cp))
+ *cp = '_';
if (mdi->disk.raid_disk >= 0)
printf("MD_DEVICE_%s_ROLE=%d\n",
- mdi->sys_name+4,
+ sysdev,
mdi->disk.raid_disk);
else
printf("MD_DEVICE_%s_ROLE=spare\n",
- mdi->sys_name+4);
+ sysdev);
if (path)
printf("MD_DEVICE_%s_DEV=%s\n",
- mdi->sys_name+4, path);
+ sysdev, path);
}
}
goto out;
^ permalink raw reply related
* [mdadm PATCH 4/4] Create: tell udev device is not ready when first created.
From: NeilBrown @ 2017-04-20 2:40 UTC (permalink / raw)
To: Jes Sorensen; +Cc: linux-raid
In-Reply-To: <149265560315.31004.3851231165281498425.stgit@noble>
When an array is created the content is not initializes,
so it could have remnants of an old filesystem or md array
etc on it.
udev will see this and might try to activate it, which is almost
certainly not what is wanted.
So create a temporary udev rules files to set ENV{SYSTEMD_READY}="0"
while the creation event is processed. This is fairly uniformly
used to suppress actions based on the contents of the device.
Signed-off-by: NeilBrown <neilb@suse.com>
---
Create.c | 9 +++++++++
lib.c | 22 ++++++++++++++++++++++
mdadm.h | 2 ++
3 files changed, 33 insertions(+)
diff --git a/Create.c b/Create.c
index 6ca092449880..915ba6f49fe8 100644
--- a/Create.c
+++ b/Create.c
@@ -605,9 +605,11 @@ int Create(struct supertype *st, char *mddev,
/* We need to create the device */
map_lock(&map);
+ udev_block();
mdfd = create_mddev(mddev, name, c->autof, LOCAL, chosen_name);
if (mdfd < 0) {
map_unlock(&map);
+ udev_unblock();
return 1;
}
/* verify if chosen_name is not in use,
@@ -620,6 +622,7 @@ int Create(struct supertype *st, char *mddev,
chosen_name);
close(mdfd);
map_unlock(&map);
+ udev_unblock();
return 1;
}
mddev = chosen_name;
@@ -1053,9 +1056,15 @@ int Create(struct supertype *st, char *mddev,
pr_err("not starting array - not enough devices.\n");
}
close(mdfd);
+ /* Give udev a moment to process the Change event caused
+ * by the close.
+ */
+ usleep(100*1000);
+ udev_unblock();
return 0;
abort:
+ udev_unblock();
map_lock(&map);
abort_locked:
map_remove(&map, fd2devnm(mdfd));
diff --git a/lib.c b/lib.c
index b640634ef6f2..c38dd7cf1660 100644
--- a/lib.c
+++ b/lib.c
@@ -163,6 +163,28 @@ char *fd2devnm(int fd)
return NULL;
}
+/* When we create a new array, we don't want the content to
+ * be immediately examined by udev - it is probably meaningless.
+ * So create /run/udev/rules.d/01-mdadm-create.rules to tell udev
+ * that the device isn't ready.
+ */
+static char udev_path[] = "/run/udev/rules.d/01-mdadm-create.rules";
+void udev_block(void)
+{
+ FILE *f;
+
+ f = fopen(udev_path, "w");
+ if (f) {
+ fputs("KERNEL==\"md*\", ENV{SYSTEMD_READY}=\"0\"\n", f);
+ fclose(f);
+ }
+}
+
+void udev_unblock(void)
+{
+ unlink(udev_path);
+}
+
/*
* convert a major/minor pair for a block device into a name in /dev, if possible.
* On the first call, walk /dev collecting name.
diff --git a/mdadm.h b/mdadm.h
index f1f643c794d4..fa4d71fc91f0 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -1556,6 +1556,8 @@ extern char *stat2kname(struct stat *st);
extern char *fd2kname(int fd);
extern char *stat2devnm(struct stat *st);
extern char *fd2devnm(int fd);
+extern void udev_block(void);
+extern void udev_unblock(void);
extern int in_initrd(void);
^ permalink raw reply related
* Re: Zombie dm device?
From: Zhilong Liu @ 2017-04-20 2:55 UTC (permalink / raw)
To: Phil Susi, linux-raid
In-Reply-To: <5b349798-c9de-04ef-d1dd-bc7774001903@ubuntu.com>
On 04/18/2017 09:00 PM, Phil Susi wrote:
> On 4/17/2017 10:36 PM, Zhilong Liu wrote:
>> can you find the /sys/block/md99/md/array_state? issue the
>> # echo clear > /sys/block/md99/md/array_state to stop array.
>> The test suit is in improving progress, a few test cases cannot
>> work well currently. you can talk about your requirement, and
>> I would record it. more details, more appreciate. :-)
> It already said it was clear. Attempting to clear it again apparently
> rebooted the virtual machine and for some reason, syslog and kern.log
> are empty and the previous one is last dated 8 days ago.
>
> Odd that it just rebooted too since I'm pretty sure a panic should have
> just halted the system with the panic message written to the vnc console.
however, we cannot put up with panic, would you mind to retest and lock
on which test
case triggered panic and reproduced, I would test it on my site and find
why it happens.
Thanks,
-Zhilong
>
> --
> 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: IMSM RAID10: Rebuild/Resync difference on Linux vs Windows
From: Matthias Dahl @ 2017-04-20 7:44 UTC (permalink / raw)
To: Artur Paszkiewicz, linux-raid
In-Reply-To: <4f749c02-3478-cafc-2ae4-40af25459f65@intel.com>
Hello Artur...
So, the same thing happened again yesterday: The RAID10 was out of sync
due to an unclean shutdown and while Linux was at 70.8% of the resync, I
restarted to Win 10 with RST 15.2.0.1020 and it recognized the RAID as
clean without any visible/noticeable verification at all.
I did a manual verification run in Win this time, just for the sake of
it, and it came up fine.
Nevertheless, this really has me worried. Did you get / see my last mail
with the information you requested?
By the way, kernel version was 4.10.8 with mdadm/mdmon 4.0.
Thanks,
Matthias
--
Dipl.-Inf. (FH) Matthias Dahl | Software Engineer | binary-island.eu
Hire me for contract work: Open Source, Proprietary, Web/Mobile/Desktop
^ permalink raw reply
* RE: [PATCH] drivers, md: convert stripe_head.count from atomic_t to refcount_t
From: Reshetova, Elena @ 2017-04-20 9:15 UTC (permalink / raw)
To: shli@kernel.org
Cc: linux-kernel@vger.kernel.org, linux-raid@vger.kernel.org,
Hans Liljestrand, Kees Cook, David Windsor
In-Reply-To: <1489401771-31169-1-git-send-email-elena.reshetova@intel.com>
Just a ping on this updated patch. Did you find any new issues or could you please apply it?
Best Regards,
Elena.
> refcount_t type and corresponding API should be
> used instead of atomic_t when the variable is used as
> a reference counter. This allows to avoid accidental
> refcounter overflows that might lead to use-after-free
> situations.
>
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: David Windsor <dwindsor@gmail.com>
> ---
> drivers/md/raid5-cache.c | 8 +++---
> drivers/md/raid5.c | 68 +++++++++++++++++++++++++-----------------------
> drivers/md/raid5.h | 3 ++-
> 3 files changed, 41 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
> index 3f307be..6c05e12 100644
> --- a/drivers/md/raid5-cache.c
> +++ b/drivers/md/raid5-cache.c
> @@ -979,7 +979,7 @@ int r5l_write_stripe(struct r5l_log *log, struct stripe_head
> *sh)
> * don't delay.
> */
> clear_bit(STRIPE_DELAYED, &sh->state);
> - atomic_inc(&sh->count);
> + refcount_inc(&sh->count);
>
> mutex_lock(&log->io_mutex);
> /* meta + data */
> @@ -1321,7 +1321,7 @@ static void r5c_flush_stripe(struct r5conf *conf, struct
> stripe_head *sh)
> assert_spin_locked(&conf->device_lock);
>
> list_del_init(&sh->lru);
> - atomic_inc(&sh->count);
> + refcount_inc(&sh->count);
>
> set_bit(STRIPE_HANDLE, &sh->state);
> atomic_inc(&conf->active_stripes);
> @@ -1424,7 +1424,7 @@ static void r5c_do_reclaim(struct r5conf *conf)
> */
> if (!list_empty(&sh->lru) &&
> !test_bit(STRIPE_HANDLE, &sh-
> >state) &&
> - atomic_read(&sh->count) == 0) {
> + refcount_read(&sh->count) == 0) {
> r5c_flush_stripe(conf,
> sh);
> if (count++ >=
> R5C_RECLAIM_STRIPE_GROUP)
> break;
> @@ -2650,7 +2650,7 @@ r5c_cache_data(struct r5l_log *log, struct stripe_head
> *sh,
> * don't delay.
> */
> clear_bit(STRIPE_DELAYED, &sh->state);
> - atomic_inc(&sh->count);
> + refcount_inc(&sh->count);
>
> mutex_lock(&log->io_mutex);
> /* meta + data */
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 2ce23b0..7e3913a 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -296,7 +296,7 @@ static void do_release_stripe(struct r5conf *conf, struct
> stripe_head *sh,
> static void __release_stripe(struct r5conf *conf, struct stripe_head *sh,
> struct list_head
> *temp_inactive_list)
> {
> - if (atomic_dec_and_test(&sh->count))
> + if (refcount_dec_and_test(&sh->count))
> do_release_stripe(conf, sh, temp_inactive_list);
> }
>
> @@ -388,7 +388,7 @@ void raid5_release_stripe(struct stripe_head *sh)
>
> /* Avoid release_list until the last reference.
> */
> - if (atomic_add_unless(&sh->count, -1, 1))
> + if (refcount_dec_not_one(&sh->count))
> return;
>
> if (unlikely(!conf->mddev->thread) ||
> @@ -401,7 +401,7 @@ void raid5_release_stripe(struct stripe_head *sh)
> slow_path:
> local_irq_save(flags);
> /* we are ok here if STRIPE_ON_RELEASE_LIST is set or not */
> - if (atomic_dec_and_lock(&sh->count, &conf->device_lock)) {
> + if (refcount_dec_and_lock(&sh->count, &conf->device_lock)) {
> INIT_LIST_HEAD(&list);
> hash = sh->hash_lock_index;
> do_release_stripe(conf, sh, &list);
> @@ -491,7 +491,7 @@ static void init_stripe(struct stripe_head *sh, sector_t
> sector, int previous)
> struct r5conf *conf = sh->raid_conf;
> int i, seq;
>
> - BUG_ON(atomic_read(&sh->count) != 0);
> + BUG_ON(refcount_read(&sh->count) != 0);
> BUG_ON(test_bit(STRIPE_HANDLE, &sh->state));
> BUG_ON(stripe_operations_active(sh));
> BUG_ON(sh->batch_head);
> @@ -668,11 +668,11 @@ raid5_get_active_stripe(struct r5conf *conf, sector_t
> sector,
> &conf-
> >cache_state);
> } else {
> init_stripe(sh, sector,
> previous);
> - atomic_inc(&sh-
> >count);
> + refcount_inc(&sh-
> >count);
> }
> - } else if (!atomic_inc_not_zero(&sh->count)) {
> + } else if (!refcount_inc_not_zero(&sh->count)) {
> spin_lock(&conf->device_lock);
> - if (!atomic_read(&sh->count)) {
> + if (!refcount_read(&sh->count)) {
> if
> (!test_bit(STRIPE_HANDLE, &sh->state))
>
> atomic_inc(&conf->active_stripes);
>
> BUG_ON(list_empty(&sh->lru) &&
> @@ -688,7 +688,7 @@ raid5_get_active_stripe(struct r5conf *conf, sector_t
> sector,
> sh-
> >group = NULL;
> }
> }
> - atomic_inc(&sh->count);
> + refcount_inc(&sh->count);
> spin_unlock(&conf->device_lock);
> }
> } while (sh == NULL);
> @@ -752,9 +752,9 @@ static void stripe_add_to_batch_list(struct r5conf *conf,
> struct stripe_head *sh
> hash = stripe_hash_locks_hash(head_sector);
> spin_lock_irq(conf->hash_locks + hash);
> head = __find_stripe(conf, head_sector, conf->generation);
> - if (head && !atomic_inc_not_zero(&head->count)) {
> + if (head && !refcount_inc_not_zero(&head->count)) {
> spin_lock(&conf->device_lock);
> - if (!atomic_read(&head->count)) {
> + if (!refcount_read(&head->count)) {
> if (!test_bit(STRIPE_HANDLE, &head-
> >state))
> atomic_inc(&conf-
> >active_stripes);
> BUG_ON(list_empty(&head->lru) &&
> @@ -770,7 +770,7 @@ static void stripe_add_to_batch_list(struct r5conf *conf,
> struct stripe_head *sh
> head->group = NULL;
> }
> }
> - atomic_inc(&head->count);
> + refcount_inc(&head->count);
> spin_unlock(&conf->device_lock);
> }
> spin_unlock_irq(conf->hash_locks + hash);
> @@ -833,7 +833,7 @@ static void stripe_add_to_batch_list(struct r5conf *conf,
> struct stripe_head *sh
> sh->batch_head->bm_seq = seq;
> }
>
> - atomic_inc(&sh->count);
> + refcount_inc(&sh->count);
> unlock_out:
> unlock_two_stripes(head, sh);
> out:
> @@ -1036,9 +1036,9 @@ static void ops_run_io(struct stripe_head *sh, struct
> stripe_head_state *s)
> pr_debug("%s: for %llu schedule op
> %d on disc %d\n",
> __func__, (unsigned
> long long)sh->sector,
> bi->bi_opf, i);
> - atomic_inc(&sh->count);
> + refcount_inc(&sh->count);
> if (sh != head_sh)
> - atomic_inc(&head_sh-
> >count);
> +
> refcount_inc(&head_sh->count);
> if (use_new_offset(conf, sh))
> bi->bi_iter.bi_sector =
> (sh->sector
>
> + rdev->new_data_offset);
> @@ -1097,9 +1097,9 @@ static void ops_run_io(struct stripe_head *sh, struct
> stripe_head_state *s)
> "replacement disc
> %d\n",
> __func__, (unsigned
> long long)sh->sector,
> rbi->bi_opf, i);
> - atomic_inc(&sh->count);
> + refcount_inc(&sh->count);
> if (sh != head_sh)
> - atomic_inc(&head_sh-
> >count);
> +
> refcount_inc(&head_sh->count);
> if (use_new_offset(conf, sh))
> rbi->bi_iter.bi_sector =
> (sh->sector
>
> + rrdev->new_data_offset);
> @@ -1275,7 +1275,7 @@ static void ops_run_biofill(struct stripe_head *sh)
> }
> }
>
> - atomic_inc(&sh->count);
> + refcount_inc(&sh->count);
> init_async_submit(&submit, ASYNC_TX_ACK, tx,
> ops_complete_biofill, sh, NULL);
> async_trigger_callback(&submit);
> }
> @@ -1353,7 +1353,7 @@ ops_run_compute5(struct stripe_head *sh, struct
> raid5_percpu *percpu)
> if (i != target)
> xor_srcs[count++] = sh->dev[i].page;
>
> - atomic_inc(&sh->count);
> + refcount_inc(&sh->count);
>
> init_async_submit(&submit,
> ASYNC_TX_FENCE|ASYNC_TX_XOR_ZERO_DST, NULL,
> ops_complete_compute, sh,
> to_addr_conv(sh, percpu, 0));
> @@ -1441,7 +1441,7 @@ ops_run_compute6_1(struct stripe_head *sh, struct
> raid5_percpu *percpu)
> BUG_ON(!test_bit(R5_Wantcompute, &tgt->flags));
> dest = tgt->page;
>
> - atomic_inc(&sh->count);
> + refcount_inc(&sh->count);
>
> if (target == qd_idx) {
> count = set_syndrome_sources(blocks, sh,
> SYNDROME_SRC_ALL);
> @@ -1516,7 +1516,7 @@ ops_run_compute6_2(struct stripe_head *sh, struct
> raid5_percpu *percpu)
> pr_debug("%s: stripe: %llu faila: %d failb: %d\n",
> __func__, (unsigned long long)sh->sector, faila,
> failb);
>
> - atomic_inc(&sh->count);
> + refcount_inc(&sh->count);
>
> if (failb == syndrome_disks+1) {
> /* Q disk is one of the missing disks */
> @@ -1784,7 +1784,7 @@ ops_run_reconstruct5(struct stripe_head *sh, struct
> raid5_percpu *percpu,
> break;
> }
> if (i >= sh->disks) {
> - atomic_inc(&sh->count);
> + refcount_inc(&sh->count);
> set_bit(R5_Discard, &sh->dev[pd_idx].flags);
> ops_complete_reconstruct(sh);
> return;
> @@ -1825,7 +1825,7 @@ ops_run_reconstruct5(struct stripe_head *sh, struct
> raid5_percpu *percpu,
> flags = ASYNC_TX_ACK |
> (prexor ? ASYNC_TX_XOR_DROP_DST
> : ASYNC_TX_XOR_ZERO_DST);
>
> - atomic_inc(&head_sh->count);
> + refcount_inc(&head_sh->count);
> init_async_submit(&submit, flags, tx,
> ops_complete_reconstruct, head_sh,
> to_addr_conv(sh,
> percpu, j));
> } else {
> @@ -1867,7 +1867,7 @@ ops_run_reconstruct6(struct stripe_head *sh, struct
> raid5_percpu *percpu,
> break;
> }
> if (i >= sh->disks) {
> - atomic_inc(&sh->count);
> + refcount_inc(&sh->count);
> set_bit(R5_Discard, &sh->dev[sh->pd_idx].flags);
> set_bit(R5_Discard, &sh->dev[sh->qd_idx].flags);
> ops_complete_reconstruct(sh);
> @@ -1891,7 +1891,7 @@ ops_run_reconstruct6(struct stripe_head *sh, struct
> raid5_percpu *percpu,
> struct stripe_head,
> batch_list) == head_sh;
>
> if (last_stripe) {
> - atomic_inc(&head_sh->count);
> + refcount_inc(&head_sh->count);
> init_async_submit(&submit, txflags, tx,
> ops_complete_reconstruct,
> head_sh,
> to_addr_conv(sh, percpu, j));
> } else
> @@ -1948,7 +1948,7 @@ static void ops_run_check_p(struct stripe_head *sh,
> struct raid5_percpu *percpu)
> tx = async_xor_val(xor_dest, xor_srcs, 0, count, STRIPE_SIZE,
> &sh->ops.zero_sum_result,
> &submit);
>
> - atomic_inc(&sh->count);
> + refcount_inc(&sh->count);
> init_async_submit(&submit, ASYNC_TX_ACK, tx,
> ops_complete_check, sh, NULL);
> tx = async_trigger_callback(&submit);
> }
> @@ -1967,7 +1967,7 @@ static void ops_run_check_pq(struct stripe_head *sh,
> struct raid5_percpu *percpu
> if (!checkp)
> srcs[count] = NULL;
>
> - atomic_inc(&sh->count);
> + refcount_inc(&sh->count);
> init_async_submit(&submit, ASYNC_TX_ACK, NULL,
> ops_complete_check,
> sh, to_addr_conv(sh, percpu, 0));
> async_syndrome_val(srcs, 0, count+2, STRIPE_SIZE,
> @@ -2057,7 +2057,7 @@ static struct stripe_head *alloc_stripe(struct
> kmem_cache *sc, gfp_t gfp,
> INIT_LIST_HEAD(&sh->lru);
> INIT_LIST_HEAD(&sh->r5c);
> INIT_LIST_HEAD(&sh->log_list);
> - atomic_set(&sh->count, 1);
> + refcount_set(&sh->count, 1);
> sh->log_start = MaxSector;
> for (i = 0; i < disks; i++) {
> struct r5dev *dev = &sh->dev[i];
> @@ -2354,7 +2354,7 @@ static int drop_one_stripe(struct r5conf *conf)
> spin_unlock_irq(conf->hash_locks + hash);
> if (!sh)
> return 0;
> - BUG_ON(atomic_read(&sh->count));
> + BUG_ON(refcount_read(&sh->count));
> shrink_buffers(sh);
> kmem_cache_free(conf->slab_cache, sh);
> atomic_dec(&conf->active_stripes);
> @@ -2386,7 +2386,7 @@ static void raid5_end_read_request(struct bio * bi)
> break;
>
> pr_debug("end_read_request %llu/%d, count: %d, error %d.\n",
> - (unsigned long long)sh->sector, i, atomic_read(&sh-
> >count),
> + (unsigned long long)sh->sector, i,
> refcount_read(&sh->count),
> bi->bi_error);
> if (i == disks) {
> bio_reset(bi);
> @@ -2523,7 +2523,7 @@ static void raid5_end_write_request(struct bio *bi)
> }
> }
> pr_debug("end_write_request %llu/%d, count %d, error: %d.\n",
> - (unsigned long long)sh->sector, i, atomic_read(&sh-
> >count),
> + (unsigned long long)sh->sector, i,
> refcount_read(&sh->count),
> bi->bi_error);
> if (i == disks) {
> bio_reset(bi);
> @@ -4545,7 +4545,7 @@ static void handle_stripe(struct stripe_head *sh)
> pr_debug("handling stripe %llu, state=%#lx cnt=%d, "
> "pd_idx=%d, qd_idx=%d\n, check:%d,
> reconstruct:%d\n",
> (unsigned long long)sh->sector, sh->state,
> - atomic_read(&sh->count), sh->pd_idx, sh->qd_idx,
> + refcount_read(&sh->count), sh->pd_idx, sh->qd_idx,
> sh->check_state, sh->reconstruct_state);
>
> analyse_stripe(sh, &s);
> @@ -4924,7 +4924,7 @@ static void activate_bit_delay(struct r5conf *conf,
> struct stripe_head *sh = list_entry(head.next, struct
> stripe_head, lru);
> int hash;
> list_del_init(&sh->lru);
> - atomic_inc(&sh->count);
> + refcount_inc(&sh->count);
> hash = sh->hash_lock_index;
> __release_stripe(conf, sh,
> &temp_inactive_list[hash]);
> }
> @@ -5240,7 +5240,9 @@ static struct stripe_head *__get_priority_stripe(struct
> r5conf *conf, int group)
> sh->group = NULL;
> }
> list_del_init(&sh->lru);
> - BUG_ON(atomic_inc_return(&sh->count) != 1);
> + BUG_ON(refcount_read(&sh->count) != 0);
> + refcount_set(&sh->count, 1);
> +
> return sh;
> }
>
> diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> index 4bb27b9..a1ed351 100644
> --- a/drivers/md/raid5.h
> +++ b/drivers/md/raid5.h
> @@ -3,6 +3,7 @@
>
> #include <linux/raid/xor.h>
> #include <linux/dmaengine.h>
> +#include <linux/refcount.h>
>
> /*
> *
> @@ -207,7 +208,7 @@ struct stripe_head {
> short ddf_layout;/* use DDF
> ordering to calculate Q */
> short hash_lock_index;
> unsigned long state; /* state
> flags */
> - atomic_t count; /* nr of active
> thread/requests */
> + refcount_t count; /* nr of active
> thread/requests */
> int bm_seq; /*
> sequence number for bitmap flushes */
> int disks;
> /* disks in stripe */
> int overwrite_disks; /*
> total overwrite disks in stripe,
> --
> 2.7.4
^ permalink raw reply
* Re: IMSM RAID10: Rebuild/Resync difference on Linux vs Windows
From: Artur Paszkiewicz @ 2017-04-20 10:06 UTC (permalink / raw)
To: Matthias Dahl, linux-raid
In-Reply-To: <01c2b1cd-1893-d299-5907-0b5be5f13776@binary-island.eu>
On 04/20/2017 09:44 AM, Matthias Dahl wrote:
> Hello Artur...
>
> So, the same thing happened again yesterday: The RAID10 was out of sync
> due to an unclean shutdown and while Linux was at 70.8% of the resync, I
> restarted to Win 10 with RST 15.2.0.1020 and it recognized the RAID as
> clean without any visible/noticeable verification at all.
>
> I did a manual verification run in Win this time, just for the sake of
> it, and it came up fine.
>
> Nevertheless, this really has me worried. Did you get / see my last mail
> with the information you requested?
>
> By the way, kernel version was 4.10.8 with mdadm/mdmon 4.0.
Hi Matthias,
This really is strange, I'll try to ask someone from the Windows RST
team about this. Unfortunately I don't have any knowledge about the
Windows driver. I saw your earlier email and everything looked OK there.
Does this happen only when you reboot to Windows? Have you ever rebooted
during resync again to Linux and did resync resume as expected? Also,
have you noticed what state did the BIOS show for the array when you
rebooted? I'm asking because this might not be caused by Windows but
maybe the BIOS/UEFI driver or Linux not updating the metadata properly
at shutdown.
Thanks,
Artur
^ permalink raw reply
* MD raid microconference of Plumber Conf 2017
From: Coly Li @ 2017-04-20 14:57 UTC (permalink / raw)
To: open list:SOFTWARE RAID (Multiple Disks) SUPPORT
Hi MD developers,
I just propose a microconference proposal to Linux Plumber Conf 2017,
the topic is "MD raid general discussion".
The proposal wiki page can be found here:
http://wiki.linuxplumbersconf.org/2017:md_raid_general_discussion
Please help to add more information to this wiki page (topics,
attendees, and other information). I do hope we can meet during LPC2017
and have face to face conversation, that will be great :-)
Thanks in advance.
Coly Li
^ permalink raw reply
* Re: GET_ARRAY_INFO assumptions?
From: Jes Sorensen @ 2017-04-20 16:05 UTC (permalink / raw)
To: NeilBrown, Shaohua Li; +Cc: linux-raid
In-Reply-To: <87k26itx2v.fsf@notabene.neil.brown.name>
On 04/17/2017 07:48 PM, NeilBrown wrote:
> On Fri, Apr 14 2017, Jes Sorensen wrote:
>> Looking some more at this, it may be simpler than I thought. How about
>> this approach (only compile tested):
>>
>> int md_array_active(int fd)
>> {
>> struct mdinfo *sra;
>> struct mdu_array_info_s array;
>> int ret;
>>
>> sra = sysfs_read(fd, NULL, GET_VERSION | GET_DISKS);
>> if (sra) {
>> if (!sra->array.raid_disks &&
>> !(sra->array.major_version == -1 &&
>> sra->array.minor_version == -2))
>> ret = -ENODEV;
>> else
>> ret = 0;
>>
>> free(sra);
>> } else {
>> ret = ioctl(fd, GET_ARRAY_INFO, &array);
>> }
>>
>> return !ret;
>> }
>>
>> Note 'major = -1 && minor = -2' is sysfs_read's way of saying 'external'.
>>
>> This pretty much mimics what the kernel does in the ioctl handler for
>> GET_ARRAY_INFO:
>>
>> case GET_ARRAY_INFO:
>> if (!mddev->raid_disks && !mddev->external)
>> err = -ENODEV;
>> else
>> err = get_array_info(mddev, argp);
>> goto out;
>>
>> What do you think?
>
> I think that it accurately mimics what the current code does.
> I'm not sure that is what we really want.
> For testing in Incremental.c if an array is "active" we really
> should be testing more than "raid_disks != 0".
> We should be testing, as Shaohua suggested, if
> array_state != 'clear' or 'inactive'.
> You cannot get that info through the ioctl interface, so I suppose
> I decided the current test was 'close enough'.
> If we are going to stop supported kernels that don't have (e.g.)
> array_state, then we should really fo the right thing and test
> array_state.
I think I got it right this time and pushed it into git. It made things
a lot prettier too IMHO :)
In the process I also changed the behavior of
sysfs_read(GET_ARRAY_STATE) as I really didn't like how it was copying
in the string rather than parsing it.
I am traveling at the moment and don't yet have my new raid test box
setup back at the office, so my testing is limited. If I broke something
badly, feel free to throw rotten tomatoes at me.
Cheers,
Jes
^ permalink raw reply
* Re: mdadm Consistency Policy initialization
From: Jes Sorensen @ 2017-04-20 16:08 UTC (permalink / raw)
To: Artur Paszkiewicz; +Cc: linux-raid
In-Reply-To: <8c3ddfa5-2069-b2e2-40c6-e1ec14b2461f@intel.com>
On 04/19/2017 06:29 AM, Artur Paszkiewicz wrote:
> On 04/18/2017 06:50 PM, Jes Sorensen wrote:
>> Hi Artur,
>>
>> In 5308f11727b889965efe5ac0e854d197c2b51f6d you introduced struct mdinfo: enum consistency_policy, but in mdadm.c you initialize it to UnSet which isn't part of the enum.
>>
>> Is there any actual difference between CONSISTENCY_POLICY_UNKNOWN and UnSet? It seems suboptimal to mix and match within the enum like this, and if CONSISTENCY_POLICY_UNKNOWN does the job, couldn't we just initialize with that?
>
> Hi Jes,
>
> The "enum consistency_policy" and "mapping_t consistency_policies[]"
> represent values that can appear in sysfs. md/consistency_policy can be
> "unknown" when the array is inactive. On the other hand, UnSet just
> means that the --consistency-policy= parameter was not provided by the
> user. I wanted to differentiate between these two cases. If you think
> this is redundant I can change it and use CONSISTENCY_POLICY_UNKNOWN
> instead, this should be straightforward.
Hi Artur,
I made some changes to map_name() and noticed that you already there
default to CONSISTENCY_POLICY_UNKNOWN if it returns UnSet in
sysfs_read(). However given that you do a lot of checks manually outside
of sysfs.c, I dind't change the code there for now. I think we the
change I made to map_name() it should be possible to simply a bunch of
that checking code.
Cheers,
Jes
^ permalink raw reply
* issue with assembly of named arrays w/selinux==permissive on CentOS7
From: Scott Sinno @ 2017-04-20 16:31 UTC (permalink / raw)
To: linux-raid
Cc: Jasen, John E.,
Spear, Carrie E. (GSFC-606.0)[COMPUTER SCIENCE CORP],
David M. Kemeza
On a reasonably vanilla CentOS 7 platform, we're having trouble
assembling *named* mdadm arrays using selinux==permissive. An example
from mdadm.conf
ARRAY /dev/md/upper7 UUID=e1203894:e9f3cd5d:94472a7f:3553137b
name=upper7 devices=/dev/dm-[0-9]*
mdadm will quietly exit code 2 when we attempt assembly. If selinux is
disabled outright, OR the array definition is changed to something more
conventional, ala
ARRAY /dev/md17 UUID=e1203894:e9f3cd5d:94472a7f:3553137b
devices=/dev/dm-[0-9]*
It assembles just fine, reports healthy, etc. Any insight?
#basic sysinfo
root@dass01:~# cat /etc/redhat-release
CentOS Linux release 7.2.1511 (Core)
root@dass01:~# uname -r
3.10.0-327.36.3.el7.x86_64
root@dass01:~# mdadm --version
mdadm - v3.3.2 - 21st August 2014
^ permalink raw reply
* Re: MD raid microconference of Plumber Conf 2017
From: Shaohua Li @ 2017-04-20 16:34 UTC (permalink / raw)
To: Coly Li; +Cc: open list:SOFTWARE RAID (Multiple Disks) SUPPORT
In-Reply-To: <ba3c9ca4-df07-8174-cf0e-bc1a2a2d7b82@suse.de>
On Thu, Apr 20, 2017 at 10:57:26PM +0800, Coly Li wrote:
> Hi MD developers,
>
> I just propose a microconference proposal to Linux Plumber Conf 2017,
> the topic is "MD raid general discussion".
>
> The proposal wiki page can be found here:
> http://wiki.linuxplumbersconf.org/2017:md_raid_general_discussion
>
> Please help to add more information to this wiki page (topics,
> attendees, and other information). I do hope we can meet during LPC2017
> and have face to face conversation, that will be great :-)
Awesome, thanks Coly! I think I'll be there.
Thanks,
Shaohua
^ permalink raw reply
* Re: [PATCH] drivers, md: convert stripe_head.count from atomic_t to refcount_t
From: Shaohua Li @ 2017-04-20 16:37 UTC (permalink / raw)
To: Reshetova, Elena
Cc: linux-kernel@vger.kernel.org, linux-raid@vger.kernel.org,
Hans Liljestrand, Kees Cook, David Windsor
In-Reply-To: <2236FBA76BA1254E88B949DDB74E612B41C8D774@IRSMSX102.ger.corp.intel.com>
On Thu, Apr 20, 2017 at 09:15:00AM +0000, Reshetova, Elena wrote:
> Just a ping on this updated patch. Did you find any new issues or could you please apply it?
sorry, for the dalay. I'm in vacation. Since the stripe_head.count could be 0
(and it's not freed), I assume it should stay as atomic_t.
Thanks,
Shaohua
> Best Regards,
> Elena.
>
> > refcount_t type and corresponding API should be
> > used instead of atomic_t when the variable is used as
> > a reference counter. This allows to avoid accidental
> > refcounter overflows that might lead to use-after-free
> > situations.
> >
> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > Signed-off-by: David Windsor <dwindsor@gmail.com>
> > ---
> > drivers/md/raid5-cache.c | 8 +++---
> > drivers/md/raid5.c | 68 +++++++++++++++++++++++++-----------------------
> > drivers/md/raid5.h | 3 ++-
> > 3 files changed, 41 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
> > index 3f307be..6c05e12 100644
> > --- a/drivers/md/raid5-cache.c
> > +++ b/drivers/md/raid5-cache.c
> > @@ -979,7 +979,7 @@ int r5l_write_stripe(struct r5l_log *log, struct stripe_head
> > *sh)
> > * don't delay.
> > */
> > clear_bit(STRIPE_DELAYED, &sh->state);
> > - atomic_inc(&sh->count);
> > + refcount_inc(&sh->count);
> >
> > mutex_lock(&log->io_mutex);
> > /* meta + data */
> > @@ -1321,7 +1321,7 @@ static void r5c_flush_stripe(struct r5conf *conf, struct
> > stripe_head *sh)
> > assert_spin_locked(&conf->device_lock);
> >
> > list_del_init(&sh->lru);
> > - atomic_inc(&sh->count);
> > + refcount_inc(&sh->count);
> >
> > set_bit(STRIPE_HANDLE, &sh->state);
> > atomic_inc(&conf->active_stripes);
> > @@ -1424,7 +1424,7 @@ static void r5c_do_reclaim(struct r5conf *conf)
> > */
> > if (!list_empty(&sh->lru) &&
> > !test_bit(STRIPE_HANDLE, &sh-
> > >state) &&
> > - atomic_read(&sh->count) == 0) {
> > + refcount_read(&sh->count) == 0) {
> > r5c_flush_stripe(conf,
> > sh);
> > if (count++ >=
> > R5C_RECLAIM_STRIPE_GROUP)
> > break;
> > @@ -2650,7 +2650,7 @@ r5c_cache_data(struct r5l_log *log, struct stripe_head
> > *sh,
> > * don't delay.
> > */
> > clear_bit(STRIPE_DELAYED, &sh->state);
> > - atomic_inc(&sh->count);
> > + refcount_inc(&sh->count);
> >
> > mutex_lock(&log->io_mutex);
> > /* meta + data */
> > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> > index 2ce23b0..7e3913a 100644
> > --- a/drivers/md/raid5.c
> > +++ b/drivers/md/raid5.c
> > @@ -296,7 +296,7 @@ static void do_release_stripe(struct r5conf *conf, struct
> > stripe_head *sh,
> > static void __release_stripe(struct r5conf *conf, struct stripe_head *sh,
> > struct list_head
> > *temp_inactive_list)
> > {
> > - if (atomic_dec_and_test(&sh->count))
> > + if (refcount_dec_and_test(&sh->count))
> > do_release_stripe(conf, sh, temp_inactive_list);
> > }
> >
> > @@ -388,7 +388,7 @@ void raid5_release_stripe(struct stripe_head *sh)
> >
> > /* Avoid release_list until the last reference.
> > */
> > - if (atomic_add_unless(&sh->count, -1, 1))
> > + if (refcount_dec_not_one(&sh->count))
> > return;
> >
> > if (unlikely(!conf->mddev->thread) ||
> > @@ -401,7 +401,7 @@ void raid5_release_stripe(struct stripe_head *sh)
> > slow_path:
> > local_irq_save(flags);
> > /* we are ok here if STRIPE_ON_RELEASE_LIST is set or not */
> > - if (atomic_dec_and_lock(&sh->count, &conf->device_lock)) {
> > + if (refcount_dec_and_lock(&sh->count, &conf->device_lock)) {
> > INIT_LIST_HEAD(&list);
> > hash = sh->hash_lock_index;
> > do_release_stripe(conf, sh, &list);
> > @@ -491,7 +491,7 @@ static void init_stripe(struct stripe_head *sh, sector_t
> > sector, int previous)
> > struct r5conf *conf = sh->raid_conf;
> > int i, seq;
> >
> > - BUG_ON(atomic_read(&sh->count) != 0);
> > + BUG_ON(refcount_read(&sh->count) != 0);
> > BUG_ON(test_bit(STRIPE_HANDLE, &sh->state));
> > BUG_ON(stripe_operations_active(sh));
> > BUG_ON(sh->batch_head);
> > @@ -668,11 +668,11 @@ raid5_get_active_stripe(struct r5conf *conf, sector_t
> > sector,
> > &conf-
> > >cache_state);
> > } else {
> > init_stripe(sh, sector,
> > previous);
> > - atomic_inc(&sh-
> > >count);
> > + refcount_inc(&sh-
> > >count);
> > }
> > - } else if (!atomic_inc_not_zero(&sh->count)) {
> > + } else if (!refcount_inc_not_zero(&sh->count)) {
> > spin_lock(&conf->device_lock);
> > - if (!atomic_read(&sh->count)) {
> > + if (!refcount_read(&sh->count)) {
> > if
> > (!test_bit(STRIPE_HANDLE, &sh->state))
> >
> > atomic_inc(&conf->active_stripes);
> >
> > BUG_ON(list_empty(&sh->lru) &&
> > @@ -688,7 +688,7 @@ raid5_get_active_stripe(struct r5conf *conf, sector_t
> > sector,
> > sh-
> > >group = NULL;
> > }
> > }
> > - atomic_inc(&sh->count);
> > + refcount_inc(&sh->count);
> > spin_unlock(&conf->device_lock);
> > }
> > } while (sh == NULL);
> > @@ -752,9 +752,9 @@ static void stripe_add_to_batch_list(struct r5conf *conf,
> > struct stripe_head *sh
> > hash = stripe_hash_locks_hash(head_sector);
> > spin_lock_irq(conf->hash_locks + hash);
> > head = __find_stripe(conf, head_sector, conf->generation);
> > - if (head && !atomic_inc_not_zero(&head->count)) {
> > + if (head && !refcount_inc_not_zero(&head->count)) {
> > spin_lock(&conf->device_lock);
> > - if (!atomic_read(&head->count)) {
> > + if (!refcount_read(&head->count)) {
> > if (!test_bit(STRIPE_HANDLE, &head-
> > >state))
> > atomic_inc(&conf-
> > >active_stripes);
> > BUG_ON(list_empty(&head->lru) &&
> > @@ -770,7 +770,7 @@ static void stripe_add_to_batch_list(struct r5conf *conf,
> > struct stripe_head *sh
> > head->group = NULL;
> > }
> > }
> > - atomic_inc(&head->count);
> > + refcount_inc(&head->count);
> > spin_unlock(&conf->device_lock);
> > }
> > spin_unlock_irq(conf->hash_locks + hash);
> > @@ -833,7 +833,7 @@ static void stripe_add_to_batch_list(struct r5conf *conf,
> > struct stripe_head *sh
> > sh->batch_head->bm_seq = seq;
> > }
> >
> > - atomic_inc(&sh->count);
> > + refcount_inc(&sh->count);
> > unlock_out:
> > unlock_two_stripes(head, sh);
> > out:
> > @@ -1036,9 +1036,9 @@ static void ops_run_io(struct stripe_head *sh, struct
> > stripe_head_state *s)
> > pr_debug("%s: for %llu schedule op
> > %d on disc %d\n",
> > __func__, (unsigned
> > long long)sh->sector,
> > bi->bi_opf, i);
> > - atomic_inc(&sh->count);
> > + refcount_inc(&sh->count);
> > if (sh != head_sh)
> > - atomic_inc(&head_sh-
> > >count);
> > +
> > refcount_inc(&head_sh->count);
> > if (use_new_offset(conf, sh))
> > bi->bi_iter.bi_sector =
> > (sh->sector
> >
> > + rdev->new_data_offset);
> > @@ -1097,9 +1097,9 @@ static void ops_run_io(struct stripe_head *sh, struct
> > stripe_head_state *s)
> > "replacement disc
> > %d\n",
> > __func__, (unsigned
> > long long)sh->sector,
> > rbi->bi_opf, i);
> > - atomic_inc(&sh->count);
> > + refcount_inc(&sh->count);
> > if (sh != head_sh)
> > - atomic_inc(&head_sh-
> > >count);
> > +
> > refcount_inc(&head_sh->count);
> > if (use_new_offset(conf, sh))
> > rbi->bi_iter.bi_sector =
> > (sh->sector
> >
> > + rrdev->new_data_offset);
> > @@ -1275,7 +1275,7 @@ static void ops_run_biofill(struct stripe_head *sh)
> > }
> > }
> >
> > - atomic_inc(&sh->count);
> > + refcount_inc(&sh->count);
> > init_async_submit(&submit, ASYNC_TX_ACK, tx,
> > ops_complete_biofill, sh, NULL);
> > async_trigger_callback(&submit);
> > }
> > @@ -1353,7 +1353,7 @@ ops_run_compute5(struct stripe_head *sh, struct
> > raid5_percpu *percpu)
> > if (i != target)
> > xor_srcs[count++] = sh->dev[i].page;
> >
> > - atomic_inc(&sh->count);
> > + refcount_inc(&sh->count);
> >
> > init_async_submit(&submit,
> > ASYNC_TX_FENCE|ASYNC_TX_XOR_ZERO_DST, NULL,
> > ops_complete_compute, sh,
> > to_addr_conv(sh, percpu, 0));
> > @@ -1441,7 +1441,7 @@ ops_run_compute6_1(struct stripe_head *sh, struct
> > raid5_percpu *percpu)
> > BUG_ON(!test_bit(R5_Wantcompute, &tgt->flags));
> > dest = tgt->page;
> >
> > - atomic_inc(&sh->count);
> > + refcount_inc(&sh->count);
> >
> > if (target == qd_idx) {
> > count = set_syndrome_sources(blocks, sh,
> > SYNDROME_SRC_ALL);
> > @@ -1516,7 +1516,7 @@ ops_run_compute6_2(struct stripe_head *sh, struct
> > raid5_percpu *percpu)
> > pr_debug("%s: stripe: %llu faila: %d failb: %d\n",
> > __func__, (unsigned long long)sh->sector, faila,
> > failb);
> >
> > - atomic_inc(&sh->count);
> > + refcount_inc(&sh->count);
> >
> > if (failb == syndrome_disks+1) {
> > /* Q disk is one of the missing disks */
> > @@ -1784,7 +1784,7 @@ ops_run_reconstruct5(struct stripe_head *sh, struct
> > raid5_percpu *percpu,
> > break;
> > }
> > if (i >= sh->disks) {
> > - atomic_inc(&sh->count);
> > + refcount_inc(&sh->count);
> > set_bit(R5_Discard, &sh->dev[pd_idx].flags);
> > ops_complete_reconstruct(sh);
> > return;
> > @@ -1825,7 +1825,7 @@ ops_run_reconstruct5(struct stripe_head *sh, struct
> > raid5_percpu *percpu,
> > flags = ASYNC_TX_ACK |
> > (prexor ? ASYNC_TX_XOR_DROP_DST
> > : ASYNC_TX_XOR_ZERO_DST);
> >
> > - atomic_inc(&head_sh->count);
> > + refcount_inc(&head_sh->count);
> > init_async_submit(&submit, flags, tx,
> > ops_complete_reconstruct, head_sh,
> > to_addr_conv(sh,
> > percpu, j));
> > } else {
> > @@ -1867,7 +1867,7 @@ ops_run_reconstruct6(struct stripe_head *sh, struct
> > raid5_percpu *percpu,
> > break;
> > }
> > if (i >= sh->disks) {
> > - atomic_inc(&sh->count);
> > + refcount_inc(&sh->count);
> > set_bit(R5_Discard, &sh->dev[sh->pd_idx].flags);
> > set_bit(R5_Discard, &sh->dev[sh->qd_idx].flags);
> > ops_complete_reconstruct(sh);
> > @@ -1891,7 +1891,7 @@ ops_run_reconstruct6(struct stripe_head *sh, struct
> > raid5_percpu *percpu,
> > struct stripe_head,
> > batch_list) == head_sh;
> >
> > if (last_stripe) {
> > - atomic_inc(&head_sh->count);
> > + refcount_inc(&head_sh->count);
> > init_async_submit(&submit, txflags, tx,
> > ops_complete_reconstruct,
> > head_sh,
> > to_addr_conv(sh, percpu, j));
> > } else
> > @@ -1948,7 +1948,7 @@ static void ops_run_check_p(struct stripe_head *sh,
> > struct raid5_percpu *percpu)
> > tx = async_xor_val(xor_dest, xor_srcs, 0, count, STRIPE_SIZE,
> > &sh->ops.zero_sum_result,
> > &submit);
> >
> > - atomic_inc(&sh->count);
> > + refcount_inc(&sh->count);
> > init_async_submit(&submit, ASYNC_TX_ACK, tx,
> > ops_complete_check, sh, NULL);
> > tx = async_trigger_callback(&submit);
> > }
> > @@ -1967,7 +1967,7 @@ static void ops_run_check_pq(struct stripe_head *sh,
> > struct raid5_percpu *percpu
> > if (!checkp)
> > srcs[count] = NULL;
> >
> > - atomic_inc(&sh->count);
> > + refcount_inc(&sh->count);
> > init_async_submit(&submit, ASYNC_TX_ACK, NULL,
> > ops_complete_check,
> > sh, to_addr_conv(sh, percpu, 0));
> > async_syndrome_val(srcs, 0, count+2, STRIPE_SIZE,
> > @@ -2057,7 +2057,7 @@ static struct stripe_head *alloc_stripe(struct
> > kmem_cache *sc, gfp_t gfp,
> > INIT_LIST_HEAD(&sh->lru);
> > INIT_LIST_HEAD(&sh->r5c);
> > INIT_LIST_HEAD(&sh->log_list);
> > - atomic_set(&sh->count, 1);
> > + refcount_set(&sh->count, 1);
> > sh->log_start = MaxSector;
> > for (i = 0; i < disks; i++) {
> > struct r5dev *dev = &sh->dev[i];
> > @@ -2354,7 +2354,7 @@ static int drop_one_stripe(struct r5conf *conf)
> > spin_unlock_irq(conf->hash_locks + hash);
> > if (!sh)
> > return 0;
> > - BUG_ON(atomic_read(&sh->count));
> > + BUG_ON(refcount_read(&sh->count));
> > shrink_buffers(sh);
> > kmem_cache_free(conf->slab_cache, sh);
> > atomic_dec(&conf->active_stripes);
> > @@ -2386,7 +2386,7 @@ static void raid5_end_read_request(struct bio * bi)
> > break;
> >
> > pr_debug("end_read_request %llu/%d, count: %d, error %d.\n",
> > - (unsigned long long)sh->sector, i, atomic_read(&sh-
> > >count),
> > + (unsigned long long)sh->sector, i,
> > refcount_read(&sh->count),
> > bi->bi_error);
> > if (i == disks) {
> > bio_reset(bi);
> > @@ -2523,7 +2523,7 @@ static void raid5_end_write_request(struct bio *bi)
> > }
> > }
> > pr_debug("end_write_request %llu/%d, count %d, error: %d.\n",
> > - (unsigned long long)sh->sector, i, atomic_read(&sh-
> > >count),
> > + (unsigned long long)sh->sector, i,
> > refcount_read(&sh->count),
> > bi->bi_error);
> > if (i == disks) {
> > bio_reset(bi);
> > @@ -4545,7 +4545,7 @@ static void handle_stripe(struct stripe_head *sh)
> > pr_debug("handling stripe %llu, state=%#lx cnt=%d, "
> > "pd_idx=%d, qd_idx=%d\n, check:%d,
> > reconstruct:%d\n",
> > (unsigned long long)sh->sector, sh->state,
> > - atomic_read(&sh->count), sh->pd_idx, sh->qd_idx,
> > + refcount_read(&sh->count), sh->pd_idx, sh->qd_idx,
> > sh->check_state, sh->reconstruct_state);
> >
> > analyse_stripe(sh, &s);
> > @@ -4924,7 +4924,7 @@ static void activate_bit_delay(struct r5conf *conf,
> > struct stripe_head *sh = list_entry(head.next, struct
> > stripe_head, lru);
> > int hash;
> > list_del_init(&sh->lru);
> > - atomic_inc(&sh->count);
> > + refcount_inc(&sh->count);
> > hash = sh->hash_lock_index;
> > __release_stripe(conf, sh,
> > &temp_inactive_list[hash]);
> > }
> > @@ -5240,7 +5240,9 @@ static struct stripe_head *__get_priority_stripe(struct
> > r5conf *conf, int group)
> > sh->group = NULL;
> > }
> > list_del_init(&sh->lru);
> > - BUG_ON(atomic_inc_return(&sh->count) != 1);
> > + BUG_ON(refcount_read(&sh->count) != 0);
> > + refcount_set(&sh->count, 1);
> > +
> > return sh;
> > }
> >
> > diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> > index 4bb27b9..a1ed351 100644
> > --- a/drivers/md/raid5.h
> > +++ b/drivers/md/raid5.h
> > @@ -3,6 +3,7 @@
> >
> > #include <linux/raid/xor.h>
> > #include <linux/dmaengine.h>
> > +#include <linux/refcount.h>
> >
> > /*
> > *
> > @@ -207,7 +208,7 @@ struct stripe_head {
> > short ddf_layout;/* use DDF
> > ordering to calculate Q */
> > short hash_lock_index;
> > unsigned long state; /* state
> > flags */
> > - atomic_t count; /* nr of active
> > thread/requests */
> > + refcount_t count; /* nr of active
> > thread/requests */
> > int bm_seq; /*
> > sequence number for bitmap flushes */
> > int disks;
> > /* disks in stripe */
> > int overwrite_disks; /*
> > total overwrite disks in stripe,
> > --
> > 2.7.4
>
^ permalink raw reply
* Re: [PATCH] uapi: fix linux/raid/md_p.h userspace compilation error
From: Shaohua Li @ 2017-04-20 16:41 UTC (permalink / raw)
To: Artur Paszkiewicz; +Cc: shli, linux-raid, gthelen, ncroxon
In-Reply-To: <20170419084806.28405-1-artur.paszkiewicz@intel.com>
On Wed, Apr 19, 2017 at 10:48:06AM +0200, Artur Paszkiewicz wrote:
> Use __le32 and __le64 instead of u32 and u64.
>
> This fixes klibc build error:
> In file included from /klibc/usr/klibc/../include/sys/md.h:30:0,
> from /klibc/usr/kinit/do_mounts_md.c:19:
> /linux-next/usr/include/linux/raid/md_p.h:414:51: error: 'u32' undeclared here (not in a function)
> (PPL_HEADER_SIZE - PPL_HDR_RESERVED - 4 * sizeof(u32) - sizeof(u64))
applied, thanks!
> Reported-by: Greg Thelen <gthelen@google.com>
> Reported-by: Nigel Croxon <ncroxon@redhat.com>
> Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
> ---
> include/uapi/linux/raid/md_p.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/raid/md_p.h b/include/uapi/linux/raid/md_p.h
> index d9a1ead867b9..d500bd224979 100644
> --- a/include/uapi/linux/raid/md_p.h
> +++ b/include/uapi/linux/raid/md_p.h
> @@ -411,7 +411,7 @@ struct ppl_header_entry {
> #define PPL_HEADER_SIZE 4096
> #define PPL_HDR_RESERVED 512
> #define PPL_HDR_ENTRY_SPACE \
> - (PPL_HEADER_SIZE - PPL_HDR_RESERVED - 4 * sizeof(u32) - sizeof(u64))
> + (PPL_HEADER_SIZE - PPL_HDR_RESERVED - 4 * sizeof(__le32) - sizeof(__le64))
> #define PPL_HDR_MAX_ENTRIES \
> (PPL_HDR_ENTRY_SPACE / sizeof(struct ppl_header_entry))
>
> --
> 2.11.0
>
> --
> 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 1/4] mdadm/util:integrate stat operations into one utility
From: Jes Sorensen @ 2017-04-20 16:42 UTC (permalink / raw)
To: Liu Zhilong; +Cc: linux-raid
In-Reply-To: <db0ed456-77cb-2188-24f7-a970a5a6be80@suse.com>
On 04/17/2017 03:08 AM, Liu Zhilong wrote:
>
>
> On 04/14/2017 11:20 PM, Jes Sorensen wrote:
>> On 04/14/2017 06:14 AM, Liu Zhilong wrote:
>>>
>>>
>>> On 04/05/2017 11:42 PM, jes.sorensen@gmail.com wrote:
>>>> Zhilong Liu <zlliu@suse.com> writes:
>> [snip]
>>>> For a function like this, lets name it better md_is_blkdev()
>>>
>>> How about return the devid after checking? Because always need the
>>> stb.st_rdev to parse
>>> the major and minor number. Although "util.c" has "devnm2devid" to
>>> gather the devid via
>>> to devnm, but it's convenient to return devid when check the blkdev with
>>> absolute path.
>>> would you mind the function like this?
>>>
>>> // returns dev-id when success, return 0 when failure
>>> dev_t stat_md_is_blkdev(char *dev)
>>> {
>>> struct stat stb;
>>>
>>> if (stat(dev, &stb) != 0) {
>>> pr_err("stat failed for %s: %s\n", dev, strerror(errno));
>>> return 0;
>>> }
>>> if ((S_IFMT & stb.st_mode) != S_IFBLK) {
>>> pr_err("%s is not a block device.\n", dev);
>>> return 0;
>>> }
>>> return stb.st_rdev;
>>> }
>>
>> I am generally wary of too many smart handlers, but I think it makes
>> some sense here to de-duplicate the repeated code.
>>
>> That said, your function would ditch the error information, and the
>> caller wouldn't know why it failed. If you made it more like this and
>> return the error code, plus stick the major/minor number into rdev, if
>> one is provided:
>>
>> int stat_md_is_blkdev(char *devname, *dev_t rdev)
>> {
>> }
>>
>
> I want to integrate two situations into one function.
> 1. only check the 'dev'(such as /dev/loop2) whether or not is block device.
> 2. optionally return the rdev-id after checking the block device.
> this sample "int stat_md_is_blkdev(char *devname, *dev_t rdev)" is smart
> for the situation 2. but for situation 1, the second parameter is a
> little waste.
>
> how about like this?
> dev_t stat_md_is_blkdev(char *devname, bool require_rdev)
> {
> struct stat stb;
>
> if (stat(devname, &stb) != 0) {
> printf("stat failed for %s.\n", devname);
> return 1;
> }
> if ((S_IFMT & stb.st_mode) != S_IFBLK) {
> printf("%s is not a block device.\n", devname);
> return 1;
> }
> if (require_rdev)
> return stb.st_rdev;
> return 0;
> }
>
> the caller would be like this,
> situation 1:
> char dev[20] = "/dev/loop2";
> if (stat_md_is_blkdev(dev, false))
> return 1;
>
> situation 2:
> dev_t st_rdev;
> char dev[20] = "/dev/loop2";
> st_rdev = stat_md_is_blkdev(dev, true);
> if (st_rdev == 1)
> return 1;
>
No, this is really ugly - if you have a function that exhibits two
different behaviors based on a boolean flag, you should have two
versions. Parse a pointer for an optional return of dev_t and return the
error value.
Jes
^ permalink raw reply
* Re: [PATCH] md/raid10: wait up frozen array in handle_write_completed
From: Shaohua Li @ 2017-04-20 16:55 UTC (permalink / raw)
To: Guoqing Jiang; +Cc: linux-raid, neilb
In-Reply-To: <20170417091105.27180-1-gqjiang@suse.com>
On Mon, Apr 17, 2017 at 05:11:05PM +0800, Guoqing Jiang wrote:
> Since nr_queued is changed, we need to call wake_up here
> if the array is already frozen and waiting for condition
> "nr_pending == nr_queued + extra" to be true.
>
> And commit 824e47daddbf ("RAID1: avoid unnecessary spin
> locks in I/O barrier code") which has already added the
> wake_up for raid1.
thanks, applied!
> Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
> ---
> drivers/md/raid10.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 4167091..acb3f46 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -2704,6 +2704,11 @@ static void handle_write_completed(struct r10conf *conf, struct r10bio *r10_bio)
> list_add(&r10_bio->retry_list, &conf->bio_end_io_list);
> conf->nr_queued++;
> spin_unlock_irq(&conf->device_lock);
> + /*
> + * In case freeze_array() is waiting for condition
> + * nr_pending == nr_queued + extra to be true.
> + */
> + wake_up(&conf->wait_barrier);
> md_wakeup_thread(conf->mddev->thread);
> } else {
> if (test_bit(R10BIO_WriteError,
> --
> 2.10.0
>
^ permalink raw reply
* Re: [mdadm PATCH 1/4] Grow_continue_command: ensure 'content' is properly initialised.
From: Jes Sorensen @ 2017-04-20 16:56 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid
In-Reply-To: <149265600552.31004.6577040107424525064.stgit@noble>
On 04/19/2017 10:40 PM, NeilBrown wrote:
> Grow_continue_command() call verify_reshape_position(), which assumes
> that info->sys_name is initialised.
> 'info' in verify_reshape_position() is 'content' in Grow_continue_command().
>
> In the st->ss->external != 0 branch of that function, sysfs_init() is called
> to initialize content->sys_name.
> In the st->ss->external == 0 branch, ->sys_name is not initialized so
> verify_reshape_position() will not do the right thing.
>
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
> Grow.c | 1 +
> 1 file changed, 1 insertion(+)
Applied!
Thanks,
Jes
^ permalink raw reply
* Re: [mdadm PATCH 2/4] systemd/mdadm-last-resort: use ConditionPathExists instead of Conflicts
From: Jes Sorensen @ 2017-04-20 16:57 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid
In-Reply-To: <149265600571.31004.1241857541944212398.stgit@noble>
On 04/19/2017 10:40 PM, NeilBrown wrote:
> Commit cec72c071bbe ("systemd/mdadm-last-resort: add Conflicts to .service file.")
>
> added a 'Conflicts' directive to the mdadm-last-resort@.service file in
> the hope that this would make sure the service didn't run after the device
> was active, even if the timer managed to get started, which is possible in
> race conditions.
>
> This seemed to work is testing, but it isn't clear why, and it is known
> to cause problems.
> If systemd happens to know that the mentioned device is a dependency of a
> mount point, the Conflicts can unmount that mountpoint, which is certainly
> not wanted.
>
> So remove the "Conflicts" and instead use
> ConditionPathExists=!/sys/devices/virtual/block/%i/md/sync_action
>
> The "sync_action" file exists for any array which requires last-resort
> handling, and only appears when the array is activated. So it is safe
> to rely on it to determine if the last-resort is really needed.
>
> Fixes: cec72c071bbe ("systemd/mdadm-last-resort: add Conflicts to .service file.")
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
> systemd/mdadm-last-resort@.service | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Ohhh obscure systemd conditionals, my favorite thing in the whole World.....
Applied!
Thanks,
Jes
^ permalink raw reply
* Re: [mdadm PATCH 3/4] Detail: ensure --export names are acceptable as shell variables.
From: Jes Sorensen @ 2017-04-20 16:59 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid
In-Reply-To: <149265600586.31004.10549541527467125631.stgit@noble>
On 04/19/2017 10:40 PM, NeilBrown wrote:
> If an array contains a device which has a name that
> contains something other than alphnumerics and underscores,
> then some values reported by "mdadm --detail --export" will
> not be valid as variable assignment of the shell.
> This particularly affects dm devices.
> e.g.
> MD_DEVICE_dm-4_ROLE=1
> MD_DEVICE_dm-4_DEV=/dev/dm-4
>
> As it is particularly useful to be able to work with these
> in a shell script, and as the precise name is not important,
> change all non-alphanumerics to '_'.
>
> MD_DEVICE_dm_4_ROLE=1
> MD_DEVICE_dm_4_DEV=/dev/dm-4
>
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
> Detail.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
Applied!
Thanks,
Jes
^ permalink raw reply
* Re: [mdadm PATCH 4/4] Create: tell udev device is not ready when first created.
From: Jes Sorensen @ 2017-04-20 17:29 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid
In-Reply-To: <149265600601.31004.323865505557190368.stgit@noble>
On 04/19/2017 10:40 PM, NeilBrown wrote:
> When an array is created the content is not initializes,
> so it could have remnants of an old filesystem or md array
> etc on it.
> udev will see this and might try to activate it, which is almost
> certainly not what is wanted.
>
> So create a temporary udev rules files to set ENV{SYSTEMD_READY}="0"
> while the creation event is processed. This is fairly uniformly
> used to suppress actions based on the contents of the device.
>
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
> Create.c | 9 +++++++++
> lib.c | 22 ++++++++++++++++++++++
> mdadm.h | 2 ++
> 3 files changed, 33 insertions(+)
Neil,
I have heard of this problem before, but I have some concerns about the
solution. First of all, /run/udev/rules.d/ isn't a universal directory.
At least Fedora doesn't have it, so we need to take that into account. I
wasn't aware it was possible to create temporary udev rules like this.
Second, isn't this going to be racey if you have multiple arrays
running? I am wondering if we cannot find a solution that relies on a
permanently installed udev rule that we enable/disable with systemctl
and use the device name as an argument?
Thoughts?
Jes
^ permalink raw reply
* Re: [md PATCH] md: handle read-only member devices better.
From: Shaohua Li @ 2017-04-20 20:24 UTC (permalink / raw)
To: NeilBrown; +Cc: Linux-RAID, Nanda Kishore Chinnaram
In-Reply-To: <877f2muvmc.fsf@notabene.neil.brown.name>
On Sat, Apr 15, 2017 at 02:45:31PM +1000, Neil Brown wrote:
> On Wed, Apr 12 2017, Shaohua Li wrote:
>
> > On Thu, Apr 13, 2017 at 08:53:48AM +1000, Neil Brown wrote:
> >>
> >> 1/ If an array has any read-only devices when it is started,
> >> the array itself must be read-only
> >> 2/ A read-only device cannot be added to an array after it is
> >> started.
> >> 3/ Setting an array to read-write should not succeed
> >> if any member devices are read-only
> >
> > Didn't get these. We call md_import_device() first to open under layer disk. We
> > always use FMOD_READ|FMOD_WRITE to open the disk. So if the disk is ro,
> > md_import_device should fail, we don't add the disk to the array. Why would we
> > have such issues?
> >
>
> Because life isn't always as simple as we might like it to be. :-(
>
> md_import_device() calls lock_rdev() which calls blkdev_get_by_dev().
>
> blkdev_get_by_dev() doesn't pay much attention to the mode, nor does
> blkdev_get() which it calls. The main place where FMODE_WRITE could be
> rejected on a read-only device is in the device's 'open()' function. A
> few open functions do check for read-only, but it isn't at all
> consistent.
> scsi/sd.c does, block/loop.c doesn't, nor does nvme. Most drivers seem
> to ignore the mode.
>
> blkdev_get_by_path() has
>
> if ((mode & FMODE_WRITE) && bdev_read_only(bdev)) {
> blkdev_put(bdev, mode);
> return ERR_PTR(-EACCES);
> }
>
> so when you open a device by path name you always get this check, but
> not when you open a device by device-number like md does.
> It is worth having a look at
> Commit: e51900f7d38c ("block: revert block_dev read-only check")
> from 2011. The bdev_read_only() check was in blkdev_get() for a while,
> but it was moved out because doing that broke md and dm and others.
>
> So at present, callers of blkdev_get_by_dev() need to do their own
> bdev_read_only() tests before writing.
> We could discuss where in md.c is the best place to put them, but unless
> you want to take on a largish project to 'fix' (or audit) all callers of
> blkdev_get_by_dev(), they need to go in md somewhere.
It's unfortunate we need the hack, but anyway I applied this. I'm wonding how
useful a ro array is. At least ro array with .sync_request is dangerous because
we could read inconsistent data.
Thanks,
Shaohua
^ permalink raw reply
* Re: [mdadm PATCH 4/4] Create: tell udev device is not ready when first created.
From: NeilBrown @ 2017-04-20 21:35 UTC (permalink / raw)
To: Jes Sorensen; +Cc: linux-raid
In-Reply-To: <dedd27cd-9c2e-f244-d8aa-7a074679d3c6@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 8300 bytes --]
On Thu, Apr 20 2017, Jes Sorensen wrote:
> On 04/19/2017 10:40 PM, NeilBrown wrote:
>> When an array is created the content is not initializes,
>> so it could have remnants of an old filesystem or md array
>> etc on it.
>> udev will see this and might try to activate it, which is almost
>> certainly not what is wanted.
>>
>> So create a temporary udev rules files to set ENV{SYSTEMD_READY}="0"
>> while the creation event is processed. This is fairly uniformly
>> used to suppress actions based on the contents of the device.
>>
>> Signed-off-by: NeilBrown <neilb@suse.com>
>> ---
>> Create.c | 9 +++++++++
>> lib.c | 22 ++++++++++++++++++++++
>> mdadm.h | 2 ++
>> 3 files changed, 33 insertions(+)
>
> Neil,
>
> I have heard of this problem before, but I have some concerns about the
> solution. First of all, /run/udev/rules.d/ isn't a universal directory.
> At least Fedora doesn't have it, so we need to take that into account. I
> wasn't aware it was possible to create temporary udev rules like this.
Hmmm.. Debian doesn't have it either. However I suspect that udev will
still try to read from the directory. The man page explicitly lists it:
RULES FILES
The udev rules are read from the files located in the system rules
directory /usr/lib/udev/rules.d, the volatile runtime directory
/run/udev/rules.d and the local administration directory
/etc/udev/rules.d. ....
So possibly mdadm would need to mkdir("/run/udev/rules.d", 0755) first.
The man page uses the term "volatile" rather than "temporary", but this
possibility does seem to be part of the design of udev.
>
> Second, isn't this going to be racey if you have multiple arrays
> running? I am wondering if we cannot find a solution that relies on a
> permanently installed udev rule that we enable/disable with systemctl
> and use the device name as an argument?
There would only be a problematic race of an array as being created at
the same time that another array is being assembled. Is that at all
likely? They tend to happen at different parts of the usage cycle... I
guess we shouldn't assume though.
I admit that blocking *all* arrays was a short cut. We need to create
the udev rule before the new device is first activated, and keep it in
existence until after the array has been started and the fd on /dev/mdXX
has been closed - and then a bit more because udev doesn't respond
instantly.
In some cases we don't know the name of the md device until
create_mddev() chooses on with find_free_devnum().
So if we want to block udev from handling a device, we need to put the
block into effect (e.g. create the rules.d file) in mddev_create() just
before the call to open_dev_excl().
If we wanted an more permanent udev rule, we would need to record the
devices that should be ignored in the filesystem somewhere else.
Maybe in /run/mdadm.
e.g.
KERNEL=="md*", TEST="/run/mdadm/creating-$kernel", ENV{SYSTEMD_READY}="0"
Then we could have something like the following (untested) in mdadm.
Does that seem more suitable?
Thanks,
NeilBrown
---
Assemble.c | 2 +-
Build.c | 2 +-
Create.c | 9 ++++++++-
Incremental.c | 4 ++--
lib.c | 29 +++++++++++++++++++++++++++++
mdadm.h | 4 +++-
mdopen.c | 4 +++-
7 files changed, 47 insertions(+), 7 deletions(-)
diff --git a/Assemble.c b/Assemble.c
index b8285239354b..309841432ff5 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -1459,7 +1459,7 @@ try_again:
name = strchr(name, ':')+1;
mdfd = create_mddev(mddev, name, ident->autof, trustworthy,
- chosen_name);
+ chosen_name, 0);
}
if (mdfd < 0) {
st->ss->free_super(st);
diff --git a/Build.c b/Build.c
index 11ba12f4ae7d..665d9067b8d6 100644
--- a/Build.c
+++ b/Build.c
@@ -109,7 +109,7 @@ int Build(char *mddev, struct mddev_dev *devlist,
/* We need to create the device. It can have no name. */
map_lock(&map);
mdfd = create_mddev(mddev, NULL, c->autof, LOCAL,
- chosen_name);
+ chosen_name, 0);
if (mdfd < 0) {
map_unlock(&map);
return 1;
diff --git a/Create.c b/Create.c
index 6ca092449880..df1bc20c635b 100644
--- a/Create.c
+++ b/Create.c
@@ -605,7 +605,7 @@ int Create(struct supertype *st, char *mddev,
/* We need to create the device */
map_lock(&map);
- mdfd = create_mddev(mddev, name, c->autof, LOCAL, chosen_name);
+ mdfd = create_mddev(mddev, name, c->autof, LOCAL, chosen_name, 1);
if (mdfd < 0) {
map_unlock(&map);
return 1;
@@ -620,6 +620,7 @@ int Create(struct supertype *st, char *mddev,
chosen_name);
close(mdfd);
map_unlock(&map);
+ udev_unblock();
return 1;
}
mddev = chosen_name;
@@ -1053,9 +1054,15 @@ int Create(struct supertype *st, char *mddev,
pr_err("not starting array - not enough devices.\n");
}
close(mdfd);
+ /* Give udev a moment to process the Change event caused
+ * by the close.
+ */
+ usleep(100*1000);
+ udev_unblock();
return 0;
abort:
+ udev_unblock();
map_lock(&map);
abort_locked:
map_remove(&map, fd2devnm(mdfd));
diff --git a/Incremental.c b/Incremental.c
index 28f1f7734956..63ed4fa1a88d 100644
--- a/Incremental.c
+++ b/Incremental.c
@@ -321,7 +321,7 @@ int Incremental(struct mddev_dev *devlist, struct context *c,
/* Couldn't find an existing array, maybe make a new one */
mdfd = create_mddev(match ? match->devname : NULL,
- name_to_use, c->autof, trustworthy, chosen_name);
+ name_to_use, c->autof, trustworthy, chosen_name, 0);
if (mdfd < 0)
goto out_unlock;
@@ -1605,7 +1605,7 @@ static int Incremental_container(struct supertype *st, char *devname,
ra->name,
c->autof,
trustworthy,
- chosen_name);
+ chosen_name, 0);
}
if (only && (!mp || strcmp(mp->devnm, only) != 0))
continue;
diff --git a/lib.c b/lib.c
index b640634ef6f2..d8692cae813d 100644
--- a/lib.c
+++ b/lib.c
@@ -163,6 +163,35 @@ char *fd2devnm(int fd)
return NULL;
}
+/* When we create a new array, we don't want the content to
+ * be immediately examined by udev - it is probably meaningless.
+ * So create /run/udev/rules.d/01-mdadm-create.rules to tell udev
+ * that the device isn't ready.
+ */
+static char block_path[] = "/run/mdadm/creating-%s";
+static char *unblock_path = NULL;
+void udev_block(char *devnm)
+{
+ int fd;
+ char *path = NULL;
+
+ xasprintf(&path, block_path, devnm);
+ fd = open(path, O_CREAT|O_RDWR, 0600);
+ if (fd >= 0) {
+ close(fd);
+ unblock_path = path;
+ } else
+ free(path);
+}
+
+void udev_unblock(void)
+{
+ if (unblock_path)
+ unlink(unblock_path);
+ free(unblock_path);
+ unblock_path = NULL;
+}
+
/*
* convert a major/minor pair for a block device into a name in /dev, if possible.
* On the first call, walk /dev collecting name.
diff --git a/mdadm.h b/mdadm.h
index f1f643c794d4..187e60050068 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -1522,7 +1522,7 @@ extern char *get_md_name(char *devnm);
extern char DefaultConfFile[];
extern int create_mddev(char *dev, char *name, int autof, int trustworthy,
- char *chosen);
+ char *chosen, int block_udev);
/* values for 'trustworthy' */
#define LOCAL 1
#define LOCAL_ANY 10
@@ -1556,6 +1556,8 @@ extern char *stat2kname(struct stat *st);
extern char *fd2kname(int fd);
extern char *stat2devnm(struct stat *st);
extern char *fd2devnm(int fd);
+extern void udev_block(char *devnm);
+extern void udev_unblock(void);
extern int in_initrd(void);
diff --git a/mdopen.c b/mdopen.c
index 82b97fc90339..1dbcdcddb9f6 100644
--- a/mdopen.c
+++ b/mdopen.c
@@ -135,7 +135,7 @@ void make_parts(char *dev, int cnt)
*/
int create_mddev(char *dev, char *name, int autof, int trustworthy,
- char *chosen)
+ char *chosen, int block_udev)
{
int mdfd;
struct stat stb;
@@ -414,6 +414,8 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
make_parts(chosen, parts);
}
}
+ if (block_udev)
+ udev_block(devnm);
mdfd = open_dev_excl(devnm);
if (mdfd < 0)
pr_err("unexpected failure opening %s\n",
--
2.12.2
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply related
* Re: GET_ARRAY_INFO assumptions?
From: NeilBrown @ 2017-04-20 21:49 UTC (permalink / raw)
To: Jes Sorensen, Shaohua Li; +Cc: linux-raid
In-Reply-To: <59ca76c6-1f1f-4d9c-4eb9-d468131e0d55@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1230 bytes --]
On Thu, Apr 20 2017, Jes Sorensen wrote:
>
> I think I got it right this time and pushed it into git. It made things
> a lot prettier too IMHO :)
>
> In the process I also changed the behavior of
> sysfs_read(GET_ARRAY_STATE) as I really didn't like how it was copying
> in the string rather than parsing it.
>
> I am traveling at the moment and don't yet have my new raid test box
> setup back at the office, so my testing is limited. If I broke something
> badly, feel free to throw rotten tomatoes at me.
>
> Cheers,
> Jes
Looks good to me, except...
/*
* Beware map_name() uses strcmp() so active-idle must come before
* active, to be detected correctly.
*/
???
If map_name() used strncmp() you might need to be careful, but not with
strcmp.
Also:
int map_name(mapping_t *map, char *name)
{
while (map->name) {
if (strcmp(map->name, name)==0)
return map->num;
map++;
}
return map->num;
}
Both returns do the same thing... so there should (could) just be one.
is:
while (map->name && strcmp(map->name, name) != 0)
map++;
return map->num;
too terse??
I do like that you could use map_name to parse the array_state file.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 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