afs: Do not ignore errors in afs_CacheFetchProc 28/13428/2
authorAndrew Deason <adeason@sinenomine.net>
Thu, 17 Jan 2019 06:12:06 +0000 (00:12 -0600)
committerBenjamin Kaduk <kaduk@mit.edu>
Fri, 25 Jan 2019 04:29:43 +0000 (23:29 -0500)
afs_CacheFetchProc currently has a section of code that looks like
this pseudocode:

    if (!code) do {
        while (length > 0) {
            code = read_from_rx();
            if (code) {
                break;
            }
            code = write_to_cache();
            if (code) {
                break;
            }
        }
        code = 0;
    } while (moredata);
    return code;

When we encounter an error when reading from rx or writing to the
cache, we break out of the current loop to stop processing and return
an error. But there are _two_ loops in this section of the code, so
what we actually do is break out of the inner loop, set 'code' to 0,
and then usually return (since 'moredata' is usually never set).

This means that when we encounter an unexpected error either from the
net or disk (or the memcache layer), we ignore the error and return
success. This means that we'll store a subset of the relevant chunk's
data to disk, and flag that chunk as complete and valid for the
relevant DV. If the error occurred before we wrote anything to disk,
this means we'll store an empty chunk and flag it as valid. The chunk
will be flagged as valid forever, serving invalid data, until the
cache chunk is evicted or manually kicked out. This can result in
files and directories appearing blank or truncated to applications
until the bad chunk is removed.

Possibly the most common way to encounter this issue is when using a
disk cache, and the underlying disk partition is full, resulting in an
unexpected ENOSPC error. Theoretically this can be seen from an
unexpected error from Rx, but we would have to see a short read from
Rx without the Rx call being aborted. If the call was aborted, we'd
get an error from the call to rx_EndCall() later on.

To fix this, change all of these 'break's into 'goto done's, to be
more explicit about where we are jumping to. Convert all of the
'break's in this function in the same way, to make the code flow more
consistent and easier to follow. Remove the 'if () do' on a single
line, since it makes it a little harder to see from a casual glance
that there are two nested loops here.

This problem appears to have been introduced in commit 61ae8792 (Unite
CacheFetchProcs and add abstraction calls), included in OpenAFS
1.5.62.

Change-Id: Ib965a526604e629dc5401fa0f1e335ce61b31b30
Reviewed-on: https://gerrit.openafs.org/13428
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>

src/afs/afs_fetchstore.c

index eb32bbe..ed25a4d 100644 (file)
@@ -1150,11 +1150,15 @@ afs_CacheFetchProc(struct afs_conn *tc, struct rx_connection *rxconn,
 
     adc->validPos = base;
 
-    if ( !code ) do {
+    if (code) {
+        goto done;
+    }
+
+    do {
        if (avc->f.states & CForeign) {
            code = (*ops->more)(rock, &length, &moredata);
            if ( code )
-               break;
+               goto done;
        }
 #ifndef AFS_NOSTATS
        bytesToXfer += length;
@@ -1177,11 +1181,11 @@ afs_CacheFetchProc(struct afs_conn *tc, struct rx_connection *rxconn,
                           ICL_TYPE_POINTER, avc, ICL_TYPE_INT32, code,
                           ICL_TYPE_INT32, length);
                code = -34;
-               break;
+               goto done;
            }
            code = (*ops->write)(rock, fP, offset, bytesread, &byteswritten);
            if ( code )
-               break;
+               goto done;
            offset += bytesread;
            base += bytesread;
            length -= bytesread;
@@ -1194,6 +1198,7 @@ afs_CacheFetchProc(struct afs_conn *tc, struct rx_connection *rxconn,
        }
        code = 0;
     } while (moredata);
+ done:
     if (!code)
        code = (*ops->close)(rock, avc, adc, tsmall);
     if (ops)