RFC: remove cifs_writepage

RFC: remove cifs_writepage

[Christoph Hellwig]

Hi Steve,

this series tries to remove the ->writepage method from cifs, as there
is no good reason for the method to exist anymore and we're trying to
remove it entirely.

The series is entirely untested as I don't have a CIFS setup at the
moment, and patch 2 is a bit crude and there might be much better
ways to handle the small wsize case.

[PATCH 1/3] cifs: wire up >migrate_folio

[Christoph Hellwig]

CIFS does not use page private data that needs migration, so it can just
wire up filemap_migrate_folio.  This prepares for removing ->writepage,
which is used as a fallback if no migrate_folio method is set.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/cifs/file.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index cd96982099309..6be924caed393 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -5240,10 +5240,10 @@ const struct address_space_operations cifs_addr_ops = {
 	.direct_IO = cifs_direct_io,
 	.invalidate_folio = cifs_invalidate_folio,
 	.launder_folio = cifs_launder_folio,
+	.migrate_folio = filemap_migrate_folio,
 	/*
-	 * TODO: investigate and if useful we could add an cifs_migratePage
-	 * helper (under an CONFIG_MIGRATION) in the future, and also
-	 * investigate and add an is_dirty_writeback helper if needed
+	 * TODO: investigate and if useful we could add an is_dirty_writeback
+	 * helper if needed
 	 */
 	.swap_activate = cifs_swap_activate,
 	.swap_deactivate = cifs_swap_deactivate,
@@ -5264,4 +5264,5 @@ const struct address_space_operations cifs_addr_ops_smallbuf = {
 	.release_folio = cifs_release_folio,
 	.invalidate_folio = cifs_invalidate_folio,
 	.launder_folio = cifs_launder_folio,
+	.migrate_folio = filemap_migrate_folio,
 };
-- 
2.30.2

[PATCH 2/3] cifs: stop using generic_writepages

[Christoph Hellwig]

generic_writepages is just a wrapper that calls ->writepages on a range,
and thus in the way of eventually removing ->writepage.  Switch cifs
to just open code it in preparation of removing ->writepage.

[note: I suspect just integrating the small wsize case with the rest
 of the writeback code might be a better idea here, but that needs
 someone more familiar with the code]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/cifs/file.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 6be924caed393..ec14e38411a13 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -2646,6 +2646,21 @@ wdata_send_pages(struct cifs_writedata *wdata, unsigned int nr_pages,
 	return rc;
 }
 
+static int
+cifs_writepage_locked(struct page *page, struct writeback_control *wbc);
+
+static int cifs_write_one_page(struct page *page, struct writeback_control *wbc,
+		void *data)
+{
+	struct address_space *mapping = data;
+	int ret;
+
+	ret = cifs_writepage_locked(page, wbc);
+	unlock_page(page);
+	mapping_set_error(mapping, ret);
+	return ret;
+}
+
 static int cifs_writepages(struct address_space *mapping,
 			   struct writeback_control *wbc)
 {
@@ -2662,10 +2677,11 @@ static int cifs_writepages(struct address_space *mapping,
 
 	/*
 	 * If wsize is smaller than the page cache size, default to writing
-	 * one page at a time via cifs_writepage
+	 * one page at a time.
 	 */
 	if (cifs_sb->ctx->wsize < PAGE_SIZE)
-		return generic_writepages(mapping, wbc);
+		return write_cache_pages(mapping, wbc, cifs_write_one_page,
+				mapping);
 
 	xid = get_xid();
 	if (wbc->range_cyclic) {
-- 
2.30.2

[PATCH 3/3] cifs: remove ->writepage

[Christoph Hellwig]

->writepage is a very inefficient method to write back data, and only
used through write_cache_pages or a a fallback when no ->migrate_folio
method is present.  Now that cifs implements ->migrate_folio and
doesn't call generic_writepages, the writepage method can be removed.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/cifs/file.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index ec14e38411a13..6701257541ab2 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -2868,13 +2868,6 @@ cifs_writepage_locked(struct page *page, struct writeback_control *wbc)
 	return rc;
 }
 
-static int cifs_writepage(struct page *page, struct writeback_control *wbc)
-{
-	int rc = cifs_writepage_locked(page, wbc);
-	unlock_page(page);
-	return rc;
-}
-
 static int cifs_write_end(struct file *file, struct address_space *mapping,
 			loff_t pos, unsigned len, unsigned copied,
 			struct page *page, void *fsdata)
@@ -5247,7 +5240,6 @@ static bool cifs_dirty_folio(struct address_space *mapping, struct folio *folio)
 const struct address_space_operations cifs_addr_ops = {
 	.read_folio = cifs_read_folio,
 	.readahead = cifs_readahead,
-	.writepage = cifs_writepage,
 	.writepages = cifs_writepages,
 	.write_begin = cifs_write_begin,
 	.write_end = cifs_write_end,
@@ -5272,7 +5264,6 @@ const struct address_space_operations cifs_addr_ops = {
  */
 const struct address_space_operations cifs_addr_ops_smallbuf = {
 	.read_folio = cifs_read_folio,
-	.writepage = cifs_writepage,
 	.writepages = cifs_writepages,
 	.write_begin = cifs_write_begin,
 	.write_end = cifs_write_end,
-- 
2.30.2

Re: RFC: remove cifs_writepage

[Steve French]

I can run some tests on this later this week.

On Wed, Nov 16, 2022 at 7:29 AM Christoph Hellwig <hch@lst.de> wrote:
>
> Hi Steve,
>
> this series tries to remove the ->writepage method from cifs, as there
> is no good reason for the method to exist anymore and we're trying to
> remove it entirely.
>
> The series is entirely untested as I don't have a CIFS setup at the
> moment, and patch 2 is a bit crude and there might be much better
> ways to handle the small wsize case.



-- 
Thanks,

Steve

Re: RFC: remove cifs_writepage

[Christoph Hellwig]

On Wed, Nov 16, 2022 at 12:29:41PM -0600, Steve French wrote:
> I can run some tests on this later this week.

Did you get a chance to do that?

Re: RFC: remove cifs_writepage

[Steve French]

Sorry about the delay - was out of town.

Ran some tests today with the three patch (remove writepage for
cifs.ko) series.  Let me know if any updates for those.  Also let me
know which gti branch you would like to see these merged from (mine or
yours e.g.)

I did see an intermittent  failure (when run with these three patches)
that doesn't appear to be obviously related to yours but am still
investigating it.   Test 043 failed once, and on one of retries of the
group  -  test 045 failed once. See example below.  This looks related
to an issue with deferred close (handle leases) and reference counts
holding up unmount, and not related to these patches (at least at
first glance).   Continuing to debug

generic/043 49s ... [failed, exit status 1]- output mismatch (see
/home/smfrench/xfstests-dev/results//generic/043.out.bad)
    --- tests/generic/043.out 2020-01-24 17:11:18.676861985 -0600
    +++ /home/smfrench/xfstests-dev/results//generic/043.out.bad
2022-12-05 18:11:44.744440542 -0600
    @@ -1 +1,6 @@
     QA output created by 043
    +umount: /mnt-local-xfstest/scratch: target is busy.
    +mount error(16): Device or resource busy


On Sun, Dec 4, 2022 at 2:19 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Wed, Nov 16, 2022 at 12:29:41PM -0600, Steve French wrote:
> > I can run some tests on this later this week.
>
> Did you get a chance to do that?



-- 
Thanks,

Steve

Re: RFC: remove cifs_writepage

[Christoph Hellwig]

On Mon, Dec 05, 2022 at 06:22:12PM -0600, Steve French wrote:
> Ran some tests today with the three patch (remove writepage for
> cifs.ko) series.  Let me know if any updates for those.  Also let me
> know which gti branch you would like to see these merged from (mine or
> yours e.g.)

If they work and look fine to you please queue them up in the cifs
tree as-is.

> I did see an intermittent  failure (when run with these three patches)
> that doesn't appear to be obviously related to yours but am still
> investigating it.   Test 043 failed once, and on one of retries of the
> group  -  test 045 failed once. See example below.  This looks related
> to an issue with deferred close (handle leases) and reference counts
> holding up unmount, and not related to these patches (at least at
> first glance).   Continuing to debug

Yes.  I have no good idea but to may suggest what NFS does about
silly renamed inodes to ensure they don't try to get in the way
of mount.