diff mbox

ceph: fix readpage from fscache

Message ID 20170804035146.47341-1-zyan@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Yan, Zheng Aug. 4, 2017, 3:51 a.m. UTC 
ceph_readpage() unlocks page prematurely prematurely in the case
that page is reading from fscache. Caller of readpage expects that
page is uptodate when it get unlocked. So page shoule get locked
by completion callback of fscache_read_or_alloc_pages()

Signed-off-by: "Yan, Zheng" <zyan@redhat.com>
---
 fs/ceph/addr.c  | 26 +++++++++++++++++---------
 fs/ceph/cache.c | 12 +++---------
 2 files changed, 20 insertions(+), 18 deletions(-)

Comments

Jeff Layton Aug. 4, 2017, 9:56 a.m. UTC | #1 
On Fri, 2017-08-04 at 11:51 +0800, Yan, Zheng wrote:
> ceph_readpage() unlocks page prematurely prematurely in the case
> that page is reading from fscache. Caller of readpage expects that
> page is uptodate when it get unlocked. So page shoule get locked
> by completion callback of fscache_read_or_alloc_pages()
> 
> Signed-off-by: "Yan, Zheng" <zyan@redhat.com>
> ---
>  fs/ceph/addr.c  | 26 +++++++++++++++++---------
>  fs/ceph/cache.c | 12 +++---------
>  2 files changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index 1b303f0..fc35627 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -189,7 +189,7 @@ static int ceph_releasepage(struct page *page, gfp_t g)
>  /*
>   * read a single page, without unlocking it.
>   */
> -static int readpage_nounlock(struct file *filp, struct page *page)
> +static int ceph_do_readpage(struct file *filp, struct page *page)
>  {
>  	struct inode *inode = file_inode(filp);
>  	struct ceph_inode_info *ci = ceph_inode(inode);
> @@ -218,8 +218,10 @@ static int readpage_nounlock(struct file *filp, struct page *page)
>  	}
>  
>  	err = ceph_readpage_from_fscache(inode, page);
> -	if (err == 0)
> +	if (err == 0) {
> +		err = -EINPROGRESS;
>  		goto out;
> +	}
>  
>  	dout("readpage inode %p file %p page %p index %lu\n",
>  	     inode, filp, page, page->index);
> @@ -249,8 +251,11 @@ static int readpage_nounlock(struct file *filp, struct page *page)
>  
>  static int ceph_readpage(struct file *filp, struct page *page)
>  {
> -	int r = readpage_nounlock(filp, page);
> -	unlock_page(page);
> +	int r = ceph_do_readpage(filp, page);
> +	if (r != -EINPROGRESS)
> +		unlock_page(page);
> +	else
> +		r = 0;
>  	return r;
>  }
>  
> @@ -1216,7 +1221,7 @@ static int ceph_update_writeable_page(struct file *file,
>  			goto retry_locked;
>  		r = writepage_nounlock(page, NULL);
>  		if (r < 0)
> -			goto fail_nosnap;
> +			goto fail_unlock;
>  		goto retry_locked;
>  	}
>  
> @@ -1244,11 +1249,14 @@ static int ceph_update_writeable_page(struct file *file,
>  	}
>  
>  	/* we need to read it. */
> -	r = readpage_nounlock(file, page);
> -	if (r < 0)
> -		goto fail_nosnap;
> +	r = ceph_do_readpage(file, page);
> +	if (r < 0) {
> +		if (r == -EINPROGRESS)
> +			return -EAGAIN;
> +		goto fail_unlock;
> +	}
>  	goto retry_locked;
> -fail_nosnap:
> +fail_unlock:
>  	unlock_page(page);
>  	return r;
>  }
> diff --git a/fs/ceph/cache.c b/fs/ceph/cache.c
> index fd11728..337f886 100644
> --- a/fs/ceph/cache.c
> +++ b/fs/ceph/cache.c
> @@ -297,13 +297,7 @@ void ceph_fscache_file_set_cookie(struct inode *inode, struct file *filp)
>  	}
>  }
>  
> -static void ceph_vfs_readpage_complete(struct page *page, void *data, int error)
> -{
> -	if (!error)
> -		SetPageUptodate(page);
> -}
> -
> -static void ceph_vfs_readpage_complete_unlock(struct page *page, void *data, int error)
> +static void ceph_readpage_from_fscache_complete(struct page *page, void *data, int error)
>  {
>  	if (!error)
>  		SetPageUptodate(page);
> @@ -331,7 +325,7 @@ int ceph_readpage_from_fscache(struct inode *inode, struct page *page)
>  		return -ENOBUFS;
>  
>  	ret = fscache_read_or_alloc_page(ci->fscache, page,
> -					 ceph_vfs_readpage_complete, NULL,
> +					 ceph_readpage_from_fscache_complete, NULL,
>  					 GFP_KERNEL);
>  
>  	switch (ret) {
> @@ -360,7 +354,7 @@ int ceph_readpages_from_fscache(struct inode *inode,
>  		return -ENOBUFS;
>  
>  	ret = fscache_read_or_alloc_pages(ci->fscache, mapping, pages, nr_pages,
> -					  ceph_vfs_readpage_complete_unlock,
> +					  ceph_readpage_from_fscache_complete,
>  					  NULL, mapping_gfp_mask(mapping));
>  
>  	switch (ret) {

Reviewed-by: Jeff Layton <jlayton@redhat.com>
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index 1b303f0..fc35627 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -189,7 +189,7 @@  static int ceph_releasepage(struct page *page, gfp_t g)
 /*
  * read a single page, without unlocking it.
  */
-static int readpage_nounlock(struct file *filp, struct page *page)
+static int ceph_do_readpage(struct file *filp, struct page *page)
 {
 	struct inode *inode = file_inode(filp);
 	struct ceph_inode_info *ci = ceph_inode(inode);
@@ -218,8 +218,10 @@  static int readpage_nounlock(struct file *filp, struct page *page)
 	}
 
 	err = ceph_readpage_from_fscache(inode, page);
-	if (err == 0)
+	if (err == 0) {
+		err = -EINPROGRESS;
 		goto out;
+	}
 
 	dout("readpage inode %p file %p page %p index %lu\n",
 	     inode, filp, page, page->index);
@@ -249,8 +251,11 @@  static int readpage_nounlock(struct file *filp, struct page *page)
 
 static int ceph_readpage(struct file *filp, struct page *page)
 {
-	int r = readpage_nounlock(filp, page);
-	unlock_page(page);
+	int r = ceph_do_readpage(filp, page);
+	if (r != -EINPROGRESS)
+		unlock_page(page);
+	else
+		r = 0;
 	return r;
 }
 
@@ -1216,7 +1221,7 @@  static int ceph_update_writeable_page(struct file *file,
 			goto retry_locked;
 		r = writepage_nounlock(page, NULL);
 		if (r < 0)
-			goto fail_nosnap;
+			goto fail_unlock;
 		goto retry_locked;
 	}
 
@@ -1244,11 +1249,14 @@  static int ceph_update_writeable_page(struct file *file,
 	}
 
 	/* we need to read it. */
-	r = readpage_nounlock(file, page);
-	if (r < 0)
-		goto fail_nosnap;
+	r = ceph_do_readpage(file, page);
+	if (r < 0) {
+		if (r == -EINPROGRESS)
+			return -EAGAIN;
+		goto fail_unlock;
+	}
 	goto retry_locked;
-fail_nosnap:
+fail_unlock:
 	unlock_page(page);
 	return r;
 }
diff --git a/fs/ceph/cache.c b/fs/ceph/cache.c
index fd11728..337f886 100644
--- a/fs/ceph/cache.c
+++ b/fs/ceph/cache.c
@@ -297,13 +297,7 @@  void ceph_fscache_file_set_cookie(struct inode *inode, struct file *filp)
 	}
 }
 
-static void ceph_vfs_readpage_complete(struct page *page, void *data, int error)
-{
-	if (!error)
-		SetPageUptodate(page);
-}
-
-static void ceph_vfs_readpage_complete_unlock(struct page *page, void *data, int error)
+static void ceph_readpage_from_fscache_complete(struct page *page, void *data, int error)
 {
 	if (!error)
 		SetPageUptodate(page);
@@ -331,7 +325,7 @@  int ceph_readpage_from_fscache(struct inode *inode, struct page *page)
 		return -ENOBUFS;
 
 	ret = fscache_read_or_alloc_page(ci->fscache, page,
-					 ceph_vfs_readpage_complete, NULL,
+					 ceph_readpage_from_fscache_complete, NULL,
 					 GFP_KERNEL);
 
 	switch (ret) {
@@ -360,7 +354,7 @@  int ceph_readpages_from_fscache(struct inode *inode,
 		return -ENOBUFS;
 
 	ret = fscache_read_or_alloc_pages(ci->fscache, mapping, pages, nr_pages,
-					  ceph_vfs_readpage_complete_unlock,
+					  ceph_readpage_from_fscache_complete,
 					  NULL, mapping_gfp_mask(mapping));
 
 	switch (ret) {