Lay down some comments related to the previous few revisions of ufs_vnops.c.

sys/ufs/ufs/ufs_vnops.c

@@ -1,4 +1,4 @@
-/*	$NetBSD: ufs_vnops.c,v 1.246 2019/02/25 06:00:40 dholland Exp $	*/
+/*	$NetBSD: ufs_vnops.c,v 1.247 2019/07/01 00:57:06 dholland Exp $	*/
 
 /*-
  * Copyright (c) 2008 The NetBSD Foundation, Inc.
@@ -66,7 +66,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: ufs_vnops.c,v 1.246 2019/02/25 06:00:40 dholland Exp $");
+__KERNEL_RCSID(0, "$NetBSD: ufs_vnops.c,v 1.247 2019/07/01 00:57:06 dholland Exp $");
 
 #if defined(_KERNEL_OPT)
 #include "opt_ffs.h"
@@ -1257,7 +1257,12 @@ ufs_readdir(void *v)
 
 	KASSERT(VOP_ISLOCKED(vp));
 
-	/* figure out where we want to read */
+	/*
+	 * Figure out where the user wants us to read and how much.
+	 *
+	 * XXX: there should probably be an upper bound on callerbytes
+	 * to avoid silliness trying to do large kernel allocations.
+	 */
 	callerbytes = calleruio->uio_resid;
 	startoffset = calleruio->uio_offset;
 	endoffset = startoffset + callerbytes;
@@ -1267,7 +1272,39 @@ ufs_readdir(void *v)
 		return EINVAL;
 	}
 
-	/* round start and end down to block boundaries */
+	/*
+	 * Now figure out where to actually start reading. Round the
+	 * start down to a block boundary: we need to start at the
+	 * beginning of a block in order to read the directory
+	 * correctly.
+	 *
+	 * We also want to always read a whole number of blocks so
+	 * that the copying code below doesn't have to worry about
+	 * partial entries. (It used to try at one point, and was a
+	 * horrible mess.)
+	 *
+	 * Furthermore, since blocks have to be scanned from the
+	 * beginning, if we go partially into another block now we'll
+	 * just have to rescan it on the next readdir call, which
+	 * doesn't really serve any useful purpose.
+	 *
+	 * So, round down the end as well. It's ok to underpopulate
+	 * the transfer buffer, as long as we send back at least one
+	 * dirent so as to avoid giving a bogus EOF indication.
+	 *
+	 * Note that because dirents are larger than ffs struct
+	 * directs, despite the rounding down we may not be able to
+	 * send all the entries in the blocks we read and may have to
+	 * rescan some of them on the next call anyway. Alternatively
+	 * if there's empty space on disk we might have actually been
+	 * able to fit the next block in, and so forth. None of this
+	 * actually matters that much in practice.
+	 *
+	 * XXX: what does ffs do if a directory block becomes
+	 * completely empty, and what happens if all the blocks we
+	 * read are completely empty even though we aren't at EOF? As
+	 * of this writing I (dholland) can't remember the details.
+	 */
 	physstart = rounddown2(startoffset, ump->um_dirblksiz);
 	physend = rounddown2(endoffset, ump->um_dirblksiz);
 
@@ -1276,10 +1313,42 @@ ufs_readdir(void *v)
 		return EINVAL;
 	}
 
+	/*
+	 * skipstart is the number of bytes we need to read in
+	 * (because we need to start at the beginning of a block) but
+	 * not transfer to the user.
+	 *
+	 * dropend is the number of bytes to ignore at the end of the
+	 * user's buffer.
+	 */
 	skipstart = startoffset - physstart;
 	dropend = endoffset - physend;
 
-	/* how much to actually read */
+	/*
+	 * Make a transfer buffer.
+	 *
+	 * Note: rawbufmax = physend - physstart. Proof:
+	 *
+	 * physend - physstart = physend - physstart
+	 *   = physend - physstart + startoffset - startoffset
+	 *   = physend + (startoffset - physstart) - startoffset
+	 *   = physend + skipstart - startoffset
+	 *   = physend + skipstart - startoffset + endoffset - endoffset
+	 *   = skipstart - startoffset + endoffset - (endoffset - physend)
+	 *   = skipstart - startoffset + endoffset - dropend
+	 *   = skipstart - startoffset + (startoffset + callerbytes) - dropend
+	 *   = skipstart + callerbytes - dropend
+	 *   = rawbufmax
+	 * Qed.
+	 *
+	 * XXX: this should just use physend - physstart.
+	 *
+	 * XXX: this should be rewritten to read the directs straight
+	 * out of bufferio buffers instead of copying twice. This would
+	 * also let us adapt better to the user's buffer size.
+	 */
+
+	/* Base buffer space for CALLERBYTES of new data */
 	rawbufmax = callerbytes + skipstart;
 	if (rawbufmax < callerbytes)
 		return EINVAL;