[PATCH 0/4] Raid0 metadata cannot be recognized

[PATCH 0/4] Raid0 metadata cannot be recognized

[Adam Kwolek]

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.

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

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
      FIX: Handle has to be closed
      FIX: Write metadata when mdmon is not running
      FIX: Ping monitor when mdmon is running only


 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(-)

-- 
Signature

[PATCH 1/4] FIX: Ping monitor when mdmon is running only

[Adam Kwolek]

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 {

[PATCH 2/4] FIX: Write metadata when mdmon is not running

[Adam Kwolek]

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);

[PATCH 3/4] FIX: Handle has to be closed

[Adam Kwolek]

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

[PATCH 4/4] FIX: ping_monitor() usage causes memory leaks

[Adam Kwolek]

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);

Re: [PATCH 0/4] Raid0 metadata cannot be recognized

[NeilBrown]

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) {

RE: [PATCH 0/4] Raid0 metadata cannot be recognized

[Kwolek, Adam]

> -----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) {