Re: [PATCH] fix: imsm: mdmon should reread component_size on wakeup

[NeilBrown <neilb@suse.de>]

[-- 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 --]