On Tue, 2008-06-17 at 17:35 -0400, Chuck Lever wrote:
> Hi Trond-
>
> On Jun 16, 2008, at 3:23 PM, Trond Myklebust wrote:
> > 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@xxxxxxxxxx>
> > ---
> >
> > fs/nfs/write.c | 105 +++++++++++++++++++++++++++
> > +----------------------------
> > 1 files changed, 53 insertions(+), 52 deletions(-)
> >
> > diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> > index 8f12157..a675dc9 100644
> > --- a/fs/nfs/write.c
> > +++ b/fs/nfs/write.c
> > @@ -34,9 +34,8 @@
> > /*
> > * Local function declarations
> > */
> > -static struct nfs_page * nfs_update_request(struct nfs_open_context*,
> > - struct page *,
> > - unsigned int, unsigned int);
> > +static struct nfs_page * nfs_setup_write_request(struct
> > nfs_open_context *ctx,
> > + struct page *page, unsigned int offset, unsigned int bytes);
> > 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);
> > @@ -178,7 +177,7 @@ static int nfs_writepage_setup(struct
> > nfs_open_context *ctx, struct page *page,
> > int ret;
> >
> > for (;;) {
> > - req = nfs_update_request(ctx, page, offset, count);
> > + req = nfs_setup_write_request(ctx, page, offset, count);
> > if (!IS_ERR(req))
> > break;
> > ret = PTR_ERR(req);
> > @@ -566,6 +565,41 @@ static inline int nfs_scan_commit(struct inode
> > *inode, struct list_head *dst, pg
> > }
> > #endif
> >
> > +static int nfs_try_to_update_request(struct nfs_page *req,
> > + unsigned int offset,
> > + unsigned int end)
> > +{
> > + unsigned int rqend;
> > +
> > + rqend = req->wb_offset + req->wb_bytes;
> > +
> > + /*
> > + * If the page doesn't match, or the offsets
> > + * are non-contiguous, tell the caller to
> > + * wait on the conflicting request.
> > + */
> > + if (req->wb_page == NULL
> > + || !nfs_dirty_request(req)
> > + || offset > rqend
> > + || end < req->wb_offset)
> > + return -EBUSY;
>
> I don't see how this is equivalent to what it replaces in
> nfs_update_request:
>
> > - if (req->wb_context != ctx
> > - || req->wb_page != page
>
>
> What happened to the context check, for example?
That is a deliberate change.
We hold the page lock across nfs_vm_page_mkwrite and/or
nfs_write_begin()/nfs_write_end(), so nobody else can add a new nfs_page
to page->private during that time.
For that reason, the check that is done in nfs_flush_incompatible()
together with the check for req->wb_page==NULL (done in
try_to_update_page() while still holding the inode->i_lock) means that
the above two checks are redundant.
...actually, come to think of it, the check for req->wb_page == NULL is
redundant too, since nfs_clear_request() is called at the end of
nfs_inode_remove_request(), and hence nfs_page_find_request_locked()
would fail to find anything.
> > +
> > + if (!nfs_set_page_tag_locked(req))
> > + return -EAGAIN;
>
> This -EAGAIN (and the resulting logic in nfs_setup_write_request) is
> headache-inducing. Is there a more straightforward way to break this
> up?
No. If the page is being written out, then we _must_ release all locks,
wait on the request, and try again. This is not a change compared to the
previous incarnation.
> > +
> > + /* Okay, the request matches. Update the region */
> > + if (offset < req->wb_offset) {
> > + req->wb_offset = offset;
> > + req->wb_pgbase = offset;
> > + }
> > + if (end > rqend)
> > + req->wb_bytes = end - req->wb_offset;
> > + else
> > + req->wb_bytes = rqend - req->wb_offset;
> > +
> > + return 0;
> > +}
> > +
> > /*
> > * 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
> > @@ -573,16 +607,15 @@ static inline int nfs_scan_commit(struct inode
> > *inode, struct list_head *dst, pg
> > *
> > * Note: Should always be called with the Page Lock held!
> > */
> > -static struct nfs_page * nfs_update_request(struct
> > nfs_open_context* ctx,
> > +static struct nfs_page * nfs_setup_write_request(struct
> > nfs_open_context* ctx,
> > struct page *page, unsigned int offset, unsigned int bytes)
> > {
> > - struct address_space *mapping = page->mapping;
> > - struct inode *inode = mapping->host;
> > + struct inode *inode = page->mapping->host;
> > struct nfs_page *req, *new = NULL;
> > - pgoff_t rqend, end;
> > + unsigned int end;
> > + int error;
> >
> > end = offset + bytes;
> > -
> > for (;;) {
> > /* Loop over all inode entries and see if we find
> > * A request for the page we wish to update
> > @@ -590,26 +623,16 @@ static struct nfs_page *
> > nfs_update_request(struct nfs_open_context* ctx,
> > 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_try_to_update_request(req, offset, end);
> > + spin_unlock(&inode->i_lock);
> > + if (error == 0)
> > + break;
> > + if (error == -EAGAIN)
> > 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);
> > - }
> > + nfs_release_request(req);
> > + if (error == 0)
> > continue;
> > - }
> > - spin_unlock(&inode->i_lock);
> > - if (new) {
> > - radix_tree_preload_end();
> > - nfs_release_request(new);
> > - }
> > + req = ERR_PTR(error);
> > break;
> > }
> >
> > @@ -632,32 +655,10 @@ static struct nfs_page *
> > nfs_update_request(struct nfs_open_context* ctx,
> > }
> > }
> >
> > - /* 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);
> > + if (new) {
> > + radix_tree_preload_end();
> > + nfs_release_request(new);
> > }
> > -
> > - /* 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;
> > -
> > out:
>
> There's only one "goto out;" left in nfs_setup_write_request(). There
> are other return sites in nfs_setup_write_request() that simply
> "return req;" -- for what purpose is this one goto still here?
It could be replaced by a single 'return req'.
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust@xxxxxxxxxx
www.netapp.com
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
