Linux RAID subsystem development
 help / color / mirror / Atom feed
* Re: Can't reshape raid0 to raid10
From: Xiao Ni @ 2015-02-05  8:44 UTC (permalink / raw)
  To: NeilBrown; +Cc: linux-raid
In-Reply-To: <20150204173921.23df9d79@notabene.brown>



----- Original Message -----
> From: "NeilBrown" <neilb@suse.de>
> To: "Xiao Ni" <xni@redhat.com>
> Cc: linux-raid@vger.kernel.org
> Sent: Wednesday, February 4, 2015 2:39:21 PM
> Subject: Re: Can't reshape raid0 to raid10
> 
> On Mon, 29 Dec 2014 22:13:41 -0500 (EST) Xiao Ni <xni@redhat.com> wrote:
> 
> > Hi Neil
> > 
> >    When I try to reshape a raid0 to raid10, it'll fail like this:
> > 
> > [root@dhcp-12-133 mdadm-3.3.2]# lsblk
> > NAME                    MAJ:MIN RM   SIZE RO TYPE MOUNTPOINT
> > sda                       8:0    0 111.8G  0 disk
> > ├─sda1                    8:1    0  1000M  0 part /boot
> > ├─sda2                    8:2    0  29.3G  0 part /
> > ├─sda3                    8:3    0   512M  0 part [SWAP]
> > ├─sda4                    8:4    0     1K  0 part
> > ├─sda5                    8:5    0   102M  0 part
> > └─sda6                    8:6    0  10.1G  0 part
> >   └─VolGroup00-LogVol00 254:0    0   9.9G  0 lvm
> > sdb                       8:16   0 111.8G  0 disk
> > ├─sdb1                    8:17   0     2G  0 part
> > └─sdb2                    8:18   0    10G  0 part
> > sdc                       8:32   0 186.3G  0 disk
> > ├─sdc1                    8:33   0     2G  0 part
> > └─sdc2                    8:34   0    10G  0 part
> > sdd                       8:48   0 111.8G  0 disk
> > ├─sdd1                    8:49   0     2G  0 part
> > └─sdd2                    8:50   0    10G  0 part
> > [root@dhcp-12-133 mdadm-3.3.2]# mdadm -CR /dev/md0 -l0 -n3 /dev/sdb1
> > /dev/sdc1 /dev/sdd1
> > mdadm: Defaulting to version 1.2 metadata
> > mdadm: array /dev/md0 started.
> > [root@dhcp-12-133 mdadm-3.3.2]# mdadm --grow /dev/md0 -l10 -a /dev/sdb2
> > /dev/sdc2 /dev/sdd2
> > mdadm: level of /dev/md0 changed to raid10
> > mdadm: add new device failed for /dev/sdb2 as 6: No space left on device
> > 
> >    
> >    But if I reshape the raid0 to raid5, reshape raid5 to raid0, then
> >    reshape raid0 to raid10 use
> > the same command it'll succeed.
> > 
> > [root@dhcp-12-133 mdadm-3.3.2]# mdadm -CR /dev/md0 -l0 -n3 /dev/sdb1
> > /dev/sdc1 /dev/sdd1
> > [root@dhcp-12-133 mdadm-3.3.2]# mdadm --grow /dev/md0 -l5
> > [root@dhcp-12-133 mdadm-3.3.2]# cat /proc/mdstat
> > Personalities : [raid6] [raid5] [raid4] [raid0] [raid10]
> > md0 : active raid5 sdd1[2] sdc1[1] sdb1[0]
> >       6285312 blocks super 1.2 level 5, 512k chunk, algorithm 2 [4/3]
> >       [UUU_]
> >       
> > unused devices: <none>
> > 
> > [root@dhcp-12-133 mdadm-3.3.2]# mdadm --grow /dev/md0 -l0
> > [root@dhcp-12-133 mdadm-3.3.2]# cat /proc/mdstat
> > Personalities : [raid6] [raid5] [raid4] [raid0] [raid10]
> > md0 : active raid0 sdd1[2] sdc1[1] sdb1[0]
> >       6285312 blocks super 1.2 512k chunks
> >       
> > unused devices: <none>
> > [root@dhcp-12-133 mdadm-3.3.2]# mdadm --grow /dev/md0 -l10 -a /dev/sdb2
> > /dev/sdc2 /dev/sdd2
> > mdadm: level of /dev/md0 changed to raid10
> > mdadm: added /dev/sdb2
> > mdadm: added /dev/sdc2
> > mdadm: added /dev/sdd2
> > 
> >     So I guess it's the problem add the disk to raid10 after the reshaping.
> >     In the function
> > super_1_validate, it'll set the  mddev->dev_sectors using the superblock
> > read from disks.
> > If it's raid0, the le64_to_cpu(sb-size) is 0. So when add disk to raid10
> > bind_rdev_to_array
> > return -ENOSPC.
> > 
> >     When create raid0, it doesn't write give the value to s->size. So the
> >     sb-size is 0.
> > I modify the code about Create.c. I'm not sure whether it's right to do so.
> > But it can resolve
> > the problem.
> 
> Thanks for the report.
> 
> I think this need to be fixing in the md driver, when the reshape happens.
> 
> Something like the following.
> 
> NeilBrown
> 
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index d1203cddb024..b8d76b1fba64 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -3872,7 +3872,7 @@ static int raid10_resize(struct mddev *mddev, sector_t
> sectors)
>  	return 0;
>  }
>  
> -static void *raid10_takeover_raid0(struct mddev *mddev)
> +static void *raid10_takeover_raid0(struct mddev *mddev, sector_t size, int
> devs)
>  {
>  	struct md_rdev *rdev;
>  	struct r10conf *conf;
> @@ -3882,6 +3882,7 @@ static void *raid10_takeover_raid0(struct mddev *mddev)
>  		       mdname(mddev));
>  		return ERR_PTR(-EINVAL);
>  	}
> +	sector_div(size, devs);
>  
>  	/* Set new parameters */
>  	mddev->new_level = 10;
> @@ -3892,12 +3893,15 @@ static void *raid10_takeover_raid0(struct mddev
> *mddev)
>  	mddev->raid_disks *= 2;
>  	/* make sure it will be not marked as dirty */
>  	mddev->recovery_cp = MaxSector;
> +	mddev->dev_sectors = size;
>  
>  	conf = setup_conf(mddev);
>  	if (!IS_ERR(conf)) {
>  		rdev_for_each(rdev, mddev)
> -			if (rdev->raid_disk >= 0)
> +			if (rdev->raid_disk >= 0) {
>  				rdev->new_raid_disk = rdev->raid_disk * 2;
> +				rdev->sectors = size;
> +			}
>  		conf->barrier = 1;
>  	}
>  
> @@ -3920,7 +3924,9 @@ static void *raid10_takeover(struct mddev *mddev)
>  			       mdname(mddev));
>  			return ERR_PTR(-EINVAL);
>  		}
> -		return raid10_takeover_raid0(mddev);
> +		return raid10_takeover_raid0(mddev,
> +			raid0_conf->strip_zone->zone_end,
> +			raid0_conf->strip_zone->nb_dev);
>  	}
>  	return ERR_PTR(-EINVAL);
>  }
> 

   Thanks, the problem is fixed.

Best Regards
Xiao
--
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

* re: md: 'array_size' sysfs attribute
From: Dan Carpenter @ 2015-02-05 11:02 UTC (permalink / raw)
  To: dan.j.williams; +Cc: linux-raid

Hello Dan Williams,

The patch b522adcde9c4: "md: 'array_size' sysfs attribute" from Mar
31, 2009, leads to the following static checker warning:

	drivers/md/md.c:5069 md_run()
	error: we previously assumed 'mddev->pers' could be null (see line 4936)

This code is really old.  I don't know why my stupid scripts are marking
it as a new warning.  We don't set "mddev->pers" to non-NULL until the
end of the function so it looks like a real bug and that "pers->size"
was intended.  When I fix that bug then it un-silences this warning:

	drivers/md/md.c:5080 md_run()
	error: we previously assumed 'mddev->pers' could be null (see line 4936)

And that also is a real bug, but I'm not sure the right fix for that.

Basically, it's bugs all the way down from the code, to the fix, to the
static checker.  *sigh*.

drivers/md/md.c
  5060          err = pers->run(mddev);
  5061          if (err)
  5062                  printk(KERN_ERR "md: pers->run() failed ...\n");
  5063          else if (pers->size(mddev, 0, 0) < mddev->array_sectors) {
  5064                  WARN_ONCE(!mddev->external_size, "%s: default size too small,"
  5065                            " but 'external_size' not in effect?\n", __func__);
  5066                  printk(KERN_ERR
  5067                         "md: invalid array_size %llu > default size %llu\n",
  5068                         (unsigned long long)mddev->array_sectors / 2,
  5069                         (unsigned long long)mddev->pers->size(mddev, 0, 0) / 2);
                                                   ^^^^^^^^^^^^^^^^^^
This should be "pers->size()".

  5070                  err = -EINVAL;
  5071          }
  5072          if (err == 0 && pers->sync_request &&
  5073              (mddev->bitmap_info.file || mddev->bitmap_info.offset)) {
  5074                  err = bitmap_create(mddev);
  5075                  if (err)
  5076                          printk(KERN_ERR "%s: failed to create bitmap (%d)\n",
  5077                                 mdname(mddev), err);
  5078          }
  5079          if (err) {
  5080                  mddev_detach(mddev);
                        ^^^^^^^^^^^^^^^^^^^
mddev_detach() will oops if mddev->pers() is not set.  We could set
mdev->pers earlier, I suppose.

  5081                  pers->free(mddev, mddev->private);
  5082                  module_put(pers->owner);
  5083                  bitmap_destroy(mddev);
  5084                  return err;
  5085          }
  5086          if (mddev->queue) {
  5087                  mddev->queue->backing_dev_info.congested_data = mddev;
  5088                  mddev->queue->backing_dev_info.congested_fn = md_congested;
  5089                  blk_queue_merge_bvec(mddev->queue, md_mergeable_bvec);
  5090          }

regards,
dan carpenter

^ permalink raw reply

* RAID1 might_sleep() warning on 3.19-rc7
From: Tony Battersby @ 2015-02-05 20:27 UTC (permalink / raw)
  To: Neil Brown, linux-raid

I get the might_sleep() warning below when writing some data to an ext3
filesystem on a RAID1.  But everything works OK, so there is no actual
problem, just a warning.

I see that there has been a fix for a might_sleep() warning in md/bitmap
since 3.19-rc7, but this is a different warning.

---

> cat /proc/mdstat
Personalities : [raid1]
md0 : active raid1 sda1[0] sdb1[1]
      1959884 blocks super 1.0 [2/2] [UU]
      
unused devices: <none>

---

> grep md0 /proc/mounts
/dev/md0 / ext3 rw,noatime,errors=continue,barrier=1,data=journal 0 0

---

WARNING: CPU: 3 PID: 1069 at kernel/sched/core.c:7300 __might_sleep+0x82/0x90()
do not call blocking ops when !TASK_RUNNING; state=2 set at [<ffffffff8028faa1>] prepare_to_wait+0x31/0xa0
Modules linked in: iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi igb i2c_algo_bit ptp pps_core mptsas mptscsih mptbase pm80xx libsas mpt2sas scsi_transport_sas raid_class sg coretemp eeprom w83795 i2c_i801
CPU: 3 PID: 1069 Comm: kjournald Not tainted 3.19.0-rc7 #1
Hardware name: Supermicro X8DTH-i/6/iF/6F/X8DTH, BIOS 2.1b       05/04/12  
 0000000000001c84 ffff88032f1df608 ffffffff80645918 0000000000001c84
 ffff88032f1df658 ffff88032f1df648 ffffffff8025ea6b ffff8800bb0b4d58
 0000000000000000 00000000000006f6 ffffffff80942b6f ffff8803317b8a00
Call Trace:
 [<ffffffff80645918>] dump_stack+0x4f/0x6f
 [<ffffffff8025ea6b>] warn_slowpath_common+0x8b/0xd0
 [<ffffffff8025eb51>] warn_slowpath_fmt+0x41/0x50
 [<ffffffff8028faa1>] ? prepare_to_wait+0x31/0xa0
 [<ffffffff8028faa1>] ? prepare_to_wait+0x31/0xa0
 [<ffffffff8027ee62>] __might_sleep+0x82/0x90
 [<ffffffff803bee06>] generic_make_request_checks+0x36/0x2d0
 [<ffffffff802943ed>] ? trace_hardirqs_on+0xd/0x10
 [<ffffffff803bf0b3>] generic_make_request+0x13/0x100
 [<ffffffff8054983b>] raid1_unplug+0x12b/0x170
 [<ffffffff803c1302>] blk_flush_plug_list+0xa2/0x230
 [<ffffffff80294315>] ? trace_hardirqs_on_caller+0x105/0x1d0
 [<ffffffff80646760>] ? bit_wait_timeout+0x70/0x70
 [<ffffffff80646383>] io_schedule+0x43/0x80
 [<ffffffff80646787>] bit_wait_io+0x27/0x50
 [<ffffffff80646a7d>] __wait_on_bit+0x5d/0x90
 [<ffffffff803bf160>] ? generic_make_request+0xc0/0x100
 [<ffffffff80646760>] ? bit_wait_timeout+0x70/0x70
 [<ffffffff80646bc3>] out_of_line_wait_on_bit+0x73/0x90
 [<ffffffff8028f680>] ? wake_atomic_t_function+0x40/0x40
 [<ffffffff8034b60f>] __wait_on_buffer+0x3f/0x50
 [<ffffffff8034df18>] __bread_gfp+0xa8/0xd0
 [<ffffffff80388d45>] ext3_get_branch+0x95/0x140
 [<ffffffff80389716>] ext3_get_blocks_handle+0xb6/0xca0
 [<ffffffff8029760c>] ? __lock_acquire+0x50c/0xc30
 [<ffffffff803114b2>] ? __slab_alloc+0x212/0x560
 [<ffffffff80294315>] ? trace_hardirqs_on_caller+0x105/0x1d0
 [<ffffffff8038a3a8>] ext3_get_block+0xa8/0x100
 [<ffffffff80349bba>] generic_block_bmap+0x3a/0x40
 [<ffffffff8038956d>] ext3_bmap+0x7d/0x90
 [<ffffffff80333e2c>] bmap+0x1c/0x20
 [<ffffffff8039ee70>] journal_bmap+0x30/0xa0
 [<ffffffff8039f238>] journal_next_log_block+0x78/0xa0
 [<ffffffff8039a637>] journal_commit_transaction+0x657/0x13e0
 [<ffffffff802aaa87>] ? lock_timer_base+0x37/0x70
 [<ffffffff802ab0c0>] ? get_next_timer_interrupt+0x240/0x240
 [<ffffffff8039e632>] kjournald+0xf2/0x210
 [<ffffffff8028f600>] ? woken_wake_function+0x10/0x10
 [<ffffffff8039e540>] ? commit_timeout+0x10/0x10
 [<ffffffff80279e2e>] kthread+0xee/0x120
 [<ffffffff80279d40>] ? __init_kthread_worker+0x70/0x70
 [<ffffffff8064b56c>] ret_from_fork+0x7c/0xb0
 [<ffffffff80279d40>] ? __init_kthread_worker+0x70/0x70
---[ end trace 27f081e879dfbb12 ]---


^ permalink raw reply

* [PATCH] drivers: md: remove duplication with time_after
From: Pierre Chevalier @ 2015-02-05 21:39 UTC (permalink / raw)
  To: agk; +Cc: linux-raid, linux-kernel, Pierre Chevalier

The logic that was expressed previously was equivalent of calling
time_after.
Remove duplication.

Signed-off-by: Pierre Chevalier <pierrechevalier83@gmail.com>
---
 drivers/md/dm-cache-target.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-cache-target.c b/drivers/md/dm-cache-target.c
index e165053..855e3b3 100644
--- a/drivers/md/dm-cache-target.c
+++ b/drivers/md/dm-cache-target.c
@@ -14,6 +14,7 @@
 #include <linux/init.h>
 #include <linux/mempool.h>
 #include <linux/module.h>
+#include <linux/jiffies.h>
 #include <linux/slab.h>
 #include <linux/vmalloc.h>
 
@@ -1562,8 +1563,7 @@ static void process_bio(struct cache *cache, struct prealloc *structs,
 
 static int need_commit_due_to_time(struct cache *cache)
 {
-	return jiffies < cache->last_commit_jiffies ||
-	       jiffies > cache->last_commit_jiffies + COMMIT_PERIOD;
+	return time_after(jiffies, cache->last_commit_jiffies + COMMIT_PERIOD);
 }
 
 static int commit_if_needed(struct cache *cache)
-- 
2.2.2

^ permalink raw reply related

* Re: RAID1 might_sleep() warning on 3.19-rc7
From: NeilBrown @ 2015-02-05 21:51 UTC (permalink / raw)
  To: Tony Battersby; +Cc: linux-raid, Peter Zijlstra, lkml, axboe
In-Reply-To: <54D3D24E.5060303@cybernetics.com>

[-- Attachment #1: Type: text/plain, Size: 4996 bytes --]

On Thu, 05 Feb 2015 15:27:58 -0500 Tony Battersby <tonyb@cybernetics.com>
wrote:

> I get the might_sleep() warning below when writing some data to an ext3
> filesystem on a RAID1.  But everything works OK, so there is no actual
> problem, just a warning.
> 
> I see that there has been a fix for a might_sleep() warning in md/bitmap
> since 3.19-rc7, but this is a different warning.

Hi Tony,
 this is another false positive caused by 

commit 8eb23b9f35aae413140d3fda766a98092c21e9b0
Author: Peter Zijlstra <peterz@infradead.org>
Date:   Wed Sep 24 10:18:55 2014 +0200

    sched: Debug nested sleeps


It is even described in that commit:

    Another observed problem is calling a blocking function from
    schedule()->sched_submit_work()->blk_schedule_flush_plug() which will
    then destroy the task state for the actual __schedule() call that
    comes after it.

That is exactly what is happening here.  However I don't think that is an
"observed problem" but rather an "observed false-positive".

If nothing inside the outer loop blocks, then in particular
generic_make_request will not be called, so nothing will be added to the
queue that blk_schedule_flush_plug flushes.
So the first time through the loop, a call the 'schedule()' may not actually
block, but every subsequent time it will.
So there is no actual problem here.

So I'd be included to add sched_annotate_sleep() in blk_flush_plug_list().

Peter: what do you think is the best way to silence this warning.

Thanks,
NeilBrown



> 
> ---
> 
> > cat /proc/mdstat
> Personalities : [raid1]
> md0 : active raid1 sda1[0] sdb1[1]
>       1959884 blocks super 1.0 [2/2] [UU]
>       
> unused devices: <none>
> 
> ---
> 
> > grep md0 /proc/mounts
> /dev/md0 / ext3 rw,noatime,errors=continue,barrier=1,data=journal 0 0
> 
> ---
> 
> WARNING: CPU: 3 PID: 1069 at kernel/sched/core.c:7300 __might_sleep+0x82/0x90()
> do not call blocking ops when !TASK_RUNNING; state=2 set at [<ffffffff8028faa1>] prepare_to_wait+0x31/0xa0
> Modules linked in: iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi igb i2c_algo_bit ptp pps_core mptsas mptscsih mptbase pm80xx libsas mpt2sas scsi_transport_sas raid_class sg coretemp eeprom w83795 i2c_i801
> CPU: 3 PID: 1069 Comm: kjournald Not tainted 3.19.0-rc7 #1
> Hardware name: Supermicro X8DTH-i/6/iF/6F/X8DTH, BIOS 2.1b       05/04/12  
>  0000000000001c84 ffff88032f1df608 ffffffff80645918 0000000000001c84
>  ffff88032f1df658 ffff88032f1df648 ffffffff8025ea6b ffff8800bb0b4d58
>  0000000000000000 00000000000006f6 ffffffff80942b6f ffff8803317b8a00
> Call Trace:
>  [<ffffffff80645918>] dump_stack+0x4f/0x6f
>  [<ffffffff8025ea6b>] warn_slowpath_common+0x8b/0xd0
>  [<ffffffff8025eb51>] warn_slowpath_fmt+0x41/0x50
>  [<ffffffff8028faa1>] ? prepare_to_wait+0x31/0xa0
>  [<ffffffff8028faa1>] ? prepare_to_wait+0x31/0xa0
>  [<ffffffff8027ee62>] __might_sleep+0x82/0x90
>  [<ffffffff803bee06>] generic_make_request_checks+0x36/0x2d0
>  [<ffffffff802943ed>] ? trace_hardirqs_on+0xd/0x10
>  [<ffffffff803bf0b3>] generic_make_request+0x13/0x100
>  [<ffffffff8054983b>] raid1_unplug+0x12b/0x170
>  [<ffffffff803c1302>] blk_flush_plug_list+0xa2/0x230
>  [<ffffffff80294315>] ? trace_hardirqs_on_caller+0x105/0x1d0
>  [<ffffffff80646760>] ? bit_wait_timeout+0x70/0x70
>  [<ffffffff80646383>] io_schedule+0x43/0x80
>  [<ffffffff80646787>] bit_wait_io+0x27/0x50
>  [<ffffffff80646a7d>] __wait_on_bit+0x5d/0x90
>  [<ffffffff803bf160>] ? generic_make_request+0xc0/0x100
>  [<ffffffff80646760>] ? bit_wait_timeout+0x70/0x70
>  [<ffffffff80646bc3>] out_of_line_wait_on_bit+0x73/0x90
>  [<ffffffff8028f680>] ? wake_atomic_t_function+0x40/0x40
>  [<ffffffff8034b60f>] __wait_on_buffer+0x3f/0x50
>  [<ffffffff8034df18>] __bread_gfp+0xa8/0xd0
>  [<ffffffff80388d45>] ext3_get_branch+0x95/0x140
>  [<ffffffff80389716>] ext3_get_blocks_handle+0xb6/0xca0
>  [<ffffffff8029760c>] ? __lock_acquire+0x50c/0xc30
>  [<ffffffff803114b2>] ? __slab_alloc+0x212/0x560
>  [<ffffffff80294315>] ? trace_hardirqs_on_caller+0x105/0x1d0
>  [<ffffffff8038a3a8>] ext3_get_block+0xa8/0x100
>  [<ffffffff80349bba>] generic_block_bmap+0x3a/0x40
>  [<ffffffff8038956d>] ext3_bmap+0x7d/0x90
>  [<ffffffff80333e2c>] bmap+0x1c/0x20
>  [<ffffffff8039ee70>] journal_bmap+0x30/0xa0
>  [<ffffffff8039f238>] journal_next_log_block+0x78/0xa0
>  [<ffffffff8039a637>] journal_commit_transaction+0x657/0x13e0
>  [<ffffffff802aaa87>] ? lock_timer_base+0x37/0x70
>  [<ffffffff802ab0c0>] ? get_next_timer_interrupt+0x240/0x240
>  [<ffffffff8039e632>] kjournald+0xf2/0x210
>  [<ffffffff8028f600>] ? woken_wake_function+0x10/0x10
>  [<ffffffff8039e540>] ? commit_timeout+0x10/0x10
>  [<ffffffff80279e2e>] kthread+0xee/0x120
>  [<ffffffff80279d40>] ? __init_kthread_worker+0x70/0x70
>  [<ffffffff8064b56c>] ret_from_fork+0x7c/0xb0
>  [<ffffffff80279d40>] ? __init_kthread_worker+0x70/0x70
> ---[ end trace 27f081e879dfbb12 ]---


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

^ permalink raw reply

* [PATCH] drivers: md: md-thin: remove duplication with time_after
From: Pierre Chevalier @ 2015-02-05 21:58 UTC (permalink / raw)
  To: agk; +Cc: linux-raid, linux-kernel, Pierre Chevalier

The logic that was expressed previously was equivalent of calling
time_after.
Remove duplication.

Signed-off-by: Pierre Chevalier <pierrechevalier83@gmail.com>
---
 drivers/md/dm-thin.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
index 07705ee..c4d319b 100644
--- a/drivers/md/dm-thin.c
+++ b/drivers/md/dm-thin.c
@@ -11,6 +11,7 @@
 #include <linux/device-mapper.h>
 #include <linux/dm-io.h>
 #include <linux/dm-kcopyd.h>
+#include <linux/jiffies.h>
 #include <linux/log2.h>
 #include <linux/list.h>
 #include <linux/rculist.h>
@@ -1700,8 +1701,7 @@ static void process_cell_fail(struct thin_c *tc, struct dm_bio_prison_cell *cell
  */
 static int need_commit_due_to_time(struct pool *pool)
 {
-	return jiffies < pool->last_commit_jiffies ||
-	       jiffies > pool->last_commit_jiffies + COMMIT_PERIOD;
+	return time_after(jiffies, pool->last_commit_jiffies + COMMIT_PERIOD);
 }
 
 #define thin_pbd(node) rb_entry((node), struct dm_thin_endio_hook, rb_node)
-- 
2.2.2

^ permalink raw reply related

* Re: md: 'array_size' sysfs attribute
From: NeilBrown @ 2015-02-05 22:34 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: dan.j.williams, linux-raid
In-Reply-To: <20150205110237.GA7933@mwanda>

[-- Attachment #1: Type: text/plain, Size: 4419 bytes --]

On Thu, 5 Feb 2015 14:02:37 +0300 Dan Carpenter <dan.carpenter@oracle.com>
wrote:

> Hello Dan Williams,
> 
> The patch b522adcde9c4: "md: 'array_size' sysfs attribute" from Mar
> 31, 2009, leads to the following static checker warning:
> 
> 	drivers/md/md.c:5069 md_run()
> 	error: we previously assumed 'mddev->pers' could be null (see line 4936)
> 
> This code is really old.  I don't know why my stupid scripts are marking
> it as a new warning. 

Probably because it really is a new warning.  The problem is that your
"stupid scripts" (and we need more like them!!) are identifying the wrong
commit.

commit 516253a32c7abf3bc5754b1210106173ed191f7c
Author: NeilBrown <neilb@suse.de>
Date:   Mon Dec 15 12:56:58 2014 +1100

    md: protect ->pers changes with mddev->lock

is the guilty party (only in -next at present).  It delays the setting of
mddev->pers, but doesn't change all intermediate uses for mddev->pers into
pers.

I've just merged:
-		       (unsigned long long)mddev->pers->size(mddev, 0, 0) / 2);
+		       (unsigned long long)pers->size(mddev, 0, 0) / 2);

into that patch.


>                       We don't set "mddev->pers" to non-NULL until the
> end of the function so it looks like a real bug and that "pers->size"
> was intended.  When I fix that bug then it un-silences this warning:
> 
> 	drivers/md/md.c:5080 md_run()
> 	error: we previously assumed 'mddev->pers' could be null (see line 4936)
> 
> And that also is a real bug, but I'm not sure the right fix for that.
> 
> Basically, it's bugs all the way down from the code, to the fix, to the
> static checker.  *sigh*.
> 
> drivers/md/md.c
>   5060          err = pers->run(mddev);
>   5061          if (err)
>   5062                  printk(KERN_ERR "md: pers->run() failed ...\n");
>   5063          else if (pers->size(mddev, 0, 0) < mddev->array_sectors) {
>   5064                  WARN_ONCE(!mddev->external_size, "%s: default size too small,"
>   5065                            " but 'external_size' not in effect?\n", __func__);
>   5066                  printk(KERN_ERR
>   5067                         "md: invalid array_size %llu > default size %llu\n",
>   5068                         (unsigned long long)mddev->array_sectors / 2,
>   5069                         (unsigned long long)mddev->pers->size(mddev, 0, 0) / 2);
>                                                    ^^^^^^^^^^^^^^^^^^
> This should be "pers->size()".
> 
>   5070                  err = -EINVAL;
>   5071          }
>   5072          if (err == 0 && pers->sync_request &&
>   5073              (mddev->bitmap_info.file || mddev->bitmap_info.offset)) {
>   5074                  err = bitmap_create(mddev);
>   5075                  if (err)
>   5076                          printk(KERN_ERR "%s: failed to create bitmap (%d)\n",
>   5077                                 mdname(mddev), err);
>   5078          }
>   5079          if (err) {
>   5080                  mddev_detach(mddev);
>                         ^^^^^^^^^^^^^^^^^^^
> mddev_detach() will oops if mddev->pers() is not set.  We could set
> mdev->pers earlier, I suppose.

mddev_detach doesn't really need ->pers in this case.  I've merged:

@@ -5134,7 +5134,7 @@ static void mddev_detach(struct mddev *mddev)
 		wait_event(bitmap->behind_wait,
 			   atomic_read(&bitmap->behind_writes) == 0);
 	}
-	if (mddev->pers->quiesce) {
+	if (mddev->pers && mddev->pers->quiesce) {
 		mddev->pers->quiesce(mddev, 1);
 		mddev->pers->quiesce(mddev, 0);
 	}


Thanks a lot to you and your static checker!!

NeilBrown

> 
>   5081                  pers->free(mddev, mddev->private);
>   5082                  module_put(pers->owner);
>   5083                  bitmap_destroy(mddev);
>   5084                  return err;
>   5085          }
>   5086          if (mddev->queue) {
>   5087                  mddev->queue->backing_dev_info.congested_data = mddev;
>   5088                  mddev->queue->backing_dev_info.congested_fn = md_congested;
>   5089                  blk_queue_merge_bvec(mddev->queue, md_mergeable_bvec);
>   5090          }
> 
> regards,
> dan carpenter
> --
> 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: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

^ permalink raw reply

* Re: [PATCH 00/24] Clustered MD RAID1
From: NeilBrown @ 2015-02-06  2:39 UTC (permalink / raw)
  To: Goldwyn Rodrigues; +Cc: lzhong, linux-raid
In-Reply-To: <20141218161456.GA29504@shrek.lan>

[-- Attachment #1: Type: text/plain, Size: 3977 bytes --]

On Thu, 18 Dec 2014 10:14:57 -0600 Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:

> Hello,
> 
> This is an attempt to make MD-RAID cluster-aware. The advantage of
> redundancy can help highly available systems to improve uptime.
> Currently, the implementation is limited to RAID1 but with further work
> (and some positive feedback), we could extend this to other compatible
> RAID scenarios.
> 
> The design document (first patch) is pretty descriptive of how
> the md has been made cluster-aware and how DLM is used to safeguard data
> and communication.
> 
> This work requires some patches to the mdadm tool [1]
> 
> A quick howto:
> 
> 1. With your corosync/pacemaker based cluster running execute:
> # mdadm --create md0 --bitmap=clustered --raid-devices=2 --level=mirror --assume-clean /dev/sda /dev/sdb
> 
> 2. On other nodes, issue:
> # mdadm --assemble md0 /dev/sda /dev/sdb
> 
> References:
> [1] mdadm tool changes: https://github.com/goldwynr/mdadm branch:cluster-md
> [2] Patches against stable 3.14: https://github.com/goldwynr/linux branch: cluster-md-devel
> 
> Regards,
> 


hi Goldwyn,
 thanks for these - and sorry for the long delay.   Lots of leave over
 southern summer, and the lots of email etc to deal with.

This patch set is very close and I am tempted to just apply it and then
fix things up with subsequent patches.  In order to allow that, could you
please:
  - rebase against current upstream
  - fix the checkpatch.pl errors and warnings.
    The "WARNING: line over 80 characters" are often a judgement call
    so I'm not particularly worried about those.  Most, if not all, of
    the others should be followed just to have consistent layout.

Then I'll queue them up for 3.21, providing I don't find anything that would
hurt non-cluster usage ....
 On that topic: why initialise rv to -EINVAL in "metadata_update sends
 message...".  That looks wrong.

I noticed that a number of times a patch will revert something that a
previous patch added.  It would be much nicer to fold these changes back into
the original patch.  Often this is just extra blank lines, but occasionally
variable names are changed (md -> mddev).  It should be given the final name
when introduced.  Every chunk in every patch should be directly relevant to
that patch.

Some other issues, that could possibly be fixed up afterwards:

 - Is a clustername 64 bytes or 63 bytes?  I would have thought 64,
   but the use of strlcpy make is 63 plus a nul.  Is that really what is
   wanted?

 - Based on https://lkml.org/lkml/2012/10/23/580 it might be good to add
   "default n" to Kconfig, and possible add a WARN() if anyone tries to use
   the code.

 - I'm a bit concerned about the behaviour on node failure.
   When a node fails, two things must happen w.r.t the bits in that node's
   bitmap.
   1/ The corresponding regions of the array need to be resynced.  You do have
      code to do this.
   2/ Other nodes must avoid read-balancing on those regions until the
      resync has completed.  

   You do have code for this second bit, but it looks wrong.  It avoids
   read-balancing if ->area_resyncing().  That isn't sufficient.
   The "area_resyncing" is always (I assume) a relatively small region of
   the array which will be completely resynced quite quickly.  It must be
   because writes are blocked to this area.  However the region in which
   we must disable re-balancing can be much larger.  It covers *all* bits
   that are set in any unsynced bitmap.  So it isn't just the area that is
   currently being synced, but all areas that will be synced.

 - I think md_reload_sb() might be too simple.  It probably should check that
   nothing serious has changed.  The "mddev->raid_disks = 0" look suspicious.
   I'll have to think about this a bit more.

That's all I can see for now.  I'll have another look once I have it all in my tree.

Thanks,
NeilBrown

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

^ permalink raw reply

* Re: RAID1 might_sleep() warning on 3.19-rc7
From: Peter Zijlstra @ 2015-02-06 11:39 UTC (permalink / raw)
  To: NeilBrown; +Cc: Tony Battersby, linux-raid, lkml, axboe, Linus Torvalds
In-Reply-To: <20150206085133.2c1ab892@notabene.brown>

On Fri, Feb 06, 2015 at 08:51:33AM +1100, NeilBrown wrote:
> That is exactly what is happening here.  However I don't think that is an
> "observed problem" but rather an "observed false-positive".
> 
> If nothing inside the outer loop blocks, then in particular
> generic_make_request will not be called, so nothing will be added to the
> queue that blk_schedule_flush_plug flushes.
> So the first time through the loop, a call the 'schedule()' may not actually
> block, but every subsequent time it will.
> So there is no actual problem here.
> 
> So I'd be included to add sched_annotate_sleep() in blk_flush_plug_list().
> 
> Peter: what do you think is the best way to silence this warning.

> > Call Trace:

> >  [<ffffffff8027ee62>] __might_sleep+0x82/0x90
> >  [<ffffffff803bee06>] generic_make_request_checks+0x36/0x2d0
> >  [<ffffffff803bf0b3>] generic_make_request+0x13/0x100
> >  [<ffffffff8054983b>] raid1_unplug+0x12b/0x170
> >  [<ffffffff803c1302>] blk_flush_plug_list+0xa2/0x230
> >  [<ffffffff80646383>] io_schedule+0x43/0x80
> >  [<ffffffff80646787>] bit_wait_io+0x27/0x50

Well, I don't know. I don't particularly like the whole blk_flush_plug()
thing scheduling while on its way to schedule. If you ever end up
calling io_schedule() from it there's 'fun'.

Also, how likely is it to actually schedule when doing all that? This
block layer stuff is somewhat impenetrable for me, too many callbacks.

You have some words on how its unlikely, but I can't even find _where_
it would schedule :/ All I see is a loop calling ->make_request_fn() and
god only knows where that ends up.

So there appear to be two blk_flush_plug() variants, one with an
@from_schedule = true, which seems to really try not to schedule, which
seems to suggest the 'false' one (the one above) is meant to schedule?

If scheduling is the rule rather than the exception, the above is
properly broken.

But again, I don't know.

If you're confident that scheduling is rare for _ALL_ (current and
future) block device implementations, not just the raid one, then you
can annotate blk_flush_plug_list() I suppose.

Otherwise I would suggest adding them one at a time in whatever blk
device thing likes to go schedule on us. Also, add a comment that
explains why its rare for the future us who need to look at it again.

^ permalink raw reply

* Re: [PATCH 3/3] dm ioctl: Delete an unnecessary check before the function call "dm_table_destroy"
From: Mike Snitzer @ 2015-02-06 21:12 UTC (permalink / raw)
  To: SF Markus Elfring
  Cc: Alasdair Kergon, Neil Brown, dm-devel, linux-raid, Julia Lawall,
	kernel-janitors, LKML
In-Reply-To: <54CF9666.5060407@users.sourceforge.net>

On Mon, Feb 02 2015 at 10:23am -0500,
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 2 Feb 2015 15:30:37 +0100
> 
> The dm_table_destroy() function tests whether its argument is NULL and then
> returns immediately. Thus the test around the call is not needed.
> 
> This issue was detected by using the Coccinelle software.

Your proposed patch (while technically correct) hurts code clarity.

Nack.

^ permalink raw reply

* md resync ignoring unreadable sectors
From: Roman Mamedov @ 2015-02-07 21:47 UTC (permalink / raw)
  To: linux-raid

Hello,

I've got some bad sectors on one drive:

dd: reading `/dev/sdh1': Input/output error
260200+0 records in
260200+0 records out
133222400 bytes (133 MB) copied, 2.97188 s, 44.8 MB/s

[ 3908.350331] ata9.00: exception Emask 0x0 SAct 0x40000 SErr 0x0 action 0x0
[ 3908.350385] ata9.00: irq_stat 0x40000008
[ 3908.350427] ata9.00: failed command: READ FPDMA QUEUED
[ 3908.350474] ata9.00: cmd 60/06:90:6a:00:04/00:00:00:00:00/40 tag 18 ncq 3072 in
[ 3908.350474]          res 51/40:06:6a:00:04/00:00:00:00:00/40 Emask 0x409 (media error) <F>
[ 3908.350628] ata9.00: status: { DRDY ERR }
[ 3908.350669] ata9.00: error: { UNC }
[ 3908.354643] ata9.00: configured for UDMA/133
[ 3908.354664] sd 8:0:0:0: [sdh] Unhandled sense code
[ 3908.354668] sd 8:0:0:0: [sdh]  
[ 3908.354671] Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
[ 3908.354674] sd 8:0:0:0: [sdh]  
[ 3908.354677] Sense Key : Medium Error [current] [descriptor]
[ 3908.354681] Descriptor sense data with sense descriptors (in hex):
[ 3908.354683]         72 03 11 04 00 00 00 0c 00 0a 80 00 00 00 00 00 
[ 3908.354695]         00 04 00 6a 
[ 3908.354701] sd 8:0:0:0: [sdh]  
[ 3908.354705] Add. Sense: Unrecovered read error - auto reallocate failed
[ 3908.354708] sd 8:0:0:0: [sdh] CDB: 
[ 3908.354710] Read(10): 28 00 00 04 00 6a 00 00 06 00
[ 3908.354721] end_request: I/O error, dev sdh, sector 262250
[ 3908.354773] Buffer I/O error on device sdh1, logical block 260202
[ 3908.354825] Buffer I/O error on device sdh1, logical block 260203
[ 3908.354891] Buffer I/O error on device sdh1, logical block 260204
[ 3908.354942] Buffer I/O error on device sdh1, logical block 260205
[ 3908.354992] Buffer I/O error on device sdh1, logical block 260206
[ 3908.355042] Buffer I/O error on device sdh1, logical block 260207
[ 3908.355125] ata9: EH complete

Generally I believe these should go away when overwritten, but how do I
overwrite them? The drive is an md RAID1 member:

/dev/md4:
        Version : 1.2
  Creation Time : Mon May 26 13:40:18 2014
     Raid Level : raid1
     Array Size : 1953379936 (1862.89 GiB 2000.26 GB)
  Used Dev Size : 1953379936 (1862.89 GiB 2000.26 GB)
   Raid Devices : 2
  Total Devices : 2
    Persistence : Superblock is persistent

  Intent Bitmap : Internal

    Update Time : Sun Feb  8 02:39:58 2015
          State : active 
 Active Devices : 2
Working Devices : 2
 Failed Devices : 0
  Spare Devices : 0

           Name : natsu.romanrm.net:4  (local to host natsu.romanrm.net)
           UUID : 3b8c3166:073249b5:e1384bd6:4611df90
         Events : 50426

    Number   Major   Minor   RaidDevice State
       0       8       49        0      active sync   /dev/sdd1
       1       8      113        1      active sync   /dev/sdh1

I thought I would run a 'check' or 'repair', this will read from both drives,
fail to read from sdh, then try to overwrite the affected areas on sdh. But
nope:

# echo 0 > /sys/block/md4/md/sync_min 
# echo check > /sys/block/md4/md/sync_action 

[ 4059.451036] md: data-check of RAID array md4
[ 4059.451040] md: minimum _guaranteed_  speed: 1000 KB/sec/disk.
[ 4059.451042] md: using maximum available idle IO bandwidth (but not more than 200000 KB/sec) for data-check.
[ 4059.451046] md: using 128k window, over a total of 1953379936k.

This happily proceeds through the supposedly unreadable area:

md4 : active raid1 sdd1[0] sdh1[1]
      1953379936 blocks super 1.2 [2/2] [UU]
      [>....................]  check =  0.0% (1479680/1953379936) finish=1116.8min speed=29128K/sec
      bitmap: 2/8 pages [8KB], 131072KB chunk

at 1.5GB already, while the unreadable sectors are at ~133MB. And no new ATA
errors in dmesg. How is this possible?

If I retry the 'dd' command right now, it fails exactly in the same way as
before (and ATA errors do indeed appear).

-- 
With respect,
Roman

^ permalink raw reply

* Re: md resync ignoring unreadable sectors
From: Eyal Lebedinsky @ 2015-02-07 22:39 UTC (permalink / raw)
  To: linux-raid
In-Reply-To: <20150208024745.01b9d3ff@natsu>

On 08/02/15 08:47, Roman Mamedov wrote:
> Hello,
>
> I've got some bad sectors on one drive:
>
> dd: reading `/dev/sdh1': Input/output error
> 260200+0 records in
> 260200+0 records out
> 133222400 bytes (133 MB) copied, 2.97188 s, 44.8 MB/s
>
> [ 3908.350331] ata9.00: exception Emask 0x0 SAct 0x40000 SErr 0x0 action 0x0
> [ 3908.350385] ata9.00: irq_stat 0x40000008
> [ 3908.350427] ata9.00: failed command: READ FPDMA QUEUED
> [ 3908.350474] ata9.00: cmd 60/06:90:6a:00:04/00:00:00:00:00/40 tag 18 ncq 3072 in
> [ 3908.350474]          res 51/40:06:6a:00:04/00:00:00:00:00/40 Emask 0x409 (media error) <F>
> [ 3908.350628] ata9.00: status: { DRDY ERR }
> [ 3908.350669] ata9.00: error: { UNC }
> [ 3908.354643] ata9.00: configured for UDMA/133
> [ 3908.354664] sd 8:0:0:0: [sdh] Unhandled sense code
> [ 3908.354668] sd 8:0:0:0: [sdh]
> [ 3908.354671] Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
> [ 3908.354674] sd 8:0:0:0: [sdh]
> [ 3908.354677] Sense Key : Medium Error [current] [descriptor]
> [ 3908.354681] Descriptor sense data with sense descriptors (in hex):
> [ 3908.354683]         72 03 11 04 00 00 00 0c 00 0a 80 00 00 00 00 00
> [ 3908.354695]         00 04 00 6a
> [ 3908.354701] sd 8:0:0:0: [sdh]
> [ 3908.354705] Add. Sense: Unrecovered read error - auto reallocate failed
> [ 3908.354708] sd 8:0:0:0: [sdh] CDB:
> [ 3908.354710] Read(10): 28 00 00 04 00 6a 00 00 06 00
> [ 3908.354721] end_request: I/O error, dev sdh, sector 262250
> [ 3908.354773] Buffer I/O error on device sdh1, logical block 260202
> [ 3908.354825] Buffer I/O error on device sdh1, logical block 260203
> [ 3908.354891] Buffer I/O error on device sdh1, logical block 260204
> [ 3908.354942] Buffer I/O error on device sdh1, logical block 260205
> [ 3908.354992] Buffer I/O error on device sdh1, logical block 260206
> [ 3908.355042] Buffer I/O error on device sdh1, logical block 260207
> [ 3908.355125] ata9: EH complete
>
> Generally I believe these should go away when overwritten, but how do I
> overwrite them? The drive is an md RAID1 member:
>
> /dev/md4:
>          Version : 1.2
>    Creation Time : Mon May 26 13:40:18 2014
>       Raid Level : raid1
>       Array Size : 1953379936 (1862.89 GiB 2000.26 GB)
>    Used Dev Size : 1953379936 (1862.89 GiB 2000.26 GB)
>     Raid Devices : 2
>    Total Devices : 2
>      Persistence : Superblock is persistent
>
>    Intent Bitmap : Internal
>
>      Update Time : Sun Feb  8 02:39:58 2015
>            State : active
>   Active Devices : 2
> Working Devices : 2
>   Failed Devices : 0
>    Spare Devices : 0
>
>             Name : natsu.romanrm.net:4  (local to host natsu.romanrm.net)
>             UUID : 3b8c3166:073249b5:e1384bd6:4611df90
>           Events : 50426
>
>      Number   Major   Minor   RaidDevice State
>         0       8       49        0      active sync   /dev/sdd1
>         1       8      113        1      active sync   /dev/sdh1
>
> I thought I would run a 'check' or 'repair', this will read from both drives,
> fail to read from sdh, then try to overwrite the affected areas on sdh. But
> nope:
>
> # echo 0 > /sys/block/md4/md/sync_min
> # echo check > /sys/block/md4/md/sync_action
>
> [ 4059.451036] md: data-check of RAID array md4
> [ 4059.451040] md: minimum _guaranteed_  speed: 1000 KB/sec/disk.
> [ 4059.451042] md: using maximum available idle IO bandwidth (but not more than 200000 KB/sec) for data-check.
> [ 4059.451046] md: using 128k window, over a total of 1953379936k.
>
> This happily proceeds through the supposedly unreadable area:
>
> md4 : active raid1 sdd1[0] sdh1[1]
>        1953379936 blocks super 1.2 [2/2] [UU]
>        [>....................]  check =  0.0% (1479680/1953379936) finish=1116.8min speed=29128K/sec
>        bitmap: 2/8 pages [8KB], 131072KB chunk
>
> at 1.5GB already, while the unreadable sectors are at ~133MB. And no new ATA
> errors in dmesg. How is this possible?
>
> If I retry the 'dd' command right now, it fails exactly in the same way as
> before (and ATA errors do indeed appear).

Hi,

I had a similar situation. In my case the bad sectors fell in an unused control area, part of the header,
which is not read (or written) by the md normally or by the sync.

The error did not show up during normal operation (or during scrub), only during the smartctl long test.
What triggered the error for you?

I looked up the size of the different parts of the RAID to arrive at that conclusion. Dumping the sectors
around the bad area also showed it to be all zeroes.

I ended up directly zeroing the bad sectors (hdparm --repair-sector ...).

YMMV

-- 
Eyal Lebedinsky (eyal@eyal.emu.id.au)

^ permalink raw reply

* Re: md resync ignoring unreadable sectors
From: Roman Mamedov @ 2015-02-07 23:04 UTC (permalink / raw)
  To: Eyal Lebedinsky; +Cc: linux-raid
In-Reply-To: <54D69433.9060605@eyal.emu.id.au>

On Sun, 08 Feb 2015 09:39:47 +1100
Eyal Lebedinsky <eyal@eyal.emu.id.au> wrote:

> The error did not show up during normal operation (or during scrub), only during the smartctl long test.
> What triggered the error for you?

Just appeared during boot-up after a reboot (after 50 days uptime) which was
performed for some hardware upgrades (RAM, SATA controller). The error doesn't
go away after swapping the SATA controller for different one.

> I looked up the size of the different parts of the RAID to arrive at that conclusion. Dumping the sectors
> around the bad area also showed it to be all zeroes.

I wouldn't expect mdadm to have any headers or unused areas as far as 133 MB
into a RAID member.

-- 
With respect,
Roman

^ permalink raw reply

* Re: md resync ignoring unreadable sectors
From: Phil Turmel @ 2015-02-07 23:42 UTC (permalink / raw)
  To: Roman Mamedov, Eyal Lebedinsky; +Cc: linux-raid
In-Reply-To: <20150208040438.156a8554@natsu>

Hi Roman,

On 02/07/2015 06:04 PM, Roman Mamedov wrote:
> On Sun, 08 Feb 2015 09:39:47 +1100
> Eyal Lebedinsky <eyal@eyal.emu.id.au> wrote:

>> I looked up the size of the different parts of the RAID to arrive at that conclusion. Dumping the sectors
>> around the bad area also showed it to be all zeroes.
> 
> I wouldn't expect mdadm to have any headers or unused areas as far as 133 MB
> into a RAID member.

Look at mdadm -E for that drive and your partition start sector.  I bet
Eyal is right.  Latest mdadm gives me a 128MB data offset.

Phil


^ permalink raw reply

* Re: md resync ignoring unreadable sectors
From: Eyal Lebedinsky @ 2015-02-07 23:43 UTC (permalink / raw)
  Cc: linux-raid
In-Reply-To: <20150208040438.156a8554@natsu>

On 08/02/15 10:04, Roman Mamedov wrote:
> On Sun, 08 Feb 2015 09:39:47 +1100
> Eyal Lebedinsky <eyal@eyal.emu.id.au> wrote:
>
>> The error did not show up during normal operation (or during scrub), only during the smartctl long test.
>> What triggered the error for you?
>
> Just appeared during boot-up after a reboot (after 50 days uptime) which was
> performed for some hardware upgrades (RAM, SATA controller). The error doesn't
> go away after swapping the SATA controller for different one.
>
>> I looked up the size of the different parts of the RAID to arrive at that conclusion. Dumping the sectors
>> around the bad area also showed it to be all zeroes.
>
> I wouldn't expect mdadm to have any headers or unused areas as far as 133 MB
> into a RAID member.

Roman,

You may want to read
	https://raid.wiki.kernel.org/index.php/RAID_superblock_formats

For me:

# parted -l
...
Number  Start   End     Size    File system  Name  Flags
  1      1049kB  4001GB  4001GB
...

# mdadm --examine /dev/sdc1
...
   Data Offset : 262144 sectors
...

With required alignment etc. it may reach your 133MB.

cheers

-- 
Eyal Lebedinsky (eyal@eyal.emu.id.au)

^ permalink raw reply

* Re: md resync ignoring unreadable sectors
From: Roman Mamedov @ 2015-02-07 23:49 UTC (permalink / raw)
  To: Phil Turmel; +Cc: Eyal Lebedinsky, linux-raid
In-Reply-To: <54D6A2CC.3050102@turmel.org>

On Sat, 07 Feb 2015 18:42:04 -0500
Phil Turmel <philip@turmel.org> wrote:

> Hi Roman,
> 
> On 02/07/2015 06:04 PM, Roman Mamedov wrote:
> > On Sun, 08 Feb 2015 09:39:47 +1100
> > Eyal Lebedinsky <eyal@eyal.emu.id.au> wrote:
> 
> >> I looked up the size of the different parts of the RAID to arrive at that conclusion. Dumping the sectors
> >> around the bad area also showed it to be all zeroes.
> > 
> > I wouldn't expect mdadm to have any headers or unused areas as far as 133 MB
> > into a RAID member.
> 
> Look at mdadm -E for that drive and your partition start sector.  I bet
> Eyal is right.  Latest mdadm gives me a 128MB data offset.

Oh indeed:

    Data Offset : 262144 sectors

The unreadable area was at 260200 sectors.

Thanks

-- 
With respect,
Roman

^ permalink raw reply

* Re: md resync ignoring unreadable sectors
From: Eyal Lebedinsky @ 2015-02-07 23:49 UTC (permalink / raw)
  Cc: linux-raid
In-Reply-To: <20150208040438.156a8554@natsu>

On 08/02/15 10:04, Roman Mamedov wrote:
> On Sun, 08 Feb 2015 09:39:47 +1100
> Eyal Lebedinsky <eyal@eyal.emu.id.au> wrote:
>
>> The error did not show up during normal operation (or during scrub), only during the smartctl long test.
>> What triggered the error for you?
>
> Just appeared during boot-up after a reboot (after 50 days uptime) which was
> performed for some hardware upgrades (RAM, SATA controller). The error doesn't
> go away after swapping the SATA controller for different one.
>
>> I looked up the size of the different parts of the RAID to arrive at that conclusion. Dumping the sectors
>> around the bad area also showed it to be all zeroes.
>
> I wouldn't expect mdadm to have any headers or unused areas as far as 133 MB
> into a RAID member.
>

You can also read the earlier discussion starting 26/Feb/14:
	Subject: how to handle bad sectors in md control areas?

cheers

-- 
Eyal Lebedinsky (eyal@eyal.emu.id.au)

^ permalink raw reply

* Re: [PATCH 3/3] dm ioctl: Delete an unnecessary check before the function call "dm_table_destroy"
From: SF Markus Elfring @ 2015-02-08  9:55 UTC (permalink / raw)
  To: Mike Snitzer
  Cc: Alasdair Kergon, Neil Brown, dm-devel, linux-raid, Julia Lawall,
	kernel-janitors, LKML
In-Reply-To: <20150206211234.GA14803@redhat.com>

> Your proposed patch (while technically correct) hurts code clarity.

How many source code readability and understanding challenges does each
additional condition check cause?

Can the affected place become also a bit more efficient?

Regards,
Markus

^ permalink raw reply

* Re: [PATCH 3/3] dm ioctl: Delete an unnecessary check before the function call "dm_table_destroy"
From: Mike Snitzer @ 2015-02-08 14:02 UTC (permalink / raw)
  To: SF Markus Elfring
  Cc: Alasdair Kergon, Neil Brown, device-mapper development,
	linux-raid@vger.kernel.org, Julia Lawall, kernel-janitors, LKML
In-Reply-To: <54D7329C.206@users.sourceforge.net>

On Sun, Feb 8, 2015 at 4:55 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>> Your proposed patch (while technically correct) hurts code clarity.
>
> How many source code readability and understanding challenges does each
> additional condition check cause?

Please don't make a mountain out of a mole hill in an attempt to
defend your robotic patch (I'm quite tired of some of these static
analyzer patch submissions).

FYI, I did stage your other patch for 3.20, see:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-for-3.20&id=d0ce7e911c97c7c6df1081dcedfefced82a0c6bf

> Can the affected place become also a bit more efficient?

Efficiency isn't a concern in this instance (it isn't a hot IO path).
And even if it were, a branch (with current code) is more efficient vs
a a jump + branch (your proposed patch) -- in the case that no active
table exists.  Now if it likely that old_map does exist then yes your
patch is always a very slight win.

But given the duality of the calling function (deals with loading a
new map and destroying the old map if it exists) I prefer to keep the
code as is.  Sorry.

^ permalink raw reply

* An old "write-mostly" read balance issue
From: Dark Penguin @ 2015-02-08 15:40 UTC (permalink / raw)
  To: linux-raid

There is an old issue about RAID1 read-balancing when "write-mostly" 
disks are present.

The problem is, according to the manual, "md driver will avoid reading 
from these devices if at all possible".

One way to understand this statement is that these drives will never be 
read from, except when the main drive can not be read from. There are A 
LOT of situations when this is the expected and desired behaviour:
- People mirroring an SSD with an HDD and suffering a performance loss;
- People mirroring a fast HDD with a slow HDD for reliability, for 
example, mirroring a 300Gb WD Raptor to a 300Gb partition on a 3Tb 5900 
"green" drive for backup; since the larger drive may be used for 
something other than this RAID, many would prefer it to be spared the 
workload.
- In my case, I have a home RAID1 storage, which is idle 95% of the 
time, and 95% of the remaining 5% I only read from it. So I want one of 
the drives to spin down and never turn on, in order to avoid wearing 
down the mechanics. They say, "The best way to keep a device from 
breaking is to turn it off and not use it". :) But even if I simply 
retrieve the contents of my volume, that request is apparently enough to 
load the first drive to 100% for a split second, which causes the second 
drive to spin up, which is extremely undesirable.

I've spent a lot of time looking for the answer "why does it spin up", 
and "normal forum users" couldn not even help me, but then I found out 
that there is another way to read that statement: apparently, there are 
other people who would like to see whatever little benefit reading from 
the second drive could give them. I can not say which side is a 
majority, but I respect their wishes as well, and personally I'm fine 
with any default behaviour as long as I have what I need.

I've found a patch for that:
http://marc.info/?l=linux-raid&m=135982797322422
Apparently, it can be used with any kernel, but I'm not good enough to 
make sure nothing's broken everytime I upgrade the kernel, and frankly, 
I think there are A LOT of people who wish to see the behaviour I would 
expect. So my plea is for the developers to accept this patch and make 
this behaviour optional, if not default. At least give us a compile 
option to build the kernel this way! There are people out there who use 
RAID1 at home and not in production, and therefore care less about 
performance than home storage idling, and who understand the words "if 
at all possible" in the more obvious way! I think that's the whole 
reason why the "write-mostly" option is there in the first place, but if 
there are people who don't agree with me - I'm not going to argue, they 
can have it their way, just give us the option to do what we want, too!


-- 
darkpenguin

^ permalink raw reply

* Re: [PATCH 3/3] dm ioctl: Delete an unnecessary check before the function call "dm_table_destroy"
From: SF Markus Elfring @ 2015-02-08 15:52 UTC (permalink / raw)
  To: Mike Snitzer
  Cc: Alasdair Kergon, Neil Brown, device-mapper development,
	linux-raid@vger.kernel.org, Julia Lawall, kernel-janitors, LKML
In-Reply-To: <CAMM=eLfpAiTeqvX17C3agsLXP03UD_Q7DnmhGnO5GjEC0pZa8Q@mail.gmail.com>

> FYI, I did stage your other patch for 3.20, see:
> https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-for-3.20&id=d0ce7e911c97c7c6df1081dcedfefced82a0c6bf

Thanks for your acceptance of the suggested clean-up around
vfree() function calls at least.
Additional source code places can also be reconsidered at other times,
can't they?

Regards,
Markus

^ permalink raw reply

* Re: md resync ignoring unreadable sectors
From: John Stoffel @ 2015-02-08 17:23 UTC (permalink / raw)
  To: Roman Mamedov; +Cc: Eyal Lebedinsky, linux-raid
In-Reply-To: <20150208040438.156a8554@natsu>

>>>>> "Roman" == Roman Mamedov <rm@romanrm.net> writes:

Roman> On Sun, 08 Feb 2015 09:39:47 +1100
Roman> Eyal Lebedinsky <eyal@eyal.emu.id.au> wrote:

>> The error did not show up during normal operation (or during scrub), only during the smartctl long test.
>> What triggered the error for you?

Roman> Just appeared during boot-up after a reboot (after 50 days uptime) which was
Roman> performed for some hardware upgrades (RAM, SATA controller). The error doesn't
Roman> go away after swapping the SATA controller for different one.

>> I looked up the size of the different parts of the RAID to arrive at that conclusion. Dumping the sectors
>> around the bad area also showed it to be all zeroes.

Roman> I wouldn't expect mdadm to have any headers or unused areas as
Roman> far as 133 MB into a RAID member.

Roman,

I would immediately add in a third RAID1 member, wait for it to
resync, then pull out the bad drive and write zeros to the entire
drive to force any and all bad sectors to get over-written and
hopefully reallocated from good sectors.  

But I'd also treat the drive as suspect and replace it ASAP.  Keet it
around as a scratch drive, or a temp space area you don't care about
if you like, but not of important data if at all possible.

John

^ permalink raw reply

* raid md126, md127 problem after reboot, howto fix?
From: Marc Widmer @ 2015-02-08 19:03 UTC (permalink / raw)
  To: linux-raid

Hi List

I have no deep unterstand about raids, beside setting them up initially and
replacing disks if needed. So this error has never happened to me before:

After a reboot i have a really strange behaviour on my server. Disks are
not marked faulty, but raid is "fallend apart".

/proc/mdstat shows me:

md126 : active raid1 sda1[0]
       10485696 blocks [2/1] [U_]

md127 : active raid1 sda2[0]
       721558464 blocks [2/1] [U_]

md1 : active raid1 sdb1[1]
       10485696 blocks [2/1] [_U]

md2 : active raid1 sdb2[1]
       721558464 blocks [2/1] [_U]

wished would be something similar to:
md1 : active raid1 sdb1[1] sda1[0]
       10238912 blocks [2/2] [UU]

md2 : active raid1 sdb2[1] sda2[0]
       1942746048 blocks [2/2] [UU]

Currently only md1, md2 are running. nmon shows me, that only disks sdb is
active, sda is not doing anything.

I run debian squeeze.

I am a bit concerned what to do, because at the moment i run on one disk
only and if things go wrong i end up with a server not running (downtime)
and possible data loss (beside backups).

Any ideas what i should do? Howto put the raid back together, possibly in
live mode, without rebooting in rescue mode and risk long downtime?

Any help would be greatly appreciated as by now the only thing i had to do
was resyncing a disk after usual hd crash.

Best
marc

^ permalink raw reply

* Re: raid md126, md127 problem after reboot, howto fix?
From: Wols Lists @ 2015-02-08 21:29 UTC (permalink / raw)
  To: Marc Widmer, linux-raid
In-Reply-To: <54D7B317.5010809@gmail.com>

On 08/02/15 19:03, Marc Widmer wrote:
> Hi List
> 
> I have no deep unterstand about raids, beside setting them up initially and
> replacing disks if needed. So this error has never happened to me before:
> 
> After a reboot i have a really strange behaviour on my server. Disks are
> not marked faulty, but raid is "fallend apart".
> 
> /proc/mdstat shows me:
> 
> md126 : active raid1 sda1[0]
>       10485696 blocks [2/1] [U_]
> 
> md127 : active raid1 sda2[0]
>       721558464 blocks [2/1] [U_]
> 
> md1 : active raid1 sdb1[1]
>       10485696 blocks [2/1] [_U]
> 
> md2 : active raid1 sdb2[1]
>       721558464 blocks [2/1] [_U]
> 
> wished would be something similar to:
> md1 : active raid1 sdb1[1] sda1[0]
>       10238912 blocks [2/2] [UU]
> 
> md2 : active raid1 sdb2[1] sda2[0]
>       1942746048 blocks [2/2] [UU]
> 
> Currently only md1, md2 are running. nmon shows me, that only disks sdb is
> active, sda is not doing anything.
> 
> I run debian squeeze.

What version of mdadm are you running? 3.2.6 or thereabouts?
> 
> I am a bit concerned what to do, because at the moment i run on one disk
> only and if things go wrong i end up with a server not running (downtime)
> and possible data loss (beside backups).
> 
> Any ideas what i should do? Howto put the raid back together, possibly in
> live mode, without rebooting in rescue mode and risk long downtime?
> 
> Any help would be greatly appreciated as by now the only thing i had to do
> was resyncing a disk after usual hd crash.
> 
The reason I ask is this looks like a bug I had - if I'm right it's a
known problem and you need to upgrade mdadm.

Cheers,
Wol


^ permalink raw reply

* Re: RAID1 might_sleep() warning on 3.19-rc7
From: NeilBrown @ 2015-02-09  1:13 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: Tony Battersby, linux-raid, lkml, axboe, Linus Torvalds
In-Reply-To: <20150206113930.GK23123@twins.programming.kicks-ass.net>

[-- Attachment #1: Type: text/plain, Size: 4744 bytes --]

On Fri, 6 Feb 2015 12:39:30 +0100 Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, Feb 06, 2015 at 08:51:33AM +1100, NeilBrown wrote:
> > That is exactly what is happening here.  However I don't think that is an
> > "observed problem" but rather an "observed false-positive".
> > 
> > If nothing inside the outer loop blocks, then in particular
> > generic_make_request will not be called, so nothing will be added to the
> > queue that blk_schedule_flush_plug flushes.
> > So the first time through the loop, a call the 'schedule()' may not actually
> > block, but every subsequent time it will.
> > So there is no actual problem here.
> > 
> > So I'd be included to add sched_annotate_sleep() in blk_flush_plug_list().
> > 
> > Peter: what do you think is the best way to silence this warning.
> 
> > > Call Trace:
> 
> > >  [<ffffffff8027ee62>] __might_sleep+0x82/0x90
> > >  [<ffffffff803bee06>] generic_make_request_checks+0x36/0x2d0
> > >  [<ffffffff803bf0b3>] generic_make_request+0x13/0x100
> > >  [<ffffffff8054983b>] raid1_unplug+0x12b/0x170
> > >  [<ffffffff803c1302>] blk_flush_plug_list+0xa2/0x230
> > >  [<ffffffff80646383>] io_schedule+0x43/0x80
> > >  [<ffffffff80646787>] bit_wait_io+0x27/0x50
> 
> Well, I don't know. I don't particularly like the whole blk_flush_plug()
> thing scheduling while on its way to schedule. If you ever end up
> calling io_schedule() from it there's 'fun'.
> 
> Also, how likely is it to actually schedule when doing all that? This
> block layer stuff is somewhat impenetrable for me, too many callbacks.
> 
> You have some words on how its unlikely, but I can't even find _where_
> it would schedule :/ All I see is a loop calling ->make_request_fn() and
> god only knows where that ends up.
> 
> So there appear to be two blk_flush_plug() variants, one with an
> @from_schedule = true, which seems to really try not to schedule, which
> seems to suggest the 'false' one (the one above) is meant to schedule?
> 
> If scheduling is the rule rather than the exception, the above is
> properly broken.
> 
> But again, I don't know.

I had to re-read the code (And your analysis) a couple of times to be sure ...

As you say, when schedule() calls blk_schedule_flush_plug(), the
@from_schedule=true variant is used and the unplug code doesn't block.
So there is no problem there.

However, when io_schedule() explicitly calls blk_flush_plug(), then
@from_schedule=false variant is used, and the unplug functions are allowed to
allocate memory and block and maybe even call mempool_alloc() which might
call io_schedule().

This shouldn't be a problem as blk_flush_plug() spliced out the plug list, so
any recursive call will find an empty list and do nothing.

Worst case is that a wait_event loop that calls io_schedule() (i.e.
wait_on_bit_io()) might not block in the first call to io_schedule()
if the unplugging needed to wait.  Every subsequent call will block as
required as there is nothing else to add requests to the plug queue.

So as long as wait_on_bio_io() can cope with a single false wakeup (which it
can), there is no problem here.

> 
> If you're confident that scheduling is rare for _ALL_ (current and
> future) block device implementations, not just the raid one, then you
> can annotate blk_flush_plug_list() I suppose.
> 
> Otherwise I would suggest adding them one at a time in whatever blk
> device thing likes to go schedule on us. Also, add a comment that
> explains why its rare for the future us who need to look at it again.

It isn't that scheduling is "rare" - it is that it can only occur once in a
loop which doesn't expect it.

So I propose the following, though I haven't tested it.

Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index e628cb11b560..b0f12ab3df23 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -4374,6 +4374,11 @@ void __sched io_schedule(void)
 
 	delayacct_blkio_start();
 	atomic_inc(&rq->nr_iowait);
+	/* Any sleeping in blk_flush_plug() should not
+	 * trigger the "do not call blocking ops" warning
+	 * as it can only happen once in a wait_event loop.
+	 */
+	sched_annotate_sleep();
 	blk_flush_plug(current);
 	current->in_iowait = 1;
 	schedule();
@@ -4390,6 +4395,11 @@ long __sched io_schedule_timeout(long timeout)
 
 	delayacct_blkio_start();
 	atomic_inc(&rq->nr_iowait);
+	/* Any sleeping in blk_flush_plug() should not
+	 * trigger the "do not call blocking ops" warning
+	 * as it can only happen once in a wait_event loop.
+	 */
+	sched_annotate_sleep();
 	blk_flush_plug(current);
 	current->in_iowait = 1;
 	ret = schedule_timeout(timeout);


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox