[dm-devel] [PATCH] dm-crypt: reexport sysfs of kcryptd workqueue

[dm-devel] [PATCH] dm-crypt: reexport sysfs of kcryptd workqueue

[yangerkun]

From: yangerkun <yangerkun@huawei.com>

'a2b8b2d97567 ("dm crypt: export sysfs of kcryptd workqueue")' give us
idea to set specific CPU or limit max_active crypt work. However sysfs
will report a warnning and fail 'cryptsetup refresh test' since the
reload will alloc workqueue with same sysfs name, and we temporarily
revert this feature to fix the problem.

What we actually should do is give a unique name once we try reload
table, we can use ida now.

Signed-off-by: yangerkun <yangerkun@huawei.com>
---
 drivers/md/dm-crypt.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 755a01d72cdb..f0b8cb4364a1 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -46,6 +46,8 @@
 
 #define DM_MSG_PREFIX "crypt"
 
+static DEFINE_IDA(crypt_queue_ida);
+
 /*
  * context holding the current state of a multi-part conversion
  */
@@ -179,6 +181,7 @@ struct crypt_config {
 		struct crypto_aead **tfms_aead;
 	} cipher_tfm;
 	unsigned tfms_count;
+	int crypt_queue_id;
 	unsigned long cipher_flags;
 
 	/*
@@ -2708,6 +2711,9 @@ static void crypt_dtr(struct dm_target *ti)
 	if (cc->crypt_queue)
 		destroy_workqueue(cc->crypt_queue);
 
+	if (cc->crypt_queue_id)
+		ida_free(&crypt_queue_ida, cc->crypt_queue_id);
+
 	crypt_free_tfms(cc);
 
 	bioset_exit(&cc->bs);
@@ -3344,12 +3350,24 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 	}
 
 	if (test_bit(DM_CRYPT_SAME_CPU, &cc->flags))
-		cc->crypt_queue = alloc_workqueue("kcryptd/%s", WQ_CPU_INTENSIVE | WQ_MEM_RECLAIM,
+		cc->crypt_queue = alloc_workqueue("kcryptd-%s", WQ_CPU_INTENSIVE | WQ_MEM_RECLAIM,
 						  1, devname);
-	else
-		cc->crypt_queue = alloc_workqueue("kcryptd/%s",
-						  WQ_CPU_INTENSIVE | WQ_MEM_RECLAIM | WQ_UNBOUND,
-						  num_online_cpus(), devname);
+	else {
+		int id = ida_alloc_min(&crypt_queue_ida, 1, GFP_KERNEL);
+
+		if (id < 0) {
+			ti->error = "Couldn't get kcryptd queue id";
+			ret = id;
+			goto bad;
+		}
+
+		cc->crypt_queue_id = id;
+		cc->crypt_queue = alloc_workqueue("kcryptd-%s-%d",
+						  WQ_CPU_INTENSIVE | WQ_MEM_RECLAIM |
+						  WQ_UNBOUND | WQ_SYSFS,
+						  num_online_cpus(), devname, id);
+	}
+
 	if (!cc->crypt_queue) {
 		ti->error = "Couldn't create kcryptd queue";
 		goto bad;
-- 
2.31.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] dm-crypt: reexport sysfs of kcryptd workqueue

[Mike Snitzer]

On Wed, Feb 22 2023 at 10:18P -0500,
yangerkun <yangerkun@huaweicloud.com> wrote:

> From: yangerkun <yangerkun@huawei.com>
> 
> 'a2b8b2d97567 ("dm crypt: export sysfs of kcryptd workqueue")' give us
> idea to set specific CPU or limit max_active crypt work. However sysfs
> will report a warnning and fail 'cryptsetup refresh test' since the
> reload will alloc workqueue with same sysfs name, and we temporarily
> revert this feature to fix the problem.
> 
> What we actually should do is give a unique name once we try reload
> table, we can use ida now.

This header needs to be rewritten to deal directly with the fact:
1) Commit 48b0777cd93d (Revert "dm crypt: export sysfs of kcryptd
   workqueue") dealt with what you call "temporarily revert".
2) Great that you fixed it using IDA but you still haven't clearly
   explained the benefit of exposing this sysfs interface (please add
   details on which userspace is consuming it). I suppose you could
   explain by referring to commit a2b8b2d975673 (that was reverted)?

Thanks,
Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] dm-crypt: reexport sysfs of kcryptd workqueue

[yangerkun]

在 2023/2/28 4:56, Mike Snitzer 写道:
> On Wed, Feb 22 2023 at 10:18P -0500,
> yangerkun <yangerkun@huaweicloud.com> wrote:
> 
>> From: yangerkun <yangerkun@huawei.com>
>>
>> 'a2b8b2d97567 ("dm crypt: export sysfs of kcryptd workqueue")' give us
>> idea to set specific CPU or limit max_active crypt work. However sysfs
>> will report a warnning and fail 'cryptsetup refresh test' since the
>> reload will alloc workqueue with same sysfs name, and we temporarily
>> revert this feature to fix the problem.
>>
>> What we actually should do is give a unique name once we try reload
>> table, we can use ida now.
> 
> This header needs to be rewritten to deal directly with the fact:
> 1) Commit 48b0777cd93d (Revert "dm crypt: export sysfs of kcryptd
>     workqueue") dealt with what you call "temporarily revert".
> 2) Great that you fixed it using IDA but you still haven't clearly
>     explained the benefit of exposing this sysfs interface (please add
>     details on which userspace is consuming it). I suppose you could
>     explain by referring to commit a2b8b2d975673 (that was reverted)?

Thanks a lot, will do it next version.

> 
> Thanks,
> Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH] dm-crypt: reexport sysfs of kcryptd workqueue

[yangerkun]

'a2b8b2d97567 ("dm crypt: export sysfs of kcryptd workqueue")' give us
idea to set specific CPU or limit max_active crypt work. However sysfs
will report a warnning and fail 'cryptsetup refresh test' since the
reload will alloc workqueue with same sysfs name, and we temporarily
revert this feature to fix the problem.

What we actually should do is give a unique name once we try reload
table, we can use ida now.

Signed-off-by: yangerkun <yangerkun@huawei.com>
---
 drivers/md/dm-crypt.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 755a01d72cdb..f0b8cb4364a1 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -46,6 +46,8 @@
 
 #define DM_MSG_PREFIX "crypt"
 
+static DEFINE_IDA(crypt_queue_ida);
+
 /*
  * context holding the current state of a multi-part conversion
  */
@@ -179,6 +181,7 @@ struct crypt_config {
 		struct crypto_aead **tfms_aead;
 	} cipher_tfm;
 	unsigned tfms_count;
+	int crypt_queue_id;
 	unsigned long cipher_flags;
 
 	/*
@@ -2708,6 +2711,9 @@ static void crypt_dtr(struct dm_target *ti)
 	if (cc->crypt_queue)
 		destroy_workqueue(cc->crypt_queue);
 
+	if (cc->crypt_queue_id)
+		ida_free(&crypt_queue_ida, cc->crypt_queue_id);
+
 	crypt_free_tfms(cc);
 
 	bioset_exit(&cc->bs);
@@ -3344,12 +3350,24 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 	}
 
 	if (test_bit(DM_CRYPT_SAME_CPU, &cc->flags))
-		cc->crypt_queue = alloc_workqueue("kcryptd/%s", WQ_CPU_INTENSIVE | WQ_MEM_RECLAIM,
+		cc->crypt_queue = alloc_workqueue("kcryptd-%s", WQ_CPU_INTENSIVE | WQ_MEM_RECLAIM,
 						  1, devname);
-	else
-		cc->crypt_queue = alloc_workqueue("kcryptd/%s",
-						  WQ_CPU_INTENSIVE | WQ_MEM_RECLAIM | WQ_UNBOUND,
-						  num_online_cpus(), devname);
+	else {
+		int id = ida_alloc_min(&crypt_queue_ida, 1, GFP_KERNEL);
+
+		if (id < 0) {
+			ti->error = "Couldn't get kcryptd queue id";
+			ret = id;
+			goto bad;
+		}
+
+		cc->crypt_queue_id = id;
+		cc->crypt_queue = alloc_workqueue("kcryptd-%s-%d",
+						  WQ_CPU_INTENSIVE | WQ_MEM_RECLAIM |
+						  WQ_UNBOUND | WQ_SYSFS,
+						  num_online_cpus(), devname, id);
+	}
+
 	if (!cc->crypt_queue) {
 		ti->error = "Couldn't create kcryptd queue";
 		goto bad;
-- 
2.31.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel