Re: [dm-devel] [PATCH v2] staging: writeboost: Add dm-writeboost
Hi, Jianjian, You really get a point at the fundamental design. > If I understand it correctly, the whole idea indeed is very simple, > the consumer/provider and circular buffer model. use SSD as a circular > write buffer, write flush thread stores incoming writes to this buffer > sequentially as provider, and writeback thread write those data logs > sequentially into backing device as consumer. > > If writeboost can do that without any random writes, then probably it > can save SSD/FTL of doing a lot of dirty jobs, and utilize the faster > sequential read/write performance from SSD. That'll be awesome. > However, I saw every data log segment in its design has meta data > header, like dirty_bits, so I guess writeboost has to randomly write > those data headers of stored data logs in SSD; also, splitting all bio > into 4KB will hurt its ability to get max raw SSD throughput, modern > NAND Flash has pages much bigger than 4KB; so overall I think the > actual benefits writeboost gets from this design will be discounted. You understand *almost* correctly. Writeboost has two circular buffers, not one; RAM buffers and SSD. The incoming bio is split into 4KB chunks at the virtual make_request and are NOT directly remapped to the SSD. As you mentioned, if I designed so, many update on the metadata happens. That's really bad since SSD is very bad at small update. Actually, the 4KB bio is first stored in RAM buffer, which is 512KB large. There are (512-4)/4=127 4KB bio data stored in the RAM buffer and 4KB metadata section at the head is made after that. The RAM buffer is now called "log" and as you mentioned, flushed to the SSD as 512KB sequential write. This definitely maximizes throughput and lifetime. Unfortunately, this is not always the case because of barrier request handlings. But, when the writes is really heavy (e.g. massive dirty page writeback), Writeboost works as above. > The good thing is that it seems writeboost doesn't use garbage > collection to clean old invalid logs, this will avoid the double > garage collection effect other caching module has, which essentially > both caching module and internal SSD will perform garbage collections > twice. Yes. And I believe SSDs can remove wear-leveling if I used it as fairly sequential. Am I right? Indeed, Writeboost is really SSD frinedly. > And one question, how long will be data logs replay time during init, > if SSD is almost full of dirty data logs? Sorry, I don't have a data now but it's slow as you may imagine. I will measure the time on later. The major reason is, it needs to read full 512KB segment to calculate checksum to know if the log isn't half written. (Read 500GB SSD that performs 500MB/sec seqread spends 1000secs) I think making the procedure done in parallel to exploit the full internal parallelism inside SSD may improve performance but it's just the matter of coefficient down from 1 to 1/n. Definitely, Writeboost isn't fit for a machine that needs reboot frequently (e.g. desktop). There is a way to reduce the init time. We can dump "what is the latest log written back" on the superblock. This can skip readings that aren't essential. The corresponding code is replay_log_on_cache() function. Please read if you are interested. Thanks, - Akira On 12/13/14 3:45 PM, Jianjian Huo wrote: > If I understand it correctly, the whole idea indeed is very simple, > the consumer/provider and circular buffer model. use SSD as a circular > write buffer, write flush thread stores incoming writes to this buffer > sequentially as provider, and writeback thread write those data logs > sequentially into backing device as consumer. > > If writeboost can do that without any random writes, then probably it > can save SSD/FTL of doing a lot of dirty jobs, and utilize the faster > sequential read/write performance from SSD. That'll be awesome. > However, I saw every data log segment in its design has meta data > header, like dirty_bits, so I guess writeboost has to randomly write > those data headers of stored data logs in SSD; also, splitting all bio > into 4KB will hurt its ability to get max raw SSD throughput, modern > NAND Flash has pages much bigger than 4KB; so overall I think the > actual benefits writeboost gets from this design will be discounted. > > The good thing is that it seems writeboost doesn't use garbage > collection to clean old invalid logs, this will avoid the double > garage collection effect other caching module has, which essentially > both caching module and internal SSD will perform garbage collections > twice. > > And one question, how long will be data logs replay time during init, > if SSD is almost full of dirty data logs? > > Jianjian > > On Fri, Dec 12, 2014 at 7:09 AM, Akira Hayakawa wrote: >>> However, after looking at the current code, and using it I think it's >>> a long, long way from being ready for production. As we've already >>> discussed there are some very naive design decisions in there, such as
Re: [PATCH] staging: lustre: fix sparse warnings related to lock context imbalance
> >>> Don't hide "implementation of locks" in functions like this, it only > >>> causes problems. This code has layers of layers of layers of > >>> abstractions due to it wanting to be originally ported to other > >>> operating systems and lots of different kernel versions of Linux itself. > >>> Unwinding and removing those layers is a good thing to do, don't paper > >>> over the nonsense by putting sparse markings on pointless functions. > >>> > >>> thanks, > >>> > >>> greg k-h > >> > >> > >> Cheers, Andreas > > > > Hello, > > > > Thanks for your answer. The annotations look like the best thing to do? > > Yes, the annotations still make sense. > > Cheers, Andreas Hello guys, I believe this patch is still relevant, and can be applied against next-20141212. -- Cheers, Loïc ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: goldfish: Fix minor coding style
Hello,
The convention for checking for NULL pointers is !ptr and not ptr == NULL.
This patch fixes such occurences in goldfish driver, it applies against
next-20141212
Signed-off-by: Loic Pefferkorn
---
drivers/staging/goldfish/goldfish_audio.c | 8
drivers/staging/goldfish/goldfish_nand.c | 10 +-
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/goldfish/goldfish_audio.c
b/drivers/staging/goldfish/goldfish_audio.c
index f200359..7ab034b 100644
--- a/drivers/staging/goldfish/goldfish_audio.c
+++ b/drivers/staging/goldfish/goldfish_audio.c
@@ -273,19 +273,19 @@ static int goldfish_audio_probe(struct platform_device
*pdev)
dma_addr_t buf_addr;
data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
- if (data == NULL)
+ if (!data)
return -ENOMEM;
spin_lock_init(&data->lock);
init_waitqueue_head(&data->wait);
platform_set_drvdata(pdev, data);
r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (r == NULL) {
+ if (!r) {
dev_err(&pdev->dev, "platform_get_resource failed\n");
return -ENODEV;
}
data->reg_base = devm_ioremap(&pdev->dev, r->start, PAGE_SIZE);
- if (data->reg_base == NULL)
+ if (!data->reg_base)
return -ENOMEM;
data->irq = platform_get_irq(pdev, 0);
@@ -295,7 +295,7 @@ static int goldfish_audio_probe(struct platform_device
*pdev)
}
data->buffer_virt = dmam_alloc_coherent(&pdev->dev,
COMBINED_BUFFER_SIZE, &buf_addr, GFP_KERNEL);
- if (data->buffer_virt == NULL) {
+ if (!data->buffer_virt) {
dev_err(&pdev->dev, "allocate buffer failed\n");
return -ENOMEM;
}
diff --git a/drivers/staging/goldfish/goldfish_nand.c
b/drivers/staging/goldfish/goldfish_nand.c
index d68f216..8e8e594 100644
--- a/drivers/staging/goldfish/goldfish_nand.c
+++ b/drivers/staging/goldfish/goldfish_nand.c
@@ -48,7 +48,7 @@ static u32 goldfish_nand_cmd_with_params(struct mtd_info *mtd,
struct cmd_params *cps = nand->cmd_params;
unsigned char __iomem *base = nand->base;
- if (cps == NULL)
+ if (!cps)
return -1;
switch (cmd) {
@@ -330,7 +330,7 @@ static int goldfish_nand_init_device(struct platform_device
*pdev,
mtd->priv = nand;
name = devm_kzalloc(&pdev->dev, name_len + 1, GFP_KERNEL);
- if (name == NULL)
+ if (!name)
return -ENOMEM;
mtd->name = name;
@@ -379,11 +379,11 @@ static int goldfish_nand_probe(struct platform_device
*pdev)
unsigned char __iomem *base;
r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (r == NULL)
+ if (!r)
return -ENODEV;
base = devm_ioremap(&pdev->dev, r->start, PAGE_SIZE);
- if (base == NULL)
+ if (!base)
return -ENOMEM;
version = readl(base + NAND_VERSION);
@@ -399,7 +399,7 @@ static int goldfish_nand_probe(struct platform_device *pdev)
nand = devm_kzalloc(&pdev->dev, sizeof(*nand) +
sizeof(struct mtd_info) * num_dev, GFP_KERNEL);
- if (nand == NULL)
+ if (!nand)
return -ENOMEM;
mutex_init(&nand->lock);
--
2.1.3
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: goldfish: Fix minor coding style
Loic,
On Sat, Dec 13, 2014 at 05:29:26PM +0100, Loic Pefferkorn wrote:
> Hello,
>
> The convention for checking for NULL pointers is !ptr and not ptr == NULL.
> This patch fixes such occurences in goldfish driver, it applies against
> next-20141212
>
Whose convention is this? I can't find any mention in
Documention/CodingStyle. checkpatch.pl doesn't complain about them.
And there are almost three thousand examples in staging which don't
use this convention.
linux-next$ grep -r "== NULL" drivers/staging/* | wc -l
2844
>
> Signed-off-by: Loic Pefferkorn
> ---
> drivers/staging/goldfish/goldfish_audio.c | 8
> drivers/staging/goldfish/goldfish_nand.c | 10 +-
> 2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/goldfish/goldfish_audio.c
> b/drivers/staging/goldfish/goldfish_audio.c
> index f200359..7ab034b 100644
> --- a/drivers/staging/goldfish/goldfish_audio.c
> +++ b/drivers/staging/goldfish/goldfish_audio.c
> @@ -273,19 +273,19 @@ static int goldfish_audio_probe(struct platform_device
> *pdev)
> dma_addr_t buf_addr;
>
> data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> - if (data == NULL)
> + if (!data)
> return -ENOMEM;
> spin_lock_init(&data->lock);
> init_waitqueue_head(&data->wait);
> platform_set_drvdata(pdev, data);
>
> r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - if (r == NULL) {
> + if (!r) {
> dev_err(&pdev->dev, "platform_get_resource failed\n");
> return -ENODEV;
> }
> data->reg_base = devm_ioremap(&pdev->dev, r->start, PAGE_SIZE);
> - if (data->reg_base == NULL)
> + if (!data->reg_base)
> return -ENOMEM;
>
[...]
--
- Jeremiah Mahler
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: goldfish: Fix minor coding style
> Whose convention is this? I can't find any mention in > Documention/CodingStyle. checkpatch.pl doesn't complain about them. > And there are almost three thousand examples in staging which don't > use this convention. > > linux-next$ grep -r "== NULL" drivers/staging/* | wc -l > 2844 Hi Jeremiah, Thanks for your feedback. I have used checkpatch.pl with the --strict flag: $ ./scripts/checkpatch.pl --strict -f drivers/staging/goldfish/goldfish_nand.c CHECK: Comparison to NULL could be written "!cps" #51: FILE: drivers/staging/goldfish/goldfish_nand.c:51: + if (cps == NULL) CHECK: Comparison to NULL could be written "!name" #333: FILE: drivers/staging/goldfish/goldfish_nand.c:333: + if (name == NULL) CHECK: Comparison to NULL could be written "!r" #382: FILE: drivers/staging/goldfish/goldfish_nand.c:382: + if (r == NULL) CHECK: Comparison to NULL could be written "!base" #386: FILE: drivers/staging/goldfish/goldfish_nand.c:386: + if (base == NULL) CHECK: Comparison to NULL could be written "!nand" #402: FILE: drivers/staging/goldfish/goldfish_nand.c:402: + if (nand == NULL) total: 0 errors, 0 warnings, 5 checks, 442 lines checked drivers/staging/goldfish/goldfish_nand.c has style problems, please review. I have also found another commit having the same purpose: 7f376cd6dc1c9bfd14514c70765e6900a961c4b8 -- Cheers, Loïc ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: goldfish: Fix minor coding style
On Sat, 13 Dec 2014 17:29:26 +0100 Loic Pefferkorn wrote: > Hello, > > The convention for checking for NULL pointers is !ptr and not ptr == NULL. > This patch fixes such occurences in goldfish driver, it applies against > next-20141212 > > > Signed-off-by: Loic Pefferkorn Pointless churn. It makes it less readable if anything, and it removes the type safety as you are now checking against 0 not (void *)0 NAK Alan ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: goldfish: Fix minor coding style
Loïc, On Sat, Dec 13, 2014 at 07:22:38PM +0100, Loic Pefferkorn wrote: > > Whose convention is this? I can't find any mention in > > Documention/CodingStyle. checkpatch.pl doesn't complain about them. > > And there are almost three thousand examples in staging which don't > > use this convention. > > > > linux-next$ grep -r "== NULL" drivers/staging/* | wc -l > > 2844 > > Hi Jeremiah, > > Thanks for your feedback. > > I have used checkpatch.pl with the --strict flag: > > $ ./scripts/checkpatch.pl --strict -f drivers/staging/goldfish/goldfish_nand.c > CHECK: Comparison to NULL could be written "!cps" > #51: FILE: drivers/staging/goldfish/goldfish_nand.c:51: > + if (cps == NULL) > > CHECK: Comparison to NULL could be written "!name" > #333: FILE: drivers/staging/goldfish/goldfish_nand.c:333: > + if (name == NULL) > > CHECK: Comparison to NULL could be written "!r" > #382: FILE: drivers/staging/goldfish/goldfish_nand.c:382: > + if (r == NULL) > > CHECK: Comparison to NULL could be written "!base" > #386: FILE: drivers/staging/goldfish/goldfish_nand.c:386: > + if (base == NULL) > > CHECK: Comparison to NULL could be written "!nand" > #402: FILE: drivers/staging/goldfish/goldfish_nand.c:402: > + if (nand == NULL) > > total: 0 errors, 0 warnings, 5 checks, 442 lines checked > > drivers/staging/goldfish/goldfish_nand.c has style problems, please review. > > I have also found another commit having the same purpose: > 7f376cd6dc1c9bfd14514c70765e6900a961c4b8 > > -- > Cheers, > Loïc It looks like you're right. I must say I am surprised. I had no idea checkpatch.pl could be even more pedantic than it already is :-) -- - Jeremiah Mahler ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: goldfish: Fix minor coding style
On Sat, Dec 13, 2014 at 07:07:05PM +, One Thousand Gnomes wrote: > > Pointless churn. It makes it less readable if anything, and it removes > the type safety as you are now checking against 0 not (void *)0 > > NAK > > Alan The type safety is an interesting point I was not aware of. Just in case, do you have something for me on your todo list? -- Cheers, Loïc ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [dm-devel] [PATCH v2] staging: writeboost: Add dm-writeboost
Hi, > The major reason is, it needs to read full 512KB segment to calculate > checksum to > know if the log isn't half written. > (Read 500GB SSD that performs 500MB/sec seqread spends 1000secs) I've just measured how long the cache resuming is. I use 2GB SSD for the cache device. 512KB seqread over the cache device: 8.252sec (242MB/sec) Resume when all caches are dirty: 10.339sec Typically, if you use 128GB SSD it will be 5-10 minutes. As I predicted, resuming time is close to seqread. In other words, it's fully IO-bound. (If you read the code you will notice that it first searchs for the oldest log as the starting point. It's 4KB metadata reads but spends to some extent. The other 2 sec is thought to be spent by this) - Akira On 12/13/14 11:07 PM, Akira Hayakawa wrote: > Hi, > > Jianjian, You really get a point at the fundamental design. > >> If I understand it correctly, the whole idea indeed is very simple, >> the consumer/provider and circular buffer model. use SSD as a circular >> write buffer, write flush thread stores incoming writes to this buffer >> sequentially as provider, and writeback thread write those data logs >> sequentially into backing device as consumer. >> >> If writeboost can do that without any random writes, then probably it >> can save SSD/FTL of doing a lot of dirty jobs, and utilize the faster >> sequential read/write performance from SSD. That'll be awesome. >> However, I saw every data log segment in its design has meta data >> header, like dirty_bits, so I guess writeboost has to randomly write >> those data headers of stored data logs in SSD; also, splitting all bio >> into 4KB will hurt its ability to get max raw SSD throughput, modern >> NAND Flash has pages much bigger than 4KB; so overall I think the >> actual benefits writeboost gets from this design will be discounted. > You understand *almost* correctly. > > Writeboost has two circular buffers, not one; RAM buffers and SSD. > The incoming bio is split into 4KB chunks at the virtual make_request > and are NOT directly remapped to the SSD. > As you mentioned, if I designed so, many update on the metadata happens. > That's really bad since SSD is very bad at small update. > > Actually, the 4KB bio is first stored in RAM buffer, which is 512KB large. > There are (512-4)/4=127 4KB bio data stored in the RAM buffer and 4KB metadata > section at the head is made after that. > > The RAM buffer is now called "log" and as you mentioned, flushed to the SSD > as 512KB sequential write. This definitely maximizes throughput and lifetime. > > Unfortunately, this is not always the case because of barrier request > handlings. > But, when the writes is really heavy (e.g. massive dirty page writeback), > Writeboost works as above. > >> The good thing is that it seems writeboost doesn't use garbage >> collection to clean old invalid logs, this will avoid the double >> garage collection effect other caching module has, which essentially >> both caching module and internal SSD will perform garbage collections >> twice. > Yes. And I believe SSDs can remove wear-leveling if I used it as fairly > sequential. > Am I right? Indeed, Writeboost is really SSD frinedly. > >> And one question, how long will be data logs replay time during init, >> if SSD is almost full of dirty data logs? > Sorry, I don't have a data now but it's slow as you may imagine. > I will measure the time on later. > > The major reason is, it needs to read full 512KB segment to calculate > checksum to > know if the log isn't half written. > (Read 500GB SSD that performs 500MB/sec seqread spends 1000secs) > I think making the procedure done in parallel to exploit the full internal > parallelism > inside SSD may improve performance but it's just the matter of coefficient > down from 1 to 1/n. > Definitely, Writeboost isn't fit for a machine that needs reboot frequently > (e.g. desktop). > > There is a way to reduce the init time. We can dump "what is the latest log > written back" > on the superblock. This can skip readings that aren't essential. > > The corresponding code is replay_log_on_cache() function. Please read if you > are > interested. > > Thanks, > > - Akira > > On 12/13/14 3:45 PM, Jianjian Huo wrote: >> If I understand it correctly, the whole idea indeed is very simple, >> the consumer/provider and circular buffer model. use SSD as a circular >> write buffer, write flush thread stores incoming writes to this buffer >> sequentially as provider, and writeback thread write those data logs >> sequentially into backing device as consumer. >> >> If writeboost can do that without any random writes, then probably it >> can save SSD/FTL of doing a lot of dirty jobs, and utilize the faster >> sequential read/write performance from SSD. That'll be awesome. >> However, I saw every data log segment in its design has meta data >> header, like dirty_bits, so I guess writeboost has to randomly write >> those data headers of stored data logs in SS
Re: [dm-devel] [PATCH v2] staging: writeboost: Add dm-writeboost
On Sat, Dec 13, 2014 at 6:07 AM, Akira Hayakawa wrote: > Hi, > > Jianjian, You really get a point at the fundamental design. > >> If I understand it correctly, the whole idea indeed is very simple, >> the consumer/provider and circular buffer model. use SSD as a circular >> write buffer, write flush thread stores incoming writes to this buffer >> sequentially as provider, and writeback thread write those data logs >> sequentially into backing device as consumer. >> >> If writeboost can do that without any random writes, then probably it >> can save SSD/FTL of doing a lot of dirty jobs, and utilize the faster >> sequential read/write performance from SSD. That'll be awesome. >> However, I saw every data log segment in its design has meta data >> header, like dirty_bits, so I guess writeboost has to randomly write >> those data headers of stored data logs in SSD; also, splitting all bio >> into 4KB will hurt its ability to get max raw SSD throughput, modern >> NAND Flash has pages much bigger than 4KB; so overall I think the >> actual benefits writeboost gets from this design will be discounted. > You understand *almost* correctly. > > Writeboost has two circular buffers, not one; RAM buffers and SSD. > The incoming bio is split into 4KB chunks at the virtual make_request > and are NOT directly remapped to the SSD. > As you mentioned, if I designed so, many update on the metadata happens. > That's really bad since SSD is very bad at small update. > > Actually, the 4KB bio is first stored in RAM buffer, which is 512KB large. > There are (512-4)/4=127 4KB bio data stored in the RAM buffer and 4KB metadata > section at the head is made after that. > > The RAM buffer is now called "log" and as you mentioned, flushed to the SSD > as 512KB sequential write. This definitely maximizes throughput and lifetime. > > Unfortunately, this is not always the case because of barrier request > handlings. > But, when the writes is really heavy (e.g. massive dirty page writeback), > Writeboost works as above. > How about invalidating previous writes on same sector address? if first write is stored in one 512KB log in SSD, later when user write the same address, will writeboost invalid old write by updating meta data header in place in that 512KB log? and other meta data like superblock, will writeboost will update them in place? if writeboost has those frequent random in-place updates, probably those benefits except utilizing the faster sequential writes will be much discounted. >> The good thing is that it seems writeboost doesn't use garbage >> collection to clean old invalid logs, this will avoid the double >> garage collection effect other caching module has, which essentially >> both caching module and internal SSD will perform garbage collections >> twice. > Yes. And I believe SSDs can remove wear-leveling if I used it as fairly > sequential. > Am I right? Indeed, Writeboost is really SSD frinedly. > Even if writeboost write everything sequentially, probably SSD won't be able to recognize that and do its own wear-leveling whatsoever. It will be easier, since there is no need to move cold data, etc. Generally, SSDs vary a lot, IMHO, one certain optimization technique may work for certain model, but may not work for others, since they internal NAND Flash management algorithms can be very different. >> And one question, how long will be data logs replay time during init, >> if SSD is almost full of dirty data logs? > Sorry, I don't have a data now but it's slow as you may imagine. > I will measure the time on later. > > The major reason is, it needs to read full 512KB segment to calculate > checksum to > know if the log isn't half written. > (Read 500GB SSD that performs 500MB/sec seqread spends 1000secs) > I think making the procedure done in parallel to exploit the full internal > parallelism > inside SSD may improve performance but it's just the matter of coefficient > down from 1 to 1/n. > Definitely, Writeboost isn't fit for a machine that needs reboot frequently > (e.g. desktop). > > There is a way to reduce the init time. We can dump "what is the latest log > written back" > on the superblock. This can skip readings that aren't essential. > > The corresponding code is replay_log_on_cache() function. Please read if you > are > interested. > > Thanks, > > - Akira > > On 12/13/14 3:45 PM, Jianjian Huo wrote: >> If I understand it correctly, the whole idea indeed is very simple, >> the consumer/provider and circular buffer model. use SSD as a circular >> write buffer, write flush thread stores incoming writes to this buffer >> sequentially as provider, and writeback thread write those data logs >> sequentially into backing device as consumer. >> >> If writeboost can do that without any random writes, then probably it >> can save SSD/FTL of doing a lot of dirty jobs, and utilize the faster >> sequential read/write performance from SSD. That'll be awesome. >> However, I saw every data log segment in its design
Re: [PATCH v2] staging: writeboost: Add dm-writeboost
Hi, I've just measured how split affects. I think seqread can make the discussion solid so these are the cases of reading 6.4GB (64MB * 100) sequentially. HDD: 64MB read real 2m1.191s user 0m0.000s sys 0m0.470s Writeboost (HDD+SSD): 64MB read real 2m13.532s user 0m0.000s sys 0m28.740s The splitting actually affects to some extent (2m1 -> 2m13 is 10% loss). But not too big if we consider the typical workload is NOT seqreads (if so, the user shouldn't use SSD caching). Splitting bio into 4KB chunks makes the cache lookup and locking simple and this contributes to the performance of both write and read is the fact, don't miss it. Without this, especially, writes isn't so fast in Writeboost but rather loses its charms. Since simple and fast is the ideal for any softwares. I am really unwilling to change this fundamental design; splitting. But, an idea of selective splitting can be proposed for future enhancement. Add a layer so that a target can choose if it needs splitting or not may be interesting. I think Writeboost can bypass big writes/reads at the cost of duplicated cache lookup. Can DM-cache also benefit from this extension? Conceptually, it's like this before: bio -> ~map:bio->bio after: bio -> ~should_split:bio->bool -> ~map:bio->bio - Akira On 12/13/14 12:09 AM, Akira Hayakawa wrote: >> However, after looking at the current code, and using it I think it's >> a long, long way from being ready for production. As we've already >> discussed there are some very naive design decisions in there, such as >> copying every bio payload to another memory buffer, splitting all io >> down to 4k. Think about the cpu overhead and memory consumption! >> Think about how it will perform when memory is constrained and it >> can't allocate many of those rambufs! I'm sure more issues will be >> found if I read further. > These decisions are made based on measurement. They are not naive. > I am a man who dislikes performance optimization without measurement. > As a result, I regard things brought by the simplicity much important > than what's from other design decisions possible. > > About the CPU consumption, > the average CPU consumption while performing random write fio > with consumer level SSD is only 3% or so, > which is 5 times efficient than bcache per iops. > > With RAM-backed cache device, it reaches about 1.5GB/sec throughput. > Even in this case the CPU consumption is only 12%. > Please see this post, > http://www.redhat.com/archives/dm-devel/2014-February/msg0.html > > I don't think the CPU consumption is small enough to ignore. > > About the memory consumption, > you seem to misunderstand the fact. > The rambufs are not dynamically allocated but statically. > The default amount is 8MB and this is usually not to argue. > >> Mike raised the question of why you want this in the kernel so much? >> You'd find none of the distros would support it; so it doesn't widen >> your audience much. It's far better for you to maintain it outside of >> the kernel at this point. Any users will be bold, adventurous people, >> who will be quite capable of building a kernel module. > Some people deploy Writeboost in their daily use. > The sound of "log-structured" seems to easily attract storage guys' attention. > If this driver is merged into upstream, I think it gains many audience and > thus feedback. > When my driver was introduced by Phoronix before, it actually drew attentions. > They must wait for Writeboost become available in upstream. > http://www.phoronix.com/scan.php?page=news_item&px=MTQ1Mjg > >> I'm sorry to have disappointed you so, but if I let this go upstream >> it would mean a massive amount of support work for me, not to mention >> a damaged reputation for dm. > If you read the code further, you will find how simple the mechanism is. > Not to mention the code itself is. > > - Akira > > On 12/12/14 11:24 PM, Joe Thornber wrote: >> On Fri, Dec 12, 2014 at 09:42:15AM +0900, Akira Hayakawa wrote: >>> The SSD-caching should be log-structured. >> >> No argument there, and this is why I've supported you with >> dm-writeboost over the last couple of years. >> >> However, after looking at the current code, and using it I think it's >> a long, long way from being ready for production. As we've already >> discussed there are some very naive design decisions in there, such as >> copying every bio payload to another memory buffer, splitting all io >> down to 4k. Think about the cpu overhead and memory consumption! >> Think about how it will perform when memory is constrained and it >> can't allocate many of those rambufs! I'm sure more issues will be >> found if I read further. >> >> I'm sorry to have disappointed you so, but if I let this go upstream >> it would mean a massive amount of support work for me, not to mention >> a damaged reputation for dm. >> >> Mike raised the question of why you want this in the kernel so much? >> You'd find none of the distros would
Re: [dm-devel] [PATCH v2] staging: writeboost: Add dm-writeboost
Jianjian, > How about invalidating previous writes on same sector address? if > first write is stored in one 512KB log in SSD, later when user write > the same address, will writeboost invalid old write by updating meta > data header in place in that 512KB log? and other meta data like > superblock, will writeboost will update them in place? if writeboost > has those frequent random in-place updates, probably those benefits > except utilizing the faster sequential writes will be much discounted. In runtime, Writeboost has equivalent metadata on the memory as hash table. This makes not only cache lookup fast but also makes invalidation easy. Without this, Writeboost's speed is really discounted. Any logs on the cache device are never updated partially (it's too dangerous if we consider partial write failure). Durability is another benefit of log-structured caching. As for writing on superblock, it's really random. But, we don't need to update superblock every write but every seconds or so. To begin with, it's not logically essential so you can turn it off (that's the default). The option is for users much care for the resume time. > Even if writeboost write everything sequentially, probably SSD won't > be able to recognize that and do its own wear-leveling whatsoever. It > will be easier, since there is no need to move cold data, etc. > > Generally, SSDs vary a lot, IMHO, one certain optimization technique > may work for certain model, but may not work for others, since they > internal NAND Flash management algorithms can be very different. Yes, Thanks. I believe making the SSD-internal works easy can not only stabilize the performance but also reduce the possibility of being trapped by some bugs that the SSD provides such as petit freeze. - Akira On 12/14/14 11:46 AM, Jianjian Huo wrote: > On Sat, Dec 13, 2014 at 6:07 AM, Akira Hayakawa wrote: >> Hi, >> >> Jianjian, You really get a point at the fundamental design. >> >>> If I understand it correctly, the whole idea indeed is very simple, >>> the consumer/provider and circular buffer model. use SSD as a circular >>> write buffer, write flush thread stores incoming writes to this buffer >>> sequentially as provider, and writeback thread write those data logs >>> sequentially into backing device as consumer. >>> >>> If writeboost can do that without any random writes, then probably it >>> can save SSD/FTL of doing a lot of dirty jobs, and utilize the faster >>> sequential read/write performance from SSD. That'll be awesome. >>> However, I saw every data log segment in its design has meta data >>> header, like dirty_bits, so I guess writeboost has to randomly write >>> those data headers of stored data logs in SSD; also, splitting all bio >>> into 4KB will hurt its ability to get max raw SSD throughput, modern >>> NAND Flash has pages much bigger than 4KB; so overall I think the >>> actual benefits writeboost gets from this design will be discounted. >> You understand *almost* correctly. >> >> Writeboost has two circular buffers, not one; RAM buffers and SSD. >> The incoming bio is split into 4KB chunks at the virtual make_request >> and are NOT directly remapped to the SSD. >> As you mentioned, if I designed so, many update on the metadata happens. >> That's really bad since SSD is very bad at small update. >> >> Actually, the 4KB bio is first stored in RAM buffer, which is 512KB large. >> There are (512-4)/4=127 4KB bio data stored in the RAM buffer and 4KB >> metadata >> section at the head is made after that. >> >> The RAM buffer is now called "log" and as you mentioned, flushed to the SSD >> as 512KB sequential write. This definitely maximizes throughput and lifetime. >> >> Unfortunately, this is not always the case because of barrier request >> handlings. >> But, when the writes is really heavy (e.g. massive dirty page writeback), >> Writeboost works as above. >> > How about invalidating previous writes on same sector address? if > first write is stored in one 512KB log in SSD, later when user write > the same address, will writeboost invalid old write by updating meta > data header in place in that 512KB log? and other meta data like > superblock, will writeboost will update them in place? if writeboost > has those frequent random in-place updates, probably those benefits > except utilizing the faster sequential writes will be much discounted. > >>> The good thing is that it seems writeboost doesn't use garbage >>> collection to clean old invalid logs, this will avoid the double >>> garage collection effect other caching module has, which essentially >>> both caching module and internal SSD will perform garbage collections >>> twice. >> Yes. And I believe SSDs can remove wear-leveling if I used it as fairly >> sequential. >> Am I right? Indeed, Writeboost is really SSD frinedly. >> > Even if writeboost write everything sequentially, probably SSD won't > be able to recognize that and do its own wear-leveling whatsoever. It > will be easier
