lkml.org 
[lkml]   [1998]   [Jan]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
Subjectpatch 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;
}
\
 
 \ /
  Last update: 2005-03-22 13:41    [W:0.043 / U:0.624 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site