| Message ID | 20190603134017.9323-1-ntsironis@arrikto.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
| Series | dm kcopyd: Increase sub-job size to 512KiB | expand |
On Mon, Jun 03 2019 at 9:40am -0400, Nikos Tsironis <ntsironis@arrikto.com> wrote: > Currently, kcopyd has a sub-job size of 64KiB and a maximum number of 8 > sub-jobs. As a result, for any kcopyd job, we have a maximum of 512KiB > of I/O in flight. > > This upper limit to the amount of in-flight I/O under-utilizes fast > devices and results in decreased throughput, e.g., when writing to a > snapshotted thin LV with I/O size less than the pool's block size (so > COW is performed using kcopyd). > > Increase kcopyd's sub-job size to 512KiB, so we have a maximum of 4MiB > of I/O in flight for each kcopyd job. This results in an up to 96% > improvement of bandwidth when writing to a snapshotted thin LV, with I/O > sizes less than the pool's block size. > > We evaluate the performance impact of the change by running the > snap_breaking_throughput benchmark, from the device mapper test suite > [1]. > > The benchmark: > > 1. Creates a 1G thin LV > 2. Provisions the thin LV > 3. Takes a snapshot of the thin LV > 4. Writes to the thin LV with: > > dd if=/dev/zero of=/dev/vg/thin_lv oflag=direct bs=<I/O size> > > Running this benchmark with various thin pool block sizes and dd I/O > sizes (all combinations triggering the use of kcopyd) we get the > following results: > > +-----------------+-------------+------------------+-----------------+ > | Pool block size | dd I/O size | BW before (MB/s) | BW after (MB/s) | > +-----------------+-------------+------------------+-----------------+ > | 1 MB | 256 KB | 242 | 280 | > | 1 MB | 512 KB | 238 | 295 | > | | | | | > | 2 MB | 256 KB | 238 | 354 | > | 2 MB | 512 KB | 241 | 380 | > | 2 MB | 1 MB | 245 | 394 | > | | | | | > | 4 MB | 256 KB | 248 | 412 | > | 4 MB | 512 KB | 234 | 432 | > | 4 MB | 1 MB | 251 | 474 | > | 4 MB | 2 MB | 257 | 504 | > | | | | | > | 8 MB | 256 KB | 239 | 420 | > | 8 MB | 512 KB | 256 | 431 | > | 8 MB | 1 MB | 264 | 467 | > | 8 MB | 2 MB | 264 | 502 | > | 8 MB | 4 MB | 281 | 537 | > +-----------------+-------------+------------------+-----------------+ > > [1] https://github.com/jthornber/device-mapper-test-suite > > Signed-off-by: Nikos Tsironis <ntsironis@arrikto.com> > --- > drivers/md/dm-kcopyd.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/md/dm-kcopyd.c b/drivers/md/dm-kcopyd.c > index 671c24332802..db0a7d1e33b7 100644 > --- a/drivers/md/dm-kcopyd.c > +++ b/drivers/md/dm-kcopyd.c > @@ -28,7 +28,7 @@ > > #include "dm-core.h" > > -#define SUB_JOB_SIZE 128 > +#define SUB_JOB_SIZE 1024 > #define SPLIT_COUNT 8 > #define MIN_JOBS 8 > #define RESERVE_PAGES (DIV_ROUND_UP(SUB_JOB_SIZE << SECTOR_SHIFT, PAGE_SIZE)) > -- > 2.11.0 > Thanks for the patch, clearly we're leaving considerable performance on the table. But I'm left wondering whether we should preserve the 64K default but allow targets to override the sub-job size at kcopyd client create time? Or do you feel that all slower storage wouldn't be adversely impacted by this sub-job size increase from 64K to 512K? Mike -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 6/3/19 5:08 PM, Mike Snitzer wrote: > On Mon, Jun 03 2019 at 9:40am -0400, > Nikos Tsironis <ntsironis@arrikto.com> wrote: > >> Currently, kcopyd has a sub-job size of 64KiB and a maximum number of 8 >> sub-jobs. As a result, for any kcopyd job, we have a maximum of 512KiB >> of I/O in flight. >> >> This upper limit to the amount of in-flight I/O under-utilizes fast >> devices and results in decreased throughput, e.g., when writing to a >> snapshotted thin LV with I/O size less than the pool's block size (so >> COW is performed using kcopyd). >> >> Increase kcopyd's sub-job size to 512KiB, so we have a maximum of 4MiB >> of I/O in flight for each kcopyd job. This results in an up to 96% >> improvement of bandwidth when writing to a snapshotted thin LV, with I/O >> sizes less than the pool's block size. >> >> We evaluate the performance impact of the change by running the >> snap_breaking_throughput benchmark, from the device mapper test suite >> [1]. >> >> The benchmark: >> >> 1. Creates a 1G thin LV >> 2. Provisions the thin LV >> 3. Takes a snapshot of the thin LV >> 4. Writes to the thin LV with: >> >> dd if=/dev/zero of=/dev/vg/thin_lv oflag=direct bs=<I/O size> >> >> Running this benchmark with various thin pool block sizes and dd I/O >> sizes (all combinations triggering the use of kcopyd) we get the >> following results: >> >> +-----------------+-------------+------------------+-----------------+ >> | Pool block size | dd I/O size | BW before (MB/s) | BW after (MB/s) | >> +-----------------+-------------+------------------+-----------------+ >> | 1 MB | 256 KB | 242 | 280 | >> | 1 MB | 512 KB | 238 | 295 | >> | | | | | >> | 2 MB | 256 KB | 238 | 354 | >> | 2 MB | 512 KB | 241 | 380 | >> | 2 MB | 1 MB | 245 | 394 | >> | | | | | >> | 4 MB | 256 KB | 248 | 412 | >> | 4 MB | 512 KB | 234 | 432 | >> | 4 MB | 1 MB | 251 | 474 | >> | 4 MB | 2 MB | 257 | 504 | >> | | | | | >> | 8 MB | 256 KB | 239 | 420 | >> | 8 MB | 512 KB | 256 | 431 | >> | 8 MB | 1 MB | 264 | 467 | >> | 8 MB | 2 MB | 264 | 502 | >> | 8 MB | 4 MB | 281 | 537 | >> +-----------------+-------------+------------------+-----------------+ >> >> [1] https://github.com/jthornber/device-mapper-test-suite >> >> Signed-off-by: Nikos Tsironis <ntsironis@arrikto.com> >> --- >> drivers/md/dm-kcopyd.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/md/dm-kcopyd.c b/drivers/md/dm-kcopyd.c >> index 671c24332802..db0a7d1e33b7 100644 >> --- a/drivers/md/dm-kcopyd.c >> +++ b/drivers/md/dm-kcopyd.c >> @@ -28,7 +28,7 @@ >> >> #include "dm-core.h" >> >> -#define SUB_JOB_SIZE 128 >> +#define SUB_JOB_SIZE 1024 >> #define SPLIT_COUNT 8 >> #define MIN_JOBS 8 >> #define RESERVE_PAGES (DIV_ROUND_UP(SUB_JOB_SIZE << SECTOR_SHIFT, PAGE_SIZE)) >> -- >> 2.11.0 >> > > Thanks for the patch, clearly we're leaving considerable performance on > the table. But I'm left wondering whether we should preserve the 64K > default but allow targets to override the sub-job size at kcopyd client > create time? > Hi Mike, We could do that, but then I think we should also expose kcopyd's sub-job size as a per-target module parameter. Targets don't know about the performance characteristics of the underlying storage, so they are not in place to make a better decision about the sub-job size. So, we should probably leave the decision to the system administrator. > Or do you feel that all slower storage wouldn't be adversely impacted by > this sub-job size increase from 64K to 512K? > Intuitively, increasing the request size will increase the request latency and thus result in worse interactive performance. But, copy bandwidth should be unaffected. Moreover, the change affects targets, e.g., dm-thin, when we use a block size greater than 512KiB, which therefore increases the amount of data we COW when writing to a shared block. But COW, with large block sizes, will result in increased latency despite this change. Thanks, Nikos > Mike > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Hi Mike, A kind reminder about this patch. Do you require any changes or will you merge it as is? Thanks, Nikos On 6/3/19 4:40 PM, Nikos Tsironis wrote: > Currently, kcopyd has a sub-job size of 64KiB and a maximum number of 8 > sub-jobs. As a result, for any kcopyd job, we have a maximum of 512KiB > of I/O in flight. > > This upper limit to the amount of in-flight I/O under-utilizes fast > devices and results in decreased throughput, e.g., when writing to a > snapshotted thin LV with I/O size less than the pool's block size (so > COW is performed using kcopyd). > > Increase kcopyd's sub-job size to 512KiB, so we have a maximum of 4MiB > of I/O in flight for each kcopyd job. This results in an up to 96% > improvement of bandwidth when writing to a snapshotted thin LV, with I/O > sizes less than the pool's block size. > > We evaluate the performance impact of the change by running the > snap_breaking_throughput benchmark, from the device mapper test suite > [1]. > > The benchmark: > > 1. Creates a 1G thin LV > 2. Provisions the thin LV > 3. Takes a snapshot of the thin LV > 4. Writes to the thin LV with: > > dd if=/dev/zero of=/dev/vg/thin_lv oflag=direct bs=<I/O size> > > Running this benchmark with various thin pool block sizes and dd I/O > sizes (all combinations triggering the use of kcopyd) we get the > following results: > > +-----------------+-------------+------------------+-----------------+ > | Pool block size | dd I/O size | BW before (MB/s) | BW after (MB/s) | > +-----------------+-------------+------------------+-----------------+ > | 1 MB | 256 KB | 242 | 280 | > | 1 MB | 512 KB | 238 | 295 | > | | | | | > | 2 MB | 256 KB | 238 | 354 | > | 2 MB | 512 KB | 241 | 380 | > | 2 MB | 1 MB | 245 | 394 | > | | | | | > | 4 MB | 256 KB | 248 | 412 | > | 4 MB | 512 KB | 234 | 432 | > | 4 MB | 1 MB | 251 | 474 | > | 4 MB | 2 MB | 257 | 504 | > | | | | | > | 8 MB | 256 KB | 239 | 420 | > | 8 MB | 512 KB | 256 | 431 | > | 8 MB | 1 MB | 264 | 467 | > | 8 MB | 2 MB | 264 | 502 | > | 8 MB | 4 MB | 281 | 537 | > +-----------------+-------------+------------------+-----------------+ > > [1] https://github.com/jthornber/device-mapper-test-suite > > Signed-off-by: Nikos Tsironis <ntsironis@arrikto.com> > --- > drivers/md/dm-kcopyd.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/md/dm-kcopyd.c b/drivers/md/dm-kcopyd.c > index 671c24332802..db0a7d1e33b7 100644 > --- a/drivers/md/dm-kcopyd.c > +++ b/drivers/md/dm-kcopyd.c > @@ -28,7 +28,7 @@ > > #include "dm-core.h" > > -#define SUB_JOB_SIZE 128 > +#define SUB_JOB_SIZE 1024 > #define SPLIT_COUNT 8 > #define MIN_JOBS 8 > #define RESERVE_PAGES (DIV_ROUND_UP(SUB_JOB_SIZE << SECTOR_SHIFT, PAGE_SIZE)) > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Fri, Jul 12 2019 at 9:45am -0400, Nikos Tsironis <ntsironis@arrikto.com> wrote: > Hi Mike, > > A kind reminder about this patch. Do you require any changes or will you > merge it as is? I think we need changes to expose knob(s) to tune this value on a global _and_ device level via sysfs. E.g.: 1) dm_mod module param for global 2) but also allow a per-device override, like: echo 512 > /sys/block/dm-X/dm/kcopyd_subjob_size 1 is super easy and is a start. Layering in 2 is a bit more involved. In hindsight (given how risk-averse I am on changing the default) I should've kept the default 128 but allowed override with modparam dm_mod.kcopyd_subjob_size=1024 Would this be an OK first step? If so, we're still in the 5.3 merge window, I'll see what I can do. Thanks, Mike -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 7/15/19 9:22 PM, Mike Snitzer wrote: > On Fri, Jul 12 2019 at 9:45am -0400, > Nikos Tsironis <ntsironis@arrikto.com> wrote: > >> Hi Mike, >> >> A kind reminder about this patch. Do you require any changes or will you >> merge it as is? > > I think we need changes to expose knob(s) to tune this value on a global > _and_ device level via sysfs. E.g.: > > 1) dm_mod module param for global > 2) but also allow a per-device override, like: > echo 512 > /sys/block/dm-X/dm/kcopyd_subjob_size > Hi Mike, Thanks for your feedback. I agree, this sounds like the best thing to do. > 1 is super easy and is a start. Layering in 2 is a bit more involved. Maybe I could help with (2). We could discuss about it and how you think it's best to do it and then I could proceed with an implementation. Please let me know what you think. > > In hindsight (given how risk-averse I am on changing the default) I > should've kept the default 128 but allowed override with modparam > dm_mod.kcopyd_subjob_size=1024 > > Would this be an OK first step? Yes, this would be great. > > If so, we're still in the 5.3 merge window, I'll see what I can do. Shall I proceed with a patch adding the dm_mod.kcopyd_subjob_size modparam? Thanks, Nikos > > Thanks, > Mike > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Tue, Jul 16 2019 at 9:59am -0400, Nikos Tsironis <ntsironis@arrikto.com> wrote: > On 7/15/19 9:22 PM, Mike Snitzer wrote: > > On Fri, Jul 12 2019 at 9:45am -0400, > > Nikos Tsironis <ntsironis@arrikto.com> wrote: > > > >> Hi Mike, > >> > >> A kind reminder about this patch. Do you require any changes or will you > >> merge it as is? > > > > I think we need changes to expose knob(s) to tune this value on a global > > _and_ device level via sysfs. E.g.: > > > > 1) dm_mod module param for global > > 2) but also allow a per-device override, like: > > echo 512 > /sys/block/dm-X/dm/kcopyd_subjob_size > > > > Hi Mike, > > Thanks for your feedback. I agree, this sounds like the best thing to do. > > > 1 is super easy and is a start. Layering in 2 is a bit more involved. > > Maybe I could help with (2). We could discuss about it and how you think > it's best to do it and then I could proceed with an implementation. > > Please let me know what you think. > > > > > In hindsight (given how risk-averse I am on changing the default) I > > should've kept the default 128 but allowed override with modparam > > dm_mod.kcopyd_subjob_size=1024 > > > > Would this be an OK first step? > > Yes, this would be great. > > > > > If so, we're still in the 5.3 merge window, I'll see what I can do. > > Shall I proceed with a patch adding the dm_mod.kcopyd_subjob_size > modparam? Sure. And it could be that we won't need 2. Ideally the default would work for every setup. Less knobs the better. But as a stop-gap I think we need to expose a knob that allows override. Thinking further, I don't think changing the default to 512K is too risky (famous last words). So please just update your original patch to include the modparam so that users can get the old 64K back if needed. BTW, the param name should probably be "kcopyd_subjob_size_kb" to reflect the value is KB. Thanks, Mike -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Tue, Jul 16 2019 at 10:11am -0400, Mike Snitzer <snitzer@redhat.com> wrote: > On Tue, Jul 16 2019 at 9:59am -0400, > Nikos Tsironis <ntsironis@arrikto.com> wrote: > > > On 7/15/19 9:22 PM, Mike Snitzer wrote: > > > On Fri, Jul 12 2019 at 9:45am -0400, > > > Nikos Tsironis <ntsironis@arrikto.com> wrote: > > > > > >> Hi Mike, > > >> > > >> A kind reminder about this patch. Do you require any changes or will you > > >> merge it as is? > > > > > > I think we need changes to expose knob(s) to tune this value on a global > > > _and_ device level via sysfs. E.g.: > > > > > > 1) dm_mod module param for global > > > 2) but also allow a per-device override, like: > > > echo 512 > /sys/block/dm-X/dm/kcopyd_subjob_size > > > > > > > Hi Mike, > > > > Thanks for your feedback. I agree, this sounds like the best thing to do. > > > > > 1 is super easy and is a start. Layering in 2 is a bit more involved. > > > > Maybe I could help with (2). We could discuss about it and how you think > > it's best to do it and then I could proceed with an implementation. > > > > Please let me know what you think. > > > > > > > > In hindsight (given how risk-averse I am on changing the default) I > > > should've kept the default 128 but allowed override with modparam > > > dm_mod.kcopyd_subjob_size=1024 > > > > > > Would this be an OK first step? > > > > Yes, this would be great. > > > > > > > > If so, we're still in the 5.3 merge window, I'll see what I can do. > > > > Shall I proceed with a patch adding the dm_mod.kcopyd_subjob_size > > modparam? > > Sure. And it could be that we won't need 2. > > Ideally the default would work for every setup. Less knobs the better. > But as a stop-gap I think we need to expose a knob that allows override. > > Thinking further, I don't think changing the default to 512K is too > risky (famous last words). So please just update your original patch to > include the modparam so that users can get the old 64K back if needed. > > BTW, the param name should probably be "kcopyd_subjob_size_kb" to > reflect the value is KB. One other thing: not sure what the max should be on this modparam.. maybe 1024K? Mike -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 7/16/19 5:14 PM, Mike Snitzer wrote: > On Tue, Jul 16 2019 at 10:11am -0400, > Mike Snitzer <snitzer@redhat.com> wrote: > >> On Tue, Jul 16 2019 at 9:59am -0400, >> Nikos Tsironis <ntsironis@arrikto.com> wrote: >> >>> On 7/15/19 9:22 PM, Mike Snitzer wrote: >>>> On Fri, Jul 12 2019 at 9:45am -0400, >>>> Nikos Tsironis <ntsironis@arrikto.com> wrote: >>>> >>>>> Hi Mike, >>>>> >>>>> A kind reminder about this patch. Do you require any changes or will you >>>>> merge it as is? >>>> >>>> I think we need changes to expose knob(s) to tune this value on a global >>>> _and_ device level via sysfs. E.g.: >>>> >>>> 1) dm_mod module param for global >>>> 2) but also allow a per-device override, like: >>>> echo 512 > /sys/block/dm-X/dm/kcopyd_subjob_size >>>> >>> >>> Hi Mike, >>> >>> Thanks for your feedback. I agree, this sounds like the best thing to do. >>> >>>> 1 is super easy and is a start. Layering in 2 is a bit more involved. >>> >>> Maybe I could help with (2). We could discuss about it and how you think >>> it's best to do it and then I could proceed with an implementation. >>> >>> Please let me know what you think. >>> >>>> >>>> In hindsight (given how risk-averse I am on changing the default) I >>>> should've kept the default 128 but allowed override with modparam >>>> dm_mod.kcopyd_subjob_size=1024 >>>> >>>> Would this be an OK first step? >>> >>> Yes, this would be great. >>> >>>> >>>> If so, we're still in the 5.3 merge window, I'll see what I can do. >>> >>> Shall I proceed with a patch adding the dm_mod.kcopyd_subjob_size >>> modparam? >> >> Sure. And it could be that we won't need 2. >> >> Ideally the default would work for every setup. Less knobs the better. >> But as a stop-gap I think we need to expose a knob that allows override. >> >> Thinking further, I don't think changing the default to 512K is too >> risky (famous last words). So please just update your original patch to >> include the modparam so that users can get the old 64K back if needed. >> >> BTW, the param name should probably be "kcopyd_subjob_size_kb" to >> reflect the value is KB. > > One other thing: not sure what the max should be on this > modparam.. maybe 1024K? I think 1024K is a reasonable maximum value. I will add the "kcopyd_subjob_size_kb" modparam and send a second version of the patch. Thanks, Nikos > > Mike > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
diff --git a/drivers/md/dm-kcopyd.c b/drivers/md/dm-kcopyd.c index 671c24332802..db0a7d1e33b7 100644 --- a/drivers/md/dm-kcopyd.c +++ b/drivers/md/dm-kcopyd.c @@ -28,7 +28,7 @@ #include "dm-core.h" -#define SUB_JOB_SIZE 128 +#define SUB_JOB_SIZE 1024 #define SPLIT_COUNT 8 #define MIN_JOBS 8 #define RESERVE_PAGES (DIV_ROUND_UP(SUB_JOB_SIZE << SECTOR_SHIFT, PAGE_SIZE))
Currently, kcopyd has a sub-job size of 64KiB and a maximum number of 8 sub-jobs. As a result, for any kcopyd job, we have a maximum of 512KiB of I/O in flight. This upper limit to the amount of in-flight I/O under-utilizes fast devices and results in decreased throughput, e.g., when writing to a snapshotted thin LV with I/O size less than the pool's block size (so COW is performed using kcopyd). Increase kcopyd's sub-job size to 512KiB, so we have a maximum of 4MiB of I/O in flight for each kcopyd job. This results in an up to 96% improvement of bandwidth when writing to a snapshotted thin LV, with I/O sizes less than the pool's block size. We evaluate the performance impact of the change by running the snap_breaking_throughput benchmark, from the device mapper test suite [1]. The benchmark: 1. Creates a 1G thin LV 2. Provisions the thin LV 3. Takes a snapshot of the thin LV 4. Writes to the thin LV with: dd if=/dev/zero of=/dev/vg/thin_lv oflag=direct bs=<I/O size> Running this benchmark with various thin pool block sizes and dd I/O sizes (all combinations triggering the use of kcopyd) we get the following results: +-----------------+-------------+------------------+-----------------+ | Pool block size | dd I/O size | BW before (MB/s) | BW after (MB/s) | +-----------------+-------------+------------------+-----------------+ | 1 MB | 256 KB | 242 | 280 | | 1 MB | 512 KB | 238 | 295 | | | | | | | 2 MB | 256 KB | 238 | 354 | | 2 MB | 512 KB | 241 | 380 | | 2 MB | 1 MB | 245 | 394 | | | | | | | 4 MB | 256 KB | 248 | 412 | | 4 MB | 512 KB | 234 | 432 | | 4 MB | 1 MB | 251 | 474 | | 4 MB | 2 MB | 257 | 504 | | | | | | | 8 MB | 256 KB | 239 | 420 | | 8 MB | 512 KB | 256 | 431 | | 8 MB | 1 MB | 264 | 467 | | 8 MB | 2 MB | 264 | 502 | | 8 MB | 4 MB | 281 | 537 | +-----------------+-------------+------------------+-----------------+ [1] https://github.com/jthornber/device-mapper-test-suite Signed-off-by: Nikos Tsironis <ntsironis@arrikto.com> --- drivers/md/dm-kcopyd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)