* [PATCH] Fix race between --create and --incremental @ 2014-04-09 15:14 Artur Paszkiewicz 2014-04-10 0:33 ` NeilBrown 0 siblings, 1 reply; 5+ messages in thread From: Artur Paszkiewicz @ 2014-04-09 15:14 UTC (permalink / raw) To: neilb; +Cc: linux-raid, Artur Paszkiewicz This modifies locking in Create to eliminate a situation where --incremental can assemble a device between write_init_super() and add_disk(), which causes Create to fail. It sporadically occurs e.g. when metadata is written on a device, causing an udev change event which triggers mdadm --incremental. Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com> --- Create.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Create.c b/Create.c index 692ff52..bac9181 100644 --- a/Create.c +++ b/Create.c @@ -833,6 +833,7 @@ int Create(struct supertype *st, char *mddev, infos = xmalloc(sizeof(*infos) * total_slots); enable_fds(total_slots); + map_lock(&map); for (pass=1; pass <=2 ; pass++) { struct mddev_dev *moved_disk = NULL; /* the disk that was moved out of the insert point */ @@ -886,7 +887,7 @@ int Create(struct supertype *st, char *mddev, pr_err("failed to open %s " "after earlier success - aborting\n", dv->devname); - goto abort; + goto abort_locked; } fstat(fd, &stb); inf->disk.major = major(stb.st_rdev); @@ -898,7 +899,7 @@ int Create(struct supertype *st, char *mddev, fd, dv->devname, dv->data_offset)) { ioctl(mdfd, STOP_ARRAY, NULL); - goto abort; + goto abort_locked; } st->ss->getinfo_super(st, inf, NULL); safe_mode_delay = inf->safe_mode_delay; @@ -924,7 +925,7 @@ int Create(struct supertype *st, char *mddev, pr_err("ADD_NEW_DISK for %s " "failed: %s\n", dv->devname, strerror(errno)); - goto abort; + goto abort_locked; } break; } @@ -941,7 +942,6 @@ int Create(struct supertype *st, char *mddev, * the subarray cursor such that ->getinfo_super once * again returns container info. */ - map_lock(&map); st->ss->getinfo_super(st, &info_new, NULL); if (st->ss->external && s->level != LEVEL_CONTAINER && !same_uuid(info_new.uuid, info.uuid, 0)) { @@ -966,12 +966,12 @@ int Create(struct supertype *st, char *mddev, info_new.uuid, path); free(path); } - map_unlock(&map); flush_metadata_updates(st); st->ss->free_super(st); } } + map_unlock(&map); free(infos); if (s->level == LEVEL_CONTAINER) { -- 1.8.4.5 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix race between --create and --incremental 2014-04-09 15:14 [PATCH] Fix race between --create and --incremental Artur Paszkiewicz @ 2014-04-10 0:33 ` NeilBrown 2014-04-11 10:08 ` Artur Paszkiewicz 0 siblings, 1 reply; 5+ messages in thread From: NeilBrown @ 2014-04-10 0:33 UTC (permalink / raw) To: Artur Paszkiewicz; +Cc: linux-raid [-- Attachment #1: Type: text/plain, Size: 5263 bytes --] On Wed, 9 Apr 2014 17:14:59 +0200 Artur Paszkiewicz <artur.paszkiewicz@intel.com> wrote: > This modifies locking in Create to eliminate a situation where > --incremental can assemble a device between write_init_super() and > add_disk(), which causes Create to fail. > > It sporadically occurs e.g. when metadata is written on a device, > causing an udev change event which triggers mdadm --incremental. > > Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com> Thanks for the patch. I've taken the liberty of changing it a little. I didn't like the fact that we dropped the lock and then took it again. It probably doesn't matter, but it feels cleaner to hold it the whole time. So this is the result. Let me know if you disagree at all. Thanks, NeilBrown From a22b82ef078a37f98e0a08c23dca32fa38792d97 Mon Sep 17 00:00:00 2001 From: Artur Paszkiewicz <artur.paszkiewicz@intel.com> Date: Wed, 9 Apr 2014 17:14:59 +0200 Subject: [PATCH] Fix race between --create and --incremental This modifies locking in Create to eliminate a situation where --incremental can assemble a device between write_init_super() and add_disk(), which causes Create to fail. It sporadically occurs e.g. when metadata is written on a device, causing an udev change event which triggers mdadm --incremental. Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com> Signed-off-by: NeilBrown <neilb@suse.de> diff --git a/Create.c b/Create.c index 9247d46bbfac..98bbdd40a161 100644 --- a/Create.c +++ b/Create.c @@ -740,7 +740,9 @@ int Create(struct supertype *st, char *mddev, map_update(&map, fd2devnm(mdfd), info.text_version, info.uuid, chosen_name); - map_unlock(&map); + /* Keep map locked until devices have been added to array + * to stop another mdadm from finding and using those devices. + */ if (s->bitmap_file && vers < 9003) { major_num = BITMAP_MAJOR_HOSTENDIAN; @@ -753,18 +755,18 @@ int Create(struct supertype *st, char *mddev, if (s->bitmap_file && strcmp(s->bitmap_file, "internal")==0) { if ((vers%100) < 2) { pr_err("internal bitmaps not supported by this kernel.\n"); - goto abort; + goto abort_locked; } if (!st->ss->add_internal_bitmap) { pr_err("internal bitmaps not supported with %s metadata\n", st->ss->name); - goto abort; + goto abort_locked; } if (!st->ss->add_internal_bitmap(st, &s->bitmap_chunk, c->delay, s->write_behind, bitmapsize, 1, major_num)) { pr_err("Given bitmap chunk size not supported.\n"); - goto abort; + goto abort_locked; } s->bitmap_file = NULL; } @@ -790,7 +792,7 @@ int Create(struct supertype *st, char *mddev, if (container_fd < 0) { pr_err("Cannot get exclusive " "open on container - weird.\n"); - goto abort; + goto abort_locked; } if (mdmon_running(st->container_devnm)) { if (c->verbose) @@ -805,7 +807,7 @@ int Create(struct supertype *st, char *mddev, if (rv) { pr_err("failed to set array info for %s: %s\n", mddev, strerror(errno)); - goto abort; + goto abort_locked; } if (s->bitmap_file) { @@ -816,18 +818,18 @@ int Create(struct supertype *st, char *mddev, c->delay, s->write_behind, bitmapsize, major_num)) { - goto abort; + goto abort_locked; } bitmap_fd = open(s->bitmap_file, O_RDWR); if (bitmap_fd < 0) { pr_err("weird: %s cannot be openned\n", s->bitmap_file); - goto abort; + goto abort_locked; } if (ioctl(mdfd, SET_BITMAP_FILE, bitmap_fd) < 0) { pr_err("Cannot set bitmap file for %s: %s\n", mddev, strerror(errno)); - goto abort; + goto abort_locked; } } @@ -884,7 +886,7 @@ int Create(struct supertype *st, char *mddev, pr_err("failed to open %s " "after earlier success - aborting\n", dv->devname); - goto abort; + goto abort_locked; } fstat(fd, &stb); inf->disk.major = major(stb.st_rdev); @@ -896,7 +898,7 @@ int Create(struct supertype *st, char *mddev, fd, dv->devname, dv->data_offset)) { ioctl(mdfd, STOP_ARRAY, NULL); - goto abort; + goto abort_locked; } st->ss->getinfo_super(st, inf, NULL); safe_mode_delay = inf->safe_mode_delay; @@ -922,7 +924,7 @@ int Create(struct supertype *st, char *mddev, pr_err("ADD_NEW_DISK for %s " "failed: %s\n", dv->devname, strerror(errno)); - goto abort; + goto abort_locked; } break; } @@ -939,7 +941,6 @@ int Create(struct supertype *st, char *mddev, * the subarray cursor such that ->getinfo_super once * again returns container info. */ - map_lock(&map); st->ss->getinfo_super(st, &info_new, NULL); if (st->ss->external && s->level != LEVEL_CONTAINER && !same_uuid(info_new.uuid, info.uuid, 0)) { @@ -964,12 +965,12 @@ int Create(struct supertype *st, char *mddev, info_new.uuid, path); free(path); } - map_unlock(&map); flush_metadata_updates(st); st->ss->free_super(st); } } + map_unlock(&map); free(infos); if (s->level == LEVEL_CONTAINER) { [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 828 bytes --] ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix race between --create and --incremental 2014-04-10 0:33 ` NeilBrown @ 2014-04-11 10:08 ` Artur Paszkiewicz 2014-04-30 11:50 ` Artur Paszkiewicz 0 siblings, 1 reply; 5+ messages in thread From: Artur Paszkiewicz @ 2014-04-11 10:08 UTC (permalink / raw) To: NeilBrown; +Cc: linux-raid On 04/10/2014 02:33 AM, NeilBrown wrote: > On Wed, 9 Apr 2014 17:14:59 +0200 Artur Paszkiewicz > <artur.paszkiewicz@intel.com> wrote: > >> This modifies locking in Create to eliminate a situation where >> --incremental can assemble a device between write_init_super() and >> add_disk(), which causes Create to fail. >> >> It sporadically occurs e.g. when metadata is written on a device, >> causing an udev change event which triggers mdadm --incremental. >> >> Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com> > > Thanks for the patch. > I've taken the liberty of changing it a little. I didn't like the fact that > we dropped the lock and then took it again. It probably doesn't matter, but > it feels cleaner to hold it the whole time. > > So this is the result. Let me know if you disagree at all. Hello Neil, Your changes are OK, thanks. Regards, Artur ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix race between --create and --incremental 2014-04-11 10:08 ` Artur Paszkiewicz @ 2014-04-30 11:50 ` Artur Paszkiewicz 2014-05-01 7:17 ` NeilBrown 0 siblings, 1 reply; 5+ messages in thread From: Artur Paszkiewicz @ 2014-04-30 11:50 UTC (permalink / raw) To: NeilBrown; +Cc: linux-raid On 04/11/2014 12:08 PM, Artur Paszkiewicz wrote: > On 04/10/2014 02:33 AM, NeilBrown wrote: >> On Wed, 9 Apr 2014 17:14:59 +0200 Artur Paszkiewicz >> <artur.paszkiewicz@intel.com> wrote: >> >>> This modifies locking in Create to eliminate a situation where >>> --incremental can assemble a device between write_init_super() and >>> add_disk(), which causes Create to fail. >>> >>> It sporadically occurs e.g. when metadata is written on a device, >>> causing an udev change event which triggers mdadm --incremental. >>> >>> Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com> >> >> Thanks for the patch. >> I've taken the liberty of changing it a little. I didn't like the >> fact that >> we dropped the lock and then took it again. It probably doesn't >> matter, but >> it feels cleaner to hold it the whole time. >> >> So this is the result. Let me know if you disagree at all. > > Hello Neil, > > Your changes are OK, thanks. Hello Neil, Are you going to apply this patch? Thanks, Artur ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix race between --create and --incremental 2014-04-30 11:50 ` Artur Paszkiewicz @ 2014-05-01 7:17 ` NeilBrown 0 siblings, 0 replies; 5+ messages in thread From: NeilBrown @ 2014-05-01 7:17 UTC (permalink / raw) To: Artur Paszkiewicz; +Cc: linux-raid [-- Attachment #1: Type: text/plain, Size: 1243 bytes --] On Wed, 30 Apr 2014 13:50:02 +0200 Artur Paszkiewicz <artur.paszkiewicz@intel.com> wrote: > On 04/11/2014 12:08 PM, Artur Paszkiewicz wrote: > > On 04/10/2014 02:33 AM, NeilBrown wrote: > >> On Wed, 9 Apr 2014 17:14:59 +0200 Artur Paszkiewicz > >> <artur.paszkiewicz@intel.com> wrote: > >> > >>> This modifies locking in Create to eliminate a situation where > >>> --incremental can assemble a device between write_init_super() and > >>> add_disk(), which causes Create to fail. > >>> > >>> It sporadically occurs e.g. when metadata is written on a device, > >>> causing an udev change event which triggers mdadm --incremental. > >>> > >>> Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com> > >> > >> Thanks for the patch. > >> I've taken the liberty of changing it a little. I didn't like the > >> fact that > >> we dropped the lock and then took it again. It probably doesn't > >> matter, but > >> it feels cleaner to hold it the whole time. > >> > >> So this is the result. Let me know if you disagree at all. > > > > Hello Neil, > > > > Your changes are OK, thanks. > > Hello Neil, > > Are you going to apply this patch? I had, but I hadn't pushed it out. I have now. NeilBrown [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 828 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-05-01 7:17 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-04-09 15:14 [PATCH] Fix race between --create and --incremental Artur Paszkiewicz 2014-04-10 0:33 ` NeilBrown 2014-04-11 10:08 ` Artur Paszkiewicz 2014-04-30 11:50 ` Artur Paszkiewicz 2014-05-01 7:17 ` NeilBrown
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).