simplify writeback thread creation

Currently the per-bdi writeback thread is only created when there is dirty any dirty data on the BDI, and it lazy exists when it's been unused for some time.

This leads to some very complex code, and the need to keep a forker thread around.

This patch removes all this code and simply creates the thread as part of the bdi registration. The downside is that we use up ressoures for possible unused devices, although that overhead is rather low, with 8k kernel stack size on x86 and few other, even smaller ressources.

If the overhead is still considered too much I can look into starting the thread explicitly instead of as part of the bdi registration, but that will require a bit of code complexity, too.

Currently the per-bdi writeback thread is only created when there is dirty any dirty data on the BDI, and it lazy exists when it's been unused for some time.

This leads to some very complex code, and the need to keep a forker thread around.

This patch removes all this code and simply creates the thread as part of the bdi registration. The downside is that we use up ressoures for possible unused devices, although that overhead is rather low, with 8k kernel stack size on x86 and few other, even smaller ressources.

If the overhead is still considered too much I can look into starting the thread explicitly instead of as part of the bdi registration, but that will require a bit of code complexity, too.

Quoted 12 lines · Christoph Hellwig

Currently the per-bdi writeback thread is only created when there is dirty any dirty data on the BDI, and it lazy exists when it's been unused for some time.

This leads to some very complex code, and the need to keep a forker thread around.

This patch removes all this code and simply creates the thread as part of the bdi registration. The downside is that we use up ressoures for possible unused devices, although that overhead is rather low, with 8k kernel stack size on x86 and few other, even smaller ressources.

Quoted 3 lines

If the overhead is still considered too much I can look into starting the thread explicitly instead of as part of the bdi registration, but that will require a bit of code complexity, too.

I'm pretty sure this will come back to bite us in the ass... If we are going to change the lazy create/exit setup, I would greatly prefer doing it at fs mount time (or something to that effect).

Quoted 13 lines · Jens Axboe

Quoted 12 lines · Christoph Hellwig

Currently the per-bdi writeback thread is only created when there is dirty any dirty data on the BDI, and it lazy exists when it's been unused for some time.

This leads to some very complex code, and the need to keep a forker thread around.

This patch removes all this code and simply creates the thread as part of the bdi registration. The downside is that we use up ressoures for possible unused devices, although that overhead is rather low, with 8k kernel stack size on x86 and few other, even smaller ressources.

Quoted 7 lines

Quoted 3 lines

If the overhead is still considered too much I can look into starting the thread explicitly instead of as part of the bdi registration, but that will require a bit of code complexity, too.

I'm pretty sure this will come back to bite us in the ass... If we are going to change the lazy create/exit setup, I would greatly prefer doing it at fs mount time (or something to that effect).

How about not starting any thread at all at the bdi registration time, and start a bdi thread only when something for this bdi becomes dirty (__mark_inode_dirty()) or a bdi work is queued (bdi_queue_work())? If we do this, then the tasks can also die by the 5min timeout, and will be forked again when dirt/bdi works arrives?

I guess it is a bit challenging to start a task in __mark_inode_dirty(), whis is supposed to be fast and non-sleeping, but we can just submit a work which will start the task.

Quoted 6 lines

How about not starting any thread at all at the bdi registration time, and start a bdi thread only when something for this bdi becomes dirty (__mark_inode_dirty()) or a bdi work is queued (bdi_queue_work())? If we do this, then the tasks can also die by the 5min timeout, and will be forked again when dirt/bdi works arrives?

Quoted 3 lines

I guess it is a bit challenging to start a task in __mark_inode_dirty(), whis is supposed to be fast and non-sleeping, but we can just submit a work which will start the task.

That work would have to reside on the stack, and __mark_inode_dirty() block on the thread startup. We can't always do that.

Quoted 10 lines · Jens Axboe

Quoted 9 lines

How about not starting any thread at all at the bdi registration time, and start a bdi thread only when something for this bdi becomes dirty (__mark_inode_dirty()) or a bdi work is queued (bdi_queue_work())? If we do this, then the tasks can also die by the 5min timeout, and will be forked again when dirt/bdi works arrives?

I guess it is a bit challenging to start a task in __mark_inode_dirty(), whis is supposed to be fast and non-sleeping, but we can just submit a work which will start the task.

Quoted 2 lines

That work would have to reside on the stack, and __mark_inode_dirty() block on the thread startup. We can't always do that.

We can have a pre-defined bdi->wb->task_start_work or something like that.

Quoted 11 lines · Artem Bityutskiy

Quoted 10 lines · Jens Axboe

Quoted 9 lines

How about not starting any thread at all at the bdi registration time, and start a bdi thread only when something for this bdi becomes dirty (__mark_inode_dirty()) or a bdi work is queued (bdi_queue_work())? If we do this, then the tasks can also die by the 5min timeout, and will be forked again when dirt/bdi works arrives?

I guess it is a bit challenging to start a task in __mark_inode_dirty(), whis is supposed to be fast and non-sleeping, but we can just submit a work which will start the task.

Quoted 5 lines

Quoted 2 lines

That work would have to reside on the stack, and __mark_inode_dirty() block on the thread startup. We can't always do that.

We can have a pre-defined bdi->wb->task_start_work or something like that.

Yeah, that would work.

Hmm, was thinking about this while driving home - the forker approach has a good resilience property - if it cannot fork - it'll do the stuff itself. I have a feeling that if something like this to be implemented with the approach I suggested, we'll end up with similar level of complexity that we wanted to get rid of...

Quoted 5 lines · Artem Bityutskiy

Hmm, was thinking about this while driving home - the forker approach has a good resilience property - if it cannot fork - it'll do the stuff itself. I have a feeling that if something like this to be implemented with the approach I suggested, we'll end up with similar level of complexity that we wanted to get rid of...

Yes, the lazy starting is what adds the complexity. I think starting it once we have any filesystem mounted on the bdi and stop it once all filesystems are gone is a lot simpler and more elegant.

Quoted 7 lines · Christoph Hellwig

Quoted 5 lines · Artem Bityutskiy

Hmm, was thinking about this while driving home - the forker approach has a good resilience property - if it cannot fork - it'll do the stuff itself. I have a feeling that if something like this to be implemented with the approach I suggested, we'll end up with similar level of complexity that we wanted to get rid of...

Quoted 3 lines

Yes, the lazy starting is what adds the complexity. I think starting it once we have any filesystem mounted on the bdi and stop it once all filesystems are gone is a lot simpler and more elegant.

But what about cases like 'dd if=/dev/zero of=/dev/sda4'? They also involve dirty data write-back.

But what about cases like 'dd if=/dev/zero of=/dev/sda4'? They also involve dirty data write-back.

Quoted 2 lines · Artem Bityutskiy

But what about cases like 'dd if=/dev/zero of=/dev/sda4'? They also involve dirty data write-back.

But only on the bdev fs inode.

(quoted diff) +0 -0

@@ -503,18 +359,10 @@ int bdi_register(struct backing_dev_info
         * and add other bdi's to the list. They will get a thread

created
* on-demand when they need it.
*/

mm/backing-dev.c +3 -10

-       if (bdi_cap_flush_forker(bdi)) {
-               struct bdi_writeback *wb = &bdi->wb;
-
-               wb->task = kthread_run(bdi_forker_task, wb, "bdi-%s",
-                                               dev_name(dev));
-               if (IS_ERR(wb->task)) {
-                       wb->task = NULL;
-                       ret = -ENOMEM;
-
-                       bdi_remove_from_list(bdi);
+       if (bdi_cap_writeback_dirty(bdi)) {
+               ret = bdi_writeback_enable(bdi, dev);
+               if (ret)
                        goto exit;