diff mbox series

[v9,03/21] cachefiles: unbind cachefiles gracefully in on-demand mode

Message ID 20220415123614.54024-4-jefflexu@linux.alibaba.com (mailing list archive)
State New, archived
Headers show
Series fscache,erofs: fscache-based on-demand read semantics | expand

Commit Message

Jingbo Xu April 15, 2022, 12:35 p.m. UTC
Add a refcount to avoid the deadlock in on-demand read mode. The
on-demand read mode will pin the corresponding cachefiles object for
each anonymous fd. The cachefiles object is unpinned when the anonymous
fd gets closed. When the user daemon exits and the fd of
"/dev/cachefiles" device node gets closed, it will wait for all
cahcefiles objects gets withdrawn. Then if there's any anonymous fd
getting closed after the fd of the device node, the user daemon will
hang forever, waiting for all objects getting withdrawn.

To fix this, add a refcount indicating if there's any object pinned by
anonymous fds. The cachefiles cache gets unbound and withdrawn when the
refcount decreased to 0. It won't change the behaviour of the original
mode, in which case the cachefiles cache gets unbound and withdrawn as
long as the fd of the device node gets closed. Besides, kref_get() is
adequate whilst kref_get_unless_zero() is not needed here, since no more
anonymous fd will be created when the .release() callback of the device
node fd has already been called.

Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
 fs/cachefiles/daemon.c   | 24 +++++++++++++++++++++---
 fs/cachefiles/internal.h |  3 +++
 fs/cachefiles/ondemand.c |  3 +++
 3 files changed, 27 insertions(+), 3 deletions(-)

Comments

David Howells April 21, 2022, 2:02 p.m. UTC | #1
Jeffle Xu <jefflexu@linux.alibaba.com> wrote:

> +	struct kref			unbind_pincount;/* refcount to do daemon unbind */

Please use refcount_t or atomic_t, especially as this isn't the refcount for
the structure.

> -	cachefiles_daemon_unbind(cache);
> -
>  	/* clean up the control file interface */
>  	cache->cachefilesd = NULL;
>  	file->private_data = NULL;
>  	cachefiles_open = 0;

Please call cachefiles_daemon_unbind() before the cleanup.

David
Jingbo Xu April 22, 2022, 2:44 a.m. UTC | #2
On 4/21/22 10:02 PM, David Howells wrote:
> Jeffle Xu <jefflexu@linux.alibaba.com> wrote:
> 
>> +	struct kref			unbind_pincount;/* refcount to do daemon unbind */
> 
> Please use refcount_t or atomic_t, especially as this isn't the refcount for
> the structure.

Okay, will be done in the next version.

> 
>> -	cachefiles_daemon_unbind(cache);
>> -
>>  	/* clean up the control file interface */
>>  	cache->cachefilesd = NULL;
>>  	file->private_data = NULL;
>>  	cachefiles_open = 0;
> 
> Please call cachefiles_daemon_unbind() before the cleanup.

Since the cachefiles_struct struct will be freed once the pincount is
decreased to 0, "cache->cachefilesd = NULL;" needs to be done before
decreasing the pincount. BTW, "cachefiles_open = 0;" indeed should be
done only when pincount has been decreased to 0.
diff mbox series

Patch

diff --git a/fs/cachefiles/daemon.c b/fs/cachefiles/daemon.c
index 69ca22aa6abf..2e946e4eb65a 100644
--- a/fs/cachefiles/daemon.c
+++ b/fs/cachefiles/daemon.c
@@ -111,6 +111,7 @@  static int cachefiles_daemon_open(struct inode *inode, struct file *file)
 	INIT_LIST_HEAD(&cache->volumes);
 	INIT_LIST_HEAD(&cache->object_list);
 	spin_lock_init(&cache->object_list_lock);
+	kref_init(&cache->unbind_pincount);
 #ifdef CONFIG_CACHEFILES_ONDEMAND
 	xa_init_flags(&cache->reqs, XA_FLAGS_ALLOC);
 	xa_init_flags(&cache->ondemand_ids, XA_FLAGS_ALLOC1);
@@ -157,6 +158,25 @@  static void cachefiles_flush_reqs(struct cachefiles_cache *cache)
 }
 #endif
 
+static void cachefiles_release_cache(struct kref *kref)
+{
+	struct cachefiles_cache *cache;
+
+	cache = container_of(kref, struct cachefiles_cache, unbind_pincount);
+	cachefiles_daemon_unbind(cache);
+	kfree(cache);
+}
+
+void cachefiles_put_unbind_pincount(struct cachefiles_cache *cache)
+{
+	kref_put(&cache->unbind_pincount, cachefiles_release_cache);
+}
+
+void cachefiles_get_unbind_pincount(struct cachefiles_cache *cache)
+{
+	kref_get(&cache->unbind_pincount);
+}
+
 /*
  * Release a cache.
  */
@@ -173,14 +193,12 @@  static int cachefiles_daemon_release(struct inode *inode, struct file *file)
 #ifdef CONFIG_CACHEFILES_ONDEMAND
 	cachefiles_flush_reqs(cache);
 #endif
-	cachefiles_daemon_unbind(cache);
-
 	/* clean up the control file interface */
 	cache->cachefilesd = NULL;
 	file->private_data = NULL;
 	cachefiles_open = 0;
 
-	kfree(cache);
+	cachefiles_put_unbind_pincount(cache);
 
 	_leave("");
 	return 0;
diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
index 8ebe238af20b..9b83d8c82709 100644
--- a/fs/cachefiles/internal.h
+++ b/fs/cachefiles/internal.h
@@ -109,6 +109,7 @@  struct cachefiles_cache {
 	char				*rootdirname;	/* name of cache root directory */
 	char				*secctx;	/* LSM security context */
 	char				*tag;		/* cache binding tag */
+	struct kref			unbind_pincount;/* refcount to do daemon unbind */
 #ifdef CONFIG_CACHEFILES_ONDEMAND
 	struct xarray			reqs;		/* xarray of pending on-demand requests */
 	struct xarray			ondemand_ids;	/* xarray for ondemand_id allocation */
@@ -167,6 +168,8 @@  extern int cachefiles_has_space(struct cachefiles_cache *cache,
  * daemon.c
  */
 extern const struct file_operations cachefiles_daemon_fops;
+extern void cachefiles_get_unbind_pincount(struct cachefiles_cache *cache);
+extern void cachefiles_put_unbind_pincount(struct cachefiles_cache *cache);
 
 /*
  * error_inject.c
diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
index 890cd3ecc2f0..eec883640efa 100644
--- a/fs/cachefiles/ondemand.c
+++ b/fs/cachefiles/ondemand.c
@@ -14,6 +14,7 @@  static int cachefiles_ondemand_fd_release(struct inode *inode,
 	object->ondemand_id = CACHEFILES_ONDEMAND_ID_CLOSED;
 	xa_erase(&cache->ondemand_ids, object_id);
 	cachefiles_put_object(object, cachefiles_obj_put_ondemand_fd);
+	cachefiles_put_unbind_pincount(cache);
 	return 0;
 }
 
@@ -169,6 +170,8 @@  static int cachefiles_ondemand_get_fd(struct cachefiles_req *req)
 	load->fd = fd;
 	req->msg.object_id = object_id;
 	object->ondemand_id = object_id;
+
+	cachefiles_get_unbind_pincount(cache);
 	return 0;
 
 err_put_fd: