* [PATCH 1/4] FIX: Ping monitor when mdmon is running only
2011-03-15 13:48 [PATCH 0/4] Raid0 metadata cannot be recognized Adam Kwolek
@ 2011-03-15 13:48 ` Adam Kwolek
2011-03-15 13:48 ` [PATCH 2/4] FIX: Write metadata when mdmon is not running Adam Kwolek
` (3 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Adam Kwolek @ 2011-03-15 13:48 UTC (permalink / raw)
To: neilb; +Cc: linux-raid, dan.j.williams, ed.ciechanowski, wojciech.neubauer
When raid0 is created (no other monitored raid in container),
mdmon is not running. Ping is not necessary in such case.
Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
---
Manage.c | 8 +++++++-
1 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/Manage.c b/Manage.c
index 5996646..a032c08 100644
--- a/Manage.c
+++ b/Manage.c
@@ -896,7 +896,13 @@ int Manage_subdevs(char *devname, int fd,
sysfs_free(sra);
return 1;
}
- ping_monitor(devnum2devname(devnum));
+ if (mdmon_running(devnum)) {
+ char *cont = devnum2devname(devnum);
+ if (cont) {
+ ping_monitor(cont);
+ free(cont);
+ }
+ }
sysfs_free(sra);
close(container_fd);
} else {
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 2/4] FIX: Write metadata when mdmon is not running
2011-03-15 13:48 [PATCH 0/4] Raid0 metadata cannot be recognized Adam Kwolek
2011-03-15 13:48 ` [PATCH 1/4] FIX: Ping monitor when mdmon is running only Adam Kwolek
@ 2011-03-15 13:48 ` Adam Kwolek
2011-03-15 13:48 ` [PATCH 3/4] FIX: Handle has to be closed Adam Kwolek
` (2 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Adam Kwolek @ 2011-03-15 13:48 UTC (permalink / raw)
To: neilb; +Cc: linux-raid, dan.j.williams, ed.ciechanowski, wojciech.neubauer
For raid0 metadata has to be written.
It is possible that Manage.c:812 requires similar guard for mdmon running.
Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
---
Manage.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/Manage.c b/Manage.c
index a032c08..f208d15 100644
--- a/Manage.c
+++ b/Manage.c
@@ -870,6 +870,12 @@ int Manage_subdevs(char *devname, int fd,
close(container_fd);
return 1;
}
+ if (!mdmon_running(devnum))
+ if (tst->ss->write_init_super(tst)) {
+ close(dfd);
+ close(container_fd);
+ return 1;
+ }
close(dfd);
sra = sysfs_read(container_fd, -1, 0);
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 3/4] FIX: Handle has to be closed
2011-03-15 13:48 [PATCH 0/4] Raid0 metadata cannot be recognized Adam Kwolek
2011-03-15 13:48 ` [PATCH 1/4] FIX: Ping monitor when mdmon is running only Adam Kwolek
2011-03-15 13:48 ` [PATCH 2/4] FIX: Write metadata when mdmon is not running Adam Kwolek
@ 2011-03-15 13:48 ` Adam Kwolek
2011-03-15 13:48 ` [PATCH 4/4] FIX: ping_monitor() usage causes memory leaks Adam Kwolek
2011-03-18 1:36 ` [PATCH 0/4] Raid0 metadata cannot be recognized NeilBrown
4 siblings, 0 replies; 7+ messages in thread
From: Adam Kwolek @ 2011-03-15 13:48 UTC (permalink / raw)
To: neilb; +Cc: linux-raid, dan.j.williams, ed.ciechanowski, wojciech.neubauer
Handle has to be closed similar to Manage.c:880
One rule should apply to handlers passed to add_to_super().
Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
---
Manage.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/Manage.c b/Manage.c
index f208d15..0349aa3 100644
--- a/Manage.c
+++ b/Manage.c
@@ -813,6 +813,7 @@ int Manage_subdevs(char *devname, int fd,
close(dfd);
return 1;
}
+ close(dfd);
} else if (dv->re_add) {
/* this had better be raid1.
* As we are "--re-add"ing we must find a spare slot
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 4/4] FIX: ping_monitor() usage causes memory leaks
2011-03-15 13:48 [PATCH 0/4] Raid0 metadata cannot be recognized Adam Kwolek
` (2 preceding siblings ...)
2011-03-15 13:48 ` [PATCH 3/4] FIX: Handle has to be closed Adam Kwolek
@ 2011-03-15 13:48 ` Adam Kwolek
2011-03-18 1:36 ` [PATCH 0/4] Raid0 metadata cannot be recognized NeilBrown
4 siblings, 0 replies; 7+ messages in thread
From: Adam Kwolek @ 2011-03-15 13:48 UTC (permalink / raw)
To: neilb; +Cc: linux-raid, dan.j.williams, ed.ciechanowski, wojciech.neubauer
When for ping_monitor() input devnum2devname() is used,
received string pointer should be passed to free() for memory release.
It is not made in several places. This use case should have function
to avoid memory leak.
Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
---
Assemble.c | 2 +-
Create.c | 2 +-
Grow.c | 2 +-
Incremental.c | 10 ++++------
Manage.c | 9 ++-------
Monitor.c | 2 +-
msg.c | 14 ++++++++++++++
msg.h | 1 +
8 files changed, 25 insertions(+), 17 deletions(-)
diff --git a/Assemble.c b/Assemble.c
index bfc879c..ee0346a 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -1598,7 +1598,7 @@ int assemble_container_content(struct supertype *st, int mdfd,
if (!err) {
if (!mdmon_running(st->container_dev))
start_mdmon(st->container_dev);
- ping_monitor(devnum2devname(st->container_dev));
+ ping_monitor_by_id(st->container_dev);
}
break;
}
diff --git a/Create.c b/Create.c
index 6349f86..9f34425 100644
--- a/Create.c
+++ b/Create.c
@@ -929,7 +929,7 @@ int Create(struct supertype *st, char *mddev,
if (need_mdmon)
start_mdmon(st->container_dev);
- ping_monitor(devnum2devname(st->container_dev));
+ ping_monitor_by_id(st->container_dev);
close(container_fd);
}
wait_for(chosen_name, mdfd);
diff --git a/Grow.c b/Grow.c
index 40e693e..b639585 100644
--- a/Grow.c
+++ b/Grow.c
@@ -3416,7 +3416,7 @@ int Grow_continue(int mdfd, struct supertype *st, struct mdinfo *info,
if (!mdmon_running(st->container_dev))
start_mdmon(st->container_dev);
- ping_monitor(devnum2devname(st->container_dev));
+ ping_monitor_by_id(st->container_dev);
if (info->reshape_active == 2) {
diff --git a/Incremental.c b/Incremental.c
index 300bdca..7218060 100644
--- a/Incremental.c
+++ b/Incremental.c
@@ -438,7 +438,7 @@ int Incremental(char *devname, int verbose, int runstop,
/* 7/ Is there enough devices to possibly start the array? */
/* 7a/ if not, finish with success. */
if (info.array.level == LEVEL_CONTAINER) {
- char *devname = NULL;
+ int devnum;
/* Try to assemble within the container */
map_unlock(&map);
sysfs_uevent(&info, "change");
@@ -448,7 +448,7 @@ int Incremental(char *devname, int verbose, int runstop,
chosen_name, info.array.working_disks);
wait_for(chosen_name, mdfd);
if (st->ss->external)
- devname = devnum2devname(fd2devnum(mdfd));
+ devnum = fd2devnum(mdfd);
close(mdfd);
sysfs_free(sra);
rv = Incremental(chosen_name, verbose, runstop,
@@ -460,10 +460,8 @@ int Incremental(char *devname, int verbose, int runstop,
rv = 0;
/* after spare is added, ping monitor for external metadata
* so that it can eg. try to rebuild degraded array */
- if (st->ss->external) {
- ping_monitor(devname);
- free(devname);
- }
+ if (st->ss->external)
+ ping_monitor_by_id(devnum);
return rv;
}
diff --git a/Manage.c b/Manage.c
index 0349aa3..bdf3e04 100644
--- a/Manage.c
+++ b/Manage.c
@@ -903,13 +903,8 @@ int Manage_subdevs(char *devname, int fd,
sysfs_free(sra);
return 1;
}
- if (mdmon_running(devnum)) {
- char *cont = devnum2devname(devnum);
- if (cont) {
- ping_monitor(cont);
- free(cont);
- }
- }
+ if (mdmon_running(devnum))
+ ping_monitor_by_id(devnum);
sysfs_free(sra);
close(container_fd);
} else {
diff --git a/Monitor.c b/Monitor.c
index d3795b1..3f211b5 100644
--- a/Monitor.c
+++ b/Monitor.c
@@ -975,7 +975,7 @@ int Wait(char *dev)
if (is_subarray(&e->metadata_version[9]))
ping_monitor(&e->metadata_version[9]);
else
- ping_monitor(devnum2devname(devnum));
+ ping_monitor_by_id(devnum);
}
free_mdstat(ms);
return rv;
diff --git a/msg.c b/msg.c
index a1f4bc6..a10c930 100644
--- a/msg.c
+++ b/msg.c
@@ -213,6 +213,20 @@ int ping_monitor(char *devname)
return err;
}
+/* ping monitor using device number */
+int ping_monitor_by_id(int devnum)
+{
+ int err = -1;
+ char *container = devnum2devname(devnum);
+
+ if (container) {
+ err = ping_monitor(container);
+ free(container);
+ }
+
+ return err;
+}
+
static char *ping_monitor_version(char *devname)
{
int sfd = connect_monitor(devname);
diff --git a/msg.h b/msg.h
index 91a7798..c6d037d 100644
--- a/msg.h
+++ b/msg.h
@@ -27,6 +27,7 @@ extern int ack(int fd, int tmo);
extern int wait_reply(int fd, int tmo);
extern int connect_monitor(char *devname);
extern int ping_monitor(char *devname);
+extern int ping_monitor_by_id(int devnum);
extern int block_subarray(struct mdinfo *sra);
extern int unblock_subarray(struct mdinfo *sra, const int unfreeze);
extern int block_monitor(char *container, const int freeze);
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 0/4] Raid0 metadata cannot be recognized
2011-03-15 13:48 [PATCH 0/4] Raid0 metadata cannot be recognized Adam Kwolek
` (3 preceding siblings ...)
2011-03-15 13:48 ` [PATCH 4/4] FIX: ping_monitor() usage causes memory leaks Adam Kwolek
@ 2011-03-18 1:36 ` NeilBrown
2011-03-18 8:38 ` Kwolek, Adam
4 siblings, 1 reply; 7+ messages in thread
From: NeilBrown @ 2011-03-18 1:36 UTC (permalink / raw)
To: Adam Kwolek
Cc: linux-raid, dan.j.williams, ed.ciechanowski, wojciech.neubauer
Sorry for the delayed reply - I've been swamped with other things.
On Tue, 15 Mar 2011 14:48:16 +0100 Adam Kwolek <adam.kwolek@intel.com> wrote:
> Hi,
>
> I've executed unit tests on today's devel3-2 and I've found that
> expansion cannot be executed due to fact that imsm metadata
> cannot be found after creation.
> Looking patches I've found that problem was introduced by patch:
>
> Manage/external: for external metadata, add_to_super needs lock on container.
>
Yes, I messed that up, didn't I.
> It is due to fact that metadata is not written by monitor for raid0.
>
> Problem is fixed by patch:
> FIX: Write metadata when mdmon is not running
That isn't quite right but does help me see where the problem is.
See below for the patch that I have committed.
>
> Other patches addresses problems/i.e. inconsistency in closing handle/
> that I've noticed during investigation.
>
> BR
> Adam
>
>
> ---
>
> Adam Kwolek (4):
> FIX: ping_monitor() usage causes memory leaks
nice clean up - thanks! Applied.
> FIX: Handle has to be closed
No it doesn't. "free_super" closes this handle. The place where I had a
'close' was wrong. I've removed it.
> FIX: Write metadata when mdmon is not running
As mentioned, I applied something which is a bit more complete.
> FIX: Ping monitor when mdmon is running only
I cannot see why this would be necessary. If mdmon is not running, then
ping_monitor will very quickly fail, so there is no need for an extra check,
is there?
Thanks,
NeilBrown
>
>
> Assemble.c | 2 +-
> Create.c | 2 +-
> Grow.c | 2 +-
> Incremental.c | 10 ++++------
> Manage.c | 10 +++++++++-
> Monitor.c | 2 +-
> msg.c | 14 ++++++++++++++
> msg.h | 1 +
> 8 files changed, 32 insertions(+), 11 deletions(-)
>
commit d6221e667f55c46505125ae182051de499000ed8
Author: NeilBrown <neilb@suse.de>
Date: Fri Mar 18 12:31:45 2011 +1100
Manage: fix the mess I made in earlier patch.
When I separated the 'native metadata' case more cleanly from the
"external metadata" case for adding a drive, I left some 'external'
code in the 'native' case, and didn't copy it to the 'external' case.
When - in the external case - we add to super, we much check for
mdmon first, so we know whether to do the metadata update ourselves
or not, then afterwards call either flush_metadata_updates (to send
to mdmon) or sync_metadata (to do it directly).
Reported-by: Adam Kwolek <adam.kwolek@intel.com>
Signed-off-by: NeilBrown <neilb@suse.de>
diff --git a/Manage.c b/Manage.c
index 3361269..a679c24 100644
--- a/Manage.c
+++ b/Manage.c
@@ -835,9 +835,6 @@ int Manage_subdevs(char *devname, int fd,
if (dv->writemostly == 1)
disc.state |= 1 << MD_DISK_WRITEMOSTLY;
dfd = dev_open(dv->devname, O_RDWR | O_EXCL|O_DIRECT);
- if (tst->ss->external &&
- mdmon_running(tst->container_dev))
- tst->update_tail = &tst->updates;
if (tst->ss->add_to_super(tst, &disc, dfd,
dv->devname)) {
close(dfd);
@@ -898,13 +895,18 @@ int Manage_subdevs(char *devname, int fd,
}
dfd = dev_open(dv->devname, O_RDWR | O_EXCL|O_DIRECT);
+ if (mdmon_running(tst->container_dev))
+ tst->update_tail = &tst->updates;
if (tst->ss->add_to_super(tst, &disc, dfd,
dv->devname)) {
close(dfd);
close(container_fd);
return 1;
}
- close(dfd);
+ if (st->update_tail)
+ flush_metadata_updates(st);
+ else
+ tst->ss->sync_metadata(st);
sra = sysfs_read(container_fd, -1, 0);
if (!sra) {
^ permalink raw reply related [flat|nested] 7+ messages in thread* RE: [PATCH 0/4] Raid0 metadata cannot be recognized
2011-03-18 1:36 ` [PATCH 0/4] Raid0 metadata cannot be recognized NeilBrown
@ 2011-03-18 8:38 ` Kwolek, Adam
0 siblings, 0 replies; 7+ messages in thread
From: Kwolek, Adam @ 2011-03-18 8:38 UTC (permalink / raw)
To: NeilBrown
Cc: linux-raid@vger.kernel.org, Williams, Dan J, Ciechanowski, Ed,
Neubauer, Wojciech
> -----Original Message-----
> From: NeilBrown [mailto:neilb@suse.de]
> Sent: Friday, March 18, 2011 2:36 AM
> To: Kwolek, Adam
> Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> Neubauer, Wojciech
> Subject: Re: [PATCH 0/4] Raid0 metadata cannot be recognized
>
>
> Sorry for the delayed reply - I've been swamped with other things.
>
>
> On Tue, 15 Mar 2011 14:48:16 +0100 Adam Kwolek <adam.kwolek@intel.com>
> wrote:
>
> > Hi,
> >
> > I've executed unit tests on today's devel3-2 and I've found that
> > expansion cannot be executed due to fact that imsm metadata
> > cannot be found after creation.
> > Looking patches I've found that problem was introduced by patch:
> >
> > Manage/external: for external metadata, add_to_super needs lock on
> container.
> >
>
> Yes, I messed that up, didn't I.
>
> > It is due to fact that metadata is not written by monitor for raid0.
> >
> > Problem is fixed by patch:
> > FIX: Write metadata when mdmon is not running
>
> That isn't quite right but does help me see where the problem is.
> See below for the patch that I have committed.
>
> >
> > Other patches addresses problems/i.e. inconsistency in closing handle/
> > that I've noticed during investigation.
> >
> > BR
> > Adam
> >
> >
> > ---
> >
> > Adam Kwolek (4):
> > FIX: ping_monitor() usage causes memory leaks
>
> nice clean up - thanks! Applied.
>
> > FIX: Handle has to be closed
>
> No it doesn't. "free_super" closes this handle. The place where I had
> a
> 'close' was wrong. I've removed it.
That was my point, we have to behalf in the same way for all cases, Thanks :)
>
> > FIX: Write metadata when mdmon is not running
>
> As mentioned, I applied something which is a bit more complete.
... but it still not working (try i.e. test 12, it fails)
I'll have a look it.
>
> > FIX: Ping monitor when mdmon is running only
>
> I cannot see why this would be necessary. If mdmon is not running, then
> ping_monitor will very quickly fail, so there is no need for an extra
> check,
> is there?
It looked cleaner for me.
/not try to do something that will for sure fail and we can know about it before we try it/.
But you are right, it is not necessary.
As I mention above I'll look why expansion UT fails again for raid0.
BR
Adam
>
> Thanks,
> NeilBrown
>
> >
> >
> > Assemble.c | 2 +-
> > Create.c | 2 +-
> > Grow.c | 2 +-
> > Incremental.c | 10 ++++------
> > Manage.c | 10 +++++++++-
> > Monitor.c | 2 +-
> > msg.c | 14 ++++++++++++++
> > msg.h | 1 +
> > 8 files changed, 32 insertions(+), 11 deletions(-)
> >
>
>
>
> commit d6221e667f55c46505125ae182051de499000ed8
> Author: NeilBrown <neilb@suse.de>
> Date: Fri Mar 18 12:31:45 2011 +1100
>
> Manage: fix the mess I made in earlier patch.
>
> When I separated the 'native metadata' case more cleanly from the
> "external metadata" case for adding a drive, I left some 'external'
> code in the 'native' case, and didn't copy it to the 'external'
> case.
>
> When - in the external case - we add to super, we much check for
> mdmon first, so we know whether to do the metadata update ourselves
> or not, then afterwards call either flush_metadata_updates (to send
> to mdmon) or sync_metadata (to do it directly).
>
> Reported-by: Adam Kwolek <adam.kwolek@intel.com>
> Signed-off-by: NeilBrown <neilb@suse.de>
>
> diff --git a/Manage.c b/Manage.c
> index 3361269..a679c24 100644
> --- a/Manage.c
> +++ b/Manage.c
> @@ -835,9 +835,6 @@ int Manage_subdevs(char *devname, int fd,
> if (dv->writemostly == 1)
> disc.state |= 1 << MD_DISK_WRITEMOSTLY;
> dfd = dev_open(dv->devname, O_RDWR |
> O_EXCL|O_DIRECT);
> - if (tst->ss->external &&
> - mdmon_running(tst->container_dev))
> - tst->update_tail = &tst->updates;
> if (tst->ss->add_to_super(tst, &disc, dfd,
> dv->devname)) {
> close(dfd);
> @@ -898,13 +895,18 @@ int Manage_subdevs(char *devname, int fd,
> }
>
> dfd = dev_open(dv->devname, O_RDWR |
> O_EXCL|O_DIRECT);
> + if (mdmon_running(tst->container_dev))
> + tst->update_tail = &tst->updates;
> if (tst->ss->add_to_super(tst, &disc, dfd,
> dv->devname)) {
> close(dfd);
> close(container_fd);
> return 1;
> }
> - close(dfd);
> + if (st->update_tail)
> + flush_metadata_updates(st);
> + else
> + tst->ss->sync_metadata(st);
>
> sra = sysfs_read(container_fd, -1, 0);
> if (!sra) {
^ permalink raw reply [flat|nested] 7+ messages in thread