[PATCH] ceph: don't implement writepage

[PATCH] ceph: don't implement writepage

[Jeff Layton]

Remove ceph_writepage as it's not strictly required these days.

To quote from commit 21b4ee7029c9 (xfs: drop ->writepage completely):

    ->writepage is only used in one place - single page writeback from
    memory reclaim. We only allow such writeback from kswapd, not from
    direct memory reclaim, and so it is rarely used. When it comes from
    kswapd, it is effectively random dirty page shoot-down, which is
    horrible for IO patterns. We will already have background writeback
    trying to clean all the dirty pages in memory as efficiently as
    possible, so having kswapd interrupt our well formed IO stream only
    slows things down. So get rid of xfs_vm_writepage() completely.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/addr.c | 25 -------------------------
 1 file changed, 25 deletions(-)

diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index 40830cb9b599..3489444c55b9 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -680,30 +680,6 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
 	return err;
 }
 
-static int ceph_writepage(struct page *page, struct writeback_control *wbc)
-{
-	int err;
-	struct inode *inode = page->mapping->host;
-	BUG_ON(!inode);
-	ihold(inode);
-
-	if (wbc->sync_mode == WB_SYNC_NONE &&
-	    ceph_inode_to_client(inode)->write_congested)
-		return AOP_WRITEPAGE_ACTIVATE;
-
-	wait_on_page_fscache(page);
-
-	err = writepage_nounlock(page, wbc);
-	if (err == -ERESTARTSYS) {
-		/* direct memory reclaimer was killed by SIGKILL. return 0
-		 * to prevent caller from setting mapping/page error */
-		err = 0;
-	}
-	unlock_page(page);
-	iput(inode);
-	return err;
-}
-
 /*
  * async writeback completion handler.
  *
@@ -1394,7 +1370,6 @@ static int ceph_write_end(struct file *file, struct address_space *mapping,
 const struct address_space_operations ceph_aops = {
 	.readpage = netfs_readpage,
 	.readahead = netfs_readahead,
-	.writepage = ceph_writepage,
 	.writepages = ceph_writepages_start,
 	.write_begin = ceph_write_begin,
 	.write_end = ceph_write_end,
-- 
2.36.1

Re: [PATCH] ceph: don't implement writepage

[Xiubo Li]

On 6/7/22 7:27 PM, Jeff Layton wrote:
> Remove ceph_writepage as it's not strictly required these days.
>
> To quote from commit 21b4ee7029c9 (xfs: drop ->writepage completely):
>
>      ->writepage is only used in one place - single page writeback from
>      memory reclaim. We only allow such writeback from kswapd, not from
>      direct memory reclaim, and so it is rarely used. When it comes from
>      kswapd, it is effectively random dirty page shoot-down, which is
>      horrible for IO patterns. We will already have background writeback
>      trying to clean all the dirty pages in memory as efficiently as
>      possible, so having kswapd interrupt our well formed IO stream only
>      slows things down. So get rid of xfs_vm_writepage() completely.
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>   fs/ceph/addr.c | 25 -------------------------
>   1 file changed, 25 deletions(-)
>
> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index 40830cb9b599..3489444c55b9 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -680,30 +680,6 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>   	return err;
>   }
>   
> -static int ceph_writepage(struct page *page, struct writeback_control *wbc)
> -{
> -	int err;
> -	struct inode *inode = page->mapping->host;
> -	BUG_ON(!inode);
> -	ihold(inode);
> -
> -	if (wbc->sync_mode == WB_SYNC_NONE &&
> -	    ceph_inode_to_client(inode)->write_congested)
> -		return AOP_WRITEPAGE_ACTIVATE;
> -
> -	wait_on_page_fscache(page);
> -
> -	err = writepage_nounlock(page, wbc);
> -	if (err == -ERESTARTSYS) {
> -		/* direct memory reclaimer was killed by SIGKILL. return 0
> -		 * to prevent caller from setting mapping/page error */
> -		err = 0;
> -	}
> -	unlock_page(page);
> -	iput(inode);
> -	return err;
> -}
> -
>   /*
>    * async writeback completion handler.
>    *
> @@ -1394,7 +1370,6 @@ static int ceph_write_end(struct file *file, struct address_space *mapping,
>   const struct address_space_operations ceph_aops = {
>   	.readpage = netfs_readpage,
>   	.readahead = netfs_readahead,
> -	.writepage = ceph_writepage,
>   	.writepages = ceph_writepages_start,
>   	.write_begin = ceph_write_begin,
>   	.write_end = ceph_write_end,

Sounds reasonable.

Will merge it into the testing branch.

Thanks Jeff!

-- Xiubo

Re: [PATCH] ceph: don't implement writepage

[Luís Henriques]

Jeff Layton <jlayton@kernel.org> writes:

> Remove ceph_writepage as it's not strictly required these days.
>
> To quote from commit 21b4ee7029c9 (xfs: drop ->writepage completely):
>
>     ->writepage is only used in one place - single page writeback from
>     memory reclaim. We only allow such writeback from kswapd, not from
>     direct memory reclaim, and so it is rarely used. When it comes from
>     kswapd, it is effectively random dirty page shoot-down, which is
>     horrible for IO patterns. We will already have background writeback
>     trying to clean all the dirty pages in memory as efficiently as
>     possible, so having kswapd interrupt our well formed IO stream only
>     slows things down. So get rid of xfs_vm_writepage() completely.
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ceph/addr.c | 25 -------------------------
>  1 file changed, 25 deletions(-)

The diffstat in particular looks great ;-)

Reviewed-by: Luís Henriques <lhenriques@suse.de>

Cheers
-- 
Luís


>
> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index 40830cb9b599..3489444c55b9 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -680,30 +680,6 @@ static int writepage_nounlock(struct page *page, struct writeback_control *wbc)
>  	return err;
>  }
>  
> -static int ceph_writepage(struct page *page, struct writeback_control *wbc)
> -{
> -	int err;
> -	struct inode *inode = page->mapping->host;
> -	BUG_ON(!inode);
> -	ihold(inode);
> -
> -	if (wbc->sync_mode == WB_SYNC_NONE &&
> -	    ceph_inode_to_client(inode)->write_congested)
> -		return AOP_WRITEPAGE_ACTIVATE;
> -
> -	wait_on_page_fscache(page);
> -
> -	err = writepage_nounlock(page, wbc);
> -	if (err == -ERESTARTSYS) {
> -		/* direct memory reclaimer was killed by SIGKILL. return 0
> -		 * to prevent caller from setting mapping/page error */
> -		err = 0;
> -	}
> -	unlock_page(page);
> -	iput(inode);
> -	return err;
> -}
> -
>  /*
>   * async writeback completion handler.
>   *
> @@ -1394,7 +1370,6 @@ static int ceph_write_end(struct file *file, struct address_space *mapping,
>  const struct address_space_operations ceph_aops = {
>  	.readpage = netfs_readpage,
>  	.readahead = netfs_readahead,
> -	.writepage = ceph_writepage,
>  	.writepages = ceph_writepages_start,
>  	.write_begin = ceph_write_begin,
>  	.write_end = ceph_write_end,
> -- 
>
> 2.36.1
>

Re: [PATCH] ceph: don't implement writepage

[Christoph Hellwig]

Do you have an urgent need for this?  I was actually planning on sending
a series to drop ->writepage entirely in the next weeks, and I'd pick
this patch up to avoid conflicts if possible.

Note that you also need to implement ->migratepage to not lose any
functionality if dropping ->writepage, and comeing up with a good
solution for that is what has been delaying the series.

Re: [PATCH] ceph: don't implement writepage

[Jeff Layton]

On Tue, 2022-06-07 at 23:37 -0700, Christoph Hellwig wrote:
> Do you have an urgent need for this?  I was actually planning on sending
> a series to drop ->writepage entirely in the next weeks, and I'd pick
> this patch up to avoid conflicts if possible.
> 


No, there's no urgent need for this. I was just following Willy's
recommendation from LSF.

> Note that you also need to implement ->migratepage to not lose any
> functionality if dropping ->writepage, and comeing up with a good
> solution for that is what has been delaying the series.

Oh, I didn't realize that! I'll plan to drop this from our series for
now. Let us know if you need us to carry any patches for this.

Thanks!
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] ceph: don't implement writepage

[Xiubo Li]

On 6/8/22 6:19 PM, Jeff Layton wrote:
> On Tue, 2022-06-07 at 23:37 -0700, Christoph Hellwig wrote:
>> Do you have an urgent need for this?  I was actually planning on sending
>> a series to drop ->writepage entirely in the next weeks, and I'd pick
>> this patch up to avoid conflicts if possible.
>>
>
> No, there's no urgent need for this. I was just following Willy's
> recommendation from LSF.
>
>> Note that you also need to implement ->migratepage to not lose any
>> functionality if dropping ->writepage, and comeing up with a good
>> solution for that is what has been delaying the series.
> Oh, I didn't realize that! I'll plan to drop this from our series for
> now. Let us know if you need us to carry any patches for this.
>
> Thanks!

Dropped it from the testing branch for now.

-- Xiubo