Currently dkcsumattach() walks the alldevs list whilst performing
operations that will sleep. This is rather bad if the list happens to be
modified (i.e. a device detachs) whilst it is sleeping.
The following diff resolves this issue. The process is very similar to
that used for softraid - we maintain a list of disks that we have checked
and each time we do something that may have slept we restart the
scan from the top of the list.
A couple of other things come along for the ride:
1. We can walk disklist rather than walking alldevs.
2. We can benefit from the more recent changes to disk_attach() and
use the devno that is stored in the disk struct.
3. The DKF_NOREADLABEL flag is used to skip disks to dkcsum, rather
than using dev_rawpart() - the list should be similar and we already
exclude cd(4) and fd(4) devices. If we do this dev_rawpart() can probably
be removed.
I have lightly tested this, however further testing (on i386 and amd64)
would be appreciated.
Comments or oks?
Index: arch/i386/i386/dkcsum.c
===================================================================
RCS file: /cvs/src/sys/arch/i386/i386/dkcsum.c,v
retrieving revision 1.29
diff -u -p -r1.29 dkcsum.c
--- arch/i386/i386/dkcsum.c 16 Apr 2011 03:21:15 -0000 1.29
+++ arch/i386/i386/dkcsum.c 2 Jul 2011 16:01:57 -0000
@@ -33,8 +33,10 @@
#include <sys/buf.h>
#include <sys/conf.h>
#include <sys/device.h>
+#include <sys/disk.h>
#include <sys/disklabel.h>
#include <sys/fcntl.h>
+#include <sys/malloc.h>
#include <sys/proc.h>
#include <sys/reboot.h>
#include <sys/stat.h>
@@ -44,16 +46,23 @@
#include <lib/libz/zlib.h>
-dev_t dev_rawpart(struct device *); /* XXX */
-
extern u_int32_t bios_cksumlen;
extern bios_diskinfo_t *bios_diskinfo;
extern dev_t bootdev;
+struct dkcsum_disk {
+ dev_t dkc_devno;
+ SLIST_ENTRY(dkcsum_disk) dkc_link;
+};
+SLIST_HEAD(dkcsum_disklist_head, dkcsum_disk);
+
void
dkcsumattach(void)
{
+ struct dkcsum_disklist_head dkc_disklist;
+ struct dkcsum_disk *dkc;
struct device *dv;
+ struct disk *dk;
struct buf *bp;
struct bdevsw *bdsw;
dev_t dev, pribootdev, altbootdev;
@@ -88,12 +97,39 @@ dkcsumattach(void)
*/
bp = geteblk(bios_cksumlen * DEV_BSIZE); /* XXX error check? */
- TAILQ_FOREACH(dv, &alldevs, dv_list) {
- if (dv->dv_class != DV_DISK)
+ SLIST_INIT(&dkc_disklist);
+ dk = TAILQ_FIRST(&disklist);
+ while (dk != TAILQ_END(&disklist)) {
+
+ dv = dk->dk_device;
+ if (dv == NULL || dk->dk_devno == NODEV ||
+ (dk->dk_flags & DKF_NOLABELREAD)) {
+ dk = TAILQ_NEXT(dk, dk_link);
continue;
- bp->b_dev = dev = dev_rawpart(dv);
- if (dev == NODEV)
+ }
+
+ SLIST_FOREACH(dkc, &dkc_disklist, dkc_link)
+ if (dkc->dkc_devno == dk->dk_devno)
+ break;
+
+ if (dkc != NULL) {
+ dk = TAILQ_NEXT(dk, dk_link);
continue;
+ }
+
+#ifdef DEBUG
+ printf("dkcsum: adding %s to the list of checked disks\n",
+ dv->dv_xname);
+#endif
+ dkc = malloc(sizeof(struct dkcsum_disk), M_DEVBUF,
+ M_NOWAIT | M_CANFAIL | M_ZERO);
+ if (dkc == NULL)
+ panic("dkcsum: failed to allocate memory");
+ dkc->dkc_devno = dk->dk_devno;
+ SLIST_INSERT_HEAD(&dkc_disklist, dkc, dkc_link);
+
+ bp->b_dev = dev = MAKEDISKDEV(major(dk->dk_devno),
+ DISKUNIT(dk->dk_devno), RAW_PART);
bdsw = &bdevsw[major(dev)];
/*
@@ -107,6 +143,7 @@ dkcsumattach(void)
printf("dkcsum: %s open failed (%d)\n",
dv->dv_xname, error);
#endif
+ dk = TAILQ_FIRST(&disklist);
continue;
}
@@ -129,6 +166,7 @@ dkcsumattach(void)
printf("dkcsum: %s close failed (%d)\n",
dv->dv_xname, error);
#endif
+ dk = TAILQ_FIRST(&disklist);
continue;
}
error = (*bdsw->d_close)(dev, FREAD, S_IFCHR, curproc);
@@ -138,6 +176,7 @@ dkcsumattach(void)
printf("dkcsum: %s closed failed (%d)\n",
dv->dv_xname, error);
#endif
+ dk = TAILQ_FIRST(&disklist);
continue;
}
@@ -173,6 +212,7 @@ dkcsumattach(void)
printf("dkcsum: %s has no matching BIOS drive\n",
dv->dv_xname);
#endif
+ dk = TAILQ_FIRST(&disklist);
continue;
}
@@ -223,9 +263,18 @@ dkcsumattach(void)
hit->bsd_dev = MAKEBOOTDEV(major(bp->b_dev), 0, 0,
DISKUNIT(bp->b_dev), RAW_PART);
hit->flags |= BDI_PICKED;
+
+ /* Restart scan since we may have slept. */
+ dk = TAILQ_FIRST(&disklist);
}
bootdev = pribootdev ? pribootdev : altbootdev ? altbootdev : bootdev;
bp->b_flags |= B_INVAL;
brelse(bp);
+
+ while (!SLIST_EMPTY(&dkc_disklist)) {
+ dkc = SLIST_FIRST(&dkc_disklist);
+ SLIST_REMOVE_HEAD(&dkc_disklist, dkc_link);
+ free(dkc, M_DEVBUF);
+ }
}
Index: arch/amd64/amd64/dkcsum.c
===================================================================
RCS file: /cvs/src/sys/arch/amd64/amd64/dkcsum.c,v
retrieving revision 1.18
diff -u -p -r1.18 dkcsum.c
--- arch/amd64/amd64/dkcsum.c 16 Apr 2011 03:21:15 -0000 1.18
+++ arch/amd64/amd64/dkcsum.c 2 Jul 2011 16:01:57 -0000
@@ -33,8 +33,10 @@
#include <sys/buf.h>
#include <sys/conf.h>
#include <sys/device.h>
+#include <sys/disk.h>
#include <sys/disklabel.h>
#include <sys/fcntl.h>
+#include <sys/malloc.h>
#include <sys/proc.h>
#include <sys/reboot.h>
#include <sys/stat.h>
@@ -44,16 +46,23 @@
#include <lib/libz/zlib.h>
-dev_t dev_rawpart(struct device *); /* XXX */
-
extern u_int32_t bios_cksumlen;
extern bios_diskinfo_t *bios_diskinfo;
extern dev_t bootdev;
+struct dkcsum_disk {
+ dev_t dkc_devno;
+ SLIST_ENTRY(dkcsum_disk) dkc_link;
+};
+SLIST_HEAD(dkcsum_disklist_head, dkcsum_disk);
+
void
dkcsumattach(void)
{
+ struct dkcsum_disklist_head dkc_disklist;
+ struct dkcsum_disk *dkc;
struct device *dv;
+ struct disk *dk;
struct buf *bp;
struct bdevsw *bdsw;
dev_t dev, pribootdev, altbootdev;
@@ -88,12 +97,39 @@ dkcsumattach(void)
*/
bp = geteblk(bios_cksumlen * DEV_BSIZE); /* XXX error check? */
- TAILQ_FOREACH(dv, &alldevs, dv_list) {
- if (dv->dv_class != DV_DISK)
+ SLIST_INIT(&dkc_disklist);
+ dk = TAILQ_FIRST(&disklist);
+ while (dk != TAILQ_END(&disklist)) {
+
+ dv = dk->dk_device;
+ if (dv == NULL || dk->dk_devno == NODEV ||
+ (dk->dk_flags & DKF_NOLABELREAD)) {
+ dk = TAILQ_NEXT(dk, dk_link);
continue;
- bp->b_dev = dev = dev_rawpart(dv);
- if (dev == NODEV)
+ }
+
+ SLIST_FOREACH(dkc, &dkc_disklist, dkc_link)
+ if (dkc->dkc_devno == dk->dk_devno)
+ break;
+
+ if (dkc != NULL) {
+ dk = TAILQ_NEXT(dk, dk_link);
continue;
+ }
+
+#ifdef DEBUG
+ printf("dkcsum: adding %s to the list of checked disks\n",
+ dv->dv_xname);
+#endif
+ dkc = malloc(sizeof(struct dkcsum_disk), M_DEVBUF,
+ M_NOWAIT | M_CANFAIL | M_ZERO);
+ if (dkc == NULL)
+ panic("dkcsum: failed to allocate memory");
+ dkc->dkc_devno = dk->dk_devno;
+ SLIST_INSERT_HEAD(&dkc_disklist, dkc, dkc_link);
+
+ bp->b_dev = dev = MAKEDISKDEV(major(dk->dk_devno),
+ DISKUNIT(dk->dk_devno), RAW_PART);
bdsw = &bdevsw[major(dev)];
/*
@@ -107,6 +143,7 @@ dkcsumattach(void)
printf("dkcsum: %s open failed (%d)\n",
dv->dv_xname, error);
#endif
+ dk = TAILQ_FIRST(&disklist);
continue;
}
@@ -129,6 +166,7 @@ dkcsumattach(void)
printf("dkcsum: %s close failed (%d)\n",
dv->dv_xname, error);
#endif
+ dk = TAILQ_FIRST(&disklist);
continue;
}
error = (*bdsw->d_close)(dev, FREAD, S_IFCHR, curproc);
@@ -138,6 +176,7 @@ dkcsumattach(void)
printf("dkcsum: %s closed failed (%d)\n",
dv->dv_xname, error);
#endif
+ dk = TAILQ_FIRST(&disklist);
continue;
}
@@ -173,6 +212,7 @@ dkcsumattach(void)
printf("dkcsum: %s has no matching BIOS drive\n",
dv->dv_xname);
#endif
+ dk = TAILQ_FIRST(&disklist);
continue;
}
@@ -223,9 +263,18 @@ dkcsumattach(void)
hit->bsd_dev = MAKEBOOTDEV(major(bp->b_dev), 0, 0,
DISKUNIT(bp->b_dev), RAW_PART);
hit->flags |= BDI_PICKED;
+
+ /* Restart scan since we may have slept. */
+ dk = TAILQ_FIRST(&disklist);
}
bootdev = pribootdev ? pribootdev : altbootdev ? altbootdev : bootdev;
bp->b_flags |= B_INVAL;
brelse(bp);
+
+ while (!SLIST_EMPTY(&dkc_disklist)) {
+ dkc = SLIST_FIRST(&dkc_disklist);
+ SLIST_REMOVE_HEAD(&dkc_disklist, dkc_link);
+ free(dkc, M_DEVBUF);
+ }
}
--
"Reason is not automatic. Those who deny it cannot be conquered by it.
Do not count on them. Leave them alone." -- Ayn Rand
