headers
Neil Brown | 1 May 2008 02:36
X-Face
Picon
Gravatar

Re: [PATCH 3/3] md: add new / required locking for calls to blk_remove_plug and blk_plug_queue

On Tuesday April 29, dan.j.williams <at> intel.com wrote:
> Now that queue flags are no longer atomic (commit:
> 75ad23bc0fcb4f992a5d06982bf0857ab1738e9e)  we must protect calls to
> blk_remove_plug with spin_lock(q->queue_lock).

Can't we just do

  q->queue_lock = &conf->device_lock

and appropriate places in the various ->run functions?

It seems to be that we are doing appropriate locking, we just need to
convince queue_flag_set / queue_flag_clear that the correct lock is
locked.
??

NeilBrown

> 
> Signed-off-by: Dan Williams <dan.j.williams <at> intel.com>
> ---
> 
>  drivers/md/md.c           |    3 +++
>  drivers/md/raid1.c        |   10 ++++++++--
>  drivers/md/raid10.c       |   10 ++++++++--
>  drivers/md/raid5.c        |   12 ++++++++++--
>  include/linux/raid/md_k.h |    3 +++
>  5 files changed, 32 insertions(+), 6 deletions(-)
> 
>
(Continue reading)

Permalink | Reply | 2 comments |
headers
Dan Williams | 1 May 2008 02:50
Picon
Favicon

Re: [PATCH 3/3] md: add new / required locking for calls to blk_remove_plug and blk_plug_queue

On Wed, Apr 30, 2008 at 5:36 PM, Neil Brown <neilb <at> suse.de> wrote:
> On Tuesday April 29, dan.j.williams <at> intel.com wrote:
>  > Now that queue flags are no longer atomic (commit:
>  > 75ad23bc0fcb4f992a5d06982bf0857ab1738e9e)  we must protect calls to
>  > blk_remove_plug with spin_lock(q->queue_lock).
>
>  Can't we just do
>
>   q->queue_lock = &conf->device_lock
>
>  and appropriate places in the various ->run functions?
>
>  It seems to be that we are doing appropriate locking, we just need to
>  convince queue_flag_set / queue_flag_clear that the correct lock is
>  locked.
>  ??
>

That's much simpler.  Hmm... as an additional cleanup should
device_lock then move to mddev_t since it really is a mddev-level
lock?  There seem to be more than a few places that cast
mddev->private just to get conf->device_lock.  Then all this lock
initialization is centralized.

--
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
(Continue reading)

Permalink | Reply | 1 comment |
headers
Neil Brown | 1 May 2008 04:33
X-Face
Picon
Gravatar

Re: [PATCH 3/3] md: add new / required locking for calls to blk_remove_plug and blk_plug_queue

On Wednesday April 30, dan.j.williams <at> intel.com wrote:
> On Wed, Apr 30, 2008 at 5:36 PM, Neil Brown <neilb <at> suse.de> wrote:
> > On Tuesday April 29, dan.j.williams <at> intel.com wrote:
> >  > Now that queue flags are no longer atomic (commit:
> >  > 75ad23bc0fcb4f992a5d06982bf0857ab1738e9e)  we must protect calls to
> >  > blk_remove_plug with spin_lock(q->queue_lock).
> >
> >  Can't we just do
> >
> >   q->queue_lock = &conf->device_lock
> >
> >  and appropriate places in the various ->run functions?
> >
> >  It seems to be that we are doing appropriate locking, we just need to
> >  convince queue_flag_set / queue_flag_clear that the correct lock is
> >  locked.
> >  ??
> >
> 
> That's much simpler.  Hmm... as an additional cleanup should
> device_lock then move to mddev_t since it really is a mddev-level
> lock?  There seem to be more than a few places that cast
> mddev->private just to get conf->device_lock.  Then all this lock
> initialization is centralized.

Conversely, there seem to be plenty of places where we access
conf->device_lock and don't have an 'mddev' handy.
They could of course become
      conf->mddev->device_lock
but I'm not sure that is an improvement.
(Continue reading)

Permalink | Reply | 0 comments |
headers
Neil Brown | 1 May 2008 06:44
X-Face
Picon
Gravatar

Re: [PATCH 1/3] md: ping userspace on 'write-pending' events

On Tuesday April 29, dan.j.williams <at> intel.com wrote:
> Also, when userspace writes 'active', clear all mddev->flags to satisfy
> md_write_start (the other bits do not matter to external-metadata arrays).
> 
> Signed-off-by: Dan Williams <dan.j.williams <at> intel.com>

Thanks Dan.

There are two things that I don't like about this patch.

1/ I don't think clearing all the flags in array_state_store is really
   right.   You do that to make sure that MD_CHANGE_DEVS is clear, but
   there is no guarantee that it won't be set again before it is
   tested in md_write_start.
   I think it is best to just get md_write_start (and md_allow_write)
   to just test the bits they are really interested in.

2/ having the tests of mddev->external isn't necessary.  I really want
   "array_state" to get a notification whenever it changes, no matter
   what sort of metadata is being used.

Change the patch based on those observations I get:

---------------------------------
Signed-off-by: Neil Brown <neilb <at> suse.de>

### Diffstat output
 ./drivers/md/md.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)
(Continue reading)

Permalink | Reply | 2 comments |
headers
Neil Brown | 1 May 2008 06:50
X-Face
Picon
Gravatar

Re: [PATCH 2/3] md: ping userspace on 'stop' events

On Tuesday April 29, dan.j.williams <at> intel.com wrote:
> This additional notification to 'array_state' is needed to allow the monitor
> application to learn about stop events via sysfs.  The
> sysfs_notify("sync_action") call that comes at the end of do_md_stop() (via
> md_new_event) is insufficient since the 'sync_action' attribute has been
> removed by this point.
> 
> (Seems like a sysfs-notify-on-removal patch is a better fix.  Currently removal
>  updates the event count but does not wake up waiters)
> 
> Signed-off-by: Dan Williams <dan.j.williams <at> intel.com>
> ---

Thanks.

Again, drop the "if (mddev->external)".  It isn't needed (I have made
this change).

NeilBrown

> 
>  drivers/md/md.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index ad53035..606c8ee 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
>  <at>  <at>  -3693,6 +3693,9  <at>  <at>  static int do_md_stop(mddev_t * mddev, int mode)
(Continue reading)

Permalink | Reply | 0 comments |
headers
Neil Brown | 1 May 2008 07:35
X-Face
Picon
Gravatar

Re: parallel resync

On Tuesday April 29, bs <at> q-leap.de wrote:
> Hello Neil,
> 
> I think in about December I already asked if you could include our parallel 
> resync patch. We need this patch, since we are doing software raid over 
> fast hardware raid devices and with these hardware raid system the cpu is 
> the bottleneck and not disk i/o.
> 
> Here is the previous thread: http://www.issociate.de/board/post/470063/[PATCH]_(2nd_try)_force_parallel_resync.html
> 
> Is there any chance you could add this patch as well, or if the patch is 
> not suitable yet, could you please tell me what needs to be done?
> 

Sorry for not responding earlier....

I seem to remember the setting used to be global, but I see in this
patch it is per-array -- which makes much more sense.
Though it brings up an interesting question.  If two arrays share a
device, and exactly one of them is flagged for parallel sync, does
that make sense?

Maybe it does.  I suspect the reality is that it is individual devices
that should be flagged for parallel sync or not, and the setting on
the array is just an "and" of the settings for the devices in the
array.  So different settings on arrays which share one device can
make sense...

Note: I'm *not* suggesting that the setting should be moved to the
component devices - that would be too clumsy.  I'm just musing.
(Continue reading)

Permalink | Reply | 2 comments |
headers
Mario 'BitKoenig' Holbe | 1 May 2008 10:52
Picon
Picon
Favicon

Re: Sleeping hard drives in an array?

Greg Cormier <gcormier <at> gmail.com> wrote:
> hdparm -S240 /dev/sdX
> But I'm fairly sure they are not spinning down :( Is there some
> activity mdadm is doing in the background?

There are lots of tools out there and likely installed on normal systems
that do access disks at regular intervals:
smartd, hddtemp, probably some hal-stuff, etc. pp. Most of them do not
access disks in standby mode or can be configured to do so. But as long
as disks are active, they keep accessing them and thus reset the disks
standby timeout.
If you use bitmaps on degraded arrays, depending on your kernel version
even md could access disks at regular intervals, but this is probably
less likely than some of the above mentioned tools.

regards
   Mario
--

-- 
Computer games don't affect kids; I mean if Pac-Man affected us as kids,
we'd all be running around in darkened rooms, munching magic pills and
listening to repetitive electronic music.
                                  -- Kristian Wilson, Nintendo Inc, 1989

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Permalink | Reply | 0 comments |
headers
David Greaves | 1 May 2008 11:55
Gravatar

Re: Sleeping hard drives in an array?

Greg Cormier wrote:
> Is it possible to sleep hard drives in an array?
yes - to be clear you sleep the drives though, not the array.

> Can I sleep the drives in my RAID5 array while it's not being used?
> It's an XFS partition.
XFS is bad at this IIRC.
Certainly noatime is important to prevent cache accesses from updating the fs.

> 
> But I'm fairly sure they are not spinning down :( Is there some
> activity mdadm is doing in the background?

from Documentation/laptops/laptop-mode.txt:

If you want to find out which process caused the disk to spin up, you can
gather information by setting the flag /proc/sys/vm/block_dump. When this flag
is set, Linux reports all disk read and write operations that take place, and
all block dirtyings done to files. This makes it possible to debug why a disk
needs to spin up, and to increase battery life even more. The output of
block_dump is written to the kernel output, and it can be retrieved using
"dmesg". When you use block_dump and your kernel logging level also includes
kernel debugging messages, you probably want to turn off klogd, otherwise
the output of block_dump will be logged, causing disk activity that is not
normally there.

also google found
http://www.nslu2-linux.org/wiki/FAQ/HowtoIdentifyWhichProcessesAccessDisk

Before going too far, make sure the array is up but not mounted and ensure that
(Continue reading)

Permalink | Reply | 0 comments |
headers
Alex Davis | 1 May 2008 13:35
Picon
Favicon

Sharing disks amoung multiple software RAIDs

Is this a bad thing? I'm guessing that it is, but I want independent
confirmation before I spoke to someone I know who's doing this.

Thanks

      ____________________________________________________________________________________
Be a better friend, newshound, and 
know-it-all with Yahoo! Mobile.  Try it now.  http://mobile.yahoo.com/;_ylt=Ahu06i62sR8HDtDypao8Wcj9tAcJ
Permalink | Reply | 0 comments |
headers
Justin Piszcz | 1 May 2008 14:50

Re: Sharing disks amoung multiple software RAIDs


On Thu, 1 May 2008, Alex Davis wrote:

> Is this a bad thing? I'm guessing that it is, but I want independent
> confirmation before I spoke to someone I know who's doing this.
>
> Thanks
>
>
>      ____________________________________________________________________________________
> Be a better friend, newshound, and
> know-it-all with Yahoo! Mobile.  Try it now.  http://mobile.yahoo.com/;_ylt=Ahu06i62sR8HDtDypao8Wcj9tAcJ
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo <at> vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

What is the use case, why would you want to do that?
I have seen people on the list do it before, for example are you going to 
be utilizing both raids at the same time?  If so, I would advise against 
it.

What is the reasoning?

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
(Continue reading)

Permalink | Reply | 0 comments |

Gmane