Differential D25671 Diff 74456 sys/geom/eli/g_eli_privacy.c

Changeset View

Standalone View

sys/geom/eli/g_eli_privacy.c

Show First 20 Lines • Show All 92 Lines • ▼ Show 20 Lines	g_eli_crypto_read_done(struct cryptop *crp)
if (sc != NULL && crp->crp_cipher_key != NULL)		if (sc != NULL && crp->crp_cipher_key != NULL)
g_eli_key_drop(sc, __DECONST(void *, crp->crp_cipher_key));		g_eli_key_drop(sc, __DECONST(void *, crp->crp_cipher_key));
crypto_freereq(crp);		crypto_freereq(crp);
/*		/*
* Do we have all sectors already?		* Do we have all sectors already?
*/		*/
if (bp->bio_inbed < bp->bio_children)		if (bp->bio_inbed < bp->bio_children)
return (0);		return (0);
		MPASS(((bp->bio_flags & BIO_UNMAPPED) != 0) ==
		(bp->bio_driver2 != NULL));
		if (bp->bio_driver2 != NULL) {
		g_io_bio_copyout(bp->bio_driver2, bp, sc->sc_cpubind);
free(bp->bio_driver2, M_ELI);		free(bp->bio_driver2, M_ELI);
bp->bio_driver2 = NULL;		bp->bio_driver2 = NULL;
		}
if (bp->bio_error != 0) {		if (bp->bio_error != 0) {
G_ELI_LOGREQ(0, bp, "Crypto READ request failed (error=%d).",		G_ELI_LOGREQ(0, bp, "Crypto READ request failed (error=%d).",
bp->bio_error);		bp->bio_error);
bp->bio_completed = 0;		bp->bio_completed = 0;
}		}
/*		/*
* Read is finished, send it up.		* Read is finished, send it up.
*/		*/
▲ Show 20 Lines • Show All 50 Lines • ▼ Show 20 Lines	G_ELI_LOGREQ(0, bp, "Crypto WRITE request failed (error=%d).",
bp->bio_error);		bp->bio_error);
free(bp->bio_driver2, M_ELI);		free(bp->bio_driver2, M_ELI);
bp->bio_driver2 = NULL;		bp->bio_driver2 = NULL;
g_destroy_bio(cbp);		g_destroy_bio(cbp);
g_io_deliver(bp, bp->bio_error);		g_io_deliver(bp, bp->bio_error);
atomic_subtract_int(&sc->sc_inflight, 1);		atomic_subtract_int(&sc->sc_inflight, 1);
return (0);		return (0);
}		}
		if (bp->bio_driver2 != NULL) {
cbp->bio_data = bp->bio_driver2;		cbp->bio_data = bp->bio_driver2;
		kibUnsubmitted Not Done Inline Actions Why do you clear the flag ? kib: Why do you clear the flag ?
		asomersAuthorUnsubmitted Done Inline Actions Because during writes, `g_eli_crypto_run` mallocs a new buffer and points `bp->bio_data` to it. So the bio is mapped now if it wasn't before. asomers: Because during writes, `g_eli_crypto_run` mallocs a new buffer and points `bp->bio_data` to it.
		kibUnsubmitted Done Inline Actions Either this should be stated as comment, or BIO_UNMAPPED cleared at the point where you reset bio_data, or both. kib: Either this should be stated as comment, or BIO_UNMAPPED cleared at the point where you reset…
		cbp->bio_flags &= ~BIO_UNMAPPED;
		markjUnsubmitted Done Inline Actions This line looks like it deserves a comment. Isn't it true that bio_driver2 != NULL if and only if the BIO is unmapped? If so, it seems clearer to check for the latter condition. markj: This line looks like it deserves a comment. Isn't it true that bio_driver2 != NULL if and only…
		}
cbp->bio_done = g_eli_write_done;		cbp->bio_done = g_eli_write_done;
cp = LIST_FIRST(&gp->consumer);		cp = LIST_FIRST(&gp->consumer);
cbp->bio_to = cp->provider;		cbp->bio_to = cp->provider;
G_ELI_LOGREQ(2, cbp, "Sending request.");		G_ELI_LOGREQ(2, cbp, "Sending request.");
/*		/*
* Send encrypted data to the provider.		* Send encrypted data to the provider.
*/		*/
g_io_request(cbp, cp);		g_io_request(cbp, cp);
▲ Show 20 Lines • Show All 75 Lines • ▼ Show 20 Lines	g_eli_crypto_run(struct g_eli_worker wr, struct bio bp)

/*		/*
* If we write the data we cannot destroy current bio_data content,		* If we write the data we cannot destroy current bio_data content,
* so we need to allocate more memory for encrypted data.		* so we need to allocate more memory for encrypted data.
*/		*/
if (bp->bio_cmd == BIO_WRITE) {		if (bp->bio_cmd == BIO_WRITE) {
data = malloc(bp->bio_length, M_ELI, M_WAITOK);		data = malloc(bp->bio_length, M_ELI, M_WAITOK);
bp->bio_driver2 = data;		bp->bio_driver2 = data;
		if ((bp->bio_flags & BIO_UNMAPPED) != 0)
		g_io_bio_copyin(bp, data, sc->sc_cpubind);
		else
bcopy(bp->bio_data, data, bp->bio_length);		bcopy(bp->bio_data, data, bp->bio_length);
} else		} else {
		if ((bp->bio_flags & BIO_UNMAPPED) != 0) {
		data = malloc(bp->bio_length, M_ELI, M_WAITOK);
		bp->bio_driver2 = data;
		g_io_bio_copyin(bp, data, sc->sc_cpubind);
		} else {
		bp->bio_driver2 = NULL;
data = bp->bio_data;		data = bp->bio_data;
		}
		}

for (i = 0, dstoff = bp->bio_offset; i < nsec; i++, dstoff += secsize) {		for (i = 0, dstoff = bp->bio_offset; i < nsec; i++, dstoff += secsize) {
crp = crypto_getreq(wr->w_sid, M_WAITOK);		crp = crypto_getreq(wr->w_sid, M_WAITOK);

		kibUnsubmitted Done Inline Actions Note that this malloc(9) is not needed on DMAP arches, it only adds overhead. Generally, malloc(9) on write path is the source of deadlocks, but probably it is too late to worry about geli. I probably should suggest this explicitly: use unmapped path only on DMAP arches, leave current transient remapping when DMAP is not available. kib: Note that this malloc(9) is not needed on DMAP arches, it only adds overhead. Generally, malloc…
		asomersAuthorUnsubmitted Done Inline Actions So you think that sf_buf would be too slow on non-DMAP arches to be worth using? Ok, that's easy. But why do you say that this malloc is not needed? Are you suggesting that crypto(9) work on an array of vm_page pointers instead or even on a struct bio ? asomers: So you think that sf_buf would be too slow on non-DMAP arches to be worth using? Ok, that's…
		kibUnsubmitted Done Inline Actions Rather I think that non-DMAP arches are already slow, and make the efforts to speed up DMAP arches too hard. I am saying that malloc() is not needed because you already have the bio_ma array and on DMAP arches it is same as the array of sf_bufs. Crypto(9) operating directly on array of vm_pages is the best option, I think. At least, when I worked on unmapped io and busdma, directly taking array of vm_pages was the option that required the minimum amount of plumbing comparing with any other case. kib: Rather I think that non-DMAP arches are already slow, and make the efforts to speed up DMAP…
crypto_use_buf(crp, data, secsize);		crypto_use_buf(crp, data, secsize);
crp->crp_opaque = (void *)bp;		crp->crp_opaque = (void *)bp;
data += secsize;		data += secsize;
if (bp->bio_cmd == BIO_WRITE) {		if (bp->bio_cmd == BIO_WRITE) {
crp->crp_op = CRYPTO_OP_ENCRYPT;		crp->crp_op = CRYPTO_OP_ENCRYPT;
		kibUnsubmitted Done Inline Actions It is not safe to allocate more than one sf buf with blocking. Imagine that there are N total sf bufs in the system, and you have N threads trying to blockingly allocate 2 sf bufs each. The resulting deadlock is real. For instance, sendfile code only blocks for the first sfbuf allocation, all later allocs are non-blocking and code adopts to possible failure. Of course this is only relevant for arches without direct map. kib: It is not safe to allocate more than one sf buf with blocking. Imagine that there are N total…
		asomersAuthorUnsubmitted Done Inline Actions So I need to ensure that any geli worker thread will only block on allocation for up to 1 buffer at a time. Should that be one buffer for the entire thread, or just one buffer for each bio? asomers: So I need to ensure that any geli worker thread will only block on allocation for up to 1…
		asomersAuthorUnsubmitted Done Inline Actions Nevermind, I'm going to do something else instead. But I need to build an x86 head VM first to test it. Don't expect new code until tomorrow. asomers: Nevermind, I'm going to do something else instead. But I need to build an x86 head VM first to…
crp->crp_callback = g_eli_crypto_write_done;		crp->crp_callback = g_eli_crypto_write_done;
} else /* if (bp->bio_cmd == BIO_READ) */ {		} else /* if (bp->bio_cmd == BIO_READ) */ {
crp->crp_op = CRYPTO_OP_DECRYPT;		crp->crp_op = CRYPTO_OP_DECRYPT;
crp->crp_callback = g_eli_crypto_read_done;		crp->crp_callback = g_eli_crypto_read_done;
}		}
crp->crp_flags = CRYPTO_F_CBIFSYNC;		crp->crp_flags = CRYPTO_F_CBIFSYNC;
if (g_eli_batch)		if (g_eli_batch)
crp->crp_flags \|= CRYPTO_F_BATCH;		crp->crp_flags \|= CRYPTO_F_BATCH;
Show All 16 Lines