Differential D19227 Diff 54206 sys/geom/concat/g_concat.c

Changeset View

Standalone View

sys/geom/concat/g_concat.c

Show All 29 Lines
__FBSDID("$FreeBSD$");		__FBSDID("$FreeBSD$");

#include <sys/param.h>		#include <sys/param.h>
#include <sys/systm.h>		#include <sys/systm.h>
#include <sys/kernel.h>		#include <sys/kernel.h>
#include <sys/module.h>		#include <sys/module.h>
#include <sys/lock.h>		#include <sys/lock.h>
#include <sys/mutex.h>		#include <sys/mutex.h>
		#include <sys/sx.h>
#include <sys/bio.h>		#include <sys/bio.h>
#include <sys/sbuf.h>		#include <sys/sbuf.h>
#include <sys/sysctl.h>		#include <sys/sysctl.h>
#include <sys/malloc.h>		#include <sys/malloc.h>
#include <geom/geom.h>		#include <geom/geom.h>
#include <geom/concat/g_concat.h>		#include <geom/concat/g_concat.h>

FEATURE(geom_concat, "GEOM concatenation support");		FEATURE(geom_concat, "GEOM concatenation support");
▲ Show 20 Lines • Show All 51 Lines • ▼ Show 20 Lines
}		}

/*		/*
* Return the number of valid disks.		* Return the number of valid disks.
*/		*/
static u_int		static u_int
g_concat_nvalid(struct g_concat_softc *sc)		g_concat_nvalid(struct g_concat_softc *sc)
{		{
u_int i, no;		u_int no;
		struct g_concat_disk *disk;

		sx_assert(&sc->sc_lock_append, SA_LOCKED);

no = 0;		no = 0;
for (i = 0; i < sc->sc_ndisks; i++) {		TAILQ_FOREACH(disk, &sc->sc_disks, d_next) {
if (sc->sc_disks[i].d_consumer != NULL)		if (disk->d_consumer != NULL)
no++;		no++;
}		}

return (no);		return (no);
}		}

static void		static void
g_concat_remove_disk(struct g_concat_disk *disk)		g_concat_remove_disk(struct g_concat_disk *disk)
▲ Show 20 Lines • Show All 49 Lines • ▼ Show 20 Lines
}		}

static int		static int
g_concat_access(struct g_provider *pp, int dr, int dw, int de)		g_concat_access(struct g_provider *pp, int dr, int dw, int de)
{		{
struct g_consumer cp1, cp2, *tmp;		struct g_consumer cp1, cp2, *tmp;
struct g_concat_disk *disk;		struct g_concat_disk *disk;
struct g_geom *gp;		struct g_geom *gp;
		struct g_concat_softc *sc;
int error;		int error;

g_topology_assert();		g_topology_assert();
gp = pp->geom;		gp = pp->geom;
		sc = gp->softc;

/* On first open, grab an extra "exclusive" bit */		/* On first open, grab an extra "exclusive" bit */
if (pp->acr == 0 && pp->acw == 0 && pp->ace == 0)		if (pp->acr == 0 && pp->acw == 0 && pp->ace == 0)
de++;		de++;
/* ... and let go of it on last close */		/* ... and let go of it on last close */
if ((pp->acr + dr) == 0 && (pp->acw + dw) == 0 && (pp->ace + de) == 0)		if ((pp->acr + dr) == 0 && (pp->acw + dw) == 0 && (pp->ace + de) == 0)
de--;		de--;

		sx_slock(&sc->sc_lock_append);
LIST_FOREACH_SAFE(cp1, &gp->consumer, consumer, tmp) {		LIST_FOREACH_SAFE(cp1, &gp->consumer, consumer, tmp) {
error = g_access(cp1, dr, dw, de);		error = g_access(cp1, dr, dw, de);
if (error != 0)		if (error != 0)
goto fail;		goto fail;
disk = cp1->private;		disk = cp1->private;
if (cp1->acr == 0 && cp1->acw == 0 && cp1->ace == 0 &&		if (cp1->acr == 0 && cp1->acw == 0 && cp1->ace == 0 &&
disk->d_removed) {		disk->d_removed) {
g_concat_remove_disk(disk); /* May destroy geom. */		g_concat_remove_disk(disk); /* May destroy geom. */
}		}
}		}
		sx_sunlock(&sc->sc_lock_append);
return (0);		return (0);

fail:		fail:
		sx_sunlock(&sc->sc_lock_append);
LIST_FOREACH(cp2, &gp->consumer, consumer) {		LIST_FOREACH(cp2, &gp->consumer, consumer) {
if (cp1 == cp2)		if (cp1 == cp2)
break;		break;
g_access(cp2, -dr, -dw, -de);		g_access(cp2, -dr, -dw, -de);
}		}
return (error);		return (error);
}		}

static void		static void
g_concat_candelete(struct bio *bp)		g_concat_candelete(struct bio *bp)
{		{
struct g_concat_softc *sc;		struct g_concat_softc *sc;
struct g_concat_disk *disk;		struct g_concat_disk *disk;
int i, val;		int val;

sc = bp->bio_to->geom->softc;		sc = bp->bio_to->geom->softc;
for (i = 0; i < sc->sc_ndisks; i++) {		sx_assert(&sc->sc_lock_append, SX_LOCKED);
disk = &sc->sc_disks[i];		TAILQ_FOREACH(disk, &sc->sc_disks, d_next) {
if (!disk->d_removed && disk->d_candelete)		if (!disk->d_removed && disk->d_candelete)
break;		break;
}		}
val = i < sc->sc_ndisks;		val = disk != NULL;
g_handleattr(bp, "GEOM::candelete", &val, sizeof(val));		g_handleattr(bp, "GEOM::candelete", &val, sizeof(val));
}		}

static void		static void
g_concat_kernel_dump(struct bio *bp)		g_concat_kernel_dump(struct bio *bp)
{		{
struct g_concat_softc *sc;		struct g_concat_softc *sc;
struct g_concat_disk *disk;		struct g_concat_disk *disk;
struct bio *cbp;		struct bio *cbp;
struct g_kerneldump *gkd;		struct g_kerneldump *gkd;
u_int i;

sc = bp->bio_to->geom->softc;		sc = bp->bio_to->geom->softc;
gkd = (struct g_kerneldump *)bp->bio_data;		gkd = (struct g_kerneldump *)bp->bio_data;
for (i = 0; i < sc->sc_ndisks; i++) {		TAILQ_FOREACH(disk, &sc->sc_disks, d_next) {
if (sc->sc_disks[i].d_start <= gkd->offset &&		if (disk->d_start <= gkd->offset &&
sc->sc_disks[i].d_end > gkd->offset)		disk->d_end > gkd->offset)
break;		break;
}		}
if (i == sc->sc_ndisks)		if (disk == NULL) {
g_io_deliver(bp, EOPNOTSUPP);		g_io_deliver(bp, EOPNOTSUPP);
disk = &sc->sc_disks[i];		return;
		noah.bergbauer_tum.deAuthorUnsubmitted Done Inline Actions I think not returning here was a bug? noah.bergbauer_tum.de: I think not returning here was a bug?
		markjUnsubmitted Not Done Inline Actions Yes, I'll go ahead and commit that now. Thanks. If you're interested in using gconcat as a kernel dump device, I found another bug some time ago :( https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=225120 markj: Yes, I'll go ahead and commit that now. Thanks. If you're interested in using gconcat as a…
		}

gkd->offset -= disk->d_start;		gkd->offset -= disk->d_start;
if (gkd->length > disk->d_end - disk->d_start - gkd->offset)		if (gkd->length > disk->d_end - disk->d_start - gkd->offset)
gkd->length = disk->d_end - disk->d_start - gkd->offset;		gkd->length = disk->d_end - disk->d_start - gkd->offset;
cbp = g_clone_bio(bp);		cbp = g_clone_bio(bp);
if (cbp == NULL) {		if (cbp == NULL) {
g_io_deliver(bp, ENOMEM);		g_io_deliver(bp, ENOMEM);
return;		return;
}		}
Show All 25 Lines
}		}

static void		static void
g_concat_flush(struct g_concat_softc sc, struct bio bp)		g_concat_flush(struct g_concat_softc sc, struct bio bp)
{		{
struct bio_queue_head queue;		struct bio_queue_head queue;
struct g_consumer *cp;		struct g_consumer *cp;
struct bio *cbp;		struct bio *cbp;
u_int no;		struct g_concat_disk *disk;

		sx_assert(&sc->sc_lock_append, SX_LOCKED);

bioq_init(&queue);		bioq_init(&queue);
for (no = 0; no < sc->sc_ndisks; no++) {		TAILQ_FOREACH(disk, &sc->sc_disks, d_next) {
cbp = g_clone_bio(bp);		cbp = g_clone_bio(bp);
if (cbp == NULL) {		if (cbp == NULL) {
while ((cbp = bioq_takefirst(&queue)) != NULL)		while ((cbp = bioq_takefirst(&queue)) != NULL)
g_destroy_bio(cbp);		g_destroy_bio(cbp);
if (bp->bio_error == 0)		if (bp->bio_error == 0)
bp->bio_error = ENOMEM;		bp->bio_error = ENOMEM;
g_io_deliver(bp, bp->bio_error);		g_io_deliver(bp, bp->bio_error);
return;		return;
}		}
bioq_insert_tail(&queue, cbp);		bioq_insert_tail(&queue, cbp);
cbp->bio_done = g_concat_done;		cbp->bio_done = g_concat_done;
cbp->bio_caller1 = sc->sc_disks[no].d_consumer;		cbp->bio_caller1 = disk->d_consumer;
cbp->bio_to = sc->sc_disks[no].d_consumer->provider;		cbp->bio_to = disk->d_consumer->provider;
}		}
while ((cbp = bioq_takefirst(&queue)) != NULL) {		while ((cbp = bioq_takefirst(&queue)) != NULL) {
G_CONCAT_LOGREQ(cbp, "Sending request.");		G_CONCAT_LOGREQ(cbp, "Sending request.");
cp = cbp->bio_caller1;		cp = cbp->bio_caller1;
cbp->bio_caller1 = NULL;		cbp->bio_caller1 = NULL;
g_io_request(cbp, cp);		g_io_request(cbp, cp);
}		}
}		}

static void		static void
g_concat_start(struct bio *bp)		g_concat_start(struct bio *bp)
{		{
struct bio_queue_head queue;		struct bio_queue_head queue;
struct g_concat_softc *sc;		struct g_concat_softc *sc;
struct g_concat_disk *disk;		struct g_concat_disk *disk;
struct g_provider *pp;		struct g_provider *pp;
off_t offset, end, length, off, len;		off_t offset, end, length, off, len;
struct bio *cbp;		struct bio *cbp;
char *addr;		char *addr;
u_int no;

pp = bp->bio_to;		pp = bp->bio_to;
sc = pp->geom->softc;		sc = pp->geom->softc;
/*		/*
* If sc == NULL, provider's error should be set and g_concat_start()		* If sc == NULL, provider's error should be set and g_concat_start()
* should not be called at all.		* should not be called at all.
*/		*/
KASSERT(sc != NULL,		KASSERT(sc != NULL,
("Provider's error should be set (error=%d)(device=%s).",		("Provider's error should be set (error=%d)(device=%s).",
bp->bio_to->error, bp->bio_to->name));		bp->bio_to->error, bp->bio_to->name));

G_CONCAT_LOGREQ(bp, "Request received.");		G_CONCAT_LOGREQ(bp, "Request received.");
		sx_slock(&sc->sc_lock_append);

switch (bp->bio_cmd) {		switch (bp->bio_cmd) {
case BIO_READ:		case BIO_READ:
case BIO_WRITE:		case BIO_WRITE:
case BIO_DELETE:		case BIO_DELETE:
break;		break;
case BIO_FLUSH:		case BIO_FLUSH:
g_concat_flush(sc, bp);		g_concat_flush(sc, bp);
return;		goto end;
case BIO_GETATTR:		case BIO_GETATTR:
if (strcmp("GEOM::kerneldump", bp->bio_attribute) == 0) {		if (strcmp("GEOM::kerneldump", bp->bio_attribute) == 0) {
g_concat_kernel_dump(bp);		g_concat_kernel_dump(bp);
return;		goto end;
} else if (strcmp("GEOM::candelete", bp->bio_attribute) == 0) {		} else if (strcmp("GEOM::candelete", bp->bio_attribute) == 0) {
g_concat_candelete(bp);		g_concat_candelete(bp);
return;		goto end;
}		}
/* To which provider it should be delivered? */		/* To which provider it should be delivered? */
/* FALLTHROUGH */		/* FALLTHROUGH */
default:		default:
g_io_deliver(bp, EOPNOTSUPP);		g_io_deliver(bp, EOPNOTSUPP);
return;		goto end;
}		}

offset = bp->bio_offset;		offset = bp->bio_offset;
length = bp->bio_length;		length = bp->bio_length;
if ((bp->bio_flags & BIO_UNMAPPED) != 0)		if ((bp->bio_flags & BIO_UNMAPPED) != 0)
addr = NULL;		addr = NULL;
else		else
addr = bp->bio_data;		addr = bp->bio_data;
end = offset + length;		end = offset + length;

bioq_init(&queue);		bioq_init(&queue);
for (no = 0; no < sc->sc_ndisks; no++) {		TAILQ_FOREACH(disk, &sc->sc_disks, d_next) {
disk = &sc->sc_disks[no];
if (disk->d_end <= offset)		if (disk->d_end <= offset)
continue;		continue;
if (disk->d_start >= end)		if (disk->d_start >= end)
break;		break;

off = offset - disk->d_start;		off = offset - disk->d_start;
len = MIN(length, disk->d_end - offset);		len = MIN(length, disk->d_end - offset);
length -= len;		length -= len;
offset += len;		offset += len;

cbp = g_clone_bio(bp);		cbp = g_clone_bio(bp);
if (cbp == NULL) {		if (cbp == NULL) {
while ((cbp = bioq_takefirst(&queue)) != NULL)		while ((cbp = bioq_takefirst(&queue)) != NULL)
g_destroy_bio(cbp);		g_destroy_bio(cbp);
if (bp->bio_error == 0)		if (bp->bio_error == 0)
bp->bio_error = ENOMEM;		bp->bio_error = ENOMEM;
g_io_deliver(bp, bp->bio_error);		g_io_deliver(bp, bp->bio_error);
return;		goto end;
}		}
bioq_insert_tail(&queue, cbp);		bioq_insert_tail(&queue, cbp);
/*		/*
* Fill in the component buf structure.		* Fill in the component buf structure.
*/		*/
if (len == bp->bio_length)		if (len == bp->bio_length)
cbp->bio_done = g_std_done;		cbp->bio_done = g_std_done;
else		else
Show All 19 Lines	KASSERT(length == 0,
("Length is still greater than 0 (class=%s, name=%s).",		("Length is still greater than 0 (class=%s, name=%s).",
bp->bio_to->geom->class->name, bp->bio_to->geom->name));		bp->bio_to->geom->class->name, bp->bio_to->geom->name));
while ((cbp = bioq_takefirst(&queue)) != NULL) {		while ((cbp = bioq_takefirst(&queue)) != NULL) {
G_CONCAT_LOGREQ(cbp, "Sending request.");		G_CONCAT_LOGREQ(cbp, "Sending request.");
disk = cbp->bio_caller1;		disk = cbp->bio_caller1;
cbp->bio_caller1 = NULL;		cbp->bio_caller1 = NULL;
g_io_request(cbp, disk->d_consumer);		g_io_request(cbp, disk->d_consumer);
}		}
		end:
		sx_sunlock(&sc->sc_lock_append);
}		}

static void		static void
g_concat_check_and_run(struct g_concat_softc *sc)		g_concat_check_and_run(struct g_concat_softc *sc)
{		{
struct g_concat_disk *disk;		struct g_concat_disk *disk;
struct g_provider dp, pp;		struct g_provider dp, pp;
u_int no, sectorsize = 0;		u_int no = 0, sectorsize = 0;
off_t start;		off_t start;
int error;		int error;

g_topology_assert();		g_topology_assert();
if (g_concat_nvalid(sc) != sc->sc_ndisks)		if (g_concat_nvalid(sc) != sc->sc_ndisks)
return;		return;

pp = g_new_providerf(sc->sc_geom, "concat/%s", sc->sc_name);		pp = g_new_providerf(sc->sc_geom, "concat/%s", sc->sc_name);
pp->flags \|= G_PF_DIRECT_SEND \| G_PF_DIRECT_RECEIVE \|		pp->flags \|= G_PF_DIRECT_SEND \| G_PF_DIRECT_RECEIVE \|
G_PF_ACCEPT_UNMAPPED;		G_PF_ACCEPT_UNMAPPED;
start = 0;		start = 0;
for (no = 0; no < sc->sc_ndisks; no++) {		TAILQ_FOREACH(disk, &sc->sc_disks, d_next) {
disk = &sc->sc_disks[no];
dp = disk->d_consumer->provider;		dp = disk->d_consumer->provider;
disk->d_start = start;		disk->d_start = start;
disk->d_end = disk->d_start + dp->mediasize;		disk->d_end = disk->d_start + dp->mediasize;
if (sc->sc_type == G_CONCAT_TYPE_AUTOMATIC)		if (sc->sc_type == G_CONCAT_TYPE_AUTOMATIC)
disk->d_end -= dp->sectorsize;		disk->d_end -= dp->sectorsize;
start = disk->d_end;		start = disk->d_end;
error = g_access(disk->d_consumer, 1, 0, 0);		error = g_access(disk->d_consumer, 1, 0, 0);
if (error == 0) {		if (error == 0) {
error = g_getattr("GEOM::candelete", disk->d_consumer,		error = g_getattr("GEOM::candelete", disk->d_consumer,
&disk->d_candelete);		&disk->d_candelete);
if (error != 0)		if (error != 0)
disk->d_candelete = 0;		disk->d_candelete = 0;
(void)g_access(disk->d_consumer, -1, 0, 0);		(void)g_access(disk->d_consumer, -1, 0, 0);
} else		} else
G_CONCAT_DEBUG(1, "Failed to access disk %s, error %d.",		G_CONCAT_DEBUG(1, "Failed to access disk %s, error %d.",
dp->name, error);		dp->name, error);
if (no == 0)		if (no == 0)
sectorsize = dp->sectorsize;		sectorsize = dp->sectorsize;
else		else
sectorsize = lcm(sectorsize, dp->sectorsize);		sectorsize = lcm(sectorsize, dp->sectorsize);
		no++;
		markjUnsubmitted Not Done Inline Actions You can get rid of this variable and just test `disk == TAILQ_FIRST(&sc->sc_disks)` above. markj: You can get rid of this variable and just test `disk == TAILQ_FIRST(&sc->sc_disks)` above.

/* A provider underneath us doesn't support unmapped */		/* A provider underneath us doesn't support unmapped */
if ((dp->flags & G_PF_ACCEPT_UNMAPPED) == 0) {		if ((dp->flags & G_PF_ACCEPT_UNMAPPED) == 0) {
G_CONCAT_DEBUG(1, "Cancelling unmapped "		G_CONCAT_DEBUG(1, "Cancelling unmapped "
"because of %s.", dp->name);		"because of %s.", dp->name);
pp->flags &= ~G_PF_ACCEPT_UNMAPPED;		pp->flags &= ~G_PF_ACCEPT_UNMAPPED;
}		}
}		}
pp->sectorsize = sectorsize;		pp->sectorsize = sectorsize;
/* We have sc->sc_disks[sc->sc_ndisks - 1].d_end in 'start'. */		/* We have sc->sc_disks[sc->sc_ndisks - 1].d_end in 'start'. */
pp->mediasize = start;		pp->mediasize = start;
pp->stripesize = sc->sc_disks[0].d_consumer->provider->stripesize;		dp = TAILQ_FIRST(&sc->sc_disks)->d_consumer->provider;
pp->stripeoffset = sc->sc_disks[0].d_consumer->provider->stripeoffset;		pp->stripesize = dp->stripesize;
		pp->stripeoffset = dp->stripeoffset;
sc->sc_provider = pp;		sc->sc_provider = pp;
g_error_provider(pp, 0);		g_error_provider(pp, 0);

G_CONCAT_DEBUG(0, "Device %s activated.", sc->sc_provider->name);		G_CONCAT_DEBUG(0, "Device %s activated.", sc->sc_provider->name);
}		}

static int		static int
g_concat_read_metadata(struct g_consumer cp, struct g_concat_metadata md)		g_concat_read_metadata(struct g_consumer cp, struct g_concat_metadata md)
Show All 30 Lines
g_concat_add_disk(struct g_concat_softc sc, struct g_provider pp, u_int no)		g_concat_add_disk(struct g_concat_softc sc, struct g_provider pp, u_int no)
{		{
struct g_concat_disk *disk;		struct g_concat_disk *disk;
struct g_consumer cp, fcp;		struct g_consumer cp, fcp;
struct g_geom *gp;		struct g_geom *gp;
int error;		int error;

g_topology_assert();		g_topology_assert();

		sx_slock(&sc->sc_lock_append);

/* Metadata corrupted? */		/* Metadata corrupted? */
if (no >= sc->sc_ndisks)		if (no >= sc->sc_ndisks)
		{
		markjUnsubmitted Not Done Inline Actions Style: the opening brace should be on the previous line. markj: Style: the opening brace should be on the previous line.
		sx_sunlock(&sc->sc_lock_append);
return (EINVAL);		return (EINVAL);
		}

disk = &sc->sc_disks[no];		disk = TAILQ_FIRST(&sc->sc_disks);
		for (; no > 0; no--) {
		markjUnsubmitted Not Done Inline Actions `disk` can be initialized in the for loop header. markj: `disk` can be initialized in the for loop header.
		disk = TAILQ_NEXT(disk, d_next);
		}

/* Check if disk is not already attached. */		/* Check if disk is not already attached. */
if (disk->d_consumer != NULL)		if (disk->d_consumer != NULL)
		{
		markjUnsubmitted Not Done Inline Actions Style: the opening brace should be on the previous line. markj: Style: the opening brace should be on the previous line.
		sx_sunlock(&sc->sc_lock_append);
return (EEXIST);		return (EEXIST);
		}

gp = sc->sc_geom;		gp = sc->sc_geom;
fcp = LIST_FIRST(&gp->consumer);		fcp = LIST_FIRST(&gp->consumer);

cp = g_new_consumer(gp);		cp = g_new_consumer(gp);
cp->flags \|= G_CF_DIRECT_SEND \| G_CF_DIRECT_RECEIVE;		cp->flags \|= G_CF_DIRECT_SEND \| G_CF_DIRECT_RECEIVE;
error = g_attach(cp, pp);		error = g_attach(cp, pp);
if (error != 0) {		if (error != 0) {
		sx_sunlock(&sc->sc_lock_append);
g_destroy_consumer(cp);		g_destroy_consumer(cp);
return (error);		return (error);
}		}

if (fcp != NULL && (fcp->acr > 0 \|\| fcp->acw > 0 \|\| fcp->ace > 0)) {		if (fcp != NULL && (fcp->acr > 0 \|\| fcp->acw > 0 \|\| fcp->ace > 0)) {
error = g_access(cp, fcp->acr, fcp->acw, fcp->ace);		error = g_access(cp, fcp->acr, fcp->acw, fcp->ace);
if (error != 0) {		if (error != 0) {
		sx_sunlock(&sc->sc_lock_append);
g_detach(cp);		g_detach(cp);
g_destroy_consumer(cp);		g_destroy_consumer(cp);
return (error);		return (error);
}		}
}		}
if (sc->sc_type == G_CONCAT_TYPE_AUTOMATIC) {		if (sc->sc_type == G_CONCAT_TYPE_AUTOMATIC) {
struct g_concat_metadata md;		struct g_concat_metadata md;

		// temporarily give up the lock to avoid lock order violation
		// due to topology unlock in g_concat_read_metadata
		markjUnsubmitted Not Done Inline Actions I think sc_lock_append should come first in the lock order. g_concat_dumpconf() would violate that order, but you can fix that by requiring both the topology lock and sc_lock_append in order to modify the disk list. Are there any other places you're assuming that the topology lock comes first in the lock order? markj: I think sc_lock_append should come first in the lock order. g_concat_dumpconf() would violate…
		noah.bergbauer_tum.deAuthorUnsubmitted Done Inline Actions I'm not sure how to accomplish this? AFAICS the topology lock is already acquired when these functions are called (acquired outside of this module even) so I don't see how I can reverse this order. Basically what happend is I implemented the thing and then WITNESS complained about this lock order reversal with topology. So suddenly there are lock order issues that I didn't anticipate at all but like this it seems to be correct - at least from my (very limited) viewpoint. noah.bergbauer_tum.de: I'm not sure how to accomplish this? AFAICS the topology lock is already acquired when these…
		markjUnsubmitted Not Done Inline Actions One issue is that the `disk` pointer may have been invalidated while the lock was dropped. That said, I don't think you need to look up `disk` until after this point? markj: One issue is that the `disk` pointer may have been invalidated while the lock was dropped. That…
		noah.bergbauer_tum.deAuthorUnsubmitted Done Inline Actions Why though? List elements are never removed so the pointer should remain valid? noah.bergbauer_tum.de: Why though? List elements are never removed so the pointer should remain valid?
		markjUnsubmitted Not Done Inline Actions Fair enough, though that invariant is not actually enforced anywhere and could become false in the future. Let's just leave the locking here as it is in the patch for now. I am mostly worried about corner-case race conditions that typically arise when code drops a lock in order to avoid a lock order reversal. There has been a number of subtle bugs in gmirror that arose this way, so I tend to be suspicious of that pattern. markj: Fair enough, though that invariant is not actually enforced anywhere and could become false in…
		sx_sunlock(&sc->sc_lock_append);
/* Re-read metadata. */		/* Re-read metadata. */
error = g_concat_read_metadata(cp, &md);		error = g_concat_read_metadata(cp, &md);
		sx_slock(&sc->sc_lock_append);

if (error != 0)		if (error != 0)
goto fail;		goto fail;

if (strcmp(md.md_magic, G_CONCAT_MAGIC) != 0 \|\|		if (strcmp(md.md_magic, G_CONCAT_MAGIC) != 0 \|\|
strcmp(md.md_name, sc->sc_name) != 0 \|\|		strcmp(md.md_name, sc->sc_name) != 0 \|\|
md.md_id != sc->sc_id) {		md.md_id != sc->sc_id) {
G_CONCAT_DEBUG(0, "Metadata on %s changed.", pp->name);		G_CONCAT_DEBUG(0, "Metadata on %s changed.", pp->name);
goto fail;		goto fail;
}		}

		disk->d_hardcoded = md.md_provider[0] != '\0';
		} else {
		disk->d_hardcoded = false;
}		}

cp->private = disk;		cp->private = disk;
disk->d_consumer = cp;		disk->d_consumer = cp;
disk->d_softc = sc;		disk->d_softc = sc;
disk->d_start = 0; /* not yet */		disk->d_start = 0; /* not yet */
disk->d_end = 0; /* not yet */		disk->d_end = 0; /* not yet */
disk->d_removed = 0;		disk->d_removed = 0;

G_CONCAT_DEBUG(0, "Disk %s attached to %s.", pp->name, sc->sc_name);		G_CONCAT_DEBUG(0, "Disk %s attached to %s.", pp->name, sc->sc_name);

g_concat_check_and_run(sc);		g_concat_check_and_run(sc);
		sx_sunlock(&sc->sc_lock_append); // need lock for check_and_run

return (0);		return (0);
fail:		fail:
		sx_sunlock(&sc->sc_lock_append);
if (fcp != NULL && (fcp->acr > 0 \|\| fcp->acw > 0 \|\| fcp->ace > 0))		if (fcp != NULL && (fcp->acr > 0 \|\| fcp->acw > 0 \|\| fcp->ace > 0))
g_access(cp, -fcp->acr, -fcp->acw, -fcp->ace);		g_access(cp, -fcp->acr, -fcp->acw, -fcp->ace);
g_detach(cp);		g_detach(cp);
g_destroy_consumer(cp);		g_destroy_consumer(cp);
return (error);		return (error);
}		}

static struct g_geom *		static struct g_geom *
g_concat_create(struct g_class mp, const struct g_concat_metadata md,		g_concat_create(struct g_class mp, const struct g_concat_metadata md,
u_int type)		u_int type)
{		{
struct g_concat_softc *sc;		struct g_concat_softc *sc;
		struct g_concat_disk *disk;
struct g_geom *gp;		struct g_geom *gp;
u_int no;		u_int no;

G_CONCAT_DEBUG(1, "Creating device %s (id=%u).", md->md_name,		G_CONCAT_DEBUG(1, "Creating device %s (id=%u).", md->md_name,
md->md_id);		md->md_id);

/* One disks is minimum. */		/* One disks is minimum. */
if (md->md_all < 1)		if (md->md_all < 1)
Show All 13 Lines	g_concat_create(struct g_class mp, const struct g_concat_metadata md,
gp->start = g_concat_start;		gp->start = g_concat_start;
gp->spoiled = g_concat_orphan;		gp->spoiled = g_concat_orphan;
gp->orphan = g_concat_orphan;		gp->orphan = g_concat_orphan;
gp->access = g_concat_access;		gp->access = g_concat_access;
gp->dumpconf = g_concat_dumpconf;		gp->dumpconf = g_concat_dumpconf;

sc->sc_id = md->md_id;		sc->sc_id = md->md_id;
sc->sc_ndisks = md->md_all;		sc->sc_ndisks = md->md_all;
sc->sc_disks = malloc(sizeof(struct g_concat_disk) * sc->sc_ndisks,		TAILQ_INIT(&sc->sc_disks);
M_CONCAT, M_WAITOK \| M_ZERO);		for (no = 0; no < sc->sc_ndisks; no++) {
for (no = 0; no < sc->sc_ndisks; no++)		disk = malloc(sizeof(*disk), M_CONCAT, M_WAITOK \| M_ZERO);
sc->sc_disks[no].d_consumer = NULL;		disk->d_consumer = NULL; // redundant, is zeroed anyways
		markjUnsubmitted Not Done Inline Actions Might as well just delete this line. markj: Might as well just delete this line.
		TAILQ_INSERT_TAIL(&sc->sc_disks, disk, d_next);
		}
sc->sc_type = type;		sc->sc_type = type;
mtx_init(&sc->sc_lock, "gconcat lock", NULL, MTX_DEF);		mtx_init(&sc->sc_lock, "gconcat lock", NULL, MTX_DEF);
		sx_init(&sc->sc_lock_append, "gconcat append lock");

gp->softc = sc;		gp->softc = sc;
sc->sc_geom = gp;		sc->sc_geom = gp;
sc->sc_provider = NULL;		sc->sc_provider = NULL;

G_CONCAT_DEBUG(0, "Device %s created (id=%u).", sc->sc_name, sc->sc_id);		G_CONCAT_DEBUG(0, "Device %s created (id=%u).", sc->sc_name, sc->sc_id);

return (gp);		return (gp);
}		}

static int		static int
g_concat_destroy(struct g_concat_softc *sc, boolean_t force)		g_concat_destroy(struct g_concat_softc *sc, boolean_t force)
{		{
struct g_provider *pp;		struct g_provider *pp;
struct g_consumer cp, cp1;		struct g_consumer cp, cp1;
struct g_geom *gp;		struct g_geom *gp;
		struct g_concat_disk disk, disk_temp;

g_topology_assert();		g_topology_assert();

if (sc == NULL)		if (sc == NULL)
return (ENXIO);		return (ENXIO);

pp = sc->sc_provider;		pp = sc->sc_provider;
if (pp != NULL && (pp->acr != 0 \|\| pp->acw != 0 \|\| pp->ace != 0)) {		if (pp != NULL && (pp->acr != 0 \|\| pp->acw != 0 \|\| pp->ace != 0)) {
Show All 15 Lines	if (cp1 == NULL)
return (0); /* Recursion happened. */		return (0); /* Recursion happened. */
}		}
if (!LIST_EMPTY(&gp->consumer))		if (!LIST_EMPTY(&gp->consumer))
return (EINPROGRESS);		return (EINPROGRESS);

gp->softc = NULL;		gp->softc = NULL;
KASSERT(sc->sc_provider == NULL, ("Provider still exists? (device=%s)",		KASSERT(sc->sc_provider == NULL, ("Provider still exists? (device=%s)",
gp->name));		gp->name));
free(sc->sc_disks, M_CONCAT);		TAILQ_FOREACH_SAFE(disk, &sc->sc_disks, d_next, disk_temp) {
		markjUnsubmitted Not Done Inline Actions You can avoid the temp variable and write: while ((disk = TAILQ_FIRST(&sc->sc_disks)) != NULL) { TAILQ_REMOVE(&sc->sc_disks, disk, d_next); free(disk, M_CONCAT); } markj: You can avoid the temp variable and write: ``` while ((disk = TAILQ_FIRST(&sc->sc_disks)) !=…
		TAILQ_REMOVE(&sc->sc_disks, disk, d_next);
		free(disk, M_CONCAT);
		}
mtx_destroy(&sc->sc_lock);		mtx_destroy(&sc->sc_lock);
		sx_destroy(&sc->sc_lock_append);
free(sc, M_CONCAT);		free(sc, M_CONCAT);

G_CONCAT_DEBUG(0, "Device %s destroyed.", gp->name);		G_CONCAT_DEBUG(0, "Device %s destroyed.", gp->name);
g_wither_geom(gp, ENXIO);		g_wither_geom(gp, ENXIO);
return (0);		return (0);
}		}

static int		static int
▲ Show 20 Lines • Show All 259 Lines • ▼ Show 20 Lines	for (i = 0; i < (u_int)*nargs; i++) {
if (error != 0) {		if (error != 0) {
gctl_error(req, "Cannot destroy device %s (error=%d).",		gctl_error(req, "Cannot destroy device %s (error=%d).",
sc->sc_name, error);		sc->sc_name, error);
return;		return;
}		}
}		}
}		}

		static struct g_concat_disk *
		g_concat_find_disk(struct g_concat_softc sc, const char name)
		{
		struct g_concat_disk *disk;

		sx_assert(&sc->sc_lock_append, SX_LOCKED);
		if (strncmp(name, "/dev/", 5) == 0)
		name += 5;
		TAILQ_FOREACH(disk, &sc->sc_disks, d_next) {
		if (disk->d_consumer == NULL)
		continue;
		if (disk->d_consumer->provider == NULL)
		continue;
		if (strcmp(disk->d_consumer->provider->name, name) == 0)
		return (disk);
		}
		return (NULL);
		}

static void		static void
		g_concat_write_metadata(struct gctl_req req, struct g_concat_softc sc)
		{
		u_int no = 0;
		struct g_concat_disk *disk;
		struct g_concat_metadata md;
		struct g_provider *pp;
		u_char *sector;
		int error;

		strlcpy(md.md_magic, G_CONCAT_MAGIC, sizeof(md.md_magic));
		md.md_version = G_CONCAT_VERSION;
		strlcpy(md.md_name, sc->sc_name, sizeof(md.md_name));
		md.md_id = sc->sc_id;
		md.md_all = sc->sc_ndisks;
		TAILQ_FOREACH(disk, &sc->sc_disks, d_next) {
		pp = disk->d_consumer->provider;

		md.md_no = no;
		bzero(md.md_provider, sizeof(md.md_provider));
		if (disk->d_hardcoded) {
		strlcpy(md.md_provider, pp->name, sizeof(md.md_provider));
		}
		md.md_provsize = disk->d_consumer->provider->mediasize;

		sector = g_malloc(pp->sectorsize, M_WAITOK);

		concat_metadata_encode(&md, sector);
		error = g_access(disk->d_consumer, 0, 1, 0);
		if (error == 0) {
		error = g_write_data(disk->d_consumer, pp->mediasize - pp->sectorsize,
		sector, pp->sectorsize);
		markjUnsubmitted Not Done Inline Actions Style: continuation lines should be indented by four spaces. markj: Style: continuation lines should be indented by four spaces.
		}
		(void)g_access(disk->d_consumer, 0, -1, 0);
		g_free(sector);
		if (error != 0) {
		gctl_error(req, "Cannot store metadata on %s: %d", pp->name, error);
		}

		no++;
		}
		}

		static void
		g_concat_ctl_append(struct gctl_req req, struct g_class mp)
		{
		struct g_concat_softc *sc;
		struct g_consumer cp, fcp;
		struct g_provider *pp;
		struct g_geom *gp;
		const char name, cname;
		struct g_concat_disk *disk;
		int nargs, hardcode;
		int error;
		int disk_candelete;

		g_topology_assert();

		nargs = gctl_get_paraml(req, "nargs", sizeof(*nargs));
		if (nargs == NULL) {
		gctl_error(req, "No '%s' argument.", "nargs");
		return;
		}
		if (*nargs != 2) {
		gctl_error(req, "Invalid number of arguments.");
		return;
		}
		hardcode = gctl_get_paraml(req, "hardcode", sizeof(*hardcode));
		if (hardcode == NULL) {
		gctl_error(req, "No '%s' argument.", "hardcode");
		return;
		}

		cname = gctl_get_asciiparam(req, "arg0");
		if (cname == NULL) {
		gctl_error(req, "No 'arg%u' argument.", 0);
		return;
		}
		sc = g_concat_find_device(mp, cname);
		if (sc == NULL) {
		gctl_error(req, "No such device: %s.", cname);
		return;
		}
		if (sc->sc_provider == NULL) {
		/*
		* this won't race with g_concat_remove_disk as both
		* are holding the topology lock
		*/
		gctl_error(req, "Device not active, can't append: %s.", cname);
		return;
		}
		G_CONCAT_DEBUG(1, "Appending to %s:", cname);
		sx_xlock(&sc->sc_lock_append);
		gp = sc->sc_geom;
		fcp = LIST_FIRST(&gp->consumer);

		name = gctl_get_asciiparam(req, "arg1");
		if (name == NULL) {
		gctl_error(req, "No 'arg%u' argument.", 1);
		goto fail;
		}
		if (strncmp(name, "/dev/", strlen("/dev/")) == 0)
		name += strlen("/dev/");
		pp = g_provider_by_name(name);
		if (pp == NULL) {
		G_CONCAT_DEBUG(1, "Disk %s is invalid.", name);
		gctl_error(req, "Disk %s is invalid.", name);
		goto fail;
		}
		G_CONCAT_DEBUG(1, "Appending %s to this", name);
		markjUnsubmitted Done Inline Actions I'd prefer to keep the disk structures on a linked list. gmirror does this, for example. The approach of realloc()ing the array and fixing up pointers is fragile, IMO. markj: I'd prefer to keep the disk structures on a linked list. gmirror does this, for example. The…

		if (g_concat_find_disk(sc, name) != NULL) {
		gctl_error(req, "Disk %s already appended.", name);
		goto fail;
		}

		if ((sc->sc_provider->sectorsize % pp->sectorsize) != 0) {
		gctl_error(req, "Providers sectorsize mismatch: %u vs %u",
		sc->sc_provider->sectorsize, pp->sectorsize);
		goto fail;
		}

		cp = g_new_consumer(gp);
		cp->flags \|= G_CF_DIRECT_SEND \| G_CF_DIRECT_RECEIVE;
		error = g_attach(cp, pp);
		if (error != 0) {
		g_destroy_consumer(cp);
		gctl_error(req, "Cannot open device %s (error=%d).",
		name, error);
		goto fail;
		}

		error = g_access(cp, 1, 0, 0);
		if (error == 0) {
		error = g_getattr("GEOM::candelete", cp, &disk_candelete);
		if (error != 0)
		disk_candelete = 0;
		(void)g_access(cp, -1, 0, 0);
		} else
		G_CONCAT_DEBUG(1, "Failed to access disk %s, error %d.", name, error);
		markjUnsubmitted Done Inline Actions The body of this loop should probably be its own subroutine. markj: The body of this loop should probably be its own subroutine.

		/* invoke g_access exactly as deep as all the other members currently are */
		if (fcp != NULL && (fcp->acr > 0 \|\| fcp->acw > 0 \|\| fcp->ace > 0)) {
		error = g_access(cp, fcp->acr, fcp->acw, fcp->ace);
		markjUnsubmitted Done Inline Actions Providers may have multiple names. For instance, /dev/ada2p1 and /dev/gptid/480b0835-ee38-11e7-81c5-001b785db800 refer to the same provider on my system. Hardcoded names ensure that the tasting process ignores these kinds of aliases. markj: Providers may have multiple names. For instance, /dev/ada2p1 and /dev/gptid/480b0835-ee38-11e7…
		if (error != 0) {
		g_detach(cp);
		g_destroy_consumer(cp);
		gctl_error(req, "Failed to access disk %s (error=%d).", name, error);
		goto fail;
		}
		}

		disk = malloc(sizeof(*disk), M_CONCAT, M_WAITOK \| M_ZERO);
		disk->d_consumer = cp;
		disk->d_softc = sc;
		disk->d_start = TAILQ_LAST(&sc->sc_disks, g_concat_disks)->d_end;
		disk->d_end = disk->d_start + cp->provider->mediasize;
		disk->d_candelete = disk_candelete;
		disk->d_removed = 0;
		disk->d_hardcoded = *hardcode;
		cp->private = disk;
		TAILQ_INSERT_TAIL(&sc->sc_disks, disk, d_next);
		sc->sc_ndisks += 1;
		markjUnsubmitted Not Done Inline Actions `sc->sc_ndisks++;` markj: `sc->sc_ndisks++;`

		if (sc->sc_type == G_CONCAT_TYPE_AUTOMATIC) {
		/* last sector is for metadata */
		disk->d_end -= cp->provider->sectorsize;

		/* update metadata on all parts */
		g_concat_write_metadata(req, sc);
		}

		g_resize_provider(sc->sc_provider, disk->d_end);

		fail:
		sx_xunlock(&sc->sc_lock_append);
		}

		static void
g_concat_config(struct gctl_req req, struct g_class mp, const char *verb)		g_concat_config(struct gctl_req req, struct g_class mp, const char *verb)
{		{
uint32_t *version;		uint32_t *version;

g_topology_assert();		g_topology_assert();

version = gctl_get_paraml(req, "version", sizeof(*version));		version = gctl_get_paraml(req, "version", sizeof(*version));
if (version == NULL) {		if (version == NULL) {
gctl_error(req, "No '%s' argument.", "version");		gctl_error(req, "No '%s' argument.", "version");
return;		return;
}		}
if (*version != G_CONCAT_VERSION) {		if (*version != G_CONCAT_VERSION) {
gctl_error(req, "Userland and kernel parts are out of sync.");		gctl_error(req, "Userland and kernel parts are out of sync.");
return;		return;
}		}

if (strcmp(verb, "create") == 0) {		if (strcmp(verb, "create") == 0) {
g_concat_ctl_create(req, mp);		g_concat_ctl_create(req, mp);
return;		return;
} else if (strcmp(verb, "destroy") == 0 \|\|		} else if (strcmp(verb, "destroy") == 0 \|\|
strcmp(verb, "stop") == 0) {		strcmp(verb, "stop") == 0) {
g_concat_ctl_destroy(req, mp);		g_concat_ctl_destroy(req, mp);
return;		return;
		} else if (strcmp(verb, "append") == 0) {
		g_concat_ctl_append(req, mp);
		return;
}		}
gctl_error(req, "Unknown verb.");		gctl_error(req, "Unknown verb.");
}		}

static void		static void
g_concat_dumpconf(struct sbuf sb, const char indent, struct g_geom *gp,		g_concat_dumpconf(struct sbuf sb, const char indent, struct g_geom *gp,
struct g_consumer cp, struct g_provider pp)		struct g_consumer cp, struct g_provider pp)
{		{
struct g_concat_softc *sc;		struct g_concat_softc *sc;

g_topology_assert();		g_topology_assert();
sc = gp->softc;		sc = gp->softc;
if (sc == NULL)		if (sc == NULL)
return;		return;

		sx_slock(&sc->sc_lock_append);
if (pp != NULL) {		if (pp != NULL) {
/* Nothing here. */		/* Nothing here. */
} else if (cp != NULL) {		} else if (cp != NULL) {
struct g_concat_disk *disk;		struct g_concat_disk *disk;

disk = cp->private;		disk = cp->private;
if (disk == NULL)		if (disk == NULL)
return;		goto end;
sbuf_printf(sb, "%s<End>%jd</End>\n", indent,		sbuf_printf(sb, "%s<End>%jd</End>\n", indent,
(intmax_t)disk->d_end);		(intmax_t)disk->d_end);
sbuf_printf(sb, "%s<Start>%jd</Start>\n", indent,		sbuf_printf(sb, "%s<Start>%jd</Start>\n", indent,
(intmax_t)disk->d_start);		(intmax_t)disk->d_start);
} else {		} else {
sbuf_printf(sb, "%s<ID>%u</ID>\n", indent, (u_int)sc->sc_id);		sbuf_printf(sb, "%s<ID>%u</ID>\n", indent, (u_int)sc->sc_id);
sbuf_printf(sb, "%s<Type>", indent);		sbuf_printf(sb, "%s<Type>", indent);
switch (sc->sc_type) {		switch (sc->sc_type) {
Show All 12 Lines	sbuf_printf(sb, "%s<Status>Total=%u, Online=%u</Status>\n",
indent, sc->sc_ndisks, g_concat_nvalid(sc));		indent, sc->sc_ndisks, g_concat_nvalid(sc));
sbuf_printf(sb, "%s<State>", indent);		sbuf_printf(sb, "%s<State>", indent);
if (sc->sc_provider != NULL && sc->sc_provider->error == 0)		if (sc->sc_provider != NULL && sc->sc_provider->error == 0)
sbuf_printf(sb, "UP");		sbuf_printf(sb, "UP");
else		else
sbuf_printf(sb, "DOWN");		sbuf_printf(sb, "DOWN");
sbuf_printf(sb, "</State>\n");		sbuf_printf(sb, "</State>\n");
}		}
		end:
		sx_sunlock(&sc->sc_lock_append);
}		}

DECLARE_GEOM_CLASS(g_concat_class, g_concat);		DECLARE_GEOM_CLASS(g_concat_class, g_concat);
MODULE_VERSION(geom_concat, 0);		MODULE_VERSION(geom_concat, 0);