diff mbox series

ceph: make the ceph-cap workqueue UNBOUND

Message ID 20240321021536.64693-1-xiubli@redhat.com
State New
Headers show
Series ceph: make the ceph-cap workqueue UNBOUND | expand

Commit Message

Xiubo Li March 21, 2024, 2:15 a.m. UTC 
From: Xiubo Li <xiubli@redhat.com>

There is not harm to mark the ceph-cap workqueue unbounded, just
like we do in ceph-inode workqueue.

URL: https://www.spinics.net/lists/ceph-users/msg78775.html
URL: https://tracker.ceph.com/issues/64977
Reported-by: Stefan Kooman <stefan@bit.nl>
Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
 fs/ceph/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ilya Dryomov May 20, 2024, 7:37 p.m. UTC | #1 
On Thu, Mar 21, 2024 at 3:18 AM <xiubli@redhat.com> wrote:
>
> From: Xiubo Li <xiubli@redhat.com>
>
> There is not harm to mark the ceph-cap workqueue unbounded, just
> like we do in ceph-inode workqueue.
>
> URL: https://www.spinics.net/lists/ceph-users/msg78775.html
> URL: https://tracker.ceph.com/issues/64977
> Reported-by: Stefan Kooman <stefan@bit.nl>
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>  fs/ceph/super.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index 4dcbbaa297f6..0bfe4f8418fd 100644
> --- a/fs/ceph/super.c
> +++ b/fs/ceph/super.c
> @@ -851,7 +851,7 @@ static struct ceph_fs_client *create_fs_client(struct ceph_mount_options *fsopt,
>         fsc->inode_wq = alloc_workqueue("ceph-inode", WQ_UNBOUND, 0);
>         if (!fsc->inode_wq)
>                 goto fail_client;
> -       fsc->cap_wq = alloc_workqueue("ceph-cap", 0, 1);
> +       fsc->cap_wq = alloc_workqueue("ceph-cap", WQ_UNBOUND, 1);

Hi Xiubo,

You wrote that there is no harm in making ceph-cap workqueue unbound,
but, if it's made unbound, it would be almost the same as ceph-inode
workqueue.  The only difference would be that max_active parameter for
ceph-cap workqueue is 1 instead of 0 (i.e. some default which is pretty
high).  Given that max_active is interpreted as a per-CPU number even
for unbound workqueues, up to $NUM_CPUS work items submitted to
ceph-cap workqueue could still be active in a system.

Does CephFS need/rely on $NUM_CPUS limit sowewhere?  If not, how about
removing ceph-cap workqueue and submitting its work items to ceph-inode
workqueue instead?

Thanks,

                Ilya
Ilya Dryomov May 20, 2024, 7:48 p.m. UTC | #2 
On Mon, May 20, 2024 at 9:37 PM Ilya Dryomov <idryomov@gmail.com> wrote:
>
> On Thu, Mar 21, 2024 at 3:18 AM <xiubli@redhat.com> wrote:
> >
> > From: Xiubo Li <xiubli@redhat.com>
> >
> > There is not harm to mark the ceph-cap workqueue unbounded, just
> > like we do in ceph-inode workqueue.
> >
> > URL: https://www.spinics.net/lists/ceph-users/msg78775.html
> > URL: https://tracker.ceph.com/issues/64977
> > Reported-by: Stefan Kooman <stefan@bit.nl>
> > Signed-off-by: Xiubo Li <xiubli@redhat.com>
> > ---
> >  fs/ceph/super.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> > index 4dcbbaa297f6..0bfe4f8418fd 100644
> > --- a/fs/ceph/super.c
> > +++ b/fs/ceph/super.c
> > @@ -851,7 +851,7 @@ static struct ceph_fs_client *create_fs_client(struct ceph_mount_options *fsopt,
> >         fsc->inode_wq = alloc_workqueue("ceph-inode", WQ_UNBOUND, 0);
> >         if (!fsc->inode_wq)
> >                 goto fail_client;
> > -       fsc->cap_wq = alloc_workqueue("ceph-cap", 0, 1);
> > +       fsc->cap_wq = alloc_workqueue("ceph-cap", WQ_UNBOUND, 1);
>
> Hi Xiubo,
>
> You wrote that there is no harm in making ceph-cap workqueue unbound,
> but, if it's made unbound, it would be almost the same as ceph-inode
> workqueue.  The only difference would be that max_active parameter for
> ceph-cap workqueue is 1 instead of 0 (i.e. some default which is pretty
> high).  Given that max_active is interpreted as a per-CPU number even
> for unbound workqueues, up to $NUM_CPUS work items submitted to
> ceph-cap workqueue could still be active in a system.
>
> Does CephFS need/rely on $NUM_CPUS limit sowewhere?  If not, how about
> removing ceph-cap workqueue and submitting its work items to ceph-inode
> workqueue instead?

Related question: why ceph_force_reconnect() flushes only one of these
workqueues (ceph-inode) instead of both?  When invalidating everything,
aren't we concerned with potential stale work items from before the
session is recovered?

Thanks,

                Ilya
Ilya Dryomov May 21, 2024, 7:39 a.m. UTC | #3 
On Tue, May 21, 2024 at 5:37 AM Xiubo Li <xiubli@redhat.com> wrote:
>
>
> On 5/21/24 03:37, Ilya Dryomov wrote:
>
> On Thu, Mar 21, 2024 at 3:18 AM <xiubli@redhat.com> wrote:
>
> From: Xiubo Li <xiubli@redhat.com>
>
> There is not harm to mark the ceph-cap workqueue unbounded, just
> like we do in ceph-inode workqueue.
>
> URL: https://www.spinics.net/lists/ceph-users/msg78775.html
> URL: https://tracker.ceph.com/issues/64977
> Reported-by: Stefan Kooman <stefan@bit.nl>
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>  fs/ceph/super.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index 4dcbbaa297f6..0bfe4f8418fd 100644
> --- a/fs/ceph/super.c
> +++ b/fs/ceph/super.c
> @@ -851,7 +851,7 @@ static struct ceph_fs_client *create_fs_client(struct ceph_mount_options *fsopt,
>         fsc->inode_wq = alloc_workqueue("ceph-inode", WQ_UNBOUND, 0);
>         if (!fsc->inode_wq)
>                 goto fail_client;
> -       fsc->cap_wq = alloc_workqueue("ceph-cap", 0, 1);
> +       fsc->cap_wq = alloc_workqueue("ceph-cap", WQ_UNBOUND, 1);
>
> Hi Xiubo,
>
> You wrote that there is no harm in making ceph-cap workqueue unbound,
> but, if it's made unbound, it would be almost the same as ceph-inode
> workqueue.  The only difference would be that max_active parameter for
> ceph-cap workqueue is 1 instead of 0 (i.e. some default which is pretty
> high).  Given that max_active is interpreted as a per-CPU number even
> for unbound workqueues, up to $NUM_CPUS work items submitted to
> ceph-cap workqueue could still be active in a system.
>
> Does CephFS need/rely on $NUM_CPUS limit sowewhere?  If not, how about
> removing ceph-cap workqueue and submitting its work items to ceph-inode
> workqueue instead?
>
> Checked it again and found that an UNBOUND and max_active==1 combo has a special meaning:
>
> Some users depend on the strict execution ordering of ST wq. The combination of @max_active of 1 and WQ_UNBOUND is used to achieve this behavior. Work items on such wq are always queued to the unbound worker-pools and only one work item can be active at any given time thus achieving the same ordering property as ST wq.

This hasn't been true for 10 years, see commit

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3bc1e711c26bff01d41ad71145ecb8dcb4412576

Thanks,

                Ilya
Xiubo Li May 21, 2024, 7:51 a.m. UTC | #4 
On 5/21/24 15:39, Ilya Dryomov wrote:
> On Tue, May 21, 2024 at 5:37 AM Xiubo Li <xiubli@redhat.com> wrote:
>>
>> On 5/21/24 03:37, Ilya Dryomov wrote:
>>
>> On Thu, Mar 21, 2024 at 3:18 AM <xiubli@redhat.com> wrote:
>>
>> From: Xiubo Li <xiubli@redhat.com>
>>
>> There is not harm to mark the ceph-cap workqueue unbounded, just
>> like we do in ceph-inode workqueue.
>>
>> URL: https://www.spinics.net/lists/ceph-users/msg78775.html
>> URL: https://tracker.ceph.com/issues/64977
>> Reported-by: Stefan Kooman <stefan@bit.nl>
>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>> ---
>>   fs/ceph/super.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
>> index 4dcbbaa297f6..0bfe4f8418fd 100644
>> --- a/fs/ceph/super.c
>> +++ b/fs/ceph/super.c
>> @@ -851,7 +851,7 @@ static struct ceph_fs_client *create_fs_client(struct ceph_mount_options *fsopt,
>>          fsc->inode_wq = alloc_workqueue("ceph-inode", WQ_UNBOUND, 0);
>>          if (!fsc->inode_wq)
>>                  goto fail_client;
>> -       fsc->cap_wq = alloc_workqueue("ceph-cap", 0, 1);
>> +       fsc->cap_wq = alloc_workqueue("ceph-cap", WQ_UNBOUND, 1);
>>
>> Hi Xiubo,
>>
>> You wrote that there is no harm in making ceph-cap workqueue unbound,
>> but, if it's made unbound, it would be almost the same as ceph-inode
>> workqueue.  The only difference would be that max_active parameter for
>> ceph-cap workqueue is 1 instead of 0 (i.e. some default which is pretty
>> high).  Given that max_active is interpreted as a per-CPU number even
>> for unbound workqueues, up to $NUM_CPUS work items submitted to
>> ceph-cap workqueue could still be active in a system.
>>
>> Does CephFS need/rely on $NUM_CPUS limit sowewhere?  If not, how about
>> removing ceph-cap workqueue and submitting its work items to ceph-inode
>> workqueue instead?
>>
>> Checked it again and found that an UNBOUND and max_active==1 combo has a special meaning:
>>
>> Some users depend on the strict execution ordering of ST wq. The combination of @max_active of 1 and WQ_UNBOUND is used to achieve this behavior. Work items on such wq are always queued to the unbound worker-pools and only one work item can be active at any given time thus achieving the same ordering property as ST wq.
> This hasn't been true for 10 years, see commit
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3bc1e711c26bff01d41ad71145ecb8dcb4412576

Okay, I found the doc I read is out of date.

- Xiubo

> Thanks,
>
>                  Ilya
>
diff mbox series

Patch

diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index 4dcbbaa297f6..0bfe4f8418fd 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -851,7 +851,7 @@  static struct ceph_fs_client *create_fs_client(struct ceph_mount_options *fsopt,
 	fsc->inode_wq = alloc_workqueue("ceph-inode", WQ_UNBOUND, 0);
 	if (!fsc->inode_wq)
 		goto fail_client;
-	fsc->cap_wq = alloc_workqueue("ceph-cap", 0, 1);
+	fsc->cap_wq = alloc_workqueue("ceph-cap", WQ_UNBOUND, 1);
 	if (!fsc->cap_wq)
 		goto fail_inode_wq;