* [PATCH] fix: imsm: mdmon should reread component_size on wakeup
@ 2012-09-17 8:37 Lukasz Dorau
2012-09-20 1:30 ` NeilBrown
0 siblings, 1 reply; 6+ messages in thread
From: Lukasz Dorau @ 2012-09-17 8:37 UTC (permalink / raw)
To: neilb; +Cc: linux-raid, ed.ciechanowski, maciej.patelczyk
Mdmon has not read component_size (in read_and_act()) on wakeup so far.
In case of size's expansion component_size has been changed but
mdmon has not updated it. As a result the metadata was incorrect
during the size's expansion. It has contained no information
that resync was in progress (there has been no checkpoint too).
The metadata has been as if resync has already been finished
but it has not.
Updating of component_size has been added to read_and_act().
Now mdmon uses the correct value of component_size and the correct
metadata (containing information about resync and checkpoint) is written.
Signed-off-by: Lukasz Dorau <lukasz.dorau@intel.com>
---
managemon.c | 6 +++++-
mdmon.h | 1 +
monitor.c | 12 ++++++++++++
3 files changed, 18 insertions(+), 1 deletions(-)
diff --git a/managemon.c b/managemon.c
index ef351b3..fd41463 100644
--- a/managemon.c
+++ b/managemon.c
@@ -123,6 +123,8 @@ static void close_aa(struct active_array *aa)
close(aa->info.state_fd);
if (aa->resync_start_fd >= 0)
close(aa->resync_start_fd);
+ if (aa->component_size_fd >= 0)
+ close(aa->component_size_fd);
if (aa->metadata_fd >= 0)
close(aa->metadata_fd);
if (aa->sync_completed_fd >= 0)
@@ -598,7 +600,8 @@ static int aa_ready(struct active_array *aa)
if (aa->info.state_fd < 0)
return 0;
- if (level > 0 && (aa->action_fd < 0 || aa->resync_start_fd < 0))
+ if (level > 0 && (aa->action_fd < 0 || aa->resync_start_fd < 0 \
+ || aa->component_size_fd < 0))
return 0;
if (!aa->container)
@@ -676,6 +679,7 @@ static void manage_new(struct mdstat_ent *mdstat,
new->action_fd = sysfs_open(new->devnum, NULL, "sync_action");
new->info.state_fd = sysfs_open(new->devnum, NULL, "array_state");
new->resync_start_fd = sysfs_open(new->devnum, NULL, "resync_start");
+ new->component_size_fd = sysfs_open(new->devnum, NULL, "component_size");
new->metadata_fd = sysfs_open(new->devnum, NULL, "metadata_version");
new->sync_completed_fd = sysfs_open(new->devnum, NULL, "sync_completed");
dprintf("%s: inst: %d action: %d state: %d\n", __func__, atoi(inst),
diff --git a/mdmon.h b/mdmon.h
index 59e1b53..0b82598 100644
--- a/mdmon.h
+++ b/mdmon.h
@@ -32,6 +32,7 @@ struct active_array {
int action_fd;
int resync_start_fd;
+ int component_size_fd; /* in case of change of array's size */
int metadata_fd; /* for monitoring rw/ro status */
int sync_completed_fd; /* for checkpoint notification events */
unsigned long long last_checkpoint; /* sync_completed fires for many
diff --git a/monitor.c b/monitor.c
index c987d10..aa58384 100644
--- a/monitor.c
+++ b/monitor.c
@@ -80,6 +80,17 @@ static unsigned long long read_resync_start(int fd)
return strtoull(buf, NULL, 10);
}
+static unsigned long long read_component_size(int fd)
+{
+ char buf[30];
+ int n;
+
+ n = read_attr(buf, 30, fd);
+ if (n <= 0)
+ return 0;
+ return strtoull(buf, NULL, 10);
+}
+
static unsigned long long read_sync_completed(int fd)
{
unsigned long long val;
@@ -229,6 +240,7 @@ static int read_and_act(struct active_array *a)
a->curr_state = read_state(a->info.state_fd);
a->curr_action = read_action(a->action_fd);
a->info.resync_start = read_resync_start(a->resync_start_fd);
+ a->info.component_size = read_component_size(a->component_size_fd) << 1;
sync_completed = read_sync_completed(a->sync_completed_fd);
for (mdi = a->info.devs; mdi ; mdi = mdi->next) {
mdi->next_state = 0;
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] fix: imsm: mdmon should reread component_size on wakeup
2012-09-17 8:37 [PATCH] fix: imsm: mdmon should reread component_size on wakeup Lukasz Dorau
@ 2012-09-20 1:30 ` NeilBrown
2012-09-25 9:08 ` Dorau, Lukasz
0 siblings, 1 reply; 6+ messages in thread
From: NeilBrown @ 2012-09-20 1:30 UTC (permalink / raw)
To: Lukasz Dorau; +Cc: linux-raid, ed.ciechanowski, maciej.patelczyk
[-- Attachment #1: Type: text/plain, Size: 4700 bytes --]
On Mon, 17 Sep 2012 10:37:55 +0200 Lukasz Dorau <lukasz.dorau@intel.com>
wrote:
> Mdmon has not read component_size (in read_and_act()) on wakeup so far.
> In case of size's expansion component_size has been changed but
> mdmon has not updated it. As a result the metadata was incorrect
> during the size's expansion. It has contained no information
> that resync was in progress (there has been no checkpoint too).
> The metadata has been as if resync has already been finished
> but it has not.
>
> Updating of component_size has been added to read_and_act().
> Now mdmon uses the correct value of component_size and the correct
> metadata (containing information about resync and checkpoint) is written.
Thanks, but I don't think I like this approach.
In my mind, the 'monitor' thread should only be monitoring things that it has
to respond to quickly - before another write can complete.
A size change does not need to be so immediate.
So I would rather that mdadm updates component_size but not array_size, and
then 'ping's the manager. The manager thread then checks and notices that
the component size has changed, asks the monitor to update the metadata, then
sets the array_size to match.
Possibly mdadm shouldn't just 'ping' the monitor, but should send the
metadata update ... not sure which is best.
Does that make sense to you?
Thanks,
NeilBrown
>
> Signed-off-by: Lukasz Dorau <lukasz.dorau@intel.com>
> ---
> managemon.c | 6 +++++-
> mdmon.h | 1 +
> monitor.c | 12 ++++++++++++
> 3 files changed, 18 insertions(+), 1 deletions(-)
>
> diff --git a/managemon.c b/managemon.c
> index ef351b3..fd41463 100644
> --- a/managemon.c
> +++ b/managemon.c
> @@ -123,6 +123,8 @@ static void close_aa(struct active_array *aa)
> close(aa->info.state_fd);
> if (aa->resync_start_fd >= 0)
> close(aa->resync_start_fd);
> + if (aa->component_size_fd >= 0)
> + close(aa->component_size_fd);
> if (aa->metadata_fd >= 0)
> close(aa->metadata_fd);
> if (aa->sync_completed_fd >= 0)
> @@ -598,7 +600,8 @@ static int aa_ready(struct active_array *aa)
> if (aa->info.state_fd < 0)
> return 0;
>
> - if (level > 0 && (aa->action_fd < 0 || aa->resync_start_fd < 0))
> + if (level > 0 && (aa->action_fd < 0 || aa->resync_start_fd < 0 \
> + || aa->component_size_fd < 0))
> return 0;
>
> if (!aa->container)
> @@ -676,6 +679,7 @@ static void manage_new(struct mdstat_ent *mdstat,
> new->action_fd = sysfs_open(new->devnum, NULL, "sync_action");
> new->info.state_fd = sysfs_open(new->devnum, NULL, "array_state");
> new->resync_start_fd = sysfs_open(new->devnum, NULL, "resync_start");
> + new->component_size_fd = sysfs_open(new->devnum, NULL, "component_size");
> new->metadata_fd = sysfs_open(new->devnum, NULL, "metadata_version");
> new->sync_completed_fd = sysfs_open(new->devnum, NULL, "sync_completed");
> dprintf("%s: inst: %d action: %d state: %d\n", __func__, atoi(inst),
> diff --git a/mdmon.h b/mdmon.h
> index 59e1b53..0b82598 100644
> --- a/mdmon.h
> +++ b/mdmon.h
> @@ -32,6 +32,7 @@ struct active_array {
>
> int action_fd;
> int resync_start_fd;
> + int component_size_fd; /* in case of change of array's size */
> int metadata_fd; /* for monitoring rw/ro status */
> int sync_completed_fd; /* for checkpoint notification events */
> unsigned long long last_checkpoint; /* sync_completed fires for many
> diff --git a/monitor.c b/monitor.c
> index c987d10..aa58384 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -80,6 +80,17 @@ static unsigned long long read_resync_start(int fd)
> return strtoull(buf, NULL, 10);
> }
>
> +static unsigned long long read_component_size(int fd)
> +{
> + char buf[30];
> + int n;
> +
> + n = read_attr(buf, 30, fd);
> + if (n <= 0)
> + return 0;
> + return strtoull(buf, NULL, 10);
> +}
> +
> static unsigned long long read_sync_completed(int fd)
> {
> unsigned long long val;
> @@ -229,6 +240,7 @@ static int read_and_act(struct active_array *a)
> a->curr_state = read_state(a->info.state_fd);
> a->curr_action = read_action(a->action_fd);
> a->info.resync_start = read_resync_start(a->resync_start_fd);
> + a->info.component_size = read_component_size(a->component_size_fd) << 1;
> sync_completed = read_sync_completed(a->sync_completed_fd);
> for (mdi = a->info.devs; mdi ; mdi = mdi->next) {
> mdi->next_state = 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] fix: imsm: mdmon should reread component_size on wakeup
2012-09-20 1:30 ` NeilBrown
@ 2012-09-25 9:08 ` Dorau, Lukasz
2012-09-26 0:59 ` NeilBrown
0 siblings, 1 reply; 6+ messages in thread
From: Dorau, Lukasz @ 2012-09-25 9:08 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid@vger.kernel.org, Ciechanowski, Ed, Patelczyk, Maciej
On Thursday, September 20, 2012 3:31 AM NeilBrown <neilb@suse.de> wrote:
> On Mon, 17 Sep 2012 10:37:55 +0200 Lukasz Dorau
> <lukasz.dorau@intel.com> wrote:
>
> > Mdmon has not read component_size (in read_and_act()) on wakeup so
> far.
> > In case of size's expansion component_size has been changed but
> > mdmon has not updated it. As a result the metadata was incorrect
> > during the size's expansion. It has contained no information
> > that resync was in progress (there has been no checkpoint too).
> > The metadata has been as if resync has already been finished
> > but it has not.
> >
> > Updating of component_size has been added to read_and_act().
> > Now mdmon uses the correct value of component_size and the correct
> > metadata (containing information about resync and checkpoint) is written.
>
> Thanks, but I don't think I like this approach.
>
> In my mind, the 'monitor' thread should only be monitoring things that it has
> to respond to quickly - before another write can complete.
> A size change does not need to be so immediate.
> So I would rather that mdadm updates component_size but not array_size,
> and
> then 'ping's the manager. The manager thread then checks and notices that
> the component size has changed, asks the monitor to update the metadata,
> then
> sets the array_size to match.
>
> Possibly mdadm shouldn't just 'ping' the monitor, but should send the
> metadata update ... not sure which is best.
>
> Does that make sense to you?
>
Maybe the description of the patch was a bit misleading and I suppose
that you have misunderstood me.
mdadm changes (updates) "component_size", not mdmon. The only thing
this patch adds is that mdmon rereads the value of "component_size" on wakeup
in case it has been changed by mdadm in meantime. So far mdmon has read
"component_size" only once on startup. It was wrong, because if "component_size"
has been changed by mdadm (in case of size's expansion), mdmon was using wrong,
old value of "component_size". This patch corrects that.
Is it OK for you?
I will send the patch again with a bit corrected description. I hope it will be better.
The code has not been changed.
Regards,
Lukasz
>
>
> >
> > Signed-off-by: Lukasz Dorau <lukasz.dorau@intel.com>
> > ---
> > managemon.c | 6 +++++-
> > mdmon.h | 1 +
> > monitor.c | 12 ++++++++++++
> > 3 files changed, 18 insertions(+), 1 deletions(-)
> >
> > diff --git a/managemon.c b/managemon.c
> > index ef351b3..fd41463 100644
> > --- a/managemon.c
> > +++ b/managemon.c
> > @@ -123,6 +123,8 @@ static void close_aa(struct active_array *aa)
> > close(aa->info.state_fd);
> > if (aa->resync_start_fd >= 0)
> > close(aa->resync_start_fd);
> > + if (aa->component_size_fd >= 0)
> > + close(aa->component_size_fd);
> > if (aa->metadata_fd >= 0)
> > close(aa->metadata_fd);
> > if (aa->sync_completed_fd >= 0)
> > @@ -598,7 +600,8 @@ static int aa_ready(struct active_array *aa)
> > if (aa->info.state_fd < 0)
> > return 0;
> >
> > - if (level > 0 && (aa->action_fd < 0 || aa->resync_start_fd < 0))
> > + if (level > 0 && (aa->action_fd < 0 || aa->resync_start_fd < 0 \
> > + || aa->component_size_fd < 0))
> > return 0;
> >
> > if (!aa->container)
> > @@ -676,6 +679,7 @@ static void manage_new(struct mdstat_ent
> *mdstat,
> > new->action_fd = sysfs_open(new->devnum, NULL, "sync_action");
> > new->info.state_fd = sysfs_open(new->devnum, NULL,
> "array_state");
> > new->resync_start_fd = sysfs_open(new->devnum, NULL,
> "resync_start");
> > + new->component_size_fd = sysfs_open(new->devnum, NULL,
> "component_size");
> > new->metadata_fd = sysfs_open(new->devnum, NULL,
> "metadata_version");
> > new->sync_completed_fd = sysfs_open(new->devnum, NULL,
> "sync_completed");
> > dprintf("%s: inst: %d action: %d state: %d\n", __func__, atoi(inst),
> > diff --git a/mdmon.h b/mdmon.h
> > index 59e1b53..0b82598 100644
> > --- a/mdmon.h
> > +++ b/mdmon.h
> > @@ -32,6 +32,7 @@ struct active_array {
> >
> > int action_fd;
> > int resync_start_fd;
> > + int component_size_fd; /* in case of change of array's size */
> > int metadata_fd; /* for monitoring rw/ro status */
> > int sync_completed_fd; /* for checkpoint notification events */
> > unsigned long long last_checkpoint; /* sync_completed fires for
> many
> > diff --git a/monitor.c b/monitor.c
> > index c987d10..aa58384 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -80,6 +80,17 @@ static unsigned long long read_resync_start(int fd)
> > return strtoull(buf, NULL, 10);
> > }
> >
> > +static unsigned long long read_component_size(int fd)
> > +{
> > + char buf[30];
> > + int n;
> > +
> > + n = read_attr(buf, 30, fd);
> > + if (n <= 0)
> > + return 0;
> > + return strtoull(buf, NULL, 10);
> > +}
> > +
> > static unsigned long long read_sync_completed(int fd)
> > {
> > unsigned long long val;
> > @@ -229,6 +240,7 @@ static int read_and_act(struct active_array *a)
> > a->curr_state = read_state(a->info.state_fd);
> > a->curr_action = read_action(a->action_fd);
> > a->info.resync_start = read_resync_start(a->resync_start_fd);
> > + a->info.component_size = read_component_size(a-
> >component_size_fd) << 1;
> > sync_completed = read_sync_completed(a->sync_completed_fd);
> > for (mdi = a->info.devs; mdi ; mdi = mdi->next) {
> > mdi->next_state = 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 [flat|nested] 6+ messages in thread
* [PATCH] fix: imsm: mdmon should reread component_size on wakeup
@ 2012-09-25 9:18 Lukasz Dorau
0 siblings, 0 replies; 6+ messages in thread
From: Lukasz Dorau @ 2012-09-25 9:18 UTC (permalink / raw)
To: neilb; +Cc: linux-raid, ed.ciechanowski, maciej.patelczyk
Mdmon does not reread component_size (in read_and_act()) on wakeup now.
It is wrong because in case of size's expansion component_size is changed
by mdadm but mdmon does not reread its new value and uses a wrong, old one.
As a result the metadata is incorrect during the size's expansion.
It contains no information that resync is in progress (there is no
checkpoint too). The metadata is as if resync has already been finished
but it has not.
Rereading of component_size has been added to read_and_act().
Now mdmon uses the correct, current value of component_size and the correct
metadata (containing information about resync and checkpoint) is written.
Signed-off-by: Lukasz Dorau <lukasz.dorau@intel.com>
---
managemon.c | 6 +++++-
mdmon.h | 1 +
monitor.c | 12 ++++++++++++
3 files changed, 18 insertions(+), 1 deletions(-)
diff --git a/managemon.c b/managemon.c
index ef351b3..fd41463 100644
--- a/managemon.c
+++ b/managemon.c
@@ -123,6 +123,8 @@ static void close_aa(struct active_array *aa)
close(aa->info.state_fd);
if (aa->resync_start_fd >= 0)
close(aa->resync_start_fd);
+ if (aa->component_size_fd >= 0)
+ close(aa->component_size_fd);
if (aa->metadata_fd >= 0)
close(aa->metadata_fd);
if (aa->sync_completed_fd >= 0)
@@ -598,7 +600,8 @@ static int aa_ready(struct active_array *aa)
if (aa->info.state_fd < 0)
return 0;
- if (level > 0 && (aa->action_fd < 0 || aa->resync_start_fd < 0))
+ if (level > 0 && (aa->action_fd < 0 || aa->resync_start_fd < 0 \
+ || aa->component_size_fd < 0))
return 0;
if (!aa->container)
@@ -676,6 +679,7 @@ static void manage_new(struct mdstat_ent *mdstat,
new->action_fd = sysfs_open(new->devnum, NULL, "sync_action");
new->info.state_fd = sysfs_open(new->devnum, NULL, "array_state");
new->resync_start_fd = sysfs_open(new->devnum, NULL, "resync_start");
+ new->component_size_fd = sysfs_open(new->devnum, NULL, "component_size");
new->metadata_fd = sysfs_open(new->devnum, NULL, "metadata_version");
new->sync_completed_fd = sysfs_open(new->devnum, NULL, "sync_completed");
dprintf("%s: inst: %d action: %d state: %d\n", __func__, atoi(inst),
diff --git a/mdmon.h b/mdmon.h
index 59e1b53..0b82598 100644
--- a/mdmon.h
+++ b/mdmon.h
@@ -32,6 +32,7 @@ struct active_array {
int action_fd;
int resync_start_fd;
+ int component_size_fd; /* in case of change of array's size */
int metadata_fd; /* for monitoring rw/ro status */
int sync_completed_fd; /* for checkpoint notification events */
unsigned long long last_checkpoint; /* sync_completed fires for many
diff --git a/monitor.c b/monitor.c
index c987d10..aa58384 100644
--- a/monitor.c
+++ b/monitor.c
@@ -80,6 +80,17 @@ static unsigned long long read_resync_start(int fd)
return strtoull(buf, NULL, 10);
}
+static unsigned long long read_component_size(int fd)
+{
+ char buf[30];
+ int n;
+
+ n = read_attr(buf, 30, fd);
+ if (n <= 0)
+ return 0;
+ return strtoull(buf, NULL, 10);
+}
+
static unsigned long long read_sync_completed(int fd)
{
unsigned long long val;
@@ -229,6 +240,7 @@ static int read_and_act(struct active_array *a)
a->curr_state = read_state(a->info.state_fd);
a->curr_action = read_action(a->action_fd);
a->info.resync_start = read_resync_start(a->resync_start_fd);
+ a->info.component_size = read_component_size(a->component_size_fd) << 1;
sync_completed = read_sync_completed(a->sync_completed_fd);
for (mdi = a->info.devs; mdi ; mdi = mdi->next) {
mdi->next_state = 0;
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] fix: imsm: mdmon should reread component_size on wakeup
2012-09-25 9:08 ` Dorau, Lukasz
@ 2012-09-26 0:59 ` NeilBrown
2012-09-26 12:22 ` Dorau, Lukasz
0 siblings, 1 reply; 6+ messages in thread
From: NeilBrown @ 2012-09-26 0:59 UTC (permalink / raw)
To: Dorau, Lukasz
Cc: linux-raid@vger.kernel.org, Ciechanowski, Ed, Patelczyk, Maciej
[-- Attachment #1: Type: text/plain, Size: 2765 bytes --]
On Tue, 25 Sep 2012 09:08:45 +0000 "Dorau, Lukasz" <lukasz.dorau@intel.com>
wrote:
> On Thursday, September 20, 2012 3:31 AM NeilBrown <neilb@suse.de> wrote:
> > On Mon, 17 Sep 2012 10:37:55 +0200 Lukasz Dorau
> > <lukasz.dorau@intel.com> wrote:
> >
> > > Mdmon has not read component_size (in read_and_act()) on wakeup so
> > far.
> > > In case of size's expansion component_size has been changed but
> > > mdmon has not updated it. As a result the metadata was incorrect
> > > during the size's expansion. It has contained no information
> > > that resync was in progress (there has been no checkpoint too).
> > > The metadata has been as if resync has already been finished
> > > but it has not.
> > >
> > > Updating of component_size has been added to read_and_act().
> > > Now mdmon uses the correct value of component_size and the correct
> > > metadata (containing information about resync and checkpoint) is written.
> >
> > Thanks, but I don't think I like this approach.
> >
> > In my mind, the 'monitor' thread should only be monitoring things that it has
> > to respond to quickly - before another write can complete.
> > A size change does not need to be so immediate.
> > So I would rather that mdadm updates component_size but not array_size,
> > and
> > then 'ping's the manager. The manager thread then checks and notices that
> > the component size has changed, asks the monitor to update the metadata,
> > then
> > sets the array_size to match.
> >
> > Possibly mdadm shouldn't just 'ping' the monitor, but should send the
> > metadata update ... not sure which is best.
> >
> > Does that make sense to you?
> >
>
> Maybe the description of the patch was a bit misleading and I suppose
> that you have misunderstood me.
> mdadm changes (updates) "component_size", not mdmon. The only thing
> this patch adds is that mdmon rereads the value of "component_size" on wakeup
> in case it has been changed by mdadm in meantime. So far mdmon has read
> "component_size" only once on startup. It was wrong, because if "component_size"
> has been changed by mdadm (in case of size's expansion), mdmon was using wrong,
> old value of "component_size". This patch corrects that.
> Is it OK for you?
That is exactly what I understood you to mean. But I don't agree with it.
It isn't the job of the monitor thread to notice geometry changes. That is
the job of the manager thread, possibly being told by mdadm via the socket.
The manager should explicitly tell the the monitor to perform a metadata
update when the size changes.
The monitor doesn't monitor chunksize or level or any other part of th
geometry of the array, and it shouldn't monitor component_size either.
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] fix: imsm: mdmon should reread component_size on wakeup
2012-09-26 0:59 ` NeilBrown
@ 2012-09-26 12:22 ` Dorau, Lukasz
0 siblings, 0 replies; 6+ messages in thread
From: Dorau, Lukasz @ 2012-09-26 12:22 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid@vger.kernel.org, Ciechanowski, Ed, Patelczyk, Maciej
On Wednesday, September 26, 2012 2:59 AM NeilBrown <neilb@suse.de> wrote:
>
> On Tue, 25 Sep 2012 09:08:45 +0000 "Dorau, Lukasz"
> <lukasz.dorau@intel.com> wrote:
>
> > On Thursday, September 20, 2012 3:31 AM NeilBrown <neilb@suse.de>
> wrote:
> > > On Mon, 17 Sep 2012 10:37:55 +0200 Lukasz Dorau
> > > <lukasz.dorau@intel.com> wrote:
> > >
> > > > Mdmon has not read component_size (in read_and_act()) on wakeup
> so
> > > far.
> > > > In case of size's expansion component_size has been changed but
> > > > mdmon has not updated it. As a result the metadata was incorrect
> > > > during the size's expansion. It has contained no information
> > > > that resync was in progress (there has been no checkpoint too).
> > > > The metadata has been as if resync has already been finished
> > > > but it has not.
> > > >
> > > > Updating of component_size has been added to read_and_act().
> > > > Now mdmon uses the correct value of component_size and the correct
> > > > metadata (containing information about resync and checkpoint) is
> written.
> > >
> > > Thanks, but I don't think I like this approach.
> > >
> > > In my mind, the 'monitor' thread should only be monitoring things that it
> has
> > > to respond to quickly - before another write can complete.
> > > A size change does not need to be so immediate.
> > > So I would rather that mdadm updates component_size but not
> array_size,
> > > and
> > > then 'ping's the manager. The manager thread then checks and notices
> that
> > > the component size has changed, asks the monitor to update the
> metadata,
> > > then
> > > sets the array_size to match.
> > >
> > > Possibly mdadm shouldn't just 'ping' the monitor, but should send the
> > > metadata update ... not sure which is best.
> > >
> > > Does that make sense to you?
> > >
> >
> > Maybe the description of the patch was a bit misleading and I suppose
> > that you have misunderstood me.
> > mdadm changes (updates) "component_size", not mdmon. The only thing
> > this patch adds is that mdmon rereads the value of "component_size" on
> wakeup
> > in case it has been changed by mdadm in meantime. So far mdmon has
> read
> > "component_size" only once on startup. It was wrong, because if
> "component_size"
> > has been changed by mdadm (in case of size's expansion), mdmon was
> using wrong,
> > old value of "component_size". This patch corrects that.
> > Is it OK for you?
>
> That is exactly what I understood you to mean. But I don't agree with it.
>
> It isn't the job of the monitor thread to notice geometry changes. That is
> the job of the manager thread, possibly being told by mdadm via the socket.
> The manager should explicitly tell the the monitor to perform a metadata
> update when the size changes.
> The monitor doesn't monitor chunksize or level or any other part of th
> geometry of the array, and it shouldn't monitor component_size either.
>
OK, thanks for explanation.
Lukasz
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-09-26 12:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-17 8:37 [PATCH] fix: imsm: mdmon should reread component_size on wakeup Lukasz Dorau
2012-09-20 1:30 ` NeilBrown
2012-09-25 9:08 ` Dorau, Lukasz
2012-09-26 0:59 ` NeilBrown
2012-09-26 12:22 ` Dorau, Lukasz
-- strict thread matches above, loose matches on Subject: below --
2012-09-25 9:18 Lukasz Dorau
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).