Re: [PATCH 2/9] FIX: Cannot add spare to monitored container

[NeilBrown <neilb@suse.de>]

On Wed, 12 Jan 2011 15:57:45 +0000 "Kwolek, Adam" <adam.kwolek@intel.com>
wrote:

> > -----Original Message-----
> > From: NeilBrown [mailto:neilb@suse.de]
> > Sent: Wednesday, January 12, 2011 6:43 AM
> > To: Kwolek, Adam
> > Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> > Neubauer, Wojciech
> > Subject: Re: [PATCH 2/9] FIX: Cannot add spare to monitored container
> > 
> > On Tue, 11 Jan 2011 15:04:03 +0100 Adam Kwolek <adam.kwolek@intel.com>
> > wrote:
> > 
> > > This fix rolls backs some changes introduced by patch:
> > > 	"Add spares to raid0 in mdadm"
> > >
> > > Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> > 
> > It would help a lot if you explained *why* you were rolling back these
> > changes.
> > 
> > Not applied.
> > 
> > NeilBrown
> 
> 
> I cannot add spare to i.e. raid5 array. In my unit tests, I'm creating raid5 array and then I'm adding spares for expansion (mdadm --add).
> If I've create container with more disks than create array that is using fewer disks, I can test expansion.
> Spares can be added to raid0 with no monitor usage without problems.
> I've started to observe this problem when patch for adding spares to raid0 was applied.
> I've identify code change that makes problem. This causes this code rollback. 
> 
> Problem that is visible is  that. Monitor cannot write major:minor to new_dev in sysfs /without code roll back/.
> I didn't dig this problem deeply. This patch is rather reminder to signalize problem /and not forget about it/.
> 

Fair enough.
Having a patch which works around a problem and reminds you to fix it later
is a good idea.  However you should make sure that the notes at the top of
the patch make it clear that is the purpose of the patch  - especially when
you post it to me.
It is just SO important to include completed details in the notes at the top
of the patch!!!!

NeilBrown


> BR
> Adam
> 
> > 
> > > ---
> > >
> > >  Manage.c |    8 ++++----
> > >  1 files changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/Manage.c b/Manage.c
> > > index 4e6afb5..fa1b13f 100644
> > > --- a/Manage.c
> > > +++ b/Manage.c
> > > @@ -800,16 +800,16 @@ 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);
> > >  					return 1;
> > >  				}
> > >  				/* write_init_super will close 'dfd' */
> > > -				if (tst->ss->write_init_super(tst))
> > > +				if (tst->ss->external)
> > > +					/* mdmon will write the metadata */
> > > +					close(dfd);
> > > +				else if (tst->ss->write_init_super(tst))
> > >  					return 1;
> > >  			} else if (dv->re_add) {
> > >  				/*  this had better be raid1.
> 
> --
> 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