[PATCH 0/2] netfs,erofs: reuse netfs_put_subrequest() and

[PATCH 0/2] netfs,erofs: reuse netfs_put_subrequest() and

[Jingbo Xu]

The data routine of erofs in fscache mode also relies on
netfs_io_request and netfs_io_subrequest.  erofs itself maintains a
copy of some helpers on netfs_io_request and netfs_io_subrequest.

To clean up the code, export netfs_put_subrequest() and
netfs_rreq_completed().


Jingbo Xu (2):
  netfs: export helpers for request and subrequest
  erofs: use netfs helpers manipulating request and subrequest

 fs/erofs/fscache.c    | 47 +++++++++----------------------------------
 fs/netfs/io.c         |  3 ++-
 fs/netfs/objects.c    |  1 +
 include/linux/netfs.h |  2 ++
 4 files changed, 15 insertions(+), 38 deletions(-)

-- 
2.19.1.6.gb485710b

[PATCH 0/2] netfs,erofs: reuse netfs_put_subrequest() and

[Jingbo Xu]

The data routine of erofs in fscache mode also relies on
netfs_io_request and netfs_io_subrequest.  erofs itself maintains a
copy of some helpers on netfs_io_request and netfs_io_subrequest.

To clean up the code, export netfs_put_subrequest() and
netfs_rreq_completed().


Jingbo Xu (2):
  netfs: export helpers for request and subrequest
  erofs: use netfs helpers manipulating request and subrequest

 fs/erofs/fscache.c    | 47 +++++++++----------------------------------
 fs/netfs/io.c         |  3 ++-
 fs/netfs/objects.c    |  1 +
 include/linux/netfs.h |  2 ++
 4 files changed, 15 insertions(+), 38 deletions(-)

-- 
2.19.1.6.gb485710b

[PATCH 1/2] netfs: export helpers for request and subrequest

[Jingbo Xu]

Export netfs_put_subrequest() and netfs_rreq_completed().

Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
---
 fs/netfs/io.c         | 3 ++-
 fs/netfs/objects.c    | 1 +
 include/linux/netfs.h | 2 ++
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/netfs/io.c b/fs/netfs/io.c
index 428925899282..58dd56e3e780 100644
--- a/fs/netfs/io.c
+++ b/fs/netfs/io.c
@@ -94,12 +94,13 @@ static void netfs_read_from_server(struct netfs_io_request *rreq,
 /*
  * Release those waiting.
  */
-static void netfs_rreq_completed(struct netfs_io_request *rreq, bool was_async)
+void netfs_rreq_completed(struct netfs_io_request *rreq, bool was_async)
 {
 	trace_netfs_rreq(rreq, netfs_rreq_trace_done);
 	netfs_clear_subrequests(rreq, was_async);
 	netfs_put_request(rreq, was_async, netfs_rreq_trace_put_complete);
 }
+EXPORT_SYMBOL(netfs_rreq_completed);
 
 /*
  * Deal with the completion of writing the data to the cache.  We have to clear
diff --git a/fs/netfs/objects.c b/fs/netfs/objects.c
index e17cdf53f6a7..478cc1a1664c 100644
--- a/fs/netfs/objects.c
+++ b/fs/netfs/objects.c
@@ -158,3 +158,4 @@ void netfs_put_subrequest(struct netfs_io_subrequest *subreq, bool was_async,
 	if (dead)
 		netfs_free_subrequest(subreq, was_async);
 }
+EXPORT_SYMBOL(netfs_put_subrequest);
diff --git a/include/linux/netfs.h b/include/linux/netfs.h
index f2402ddeafbf..d519fb709d7f 100644
--- a/include/linux/netfs.h
+++ b/include/linux/netfs.h
@@ -282,6 +282,8 @@ int netfs_write_begin(struct netfs_inode *, struct file *,
 		struct address_space *, loff_t pos, unsigned int len,
 		struct folio **, void **fsdata);
 
+void netfs_rreq_completed(struct netfs_io_request *rreq, bool was_async);
+
 void netfs_subreq_terminated(struct netfs_io_subrequest *, ssize_t, bool);
 void netfs_get_subrequest(struct netfs_io_subrequest *subreq,
 			  enum netfs_sreq_ref_trace what);
-- 
2.19.1.6.gb485710b

[PATCH 1/2] netfs: export helpers for request and subrequest

[Jingbo Xu]

Export netfs_put_subrequest() and netfs_rreq_completed().

Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
---
 fs/netfs/io.c         | 3 ++-
 fs/netfs/objects.c    | 1 +
 include/linux/netfs.h | 2 ++
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/netfs/io.c b/fs/netfs/io.c
index 428925899282..58dd56e3e780 100644
--- a/fs/netfs/io.c
+++ b/fs/netfs/io.c
@@ -94,12 +94,13 @@ static void netfs_read_from_server(struct netfs_io_request *rreq,
 /*
  * Release those waiting.
  */
-static void netfs_rreq_completed(struct netfs_io_request *rreq, bool was_async)
+void netfs_rreq_completed(struct netfs_io_request *rreq, bool was_async)
 {
 	trace_netfs_rreq(rreq, netfs_rreq_trace_done);
 	netfs_clear_subrequests(rreq, was_async);
 	netfs_put_request(rreq, was_async, netfs_rreq_trace_put_complete);
 }
+EXPORT_SYMBOL(netfs_rreq_completed);
 
 /*
  * Deal with the completion of writing the data to the cache.  We have to clear
diff --git a/fs/netfs/objects.c b/fs/netfs/objects.c
index e17cdf53f6a7..478cc1a1664c 100644
--- a/fs/netfs/objects.c
+++ b/fs/netfs/objects.c
@@ -158,3 +158,4 @@ void netfs_put_subrequest(struct netfs_io_subrequest *subreq, bool was_async,
 	if (dead)
 		netfs_free_subrequest(subreq, was_async);
 }
+EXPORT_SYMBOL(netfs_put_subrequest);
diff --git a/include/linux/netfs.h b/include/linux/netfs.h
index f2402ddeafbf..d519fb709d7f 100644
--- a/include/linux/netfs.h
+++ b/include/linux/netfs.h
@@ -282,6 +282,8 @@ int netfs_write_begin(struct netfs_inode *, struct file *,
 		struct address_space *, loff_t pos, unsigned int len,
 		struct folio **, void **fsdata);
 
+void netfs_rreq_completed(struct netfs_io_request *rreq, bool was_async);
+
 void netfs_subreq_terminated(struct netfs_io_subrequest *, ssize_t, bool);
 void netfs_get_subrequest(struct netfs_io_subrequest *subreq,
 			  enum netfs_sreq_ref_trace what);
-- 
2.19.1.6.gb485710b

[PATCH 2/2] erofs: use netfs helpers manipulating request and subrequest

[Jingbo Xu]

Use netfs_put_subrequest() and netfs_rreq_completed() completing request
and subrequest.

It is worth noting that a noop netfs_request_ops is introduced for erofs
since some netfs routine, e.g. netfs_free_request(), will call into
this ops.

Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
---
 fs/erofs/fscache.c | 47 ++++++++++------------------------------------
 1 file changed, 10 insertions(+), 37 deletions(-)

diff --git a/fs/erofs/fscache.c b/fs/erofs/fscache.c
index fe05bc51f9f2..fa3f4ab5e3b6 100644
--- a/fs/erofs/fscache.c
+++ b/fs/erofs/fscache.c
@@ -4,6 +4,7 @@
  * Copyright (C) 2022, Bytedance Inc. All rights reserved.
  */
 #include <linux/fscache.h>
+#include <trace/events/netfs.h>
 #include "internal.h"
 
 static DEFINE_MUTEX(erofs_domain_list_lock);
@@ -11,6 +12,8 @@ static DEFINE_MUTEX(erofs_domain_cookies_lock);
 static LIST_HEAD(erofs_domain_list);
 static struct vfsmount *erofs_pseudo_mnt;
 
+static const struct netfs_request_ops erofs_noop_req_ops;
+
 static struct netfs_io_request *erofs_fscache_alloc_request(struct address_space *mapping,
 					     loff_t start, size_t len)
 {
@@ -24,40 +27,12 @@ static struct netfs_io_request *erofs_fscache_alloc_request(struct address_space
 	rreq->len	= len;
 	rreq->mapping	= mapping;
 	rreq->inode	= mapping->host;
+	rreq->netfs_ops	= &erofs_noop_req_ops;
 	INIT_LIST_HEAD(&rreq->subrequests);
 	refcount_set(&rreq->ref, 1);
 	return rreq;
 }
 
-static void erofs_fscache_put_request(struct netfs_io_request *rreq)
-{
-	if (!refcount_dec_and_test(&rreq->ref))
-		return;
-	if (rreq->cache_resources.ops)
-		rreq->cache_resources.ops->end_operation(&rreq->cache_resources);
-	kfree(rreq);
-}
-
-static void erofs_fscache_put_subrequest(struct netfs_io_subrequest *subreq)
-{
-	if (!refcount_dec_and_test(&subreq->ref))
-		return;
-	erofs_fscache_put_request(subreq->rreq);
-	kfree(subreq);
-}
-
-static void erofs_fscache_clear_subrequests(struct netfs_io_request *rreq)
-{
-	struct netfs_io_subrequest *subreq;
-
-	while (!list_empty(&rreq->subrequests)) {
-		subreq = list_first_entry(&rreq->subrequests,
-				struct netfs_io_subrequest, rreq_link);
-		list_del(&subreq->rreq_link);
-		erofs_fscache_put_subrequest(subreq);
-	}
-}
-
 static void erofs_fscache_rreq_unlock_folios(struct netfs_io_request *rreq)
 {
 	struct netfs_io_subrequest *subreq;
@@ -114,11 +89,10 @@ static void erofs_fscache_rreq_unlock_folios(struct netfs_io_request *rreq)
 static void erofs_fscache_rreq_complete(struct netfs_io_request *rreq)
 {
 	erofs_fscache_rreq_unlock_folios(rreq);
-	erofs_fscache_clear_subrequests(rreq);
-	erofs_fscache_put_request(rreq);
+	netfs_rreq_completed(rreq, false);
 }
 
-static void erofc_fscache_subreq_complete(void *priv,
+static void erofs_fscache_subreq_complete(void *priv,
 		ssize_t transferred_or_error, bool was_async)
 {
 	struct netfs_io_subrequest *subreq = priv;
@@ -130,7 +104,7 @@ static void erofc_fscache_subreq_complete(void *priv,
 	if (atomic_dec_and_test(&rreq->nr_outstanding))
 		erofs_fscache_rreq_complete(rreq);
 
-	erofs_fscache_put_subrequest(subreq);
+	netfs_put_subrequest(subreq, false, netfs_sreq_trace_put_terminated);
 }
 
 /*
@@ -171,9 +145,8 @@ static int erofs_fscache_read_folios_async(struct fscache_cookie *cookie,
 		}
 
 		subreq->start = pstart + done;
-		subreq->len	=  len - done;
+		subreq->len   =  len - done;
 		subreq->flags = 1 << NETFS_SREQ_ONDEMAND;
-
 		list_add_tail(&subreq->rreq_link, &rreq->subrequests);
 
 		source = cres->ops->prepare_read(subreq, LLONG_MAX);
@@ -184,7 +157,7 @@ static int erofs_fscache_read_folios_async(struct fscache_cookie *cookie,
 				  source);
 			ret = -EIO;
 			subreq->error = ret;
-			erofs_fscache_put_subrequest(subreq);
+			netfs_put_subrequest(subreq, false, netfs_sreq_trace_put_failed);
 			goto out;
 		}
 
@@ -195,7 +168,7 @@ static int erofs_fscache_read_folios_async(struct fscache_cookie *cookie,
 
 		ret = fscache_read(cres, subreq->start, &iter,
 				   NETFS_READ_HOLE_FAIL,
-				   erofc_fscache_subreq_complete, subreq);
+				   erofs_fscache_subreq_complete, subreq);
 		if (ret == -EIOCBQUEUED)
 			ret = 0;
 		if (ret) {
-- 
2.19.1.6.gb485710b

[PATCH 2/2] erofs: use netfs helpers manipulating request and subrequest

[Jingbo Xu]

Use netfs_put_subrequest() and netfs_rreq_completed() completing request
and subrequest.

It is worth noting that a noop netfs_request_ops is introduced for erofs
since some netfs routine, e.g. netfs_free_request(), will call into
this ops.

Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
---
 fs/erofs/fscache.c | 47 ++++++++++------------------------------------
 1 file changed, 10 insertions(+), 37 deletions(-)

diff --git a/fs/erofs/fscache.c b/fs/erofs/fscache.c
index fe05bc51f9f2..fa3f4ab5e3b6 100644
--- a/fs/erofs/fscache.c
+++ b/fs/erofs/fscache.c
@@ -4,6 +4,7 @@
  * Copyright (C) 2022, Bytedance Inc. All rights reserved.
  */
 #include <linux/fscache.h>
+#include <trace/events/netfs.h>
 #include "internal.h"
 
 static DEFINE_MUTEX(erofs_domain_list_lock);
@@ -11,6 +12,8 @@ static DEFINE_MUTEX(erofs_domain_cookies_lock);
 static LIST_HEAD(erofs_domain_list);
 static struct vfsmount *erofs_pseudo_mnt;
 
+static const struct netfs_request_ops erofs_noop_req_ops;
+
 static struct netfs_io_request *erofs_fscache_alloc_request(struct address_space *mapping,
 					     loff_t start, size_t len)
 {
@@ -24,40 +27,12 @@ static struct netfs_io_request *erofs_fscache_alloc_request(struct address_space
 	rreq->len	= len;
 	rreq->mapping	= mapping;
 	rreq->inode	= mapping->host;
+	rreq->netfs_ops	= &erofs_noop_req_ops;
 	INIT_LIST_HEAD(&rreq->subrequests);
 	refcount_set(&rreq->ref, 1);
 	return rreq;
 }
 
-static void erofs_fscache_put_request(struct netfs_io_request *rreq)
-{
-	if (!refcount_dec_and_test(&rreq->ref))
-		return;
-	if (rreq->cache_resources.ops)
-		rreq->cache_resources.ops->end_operation(&rreq->cache_resources);
-	kfree(rreq);
-}
-
-static void erofs_fscache_put_subrequest(struct netfs_io_subrequest *subreq)
-{
-	if (!refcount_dec_and_test(&subreq->ref))
-		return;
-	erofs_fscache_put_request(subreq->rreq);
-	kfree(subreq);
-}
-
-static void erofs_fscache_clear_subrequests(struct netfs_io_request *rreq)
-{
-	struct netfs_io_subrequest *subreq;
-
-	while (!list_empty(&rreq->subrequests)) {
-		subreq = list_first_entry(&rreq->subrequests,
-				struct netfs_io_subrequest, rreq_link);
-		list_del(&subreq->rreq_link);
-		erofs_fscache_put_subrequest(subreq);
-	}
-}
-
 static void erofs_fscache_rreq_unlock_folios(struct netfs_io_request *rreq)
 {
 	struct netfs_io_subrequest *subreq;
@@ -114,11 +89,10 @@ static void erofs_fscache_rreq_unlock_folios(struct netfs_io_request *rreq)
 static void erofs_fscache_rreq_complete(struct netfs_io_request *rreq)
 {
 	erofs_fscache_rreq_unlock_folios(rreq);
-	erofs_fscache_clear_subrequests(rreq);
-	erofs_fscache_put_request(rreq);
+	netfs_rreq_completed(rreq, false);
 }
 
-static void erofc_fscache_subreq_complete(void *priv,
+static void erofs_fscache_subreq_complete(void *priv,
 		ssize_t transferred_or_error, bool was_async)
 {
 	struct netfs_io_subrequest *subreq = priv;
@@ -130,7 +104,7 @@ static void erofc_fscache_subreq_complete(void *priv,
 	if (atomic_dec_and_test(&rreq->nr_outstanding))
 		erofs_fscache_rreq_complete(rreq);
 
-	erofs_fscache_put_subrequest(subreq);
+	netfs_put_subrequest(subreq, false, netfs_sreq_trace_put_terminated);
 }
 
 /*
@@ -171,9 +145,8 @@ static int erofs_fscache_read_folios_async(struct fscache_cookie *cookie,
 		}
 
 		subreq->start = pstart + done;
-		subreq->len	=  len - done;
+		subreq->len   =  len - done;
 		subreq->flags = 1 << NETFS_SREQ_ONDEMAND;
-
 		list_add_tail(&subreq->rreq_link, &rreq->subrequests);
 
 		source = cres->ops->prepare_read(subreq, LLONG_MAX);
@@ -184,7 +157,7 @@ static int erofs_fscache_read_folios_async(struct fscache_cookie *cookie,
 				  source);
 			ret = -EIO;
 			subreq->error = ret;
-			erofs_fscache_put_subrequest(subreq);
+			netfs_put_subrequest(subreq, false, netfs_sreq_trace_put_failed);
 			goto out;
 		}
 
@@ -195,7 +168,7 @@ static int erofs_fscache_read_folios_async(struct fscache_cookie *cookie,
 
 		ret = fscache_read(cres, subreq->start, &iter,
 				   NETFS_READ_HOLE_FAIL,
-				   erofc_fscache_subreq_complete, subreq);
+				   erofs_fscache_subreq_complete, subreq);
 		if (ret == -EIOCBQUEUED)
 			ret = 0;
 		if (ret) {
-- 
2.19.1.6.gb485710b

Re: [PATCH 1/2] netfs: export helpers for request and subrequest

[Gao Xiang]

On Fri, Oct 21, 2022 at 04:49:11PM +0800, Jingbo Xu wrote:
> Export netfs_put_subrequest() and netfs_rreq_completed().
> 
> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>

Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.com>

Thanks,
Gao Xiang

> ---
>  fs/netfs/io.c         | 3 ++-
>  fs/netfs/objects.c    | 1 +
>  include/linux/netfs.h | 2 ++
>  3 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/netfs/io.c b/fs/netfs/io.c
> index 428925899282..58dd56e3e780 100644
> --- a/fs/netfs/io.c
> +++ b/fs/netfs/io.c
> @@ -94,12 +94,13 @@ static void netfs_read_from_server(struct netfs_io_request *rreq,
>  /*
>   * Release those waiting.
>   */
> -static void netfs_rreq_completed(struct netfs_io_request *rreq, bool was_async)
> +void netfs_rreq_completed(struct netfs_io_request *rreq, bool was_async)
>  {
>  	trace_netfs_rreq(rreq, netfs_rreq_trace_done);
>  	netfs_clear_subrequests(rreq, was_async);
>  	netfs_put_request(rreq, was_async, netfs_rreq_trace_put_complete);
>  }
> +EXPORT_SYMBOL(netfs_rreq_completed);
>  
>  /*
>   * Deal with the completion of writing the data to the cache.  We have to clear
> diff --git a/fs/netfs/objects.c b/fs/netfs/objects.c
> index e17cdf53f6a7..478cc1a1664c 100644
> --- a/fs/netfs/objects.c
> +++ b/fs/netfs/objects.c
> @@ -158,3 +158,4 @@ void netfs_put_subrequest(struct netfs_io_subrequest *subreq, bool was_async,
>  	if (dead)
>  		netfs_free_subrequest(subreq, was_async);
>  }
> +EXPORT_SYMBOL(netfs_put_subrequest);
> diff --git a/include/linux/netfs.h b/include/linux/netfs.h
> index f2402ddeafbf..d519fb709d7f 100644
> --- a/include/linux/netfs.h
> +++ b/include/linux/netfs.h
> @@ -282,6 +282,8 @@ int netfs_write_begin(struct netfs_inode *, struct file *,
>  		struct address_space *, loff_t pos, unsigned int len,
>  		struct folio **, void **fsdata);
>  
> +void netfs_rreq_completed(struct netfs_io_request *rreq, bool was_async);
> +
>  void netfs_subreq_terminated(struct netfs_io_subrequest *, ssize_t, bool);
>  void netfs_get_subrequest(struct netfs_io_subrequest *subreq,
>  			  enum netfs_sreq_ref_trace what);
> -- 
> 2.19.1.6.gb485710b

Re: [PATCH 1/2] netfs: export helpers for request and subrequest

[Gao Xiang]

On Fri, Oct 21, 2022 at 04:49:11PM +0800, Jingbo Xu wrote:
> Export netfs_put_subrequest() and netfs_rreq_completed().
> 
> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>

Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.com>

Thanks,
Gao Xiang

> ---
>  fs/netfs/io.c         | 3 ++-
>  fs/netfs/objects.c    | 1 +
>  include/linux/netfs.h | 2 ++
>  3 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/netfs/io.c b/fs/netfs/io.c
> index 428925899282..58dd56e3e780 100644
> --- a/fs/netfs/io.c
> +++ b/fs/netfs/io.c
> @@ -94,12 +94,13 @@ static void netfs_read_from_server(struct netfs_io_request *rreq,
>  /*
>   * Release those waiting.
>   */
> -static void netfs_rreq_completed(struct netfs_io_request *rreq, bool was_async)
> +void netfs_rreq_completed(struct netfs_io_request *rreq, bool was_async)
>  {
>  	trace_netfs_rreq(rreq, netfs_rreq_trace_done);
>  	netfs_clear_subrequests(rreq, was_async);
>  	netfs_put_request(rreq, was_async, netfs_rreq_trace_put_complete);
>  }
> +EXPORT_SYMBOL(netfs_rreq_completed);
>  
>  /*
>   * Deal with the completion of writing the data to the cache.  We have to clear
> diff --git a/fs/netfs/objects.c b/fs/netfs/objects.c
> index e17cdf53f6a7..478cc1a1664c 100644
> --- a/fs/netfs/objects.c
> +++ b/fs/netfs/objects.c
> @@ -158,3 +158,4 @@ void netfs_put_subrequest(struct netfs_io_subrequest *subreq, bool was_async,
>  	if (dead)
>  		netfs_free_subrequest(subreq, was_async);
>  }
> +EXPORT_SYMBOL(netfs_put_subrequest);
> diff --git a/include/linux/netfs.h b/include/linux/netfs.h
> index f2402ddeafbf..d519fb709d7f 100644
> --- a/include/linux/netfs.h
> +++ b/include/linux/netfs.h
> @@ -282,6 +282,8 @@ int netfs_write_begin(struct netfs_inode *, struct file *,
>  		struct address_space *, loff_t pos, unsigned int len,
>  		struct folio **, void **fsdata);
>  
> +void netfs_rreq_completed(struct netfs_io_request *rreq, bool was_async);
> +
>  void netfs_subreq_terminated(struct netfs_io_subrequest *, ssize_t, bool);
>  void netfs_get_subrequest(struct netfs_io_subrequest *subreq,
>  			  enum netfs_sreq_ref_trace what);
> -- 
> 2.19.1.6.gb485710b

Re: [PATCH 2/2] erofs: use netfs helpers manipulating request and subrequest

[Gao Xiang]

On Fri, Oct 21, 2022 at 04:49:12PM +0800, Jingbo Xu wrote:
> Use netfs_put_subrequest() and netfs_rreq_completed() completing request
> and subrequest.
> 
> It is worth noting that a noop netfs_request_ops is introduced for erofs
> since some netfs routine, e.g. netfs_free_request(), will call into
> this ops.
> 
> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>

Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.com>

Thanks,
Gao Xiang

> ---
>  fs/erofs/fscache.c | 47 ++++++++++------------------------------------
>  1 file changed, 10 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/erofs/fscache.c b/fs/erofs/fscache.c
> index fe05bc51f9f2..fa3f4ab5e3b6 100644
> --- a/fs/erofs/fscache.c
> +++ b/fs/erofs/fscache.c
> @@ -4,6 +4,7 @@
>   * Copyright (C) 2022, Bytedance Inc. All rights reserved.
>   */
>  #include <linux/fscache.h>
> +#include <trace/events/netfs.h>
>  #include "internal.h"
>  
>  static DEFINE_MUTEX(erofs_domain_list_lock);
> @@ -11,6 +12,8 @@ static DEFINE_MUTEX(erofs_domain_cookies_lock);
>  static LIST_HEAD(erofs_domain_list);
>  static struct vfsmount *erofs_pseudo_mnt;
>  
> +static const struct netfs_request_ops erofs_noop_req_ops;
> +
>  static struct netfs_io_request *erofs_fscache_alloc_request(struct address_space *mapping,
>  					     loff_t start, size_t len)
>  {
> @@ -24,40 +27,12 @@ static struct netfs_io_request *erofs_fscache_alloc_request(struct address_space
>  	rreq->len	= len;
>  	rreq->mapping	= mapping;
>  	rreq->inode	= mapping->host;
> +	rreq->netfs_ops	= &erofs_noop_req_ops;
>  	INIT_LIST_HEAD(&rreq->subrequests);
>  	refcount_set(&rreq->ref, 1);
>  	return rreq;
>  }
>  
> -static void erofs_fscache_put_request(struct netfs_io_request *rreq)
> -{
> -	if (!refcount_dec_and_test(&rreq->ref))
> -		return;
> -	if (rreq->cache_resources.ops)
> -		rreq->cache_resources.ops->end_operation(&rreq->cache_resources);
> -	kfree(rreq);
> -}
> -
> -static void erofs_fscache_put_subrequest(struct netfs_io_subrequest *subreq)
> -{
> -	if (!refcount_dec_and_test(&subreq->ref))
> -		return;
> -	erofs_fscache_put_request(subreq->rreq);
> -	kfree(subreq);
> -}
> -
> -static void erofs_fscache_clear_subrequests(struct netfs_io_request *rreq)
> -{
> -	struct netfs_io_subrequest *subreq;
> -
> -	while (!list_empty(&rreq->subrequests)) {
> -		subreq = list_first_entry(&rreq->subrequests,
> -				struct netfs_io_subrequest, rreq_link);
> -		list_del(&subreq->rreq_link);
> -		erofs_fscache_put_subrequest(subreq);
> -	}
> -}
> -
>  static void erofs_fscache_rreq_unlock_folios(struct netfs_io_request *rreq)
>  {
>  	struct netfs_io_subrequest *subreq;
> @@ -114,11 +89,10 @@ static void erofs_fscache_rreq_unlock_folios(struct netfs_io_request *rreq)
>  static void erofs_fscache_rreq_complete(struct netfs_io_request *rreq)
>  {
>  	erofs_fscache_rreq_unlock_folios(rreq);
> -	erofs_fscache_clear_subrequests(rreq);
> -	erofs_fscache_put_request(rreq);
> +	netfs_rreq_completed(rreq, false);
>  }
>  
> -static void erofc_fscache_subreq_complete(void *priv,
> +static void erofs_fscache_subreq_complete(void *priv,
>  		ssize_t transferred_or_error, bool was_async)
>  {
>  	struct netfs_io_subrequest *subreq = priv;
> @@ -130,7 +104,7 @@ static void erofc_fscache_subreq_complete(void *priv,
>  	if (atomic_dec_and_test(&rreq->nr_outstanding))
>  		erofs_fscache_rreq_complete(rreq);
>  
> -	erofs_fscache_put_subrequest(subreq);
> +	netfs_put_subrequest(subreq, false, netfs_sreq_trace_put_terminated);
>  }
>  
>  /*
> @@ -171,9 +145,8 @@ static int erofs_fscache_read_folios_async(struct fscache_cookie *cookie,
>  		}
>  
>  		subreq->start = pstart + done;
> -		subreq->len	=  len - done;
> +		subreq->len   =  len - done;
>  		subreq->flags = 1 << NETFS_SREQ_ONDEMAND;
> -
>  		list_add_tail(&subreq->rreq_link, &rreq->subrequests);
>  
>  		source = cres->ops->prepare_read(subreq, LLONG_MAX);
> @@ -184,7 +157,7 @@ static int erofs_fscache_read_folios_async(struct fscache_cookie *cookie,
>  				  source);
>  			ret = -EIO;
>  			subreq->error = ret;
> -			erofs_fscache_put_subrequest(subreq);
> +			netfs_put_subrequest(subreq, false, netfs_sreq_trace_put_failed);
>  			goto out;
>  		}
>  
> @@ -195,7 +168,7 @@ static int erofs_fscache_read_folios_async(struct fscache_cookie *cookie,
>  
>  		ret = fscache_read(cres, subreq->start, &iter,
>  				   NETFS_READ_HOLE_FAIL,
> -				   erofc_fscache_subreq_complete, subreq);
> +				   erofs_fscache_subreq_complete, subreq);
>  		if (ret == -EIOCBQUEUED)
>  			ret = 0;
>  		if (ret) {
> -- 
> 2.19.1.6.gb485710b

Re: [PATCH 2/2] erofs: use netfs helpers manipulating request and subrequest

[Gao Xiang]

On Fri, Oct 21, 2022 at 04:49:12PM +0800, Jingbo Xu wrote:
> Use netfs_put_subrequest() and netfs_rreq_completed() completing request
> and subrequest.
> 
> It is worth noting that a noop netfs_request_ops is introduced for erofs
> since some netfs routine, e.g. netfs_free_request(), will call into
> this ops.
> 
> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>

Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.com>

Thanks,
Gao Xiang

> ---
>  fs/erofs/fscache.c | 47 ++++++++++------------------------------------
>  1 file changed, 10 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/erofs/fscache.c b/fs/erofs/fscache.c
> index fe05bc51f9f2..fa3f4ab5e3b6 100644
> --- a/fs/erofs/fscache.c
> +++ b/fs/erofs/fscache.c
> @@ -4,6 +4,7 @@
>   * Copyright (C) 2022, Bytedance Inc. All rights reserved.
>   */
>  #include <linux/fscache.h>
> +#include <trace/events/netfs.h>
>  #include "internal.h"
>  
>  static DEFINE_MUTEX(erofs_domain_list_lock);
> @@ -11,6 +12,8 @@ static DEFINE_MUTEX(erofs_domain_cookies_lock);
>  static LIST_HEAD(erofs_domain_list);
>  static struct vfsmount *erofs_pseudo_mnt;
>  
> +static const struct netfs_request_ops erofs_noop_req_ops;
> +
>  static struct netfs_io_request *erofs_fscache_alloc_request(struct address_space *mapping,
>  					     loff_t start, size_t len)
>  {
> @@ -24,40 +27,12 @@ static struct netfs_io_request *erofs_fscache_alloc_request(struct address_space
>  	rreq->len	= len;
>  	rreq->mapping	= mapping;
>  	rreq->inode	= mapping->host;
> +	rreq->netfs_ops	= &erofs_noop_req_ops;
>  	INIT_LIST_HEAD(&rreq->subrequests);
>  	refcount_set(&rreq->ref, 1);
>  	return rreq;
>  }
>  
> -static void erofs_fscache_put_request(struct netfs_io_request *rreq)
> -{
> -	if (!refcount_dec_and_test(&rreq->ref))
> -		return;
> -	if (rreq->cache_resources.ops)
> -		rreq->cache_resources.ops->end_operation(&rreq->cache_resources);
> -	kfree(rreq);
> -}
> -
> -static void erofs_fscache_put_subrequest(struct netfs_io_subrequest *subreq)
> -{
> -	if (!refcount_dec_and_test(&subreq->ref))
> -		return;
> -	erofs_fscache_put_request(subreq->rreq);
> -	kfree(subreq);
> -}
> -
> -static void erofs_fscache_clear_subrequests(struct netfs_io_request *rreq)
> -{
> -	struct netfs_io_subrequest *subreq;
> -
> -	while (!list_empty(&rreq->subrequests)) {
> -		subreq = list_first_entry(&rreq->subrequests,
> -				struct netfs_io_subrequest, rreq_link);
> -		list_del(&subreq->rreq_link);
> -		erofs_fscache_put_subrequest(subreq);
> -	}
> -}
> -
>  static void erofs_fscache_rreq_unlock_folios(struct netfs_io_request *rreq)
>  {
>  	struct netfs_io_subrequest *subreq;
> @@ -114,11 +89,10 @@ static void erofs_fscache_rreq_unlock_folios(struct netfs_io_request *rreq)
>  static void erofs_fscache_rreq_complete(struct netfs_io_request *rreq)
>  {
>  	erofs_fscache_rreq_unlock_folios(rreq);
> -	erofs_fscache_clear_subrequests(rreq);
> -	erofs_fscache_put_request(rreq);
> +	netfs_rreq_completed(rreq, false);
>  }
>  
> -static void erofc_fscache_subreq_complete(void *priv,
> +static void erofs_fscache_subreq_complete(void *priv,
>  		ssize_t transferred_or_error, bool was_async)
>  {
>  	struct netfs_io_subrequest *subreq = priv;
> @@ -130,7 +104,7 @@ static void erofc_fscache_subreq_complete(void *priv,
>  	if (atomic_dec_and_test(&rreq->nr_outstanding))
>  		erofs_fscache_rreq_complete(rreq);
>  
> -	erofs_fscache_put_subrequest(subreq);
> +	netfs_put_subrequest(subreq, false, netfs_sreq_trace_put_terminated);
>  }
>  
>  /*
> @@ -171,9 +145,8 @@ static int erofs_fscache_read_folios_async(struct fscache_cookie *cookie,
>  		}
>  
>  		subreq->start = pstart + done;
> -		subreq->len	=  len - done;
> +		subreq->len   =  len - done;
>  		subreq->flags = 1 << NETFS_SREQ_ONDEMAND;
> -
>  		list_add_tail(&subreq->rreq_link, &rreq->subrequests);
>  
>  		source = cres->ops->prepare_read(subreq, LLONG_MAX);
> @@ -184,7 +157,7 @@ static int erofs_fscache_read_folios_async(struct fscache_cookie *cookie,
>  				  source);
>  			ret = -EIO;
>  			subreq->error = ret;
> -			erofs_fscache_put_subrequest(subreq);
> +			netfs_put_subrequest(subreq, false, netfs_sreq_trace_put_failed);
>  			goto out;
>  		}
>  
> @@ -195,7 +168,7 @@ static int erofs_fscache_read_folios_async(struct fscache_cookie *cookie,
>  
>  		ret = fscache_read(cres, subreq->start, &iter,
>  				   NETFS_READ_HOLE_FAIL,
> -				   erofc_fscache_subreq_complete, subreq);
> +				   erofs_fscache_subreq_complete, subreq);
>  		if (ret == -EIOCBQUEUED)
>  			ret = 0;
>  		if (ret) {
> -- 
> 2.19.1.6.gb485710b

Re: [PATCH 2/2] erofs: use netfs helpers manipulating request and subrequest

[Jeff Layton]

On Fri, 2022-10-21 at 16:49 +0800, Jingbo Xu wrote:
> Use netfs_put_subrequest() and netfs_rreq_completed() completing request
> and subrequest.
> 
> It is worth noting that a noop netfs_request_ops is introduced for erofs
> since some netfs routine, e.g. netfs_free_request(), will call into
> this ops.
> 
> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
> ---
>  fs/erofs/fscache.c | 47 ++++++++++------------------------------------
>  1 file changed, 10 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/erofs/fscache.c b/fs/erofs/fscache.c
> index fe05bc51f9f2..fa3f4ab5e3b6 100644
> --- a/fs/erofs/fscache.c
> +++ b/fs/erofs/fscache.c
> @@ -4,6 +4,7 @@
>   * Copyright (C) 2022, Bytedance Inc. All rights reserved.
>   */
>  #include <linux/fscache.h>
> +#include <trace/events/netfs.h>
>  #include "internal.h"
>  
>  static DEFINE_MUTEX(erofs_domain_list_lock);
> @@ -11,6 +12,8 @@ static DEFINE_MUTEX(erofs_domain_cookies_lock);
>  static LIST_HEAD(erofs_domain_list);
>  static struct vfsmount *erofs_pseudo_mnt;
>  
> +static const struct netfs_request_ops erofs_noop_req_ops;
> +
>  static struct netfs_io_request *erofs_fscache_alloc_request(struct address_space *mapping,
>  					     loff_t start, size_t len)
>  {
> @@ -24,40 +27,12 @@ static struct netfs_io_request *erofs_fscache_alloc_request(struct address_space
>  	rreq->len	= len;
>  	rreq->mapping	= mapping;
>  	rreq->inode	= mapping->host;
> +	rreq->netfs_ops	= &erofs_noop_req_ops;
>  	INIT_LIST_HEAD(&rreq->subrequests);
>  	refcount_set(&rreq->ref, 1);
>  	return rreq;
>  }
>  

Why is erofs allocating its own netfs structures? This seems quite
fragile, and a layering violation too.

> -static void erofs_fscache_put_request(struct netfs_io_request *rreq)
> -{
> -	if (!refcount_dec_and_test(&rreq->ref))
> -		return;
> -	if (rreq->cache_resources.ops)
> -		rreq->cache_resources.ops->end_operation(&rreq->cache_resources);
> -	kfree(rreq);
> -}
> -
> -static void erofs_fscache_put_subrequest(struct netfs_io_subrequest *subreq)
> -{
> -	if (!refcount_dec_and_test(&subreq->ref))
> -		return;
> -	erofs_fscache_put_request(subreq->rreq);
> -	kfree(subreq);
> -}
> -
> -static void erofs_fscache_clear_subrequests(struct netfs_io_request *rreq)
> -{
> -	struct netfs_io_subrequest *subreq;
> -
> -	while (!list_empty(&rreq->subrequests)) {
> -		subreq = list_first_entry(&rreq->subrequests,
> -				struct netfs_io_subrequest, rreq_link);
> -		list_del(&subreq->rreq_link);
> -		erofs_fscache_put_subrequest(subreq);
> -	}
> -}
> -
>  static void erofs_fscache_rreq_unlock_folios(struct netfs_io_request *rreq)
>  {
>  	struct netfs_io_subrequest *subreq;
> @@ -114,11 +89,10 @@ static void erofs_fscache_rreq_unlock_folios(struct netfs_io_request *rreq)
>  static void erofs_fscache_rreq_complete(struct netfs_io_request *rreq)
>  {
>  	erofs_fscache_rreq_unlock_folios(rreq);
> -	erofs_fscache_clear_subrequests(rreq);
> -	erofs_fscache_put_request(rreq);
> +	netfs_rreq_completed(rreq, false);
>  }
>  
> -static void erofc_fscache_subreq_complete(void *priv,
> +static void erofs_fscache_subreq_complete(void *priv,
>  		ssize_t transferred_or_error, bool was_async)
>  {
>  	struct netfs_io_subrequest *subreq = priv;
> @@ -130,7 +104,7 @@ static void erofc_fscache_subreq_complete(void *priv,
>  	if (atomic_dec_and_test(&rreq->nr_outstanding))
>  		erofs_fscache_rreq_complete(rreq);
>  
> -	erofs_fscache_put_subrequest(subreq);
> +	netfs_put_subrequest(subreq, false, netfs_sreq_trace_put_terminated);
>  }
>  
>  /*
> @@ -171,9 +145,8 @@ static int erofs_fscache_read_folios_async(struct fscache_cookie *cookie,
>  		}
>  
>  		subreq->start = pstart + done;
> -		subreq->len	=  len - done;
> +		subreq->len   =  len - done;
>  		subreq->flags = 1 << NETFS_SREQ_ONDEMAND;
> -
>  		list_add_tail(&subreq->rreq_link, &rreq->subrequests);
>  
>  		source = cres->ops->prepare_read(subreq, LLONG_MAX);
> @@ -184,7 +157,7 @@ static int erofs_fscache_read_folios_async(struct fscache_cookie *cookie,
>  				  source);
>  			ret = -EIO;
>  			subreq->error = ret;
> -			erofs_fscache_put_subrequest(subreq);
> +			netfs_put_subrequest(subreq, false, netfs_sreq_trace_put_failed);
>  			goto out;
>  		}
>  
> @@ -195,7 +168,7 @@ static int erofs_fscache_read_folios_async(struct fscache_cookie *cookie,
>  
>  		ret = fscache_read(cres, subreq->start, &iter,
>  				   NETFS_READ_HOLE_FAIL,
> -				   erofc_fscache_subreq_complete, subreq);
> +				   erofs_fscache_subreq_complete, subreq);
>  		if (ret == -EIOCBQUEUED)
>  			ret = 0;
>  		if (ret) {

I'd rather see this done differently. Either change erofs to use the
netfs infrastructure in a more standard fashion, or maybe consider
teaching erofs to talk to cachefiles directly?

IDK, but this sort of mucking around in the low level netfs objects
seems wrong to me.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 2/2] erofs: use netfs helpers manipulating request and subrequest

[Jeff Layton]

On Fri, 2022-10-21 at 16:49 +0800, Jingbo Xu wrote:
> Use netfs_put_subrequest() and netfs_rreq_completed() completing request
> and subrequest.
> 
> It is worth noting that a noop netfs_request_ops is introduced for erofs
> since some netfs routine, e.g. netfs_free_request(), will call into
> this ops.
> 
> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
> ---
>  fs/erofs/fscache.c | 47 ++++++++++------------------------------------
>  1 file changed, 10 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/erofs/fscache.c b/fs/erofs/fscache.c
> index fe05bc51f9f2..fa3f4ab5e3b6 100644
> --- a/fs/erofs/fscache.c
> +++ b/fs/erofs/fscache.c
> @@ -4,6 +4,7 @@
>   * Copyright (C) 2022, Bytedance Inc. All rights reserved.
>   */
>  #include <linux/fscache.h>
> +#include <trace/events/netfs.h>
>  #include "internal.h"
>  
>  static DEFINE_MUTEX(erofs_domain_list_lock);
> @@ -11,6 +12,8 @@ static DEFINE_MUTEX(erofs_domain_cookies_lock);
>  static LIST_HEAD(erofs_domain_list);
>  static struct vfsmount *erofs_pseudo_mnt;
>  
> +static const struct netfs_request_ops erofs_noop_req_ops;
> +
>  static struct netfs_io_request *erofs_fscache_alloc_request(struct address_space *mapping,
>  					     loff_t start, size_t len)
>  {
> @@ -24,40 +27,12 @@ static struct netfs_io_request *erofs_fscache_alloc_request(struct address_space
>  	rreq->len	= len;
>  	rreq->mapping	= mapping;
>  	rreq->inode	= mapping->host;
> +	rreq->netfs_ops	= &erofs_noop_req_ops;
>  	INIT_LIST_HEAD(&rreq->subrequests);
>  	refcount_set(&rreq->ref, 1);
>  	return rreq;
>  }
>  

Why is erofs allocating its own netfs structures? This seems quite
fragile, and a layering violation too.

> -static void erofs_fscache_put_request(struct netfs_io_request *rreq)
> -{
> -	if (!refcount_dec_and_test(&rreq->ref))
> -		return;
> -	if (rreq->cache_resources.ops)
> -		rreq->cache_resources.ops->end_operation(&rreq->cache_resources);
> -	kfree(rreq);
> -}
> -
> -static void erofs_fscache_put_subrequest(struct netfs_io_subrequest *subreq)
> -{
> -	if (!refcount_dec_and_test(&subreq->ref))
> -		return;
> -	erofs_fscache_put_request(subreq->rreq);
> -	kfree(subreq);
> -}
> -
> -static void erofs_fscache_clear_subrequests(struct netfs_io_request *rreq)
> -{
> -	struct netfs_io_subrequest *subreq;
> -
> -	while (!list_empty(&rreq->subrequests)) {
> -		subreq = list_first_entry(&rreq->subrequests,
> -				struct netfs_io_subrequest, rreq_link);
> -		list_del(&subreq->rreq_link);
> -		erofs_fscache_put_subrequest(subreq);
> -	}
> -}
> -
>  static void erofs_fscache_rreq_unlock_folios(struct netfs_io_request *rreq)
>  {
>  	struct netfs_io_subrequest *subreq;
> @@ -114,11 +89,10 @@ static void erofs_fscache_rreq_unlock_folios(struct netfs_io_request *rreq)
>  static void erofs_fscache_rreq_complete(struct netfs_io_request *rreq)
>  {
>  	erofs_fscache_rreq_unlock_folios(rreq);
> -	erofs_fscache_clear_subrequests(rreq);
> -	erofs_fscache_put_request(rreq);
> +	netfs_rreq_completed(rreq, false);
>  }
>  
> -static void erofc_fscache_subreq_complete(void *priv,
> +static void erofs_fscache_subreq_complete(void *priv,
>  		ssize_t transferred_or_error, bool was_async)
>  {
>  	struct netfs_io_subrequest *subreq = priv;
> @@ -130,7 +104,7 @@ static void erofc_fscache_subreq_complete(void *priv,
>  	if (atomic_dec_and_test(&rreq->nr_outstanding))
>  		erofs_fscache_rreq_complete(rreq);
>  
> -	erofs_fscache_put_subrequest(subreq);
> +	netfs_put_subrequest(subreq, false, netfs_sreq_trace_put_terminated);
>  }
>  
>  /*
> @@ -171,9 +145,8 @@ static int erofs_fscache_read_folios_async(struct fscache_cookie *cookie,
>  		}
>  
>  		subreq->start = pstart + done;
> -		subreq->len	=  len - done;
> +		subreq->len   =  len - done;
>  		subreq->flags = 1 << NETFS_SREQ_ONDEMAND;
> -
>  		list_add_tail(&subreq->rreq_link, &rreq->subrequests);
>  
>  		source = cres->ops->prepare_read(subreq, LLONG_MAX);
> @@ -184,7 +157,7 @@ static int erofs_fscache_read_folios_async(struct fscache_cookie *cookie,
>  				  source);
>  			ret = -EIO;
>  			subreq->error = ret;
> -			erofs_fscache_put_subrequest(subreq);
> +			netfs_put_subrequest(subreq, false, netfs_sreq_trace_put_failed);
>  			goto out;
>  		}
>  
> @@ -195,7 +168,7 @@ static int erofs_fscache_read_folios_async(struct fscache_cookie *cookie,
>  
>  		ret = fscache_read(cres, subreq->start, &iter,
>  				   NETFS_READ_HOLE_FAIL,
> -				   erofc_fscache_subreq_complete, subreq);
> +				   erofs_fscache_subreq_complete, subreq);
>  		if (ret == -EIOCBQUEUED)
>  			ret = 0;
>  		if (ret) {

I'd rather see this done differently. Either change erofs to use the
netfs infrastructure in a more standard fashion, or maybe consider
teaching erofs to talk to cachefiles directly?

IDK, but this sort of mucking around in the low level netfs objects
seems wrong to me.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 2/2] erofs: use netfs helpers manipulating request and subrequest

[Gao Xiang]

Hi Jeff,

On Fri, Oct 21, 2022 at 08:38:38AM -0400, Jeff Layton wrote:
> On Fri, 2022-10-21 at 16:49 +0800, Jingbo Xu wrote:
> > Use netfs_put_subrequest() and netfs_rreq_completed() completing request
> > and subrequest.
> > 
> > It is worth noting that a noop netfs_request_ops is introduced for erofs
> > since some netfs routine, e.g. netfs_free_request(), will call into
> > this ops.
> > 
> > Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
> > ---
> >  fs/erofs/fscache.c | 47 ++++++++++------------------------------------
> >  1 file changed, 10 insertions(+), 37 deletions(-)
> > 
> > diff --git a/fs/erofs/fscache.c b/fs/erofs/fscache.c
> > index fe05bc51f9f2..fa3f4ab5e3b6 100644
> > --- a/fs/erofs/fscache.c
> > +++ b/fs/erofs/fscache.c
> > @@ -4,6 +4,7 @@
> >   * Copyright (C) 2022, Bytedance Inc. All rights reserved.
> >   */
> >  #include <linux/fscache.h>
> > +#include <trace/events/netfs.h>
> >  #include "internal.h"
> >  
> >  static DEFINE_MUTEX(erofs_domain_list_lock);
> > @@ -11,6 +12,8 @@ static DEFINE_MUTEX(erofs_domain_cookies_lock);
> >  static LIST_HEAD(erofs_domain_list);
> >  static struct vfsmount *erofs_pseudo_mnt;
> >  
> > +static const struct netfs_request_ops erofs_noop_req_ops;
> > +
> >  static struct netfs_io_request *erofs_fscache_alloc_request(struct address_space *mapping,
> >  					     loff_t start, size_t len)
> >  {
> > @@ -24,40 +27,12 @@ static struct netfs_io_request *erofs_fscache_alloc_request(struct address_space
> >  	rreq->len	= len;
> >  	rreq->mapping	= mapping;
> >  	rreq->inode	= mapping->host;
> > +	rreq->netfs_ops	= &erofs_noop_req_ops;
> >  	INIT_LIST_HEAD(&rreq->subrequests);
> >  	refcount_set(&rreq->ref, 1);
> >  	return rreq;
> >  }
> >  
> 
> Why is erofs allocating its own netfs structures? This seems quite
> fragile, and a layering violation too.

Thanks for the reply.

I've talked this to David on IRC about this as well.  Actually what we
really want to use is to do raw I/O requests directly to
fscache/cachefiles.

Because as I said for many times, new netfs per-inode cookie model
doesn't suit for erofs use cases.  Since we treat each cookie as a
data blob, and each erofs inode can refer to one or more blobs in
the chunk-deduplicated way.

So we need a raw I/O data request to fscache/cachefiles.  I'm not sure
if it will also be a future feature request (raw I/O request on cookies)
for network fses in order to get some special file data/metadata.

> 
> > -static void erofs_fscache_put_request(struct netfs_io_request *rreq)
> > -{
> > -	if (!refcount_dec_and_test(&rreq->ref))
> > -		return;
> > -	if (rreq->cache_resources.ops)
> > -		rreq->cache_resources.ops->end_operation(&rreq->cache_resources);
> > -	kfree(rreq);
> > -}
> > -
> > -static void erofs_fscache_put_subrequest(struct netfs_io_subrequest *subreq)
> > -{
> > -	if (!refcount_dec_and_test(&subreq->ref))
> > -		return;
> > -	erofs_fscache_put_request(subreq->rreq);
> > -	kfree(subreq);
> > -}
> > -
> > -static void erofs_fscache_clear_subrequests(struct netfs_io_request *rreq)
> > -{
> > -	struct netfs_io_subrequest *subreq;
> > -
> > -	while (!list_empty(&rreq->subrequests)) {
> > -		subreq = list_first_entry(&rreq->subrequests,
> > -				struct netfs_io_subrequest, rreq_link);
> > -		list_del(&subreq->rreq_link);
> > -		erofs_fscache_put_subrequest(subreq);
> > -	}
> > -}
> > -
> >  static void erofs_fscache_rreq_unlock_folios(struct netfs_io_request *rreq)
> >  {
> >  	struct netfs_io_subrequest *subreq;
> > @@ -114,11 +89,10 @@ static void erofs_fscache_rreq_unlock_folios(struct netfs_io_request *rreq)
> >  static void erofs_fscache_rreq_complete(struct netfs_io_request *rreq)
> >  {
> >  	erofs_fscache_rreq_unlock_folios(rreq);
> > -	erofs_fscache_clear_subrequests(rreq);
> > -	erofs_fscache_put_request(rreq);
> > +	netfs_rreq_completed(rreq, false);
> >  }
> >  
> > -static void erofc_fscache_subreq_complete(void *priv,
> > +static void erofs_fscache_subreq_complete(void *priv,
> >  		ssize_t transferred_or_error, bool was_async)
> >  {
> >  	struct netfs_io_subrequest *subreq = priv;
> > @@ -130,7 +104,7 @@ static void erofc_fscache_subreq_complete(void *priv,
> >  	if (atomic_dec_and_test(&rreq->nr_outstanding))
> >  		erofs_fscache_rreq_complete(rreq);
> >  
> > -	erofs_fscache_put_subrequest(subreq);
> > +	netfs_put_subrequest(subreq, false, netfs_sreq_trace_put_terminated);
> >  }
> >  
> >  /*
> > @@ -171,9 +145,8 @@ static int erofs_fscache_read_folios_async(struct fscache_cookie *cookie,
> >  		}
> >  
> >  		subreq->start = pstart + done;
> > -		subreq->len	=  len - done;
> > +		subreq->len   =  len - done;
> >  		subreq->flags = 1 << NETFS_SREQ_ONDEMAND;
> > -
> >  		list_add_tail(&subreq->rreq_link, &rreq->subrequests);
> >  
> >  		source = cres->ops->prepare_read(subreq, LLONG_MAX);
> > @@ -184,7 +157,7 @@ static int erofs_fscache_read_folios_async(struct fscache_cookie *cookie,
> >  				  source);
> >  			ret = -EIO;
> >  			subreq->error = ret;
> > -			erofs_fscache_put_subrequest(subreq);
> > +			netfs_put_subrequest(subreq, false, netfs_sreq_trace_put_failed);
> >  			goto out;
> >  		}
> >  
> > @@ -195,7 +168,7 @@ static int erofs_fscache_read_folios_async(struct fscache_cookie *cookie,
> >  
> >  		ret = fscache_read(cres, subreq->start, &iter,
> >  				   NETFS_READ_HOLE_FAIL,
> > -				   erofc_fscache_subreq_complete, subreq);
> > +				   erofs_fscache_subreq_complete, subreq);
> >  		if (ret == -EIOCBQUEUED)
> >  			ret = 0;
> >  		if (ret) {
> 
> I'd rather see this done differently. Either change erofs to use the
> netfs infrastructure in a more standard fashion, or maybe consider
> teaching erofs to talk to cachefiles directly?

I've requested David on IRC to shift netfs_io_request and
netfs_io_subrequest into a more natural new prefix other than
(netfs or fscache) but we didn't get into a proper conclusion (David
don't want to use fscache_ since fscache can be disabled but netfs
can still work.)

Like what we said for many times before, the reason why EROFS uses
fscache/cachefiles infrastructure is that we don't want to
duplicate/reinvent another same caching subsystem in order to manage
local caching in order to do lazy pulling / on-demand read, and the
majority code can be shared other than exist the same two codebases
to do the same thing, also:

content map: https://listman.redhat.com/archives/linux-cachefs/2022-August/007050.html

Also if we have the only one caching subsystem, the main codebase can
be tested better compared with two duplicated ones.

And I think overlayfs can also use it for partial copy up as anyone
interested.

> 
> IDK, but this sort of mucking around in the low level netfs objects
> seems wrong to me.

My suggestion is to abstract natural raw data interfaces for all fses
to use, rather than expose a per-inode cookie netfs interface only.

Thanks,
Gao Xiang

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

Re: [PATCH 2/2] erofs: use netfs helpers manipulating request and subrequest

[Gao Xiang]

Hi Jeff,

On Fri, Oct 21, 2022 at 08:38:38AM -0400, Jeff Layton wrote:
> On Fri, 2022-10-21 at 16:49 +0800, Jingbo Xu wrote:
> > Use netfs_put_subrequest() and netfs_rreq_completed() completing request
> > and subrequest.
> > 
> > It is worth noting that a noop netfs_request_ops is introduced for erofs
> > since some netfs routine, e.g. netfs_free_request(), will call into
> > this ops.
> > 
> > Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
> > ---
> >  fs/erofs/fscache.c | 47 ++++++++++------------------------------------
> >  1 file changed, 10 insertions(+), 37 deletions(-)
> > 
> > diff --git a/fs/erofs/fscache.c b/fs/erofs/fscache.c
> > index fe05bc51f9f2..fa3f4ab5e3b6 100644
> > --- a/fs/erofs/fscache.c
> > +++ b/fs/erofs/fscache.c
> > @@ -4,6 +4,7 @@
> >   * Copyright (C) 2022, Bytedance Inc. All rights reserved.
> >   */
> >  #include <linux/fscache.h>
> > +#include <trace/events/netfs.h>
> >  #include "internal.h"
> >  
> >  static DEFINE_MUTEX(erofs_domain_list_lock);
> > @@ -11,6 +12,8 @@ static DEFINE_MUTEX(erofs_domain_cookies_lock);
> >  static LIST_HEAD(erofs_domain_list);
> >  static struct vfsmount *erofs_pseudo_mnt;
> >  
> > +static const struct netfs_request_ops erofs_noop_req_ops;
> > +
> >  static struct netfs_io_request *erofs_fscache_alloc_request(struct address_space *mapping,
> >  					     loff_t start, size_t len)
> >  {
> > @@ -24,40 +27,12 @@ static struct netfs_io_request *erofs_fscache_alloc_request(struct address_space
> >  	rreq->len	= len;
> >  	rreq->mapping	= mapping;
> >  	rreq->inode	= mapping->host;
> > +	rreq->netfs_ops	= &erofs_noop_req_ops;
> >  	INIT_LIST_HEAD(&rreq->subrequests);
> >  	refcount_set(&rreq->ref, 1);
> >  	return rreq;
> >  }
> >  
> 
> Why is erofs allocating its own netfs structures? This seems quite
> fragile, and a layering violation too.

Thanks for the reply.

I've talked this to David on IRC about this as well.  Actually what we
really want to use is to do raw I/O requests directly to
fscache/cachefiles.

Because as I said for many times, new netfs per-inode cookie model
doesn't suit for erofs use cases.  Since we treat each cookie as a
data blob, and each erofs inode can refer to one or more blobs in
the chunk-deduplicated way.

So we need a raw I/O data request to fscache/cachefiles.  I'm not sure
if it will also be a future feature request (raw I/O request on cookies)
for network fses in order to get some special file data/metadata.

> 
> > -static void erofs_fscache_put_request(struct netfs_io_request *rreq)
> > -{
> > -	if (!refcount_dec_and_test(&rreq->ref))
> > -		return;
> > -	if (rreq->cache_resources.ops)
> > -		rreq->cache_resources.ops->end_operation(&rreq->cache_resources);
> > -	kfree(rreq);
> > -}
> > -
> > -static void erofs_fscache_put_subrequest(struct netfs_io_subrequest *subreq)
> > -{
> > -	if (!refcount_dec_and_test(&subreq->ref))
> > -		return;
> > -	erofs_fscache_put_request(subreq->rreq);
> > -	kfree(subreq);
> > -}
> > -
> > -static void erofs_fscache_clear_subrequests(struct netfs_io_request *rreq)
> > -{
> > -	struct netfs_io_subrequest *subreq;
> > -
> > -	while (!list_empty(&rreq->subrequests)) {
> > -		subreq = list_first_entry(&rreq->subrequests,
> > -				struct netfs_io_subrequest, rreq_link);
> > -		list_del(&subreq->rreq_link);
> > -		erofs_fscache_put_subrequest(subreq);
> > -	}
> > -}
> > -
> >  static void erofs_fscache_rreq_unlock_folios(struct netfs_io_request *rreq)
> >  {
> >  	struct netfs_io_subrequest *subreq;
> > @@ -114,11 +89,10 @@ static void erofs_fscache_rreq_unlock_folios(struct netfs_io_request *rreq)
> >  static void erofs_fscache_rreq_complete(struct netfs_io_request *rreq)
> >  {
> >  	erofs_fscache_rreq_unlock_folios(rreq);
> > -	erofs_fscache_clear_subrequests(rreq);
> > -	erofs_fscache_put_request(rreq);
> > +	netfs_rreq_completed(rreq, false);
> >  }
> >  
> > -static void erofc_fscache_subreq_complete(void *priv,
> > +static void erofs_fscache_subreq_complete(void *priv,
> >  		ssize_t transferred_or_error, bool was_async)
> >  {
> >  	struct netfs_io_subrequest *subreq = priv;
> > @@ -130,7 +104,7 @@ static void erofc_fscache_subreq_complete(void *priv,
> >  	if (atomic_dec_and_test(&rreq->nr_outstanding))
> >  		erofs_fscache_rreq_complete(rreq);
> >  
> > -	erofs_fscache_put_subrequest(subreq);
> > +	netfs_put_subrequest(subreq, false, netfs_sreq_trace_put_terminated);
> >  }
> >  
> >  /*
> > @@ -171,9 +145,8 @@ static int erofs_fscache_read_folios_async(struct fscache_cookie *cookie,
> >  		}
> >  
> >  		subreq->start = pstart + done;
> > -		subreq->len	=  len - done;
> > +		subreq->len   =  len - done;
> >  		subreq->flags = 1 << NETFS_SREQ_ONDEMAND;
> > -
> >  		list_add_tail(&subreq->rreq_link, &rreq->subrequests);
> >  
> >  		source = cres->ops->prepare_read(subreq, LLONG_MAX);
> > @@ -184,7 +157,7 @@ static int erofs_fscache_read_folios_async(struct fscache_cookie *cookie,
> >  				  source);
> >  			ret = -EIO;
> >  			subreq->error = ret;
> > -			erofs_fscache_put_subrequest(subreq);
> > +			netfs_put_subrequest(subreq, false, netfs_sreq_trace_put_failed);
> >  			goto out;
> >  		}
> >  
> > @@ -195,7 +168,7 @@ static int erofs_fscache_read_folios_async(struct fscache_cookie *cookie,
> >  
> >  		ret = fscache_read(cres, subreq->start, &iter,
> >  				   NETFS_READ_HOLE_FAIL,
> > -				   erofc_fscache_subreq_complete, subreq);
> > +				   erofs_fscache_subreq_complete, subreq);
> >  		if (ret == -EIOCBQUEUED)
> >  			ret = 0;
> >  		if (ret) {
> 
> I'd rather see this done differently. Either change erofs to use the
> netfs infrastructure in a more standard fashion, or maybe consider
> teaching erofs to talk to cachefiles directly?

I've requested David on IRC to shift netfs_io_request and
netfs_io_subrequest into a more natural new prefix other than
(netfs or fscache) but we didn't get into a proper conclusion (David
don't want to use fscache_ since fscache can be disabled but netfs
can still work.)

Like what we said for many times before, the reason why EROFS uses
fscache/cachefiles infrastructure is that we don't want to
duplicate/reinvent another same caching subsystem in order to manage
local caching in order to do lazy pulling / on-demand read, and the
majority code can be shared other than exist the same two codebases
to do the same thing, also:

content map: https://listman.redhat.com/archives/linux-cachefs/2022-August/007050.html

Also if we have the only one caching subsystem, the main codebase can
be tested better compared with two duplicated ones.

And I think overlayfs can also use it for partial copy up as anyone
interested.

> 
> IDK, but this sort of mucking around in the low level netfs objects
> seems wrong to me.

My suggestion is to abstract natural raw data interfaces for all fses
to use, rather than expose a per-inode cookie netfs interface only.

Thanks,
Gao Xiang

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