NFS: Clean up nfs_update_request()

Simplify the loop in nfs_update_request by moving into a separate function
the code that attempts to update an existing cached NFS write.

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>

1 files changed, 103 insertions, 98 deletions

diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 21d8a48b624..04f51e52e18 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -34,9 +34,6 @@
 /*
  * Local function declarations
  */
-static struct nfs_page * nfs_update_request(struct nfs_open_context*,
-                                            struct page *,
-                                            unsigned int, unsigned int);
 static void nfs_pageio_init_write(struct nfs_pageio_descriptor *desc,
                                   struct inode *inode, int ioflags);
 static void nfs_redirty_request(struct nfs_page *req);
@@ -169,30 +166,6 @@ static void nfs_mark_uptodate(struct page *page, unsigned int base, unsigned int
         SetPageUptodate(page);
 }
 
-static int nfs_writepage_setup(struct nfs_open_context *ctx, struct page *page,
-                unsigned int offset, unsigned int count)
-{
-        struct nfs_page *req;
-        int ret;
-
-        for (;;) {
-                req = nfs_update_request(ctx, page, offset, count);
-                if (!IS_ERR(req))
-                        break;
-                ret = PTR_ERR(req);
-                if (ret != -EBUSY)
-                        return ret;
-                ret = nfs_wb_page(page->mapping->host, page);
-                if (ret != 0)
-                        return ret;
-        }
-        /* Update file length */
-        nfs_grow_file(page, offset, count);
-        nfs_mark_uptodate(page, req->wb_pgbase, req->wb_bytes);
-        nfs_clear_page_tag_locked(req);
-        return 0;
-}
-
 static int wb_priority(struct writeback_control *wbc)
 {
         if (wbc->for_reclaim)
@@ -356,11 +329,19 @@ int nfs_writepages(struct address_space *mapping, struct writeback_control *wbc)
 /*
  * Insert a write request into an inode
  */
-static void nfs_inode_add_request(struct inode *inode, struct nfs_page *req)
+static int nfs_inode_add_request(struct inode *inode, struct nfs_page *req)
 {
         struct nfs_inode *nfsi = NFS_I(inode);
         int error;
 
+        error = radix_tree_preload(GFP_NOFS);
+        if (error != 0)
+                goto out;
+
+        /* Lock the request! */
+        nfs_lock_request_dontget(req);
+
+        spin_lock(&inode->i_lock);
         error = radix_tree_insert(&nfsi->nfs_page_tree, req->wb_index, req);
         BUG_ON(error);
         if (!nfsi->npages) {
@@ -374,6 +355,10 @@ static void nfs_inode_add_request(struct inode *inode, struct nfs_page *req)
         kref_get(&req->wb_kref);
         radix_tree_tag_set(&nfsi->nfs_page_tree, req->wb_index,
                                 NFS_PAGE_TAG_LOCKED);
+        spin_unlock(&inode->i_lock);
+        radix_tree_preload_end();
+out:
+        return error;
 }
 
 /*
@@ -565,101 +550,121 @@ static inline int nfs_scan_commit(struct inode *inode, struct list_head *dst, pg
 #endif
 
 /*
- * Try to update any existing write request, or create one if there is none.
- * In order to match, the request's credentials must match those of
- * the calling process.
+ * Search for an existing write request, and attempt to update
+ * it to reflect a new dirty region on a given page.
  *
- * Note: Should always be called with the Page Lock held!
+ * If the attempt fails, then the existing request is flushed out
+ * to disk.
  */
-static struct nfs_page * nfs_update_request(struct nfs_open_context* ctx,
-                struct page *page, unsigned int offset, unsigned int bytes)
+static struct nfs_page *nfs_try_to_update_request(struct inode *inode,
+                struct page *page,
+                unsigned int offset,
+                unsigned int bytes)
 {
-        struct address_space *mapping = page->mapping;
-        struct inode *inode = mapping->host;
-        struct nfs_page         *req, *new = NULL;
-        pgoff_t         rqend, end;
+        struct nfs_page *req;
+        unsigned int rqend;
+        unsigned int end;
+        int error;
+
+        if (!PagePrivate(page))
+                return NULL;
 
         end = offset + bytes;
+        spin_lock(&inode->i_lock);
 
         for (;;) {
-                /* Loop over all inode entries and see if we find
-                 * A request for the page we wish to update
-                 */
-                spin_lock(&inode->i_lock);
                 req = nfs_page_find_request_locked(page);
-                if (req) {
-                        if (!nfs_set_page_tag_locked(req)) {
-                                int error;
-
-                                spin_unlock(&inode->i_lock);
-                                error = nfs_wait_on_request(req);
-                                nfs_release_request(req);
-                                if (error < 0) {
-                                        if (new) {
-                                                radix_tree_preload_end();
-                                                nfs_release_request(new);
-                                        }
-                                        return ERR_PTR(error);
-                                }
-                                continue;
-                        }
-                        spin_unlock(&inode->i_lock);
-                        if (new) {
-                                radix_tree_preload_end();
-                                nfs_release_request(new);
-                        }
+                if (req == NULL)
+                        goto out_unlock;
+
+                rqend = req->wb_offset + req->wb_bytes;
+                /*
+                 * Tell the caller to flush out the request if
+                 * the offsets are non-contiguous.
+                 * Note: nfs_flush_incompatible() will already
+                 * have flushed out requests having wrong owners.
+                 */
+                if (!nfs_dirty_request(req)
+                    || offset > rqend
+                    || end < req->wb_offset)
+                        goto out_flushme;
+
+                if (nfs_set_page_tag_locked(req))
                         break;
-                }
 
-                if (new) {
-                        nfs_lock_request_dontget(new);
-                        nfs_inode_add_request(inode, new);
-                        spin_unlock(&inode->i_lock);
-                        radix_tree_preload_end();
-                        req = new;
-                        goto out;
-                }
+                /* The request is locked, so wait and then retry */
                 spin_unlock(&inode->i_lock);
-
-                new = nfs_create_request(ctx, inode, page, offset, bytes);
-                if (IS_ERR(new))
-                        return new;
-                if (radix_tree_preload(GFP_NOFS)) {
-                        nfs_release_request(new);
-                        return ERR_PTR(-ENOMEM);
-                }
-        }
-
-        /* We have a request for our page.
-         * If the creds don't match, or the
-         * page addresses don't match,
-         * tell the caller to wait on the conflicting
-         * request.
-         */
-        rqend = req->wb_offset + req->wb_bytes;
-        if (req->wb_context != ctx
-            || req->wb_page != page
-            || !nfs_dirty_request(req)
-            || offset > rqend || end < req->wb_offset) {
-                nfs_clear_page_tag_locked(req);
-                return ERR_PTR(-EBUSY);
+                error = nfs_wait_on_request(req);
+                nfs_release_request(req);
+                if (error != 0)
+                        goto out_err;
+                spin_lock(&inode->i_lock);
         }
 
         /* Okay, the request matches. Update the region */
         if (offset < req->wb_offset) {
                 req->wb_offset = offset;
                 req->wb_pgbase = offset;
-                req->wb_bytes = max(end, rqend) - req->wb_offset;
-                goto out;
         }
-
         if (end > rqend)
                 req->wb_bytes = end - req->wb_offset;
+        else
+                req->wb_bytes = rqend - req->wb_offset;
+out_unlock:
+        spin_unlock(&inode->i_lock);
+        return req;
+out_flushme:
+        spin_unlock(&inode->i_lock);
+        nfs_release_request(req);
+        error = nfs_wb_page(inode, page);
+out_err:
+        return ERR_PTR(error);
+}
 
+/*
+ * Try to update an existing write request, or create one if there is none.
+ *
+ * Note: Should always be called with the Page Lock held to prevent races
+ * if we have to add a new request. Also assumes that the caller has
+ * already called nfs_flush_incompatible() if necessary.
+ */
+static struct nfs_page * nfs_setup_write_request(struct nfs_open_context* ctx,
+                struct page *page, unsigned int offset, unsigned int bytes)
+{
+        struct inode *inode = page->mapping->host;
+        struct nfs_page *req;
+        int error;
+
+        req = nfs_try_to_update_request(inode, page, offset, bytes);
+        if (req != NULL)
+                goto out;
+        req = nfs_create_request(ctx, inode, page, offset, bytes);
+        if (IS_ERR(req))
+                goto out;
+        error = nfs_inode_add_request(inode, req);
+        if (error != 0) {
+                nfs_release_request(req);
+                req = ERR_PTR(error);
+        }
 out:
         return req;
 }
 
+static int nfs_writepage_setup(struct nfs_open_context *ctx, struct page *page,
+                unsigned int offset, unsigned int count)
+{
+        struct nfs_page *req;
+
+        req = nfs_setup_write_request(ctx, page, offset, count);
+        if (IS_ERR(req))
+                return PTR_ERR(req);
+        /* Update file length */
+        nfs_grow_file(page, offset, count);
+        nfs_mark_uptodate(page, req->wb_pgbase, req->wb_bytes);
+        nfs_clear_page_tag_locked(req);
+        return 0;
+}
+
 int nfs_flush_incompatible(struct file *file, struct page *page)
 {
         struct nfs_open_context *ctx = nfs_file_open_context(file);