[src/trunk]: src/sys/ufs/ufs Lay down some comments related to the previous f...

[dholland <dholland%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/0ed0b4be5c69
branches:  trunk
changeset: 461704:0ed0b4be5c69
user:      dholland <dholland%NetBSD.org@localhost>
date:      Mon Jul 01 00:57:06 2019 +0000

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

diffstat:

 sys/ufs/ufs/ufs_vnops.c |  79 +++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 74 insertions(+), 5 deletions(-)

diffs (117 lines):

diff -r 2978002d477e -r 0ed0b4be5c69 sys/ufs/ufs/ufs_vnops.c
--- a/sys/ufs/ufs/ufs_vnops.c   Sun Jun 30 22:37:11 2019 +0000
+++ b/sys/ufs/ufs/ufs_vnops.c   Mon Jul 01 00:57:06 2019 +0000
@@ -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 @@
 
        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 @@
                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 @@
                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;