Messages in this thread Patch in this message | | | Date | Mon, 19 Jan 1998 21:41:28 -0500 | From | Bill Hawes <> | Subject | patch for 2.1.79 nfs write.c |
| |
Linus Torvalds wrote:
> Note that it should be entirely correct to mark it uptodate, BUT the page > should be marked locked until the copy has been successfully completed. > Otherwise some reader might get in while the page is still being written.
I've attached a patch that partially addresses the problem with the pages marked up-to-date in advance. The logic in nfs_updatepage has been reordered slightly to separate the case of a pre-existing request and a new request. On the latter branch, any failure to create the write request will result in the PG_uptodate bit being cleared before the page is unlocked.
By reordering I've eliminated one of the copy_to_user() calls and added a check for faults in the copy. The case of an existing request being updated still needs a check for faults.
Other minor changes include not refreshing the inode if a read or write call failed, as the fattr would probably be invalid. I've also added a NFS_WRITE_COMPLETE flag to indicate completed RPC requests that might still be on the inode writeback queue.
Regards, Bill--- linux-2.1.79/include/linux/nfs_fs.h.old Thu Jan 15 16:51:51 1998 +++ linux-2.1.79/include/linux/nfs_fs.h Sun Jan 18 11:43:41 1998 @@ -118,7 +118,8 @@ #define NFS_WRITE_CANCELLED 0x0004 /* has been cancelled */ #define NFS_WRITE_UNCOMMITTED 0x0008 /* written but uncommitted (NFSv3) */ #define NFS_WRITE_INVALIDATE 0x0010 /* invalidate after write */ -#define NFS_WRITE_INPROGRESS 0x0020 /* RPC call in progress */ +#define NFS_WRITE_INPROGRESS 0x0100 /* RPC call in progress */ +#define NFS_WRITE_COMPLETE 0x0200 /* RPC call completed */ #define WB_WANTLOCK(req) ((req)->wb_flags & NFS_WRITE_WANTLOCK) #define WB_HAVELOCK(req) ((req)->wb_flags & NFS_WRITE_LOCKED) @@ -126,6 +127,7 @@ #define WB_UNCOMMITTED(req) ((req)->wb_flags & NFS_WRITE_UNCOMMITTED) #define WB_INVALIDATE(req) ((req)->wb_flags & NFS_WRITE_INVALIDATE) #define WB_INPROGRESS(req) ((req)->wb_flags & NFS_WRITE_INPROGRESS) +#define WB_COMPLETE(req) ((req)->wb_flags & NFS_WRITE_COMPLETE) /* * linux/fs/nfs/proc.c --- linux-2.1.79/fs/nfs/write.c.old Tue Jan 13 10:38:29 1998 +++ linux-2.1.79/fs/nfs/write.c Mon Jan 19 21:39:18 1998 @@ -128,7 +128,7 @@ req->wb_flags |= NFS_WRITE_LOCKED; rpc_wake_up_task(&req->wb_task); - dprintk("nfs: wake up task %d (flags %x)\n", + dprintk("NFS: wake up task %d (flags %x)\n", req->wb_task.tk_pid, req->wb_flags); } @@ -182,8 +182,8 @@ } while (count); io_error: - /* N.B. do we want to refresh if there was an error?? (fattr valid?) */ - if (refresh) { + /* Note: we don't refresh if the call failed (fattr invalid) */ + if (refresh && result >= 0) { /* See comments in nfs_wback_result */ /* N.B. I don't think this is right -- sync writes in order */ if (fattr.size < inode->i_size) @@ -509,25 +509,16 @@ nfs_updatepage(struct dentry *dentry, struct page *page, const char *buffer, unsigned long offset, unsigned int count, int sync) { - struct inode *inode = dentry->d_inode; + struct inode *inode = dentry->d_inode; + u8 *page_addr = (u8 *) page_address(page); struct nfs_wreq *req; int status = 0, page_locked = 1; - u8 *page_addr; dprintk("NFS: nfs_updatepage(%s/%s %d@%ld, sync=%d)\n", dentry->d_parent->d_name.name, dentry->d_name.name, count, page->offset+offset, sync); set_bit(PG_locked, &page->flags); - page_addr = (u8 *) page_address(page); - - /* If wsize is smaller than page size, update and write - * page synchronously. - */ - if (NFS_SERVER(inode)->wsize < PAGE_SIZE) { - copy_from_user(page_addr + offset, buffer, count); - return nfs_writepage_sync(dentry, inode, page, offset, count); - } /* * Try to find a corresponding request on the writeback queue. @@ -541,6 +532,7 @@ */ if ((req = find_write_request(inode, page)) != NULL) { if (update_write_request(req, offset, count)) { + /* N.B. check for a fault here and cancel the req */ copy_from_user(page_addr + offset, buffer, count); goto updated; } @@ -549,16 +541,23 @@ return 0; } + /* Copy data to page buffer. */ + status = -EFAULT; + if (copy_from_user(page_addr + offset, buffer, count)) + goto done; + + /* If wsize is smaller than page size, update and write + * page synchronously. + */ + if (NFS_SERVER(inode)->wsize < PAGE_SIZE) + return nfs_writepage_sync(dentry, inode, page, offset, count); + /* Create the write request. */ status = -ENOBUFS; req = create_write_request(dentry, inode, page, offset, count); if (!req) goto done; - /* Copy data to page buffer. */ - /* N.B. should check for fault here ... */ - copy_from_user(page_addr + offset, buffer, count); - /* Schedule request */ page_locked = schedule_write_request(req, sync); @@ -588,8 +587,14 @@ if ((count = nfs_write_error(inode)) < 0) status = count; } - } else + } else { + if (status < 0) { +printk("NFS: %s/%s write failed, clearing bit\n", +dentry->d_parent->d_name.name, dentry->d_name.name); + clear_bit(PG_uptodate, &page->flags); + } nfs_unlock_page(page); + } } dprintk("NFS: nfs_updatepage returns %d (isize %ld)\n", @@ -815,32 +820,26 @@ struct page *page = req->wb_page; struct dentry *dentry = req->wb_dentry; - dprintk("NFS: %4d nfs_wback_lock (status %d flags %x)\n", - task->tk_pid, task->tk_status, req->wb_flags); + dprintk("NFS: %4d nfs_wback_lock (%s/%s, status=%d flags=%x)\n", + task->tk_pid, dentry->d_parent->d_name.name, + dentry->d_name.name, task->tk_status, req->wb_flags); if (!WB_HAVELOCK(req)) req->wb_flags |= NFS_WRITE_WANTLOCK; - if (WB_WANTLOCK(req) && test_and_set_bit(PG_locked, &page->flags)) { - printk("NFS: page already locked in writeback_lock!\n"); - task->tk_timeout = 2 * HZ; - rpc_sleep_on(&write_queue, task, NULL, NULL); - return; - } - task->tk_status = 0; + if (WB_WANTLOCK(req) && test_and_set_bit(PG_locked, &page->flags)) + goto out_locked; req->wb_flags &= ~NFS_WRITE_WANTLOCK; req->wb_flags |= NFS_WRITE_LOCKED; + task->tk_status = 0; if (req->wb_args == 0) { size_t size = sizeof(struct nfs_writeargs) + sizeof(struct nfs_fattr); void *ptr; - if (!(ptr = kmalloc(size, GFP_KERNEL))) { - task->tk_timeout = HZ; - rpc_sleep_on(&write_queue, task, NULL, NULL); - return; - } + if (!(ptr = kmalloc(size, GFP_KERNEL))) + goto out_no_args; req->wb_args = (struct nfs_writeargs *) ptr; req->wb_fattr = (struct nfs_fattr *) (req->wb_args + 1); } @@ -854,6 +853,18 @@ rpc_call_setup(task, NFSPROC_WRITE, req->wb_args, req->wb_fattr, 0); req->wb_flags |= NFS_WRITE_INPROGRESS; + return; + +out_locked: + printk("NFS: page already locked in writeback_lock!\n"); + task->tk_timeout = 2 * HZ; + rpc_sleep_on(&write_queue, task, NULL, NULL); + return; +out_no_args: + printk("NFS: can't alloc args, sleeping\n"); + task->tk_timeout = HZ; + rpc_sleep_on(&write_queue, task, NULL, NULL); + return; } /* @@ -867,9 +878,12 @@ struct page *page = req->wb_page; int status = task->tk_status; - dprintk("NFS: %4d nfs_wback_result (status %d)\n", - task->tk_pid, status); + dprintk("NFS: %4d nfs_wback_result (%s/%s, status=%d, flags=%x)\n", + task->tk_pid, req->wb_dentry->d_parent->d_name.name, + req->wb_dentry->d_name.name, status, req->wb_flags); + /* Set the WRITE_COMPLETE flag, but leave INPROGRESS set */ + req->wb_flags |= NFS_WRITE_COMPLETE; if (status < 0) { /* * An error occurred. Report the error back to the --- linux-2.1.79/fs/nfs/read.c.old Tue Jan 6 11:39:08 1998 +++ linux-2.1.79/fs/nfs/read.c Mon Jan 19 19:29:11 1998 @@ -115,8 +115,10 @@ result = 0; io_error: - if (refresh) + /* Note: we don't refresh if the call returned error */ + if (refresh && result >= 0) nfs_refresh_inode(inode, &rqst.ra_fattr); + /* N.B. Use nfs_unlock_page here? */ clear_bit(PG_locked, &page->flags); wake_up(&page->wait); return result; @@ -131,17 +133,17 @@ { struct nfs_rreq *req = (struct nfs_rreq *) task->tk_calldata; struct page *page = req->ra_page; + unsigned long address = page_address(page); int result = task->tk_status; static int succ = 0, fail = 0; dprintk("NFS: %4d received callback for page %lx, result %d\n", - task->tk_pid, page_address(page), result); + task->tk_pid, address, result); if (result >= 0) { result = req->ra_res.count; if (result < PAGE_SIZE) { - memset((char *) page_address(page) + result, 0, - PAGE_SIZE - result); + memset((char *) address + result, 0, PAGE_SIZE - result); } nfs_refresh_inode(req->ra_inode, &req->ra_fattr); set_bit(PG_uptodate, &page->flags); @@ -151,10 +153,11 @@ fail++; dprintk("NFS: %d successful reads, %d failures\n", succ, fail); } + /* N.B. Use nfs_unlock_page here? */ clear_bit(PG_locked, &page->flags); wake_up(&page->wait); - free_page(page_address(page)); + free_page(address); rpc_release_task(task); kfree(req); @@ -164,38 +167,45 @@ nfs_readpage_async(struct dentry *dentry, struct inode *inode, struct page *page) { + unsigned long address = page_address(page); struct nfs_rreq *req; - int result, flags; + int result = -1, flags; dprintk("NFS: nfs_readpage_async(%p)\n", page); - flags = RPC_TASK_ASYNC | (IS_SWAPFILE(inode)? NFS_RPC_SWAPFLAGS : 0); + if (NFS_CONGESTED(inode)) + goto out_defer; - if (NFS_CONGESTED(inode) - || !(req = (struct nfs_rreq *) rpc_allocate(flags, sizeof(*req)))) { - dprintk("NFS: deferring async READ request.\n"); - return -1; - } + /* N.B. Do we need to test? Never called for swapfile inode */ + flags = RPC_TASK_ASYNC | (IS_SWAPFILE(inode)? NFS_RPC_SWAPFLAGS : 0); + req = (struct nfs_rreq *) rpc_allocate(flags, sizeof(*req)); + if (!req) + goto out_defer; /* Initialize request */ + /* N.B. Will the dentry remain valid for life of request? */ nfs_readreq_setup(req, NFS_FH(dentry), page->offset, - (void *) page_address(page), PAGE_SIZE); + (void *) address, PAGE_SIZE); req->ra_inode = inode; - req->ra_page = page; + req->ra_page = page; /* count has been incremented by caller */ /* Start the async call */ dprintk("NFS: executing async READ request.\n"); result = rpc_do_call(NFS_CLIENT(inode), NFSPROC_READ, &req->ra_args, &req->ra_res, flags, nfs_readpage_result, req); + if (result < 0) + goto out_free; + result = 0; +out: + return result; - if (result >= 0) { - atomic_inc(&page->count); - return 0; - } - +out_defer: + dprintk("NFS: deferring async READ request.\n"); + goto out; +out_free: dprintk("NFS: failed to enqueue async READ request.\n"); kfree(req); - return -1; + goto out; } /* @@ -214,20 +224,20 @@ nfs_readpage(struct dentry *dentry, struct page *page) { struct inode *inode = dentry->d_inode; - unsigned long address; int error = -1; - dprintk("NFS: nfs_readpage %08lx\n", page_address(page)); + dprintk("NFS: nfs_readpage (%p %ld@%ld)\n", + page, PAGE_SIZE, page->offset); set_bit(PG_locked, &page->flags); - address = page_address(page); atomic_inc(&page->count); if (!IS_SWAPFILE(inode) && !PageError(page) && NFS_SERVER(inode)->rsize >= PAGE_SIZE) error = nfs_readpage_async(dentry, inode, page); - if (error < 0) /* couldn't enqueue */ + if (error < 0) { /* couldn't enqueue */ error = nfs_readpage_sync(dentry, inode, page); - if (error < 0 && IS_SWAPFILE(inode)) - printk("Aiee.. nfs swap-in of page failed!\n"); - free_page(address); + if (error < 0 && IS_SWAPFILE(inode)) + printk("Aiee.. nfs swap-in of page failed!\n"); + free_page(page_address(page)); + } return error; } | |