WIP: zvol: factor out zvol write & discard into common code

[problame]

I'm not sure whether this is a great idea because the uio type is
different on the platforms and not treated as an opaque type in the
code.

My motivation for sharing the code is a larger refactoring of ZVOL
replay (not part of this patch) that aligns ZVOL replay with how the ZPL
does replay in an effort to enforce some rather ... intricate .. details
of ZIL replay through assertions in the ZIL code.

Specifically, ZPL currently replays by

zfs_replay_OP -> zfs_OP -> zfs_log_OP -> { dmu_tx_create(), zil_replaying(), ..., dmu_..., }

ZVOL currently doesn't call zil_replaying() which means that we don't
record replay progress. This is just barely acceptable for ZVOLs because
all log entries are idempotent. But since the ZIL is shared between ZPL
and ZVOL it prevents us from enforcing in the ZIL code that a zfs_replay_OP
calls zil_replaying() to confirm successful replay.

[problame]

This is a draft patch as a basis for discussion.
I factored it out of a larger project and didn't want to put in the effort to rebase it onto d0cd9a5 because it's irrelevant for the discussion.

[ahrens]

Seems like good cleanup to me.

[behlendorf]

I'm not sure whether this is a great idea because the uio type is different on the platforms and not treated as an opaque type in the code.

For any place where this is not the case we should fix it. The idea behind introducing the zfs_uio_t type was that it should be treated as an opaque type in all of the common code. The zfs_uio_* accessor macro's were introduced and should be used exclusively in the common code. Platform specific code can manipulate the internals of zfs_uio_t as needed.

[problame]

I started to rebase this patch onto the zfs_uio_t changes and addressed the issued raised in review so far.
However, I got stuck (see below) and didn't want to waste effort cleaning things up until I know how to proceed.

• dataset read /write kstat accounting happens in common code
• moved zvol_read() to common code
• I discovered FreeBSD's zvol_geom_bio_strategy which was not part of the original RFC commit
  • zvol_geom_bio_strategy on FreeBSD uses FreeBSD's struct bio, not ZFS's zfs_uio_t
  • But the now-common zvol_{read,write,truncate} functions expect a zfs_uio_t
  • ISTM that the the proper way to resolve this is to add support for FreeBSD's struct bio to zfs_uio_t, similar to how the Linux zfs_uio_t supports struct bvec.
  • => I restructured zvol_geom_bio_strategy to prepare the way for bio support, lots of XXX comments there
  • @amotin do you agree with this assessment?
  • If so: who is going to do the work on FreeBSD's zfs_uio_t? I am not familiar with FreeBSD and don't have the time / motivation.

[problame]

• it seems like we could use the opportunity to abstract away block I/O accounting as well? See Restore FreeBSD resource usage accounting #11613 (comment)

[kithrup]

I haven't even build-tested this, but something along the lines of the below?

diff --git a/module/os/freebsd/zfs/zvol_os.c b/module/os/freebsd/zfs/zvol_os.c
index 826bb8121..0b05163d8 100644
--- a/module/os/freebsd/zfs/zvol_os.c
+++ b/module/os/freebsd/zfs/zvol_os.c
@@ -657,43 +657,33 @@ zvol_geom_bio_strategy(struct bio *bp)
 		 * => need support for `struct bio` in zfs_uio_t, similar to how we do it on Linux
 		 *    with the `struct bvec`
 		 */
+		uio_t freebsd_uio;
+		struct iovec uio_iovec = {
+			.iov_base = bp->bio_data,
+			.iov_len = bp->bio_length
+		};
+		zfs_uio_t zfs_uio = { &freebsd_uio };
 		uint64_t off = bp->bio_offset;
 		char *addr = bp->bio_data;
 		size_t resid = bp->bio_length;
-		if (resid > 0 && off >= volsize) {
+
+		freebsd_uio.uio_iov = &uio_iovc;
+		freebsd_uio.uio_iovcnt = 1;
+		freebsd_uio.uio_offset = bp->bio_offset;
+		freebsd_uio.uio_resid = bp->bio_length;
+		freebsd_uio.uio_segflg = UIO_SYSSPACE;
+		freebsd_uio.uio_rw = UIO_WRITE;
+		freebsd_uio.uio_td = NULL;	/* Ok? Shouldn't need to use it for UIO_SYSSPACE */
+		
+		if (freebsd.uio_resid > 0 && freebsd_uio.uio_offset >= volsize) {
 			error = SET_ERROR(EIO);
 			goto resume;
 		}
-		zfs_locked_range_t *lr;
-		lr = zfs_rangelock_enter(&zv->zv_rangelock, off, resid, RL_WRITER);
-		while (resid != 0 && off < volsize) {
-			size_t size = MIN(resid, DMU_MAX_ACCESS >> 1);
-			dmu_tx_t *tx = dmu_tx_create(os);
-			dmu_tx_hold_write_by_dnode(tx, zv->zv_dn, off, size);
-			error = dmu_tx_assign(tx, TXG_WAIT);
-			if (error) {
-				dmu_tx_abort(tx);
-			} else {
-				dmu_write(os, ZVOL_OBJ, off, size, addr, tx);
-				zvol_log_write(zv, tx, off, size, sync);
-				dmu_tx_commit(tx);
-			}
-			/* XXX this error handling is (and has been!) different than on the cdev_write code path */
-			if (error) {
-				/* convert checksum errors into IO errors */
-				if (error == ECKSUM)
-					error = SET_ERROR(EIO);
-				break;
-			}
-			off += size;
-			addr += size;
-			resid -= size;
-		}
-		zfs_rangelock_exit(lr);
 
+		error = zvol_write(zv, &zfs_uio, sync);
 
-		bp->bio_completed = bp->bio_length - resid;
-		if (bp->bio_completed < bp->bio_length && off > volsize)
+		bp->bio_completed = bp->bio_length - freebsd_uio.uio_resid;
+		if (bp->bio_completed < bp->bio_length && freebsd_uio.uio_offset > volsize)
 			error = SET_ERROR(EINVAL);
 
 		dataset_kstats_update_write_kstats(&zv->zv_kstat,