[RFC PATCH 0/5] Introduce daemon failover mechanism to recover from crashing

[RFC PATCH 0/5] Introduce daemon failover mechanism to recover from crashing

[Jia Zhu]

[Background]
============
In ondemand read mode, if user daemon closes anonymous fd(e.g. daemon
crashes), subsequent read and inflight requests based on these fd will
return -EIO.
Even if above mentioned case is tolerable for some individual users, but
when it happenens in real cloud service production environment, such IO
errors will be passed to cloud service users and impact its working jobs.
It's terrible for cloud service stability.

[Design]
========
This patchset introduce three states for ondemand object:
CLOSE: Object which just be allocated or closed by user daemon.
OPEN: Object which related OPEN request has been processed correctly.
REOPENING: Object which has been closed, and is drived to open by a read
request.

[Flow Path]
===========
[Daemon Crash] 
0. Daemon use UDS send/receive fd to keep and pass the fd reference of
   "/dev/cachefiles".
1. User daemon crashes -> restart and recover dev fd's reference.
2. User daemon write "restore" to device.
   2.1 Reset the object's state from CLOSE to OPENING.
   2.2 Init a work which reinit the object and add it to wq. (daemon can
       get rid of kernel space and handle that open request).
3. The user of upper filesystem won't notice that the daemon ever crashed
   since the inflight IO is restored and handled correctly.

[Daemon Close fd]
1. User daemon closes an anonymous fd.
2. User daemon reads a READ request which the associated anonymous fd was
   closed and init a work which re-open the object.
3. User daemon handles above open request normally.
4. The user of upper filesystem won't notice that the daemon ever closed
   any fd since the closed object is re-opened and related request was
   handled correctly.

[Test]
======
There is a testcase for above mentioned scenario.
A user process read the file by fscache ondemand reading.
At the same time, we kill the daemon constantly.
The expected result is that the file read by user is consistent with
original, and the user doesn't notice that daemon has ever been killed.

https://github.com/userzj/demand-read-cachefilesd/commits/failover-test

[GitWeb]
========
https://github.com/userzj/linux/tree/fscache-failover-v1

Jia Zhu (5):
  cachefiles: introduce object ondemand state
  cachefiles: extract ondemand info field from cachefiles_object
  cachefiles: resend an open request if the read request's object is
    closed
  cachefiles: narrow the scope of triggering EPOLLIN events in ondemand
    mode
  cachefiles: add restore command to recover inflight ondemand read
    requests

 fs/cachefiles/daemon.c    |  14 +++-
 fs/cachefiles/interface.c |   6 ++
 fs/cachefiles/internal.h  |  74 ++++++++++++++++++++-
 fs/cachefiles/ondemand.c  | 135 ++++++++++++++++++++++++++++----------
 4 files changed, 193 insertions(+), 36 deletions(-)

-- 
2.20.1

[RFC PATCH 1/5] cachefiles: introduce object ondemand state

[Jia Zhu]

Previously, @ondemand_id field was used not only to identify ondemand
state of the object, but also to represent the index of the xarray.
This commit introduces @state field to decouple the role of @ondemand_id
and adds helpers to access it.

Signed-off-by: Jia Zhu <zhujia.zj@bytedance.com>
Reviewed-by: Xin Yin <yinxin.x@bytedance.com>
---
 fs/cachefiles/internal.h | 33 +++++++++++++++++++++++++++++++++
 fs/cachefiles/ondemand.c | 15 +++++++++------
 2 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
index 6cba2c6de2f9..6661b3e361da 100644
--- a/fs/cachefiles/internal.h
+++ b/fs/cachefiles/internal.h
@@ -17,6 +17,7 @@
 #include <linux/security.h>
 #include <linux/xarray.h>
 #include <linux/cachefiles.h>
+#include <linux/atomic.h>
 
 #define CACHEFILES_DIO_BLOCK_SIZE 4096
 
@@ -44,6 +45,11 @@ struct cachefiles_volume {
 	struct dentry			*fanout[256];	/* Fanout subdirs */
 };
 
+enum cachefiles_object_state {
+	CACHEFILES_ONDEMAND_OBJSTATE_close, /* Anonymous fd closed by daemon or initial state */
+	CACHEFILES_ONDEMAND_OBJSTATE_open, /* Anonymous fd associated with object is available */
+};
+
 /*
  * Backing file state.
  */
@@ -62,6 +68,7 @@ struct cachefiles_object {
 #define CACHEFILES_OBJECT_USING_TMPFILE	0		/* Have an unlinked tmpfile */
 #ifdef CONFIG_CACHEFILES_ONDEMAND
 	int				ondemand_id;
+	enum cachefiles_object_state	state;
 #endif
 };
 
@@ -295,6 +302,32 @@ extern void cachefiles_ondemand_clean_object(struct cachefiles_object *object);
 extern int cachefiles_ondemand_read(struct cachefiles_object *object,
 				    loff_t pos, size_t len);
 
+#define CACHEFILES_OBJECT_STATE_FUNCS(_state)	\
+static inline bool								\
+cachefiles_ondemand_object_is_##_state(const struct cachefiles_object *object) \
+{												\
+	/*
+	 * Pairs with smp_store_release() in set_object_##_state()
+	 * I.e. another task can publish state concurrently, by executing
+	 * a RELEASE barrier. We need to use smp_load_acquire() here
+	 * to safely ACQUIRE the memory the other task published.
+	 */											\
+	return smp_load_acquire(&object->state) == CACHEFILES_ONDEMAND_OBJSTATE_##_state; \
+}												\
+												\
+static inline void								\
+cachefiles_ondemand_set_object_##_state(struct cachefiles_object *object) \
+{												\
+	/*
+	 * Pairs with smp_load_acquire() in object_is_##_state()
+	 * I.e. here we publish a state with a RELEASE barrier
+	 * so that concurrent tasks can ACQUIRE it.
+	 */											\
+	smp_store_release(&object->state, CACHEFILES_ONDEMAND_OBJSTATE_##_state); \
+}
+
+CACHEFILES_OBJECT_STATE_FUNCS(open);
+CACHEFILES_OBJECT_STATE_FUNCS(close);
 #else
 static inline ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
 					char __user *_buffer, size_t buflen)
diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
index 1fee702d5529..e3155a5f32e4 100644
--- a/fs/cachefiles/ondemand.c
+++ b/fs/cachefiles/ondemand.c
@@ -15,6 +15,7 @@ static int cachefiles_ondemand_fd_release(struct inode *inode,
 
 	xa_lock(&cache->reqs);
 	object->ondemand_id = CACHEFILES_ONDEMAND_ID_CLOSED;
+	cachefiles_ondemand_set_object_close(object);
 
 	/*
 	 * Flush all pending READ requests since their completion depends on
@@ -172,6 +173,8 @@ int cachefiles_ondemand_copen(struct cachefiles_cache *cache, char *args)
 		set_bit(FSCACHE_COOKIE_NO_DATA_TO_READ, &cookie->flags);
 	trace_cachefiles_ondemand_copen(req->object, id, size);
 
+	cachefiles_ondemand_set_object_open(req->object);
+
 out:
 	complete(&req->done);
 	return ret;
@@ -353,7 +356,8 @@ static int cachefiles_ondemand_send_req(struct cachefiles_object *object,
 		/* coupled with the barrier in cachefiles_flush_reqs() */
 		smp_mb();
 
-		if (opcode != CACHEFILES_OP_OPEN && object->ondemand_id <= 0) {
+		if (opcode != CACHEFILES_OP_OPEN &&
+			!cachefiles_ondemand_object_is_open(object)) {
 			WARN_ON_ONCE(object->ondemand_id == 0);
 			xas_unlock(&xas);
 			ret = -EIO;
@@ -420,7 +424,6 @@ static int cachefiles_ondemand_init_close_req(struct cachefiles_req *req,
 					      void *private)
 {
 	struct cachefiles_object *object = req->object;
-	int object_id = object->ondemand_id;
 
 	/*
 	 * It's possible that object id is still 0 if the cookie looking up
@@ -428,10 +431,10 @@ static int cachefiles_ondemand_init_close_req(struct cachefiles_req *req,
 	 * sending CLOSE request for CACHEFILES_ONDEMAND_ID_CLOSED, which means
 	 * anon_fd has already been closed.
 	 */
-	if (object_id <= 0)
+	if (!cachefiles_ondemand_object_is_open(object))
 		return -ENOENT;
 
-	req->msg.object_id = object_id;
+	req->msg.object_id = object->ondemand_id;
 	trace_cachefiles_ondemand_close(object, &req->msg);
 	return 0;
 }
@@ -450,7 +453,7 @@ static int cachefiles_ondemand_init_read_req(struct cachefiles_req *req,
 	int object_id = object->ondemand_id;
 
 	/* Stop enqueuing requests when daemon has closed anon_fd. */
-	if (object_id <= 0) {
+	if (!cachefiles_ondemand_object_is_open(object)) {
 		WARN_ON_ONCE(object_id == 0);
 		pr_info_once("READ: anonymous fd closed prematurely.\n");
 		return -EIO;
@@ -475,7 +478,7 @@ int cachefiles_ondemand_init_object(struct cachefiles_object *object)
 	 * creating a new tmpfile as the cache file. Reuse the previously
 	 * allocated object ID if any.
 	 */
-	if (object->ondemand_id > 0)
+	if (cachefiles_ondemand_object_is_open(object))
 		return 0;
 
 	volume_key_size = volume->key[0] + 1;
-- 
2.20.1

[RFC PATCH 2/5] cachefiles: extract ondemand info field from cachefiles_object

[Jia Zhu]

We'll introduce a @work_struct field for @object in subsequent patches,
it will enlarge the size of @object.
As the result of that, this commit extracts ondemand info field from
@object.

Signed-off-by: Jia Zhu <zhujia.zj@bytedance.com>
---
 fs/cachefiles/interface.c |  6 ++++++
 fs/cachefiles/internal.h  | 29 +++++++++++++++++++++++++----
 fs/cachefiles/ondemand.c  | 28 ++++++++++++++++++++++------
 3 files changed, 53 insertions(+), 10 deletions(-)

diff --git a/fs/cachefiles/interface.c b/fs/cachefiles/interface.c
index a69073a1d3f0..f21f5660ea7f 100644
--- a/fs/cachefiles/interface.c
+++ b/fs/cachefiles/interface.c
@@ -31,6 +31,11 @@ struct cachefiles_object *cachefiles_alloc_object(struct fscache_cookie *cookie)
 	if (!object)
 		return NULL;
 
+	if (cachefiles_ondemand_init_obj_info(object, volume)) {
+		kmem_cache_free(cachefiles_object_jar, object);
+		return NULL;
+	}
+
 	refcount_set(&object->ref, 1);
 
 	spin_lock_init(&object->lock);
@@ -88,6 +93,7 @@ void cachefiles_put_object(struct cachefiles_object *object,
 		ASSERTCMP(object->file, ==, NULL);
 
 		kfree(object->d_name);
+		kfree(CACHEFILES_ONDEMAND_OBJINFO(object));
 
 		cache = object->volume->cache->cache;
 		fscache_put_cookie(object->cookie, fscache_cookie_put_object);
diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
index 6661b3e361da..cdf4ec781933 100644
--- a/fs/cachefiles/internal.h
+++ b/fs/cachefiles/internal.h
@@ -50,6 +50,12 @@ enum cachefiles_object_state {
 	CACHEFILES_ONDEMAND_OBJSTATE_open, /* Anonymous fd associated with object is available */
 };
 
+struct cachefiles_ondemand_info {
+	int				ondemand_id;
+	enum cachefiles_object_state	state;
+	struct cachefiles_object	*object;
+};
+
 /*
  * Backing file state.
  */
@@ -67,8 +73,7 @@ struct cachefiles_object {
 	unsigned long			flags;
 #define CACHEFILES_OBJECT_USING_TMPFILE	0		/* Have an unlinked tmpfile */
 #ifdef CONFIG_CACHEFILES_ONDEMAND
-	int				ondemand_id;
-	enum cachefiles_object_state	state;
+	void				*private;
 #endif
 };
 
@@ -302,6 +307,12 @@ extern void cachefiles_ondemand_clean_object(struct cachefiles_object *object);
 extern int cachefiles_ondemand_read(struct cachefiles_object *object,
 				    loff_t pos, size_t len);
 
+extern int cachefiles_ondemand_init_obj_info(struct cachefiles_object *object,
+					struct cachefiles_volume *volume);
+
+#define CACHEFILES_ONDEMAND_OBJINFO(object)	\
+		((struct cachefiles_ondemand_info *)(object)->private)
+
 #define CACHEFILES_OBJECT_STATE_FUNCS(_state)	\
 static inline bool								\
 cachefiles_ondemand_object_is_##_state(const struct cachefiles_object *object) \
@@ -312,7 +323,8 @@ cachefiles_ondemand_object_is_##_state(const struct cachefiles_object *object) \
 	 * a RELEASE barrier. We need to use smp_load_acquire() here
 	 * to safely ACQUIRE the memory the other task published.
 	 */											\
-	return smp_load_acquire(&object->state) == CACHEFILES_ONDEMAND_OBJSTATE_##_state; \
+	return smp_load_acquire(&(CACHEFILES_ONDEMAND_OBJINFO(object)->state)) == \
+			CACHEFILES_ONDEMAND_OBJSTATE_##_state; \
 }												\
 												\
 static inline void								\
@@ -323,12 +335,15 @@ cachefiles_ondemand_set_object_##_state(struct cachefiles_object *object) \
 	 * I.e. here we publish a state with a RELEASE barrier
 	 * so that concurrent tasks can ACQUIRE it.
 	 */											\
-	smp_store_release(&object->state, CACHEFILES_ONDEMAND_OBJSTATE_##_state); \
+	smp_store_release(&(CACHEFILES_ONDEMAND_OBJINFO(object)->state),	\
+		CACHEFILES_ONDEMAND_OBJSTATE_##_state); \
 }
 
 CACHEFILES_OBJECT_STATE_FUNCS(open);
 CACHEFILES_OBJECT_STATE_FUNCS(close);
 #else
+#define CACHEFILES_ONDEMAND_OBJINFO(object)	NULL
+
 static inline ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
 					char __user *_buffer, size_t buflen)
 {
@@ -349,6 +364,12 @@ static inline int cachefiles_ondemand_read(struct cachefiles_object *object,
 {
 	return -EOPNOTSUPP;
 }
+
+static inline int cachefiles_ondemand_init_obj_info(struct cachefiles_object *object,
+						struct cachefiles_volume *volume)
+{
+	return 0;
+}
 #endif
 
 /*
diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
index e3155a5f32e4..f51266554e4d 100644
--- a/fs/cachefiles/ondemand.c
+++ b/fs/cachefiles/ondemand.c
@@ -9,12 +9,13 @@ static int cachefiles_ondemand_fd_release(struct inode *inode,
 {
 	struct cachefiles_object *object = file->private_data;
 	struct cachefiles_cache *cache = object->volume->cache;
-	int object_id = object->ondemand_id;
+	struct cachefiles_ondemand_info *info = CACHEFILES_ONDEMAND_OBJINFO(object);
+	int object_id = info->ondemand_id;
 	struct cachefiles_req *req;
 	XA_STATE(xas, &cache->reqs, 0);
 
 	xa_lock(&cache->reqs);
-	object->ondemand_id = CACHEFILES_ONDEMAND_ID_CLOSED;
+	info->ondemand_id = CACHEFILES_ONDEMAND_ID_CLOSED;
 	cachefiles_ondemand_set_object_close(object);
 
 	/*
@@ -218,7 +219,7 @@ static int cachefiles_ondemand_get_fd(struct cachefiles_req *req)
 	load = (void *)req->msg.data;
 	load->fd = fd;
 	req->msg.object_id = object_id;
-	object->ondemand_id = object_id;
+	CACHEFILES_ONDEMAND_OBJINFO(object)->ondemand_id = object_id;
 
 	cachefiles_get_unbind_pincount(cache);
 	trace_cachefiles_ondemand_open(object, &req->msg, load);
@@ -358,7 +359,7 @@ static int cachefiles_ondemand_send_req(struct cachefiles_object *object,
 
 		if (opcode != CACHEFILES_OP_OPEN &&
 			!cachefiles_ondemand_object_is_open(object)) {
-			WARN_ON_ONCE(object->ondemand_id == 0);
+			WARN_ON_ONCE(CACHEFILES_ONDEMAND_OBJINFO(object)->ondemand_id == 0);
 			xas_unlock(&xas);
 			ret = -EIO;
 			goto out;
@@ -434,7 +435,7 @@ static int cachefiles_ondemand_init_close_req(struct cachefiles_req *req,
 	if (!cachefiles_ondemand_object_is_open(object))
 		return -ENOENT;
 
-	req->msg.object_id = object->ondemand_id;
+	req->msg.object_id = CACHEFILES_ONDEMAND_OBJINFO(object)->ondemand_id;
 	trace_cachefiles_ondemand_close(object, &req->msg);
 	return 0;
 }
@@ -450,7 +451,7 @@ static int cachefiles_ondemand_init_read_req(struct cachefiles_req *req,
 	struct cachefiles_object *object = req->object;
 	struct cachefiles_read *load = (void *)req->msg.data;
 	struct cachefiles_read_ctx *read_ctx = private;
-	int object_id = object->ondemand_id;
+	int object_id = CACHEFILES_ONDEMAND_OBJINFO(object)->ondemand_id;
 
 	/* Stop enqueuing requests when daemon has closed anon_fd. */
 	if (!cachefiles_ondemand_object_is_open(object)) {
@@ -496,6 +497,21 @@ void cachefiles_ondemand_clean_object(struct cachefiles_object *object)
 			cachefiles_ondemand_init_close_req, NULL);
 }
 
+int cachefiles_ondemand_init_obj_info(struct cachefiles_object *object,
+				struct cachefiles_volume *volume)
+{
+	if (!cachefiles_in_ondemand_mode(volume->cache))
+		return 0;
+
+	object->private = kzalloc(sizeof(struct cachefiles_ondemand_info),
+					GFP_KERNEL);
+	if (!object->private)
+		return -ENOMEM;
+
+	CACHEFILES_ONDEMAND_OBJINFO(object)->object = object;
+	return 0;
+}
+
 int cachefiles_ondemand_read(struct cachefiles_object *object,
 			     loff_t pos, size_t len)
 {
-- 
2.20.1

Re: [RFC PATCH 2/5] cachefiles: extract ondemand info field from cachefiles_object

[JeffleXu]

On 8/18/22 9:52 PM, Jia Zhu wrote:

>  /*
>   * Backing file state.
>   */
> @@ -67,8 +73,7 @@ struct cachefiles_object {
>  	unsigned long			flags;
>  #define CACHEFILES_OBJECT_USING_TMPFILE	0		/* Have an unlinked tmpfile */
>  #ifdef CONFIG_CACHEFILES_ONDEMAND
> -	int				ondemand_id;
> -	enum cachefiles_object_state	state;
> +	void				*private;
>  #endif
>  };

Personally I would prefer

	struct cachefiles_object {
		...
	#ifdef CONFIG_CACHEFILES_ONDEMAND
		struct cachefiles_ondemand_info *private;
	#endif
	}

and

> @@ -88,6 +93,7 @@ void cachefiles_put_object(struct cachefiles_object
*object,
>  		ASSERTCMP(object->file, ==, NULL);
>
>  		kfree(object->d_name);
> + #ifdef CONFIG_CACHEFILES_ONDEMAND
> +		kfree(object->private);
> + #endif
>
>  		cache = object->volume->cache->cache;
>  		fscache_put_cookie(object->cookie,

so that we can get rid of CACHEFILES_ONDEMAND_OBJINFO() stuff, to make
the code more readable.



-- 
Thanks,
Jingbo

Re: [External] Re: [RFC PATCH 2/5] cachefiles: extract ondemand info field from cachefiles_object

[Jia Zhu]

在 2022/10/8 17:06, JeffleXu 写道:
> 
> 
> On 8/18/22 9:52 PM, Jia Zhu wrote:
> 
>>   /*
>>    * Backing file state.
>>    */
>> @@ -67,8 +73,7 @@ struct cachefiles_object {
>>   	unsigned long			flags;
>>   #define CACHEFILES_OBJECT_USING_TMPFILE	0		/* Have an unlinked tmpfile */
>>   #ifdef CONFIG_CACHEFILES_ONDEMAND
>> -	int				ondemand_id;
>> -	enum cachefiles_object_state	state;
>> +	void				*private;
>>   #endif
>>   };
> 
> Personally I would prefer
> 
> 	struct cachefiles_object {
> 		...
> 	#ifdef CONFIG_CACHEFILES_ONDEMAND
> 		struct cachefiles_ondemand_info *private;
> 	#endif
> 	}
> 
> and
> 
>> @@ -88,6 +93,7 @@ void cachefiles_put_object(struct cachefiles_object
> *object,
>>   		ASSERTCMP(object->file, ==, NULL);
>>
>>   		kfree(object->d_name);
>> + #ifdef CONFIG_CACHEFILES_ONDEMAND
>> +		kfree(object->private);
>> + #endif
>>
>>   		cache = object->volume->cache->cache;
>>   		fscache_put_cookie(object->cookie,
> 
> so that we can get rid of CACHEFILES_ONDEMAND_OBJINFO() stuff, to make
> the code more readable.
Hi JingBo. Thanks for your review. I'll revise it in next version.
> 
> 
> 

[RFC PATCH 3/5] cachefiles: resend an open request if the read request's object is closed

[Jia Zhu]

When an anonymous fd is closed by user daemon, if there is a new read
request for this file comes up, the anonymous fd should be re-opened
to handle that read request rather than fail it directly.

1. Introduce reopening state for objects that are closed but have
   inflight/subsequent read requests.
2. No longer flush READ requests but only CLOSE requests when anonymous
   fd is closed.
3. Enqueue the reopen work to workqueue, thus user daemon could get rid
   of daemon_read context and handle that request smoothly. Otherwise,
   the user daemon will send a reopen request and wait for itself to
   process the request.

Signed-off-by: Jia Zhu <zhujia.zj@bytedance.com>
Reviewed-by: Xin Yin <yinxin.x@bytedance.com>
---
 fs/cachefiles/internal.h |  3 ++
 fs/cachefiles/ondemand.c | 79 +++++++++++++++++++++++++++-------------
 2 files changed, 56 insertions(+), 26 deletions(-)

diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
index cdf4ec781933..66bbd4f1d22a 100644
--- a/fs/cachefiles/internal.h
+++ b/fs/cachefiles/internal.h
@@ -48,9 +48,11 @@ struct cachefiles_volume {
 enum cachefiles_object_state {
 	CACHEFILES_ONDEMAND_OBJSTATE_close, /* Anonymous fd closed by daemon or initial state */
 	CACHEFILES_ONDEMAND_OBJSTATE_open, /* Anonymous fd associated with object is available */
+	CACHEFILES_ONDEMAND_OBJSTATE_reopening, /* Object that was closed and is being reopened. */
 };
 
 struct cachefiles_ondemand_info {
+	struct work_struct		work;
 	int				ondemand_id;
 	enum cachefiles_object_state	state;
 	struct cachefiles_object	*object;
@@ -341,6 +343,7 @@ cachefiles_ondemand_set_object_##_state(struct cachefiles_object *object) \
 
 CACHEFILES_OBJECT_STATE_FUNCS(open);
 CACHEFILES_OBJECT_STATE_FUNCS(close);
+CACHEFILES_OBJECT_STATE_FUNCS(reopening);
 #else
 #define CACHEFILES_ONDEMAND_OBJINFO(object)	NULL
 
diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
index f51266554e4d..79ffb19380cd 100644
--- a/fs/cachefiles/ondemand.c
+++ b/fs/cachefiles/ondemand.c
@@ -18,14 +18,10 @@ static int cachefiles_ondemand_fd_release(struct inode *inode,
 	info->ondemand_id = CACHEFILES_ONDEMAND_ID_CLOSED;
 	cachefiles_ondemand_set_object_close(object);
 
-	/*
-	 * Flush all pending READ requests since their completion depends on
-	 * anon_fd.
-	 */
-	xas_for_each(&xas, req, ULONG_MAX) {
+	/* Only flush CACHEFILES_REQ_NEW marked req to avoid race with daemon_read */
+	xas_for_each_marked(&xas, req, ULONG_MAX, CACHEFILES_REQ_NEW) {
 		if (req->msg.object_id == object_id &&
-		    req->msg.opcode == CACHEFILES_OP_READ) {
-			req->error = -EIO;
+		    req->msg.opcode == CACHEFILES_OP_CLOSE) {
 			complete(&req->done);
 			xas_store(&xas, NULL);
 		}
@@ -175,6 +171,7 @@ int cachefiles_ondemand_copen(struct cachefiles_cache *cache, char *args)
 	trace_cachefiles_ondemand_copen(req->object, id, size);
 
 	cachefiles_ondemand_set_object_open(req->object);
+	wake_up_all(&cache->daemon_pollwq);
 
 out:
 	complete(&req->done);
@@ -234,6 +231,14 @@ static int cachefiles_ondemand_get_fd(struct cachefiles_req *req)
 	return ret;
 }
 
+static void ondemand_object_worker(struct work_struct *work)
+{
+	struct cachefiles_object *object =
+		((struct cachefiles_ondemand_info *)work)->object;
+
+	cachefiles_ondemand_init_object(object);
+}
+
 ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
 					char __user *_buffer, size_t buflen)
 {
@@ -249,7 +254,27 @@ ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
 	 * requests from being processed repeatedly.
 	 */
 	xa_lock(&cache->reqs);
-	req = xas_find_marked(&xas, UINT_MAX, CACHEFILES_REQ_NEW);
+	xas_for_each_marked(&xas, req, UINT_MAX, CACHEFILES_REQ_NEW) {
+		/*
+		 * Reopen the closed object with associated read request.
+		 * Skip read requests whose related object are reopening.
+		 */
+		if (req->msg.opcode == CACHEFILES_OP_READ) {
+			ret = cmpxchg(&CACHEFILES_ONDEMAND_OBJINFO(req->object)->state,
+						  CACHEFILES_ONDEMAND_OBJSTATE_close,
+						  CACHEFILES_ONDEMAND_OBJSTATE_reopening);
+			if (ret == CACHEFILES_ONDEMAND_OBJSTATE_close) {
+				INIT_WORK(&CACHEFILES_ONDEMAND_OBJINFO(req->object)->work,
+						ondemand_object_worker);
+				queue_work(fscache_wq,
+					&CACHEFILES_ONDEMAND_OBJINFO(req->object)->work);
+				continue;
+			} else if (ret == CACHEFILES_ONDEMAND_OBJSTATE_reopening) {
+				continue;
+			}
+		}
+		break;
+	}
 	if (!req) {
 		xa_unlock(&cache->reqs);
 		return 0;
@@ -267,14 +292,18 @@ ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
 	xa_unlock(&cache->reqs);
 
 	id = xas.xa_index;
-	msg->msg_id = id;
 
 	if (msg->opcode == CACHEFILES_OP_OPEN) {
 		ret = cachefiles_ondemand_get_fd(req);
-		if (ret)
+		if (ret) {
+			cachefiles_ondemand_set_object_close(req->object);
 			goto error;
+		}
 	}
 
+	msg->msg_id = id;
+	msg->object_id = CACHEFILES_ONDEMAND_OBJINFO(req->object)->ondemand_id;
+
 	if (copy_to_user(_buffer, msg, n) != 0) {
 		ret = -EFAULT;
 		goto err_put_fd;
@@ -307,19 +336,23 @@ static int cachefiles_ondemand_send_req(struct cachefiles_object *object,
 					void *private)
 {
 	struct cachefiles_cache *cache = object->volume->cache;
-	struct cachefiles_req *req;
+	struct cachefiles_req *req = NULL;
 	XA_STATE(xas, &cache->reqs, 0);
 	int ret;
 
 	if (!test_bit(CACHEFILES_ONDEMAND_MODE, &cache->flags))
 		return 0;
 
-	if (test_bit(CACHEFILES_DEAD, &cache->flags))
-		return -EIO;
+	if (test_bit(CACHEFILES_DEAD, &cache->flags)) {
+		ret = -EIO;
+		goto out;
+	}
 
 	req = kzalloc(sizeof(*req) + data_len, GFP_KERNEL);
-	if (!req)
-		return -ENOMEM;
+	if (!req) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
 	req->object = object;
 	init_completion(&req->done);
@@ -357,7 +390,7 @@ static int cachefiles_ondemand_send_req(struct cachefiles_object *object,
 		/* coupled with the barrier in cachefiles_flush_reqs() */
 		smp_mb();
 
-		if (opcode != CACHEFILES_OP_OPEN &&
+		if (opcode == CACHEFILES_OP_CLOSE &&
 			!cachefiles_ondemand_object_is_open(object)) {
 			WARN_ON_ONCE(CACHEFILES_ONDEMAND_OBJINFO(object)->ondemand_id == 0);
 			xas_unlock(&xas);
@@ -382,8 +415,12 @@ static int cachefiles_ondemand_send_req(struct cachefiles_object *object,
 	wake_up_all(&cache->daemon_pollwq);
 	wait_for_completion(&req->done);
 	ret = req->error;
+	kfree(req);
+	return ret;
 out:
 	kfree(req);
+	if (opcode == CACHEFILES_OP_OPEN)
+		cachefiles_ondemand_set_object_close(req->object);
 	return ret;
 }
 
@@ -435,7 +472,6 @@ static int cachefiles_ondemand_init_close_req(struct cachefiles_req *req,
 	if (!cachefiles_ondemand_object_is_open(object))
 		return -ENOENT;
 
-	req->msg.object_id = CACHEFILES_ONDEMAND_OBJINFO(object)->ondemand_id;
 	trace_cachefiles_ondemand_close(object, &req->msg);
 	return 0;
 }
@@ -451,16 +487,7 @@ static int cachefiles_ondemand_init_read_req(struct cachefiles_req *req,
 	struct cachefiles_object *object = req->object;
 	struct cachefiles_read *load = (void *)req->msg.data;
 	struct cachefiles_read_ctx *read_ctx = private;
-	int object_id = CACHEFILES_ONDEMAND_OBJINFO(object)->ondemand_id;
 
-	/* Stop enqueuing requests when daemon has closed anon_fd. */
-	if (!cachefiles_ondemand_object_is_open(object)) {
-		WARN_ON_ONCE(object_id == 0);
-		pr_info_once("READ: anonymous fd closed prematurely.\n");
-		return -EIO;
-	}
-
-	req->msg.object_id = object_id;
 	load->off = read_ctx->off;
 	load->len = read_ctx->len;
 	trace_cachefiles_ondemand_read(object, &req->msg, load);
-- 
2.20.1

Re: [RFC PATCH 3/5] cachefiles: resend an open request if the read request's object is closed

[JeffleXu]

On 8/18/22 9:52 PM, Jia Zhu wrote:
> When an anonymous fd is closed by user daemon, if there is a new read
> request for this file comes up, the anonymous fd should be re-opened
> to handle that read request rather than fail it directly.
> 
> 1. Introduce reopening state for objects that are closed but have
>    inflight/subsequent read requests.
> 2. No longer flush READ requests but only CLOSE requests when anonymous
>    fd is closed.
> 3. Enqueue the reopen work to workqueue, thus user daemon could get rid
>    of daemon_read context and handle that request smoothly. Otherwise,
>    the user daemon will send a reopen request and wait for itself to
>    process the request.
> 
> Signed-off-by: Jia Zhu <zhujia.zj@bytedance.com>
> Reviewed-by: Xin Yin <yinxin.x@bytedance.com>
> ---
>  fs/cachefiles/internal.h |  3 ++
>  fs/cachefiles/ondemand.c | 79 +++++++++++++++++++++++++++-------------
>  2 files changed, 56 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
> index cdf4ec781933..66bbd4f1d22a 100644
> --- a/fs/cachefiles/internal.h
> +++ b/fs/cachefiles/internal.h
> @@ -48,9 +48,11 @@ struct cachefiles_volume {
>  enum cachefiles_object_state {
>  	CACHEFILES_ONDEMAND_OBJSTATE_close, /* Anonymous fd closed by daemon or initial state */
>  	CACHEFILES_ONDEMAND_OBJSTATE_open, /* Anonymous fd associated with object is available */
> +	CACHEFILES_ONDEMAND_OBJSTATE_reopening, /* Object that was closed and is being reopened. */
>  };
>  
>  struct cachefiles_ondemand_info {
> +	struct work_struct		work;
>  	int				ondemand_id;
>  	enum cachefiles_object_state	state;
>  	struct cachefiles_object	*object;
> @@ -341,6 +343,7 @@ cachefiles_ondemand_set_object_##_state(struct cachefiles_object *object) \
>  
>  CACHEFILES_OBJECT_STATE_FUNCS(open);
>  CACHEFILES_OBJECT_STATE_FUNCS(close);
> +CACHEFILES_OBJECT_STATE_FUNCS(reopening);
>  #else
>  #define CACHEFILES_ONDEMAND_OBJINFO(object)	NULL
>  
> diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
> index f51266554e4d..79ffb19380cd 100644
> --- a/fs/cachefiles/ondemand.c
> +++ b/fs/cachefiles/ondemand.c
> @@ -18,14 +18,10 @@ static int cachefiles_ondemand_fd_release(struct inode *inode,
>  	info->ondemand_id = CACHEFILES_ONDEMAND_ID_CLOSED;
>  	cachefiles_ondemand_set_object_close(object);
>  
> -	/*
> -	 * Flush all pending READ requests since their completion depends on
> -	 * anon_fd.
> -	 */
> -	xas_for_each(&xas, req, ULONG_MAX) {
> +	/* Only flush CACHEFILES_REQ_NEW marked req to avoid race with daemon_read */
> +	xas_for_each_marked(&xas, req, ULONG_MAX, CACHEFILES_REQ_NEW) {

Could you please add a more detailed comment here, explaing why flushing
CLOSE requests when anony fd gets closed is needed, and why the original
xas_for_each() would race with daemon_read()? There are some refs at [1]
and [2].

[1]
https://hackmd.io/YNsTQqLcQYOZ4gAlFWrNcA#flush-CLOSE-requests-when-anon-fd-is-closed
[2]
https://hackmd.io/YNsTQqLcQYOZ4gAlFWrNcA#race-between-readingflush-requests

The sequence chart is welcome to be added into the comment to explain
the race, or the code will be difficult to understand since the subtlety
of the race.


>  		if (req->msg.object_id == object_id &&
> -		    req->msg.opcode == CACHEFILES_OP_READ) {
> -			req->error = -EIO;
> +		    req->msg.opcode == CACHEFILES_OP_CLOSE) {
>  			complete(&req->done);
>  			xas_store(&xas, NULL);
>  		}
> @@ -175,6 +171,7 @@ int cachefiles_ondemand_copen(struct cachefiles_cache *cache, char *args)
>  	trace_cachefiles_ondemand_copen(req->object, id, size);
>  
>  	cachefiles_ondemand_set_object_open(req->object);
> +	wake_up_all(&cache->daemon_pollwq);
>  
>  out:
>  	complete(&req->done);
> @@ -234,6 +231,14 @@ static int cachefiles_ondemand_get_fd(struct cachefiles_req *req)
>  	return ret;
>  }
>  
> +static void ondemand_object_worker(struct work_struct *work)
> +{
> +	struct cachefiles_object *object =
> +		((struct cachefiles_ondemand_info *)work)->object;
> +
> +	cachefiles_ondemand_init_object(object);
> +}
> +
>  ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
>  					char __user *_buffer, size_t buflen)
>  {
> @@ -249,7 +254,27 @@ ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
>  	 * requests from being processed repeatedly.
>  	 */
>  	xa_lock(&cache->reqs);
> -	req = xas_find_marked(&xas, UINT_MAX, CACHEFILES_REQ_NEW);
> +	xas_for_each_marked(&xas, req, UINT_MAX, CACHEFILES_REQ_NEW) {
> +		/*
> +		 * Reopen the closed object with associated read request.
> +		 * Skip read requests whose related object are reopening.
> +		 */
> +		if (req->msg.opcode == CACHEFILES_OP_READ) {
> +			ret = cmpxchg(&CACHEFILES_ONDEMAND_OBJINFO(req->object)->state,
> +						  CACHEFILES_ONDEMAND_OBJSTATE_close,
> +						  CACHEFILES_ONDEMAND_OBJSTATE_reopening);
> +			if (ret == CACHEFILES_ONDEMAND_OBJSTATE_close) {
> +				INIT_WORK(&CACHEFILES_ONDEMAND_OBJINFO(req->object)->work,
> +						ondemand_object_worker);

How about initializing @work in cachefiles_ondemand_init_obj_info(), so
that the work_struct of each object only needs to be initialized once?


> +				queue_work(fscache_wq,
> +					&CACHEFILES_ONDEMAND_OBJINFO(req->object)->work);
> +				continue;
> +			} else if (ret == CACHEFILES_ONDEMAND_OBJSTATE_reopening) {
> +				continue;
> +			}
> +		}
> +		break;
> +	}
>  	if (!req) {
>  		xa_unlock(&cache->reqs);
>  		return 0;
> @@ -267,14 +292,18 @@ ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
>  	xa_unlock(&cache->reqs);
>  
>  	id = xas.xa_index;
> -	msg->msg_id = id;
>  
>  	if (msg->opcode == CACHEFILES_OP_OPEN) {
>  		ret = cachefiles_ondemand_get_fd(req);
> -		if (ret)
> +		if (ret) {
> +			cachefiles_ondemand_set_object_close(req->object);
>  			goto error;
> +		}
>  	}
>  
> +	msg->msg_id = id;
> +	msg->object_id = CACHEFILES_ONDEMAND_OBJINFO(req->object)->ondemand_id;
> +
>  	if (copy_to_user(_buffer, msg, n) != 0) {
>  		ret = -EFAULT;
>  		goto err_put_fd;
> @@ -307,19 +336,23 @@ static int cachefiles_ondemand_send_req(struct cachefiles_object *object,
>  					void *private)
>  {
>  	struct cachefiles_cache *cache = object->volume->cache;
> -	struct cachefiles_req *req;
> +	struct cachefiles_req *req = NULL;
>  	XA_STATE(xas, &cache->reqs, 0);
>  	int ret;
>  
>  	if (!test_bit(CACHEFILES_ONDEMAND_MODE, &cache->flags))
>  		return 0;
>  
> -	if (test_bit(CACHEFILES_DEAD, &cache->flags))
> -		return -EIO;
> +	if (test_bit(CACHEFILES_DEAD, &cache->flags)) {
> +		ret = -EIO;
> +		goto out;
> +	}
>  
>  	req = kzalloc(sizeof(*req) + data_len, GFP_KERNEL);
> -	if (!req)
> -		return -ENOMEM;
> +	if (!req) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
>  
>  	req->object = object;
>  	init_completion(&req->done);
> @@ -357,7 +390,7 @@ static int cachefiles_ondemand_send_req(struct cachefiles_object *object,
>  		/* coupled with the barrier in cachefiles_flush_reqs() */
>  		smp_mb();
>  
> -		if (opcode != CACHEFILES_OP_OPEN &&
> +		if (opcode == CACHEFILES_OP_CLOSE &&
>  			!cachefiles_ondemand_object_is_open(object)) {
>  			WARN_ON_ONCE(CACHEFILES_ONDEMAND_OBJINFO(object)->ondemand_id == 0);
>  			xas_unlock(&xas);
> @@ -382,8 +415,12 @@ static int cachefiles_ondemand_send_req(struct cachefiles_object *object,
>  	wake_up_all(&cache->daemon_pollwq);
>  	wait_for_completion(&req->done);
>  	ret = req->error;
> +	kfree(req);
> +	return ret;
>  out:
>  	kfree(req);
> +	if (opcode == CACHEFILES_OP_OPEN)
> +		cachefiles_ondemand_set_object_close(req->object);

Could you please add a comment here explaining why we need to set the
object state back to CLOSE state for OPEN (espectially reopening)
requests when error occured, and why we only set it back to CLOSE state
when error occured before the anony fd gets initialized? (That's because
when the error occures after the anony fd has been initialized, the
object will be reset to CLOSE state through
cachefiles_ondemand_fd_release() triggered by close_fd().) Or the code
is quite difficult to comprehend.


>  	return ret;
>  }
>  
> @@ -435,7 +472,6 @@ static int cachefiles_ondemand_init_close_req(struct cachefiles_req *req,
>  	if (!cachefiles_ondemand_object_is_open(object))
>  		return -ENOENT;
>  
> -	req->msg.object_id = CACHEFILES_ONDEMAND_OBJINFO(object)->ondemand_id;
>  	trace_cachefiles_ondemand_close(object, &req->msg);
>  	return 0;
>  }
> @@ -451,16 +487,7 @@ static int cachefiles_ondemand_init_read_req(struct cachefiles_req *req,
>  	struct cachefiles_object *object = req->object;
>  	struct cachefiles_read *load = (void *)req->msg.data;
>  	struct cachefiles_read_ctx *read_ctx = private;
> -	int object_id = CACHEFILES_ONDEMAND_OBJINFO(object)->ondemand_id;
>  
> -	/* Stop enqueuing requests when daemon has closed anon_fd. */
> -	if (!cachefiles_ondemand_object_is_open(object)) {
> -		WARN_ON_ONCE(object_id == 0);
> -		pr_info_once("READ: anonymous fd closed prematurely.\n");
> -		return -EIO;
> -	}
> -
> -	req->msg.object_id = object_id;
>  	load->off = read_ctx->off;
>  	load->len = read_ctx->len;
>  	trace_cachefiles_ondemand_read(object, &req->msg, load);

-- 
Thanks,
Jingbo

Re: [External] Re: [RFC PATCH 3/5] cachefiles: resend an open request if the read request's object is closed

[Jia Zhu]

在 2022/10/8 17:05, JeffleXu 写道:
> 
> 
> On 8/18/22 9:52 PM, Jia Zhu wrote:
>> When an anonymous fd is closed by user daemon, if there is a new read
>> request for this file comes up, the anonymous fd should be re-opened
>> to handle that read request rather than fail it directly.
>>
>> 1. Introduce reopening state for objects that are closed but have
>>     inflight/subsequent read requests.
>> 2. No longer flush READ requests but only CLOSE requests when anonymous
>>     fd is closed.
>> 3. Enqueue the reopen work to workqueue, thus user daemon could get rid
>>     of daemon_read context and handle that request smoothly. Otherwise,
>>     the user daemon will send a reopen request and wait for itself to
>>     process the request.
>>
>> Signed-off-by: Jia Zhu <zhujia.zj@bytedance.com>
>> Reviewed-by: Xin Yin <yinxin.x@bytedance.com>
>> ---
>>   fs/cachefiles/internal.h |  3 ++
>>   fs/cachefiles/ondemand.c | 79 +++++++++++++++++++++++++++-------------
>>   2 files changed, 56 insertions(+), 26 deletions(-)
>>
>> diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
>> index cdf4ec781933..66bbd4f1d22a 100644
>> --- a/fs/cachefiles/internal.h
>> +++ b/fs/cachefiles/internal.h
>> @@ -48,9 +48,11 @@ struct cachefiles_volume {
>>   enum cachefiles_object_state {
>>   	CACHEFILES_ONDEMAND_OBJSTATE_close, /* Anonymous fd closed by daemon or initial state */
>>   	CACHEFILES_ONDEMAND_OBJSTATE_open, /* Anonymous fd associated with object is available */
>> +	CACHEFILES_ONDEMAND_OBJSTATE_reopening, /* Object that was closed and is being reopened. */
>>   };
>>   
>>   struct cachefiles_ondemand_info {
>> +	struct work_struct		work;
>>   	int				ondemand_id;
>>   	enum cachefiles_object_state	state;
>>   	struct cachefiles_object	*object;
>> @@ -341,6 +343,7 @@ cachefiles_ondemand_set_object_##_state(struct cachefiles_object *object) \
>>   
>>   CACHEFILES_OBJECT_STATE_FUNCS(open);
>>   CACHEFILES_OBJECT_STATE_FUNCS(close);
>> +CACHEFILES_OBJECT_STATE_FUNCS(reopening);
>>   #else
>>   #define CACHEFILES_ONDEMAND_OBJINFO(object)	NULL
>>   
>> diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
>> index f51266554e4d..79ffb19380cd 100644
>> --- a/fs/cachefiles/ondemand.c
>> +++ b/fs/cachefiles/ondemand.c
>> @@ -18,14 +18,10 @@ static int cachefiles_ondemand_fd_release(struct inode *inode,
>>   	info->ondemand_id = CACHEFILES_ONDEMAND_ID_CLOSED;
>>   	cachefiles_ondemand_set_object_close(object);
>>   
>> -	/*
>> -	 * Flush all pending READ requests since their completion depends on
>> -	 * anon_fd.
>> -	 */
>> -	xas_for_each(&xas, req, ULONG_MAX) {
>> +	/* Only flush CACHEFILES_REQ_NEW marked req to avoid race with daemon_read */
>> +	xas_for_each_marked(&xas, req, ULONG_MAX, CACHEFILES_REQ_NEW) {
> 
> Could you please add a more detailed comment here, explaing why flushing
> CLOSE requests when anony fd gets closed is needed, and why the original
> xas_for_each() would race with daemon_read()? There are some refs at [1]
> and [2].
> 
> [1]
> https://hackmd.io/YNsTQqLcQYOZ4gAlFWrNcA#flush-CLOSE-requests-when-anon-fd-is-closed
> [2]
> https://hackmd.io/YNsTQqLcQYOZ4gAlFWrNcA#race-between-readingflush-requests
> 
> The sequence chart is welcome to be added into the comment to explain
> the race, or the code will be difficult to understand since the subtlety
> of the race.
OK, I'd like to do that.
> 
> 
>>   		if (req->msg.object_id == object_id &&
>> -		    req->msg.opcode == CACHEFILES_OP_READ) {
>> -			req->error = -EIO;
>> +		    req->msg.opcode == CACHEFILES_OP_CLOSE) {
>>   			complete(&req->done);
>>   			xas_store(&xas, NULL);
>>   		}
>> @@ -175,6 +171,7 @@ int cachefiles_ondemand_copen(struct cachefiles_cache *cache, char *args)
>>   	trace_cachefiles_ondemand_copen(req->object, id, size);
>>   
>>   	cachefiles_ondemand_set_object_open(req->object);
>> +	wake_up_all(&cache->daemon_pollwq);
>>   
>>   out:
>>   	complete(&req->done);
>> @@ -234,6 +231,14 @@ static int cachefiles_ondemand_get_fd(struct cachefiles_req *req)
>>   	return ret;
>>   }
>>   
>> +static void ondemand_object_worker(struct work_struct *work)
>> +{
>> +	struct cachefiles_object *object =
>> +		((struct cachefiles_ondemand_info *)work)->object;
>> +
>> +	cachefiles_ondemand_init_object(object);
>> +}
>> +
>>   ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
>>   					char __user *_buffer, size_t buflen)
>>   {
>> @@ -249,7 +254,27 @@ ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
>>   	 * requests from being processed repeatedly.
>>   	 */
>>   	xa_lock(&cache->reqs);
>> -	req = xas_find_marked(&xas, UINT_MAX, CACHEFILES_REQ_NEW);
>> +	xas_for_each_marked(&xas, req, UINT_MAX, CACHEFILES_REQ_NEW) {
>> +		/*
>> +		 * Reopen the closed object with associated read request.
>> +		 * Skip read requests whose related object are reopening.
>> +		 */
>> +		if (req->msg.opcode == CACHEFILES_OP_READ) {
>> +			ret = cmpxchg(&CACHEFILES_ONDEMAND_OBJINFO(req->object)->state,
>> +						  CACHEFILES_ONDEMAND_OBJSTATE_close,
>> +						  CACHEFILES_ONDEMAND_OBJSTATE_reopening);
>> +			if (ret == CACHEFILES_ONDEMAND_OBJSTATE_close) {
>> +				INIT_WORK(&CACHEFILES_ONDEMAND_OBJINFO(req->object)->work,
>> +						ondemand_object_worker);
> 
> How about initializing @work in cachefiles_ondemand_init_obj_info(), so
> that the work_struct of each object only needs to be initialized once?
> 
SGTM.
> 
>> +				queue_work(fscache_wq,
>> +					&CACHEFILES_ONDEMAND_OBJINFO(req->object)->work);
>> +				continue;
>> +			} else if (ret == CACHEFILES_ONDEMAND_OBJSTATE_reopening) {
>> +				continue;
>> +			}
>> +		}
>> +		break;
>> +	}
>>   	if (!req) {
>>   		xa_unlock(&cache->reqs);
>>   		return 0;
>> @@ -267,14 +292,18 @@ ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
>>   	xa_unlock(&cache->reqs);
>>   
>>   	id = xas.xa_index;
>> -	msg->msg_id = id;
>>   
>>   	if (msg->opcode == CACHEFILES_OP_OPEN) {
>>   		ret = cachefiles_ondemand_get_fd(req);
>> -		if (ret)
>> +		if (ret) {
>> +			cachefiles_ondemand_set_object_close(req->object);
>>   			goto error;
>> +		}
>>   	}
>>   
>> +	msg->msg_id = id;
>> +	msg->object_id = CACHEFILES_ONDEMAND_OBJINFO(req->object)->ondemand_id;
>> +
>>   	if (copy_to_user(_buffer, msg, n) != 0) {
>>   		ret = -EFAULT;
>>   		goto err_put_fd;
>> @@ -307,19 +336,23 @@ static int cachefiles_ondemand_send_req(struct cachefiles_object *object,
>>   					void *private)
>>   {
>>   	struct cachefiles_cache *cache = object->volume->cache;
>> -	struct cachefiles_req *req;
>> +	struct cachefiles_req *req = NULL;
>>   	XA_STATE(xas, &cache->reqs, 0);
>>   	int ret;
>>   
>>   	if (!test_bit(CACHEFILES_ONDEMAND_MODE, &cache->flags))
>>   		return 0;
>>   
>> -	if (test_bit(CACHEFILES_DEAD, &cache->flags))
>> -		return -EIO;
>> +	if (test_bit(CACHEFILES_DEAD, &cache->flags)) {
>> +		ret = -EIO;
>> +		goto out;
>> +	}
>>   
>>   	req = kzalloc(sizeof(*req) + data_len, GFP_KERNEL);
>> -	if (!req)
>> -		return -ENOMEM;
>> +	if (!req) {
>> +		ret = -ENOMEM;
>> +		goto out;
>> +	}
>>   
>>   	req->object = object;
>>   	init_completion(&req->done);
>> @@ -357,7 +390,7 @@ static int cachefiles_ondemand_send_req(struct cachefiles_object *object,
>>   		/* coupled with the barrier in cachefiles_flush_reqs() */
>>   		smp_mb();
>>   
>> -		if (opcode != CACHEFILES_OP_OPEN &&
>> +		if (opcode == CACHEFILES_OP_CLOSE &&
>>   			!cachefiles_ondemand_object_is_open(object)) {
>>   			WARN_ON_ONCE(CACHEFILES_ONDEMAND_OBJINFO(object)->ondemand_id == 0);
>>   			xas_unlock(&xas);
>> @@ -382,8 +415,12 @@ static int cachefiles_ondemand_send_req(struct cachefiles_object *object,
>>   	wake_up_all(&cache->daemon_pollwq);
>>   	wait_for_completion(&req->done);
>>   	ret = req->error;
>> +	kfree(req);
>> +	return ret;
>>   out:
>>   	kfree(req);
>> +	if (opcode == CACHEFILES_OP_OPEN)
>> +		cachefiles_ondemand_set_object_close(req->object);
> 
> Could you please add a comment here explaining why we need to set the
> object state back to CLOSE state for OPEN (espectially reopening)
> requests when error occured, and why we only set it back to CLOSE state
> when error occured before the anony fd gets initialized? (That's because
> when the error occures after the anony fd has been initialized, the
> object will be reset to CLOSE state through
> cachefiles_ondemand_fd_release() triggered by close_fd().) Or the code
> is quite difficult to comprehend.
> 
Thanks for the suggestion. I'll do it.
> 
>>   	return ret;
>>   }
>>   
>> @@ -435,7 +472,6 @@ static int cachefiles_ondemand_init_close_req(struct cachefiles_req *req,
>>   	if (!cachefiles_ondemand_object_is_open(object))
>>   		return -ENOENT;
>>   
>> -	req->msg.object_id = CACHEFILES_ONDEMAND_OBJINFO(object)->ondemand_id;
>>   	trace_cachefiles_ondemand_close(object, &req->msg);
>>   	return 0;
>>   }
>> @@ -451,16 +487,7 @@ static int cachefiles_ondemand_init_read_req(struct cachefiles_req *req,
>>   	struct cachefiles_object *object = req->object;
>>   	struct cachefiles_read *load = (void *)req->msg.data;
>>   	struct cachefiles_read_ctx *read_ctx = private;
>> -	int object_id = CACHEFILES_ONDEMAND_OBJINFO(object)->ondemand_id;
>>   
>> -	/* Stop enqueuing requests when daemon has closed anon_fd. */
>> -	if (!cachefiles_ondemand_object_is_open(object)) {
>> -		WARN_ON_ONCE(object_id == 0);
>> -		pr_info_once("READ: anonymous fd closed prematurely.\n");
>> -		return -EIO;
>> -	}
>> -
>> -	req->msg.object_id = object_id;
>>   	load->off = read_ctx->off;
>>   	load->len = read_ctx->len;
>>   	trace_cachefiles_ondemand_read(object, &req->msg, load);
> 

[RFC PATCH 4/5] cachefiles: narrow the scope of triggering EPOLLIN events in ondemand mode

[Jia Zhu]

Don't trigger EPOLLIN when there are only reopening read requests in
xarray.

Suggested-by: Xin Yin <yinxin.x@bytedance.com>
Signed-off-by: Jia Zhu <zhujia.zj@bytedance.com>
---
 fs/cachefiles/daemon.c   | 13 +++++++++++--
 fs/cachefiles/internal.h | 12 ++++++++++++
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/fs/cachefiles/daemon.c b/fs/cachefiles/daemon.c
index aa4efcabb5e3..c74bd1f4ecf5 100644
--- a/fs/cachefiles/daemon.c
+++ b/fs/cachefiles/daemon.c
@@ -355,14 +355,23 @@ static __poll_t cachefiles_daemon_poll(struct file *file,
 					   struct poll_table_struct *poll)
 {
 	struct cachefiles_cache *cache = file->private_data;
+	struct xarray *xa = &cache->reqs;
+	struct cachefiles_req *req;
+	unsigned long index;
 	__poll_t mask;
 
 	poll_wait(file, &cache->daemon_pollwq, poll);
 	mask = 0;
 
 	if (cachefiles_in_ondemand_mode(cache)) {
-		if (!xa_empty(&cache->reqs))
-			mask |= EPOLLIN;
+		if (!xa_empty(xa)) {
+			xa_for_each_marked(xa, index, req, CACHEFILES_REQ_NEW) {
+				if (!cachefiles_ondemand_is_reopening_read(req)) {
+					mask |= EPOLLIN;
+					break;
+				}
+			}
+		}
 	} else {
 		if (test_bit(CACHEFILES_STATE_CHANGED, &cache->flags))
 			mask |= EPOLLIN;
diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
index 66bbd4f1d22a..b4af67f1cbd6 100644
--- a/fs/cachefiles/internal.h
+++ b/fs/cachefiles/internal.h
@@ -344,6 +344,13 @@ cachefiles_ondemand_set_object_##_state(struct cachefiles_object *object) \
 CACHEFILES_OBJECT_STATE_FUNCS(open);
 CACHEFILES_OBJECT_STATE_FUNCS(close);
 CACHEFILES_OBJECT_STATE_FUNCS(reopening);
+
+static inline bool cachefiles_ondemand_is_reopening_read(struct cachefiles_req *req)
+{
+	return cachefiles_ondemand_object_is_reopening(req->object) &&
+			req->msg.opcode == CACHEFILES_OP_READ;
+}
+
 #else
 #define CACHEFILES_ONDEMAND_OBJINFO(object)	NULL
 
@@ -373,6 +380,11 @@ static inline int cachefiles_ondemand_init_obj_info(struct cachefiles_object *ob
 {
 	return 0;
 }
+
+static inline bool cachefiles_ondemand_is_reopening_read(struct cachefiles_req *req)
+{
+	return false;
+}
 #endif
 
 /*
-- 
2.20.1

[RFC PATCH 5/5] cachefiles: add restore command to recover inflight ondemand read requests

[Jia Zhu]

Previously, in ondemand read scenario, if the anonymous fd was closed by
user daemon, inflight and subsequent read requests would return EIO.
As long as the device connection is not released, user daemon can hold
and restore inflight requests by setting the request flag to
CACHEFILES_REQ_NEW.

Suggested-by: Gao Xiang <hsiangkao@linux.alibaba.com>
Signed-off-by: Jia Zhu <zhujia.zj@bytedance.com>
Signed-off-by: Xin Yin <yinxin.x@bytedance.com>
---
 fs/cachefiles/daemon.c   |  1 +
 fs/cachefiles/internal.h |  3 +++
 fs/cachefiles/ondemand.c | 23 +++++++++++++++++++++++
 3 files changed, 27 insertions(+)

diff --git a/fs/cachefiles/daemon.c b/fs/cachefiles/daemon.c
index c74bd1f4ecf5..014369266cb2 100644
--- a/fs/cachefiles/daemon.c
+++ b/fs/cachefiles/daemon.c
@@ -77,6 +77,7 @@ static const struct cachefiles_daemon_cmd cachefiles_daemon_cmds[] = {
 	{ "tag",	cachefiles_daemon_tag		},
 #ifdef CONFIG_CACHEFILES_ONDEMAND
 	{ "copen",	cachefiles_ondemand_copen	},
+	{ "restore",	cachefiles_ondemand_restore	},
 #endif
 	{ "",		NULL				}
 };
diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
index b4af67f1cbd6..d504c61a5f03 100644
--- a/fs/cachefiles/internal.h
+++ b/fs/cachefiles/internal.h
@@ -303,6 +303,9 @@ extern ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
 extern int cachefiles_ondemand_copen(struct cachefiles_cache *cache,
 				     char *args);
 
+extern int cachefiles_ondemand_restore(struct cachefiles_cache *cache,
+					char *args);
+
 extern int cachefiles_ondemand_init_object(struct cachefiles_object *object);
 extern void cachefiles_ondemand_clean_object(struct cachefiles_object *object);
 
diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
index 79ffb19380cd..5b1c447da976 100644
--- a/fs/cachefiles/ondemand.c
+++ b/fs/cachefiles/ondemand.c
@@ -178,6 +178,29 @@ int cachefiles_ondemand_copen(struct cachefiles_cache *cache, char *args)
 	return ret;
 }
 
+int cachefiles_ondemand_restore(struct cachefiles_cache *cache, char *args)
+{
+	struct cachefiles_req *req;
+
+	XA_STATE(xas, &cache->reqs, 0);
+
+	if (!test_bit(CACHEFILES_ONDEMAND_MODE, &cache->flags))
+		return -EOPNOTSUPP;
+
+	/*
+	 * Search the requests which being processed before
+	 * the user daemon crashed.
+	 * Set the CACHEFILES_REQ_NEW flag and user daemon will reprocess it.
+	 */
+	xas_lock(&xas);
+	xas_for_each(&xas, req, ULONG_MAX)
+		xas_set_mark(&xas, CACHEFILES_REQ_NEW);
+	xas_unlock(&xas);
+
+	wake_up_all(&cache->daemon_pollwq);
+	return 0;
+}
+
 static int cachefiles_ondemand_get_fd(struct cachefiles_req *req)
 {
 	struct cachefiles_object *object;
-- 
2.20.1

Re: [RFC PATCH 5/5] cachefiles: add restore command to recover inflight ondemand read requests

[JeffleXu]

On 8/18/22 9:52 PM, Jia Zhu wrote:
> Previously, in ondemand read scenario, if the anonymous fd was closed by
> user daemon, inflight and subsequent read requests would return EIO.
> As long as the device connection is not released, user daemon can hold
> and restore inflight requests by setting the request flag to
> CACHEFILES_REQ_NEW.
> 
> Suggested-by: Gao Xiang <hsiangkao@linux.alibaba.com>
> Signed-off-by: Jia Zhu <zhujia.zj@bytedance.com>
> Signed-off-by: Xin Yin <yinxin.x@bytedance.com>
> ---
>  fs/cachefiles/daemon.c   |  1 +
>  fs/cachefiles/internal.h |  3 +++
>  fs/cachefiles/ondemand.c | 23 +++++++++++++++++++++++
>  3 files changed, 27 insertions(+)
> 
> diff --git a/fs/cachefiles/daemon.c b/fs/cachefiles/daemon.c
> index c74bd1f4ecf5..014369266cb2 100644
> --- a/fs/cachefiles/daemon.c
> +++ b/fs/cachefiles/daemon.c
> @@ -77,6 +77,7 @@ static const struct cachefiles_daemon_cmd cachefiles_daemon_cmds[] = {
>  	{ "tag",	cachefiles_daemon_tag		},
>  #ifdef CONFIG_CACHEFILES_ONDEMAND
>  	{ "copen",	cachefiles_ondemand_copen	},
> +	{ "restore",	cachefiles_ondemand_restore	},
>  #endif
>  	{ "",		NULL				}
>  };
> diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
> index b4af67f1cbd6..d504c61a5f03 100644
> --- a/fs/cachefiles/internal.h
> +++ b/fs/cachefiles/internal.h
> @@ -303,6 +303,9 @@ extern ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
>  extern int cachefiles_ondemand_copen(struct cachefiles_cache *cache,
>  				     char *args);
>  
> +extern int cachefiles_ondemand_restore(struct cachefiles_cache *cache,
> +					char *args);
> +
>  extern int cachefiles_ondemand_init_object(struct cachefiles_object *object);
>  extern void cachefiles_ondemand_clean_object(struct cachefiles_object *object);
>  
> diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
> index 79ffb19380cd..5b1c447da976 100644
> --- a/fs/cachefiles/ondemand.c
> +++ b/fs/cachefiles/ondemand.c
> @@ -178,6 +178,29 @@ int cachefiles_ondemand_copen(struct cachefiles_cache *cache, char *args)
>  	return ret;
>  }
>  
> +int cachefiles_ondemand_restore(struct cachefiles_cache *cache, char *args)
> +{
> +	struct cachefiles_req *req;
> +
> +	XA_STATE(xas, &cache->reqs, 0);
> +
> +	if (!test_bit(CACHEFILES_ONDEMAND_MODE, &cache->flags))
> +		return -EOPNOTSUPP;
> +
> +	/*
> +	 * Search the requests which being processed before
> +	 * the user daemon crashed.
> +	 * Set the CACHEFILES_REQ_NEW flag and user daemon will reprocess it.
> +	 */

The comment can be improved as:

	Reset the requests to CACHEFILES_REQ_NEW state, so that the
        requests have been processed halfway before the crash of the
        user daemon could be reprocessed after the recovery.


> +	xas_lock(&xas);
> +	xas_for_each(&xas, req, ULONG_MAX)
> +		xas_set_mark(&xas, CACHEFILES_REQ_NEW);
> +	xas_unlock(&xas);
> +
> +	wake_up_all(&cache->daemon_pollwq);
> +	return 0;
> +}
> +
>  static int cachefiles_ondemand_get_fd(struct cachefiles_req *req)
>  {
>  	struct cachefiles_object *object;

-- 
Thanks,
Jingbo

Re: [External] Re: [RFC PATCH 5/5] cachefiles: add restore command to recover inflight ondemand read requests

[Jia Zhu]

在 2022/10/8 17:00, JeffleXu 写道:
> 
> 
> On 8/18/22 9:52 PM, Jia Zhu wrote:
>> Previously, in ondemand read scenario, if the anonymous fd was closed by
>> user daemon, inflight and subsequent read requests would return EIO.
>> As long as the device connection is not released, user daemon can hold
>> and restore inflight requests by setting the request flag to
>> CACHEFILES_REQ_NEW.
>>
>> Suggested-by: Gao Xiang <hsiangkao@linux.alibaba.com>
>> Signed-off-by: Jia Zhu <zhujia.zj@bytedance.com>
>> Signed-off-by: Xin Yin <yinxin.x@bytedance.com>
>> ---
>>   fs/cachefiles/daemon.c   |  1 +
>>   fs/cachefiles/internal.h |  3 +++
>>   fs/cachefiles/ondemand.c | 23 +++++++++++++++++++++++
>>   3 files changed, 27 insertions(+)
>>
>> diff --git a/fs/cachefiles/daemon.c b/fs/cachefiles/daemon.c
>> index c74bd1f4ecf5..014369266cb2 100644
>> --- a/fs/cachefiles/daemon.c
>> +++ b/fs/cachefiles/daemon.c
>> @@ -77,6 +77,7 @@ static const struct cachefiles_daemon_cmd cachefiles_daemon_cmds[] = {
>>   	{ "tag",	cachefiles_daemon_tag		},
>>   #ifdef CONFIG_CACHEFILES_ONDEMAND
>>   	{ "copen",	cachefiles_ondemand_copen	},
>> +	{ "restore",	cachefiles_ondemand_restore	},
>>   #endif
>>   	{ "",		NULL				}
>>   };
>> diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
>> index b4af67f1cbd6..d504c61a5f03 100644
>> --- a/fs/cachefiles/internal.h
>> +++ b/fs/cachefiles/internal.h
>> @@ -303,6 +303,9 @@ extern ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
>>   extern int cachefiles_ondemand_copen(struct cachefiles_cache *cache,
>>   				     char *args);
>>   
>> +extern int cachefiles_ondemand_restore(struct cachefiles_cache *cache,
>> +					char *args);
>> +
>>   extern int cachefiles_ondemand_init_object(struct cachefiles_object *object);
>>   extern void cachefiles_ondemand_clean_object(struct cachefiles_object *object);
>>   
>> diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
>> index 79ffb19380cd..5b1c447da976 100644
>> --- a/fs/cachefiles/ondemand.c
>> +++ b/fs/cachefiles/ondemand.c
>> @@ -178,6 +178,29 @@ int cachefiles_ondemand_copen(struct cachefiles_cache *cache, char *args)
>>   	return ret;
>>   }
>>   
>> +int cachefiles_ondemand_restore(struct cachefiles_cache *cache, char *args)
>> +{
>> +	struct cachefiles_req *req;
>> +
>> +	XA_STATE(xas, &cache->reqs, 0);
>> +
>> +	if (!test_bit(CACHEFILES_ONDEMAND_MODE, &cache->flags))
>> +		return -EOPNOTSUPP;
>> +
>> +	/*
>> +	 * Search the requests which being processed before
>> +	 * the user daemon crashed.
>> +	 * Set the CACHEFILES_REQ_NEW flag and user daemon will reprocess it.
>> +	 */
> 
> The comment can be improved as:
> 
> 	Reset the requests to CACHEFILES_REQ_NEW state, so that the
>          requests have been processed halfway before the crash of the
>          user daemon could be reprocessed after the recovery.
> 
Thanks, I'll apply it.
> 
>> +	xas_lock(&xas);
>> +	xas_for_each(&xas, req, ULONG_MAX)
>> +		xas_set_mark(&xas, CACHEFILES_REQ_NEW);
>> +	xas_unlock(&xas);
>> +
>> +	wake_up_all(&cache->daemon_pollwq);
>> +	return 0;
>> +}
>> +
>>   static int cachefiles_ondemand_get_fd(struct cachefiles_req *req)
>>   {
>>   	struct cachefiles_object *object;
>