Return-Path: Received: from mx1.redhat.com ([209.132.183.28]:5285 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758818Ab0J1NzN (ORCPT ); Thu, 28 Oct 2010 09:55:13 -0400 Date: Thu, 28 Oct 2010 09:55:07 -0400 From: Jeff Layton To: Trond Myklebust Cc: Steve Dickson , Linux NFS Mailing List Subject: Re: [PATCH] [bz 192] Fixed Regression in NFS Direct I/O path Message-ID: <20101028095507.3a11ee17@tlielax.poochiereds.net> In-Reply-To: <20101028083435.35c990f0@tlielax.poochiereds.net> References: <1288268274-9444-1-git-send-email-steved@redhat.com> <20101028083435.35c990f0@tlielax.poochiereds.net> Content-Type: text/plain; charset=US-ASCII Sender: linux-nfs-owner@vger.kernel.org List-ID: MIME-Version: 1.0 On Thu, 28 Oct 2010 08:34:35 -0400 Jeff Layton wrote: > On Thu, 28 Oct 2010 08:17:54 -0400 > Steve Dickson wrote: > > > A typo, introduced by commit f11ac8db, in the nfs_direct_write() > > routine causes writes with O_DIRECT set to fail with a ENOMEM error. > > > > Found-by: Jeff Layton > > Signed-off-by: Steve Dickson > > --- > > fs/nfs/direct.c | 2 +- > > 1 files changed, 1 insertions(+), 1 deletions(-) > > > > diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c > > index 064a809..84d3c8b 100644 > > --- a/fs/nfs/direct.c > > +++ b/fs/nfs/direct.c > > @@ -873,7 +873,7 @@ static ssize_t nfs_direct_write(struct kiocb *iocb, const struct iovec *iov, > > dreq->inode = inode; > > dreq->ctx = get_nfs_open_context(nfs_file_open_context(iocb->ki_filp)); > > dreq->l_ctx = nfs_get_lock_context(dreq->ctx); > > - if (dreq->l_ctx != NULL) > > + if (dreq->l_ctx == NULL) > > goto out_release; > > if (!is_sync_kiocb(iocb)) > > dreq->iocb = iocb; > > Also, since get_lock_context holds references, this prevents the fs > from being unmounted. It looks like this bug is in 2.6.36 too, so this > may be suitable for stable series as well. > Oh...and another thing I noticed too... nfs_create_request doesn't check for a NULL return from nfs_get_lock_context. If it ever does, it looks like that will likely trickle down to an oops in encode_stateid. It might be good to fix that as well. Maybe something like this compile-tested-only patch? --------------------[snip]--------------------- nfs: handle lock context allocation failures in nfs_create_request nfs_get_lock_context can return NULL on an allocation failure. Signed-off-by: Jeff Layton --- fs/nfs/pagelist.c | 8 +++++++- 1 files changed, 7 insertions(+), 1 deletions(-) diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c index 9194902..137b549 100644 --- a/fs/nfs/pagelist.c +++ b/fs/nfs/pagelist.c @@ -65,6 +65,13 @@ nfs_create_request(struct nfs_open_context *ctx, struct inode *inode, if (req == NULL) return ERR_PTR(-ENOMEM); + /* get lock context early so we can deal with alloc failures */ + req->wb_lock_context = nfs_get_lock_context(ctx); + if (req->wb_lock_context == NULL) { + nfs_page_free(req); + return ERR_PTR(-ENOMEM); + } + /* Initialize the request struct. Initially, we assume a * long write-back delay. This will be adjusted in * update_nfs_request below if the region is not locked. */ @@ -79,7 +86,6 @@ nfs_create_request(struct nfs_open_context *ctx, struct inode *inode, req->wb_pgbase = offset; req->wb_bytes = count; req->wb_context = get_nfs_open_context(ctx); - req->wb_lock_context = nfs_get_lock_context(ctx); kref_init(&req->wb_kref); return req; } -- 1.7.2.3