Always unlock pages when returning from writepage
authorSimon Wilkinson <sxw@inf.ed.ac.uk>
Fri, 16 Oct 2009 15:39:24 +0000 (16:39 +0100)
committerDerrick Brashear <shadow|account-1000005@unknown>
Mon, 19 Oct 2009 23:39:57 +0000 (16:39 -0700)
Writepage has a return path which returns an error with a locked
page. However, all returns that are not AOP_WRITEPAGE_ACTIVATE must
unlock their pages - using this codepath would leave a stray page lock,
which would eventually hang the machine.

The logic behind the -EIO return was also incorrect. In the Linux page
cache model, truncates simply reduce the size recorded in the inode. If
there are pages pending writeback then they may still have writepage() called
upon them - it's up to the writepage routine to discard the write
request (rather than returning an error)

Reviewed-on: http://gerrit.openafs.org/685
Tested-by: Derrick Brashear <shadow@dementia.org>
Reviewed-by: Derrick Brashear <shadow@dementia.org>

src/afs/LINUX/osi_vnodeops.c

index 75a5103..4be7eea 100644 (file)
@@ -2067,31 +2067,41 @@ afs_linux_writepage(struct page *pp)
 {
     struct address_space *mapping = pp->mapping;
     struct inode *inode;
-    unsigned long end_index;
-    unsigned offset = PAGE_CACHE_SIZE;
-    long status;
+    unsigned int to = PAGE_CACHE_SIZE;
+    loff_t isize;
+    int status = 0;
 
     if (PageReclaim(pp)) {
        return AOP_WRITEPAGE_ACTIVATE;
+       /* XXX - Do we need to redirty the page here? */
     }
 
+    page_cache_get(pp);
+
     inode = (struct inode *)mapping->host;
-    end_index = i_size_read(inode) >> PAGE_CACHE_SHIFT;
-
-    /* easy case */
-    if (pp->index < end_index)
-       goto do_it;
-    /* things got complicated... */
-    offset = i_size_read(inode) & (PAGE_CACHE_SIZE - 1);
-    /* OK, are we completely out? */
-    if (pp->index >= end_index + 1 || !offset)
-       return -EIO;
-  do_it:
-    status = afs_linux_writepage_sync(inode, pp, 0, offset);
+    isize = i_size_read(inode);
+
+    /* Don't defeat an earlier truncate */
+    if (page_offset(pp) > isize)
+       goto done;
+
+    /* If this is the final page, then just write the number of bytes that
+     * are actually in it */
+    if ((isize - page_offset(pp)) < to )
+       to = isize - page_offset(pp);
+
+    status = afs_linux_writepage_sync(inode, pp, 0, to);
+
+done:
     SetPageUptodate(pp);
-    if ( status != AOP_WRITEPAGE_ACTIVATE )
+    if ( status != AOP_WRITEPAGE_ACTIVATE ) {
+       /* XXX - do we need to redirty the page here? */
        UnlockPage(pp);
-    if (status == offset)
+    }
+
+    page_cache_release(pp);
+
+    if (status == to)
        return 0;
     else
        return status;