On Monday February 25, gody@master.slon.net wrote:
> On Mon, 25 Feb 2002, Neil Brown wrote:
>
> > As he says, the patch is rather ugly and doesn't really address the
> > root problem. But if it works for you, that is good.
> >
> I think it's ugly because it puts some structures into raid5.c code, which
> should be accessible from structures already defined in the code
> (function).
>
> I do think, that patch is right as far as root of the problem is
> concerned.
I disagree. The patch just covers over the problem by making the
race substantially less likely to be lost.
You must remember that the raid5d could be running at any time,
especially on an SMP machine. So there much be some sort of locking
to make sure that rdev->sb is not freed while the super blocks are
being written out.
I have a patch, included below, which I think addresses the problem
most thoroughly. I would appreciate comments.
A particular feature is that rdev->sb is *not* freed wheneven ->faulty
is set to 1. Rather, the free is delayed until md_update_db is
called, which will be shortly after. This goes a long way to curing
the race.
I have lock/unlock_mddev at several places, so that any reconfiguring
of the mddev, is all under the reconfig_sem semaphore.
>
> What I don't understand is, why is ->faulty flag used all thru md.c when
> we have mark_disk_faulty(sb->disks+disk->number); and bitmaped status for
> the same reason. Are they diferent in any case, or is it the case, that
> structure mdp_disk_t used in disk_faulty is not accessible on those
> places.
There is a lot of this sort of duplication of information in the md
code. I did a bit of work to clean it up a while ago, but never
completed it. I should dig out that patch one day and try again.
NeilBrown
-------------------------------------------------------
Improve locking of md devices when configuration changes.
In particular, don't allow config changes while
writing out the superblocks as this can cause confusion.
Instead of freeing the per-rdev superblock in
md_error, which could be from interrupt, we free it when
writing out superblocks, under a lock. This
we can never get an unexpected NULL for rdev->sb.
Also, we get alloc_mddev to pre-lock and set reference count
on the md device.
When we ITERATE_MDDEV, we now normally lock each device
so that the code in the loop can operate safely.
We still need to lock the list of md devices though...
----------- Diffstat output ------------
./drivers/md/md.c | 57 +++++++++++++++++++++-----------------------
./drivers/md/multipath.c | 8 ++++--
./drivers/md/raid1.c | 8 ++++--
./drivers/md/raid5.c | 8 ++++--
./include/linux/raid/md_k.h | 15 ++++++++++-
5 files changed, 60 insertions(+), 36 deletions(-)
--- ./include/linux/raid/md_k.h 2002/02/27 01:12:21 1.1
+++ ./include/linux/raid/md_k.h 2002/02/27 04:57:36 1.2
@@ -309,7 +309,20 @@
tmp = tmp->next, tmp->prev != &all_mddevs \
; )
-static inline int lock_mddev (mddev_t * mddev)
+#define ITERATE_MDDEV_LOCK(mddev,tmp) \
+ \
+ for (tmp = all_mddevs.next; \
+ mddev = md_list_entry(tmp, mddev_t, all_mddevs), \
+ tmp = tmp->next, tmp->prev != &all_mddevs \
+ && (down(&mddev->reconfig_sem), 1) \
+ ; up(&mddev->reconfig_sem))
+
+static inline void lock_mddev (mddev_t * mddev)
+{
+ return down(&mddev->reconfig_sem);
+}
+
+static inline int lock_mddev_interruptible (mddev_t * mddev)
{
return down_interruptible(&mddev->reconfig_sem);
}
--- ./drivers/md/md.c 2002/02/27 00:49:36 1.1
+++ ./drivers/md/md.c 2002/02/27 04:57:36 1.2
@@ -199,12 +199,12 @@
memset(mddev, 0, sizeof(*mddev));
mddev->__minor = MINOR(dev);
- init_MUTEX(&mddev->reconfig_sem);
+ init_MUTEX_LOCKED(&mddev->reconfig_sem);
init_MUTEX(&mddev->recovery_sem);
init_MUTEX(&mddev->resync_sem);
MD_INIT_LIST_HEAD(&mddev->disks);
MD_INIT_LIST_HEAD(&mddev->all_mddevs);
- atomic_set(&mddev->active, 0);
+ atomic_set(&mddev->active, 1);
/*
* The 'base' mddev is the one with data NULL.
@@ -802,7 +802,7 @@
printk("md: **********************************\n");
printk("md: * <COMPLETE RAID STATE PRINTOUT> *\n");
printk("md: **********************************\n");
- ITERATE_MDDEV(mddev,tmp) {
+ ITERATE_MDDEV_LOCK(mddev,tmp) {
printk("md%d: ", mdidx(mddev));
ITERATE_RDEV(mddev,rdev,tmp2)
@@ -899,10 +899,6 @@
MD_BUG();
return 1;
}
- if (rdev->faulty) {
- MD_BUG();
- return 1;
- }
if (rdev->sb->md_magic != MD_SB_MAGIC) {
MD_BUG();
return 1;
@@ -1000,6 +996,11 @@
struct md_list_head *tmp;
mdk_rdev_t *rdev;
+ if (!down_trylock(&mddev->reconfig_sem)) {
+ up(&mddev->reconfig_sem);
+ BUG();
+ }
+
if (!mddev->sb_dirty) {
printk("hm, md_update_sb() called without ->sb_dirty == 1, from %p.\n", __builtin_return_address(0));
return 0;
@@ -1034,8 +1035,11 @@
err = 0;
ITERATE_RDEV(mddev,rdev,tmp) {
printk(KERN_INFO "md: ");
- if (rdev->faulty)
+ if (rdev->faulty) {
printk("(skipping faulty ");
+ if (rdev->sb)
+ free_disk_sb(rdev);
+ }
if (rdev->alias_device)
printk("(skipping alias ");
@@ -1818,9 +1822,13 @@
* hangs the process if some reconstruction has not
* finished.
*/
+ unlock_mddev(mddev);
+
down(&mddev->recovery_sem);
up(&mddev->recovery_sem);
+ lock_mddev(mddev);
+
invalidate_device(dev, 1);
if (ro) {
@@ -1972,8 +1980,6 @@
printk(KERN_ERR "md: cannot allocate memory for md drive.\n");
break;
}
- if (md_kdev == countdev)
- atomic_inc(&mddev->active);
printk(KERN_INFO "md: created md%d\n", mdidx(mddev));
ITERATE_RDEV_GENERIC(candidates,pending,rdev,tmp) {
bind_rdev_to_array(rdev, mddev);
@@ -1981,6 +1987,9 @@
MD_INIT_LIST_HEAD(&rdev->pending);
}
autorun_array(mddev);
+ unlock_mddev(mddev);
+ if (md_kdev != countdev)
+ atomic_dec(&mddev->active);
}
printk(KERN_INFO "md: ... autorun DONE.\n");
}
@@ -2662,17 +2671,6 @@
err = -ENOMEM;
goto abort;
}
- atomic_inc(&mddev->active);
-
- /*
- * alloc_mddev() should possibly self-lock.
- */
- err = lock_mddev(mddev);
- if (err) {
- printk(KERN_WARNING "md: ioctl, reason %d, cmd %d\n",
- err, cmd);
- goto abort;
- }
if (mddev->sb) {
printk(KERN_WARNING "md: array md%d already has a superblock!\n",
@@ -2717,7 +2715,7 @@
err = -ENODEV;
goto abort;
}
- err = lock_mddev(mddev);
+ err = lock_mddev_interruptible(mddev);
if (err) {
printk(KERN_INFO "md: ioctl lock interrupted, reason %d, cmd %d\n",err, cmd);
goto abort;
@@ -3056,7 +3054,6 @@
return 0;
if (!mddev->pers->error_handler
|| mddev->pers->error_handler(mddev,rdev) <= 0) {
- free_disk_sb(rrdev);
rrdev->faulty = 1;
} else
return 1;
@@ -3180,7 +3177,7 @@
else
sz += sprintf(page+sz, "%d sectors\n", read_ahead[MD_MAJOR]);
- ITERATE_MDDEV(mddev,tmp) {
+ ITERATE_MDDEV_LOCK(mddev,tmp) {
sz += sprintf(page + sz, "md%d : %sactive", mdidx(mddev),
mddev->pers ? "" : "in");
if (mddev->pers) {
@@ -3512,8 +3509,8 @@
struct md_list_head *tmp;
printk(KERN_INFO "md: recovery thread got woken up ...\n");
-restart:
- ITERATE_MDDEV(mddev,tmp) {
+
+ ITERATE_MDDEV_LOCK(mddev,tmp) {
sb = mddev->sb;
if (!sb)
continue;
@@ -3540,9 +3537,11 @@
continue;
if (mddev->pers->diskop(mddev, &spare, DISKOP_SPARE_WRITE))
continue;
+ unlock_mddev(mddev);
down(&mddev->recovery_sem);
mddev->recovery_running = 1;
err = md_do_sync(mddev, spare);
+ lock_mddev(mddev);
if (err == -EIO) {
printk(KERN_INFO "md%d: spare disk %s failed, skipping to next spare.\n",
mdidx(mddev), partition_name(MKDEV(spare->major,spare->minor)));
@@ -3586,7 +3585,6 @@
}
mddev->sb_dirty = 1;
md_update_sb(mddev);
- goto restart;
}
printk(KERN_INFO "md: recovery thread finished ...\n");
@@ -3603,7 +3601,7 @@
printk(KERN_INFO "md: stopping all md devices.\n");
- ITERATE_MDDEV(mddev,tmp)
+ ITERATE_MDDEV_LOCK(mddev,tmp)
do_md_stop (mddev, 1);
/*
* certain more exotic SCSI devices are known to be
@@ -3944,7 +3942,8 @@
mddev->sb_dirty = 0;
do_md_stop(mddev, 0);
printk(KERN_WARNING "md: starting md%d failed\n", minor);
- }
+ } else
+ unlock_mddev(mddev);
}
}
--- ./drivers/md/raid5.c 2002/02/27 01:20:36 1.1
+++ ./drivers/md/raid5.c 2002/02/27 04:57:36 1.2
@@ -1293,8 +1293,12 @@
handled = 0;
- if (mddev->sb_dirty)
- md_update_sb(mddev);
+ if (mddev->sb_dirty) {
+ lock_mddev(&mddev);
+ if (mddev->sb_dirty)
+ md_update_sb(mddev);
+ unlock_mddev(mddev);
+ }
md_spin_lock_irq(&conf->device_lock);
while (1) {
struct list_head *first;
--- ./drivers/md/raid1.c 2002/02/27 01:21:08 1.1
+++ ./drivers/md/raid1.c 2002/02/27 04:57:36 1.2
@@ -1136,8 +1136,12 @@
md_spin_unlock_irqrestore(&retry_list_lock, flags);
mddev = r1_bh->mddev;
- if (mddev->sb_dirty)
- md_update_sb(mddev);
+ if (mddev->sb_dirty) {
+ lock_mddev(mddev);
+ if (mddev->sb_dirty)
+ md_update_sb(mddev);
+ unlock_mddev(mddev);
+ }
bh = &r1_bh->bh_req;
switch(r1_bh->cmd) {
case SPECIAL:
--- ./drivers/md/multipath.c 2002/02/27 01:21:44 1.1
+++ ./drivers/md/multipath.c 2002/02/27 04:57:36 1.2
@@ -701,8 +701,12 @@
md_spin_unlock_irqrestore(&retry_list_lock, flags);
mddev = mp_bh->mddev;
- if (mddev->sb_dirty)
- md_update_sb(mddev);
+ if (mddev->sb_dirty) {
+ lock_mddev(mddev);
+ if (mddev->sb_dirty)
+ md_update_sb(mddev);
+ unlock_mddev(mddev);
+ }
bh = &mp_bh->bh_req;
dev = bh->b_dev;
-
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
