[PATCH 1/3] ceph: remove useless page->mapping check in writepage_nounlock()

[PATCH 1/3] ceph: remove useless page->mapping check in writepage_nounlock()

[Yan, Zheng]

Callers of writepage_nounlock() have already ensured non-null
page->mapping.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: "Yan, Zheng" <zyan@redhat.com>
---
 fs/ceph/addr.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index f49d6630..ff25239 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -534,10 +534,6 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
 
 	dout("writepage %p idx %lu\n", page, page->index);
 
-	if (!page->mapping || !page->mapping->host) {
-		dout("writepage %p - no mapping\n", page);
-		return -EFAULT;
-	}
 	inode = page->mapping->host;
 	ci = ceph_inode(inode);
 	fsc = ceph_inode_to_client(inode);
-- 
2.9.4

[PATCH 2/3] ceph: redirty page when writepage_nounlock() skips unwritable page

[Yan, Zheng]

Ceph needs to flush dirty page in the order in which in which snap
context they belong to. Dirty pages belong to older snap context
should be flushed earlier. writepage_nounlock() can not flush a
page, it should redirty the page.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: "Yan, Zheng" <zyan@redhat.com>
---
 fs/ceph/addr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index ff25239..1880125 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -551,8 +551,9 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
 		dout("writepage %p page %p snapc %p not writeable - noop\n",
 		     inode, page, snapc);
 		/* we should only noop if called by kswapd */
-		WARN_ON((current->flags & PF_MEMALLOC) == 0);
+		WARN_ON(!(current->flags & PF_MEMALLOC));
 		ceph_put_snap_context(oldest);
+		redirty_page_for_writepage(wbc, page);
 		goto out;
 	}
 	ceph_put_snap_context(oldest);
-- 
2.9.4

Re: [PATCH 2/3] ceph: redirty page when writepage_nounlock() skips unwritable page

[Jeff Layton]

On Tue, 2017-05-23 at 17:54 +0800, Yan, Zheng wrote:
> Ceph needs to flush dirty page in the order in which in which snap
> context they belong to. Dirty pages belong to older snap context
> should be flushed earlier. writepage_nounlock() can not flush a
> page, it should redirty the page.
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: "Yan, Zheng" <zyan@redhat.com>
> ---
>  fs/ceph/addr.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index ff25239..1880125 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -551,8 +551,9 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>  		dout("writepage %p page %p snapc %p not writeable - noop\n",
>  		     inode, page, snapc);
>  		/* we should only noop if called by kswapd */
> -		WARN_ON((current->flags & PF_MEMALLOC) == 0);
> +		WARN_ON(!(current->flags & PF_MEMALLOC));
>  		ceph_put_snap_context(oldest);
> +		redirty_page_for_writepage(wbc, page);
>  		goto out;
>  	}
>  	ceph_put_snap_context(oldest);

Reviewed-by: Jeff Layton <jlayton@redhat.com>

[PATCH 3/3] ceph: cleanup writepage_nounlock()

[Yan, Zheng]

Signed-off-by: "Yan, Zheng" <zyan@redhat.com>
---
 fs/ceph/addr.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index 1880125..777acee 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -530,7 +530,7 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
 	long writeback_stat;
 	u64 truncate_size;
 	u32 truncate_seq;
-	int err = 0, len = PAGE_SIZE;
+	int err, len = PAGE_SIZE;
 
 	dout("writepage %p idx %lu\n", page, page->index);
 
@@ -543,7 +543,7 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
 	snapc = page_snap_context(page);
 	if (snapc == NULL) {
 		dout("writepage %p page %p not dirty?\n", inode, page);
-		goto out;
+		return 0;
 	}
 	oldest = get_oldest_context(inode, &snap_size,
 				    &truncate_size, &truncate_seq);
@@ -554,7 +554,7 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
 		WARN_ON(!(current->flags & PF_MEMALLOC));
 		ceph_put_snap_context(oldest);
 		redirty_page_for_writepage(wbc, page);
-		goto out;
+		return 0;
 	}
 	ceph_put_snap_context(oldest);
 
@@ -564,8 +564,9 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
 	/* is this a partial page at end of file? */
 	if (page_off >= snap_size) {
 		dout("%p page eof %llu\n", page, snap_size);
-		goto out;
+		return 0;
 	}
+
 	if (snap_size < page_off + len)
 		len = snap_size - page_off;
 
@@ -592,7 +593,7 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
 			dout("writepage interrupted page %p\n", page);
 			redirty_page_for_writepage(wbc, page);
 			end_page_writeback(page);
-			goto out;
+			return err;
 		}
 		dout("writepage setting page/mapping error %d %p\n",
 		     err, page);
@@ -608,7 +609,6 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
 	end_page_writeback(page);
 	ceph_put_wrbuffer_cap_refs(ci, 1, snapc);
 	ceph_put_snap_context(snapc);  /* page's reference */
-out:
 	return err;
 }
 
-- 
2.9.4

Re: [PATCH 3/3] ceph: cleanup writepage_nounlock()

[Jeff Layton]

On Tue, 2017-05-23 at 17:54 +0800, Yan, Zheng wrote:
> Signed-off-by: "Yan, Zheng" <zyan@redhat.com>
> ---
>  fs/ceph/addr.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index 1880125..777acee 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -530,7 +530,7 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>  	long writeback_stat;
>  	u64 truncate_size;
>  	u32 truncate_seq;
> -	int err = 0, len = PAGE_SIZE;
> +	int err, len = PAGE_SIZE;
>  
>  	dout("writepage %p idx %lu\n", page, page->index);
>  
> @@ -543,7 +543,7 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>  	snapc = page_snap_context(page);
>  	if (snapc == NULL) {
>  		dout("writepage %p page %p not dirty?\n", inode, page);
> -		goto out;
> +		return 0;
>  	}
>  	oldest = get_oldest_context(inode, &snap_size,
>  				    &truncate_size, &truncate_seq);
> @@ -554,7 +554,7 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>  		WARN_ON(!(current->flags & PF_MEMALLOC));
>  		ceph_put_snap_context(oldest);
>  		redirty_page_for_writepage(wbc, page);
> -		goto out;
> +		return 0;
>  	}
>  	ceph_put_snap_context(oldest);
>  
> @@ -564,8 +564,9 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>  	/* is this a partial page at end of file? */
>  	if (page_off >= snap_size) {
>  		dout("%p page eof %llu\n", page, snap_size);
> -		goto out;
> +		return 0;
>  	}
> +
>  	if (snap_size < page_off + len)
>  		len = snap_size - page_off;
>  
> @@ -592,7 +593,7 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>  			dout("writepage interrupted page %p\n", page);
>  			redirty_page_for_writepage(wbc, page);
>  			end_page_writeback(page);
> -			goto out;
> +			return err;
>  		}
>  		dout("writepage setting page/mapping error %d %p\n",
>  		     err, page);
> @@ -608,7 +609,6 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>  	end_page_writeback(page);
>  	ceph_put_wrbuffer_cap_refs(ci, 1, snapc);
>  	ceph_put_snap_context(snapc);  /* page's reference */
> -out:
>  	return err;
>  }
>  

Looks correct:

Reviewed-by: Jeff Layton <jlayton@redhat.com>

Re: [PATCH 1/3] ceph: remove useless page->mapping check in writepage_nounlock()

[Jeff Layton]

On Tue, 2017-05-23 at 17:54 +0800, Yan, Zheng wrote:
> Callers of writepage_nounlock() have already ensured non-null
> page->mapping.
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: "Yan, Zheng" <zyan@redhat.com>
> ---
>  fs/ceph/addr.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index f49d6630..ff25239 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -534,10 +534,6 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>  
>  	dout("writepage %p idx %lu\n", page, page->index);
>  
> -	if (!page->mapping || !page->mapping->host) {
> -		dout("writepage %p - no mapping\n", page);
> -		return -EFAULT;
> -	}
>  	inode = page->mapping->host;
>  	ci = ceph_inode(inode);
>  	fsc = ceph_inode_to_client(inode);

Reviewed-by: Jeff Layton <jlayton@redhat.com>