ihandle: Fix IH_REALLYCLOSE for positional I/O

Currently, ih_fdclose (which is called by IH_REALLYCLOSE), goes
through every FD_HANDLE_OPEN FdHandle_t and closes it. If it finds
handles that are FD_HANDLE_INUSE, it skips those and sets a flag on
the parent IHandle_t. For non-positional I/O, any future opens cannot
use these _INUSE handles, since _INUSE handles cannot be reused, and
the handle will be actually closed when it is FDH_CLOSE'd.

For positional I/O, the situation is different. Multiple threads can
use the same _INUSE FdHandle_t, and so there is nothing currently
stopping a thread from IH_OPEN'ing an ihandle that has been
IH_REALLYCLOSE'd, and getting back an FdHandle_t that existed before
the IH_REALLYCLOSE was issued. This is important, since IH_REALLYCLOSE
is used on files that are deleted, and future IH_OPENs for the same
inode must not use the cached file descriptor. Getting this wrong can
cause data loss, since it can cause us to read from or write to a file
descriptor referring to a deleted file, when we instead should open a
new copy of that file.

To fix this, we create a new FdHandle_t state called
FD_HANDLE_CLOSING, which is set in IH_REALLYCLOSE if we encounter an
FD_HANDLE_INUSE FdHandle_t. In IH_OPEN, we always skip
FD_HANDLE_CLOSING handles, so we can never get back a cached file
descriptor from before an IH_REALLYCLOSE call.

Change-Id: I3188a18f7833950cf5454b3ffe4a4ce0c69e234f
Reviewed-on: http://gerrit.openafs.org/5308
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Derrick Brashear <shadow@dementix.org>

diff --git a/src/vol/ihandle.c b/src/vol/ihandle.c
index 7d8a17e..8c0e83b 100644 (file)
--- a/src/vol/ihandle.c
+++ b/src/vol/ihandle.c
@@ -325,6 +325,11 @@ ih_open(IHandle_t * ihP)
 
     /* Do we already have an open file handle for this Inode? */
     for (fdP = ihP->ih_fdtail; fdP != NULL; fdP = fdP->fd_ihprev) {
+       if (fdP->fd_status == FD_HANDLE_CLOSING) {
+           /* The handle was open when an IH_REALLYCLOSE was issued, so we
+            * cannot reuse it; it will be closed soon. */
+           continue;
+       }
 #ifndef HAVE_PIO
        /*
         * If we don't have positional i/o, don't try to share fds, since
@@ -429,7 +434,8 @@ fd_close(FdHandle_t * fdP)
     IH_LOCK;
     osi_Assert(ih_Inited);
     osi_Assert(fdInUseCount > 0);
-    osi_Assert(fdP->fd_status == FD_HANDLE_INUSE);
+    osi_Assert(fdP->fd_status == FD_HANDLE_INUSE ||
+               fdP->fd_status == FD_HANDLE_CLOSING);
 
     ihP = fdP->fd_ih;
 
@@ -438,7 +444,8 @@ fd_close(FdHandle_t * fdP)
      * failed (this is determined by checking the ihandle for the flag
      * IH_REALLY_CLOSED) or we have too many open files.
      */
-    if (ihP->ih_flags & IH_REALLY_CLOSED || fdInUseCount > fdCacheSize) {
+    if (fdP->fd_status == FD_HANDLE_CLOSING ||
+        ihP->ih_flags & IH_REALLY_CLOSED || fdInUseCount > fdCacheSize) {
        IH_UNLOCK;
        return fd_reallyclose(fdP);
     }
@@ -479,7 +486,8 @@ fd_reallyclose(FdHandle_t * fdP)
     IH_LOCK;
     osi_Assert(ih_Inited);
     osi_Assert(fdInUseCount > 0);
-    osi_Assert(fdP->fd_status == FD_HANDLE_INUSE);
+    osi_Assert(fdP->fd_status == FD_HANDLE_INUSE ||
+               fdP->fd_status == FD_HANDLE_CLOSING);
 
     ihP = fdP->fd_ih;
     closeFd = fdP->fd_fd;
@@ -783,7 +791,8 @@ ih_fdclose(IHandle_t * ihP)
        next = fdP->fd_ihnext;
        osi_Assert(fdP->fd_ih == ihP);
        osi_Assert(fdP->fd_status == FD_HANDLE_OPEN
-              || fdP->fd_status == FD_HANDLE_INUSE);
+              || fdP->fd_status == FD_HANDLE_INUSE
+              || fdP->fd_status == FD_HANDLE_CLOSING);
        if (fdP->fd_status == FD_HANDLE_OPEN) {
            DLL_DELETE(fdP, ihP->ih_fdhead, ihP->ih_fdtail, fd_ihnext,
                       fd_ihprev);
@@ -791,6 +800,7 @@ ih_fdclose(IHandle_t * ihP)
            DLL_INSERT_TAIL(fdP, head, tail, fd_next, fd_prev);
        } else {
            closedAll = 0;
+           fdP->fd_status = FD_HANDLE_CLOSING;
            ihP->ih_flags |= IH_REALLY_CLOSED;
        }
     }

diff --git a/src/vol/ihandle.h b/src/vol/ihandle.h
index 08e2d8b..8652f88 100644 (file)
--- a/src/vol/ihandle.h
+++ b/src/vol/ihandle.h
@@ -171,6 +171,9 @@ typedef struct FdHandle_s {
 #define FD_HANDLE_AVAIL                1       /* handle is not open and available */
 #define FD_HANDLE_OPEN         2       /* handle is open and not in use */
 #define FD_HANDLE_INUSE                3       /* handle is open and in use */
+#define FD_HANDLE_CLOSING      4       /* handle is open, in use, and has been
+                                        * IH_REALLYCLOSE'd. It should not be
+                                        * used for subsequent opens. */
 
 /* buffered file descriptor handle */
 #define STREAM_HANDLE_BUFSIZE  2048    /* buffer size for STR_READ/STR_WRITE */