| Message ID | 20230226160259.18354-1-ammarfaizi2@gnuweeb.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introducing `wq_cpu_set` mount option for btrfs | expand |
On Sun, Feb 26, 2023 at 11:02:53PM +0700, Ammar Faizi wrote: > Hi, > > This is an RFC patchset that introduces the `wq_cpu_set` mount option. > This option lets the user specify a CPU set that the Btrfs workqueues > will use. > > Btrfs workqueues can slow sensitive user tasks down because they can use > any online CPU to perform heavy workloads on an SMP system. Add a mount > option to isolate the Btrfs workqueues to a set of CPUs. It is helpful > to avoid sensitive user tasks being preempted by Btrfs heavy workqueues. > > This option is similar to the taskset bitmask except that the comma > separator is replaced with a dot. The reason for this is that the mount > option parser uses commas to separate mount options. Hmm... the allowed cpumasks for unbounded workqueues can already be set through /sys/devices/virtual/workqueue/cpumask and also each individual workqueue can be exposed in the matching subdirectory by setting WQ_SYSFS. Wouldn't the proposed btrfs option be a bit reduandant? Thanks.
Hello, On Sun, 26 Feb 2023 07:01:41 -1000, Tejun Heo wrote: > Hmm... the allowed cpumasks for unbounded workqueues can already be set > through /sys/devices/virtual/workqueue/cpumask and also each individual > workqueue can be exposed in the matching subdirectory by setting WQ_SYSFS. > Wouldn't the proposed btrfs option be a bit reduandant? Thank you for the comment. I just realized the sysfs facility for this. So I will take a look into it deeper. For now, I have several reasons to use the `wq_cpu_set` option: 1. Currently, there are 15 btrfs workqueues. It would not be convenient to let the user manage each of them via the sysfs. Using `wq_cpu_set` option at mounting time allows the user to set all of them in one shot. (for btrfs maintainers): I am also not sure if the number of btrfs workqueues is stable so that the user can rely on the WQ_SYSFS facility. 2. I looked at /sys/devices/virtual/workqueue/ and saw its subdirectories. The directory name is taken from the wq->name. But how to distinguish multiple workqueues with the same name? Each btrfs mount will at least do this: alloc_workqueue("btrfs-compressed-write", flags, max_active); When we do: mount -t -o rw btrfs /dev/sda1 a; mount -t -o rw btrfs /dev/sda2 b; mount -t -o rw btrfs /dev/sda3 c; mount -t -o rw btrfs /dev/sda4 d; Is there a way to identify which sysfs devices correspond to a specific mounted btrfs fs workqueues? Let's say I want each mount to have a different CPU mask.
On Mon, Feb 27, 2023 at 01:26:24AM +0700, Ammar Faizi wrote: > mount -t -o rw btrfs /dev/sda1 a; > mount -t -o rw btrfs /dev/sda2 b; > mount -t -o rw btrfs /dev/sda3 c; > mount -t -o rw btrfs /dev/sda4 d; Excuse the wrong mount commands, should be something like this: mount -t btrfs -o rw /dev/sda1 a; mount -t btrfs -o rw /dev/sda2 b; mount -t btrfs -o rw /dev/sda3 c; mount -t btrfs -o rw /dev/sda4 d;
On 2023/2/27 00:02, Ammar Faizi wrote: > Hi, > > This is an RFC patchset that introduces the `wq_cpu_set` mount option. > This option lets the user specify a CPU set that the Btrfs workqueues > will use. > > Btrfs workqueues can slow sensitive user tasks down because they can use > any online CPU to perform heavy workloads on an SMP system. Add a mount > option to isolate the Btrfs workqueues to a set of CPUs. It is helpful > to avoid sensitive user tasks being preempted by Btrfs heavy workqueues. I'm not sure if pinning the wq is really the best way to your problem. Yes, I understand you want to limit the CPU usage of btrfs workqueues, but have you tried "thread_pool=" mount option? That mount option should limit the max amount of in-flight work items, thus at least limit the CPU usage. For the wq CPU pinning part, I'm not sure if it's really needed, although it's known CPU pinning can affect some performance characteristics. Thanks, Qu > > This option is similar to the taskset bitmask except that the comma > separator is replaced with a dot. The reason for this is that the mount > option parser uses commas to separate mount options. > > Figure (the CPU usage when `wq_cpu_set` is used VS when it is not): > https://gist.githubusercontent.com/ammarfaizi2/a10f8073e58d1712c1ed49af83ae4ad1/raw/a4f7cbc4eb163db792a669d570ff542495e8c704/wq_cpu_set.png > > A simple stress testing: > > 1. Open htop. > 2. Open a new terminal. > 3. Mount and perform a heavy workload on the mounted Btrfs filesystem. > > ## Test without wq_cpu_set > sudo mount -t btrfs -o rw,compress-force=zstd:15,commit=1500 /dev/sda2 hdd/a; > cp -rf /path/folder_with_many_large_files/ hdd/a/test; > sync; # See the CPU usage in htop. > sudo umount hdd/a; > > ## Test wq_cpu_set > sudo mount -t btrfs -o rw,compress-force=zstd:15,commit=1500,wq_cpu_set=0.4.1.5 /dev/sda2 hdd/a; > cp -rf /path/folder_with_many_large_files/ hdd/a/test; > sync; # See the CPU usage in htop. > sudo umount hdd/a; > > Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org> > --- > > Ammar Faizi (6): > workqueue: Add set_workqueue_cpumask() helper function > btrfs: Change `mount_opt` type in `struct btrfs_fs_info` to `u64` > btrfs: Create btrfs CPU set struct and helpers > btrfs: Add wq_cpu_set=%s mount option > btrfs: Adjust the default thread pool size when `wq_cpu_set` option is used > btrfs: Add `BTRFS_DEFAULT_MAX_THREAD_POOL_SIZE` macro > > fs/btrfs/async-thread.c | 51 ++++++++++++++++++++ > fs/btrfs/async-thread.h | 3 ++ > fs/btrfs/disk-io.c | 6 ++- > fs/btrfs/fs.c | 97 +++++++++++++++++++++++++++++++++++++++ > fs/btrfs/fs.h | 12 ++++- > fs/btrfs/super.c | 83 +++++++++++++++++++++++++++++++++ > include/linux/workqueue.h | 3 ++ > kernel/workqueue.c | 19 ++++++++ > 8 files changed, 271 insertions(+), 3 deletions(-) > > > base-commit: 2fcd07b7ccd5fd10b2120d298363e4e6c53ccf9c
On Sun, Feb 26, 2023 at 4:31 PM Ammar Faizi <ammarfaizi2@gnuweeb.org> wrote: > > Hi, > > This is an RFC patchset that introduces the `wq_cpu_set` mount option. > This option lets the user specify a CPU set that the Btrfs workqueues > will use. > > Btrfs workqueues can slow sensitive user tasks down because they can use > any online CPU to perform heavy workloads on an SMP system. Add a mount > option to isolate the Btrfs workqueues to a set of CPUs. It is helpful > to avoid sensitive user tasks being preempted by Btrfs heavy workqueues. > > This option is similar to the taskset bitmask except that the comma > separator is replaced with a dot. The reason for this is that the mount > option parser uses commas to separate mount options. > > Figure (the CPU usage when `wq_cpu_set` is used VS when it is not): > https://gist.githubusercontent.com/ammarfaizi2/a10f8073e58d1712c1ed49af83ae4ad1/raw/a4f7cbc4eb163db792a669d570ff542495e8c704/wq_cpu_set.png I haven't read the patchset. It's great that it reduces CPU usage. But does it also provide other performance benefits, like lower latency or higher throughput for some workloads? Or using less CPU also affects negatively in those other aspects? Thanks. > > A simple stress testing: > > 1. Open htop. > 2. Open a new terminal. > 3. Mount and perform a heavy workload on the mounted Btrfs filesystem. > > ## Test without wq_cpu_set > sudo mount -t btrfs -o rw,compress-force=zstd:15,commit=1500 /dev/sda2 hdd/a; > cp -rf /path/folder_with_many_large_files/ hdd/a/test; > sync; # See the CPU usage in htop. > sudo umount hdd/a; > > ## Test wq_cpu_set > sudo mount -t btrfs -o rw,compress-force=zstd:15,commit=1500,wq_cpu_set=0.4.1.5 /dev/sda2 hdd/a; > cp -rf /path/folder_with_many_large_files/ hdd/a/test; > sync; # See the CPU usage in htop. > sudo umount hdd/a; > > Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org> > --- > > Ammar Faizi (6): > workqueue: Add set_workqueue_cpumask() helper function > btrfs: Change `mount_opt` type in `struct btrfs_fs_info` to `u64` > btrfs: Create btrfs CPU set struct and helpers > btrfs: Add wq_cpu_set=%s mount option > btrfs: Adjust the default thread pool size when `wq_cpu_set` option is used > btrfs: Add `BTRFS_DEFAULT_MAX_THREAD_POOL_SIZE` macro > > fs/btrfs/async-thread.c | 51 ++++++++++++++++++++ > fs/btrfs/async-thread.h | 3 ++ > fs/btrfs/disk-io.c | 6 ++- > fs/btrfs/fs.c | 97 +++++++++++++++++++++++++++++++++++++++ > fs/btrfs/fs.h | 12 ++++- > fs/btrfs/super.c | 83 +++++++++++++++++++++++++++++++++ > include/linux/workqueue.h | 3 ++ > kernel/workqueue.c | 19 ++++++++ > 8 files changed, 271 insertions(+), 3 deletions(-) > > > base-commit: 2fcd07b7ccd5fd10b2120d298363e4e6c53ccf9c > -- > Ammar Faizi >
On 2023/2/27 19:02, Filipe Manana wrote: > On Sun, Feb 26, 2023 at 4:31 PM Ammar Faizi <ammarfaizi2@gnuweeb.org> wrote: >> >> Hi, >> >> This is an RFC patchset that introduces the `wq_cpu_set` mount option. >> This option lets the user specify a CPU set that the Btrfs workqueues >> will use. >> >> Btrfs workqueues can slow sensitive user tasks down because they can use >> any online CPU to perform heavy workloads on an SMP system. Add a mount >> option to isolate the Btrfs workqueues to a set of CPUs. It is helpful >> to avoid sensitive user tasks being preempted by Btrfs heavy workqueues. >> >> This option is similar to the taskset bitmask except that the comma >> separator is replaced with a dot. The reason for this is that the mount >> option parser uses commas to separate mount options. >> >> Figure (the CPU usage when `wq_cpu_set` is used VS when it is not): >> https://gist.githubusercontent.com/ammarfaizi2/a10f8073e58d1712c1ed49af83ae4ad1/raw/a4f7cbc4eb163db792a669d570ff542495e8c704/wq_cpu_set.png > > I haven't read the patchset. > > It's great that it reduces CPU usage. > But does it also provide other performance benefits, like lower > latency or higher throughput for some workloads? Or using less CPU > also affects negatively in those other aspects? So far it looks like to just set CPU masks for each workqueue. Thus if it's reducing CPU usage, it also takes longer time to finish the workload (compression,csum calculation etc). Thanks, Qu > > Thanks. > >> >> A simple stress testing: >> >> 1. Open htop. >> 2. Open a new terminal. >> 3. Mount and perform a heavy workload on the mounted Btrfs filesystem. >> >> ## Test without wq_cpu_set >> sudo mount -t btrfs -o rw,compress-force=zstd:15,commit=1500 /dev/sda2 hdd/a; >> cp -rf /path/folder_with_many_large_files/ hdd/a/test; >> sync; # See the CPU usage in htop. >> sudo umount hdd/a; >> >> ## Test wq_cpu_set >> sudo mount -t btrfs -o rw,compress-force=zstd:15,commit=1500,wq_cpu_set=0.4.1.5 /dev/sda2 hdd/a; >> cp -rf /path/folder_with_many_large_files/ hdd/a/test; >> sync; # See the CPU usage in htop. >> sudo umount hdd/a; >> >> Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org> >> --- >> >> Ammar Faizi (6): >> workqueue: Add set_workqueue_cpumask() helper function >> btrfs: Change `mount_opt` type in `struct btrfs_fs_info` to `u64` >> btrfs: Create btrfs CPU set struct and helpers >> btrfs: Add wq_cpu_set=%s mount option >> btrfs: Adjust the default thread pool size when `wq_cpu_set` option is used >> btrfs: Add `BTRFS_DEFAULT_MAX_THREAD_POOL_SIZE` macro >> >> fs/btrfs/async-thread.c | 51 ++++++++++++++++++++ >> fs/btrfs/async-thread.h | 3 ++ >> fs/btrfs/disk-io.c | 6 ++- >> fs/btrfs/fs.c | 97 +++++++++++++++++++++++++++++++++++++++ >> fs/btrfs/fs.h | 12 ++++- >> fs/btrfs/super.c | 83 +++++++++++++++++++++++++++++++++ >> include/linux/workqueue.h | 3 ++ >> kernel/workqueue.c | 19 ++++++++ >> 8 files changed, 271 insertions(+), 3 deletions(-) >> >> >> base-commit: 2fcd07b7ccd5fd10b2120d298363e4e6c53ccf9c >> -- >> Ammar Faizi >>
On Mon, Feb 27, 2023 at 06:18:43PM +0800, Qu Wenruo wrote: > I'm not sure if pinning the wq is really the best way to your problem. > > Yes, I understand you want to limit the CPU usage of btrfs workqueues, but > have you tried "thread_pool=" mount option? > > That mount option should limit the max amount of in-flight work items, thus > at least limit the CPU usage. I have tried to use the thread_poll=%u mount option previously. But I didn't observe the effect intensively. I'll try to play with this option more and see if it can yield the desired behavior. > For the wq CPU pinning part, I'm not sure if it's really needed, although > it's known CPU pinning can affect some performance characteristics. What I like about CPU pinning is that we can dedicate CPUs for specific workloads so it won't cause scheduling noise to the app we've dedicated other CPUs for.
On 2/27/23 6:46 PM, Qu Wenruo wrote: > On 2023/2/27 19:02, Filipe Manana wrote: >> On Sun, Feb 26, 2023 at 4:31 PM Ammar Faizi <ammarfaizi2@gnuweeb.org> wrote: >>> Figure (the CPU usage when `wq_cpu_set` is used VS when it is not): >>> https://gist.githubusercontent.com/ammarfaizi2/a10f8073e58d1712c1ed49af83ae4ad1/raw/a4f7cbc4eb163db792a669d570ff542495e8c704/wq_cpu_set.png >> >> I haven't read the patchset. >> >> It's great that it reduces CPU usage. But does it also provide >> other performance benefits, like lower latency or higher throughput >> for some workloads? Or using less CPU also affects negatively in >> those other aspects? Based on my testing, it gives lower latency for a browser app playing a YouTube video. Without this proposed option, high-level compression on a btrfs storage is a real noise to user space apps. It periodically freezes the UI for 2 to 3 seconds and causes audio lag; it mostly happens when it starts writing the dirty write to the disk. It's reasonably easy to reproduce by making a large dirty write and invoking a "sync" command. Side note: Pin user apps to CPUs a,b,c,d and btrfs workquques to CPUs w,x,y,z. > So far it looks like to just set CPU masks for each workqueue. > > Thus if it's reducing CPU usage, it also takes longer time to finish > the workload (compression,csum calculation etc). Yes, that's correct. I see this as a good mount option for btrfs because the btrfs-workload in question is CPU bound, specifically for the writing operation. While it may degrade the btrfs workload because we limit the number of usable CPUs, there is a condition where users don't prioritize writing to disk. Let's say: I want to run a smooth app with video. I also want to have high-level compression for my btrfs storage. But I don't want the compression and checksum work to bother my video; here, I give you CPU x,y,z for the btrfs work. And here I give you CPU a,b,c,d,e,f for the video work. I have a similar case on a torrent seeder server where high-level compression is expected. And I believe there are more cases where this option is advantageous. Thank you all for the comments,
On Mon, 27 Feb 2023 20:45:26 +0700 Ammar Faizi <ammarfaizi2@gmail.com> wrote: > Based on my testing, it gives lower latency for a browser app playing > a YouTube video. > > Without this proposed option, high-level compression on a btrfs > storage is a real noise to user space apps. It periodically freezes > the UI for 2 to 3 seconds and causes audio lag; it mostly happens when > it starts writing the dirty write to the disk. > > It's reasonably easy to reproduce by making a large dirty write and > invoking a "sync" command. > > Side note: Pin user apps to CPUs a,b,c,d and btrfs workquques to CPUs > w,x,y,z. The end user should not be expected to do that. (At least that is my opinion as an end user :) The worst part, in times when Btrfs has no current work to do, it means the user apps are hard-capped to 4 cores for no good reason, and the other 4 are idling. Even with a split like 6/2, this is still looks like giving up on the achievements of multi-tasking operating systems and falling back to some antique state with fixed separation. > I want to run a smooth app with video. I also want to have high-level > compression for my btrfs storage. But I don't want the compression and > checksum work to bother my video; here, I give you CPU x,y,z for the > btrfs work. And here I give you CPU a,b,c,d,e,f for the video work. > > I have a similar case on a torrent seeder server where high-level > compression is expected. And I believe there are more cases where this > option is advantageous. I really hope there can be some other approach. Such as some adjustment to kworker processing so that it's not as aggressive in starving userspace. There are no possibility to run kernel task at a lower priority, right? But if no other way, maybe an option like that is good to have for the moment.
On Sun, Feb 26, 2023 at 11:02:53PM +0700, Ammar Faizi wrote: > ## Test wq_cpu_set > sudo mount -t btrfs -o rw,compress-force=zstd:15,commit=1500,wq_cpu_set=0.4.1.5 /dev/sda2 hdd/a; > cp -rf /path/folder_with_many_large_files/ hdd/a/test; > sync; # See the CPU usage in htop. > sudo umount hdd/a; This seems like the wrong model for setting cpu locality for internal filesystem threads. Users are used to controlling cpu sets and other locality behaviour of a task with wrapper tools like numactl. Wrap th emount command with a numactl command to limit the CPU set, then have the btrfs fill_super() callback set the cpu mask for the work queues it creates based on the cpu mask that has been set for the mount task. That is, I think the model should be "inherit cpu mask from parent task" rather than adding mount options. This model allows anything that numactl can control (e.g. memory locality) to also influence the filesystem default behaviour without having to add yet more mount options in the future.... -Dave.
On 2023/2/27 21:42, Ammar Faizi wrote: > On Mon, Feb 27, 2023 at 06:18:43PM +0800, Qu Wenruo wrote: >> I'm not sure if pinning the wq is really the best way to your problem. >> >> Yes, I understand you want to limit the CPU usage of btrfs workqueues, but >> have you tried "thread_pool=" mount option? >> >> That mount option should limit the max amount of in-flight work items, thus >> at least limit the CPU usage. > > I have tried to use the thread_poll=%u mount option previously. But I > didn't observe the effect intensively. I'll try to play with this option > more and see if it can yield the desired behavior. The thread_pool mount option is much harder to observe the behavior change. As wq pinned to one or two CPUs is easy to observe using htop, while the unbounded wq, even with thread_pool, is much harder to observe. Thus it needs more systematic testing to find the difference. > >> For the wq CPU pinning part, I'm not sure if it's really needed, although >> it's known CPU pinning can affect some performance characteristics. > > What I like about CPU pinning is that we can dedicate CPUs for specific > workloads so it won't cause scheduling noise to the app we've dedicated > other CPUs for. > I'm not 100% sure if we're really any better than the scheduler developers, as there are a lot of more things to consider. E.g. for recent Intel CPUs, they have BIG and little cores, and BIG cores even have SMT supports. For current kernels, scheduler would avoid putting workloads into the threads sharing the same physical cores. Thus it can result seemingly weird priority like BIG core thread1 > little core > BIG core thread2. But that results overall better performance. So overall, unless necessary I'd avoid manual CPU pinning. Thanks, Qu
On Tue, Feb 28, 2023 at 09:17:45AM +1100, Dave Chinner wrote: > This seems like the wrong model for setting cpu locality for > internal filesystem threads. > > Users are used to controlling cpu sets and other locality behaviour > of a task with wrapper tools like numactl. Wrap th emount command > with a numactl command to limit the CPU set, then have the btrfs > fill_super() callback set the cpu mask for the work queues it > creates based on the cpu mask that has been set for the mount task. > > That is, I think the model should be "inherit cpu mask from parent > task" rather than adding mount options. This model allows anything > that numactl can control (e.g. memory locality) to also influence > the filesystem default behaviour without having to add yet more > mount options in the future.... Good idea on the tooling part. I like the idea of using numactl to determine a proper CPU set. But users may also use /etc/fstab to mount their btrfs storage. In that case, using mount option is still handy. Also, if we always inherit CPU mask from the parent task who calls the mount, it will be breaking the CPU affinity for old users who inadvertently call their mount cmd with random CPU mask. We should keep the old behavior and allow user to opt in if they want to. Maybe something like: numactl -N 0 mount -t btrfs -o rw,wq_cpu_set=inherit /dev/bla bla
Hi, This is an RFC patchset that introduces the `wq_cpu_set` mount option. This option lets the user specify a CPU set that the Btrfs workqueues will use. Btrfs workqueues can slow sensitive user tasks down because they can use any online CPU to perform heavy workloads on an SMP system. Add a mount option to isolate the Btrfs workqueues to a set of CPUs. It is helpful to avoid sensitive user tasks being preempted by Btrfs heavy workqueues. This option is similar to the taskset bitmask except that the comma separator is replaced with a dot. The reason for this is that the mount option parser uses commas to separate mount options. Figure (the CPU usage when `wq_cpu_set` is used VS when it is not): https://gist.githubusercontent.com/ammarfaizi2/a10f8073e58d1712c1ed49af83ae4ad1/raw/a4f7cbc4eb163db792a669d570ff542495e8c704/wq_cpu_set.png A simple stress testing: 1. Open htop. 2. Open a new terminal. 3. Mount and perform a heavy workload on the mounted Btrfs filesystem. ## Test without wq_cpu_set sudo mount -t btrfs -o rw,compress-force=zstd:15,commit=1500 /dev/sda2 hdd/a; cp -rf /path/folder_with_many_large_files/ hdd/a/test; sync; # See the CPU usage in htop. sudo umount hdd/a; ## Test wq_cpu_set sudo mount -t btrfs -o rw,compress-force=zstd:15,commit=1500,wq_cpu_set=0.4.1.5 /dev/sda2 hdd/a; cp -rf /path/folder_with_many_large_files/ hdd/a/test; sync; # See the CPU usage in htop. sudo umount hdd/a; Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org> --- Ammar Faizi (6): workqueue: Add set_workqueue_cpumask() helper function btrfs: Change `mount_opt` type in `struct btrfs_fs_info` to `u64` btrfs: Create btrfs CPU set struct and helpers btrfs: Add wq_cpu_set=%s mount option btrfs: Adjust the default thread pool size when `wq_cpu_set` option is used btrfs: Add `BTRFS_DEFAULT_MAX_THREAD_POOL_SIZE` macro fs/btrfs/async-thread.c | 51 ++++++++++++++++++++ fs/btrfs/async-thread.h | 3 ++ fs/btrfs/disk-io.c | 6 ++- fs/btrfs/fs.c | 97 +++++++++++++++++++++++++++++++++++++++ fs/btrfs/fs.h | 12 ++++- fs/btrfs/super.c | 83 +++++++++++++++++++++++++++++++++ include/linux/workqueue.h | 3 ++ kernel/workqueue.c | 19 ++++++++ 8 files changed, 271 insertions(+), 3 deletions(-) base-commit: 2fcd07b7ccd5fd10b2120d298363e4e6c53ccf9c