afs: Do not limit fetches based on vcache length

Currently, when we go to the fileserver to fetch some data, we try to
make sure that we do not ask for data beyond the end of the file. For
example, if our chunk size is 1M, and we need to get the first chunk
for a file that is 4 bytes long, we will only ask the fileserver for 4
bytes.

This can cause issues when the file is being extended at the same time
as when we are trying to read the file. Consider the following
example. There is a file named X that has contents "abcd" at dv 1, and
we issue a FetchData64 request for X, only requesting 4 bytes. Right
before the fileserver gets the FetchData64 request, another client
writes the contents "12345" to file X.

The client will then fetch the contents "1234" for that file, at dv 2,
and store that as the contents of the first chunk for file X. On
subsequent reads for file X, applications will now get "1234<NUL>" as
the contents, since the size of the file will be updated to 5, but the
cache manager thinks that "1234" is the correct contents for the first
chunk of X at dv 2. The cache manager will continue to think so until
the cache entry is evicted or invalidated for whatever reason.

To avoid this scenario, always request a full chunk of data if we have
any data to fetch and the file has not been locally truncated. We can
still avoid the fetch at all if it looks like we're fetching beyond
end-of-file, since we know that at least at some point that was
correct information about the file. If this results in us trying to
fetch beyond end-of-file, the fileserver will respond with the correct
length anyway.

We still need to restrict the fetch request length based on
avc->f.truncPos, since the dcache data after avc->f.truncPos needs to
stay empty, since we don't track truncated data any other way. If we
also avoided this restriction, extending a file via truncation after
reducing a file's length via truncation could cause the old file data
to appear again, instead of filling the new file range with NULs.

Note that on at least Linux, with this fix an application can still
read the contents "1234" on the first read in the above example, and
"12345" on subsequent reads. This is just due to when we give the VFS
updates about file metadata, and could be remedied by updating file
metadata immediately from the FetchStatus information from the
FetchData64 call. However, just reading the contents "1234" in the
above example seems like a somewhat plausible outcome; at the very
least, it is an improvement.

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

diff --git a/src/afs/afs_dcache.c b/src/afs/afs_dcache.c
index 3fc0bb5..be80c7a 100644 (file)
--- a/src/afs/afs_dcache.c
+++ b/src/afs/afs_dcache.c
@@ -2111,6 +2111,32 @@ afs_GetDCache(struct vcache *avc, afs_size_t abyte,
                afs_AdjustSize(tdc, size);      /* changes chunkBytes */
                /* max size for transfer still in size */
            }
+
+           if (size) {
+               /* For the actual fetch, do not limit the request to the
+                * length of the file. If this results in a read past EOF on
+                * the server, the server will just reply with less data than
+                * requested. If we limit ourselves to only requesting data up
+                * to the avc file length, we open ourselves up to races if the
+                * file is extended on the server at about the same time.
+                *
+                * However, we must restrict ourselves to the avc->f.truncPos
+                * length, since this represents an outstanding local
+                * truncation of the file that will be committed to the
+                * fileserver when we actually write the fileserver contents.
+                * If we do not restrict the fetch length based on
+                * avc->f.truncPos, a different truncate operation extending
+                * the file length could cause the old data after
+                * avc->f.truncPos to reappear, instead of extending the file
+                * with NUL bytes. */
+               size = AFS_CHUNKSIZE(abyte);
+               if (Position + size > avc->f.truncPos) {
+                   size = avc->f.truncPos - Position;
+               }
+               if (size < 0) {
+                   size = 0;
+               }
+           }
        }
        if (afs_mariner && !tdc->f.chunk)
            afs_MarinerLog("fetch$Fetching", avc);      /* , Position, size, afs_indexCounter ); */