Updating based on PR Feedback(1)

Updating code based on PR code comments. I adjusted the following parts
of code based on comments:
1.  Revert dbuf_undirty() to original logic and got rid of uncessary
    code change.
2.  Cleanup in abd_impl.h
3.  Cleanup in abd.h
4.  Got rid of duplicate declaration of dmu_buf_hold_noread() in dmu.h.
5.  Cleaned up comment for db_mtx in dmu_imp.h
6.  Updated zfsprop man page to state correct ZFS version
7.  Updated to correct cast in zfs_uio_page_aligned() calls to use
    uintptr_t.
8.  Cleaned up comment in FreeBSD uio code.
9.  Removed unnecessary format changes in comments in Linux abd code.
10.  Updated ZFS VFS hook for direct_IO to use PANIC().
11. Updated comment above dbuf_undirty to use double space again.
12. Converted module paramter zfs_vdev_direct_write_verify_pct OS
    indedepent and in doing so this removed the uneccessary check for
    bounds.
13. Updated to casting in zfs_dio_page_aligned to uniptr_t and added
    kernel guard.
14. Updated zfs_dio_size_aligned() to use modulo math because
    dn->dn_datablksz is not required to be a power of 2.
15. Removed abd scatter stats update calls from all
    ABD_FLAG_FROM_PAGES.
16. Updated check in abd_alloc_from_pages() for the linear page. This
    way a single page that is even 4K can represented as an
    ABD_FLAG_LINEAR_PAGE.
17. Updated npages in zfs_uio_dio_t struct to be size_t.

Signed-off-by: Brian Atkinson <[email protected]>

Author: bwatkinson

include/os/freebsd/spl/sys/mod_os.h

@@ -100,9 +100,6 @@
 #define	spa_taskq_write_param_set_args(var) \
     CTLTYPE_STRING, NULL, 0, spa_taskq_write_param, "A"
 
-#define	param_set_direct_write_verify_pct_args(var) \
-    CTLTYPE_UINT, NULL, 0, param_set_direct_write_verify_pct, "IU"
-
 #define	fletcher_4_param_set_args(var) \
     CTLTYPE_STRING, NULL, 0, fletcher_4_param, "A"
 

include/os/freebsd/spl/sys/uio.h

@@ -50,7 +50,7 @@ typedef	enum uio_rw	zfs_uio_rw_t;
  */
 typedef struct {
 	vm_page_t	*pages;
-	int		npages;
+	size_t		npages;
 } zfs_uio_dio_t;
 
 typedef struct zfs_uio {

include/os/linux/spl/sys/uio.h

@@ -65,7 +65,7 @@ typedef enum zfs_uio_seg {
  */
 typedef struct {
 	struct page	**pages;	/* Mapped pages */
-	int		npages;		/* Number of mapped pages */
+	size_t		npages;		/* Number of mapped pages */
 } zfs_uio_dio_t;
 
 typedef struct zfs_uio {

include/sys/abd.h

@@ -34,7 +34,9 @@
 extern "C" {
 #endif
 
+#if defined(__FreeBSD__) && defined(_KERNEL)
 struct sf_buf;
+#endif
 
 typedef enum abd_flags {
 	ABD_FLAG_LINEAR		= 1 << 0, /* is buffer linear (or scattered)? */
@@ -71,8 +73,11 @@ typedef struct abd {
 		} abd_scatter;
 		struct abd_linear {
 			void		*abd_buf;
-			struct scatterlist *abd_sgl; /* for LINEAR_PAGE Linux */
+#if defined(__FreeBSD__) && defined(_KERNEL)
 			struct sf_buf	*sf; /* for LINEAR_PAGE FreeBSD */
+#else
+			struct scatterlist *abd_sgl; /* for LINEAR_PAGE Linux */
+#endif
 		} abd_linear;
 		struct abd_gang {
 			list_t abd_gang_chain;

include/sys/abd_impl.h

@@ -72,9 +72,10 @@ struct abd_iter {
 	size_t		iter_pos;
 	size_t		iter_offset;	/* offset in current sg/abd_buf, */
 					/* abd_offset included */
-	struct scatterlist *iter_sg;	/* current sg */
 #if defined(__FreeBSD__) && defined(_KERNEL)
 	struct sf_buf	*sf;		/* used to map in vm_page_t FreeBSD */
+#else
+	struct scatterlist *iter_sg;	/* current sg */
 #endif
 };
 

include/sys/dmu.h

@@ -568,8 +568,6 @@ int dmu_spill_hold_existing(dmu_buf_t *bonus, const void *tag, dmu_buf_t **dbp);
  *
  * The object number must be a valid, allocated object number.
  */
-int dmu_buf_hold_noread(objset_t *os, uint64_t object, uint64_t offset,
-    const void *tag, dmu_buf_t **dbp);
 int dmu_buf_hold(objset_t *os, uint64_t object, uint64_t offset,
     const void *tag, dmu_buf_t **, int flags);
 int dmu_buf_hold_array(objset_t *os, uint64_t object, uint64_t offset,

include/sys/dmu_impl.h

@@ -138,7 +138,7 @@ extern "C" {
  * 	db_data_pending
  * 	db_dirtied
  * 	db_link
- * 	dbuf_dirty_records
+ * 	db_dirty_records
  * 	db_dirtycnt
  * 	db_d.*
  * 	db.*

include/sys/uio_impl.h

@@ -49,12 +49,14 @@ extern void zfs_uio_free_dio_pages(zfs_uio_t *, zfs_uio_rw_t);
 extern int zfs_uio_get_dio_pages_alloc(zfs_uio_t *, zfs_uio_rw_t);
 extern boolean_t zfs_uio_page_aligned(zfs_uio_t *);
 
+#ifdef _KERNEL
 static inline boolean_t
 zfs_dio_page_aligned(void *buf)
 {
-	return ((((unsigned long)(buf) & (PAGESIZE - 1)) == 0) ?
+	return ((((uintptr_t)(buf) & (PAGESIZE - 1)) == 0) ?
 	    B_TRUE : B_FALSE);
 }
+#endif
 
 static inline boolean_t
 zfs_dio_offset_aligned(uint64_t offset, uint64_t blksz)
@@ -65,7 +67,7 @@ zfs_dio_offset_aligned(uint64_t offset, uint64_t blksz)
 static inline boolean_t
 zfs_dio_size_aligned(uint64_t size, uint64_t blksz)
 {
-	return (IS_P2ALIGNED(size, blksz));
+	return ((size % blksz) == 0);
 }
 
 static inline boolean_t

include/sys/vdev_impl.h

@@ -658,7 +658,6 @@ int param_set_max_auto_ashift(ZFS_MODULE_PARAM_ARGS);
  * VDEV checksum verification precentage for Direct I/O writes
  */
 extern uint_t zfs_vdev_direct_write_verify_pct;
-int param_set_direct_write_verify_pct(ZFS_MODULE_PARAM_ARGS);
 
 #ifdef	__cplusplus
 }

lib/libspl/include/sys/uio.h

@@ -98,7 +98,7 @@ zfs_dio_offset_aligned(uint64_t offset, uint64_t blksz)
 static inline boolean_t
 zfs_dio_size_aligned(uint64_t size, uint64_t blksz)
 {
-	return (IS_P2ALIGNED(size, blksz));
+	return ((size % blksz) == 0);
 }
 
 static inline boolean_t

man/man7/zfsprops.7

@@ -1054,7 +1054,7 @@ causes every properly aligned read or write to be treated as a direct request.
 .Sy disabled
 causes the O_DIRECT flag to be silently ignored and all direct requests will
 be handled by the ARC.
-This is the default behavior for OpenZFS 2.1 and prior releases.
+This is the default behavior for OpenZFS 2.2 and prior releases.
 .Pp
 Bypassing the ARC requires that a direct request be correctly aligned.
 For write requests the starting offset and size of the request must be

module/Kbuild.in

@@ -445,7 +445,6 @@ ZFS_OBJS_OS := \
 	vdev_disk.o \
 	vdev_file.o \
 	vdev_label_os.o \
-	vdev_os.o \
 	zfs_acl.o \
 	zfs_ctldir.o \
 	zfs_debug.o \

module/os/freebsd/spl/spl_uio.c

@@ -119,7 +119,7 @@ zfs_uio_page_aligned(zfs_uio_t *uio)
 	const struct iovec *iov = GET_UIO_STRUCT(uio)->uio_iov;
 
 	for (int i = zfs_uio_iovcnt(uio); i > 0; iov++, i--) {
-		unsigned long addr = (unsigned long)iov->iov_base;
+		uintptr_t addr = (uintptr_t)iov->iov_base;
 		size_t size = iov->iov_len;
 		if ((addr & (PAGE_SIZE - 1)) || (size & (PAGE_SIZE - 1))) {
 				return (B_FALSE);
@@ -140,7 +140,7 @@ zfs_uio_set_pages_to_stable(zfs_uio_t *uio)
 
 	obj = uio->uio_dio.pages[0]->object;
 	zfs_vmobject_wlock(obj);
-	for (int i = 0; i < uio->uio_dio.npages; i++) {
+	for (size_t i = 0; i < uio->uio_dio.npages; i++) {
 		vm_page_t page = uio->uio_dio.pages[i];
 
 		ASSERT3P(page, !=, NULL);
@@ -160,7 +160,7 @@ static void
 zfs_uio_release_stable_pages(zfs_uio_t *uio)
 {
 	ASSERT3P(uio->uio_dio.pages, !=, NULL);
-	for (int i = 0; i < uio->uio_dio.npages; i++) {
+	for (size_t i = 0; i < uio->uio_dio.npages; i++) {
 		vm_page_t page = uio->uio_dio.pages[i];
 
 		ASSERT3P(page, !=, NULL);
@@ -177,7 +177,7 @@ zfs_uio_set_pages_to_stable(zfs_uio_t *uio)
 	ASSERT3P(uio->uio_dio.pages, !=, NULL);
 	ASSERT3U(uio->uio_dio.npages, >, 0);
 
-	for (int i = 0; i < uio->uio_dio.npages; i++) {
+	for (size_t i = 0; i < uio->uio_dio.npages; i++) {
 		vm_page_t page = uio->uio_dio.pages[i];
 		ASSERT3P(page, !=, NULL);
 
@@ -191,7 +191,7 @@ static void
 zfs_uio_release_stable_pages(zfs_uio_t *uio)
 {
 	ASSERT3P(uio->uio_dio.pages, !=, NULL);
-	for (int i = 0; i < uio->uio_dio.npages; i++) {
+	for (size_t i = 0; i < uio->uio_dio.npages; i++) {
 		vm_page_t page = uio->uio_dio.pages[i];
 
 		ASSERT3P(page, !=, NULL);
@@ -205,13 +205,13 @@ zfs_uio_release_stable_pages(zfs_uio_t *uio)
  * If the operation is marked as read, then we are stating the pages will be
  * written to and must be given write access.
  */
-static int
-zfs_uio_hold_pages(unsigned long start, size_t len, unsigned long nr_pages,
+static size_t
+zfs_uio_hold_pages(unsigned long start, size_t len, size_t nr_pages,
     zfs_uio_rw_t rw, vm_page_t *pages)
 {
 	vm_map_t map;
 	vm_prot_t prot;
-	int count;
+	size_t count;
 
 	map = &curthread->td_proc->p_vmspace->vm_map;
 	ASSERT3S(len, >, 0);
@@ -224,10 +224,10 @@ zfs_uio_hold_pages(unsigned long start, size_t len, unsigned long nr_pages,
 }
 
 static void
-zfs_uio_unhold_pages(vm_page_t *m, int count)
+zfs_uio_unhold_pages(vm_page_t *m, size_t count)
 {
 #if __FreeBSD_version < 1300050
-	for (int i = 0; i < count; i++) {
+	for (size_t i = 0; i < count; i++) {
 		vm_page_t page = m[i];
 		ASSERT3P(page, !=, NULL);
 		vm_page_lock(page);
@@ -256,11 +256,11 @@ zfs_uio_free_dio_pages(zfs_uio_t *uio, zfs_uio_rw_t rw)
 	    uio->uio_dio.npages * sizeof (vm_page_t));
 }
 
-static long
-zfs_uio_get_user_pages(unsigned long start, unsigned long nr_pages,
+static size_t
+zfs_uio_get_user_pages(unsigned long start, size_t nr_pages,
     size_t len, zfs_uio_rw_t rw, vm_page_t *pages)
 {
-	int count;
+	size_t count;
 
 	count = zfs_uio_hold_pages(start, len, nr_pages, rw, pages);
 
@@ -286,21 +286,20 @@ zfs_uio_get_user_pages(unsigned long start, unsigned long nr_pages,
 }
 
 static size_t
-zfs_uio_iov_step(struct iovec v, zfs_uio_t *uio, int *numpages)
+zfs_uio_iov_step(struct iovec v, zfs_uio_t *uio, size_t *numpages)
 {
 	unsigned long addr = (unsigned long)(v.iov_base);
 	size_t len = v.iov_len;
-	int n = DIV_ROUND_UP(len, PAGE_SIZE);
+	size_t n = DIV_ROUND_UP(len, PAGE_SIZE);
 
-	int res = zfs_uio_get_user_pages(
+	size_t res = zfs_uio_get_user_pages(
 	    P2ALIGN_TYPED(addr, PAGE_SIZE, unsigned long), n, len,
 	    zfs_uio_rw(uio), &uio->uio_dio.pages[uio->uio_dio.npages]);
 	if (res != n) {
-		*numpages = -1;
-		return (SET_ERROR(EFAULT));
+		return (0);
 	}
 
-	ASSERT3S(len, ==, res * PAGE_SIZE);
+	ASSERT3U(len, ==, res * PAGE_SIZE);
 	*numpages = res;
 	return (len);
 }
@@ -316,7 +315,7 @@ zfs_uio_get_dio_pages_impl(zfs_uio_t *uio)
 
 	for (int i = 0; i < zfs_uio_iovcnt(uio); i++) {
 		struct iovec iov;
-		int numpages = 0;
+		size_t numpages = 0;
 
 		if (iovp->iov_len == 0) {
 			iovp++;
@@ -326,8 +325,8 @@ zfs_uio_get_dio_pages_impl(zfs_uio_t *uio)
 		iov.iov_base = iovp->iov_base;
 		size_t left = zfs_uio_iov_step(iov, uio, &numpages);
 
-		if (numpages == -1)
-			return (left);
+		if (left == 0)
+			return (SET_ERROR(EFAULT));
 
 		ASSERT3U(left, ==, iov.iov_len);
 		uio->uio_dio.npages += numpages;
@@ -343,7 +342,7 @@ zfs_uio_get_dio_pages_impl(zfs_uio_t *uio)
 
 /*
  * This function maps user pages into the kernel. In the event that the user
- * pages were not mapped successfully an error value is reutrned.
+ * pages were not mapped successfully an error value is returned.
  *
  * On success, 0 is returned.
  */

module/os/freebsd/zfs/abd_os.c

@@ -144,15 +144,9 @@ abd_update_scatter_stats(abd_t *abd, abd_stats_op_t op)
 {
 	uint_t n;
 
-	if (abd_is_from_pages(abd))
-		n = abd_chunkcnt_for_bytes(abd->abd_size);
-	else
-		n = abd_scatter_chunkcnt(abd);
+	n = abd_scatter_chunkcnt(abd);
 	ASSERT(op == ABDSTAT_INCR || op == ABDSTAT_DECR);
 	int waste = (n << PAGE_SHIFT) - abd->abd_size;
-	ASSERT3U(n, >, 0);
-	ASSERT3S(waste, >=, 0);
-	IMPLY(abd_is_linear_page(abd), waste < PAGE_SIZE);
 	if (op == ABDSTAT_INCR) {
 		ABDSTAT_BUMP(abdstat_scatter_cnt);
 		ABDSTAT_INCR(abdstat_scatter_data_size, abd->abd_size);
@@ -367,7 +361,6 @@ abd_free_linear_page(abd_t *abd)
 	ASSERT3P(abd->abd_u.abd_linear.sf, !=, NULL);
 	zfs_unmap_page(abd->abd_u.abd_linear.sf);
 
-	abd_update_scatter_stats(abd, ABDSTAT_DECR);
 #else
 	/*
 	 * The ABD flag ABD_FLAG_LINEAR_PAGE should only be set in
@@ -477,14 +470,13 @@ abd_alloc_from_pages(vm_page_t *pages, unsigned long offset, uint64_t size)
 	abd->abd_flags |= ABD_FLAG_OWNER | ABD_FLAG_FROM_PAGES;
 	abd->abd_size = size;
 
-	if (size < PAGE_SIZE) {
+	if ((offset + size) <= PAGE_SIZE) {
 		/*
-		 * We do not have a full page so we will just use  a linear ABD.
-		 * We have to make sure to take into account the offset though.
-		 * In all other cases our offset will be 0 as we are always
-		 * PAGE_SIZE aligned.
+		 * There is only a single page worth of data, so we will just
+		 * use  a linear ABD. We have to make sure to take into account
+		 * the offset though. In all other cases our offset will be 0
+		 * as we are always PAGE_SIZE aligned.
 		 */
-		ASSERT3U(offset + size, <=, PAGE_SIZE);
 		abd->abd_flags |= ABD_FLAG_LINEAR | ABD_FLAG_LINEAR_PAGE;
 		ABD_LINEAR_BUF(abd) = (char *)zfs_map_page(pages[0],
 		    &abd->abd_u.abd_linear.sf) + offset;
@@ -499,8 +491,6 @@ abd_alloc_from_pages(vm_page_t *pages, unsigned long offset, uint64_t size)
 			ABD_SCATTER(abd).abd_chunks[i] = pages[i];
 	}
 
-	abd_update_scatter_stats(abd, ABDSTAT_INCR);
-
 	return (abd);
 }
 

module/os/freebsd/zfs/sysctl_os.c

@@ -832,35 +832,6 @@ SYSCTL_PROC(_vfs_zfs, OID_AUTO, max_auto_ashift,
 	" new top-level vdevs. (LEGACY)");
 /* END CSTYLED */
 
-int
-param_set_direct_write_verify_pct(SYSCTL_HANDLER_ARGS)
-{
-	int val;
-	int err;
-
-	val = zfs_vdev_direct_write_verify_pct;
-	err = sysctl_handle_int(oidp, &val, 0, req);
-	if (err != 0 || req->newptr == NULL)
-		return (SET_ERROR(err));
-
-	if (val > 100 || val < 0)
-		return (SET_ERROR(EINVAL));
-
-	zfs_vdev_direct_write_verify_pct = val;
-
-	return (0);
-}
-
-/* BEGIN CSTYLED */
-SYSCTL_PROC(_vfs_zfs, OID_AUTO, direct_write_verify_pct,
-	CTLTYPE_UINT | CTLFLAG_RWTUN | CTLFLAG_MPSAFE,
-	&zfs_vdev_direct_write_verify_pct,
-	sizeof (zfs_vdev_direct_write_verify_pct),
-	param_set_direct_write_verify_pct, "IU",
-	"Percentage of Direct I/O writes per top-level VDEV for checksum"
-	" verification to be performed");
-/* END CSTYLED */
-
 /*
  * Since the DTL space map of a vdev is not expected to have a lot of
  * entries, we default its block size to 4K.