Re: [patch v3]DM: dm-insitu-comp: a compressed DM target for SSD
On Tue, Mar 18 2014 at 9:45pm -0400, Shaohua Li wrote: > On Tue, Mar 18, 2014 at 05:28:43PM -0400, Mike Snitzer wrote: > > On Tue, Mar 18 2014 at 3:41am -0400, > > Shaohua Li wrote: > > > > > On Mon, Mar 17, 2014 at 04:00:40PM -0400, Mike Snitzer wrote: > > > > > > > > I folded your changes in, and then committed a patch ontop that cleans > > > > some code up. But added 2 FIXMEs that still speak to pretty fundamental > > > > problems with the architecture of the dm-insitu-comp target, see: > > > > https://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/commit/?h=for-3.15-insitu-comp&id=8565ab6b04837591d03c94851c2f9f9162ce12f4 > > > > > > > > Unfortunately the single insitu_comp_wq workqueue that all insitu-comp > > > > targets are to share isn't a workable solution. Each target needs to > > > > have resource isolation from other targets (imagine insitu-comp used for > > > > multiple SSDs). This is important for suspend too because you'll need > > > > to flush/stop the workqueue. > > > > > > Is this just because of suspend? I didn't see fundamental reason why the > > > workqueue can't be shared even for several targets. > > > > I'm not seeing how you are guaranteeing that all queued work is > > completed during suspend. insitu_comp_queue_req() just calls > > queue_work_on(). > > > > BTW, queue_work_on()'s comment above its implementation says: > > "We queue the work to a specific CPU, the caller must ensure it can't go > > away." -- you're not able to insure a cpu isn't hotplugged so... but I > > also see you've used it in your raid5 perf improvement changes so you > > obviously have experience with using this interface. > > Good point, I did miss this. the raid5 case hold a lock, while this case > doesn't. A fix is attached below. I've pushed it to the branch. > > > > You introduced a state machine for tracking suspending, suspended, > > > > resumed. This really isn't necessary. During suspend you need to > > > > flush_workqueue(). On resume you shouldn't need to do anything special. > > > > > > > > As I noted in the commit, the thin and cache targets can serve as > > > > references for how you can manage the workqueue across suspend/resume > > > > and the lifetime of these workqueues relative to .ctr and .dtr. > > > > > > As far as I checking the code, .postsuspend is called after all requests > > > are > > > finished. This already guarantees no pending requests running in > > > insitu-comp > > > workqueue. > > > > I could easily be missing something obvious, but I don't see where that > > guarantee is implemented. > > Alright, so this is the divergence. dm_suspend() calls > dm_wait_for_completion() > and then dm_table_postsuspend_targets(). As far as I understand, > dm_wait_for_completion() will wait all pending requests to finish. The > comments > declaim this too. Am I missing anything? > > Basically the two kinds of IO. IO requests from upper layer, which > dm_wait_for_completion() will guarantee they are finished. Metadata IO > requests, which .postsuspend should make sure they are finished. OK, I see. You don't have the notion of a transaction, that I can see, so this this begs the question: what kind of crash consistency/recovery are you providing? Seems that the metadata writeback support is allowing for more potential for lost data on a crash. Also, it isn't clear how you're coping with the potential for a crash while updating a extent (when metadata bits are borrowed from the tail, etc). Without transaction a block (or extent of blocks) could be partially updated, how are you guaranteeing the corresponding data is either entirely old or new? The compressed nature of this data makes a requirement for atomic updates to occur here. Are you somehow leveraging Fusion-io SSD to provide such guarantee? So effectively there are concerns about data integrity of this target that need answering. Unfortunately I'm running low on time I can dedicate to continued review of this target and need to transition to other priorities. > > > Doing a workqueue flush isn't required. The writeback thread is > > > running in background and waiting for requests completion can't guarantee > > > the > > > thread isn't running, so we must make sure it is safely parked. > > > > Sure, but you don't need a state machine to do that. The DM core takes > > care of calling these hooks, so you just need to stop the writeback > > thread during suspend and (re)start/kick it on resume (preresume). > > Yep, I need wait the writeback thread finish all pending metadata IO, the > state > machine works well here. I see what you've done, I get that you're using the state machine to wait but I still contend it isn't needed. Like I said before, just stop writeback in suspend and restart on resume. No state is needed. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.or
Re: [patch v3]DM: dm-insitu-comp: a compressed DM target for SSD
On Tue, Mar 18, 2014 at 05:28:43PM -0400, Mike Snitzer wrote:
> On Tue, Mar 18 2014 at 3:41am -0400,
> Shaohua Li wrote:
>
> > On Mon, Mar 17, 2014 at 04:00:40PM -0400, Mike Snitzer wrote:
> > >
> > > I folded your changes in, and then committed a patch ontop that cleans
> > > some code up. But added 2 FIXMEs that still speak to pretty fundamental
> > > problems with the architecture of the dm-insitu-comp target, see:
> > > https://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/commit/?h=for-3.15-insitu-comp&id=8565ab6b04837591d03c94851c2f9f9162ce12f4
> > >
> > > Unfortunately the single insitu_comp_wq workqueue that all insitu-comp
> > > targets are to share isn't a workable solution. Each target needs to
> > > have resource isolation from other targets (imagine insitu-comp used for
> > > multiple SSDs). This is important for suspend too because you'll need
> > > to flush/stop the workqueue.
> >
> > Is this just because of suspend? I didn't see fundamental reason why the
> > workqueue can't be shared even for several targets.
>
> I'm not seeing how you are guaranteeing that all queued work is
> completed during suspend. insitu_comp_queue_req() just calls
> queue_work_on().
>
> BTW, queue_work_on()'s comment above its implementation says:
> "We queue the work to a specific CPU, the caller must ensure it can't go
> away." -- you're not able to insure a cpu isn't hotplugged so... but I
> also see you've used it in your raid5 perf improvement changes so you
> obviously have experience with using this interface.
Good point, I did miss this. the raid5 case hold a lock, while this case
doesn't. A fix is attached below.
> > > You introduced a state machine for tracking suspending, suspended,
> > > resumed. This really isn't necessary. During suspend you need to
> > > flush_workqueue(). On resume you shouldn't need to do anything special.
> > >
> > > As I noted in the commit, the thin and cache targets can serve as
> > > references for how you can manage the workqueue across suspend/resume
> > > and the lifetime of these workqueues relative to .ctr and .dtr.
> >
> > As far as I checking the code, .postsuspend is called after all requests are
> > finished. This already guarantees no pending requests running in insitu-comp
> > workqueue.
>
> I could easily be missing something obvious, but I don't see where that
> guarantee is implemented.
Alright, so this is the divergence. dm_suspend() calls dm_wait_for_completion()
and then dm_table_postsuspend_targets(). As far as I understand,
dm_wait_for_completion() will wait all pending requests to finish. The comments
declaim this too. Am I missing anything?
Basically the two kinds of IO. IO requests from upper layer, which
dm_wait_for_completion() will guarantee they are finished. Metadata IO
requests, which .postsuspend should make sure they are finished.
> > Doing a workqueue flush isn't required. The writeback thread is
> > running in background and waiting for requests completion can't guarantee
> > the
> > thread isn't running, so we must make sure it is safely parked.
>
> Sure, but you don't need a state machine to do that. The DM core takes
> care of calling these hooks, so you just need to stop the writeback
> thread during suspend and (re)start/kick it on resume (preresume).
Yep, I need wait the writeback thread finish all pending metadata IO, the state
machine works well here.
---
drivers/md/dm-insitu-comp.c |9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
Index: linux/drivers/md/dm-insitu-comp.c
===
--- linux.orig/drivers/md/dm-insitu-comp.c 2014-03-17 17:40:01.106660303
+0800
+++ linux/drivers/md/dm-insitu-comp.c 2014-03-19 08:49:12.314627050 +0800
@@ -704,14 +704,19 @@ static void insitu_comp_queue_req(struct
struct insitu_comp_req *req)
{
unsigned long flags;
- struct insitu_comp_io_worker *worker =
- &insitu_comp_io_workers[req->cpu];
+ struct insitu_comp_io_worker *worker;
+
+ preempt_disable();
+ if (!cpu_online(req->cpu))
+ req->cpu = cpumask_any(cpu_online_mask);
+ worker = &insitu_comp_io_workers[req->cpu];
spin_lock_irqsave(&worker->lock, flags);
list_add_tail(&req->sibling, &worker->pending);
spin_unlock_irqrestore(&worker->lock, flags);
queue_work_on(req->cpu, insitu_comp_wq, &worker->work);
+ preempt_enable();
}
static void insitu_comp_queue_req_list(struct insitu_comp_info *info,
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch v3]DM: dm-insitu-comp: a compressed DM target for SSD
On Tue, Mar 18 2014 at 3:41am -0400, Shaohua Li wrote: > On Mon, Mar 17, 2014 at 04:00:40PM -0400, Mike Snitzer wrote: > > > > I folded your changes in, and then committed a patch ontop that cleans > > some code up. But added 2 FIXMEs that still speak to pretty fundamental > > problems with the architecture of the dm-insitu-comp target, see: > > https://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/commit/?h=for-3.15-insitu-comp&id=8565ab6b04837591d03c94851c2f9f9162ce12f4 > > > > Unfortunately the single insitu_comp_wq workqueue that all insitu-comp > > targets are to share isn't a workable solution. Each target needs to > > have resource isolation from other targets (imagine insitu-comp used for > > multiple SSDs). This is important for suspend too because you'll need > > to flush/stop the workqueue. > > Is this just because of suspend? I didn't see fundamental reason why the > workqueue can't be shared even for several targets. I'm not seeing how you are guaranteeing that all queued work is completed during suspend. insitu_comp_queue_req() just calls queue_work_on(). BTW, queue_work_on()'s comment above its implementation says: "We queue the work to a specific CPU, the caller must ensure it can't go away." -- you're not able to insure a cpu isn't hotplugged so... but I also see you've used it in your raid5 perf improvement changes so you obviously have experience with using this interface. > > You introduced a state machine for tracking suspending, suspended, > > resumed. This really isn't necessary. During suspend you need to > > flush_workqueue(). On resume you shouldn't need to do anything special. > > > > As I noted in the commit, the thin and cache targets can serve as > > references for how you can manage the workqueue across suspend/resume > > and the lifetime of these workqueues relative to .ctr and .dtr. > > As far as I checking the code, .postsuspend is called after all requests are > finished. This already guarantees no pending requests running in insitu-comp > workqueue. I could easily be missing something obvious, but I don't see where that guarantee is implemented. > Doing a workqueue flush isn't required. The writeback thread is > running in background and waiting for requests completion can't guarantee the > thread isn't running, so we must make sure it is safely parked. Sure, but you don't need a state machine to do that. The DM core takes care of calling these hooks, so you just need to stop the writeback thread during suspend and (re)start/kick it on resume (preresume). -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch v3]DM: dm-insitu-comp: a compressed DM target for SSD
On Mon, Mar 17, 2014 at 04:00:40PM -0400, Mike Snitzer wrote: > On Mon, Mar 17 2014 at 5:56am -0400, > Shaohua Li wrote: > > > On Fri, Mar 14, 2014 at 06:44:45PM -0400, Mike Snitzer wrote: > > > On Fri, Mar 14 2014 at 5:40am -0400, > > > Shaohua Li wrote: > > > > > > > On Mon, Mar 10, 2014 at 09:52:56AM -0400, Mike Snitzer wrote: > > > > > On Fri, Mar 07 2014 at 2:57am -0500, > > > > > Shaohua Li wrote: > > > > > > > > > > > ping! > > > > > > > > > > Hi, > > > > > > > > > > I intend to get dm-insitu-comp reviewed for 3.15. Sorry I haven't > > > > > gotten back with you before now, been busy tending to 3.14-rc issues. > > > > > > > > > > I took a quick first pass over your code a couple weeks ago. Looks to > > > > > be in great shape relative to coding conventions and the more DM > > > > > specific conventions. Clearly demonstrates you have a good command of > > > > > DM concepts and quirks. > > > > > > Think I need to eat my words from above at least partially. Given you > > > haven't implemented any of the target suspend or resume hooks this > > > target will _not_ work properly across suspend + resume cycles that all > > > DM targets must support. > > > > > > But we can obviously work through it with urgency for 3.15. > > > > > > I've pulled your v3 patch into git and have overlayed edits from my > > > first pass. Lots of funky wrapping to conform to 80 columns. But > > > whitespace aside, I've added FIXME:s in the relevant files. If you work > > > on any of these FIXMEs please send follow-up patches so that we don't > > > step on each others' toes. > > > > > > Please see the 'for-3.15-insitu-comp' branch of this git repo: > > > git://git.kernel.org/pub/scm/linux/kernel/git/snitzer/linux.git > > > > Thanks for your to look at it. I fixed them against your tree. Please check > > below patch. > > I folded your changes in, and then committed a patch ontop that cleans > some code up. But added 2 FIXMEs that still speak to pretty fundamental > problems with the architecture of the dm-insitu-comp target, see: > https://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/commit/?h=for-3.15-insitu-comp&id=8565ab6b04837591d03c94851c2f9f9162ce12f4 > > Unfortunately the single insitu_comp_wq workqueue that all insitu-comp > targets are to share isn't a workable solution. Each target needs to > have resource isolation from other targets (imagine insitu-comp used for > multiple SSDs). This is important for suspend too because you'll need > to flush/stop the workqueue. Is this just because of suspend? I didn't see fundamental reason why the workqueue can't be shared even for several targets. > You introduced a state machine for tracking suspending, suspended, > resumed. This really isn't necessary. During suspend you need to > flush_workqueue(). On resume you shouldn't need to do anything special. > > As I noted in the commit, the thin and cache targets can serve as > references for how you can manage the workqueue across suspend/resume > and the lifetime of these workqueues relative to .ctr and .dtr. As far as I checking the code, .postsuspend is called after all requests are finished. This already guarantees no pending requests running in insitu-comp workqueue. Doing a workqueue flush isn't required. The writeback thread is running in background and waiting for requests completion can't guarantee the thread isn't running, so we must make sure it is safely parked. Thanks, Shaohua -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch v3]DM: dm-insitu-comp: a compressed DM target for SSD
On Mon, Mar 17 2014 at 5:56am -0400, Shaohua Li wrote: > On Fri, Mar 14, 2014 at 06:44:45PM -0400, Mike Snitzer wrote: > > On Fri, Mar 14 2014 at 5:40am -0400, > > Shaohua Li wrote: > > > > > On Mon, Mar 10, 2014 at 09:52:56AM -0400, Mike Snitzer wrote: > > > > On Fri, Mar 07 2014 at 2:57am -0500, > > > > Shaohua Li wrote: > > > > > > > > > ping! > > > > > > > > Hi, > > > > > > > > I intend to get dm-insitu-comp reviewed for 3.15. Sorry I haven't > > > > gotten back with you before now, been busy tending to 3.14-rc issues. > > > > > > > > I took a quick first pass over your code a couple weeks ago. Looks to > > > > be in great shape relative to coding conventions and the more DM > > > > specific conventions. Clearly demonstrates you have a good command of > > > > DM concepts and quirks. > > > > Think I need to eat my words from above at least partially. Given you > > haven't implemented any of the target suspend or resume hooks this > > target will _not_ work properly across suspend + resume cycles that all > > DM targets must support. > > > > But we can obviously work through it with urgency for 3.15. > > > > I've pulled your v3 patch into git and have overlayed edits from my > > first pass. Lots of funky wrapping to conform to 80 columns. But > > whitespace aside, I've added FIXME:s in the relevant files. If you work > > on any of these FIXMEs please send follow-up patches so that we don't > > step on each others' toes. > > > > Please see the 'for-3.15-insitu-comp' branch of this git repo: > > git://git.kernel.org/pub/scm/linux/kernel/git/snitzer/linux.git > > Thanks for your to look at it. I fixed them against your tree. Please check > below patch. I folded your changes in, and then committed a patch ontop that cleans some code up. But added 2 FIXMEs that still speak to pretty fundamental problems with the architecture of the dm-insitu-comp target, see: https://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/commit/?h=for-3.15-insitu-comp&id=8565ab6b04837591d03c94851c2f9f9162ce12f4 Unfortunately the single insitu_comp_wq workqueue that all insitu-comp targets are to share isn't a workable solution. Each target needs to have resource isolation from other targets (imagine insitu-comp used for multiple SSDs). This is important for suspend too because you'll need to flush/stop the workqueue. You introduced a state machine for tracking suspending, suspended, resumed. This really isn't necessary. During suspend you need to flush_workqueue(). On resume you shouldn't need to do anything special. As I noted in the commit, the thin and cache targets can serve as references for how you can manage the workqueue across suspend/resume and the lifetime of these workqueues relative to .ctr and .dtr. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch v3]DM: dm-insitu-comp: a compressed DM target for SSD
On Fri, Mar 14, 2014 at 06:44:45PM -0400, Mike Snitzer wrote:
> On Fri, Mar 14 2014 at 5:40am -0400,
> Shaohua Li wrote:
>
> > On Mon, Mar 10, 2014 at 09:52:56AM -0400, Mike Snitzer wrote:
> > > On Fri, Mar 07 2014 at 2:57am -0500,
> > > Shaohua Li wrote:
> > >
> > > > ping!
> > >
> > > Hi,
> > >
> > > I intend to get dm-insitu-comp reviewed for 3.15. Sorry I haven't
> > > gotten back with you before now, been busy tending to 3.14-rc issues.
> > >
> > > I took a quick first pass over your code a couple weeks ago. Looks to
> > > be in great shape relative to coding conventions and the more DM
> > > specific conventions. Clearly demonstrates you have a good command of
> > > DM concepts and quirks.
>
> Think I need to eat my words from above at least partially. Given you
> haven't implemented any of the target suspend or resume hooks this
> target will _not_ work properly across suspend + resume cycles that all
> DM targets must support.
>
> But we can obviously work through it with urgency for 3.15.
>
> I've pulled your v3 patch into git and have overlayed edits from my
> first pass. Lots of funky wrapping to conform to 80 columns. But
> whitespace aside, I've added FIXME:s in the relevant files. If you work
> on any of these FIXMEs please send follow-up patches so that we don't
> step on each others' toes.
>
> Please see the 'for-3.15-insitu-comp' branch of this git repo:
> git://git.kernel.org/pub/scm/linux/kernel/git/snitzer/linux.git
Thanks for your to look at it. I fixed them against your tree. Please check
below patch.
Subject: dm-insitu-comp: fix different issues
Fix different issues pointed out by Mike and add suspend/resume support.
Signed-off-by: Shaohua Li
---
drivers/md/dm-insitu-comp.c | 108 ++--
drivers/md/dm-insitu-comp.h |8 +++
2 files changed, 84 insertions(+), 32 deletions(-)
Index: linux/drivers/md/dm-insitu-comp.c
===
--- linux.orig/drivers/md/dm-insitu-comp.c 2014-03-17 12:37:37.850751341
+0800
+++ linux/drivers/md/dm-insitu-comp.c 2014-03-17 17:40:01.106660303 +0800
@@ -17,6 +17,7 @@
#include "dm-insitu-comp.h"
#define DM_MSG_PREFIX "insitu-comp"
+#define DEFAULT_WRITEBACK_DELAY 5
static inline int lzo_comp_len(int comp_len)
{
@@ -40,6 +41,10 @@ static struct kmem_cache *insitu_comp_me
static struct insitu_comp_io_worker insitu_comp_io_workers[NR_CPUS];
static struct workqueue_struct *insitu_comp_wq;
+#define BYTE_BITS 8
+#define BYTE_BITS_SHIFT 3
+#define BYTE_BITS_MASK (BYTE_BITS - 1)
+
/* each block has 5 bits of metadata */
static u8 insitu_comp_get_meta(struct insitu_comp_info *info, u64 block_index)
{
@@ -47,15 +52,14 @@ static u8 insitu_comp_get_meta(struct in
int bits, offset;
u8 data, ret = 0;
- // FIXME: "magic" numbers in this function (7, 3)
- offset = first_bit & 7;
- bits = min_t(u8, INSITU_COMP_META_BITS, 8 - offset);
+ offset = first_bit & BYTE_BITS_MASK;
+ bits = min_t(u8, INSITU_COMP_META_BITS, BYTE_BITS - offset);
- data = info->meta_bitmap[first_bit >> 3];
+ data = info->meta_bitmap[first_bit >> BYTE_BITS_SHIFT];
ret = (data >> offset) & ((1 << bits) - 1);
if (bits < INSITU_COMP_META_BITS) {
- data = info->meta_bitmap[(first_bit >> 3) + 1];
+ data = info->meta_bitmap[(first_bit >> BYTE_BITS_SHIFT) + 1];
bits = INSITU_COMP_META_BITS - bits;
ret |= (data & ((1 << bits) - 1)) <<
(INSITU_COMP_META_BITS - bits);
@@ -71,14 +75,13 @@ static void insitu_comp_set_meta(struct
u8 data;
struct page *page;
- // FIXME: "magic" numbers in this function (7, 3)
- offset = first_bit & 7;
- bits = min_t(u8, INSITU_COMP_META_BITS, 8 - offset);
+ offset = first_bit & BYTE_BITS_MASK;
+ bits = min_t(u8, INSITU_COMP_META_BITS, BYTE_BITS - offset);
- data = info->meta_bitmap[first_bit >> 3];
+ data = info->meta_bitmap[first_bit >> BYTE_BITS_SHIFT];
data &= ~(((1 << bits) - 1) << offset);
data |= (meta & ((1 << bits) - 1)) << offset;
- info->meta_bitmap[first_bit >> 3] = data;
+ info->meta_bitmap[first_bit >> BYTE_BITS_SHIFT] = data;
/*
* For writethrough, we write metadata directly. For writeback, if
@@ -301,9 +304,24 @@ static int insitu_comp_meta_writeback_th
init_completion(&wb.complete);
while (!kthread_should_stop()) {
- // FIXME: writeback_delay should be in secs
-
schedule_timeout_interruptible(msecs_to_jiffies(info->writeback_delay * 1000));
+ schedule_timeout_interruptible(
+ msecs_to_jiffies(info->writeback_delay * MSEC_PER_SEC));
insitu_comp_flush_dirty_meta(info, &wb);
+
+ if (info->wb_thread_suspend_status != WB_THREAD_RESUMED) {
+
Re: [patch v3]DM: dm-insitu-comp: a compressed DM target for SSD
On Fri, Mar 14 2014 at 5:40am -0400, Shaohua Li wrote: > On Mon, Mar 10, 2014 at 09:52:56AM -0400, Mike Snitzer wrote: > > On Fri, Mar 07 2014 at 2:57am -0500, > > Shaohua Li wrote: > > > > > ping! > > > > Hi, > > > > I intend to get dm-insitu-comp reviewed for 3.15. Sorry I haven't > > gotten back with you before now, been busy tending to 3.14-rc issues. > > > > I took a quick first pass over your code a couple weeks ago. Looks to > > be in great shape relative to coding conventions and the more DM > > specific conventions. Clearly demonstrates you have a good command of > > DM concepts and quirks. Think I need to eat my words from above at least partially. Given you haven't implemented any of the target suspend or resume hooks this target will _not_ work properly across suspend + resume cycles that all DM targets must support. But we can obviously work through it with urgency for 3.15. I've pulled your v3 patch into git and have overlayed edits from my first pass. Lots of funky wrapping to conform to 80 columns. But whitespace aside, I've added FIXME:s in the relevant files. If you work on any of these FIXMEs please send follow-up patches so that we don't step on each others' toes. Please see the 'for-3.15-insitu-comp' branch of this git repo: git://git.kernel.org/pub/scm/linux/kernel/git/snitzer/linux.git https://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/log/?h=for-3.15-insitu-comp > > But one thing that would really help get dm-insitu-comp into 3.15 is to > > show that the code is working as you'd expect. To that end, it'd be > > great if you'd be willing to add dm-insitu-comp support to the > > device-mapper-test-suite, see: > > https://github.com/jthornber/device-mapper-test-suite > > > > I recently added barebones/simple dm-crypt support, see: > > https://github.com/jthornber/device-mapper-test-suite/commit/c865bcd4e48228e18626d94327fb2485cf9ec9a1 > > > > But It may be that activation/test code for the other targets (e.g. thin > > or cache) are more useful examples to follow for implemnting > > dm-insitu-comp stack activation, see: > > https://github.com/jthornber/device-mapper-test-suite/blob/master/lib/dmtest/pool-stack.rb > > https://github.com/jthornber/device-mapper-test-suite/blob/master/lib/dmtest/cache_stack.rb > > > > All said, implementing dm-insitu-comp support for dmts (including some > > tests that establish it is working as intended) isn't a hard requirement > > for getting the target upstream but it would _really_ help. > > Ok, I added some simple tests in the test suites. OK, I missed this before because it was an attachment. I was confused as to whether you already added or will add support. Now that I've replied to this mail mutt pulled in the attachment ;) I'll take for a spin on Monday (or over the weekend if I'm bored). Thanks, Mike -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch v3]DM: dm-insitu-comp: a compressed DM target for SSD
On Mon, Mar 10, 2014 at 09:52:56AM -0400, Mike Snitzer wrote:
> On Fri, Mar 07 2014 at 2:57am -0500,
> Shaohua Li wrote:
>
> > ping!
>
> Hi,
>
> I intend to get dm-insitu-comp reviewed for 3.15. Sorry I haven't
> gotten back with you before now, been busy tending to 3.14-rc issues.
>
> I took a quick first pass over your code a couple weeks ago. Looks to
> be in great shape relative to coding conventions and the more DM
> specific conventions. Clearly demonstrates you have a good command of
> DM concepts and quirks.
>
> But one thing that would really help get dm-insitu-comp into 3.15 is to
> show that the code is working as you'd expect. To that end, it'd be
> great if you'd be willing to add dm-insitu-comp support to the
> device-mapper-test-suite, see:
> https://github.com/jthornber/device-mapper-test-suite
>
> I recently added barebones/simple dm-crypt support, see:
> https://github.com/jthornber/device-mapper-test-suite/commit/c865bcd4e48228e18626d94327fb2485cf9ec9a1
>
> But It may be that activation/test code for the other targets (e.g. thin
> or cache) are more useful examples to follow for implemnting
> dm-insitu-comp stack activation, see:
> https://github.com/jthornber/device-mapper-test-suite/blob/master/lib/dmtest/pool-stack.rb
> https://github.com/jthornber/device-mapper-test-suite/blob/master/lib/dmtest/cache_stack.rb
>
> All said, implementing dm-insitu-comp support for dmts (including some
> tests that establish it is working as intended) isn't a hard requirement
> for getting the target upstream but it would _really_ help.
Ok, I added some simple tests in the test suites.
Thanks,
Shaohua
---
lib/dmtest/suites/insitu-comp.rb |1
lib/dmtest/tests/insitu-comp/insitu-comp_tests.rb | 120 ++
2 files changed, 121 insertions(+)
Index: device-mapper-test-suite/lib/dmtest/suites/insitu-comp.rb
===
--- /dev/null 1970-01-01 00:00:00.0 +
+++ device-mapper-test-suite/lib/dmtest/suites/insitu-comp.rb 2014-03-14 17:16:14.043519177 +0800
@@ -0,0 +1 @@
+require 'dmtest/tests/insitu-comp/insitu-comp_tests'
Index: device-mapper-test-suite/lib/dmtest/tests/insitu-comp/insitu-comp_tests.rb
===
--- /dev/null 1970-01-01 00:00:00.0 +
+++ device-mapper-test-suite/lib/dmtest/tests/insitu-comp/insitu-comp_tests.rb 2014-03-14 17:16:14.043519177 +0800
@@ -0,0 +1,120 @@
+require 'dmtest/config'
+require 'dmtest/git'
+require 'dmtest/log'
+require 'dmtest/utils'
+require 'dmtest/fs'
+require 'dmtest/tags'
+require 'dmtest/thinp-test'
+require 'dmtest/cache-status'
+require 'dmtest/disk-units'
+require 'dmtest/test-utils'
+require 'dmtest/tests/cache/fio_subvolume_scenario'
+
+require 'pp'
+
+#
+
+class InsitucompStack
+ include DM
+ include DM::LexicalOperators
+ include Utils
+
+ def initialize(dm, dev, opts)
+@dm = dm
+@dev = dev
+@opts = opts
+ end
+
+ def activate(&block)
+ with_dev(table) do |comp|
+ @comp = comp
+ block.call(comp)
+ end
+ end
+
+ def table
+total_blocks = dev_size(@dev) >> 3
+data_blocks = total_blocks - 1
+rem = data_blocks % (4096 * 8 + 5)
+data_blocks /= 4096 * 8 + 5
+meta_blocks = data_blocks * 5
+data_blocks *= 4096 * 8
+
+cnt = rem
+rem /= (4096 * 8 / 5 + 1)
+data_blocks += rem * (4096 * 8 / 5)
+meta_blocks += rem
+
+cnt %= (4096 * 8 / 5 + 1)
+meta_blocks += 1
+data_blocks += cnt - 1
+
+sector_count = data_blocks << 3
+
+writethrough = @opts.fetch(:writethrough, true)
+if writethrough
+ t = Table.new(Target.new('insitu_comp', sector_count, @dev, 'writethrough'))
+else
+ wb_interval = @opts.fetch(:writeback_interval, 5)
+ t = Table.new(Target.new('insitu_comp', sector_count, @dev, 'writeback', wb_interval))
+end
+t
+ end
+
+ private
+ def dm_interface
+@dm
+ end
+end
+
+#
+
+class InsitucompTests < ThinpTestCase
+ include Utils
+ include DiskUnits
+ include FioSubVolumeScenario
+
+ def test_basic_setup_writethrough
+test_basic_setup()
+ end
+
+ def test_basic_setup_writeback
+test_basic_setup(false, 5)
+ end
+
+ def test_fio_writethrough
+test_fio()
+ end
+
+ def test_fio_writeback
+test_fio(false, 5)
+ end
+
+ private
+ def alloc_stack(writethrough, wbinterval)
+if writethrough
+ stack = InsitucompStack.new(@dm, @data_dev, :writethrough => true)
+else
+ stack = InsitucompStack.new(@dm, @data_dev, :writethrough => false, :writeback_interval => wbinterval)
+end
+stack
+ end
+
+ private
+ def test_basic_setup(writethrough = true, wbinterval = 5)
+stack = alloc_stack(writethrough, wbinterval)
+stack.activate do |comp|
+ wipe_device(comp)
+end
+ end
+
+ private
+ d
Re: [patch v3]DM: dm-insitu-comp: a compressed DM target for SSD
On Fri, Mar 07 2014 at 2:57am -0500, Shaohua Li wrote: > ping! Hi, I intend to get dm-insitu-comp reviewed for 3.15. Sorry I haven't gotten back with you before now, been busy tending to 3.14-rc issues. I took a quick first pass over your code a couple weeks ago. Looks to be in great shape relative to coding conventions and the more DM specific conventions. Clearly demonstrates you have a good command of DM concepts and quirks. But one thing that would really help get dm-insitu-comp into 3.15 is to show that the code is working as you'd expect. To that end, it'd be great if you'd be willing to add dm-insitu-comp support to the device-mapper-test-suite, see: https://github.com/jthornber/device-mapper-test-suite I recently added barebones/simple dm-crypt support, see: https://github.com/jthornber/device-mapper-test-suite/commit/c865bcd4e48228e18626d94327fb2485cf9ec9a1 But It may be that activation/test code for the other targets (e.g. thin or cache) are more useful examples to follow for implemnting dm-insitu-comp stack activation, see: https://github.com/jthornber/device-mapper-test-suite/blob/master/lib/dmtest/pool-stack.rb https://github.com/jthornber/device-mapper-test-suite/blob/master/lib/dmtest/cache_stack.rb All said, implementing dm-insitu-comp support for dmts (including some tests that establish it is working as intended) isn't a hard requirement for getting the target upstream but it would _really_ help. Mike -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch v3]DM: dm-insitu-comp: a compressed DM target for SSD
ping! On Tue, Feb 18, 2014 at 06:13:04PM +0800, Shaohua Li wrote: > > This is a simple DM target supporting compression for SSD only. Under layer > SSD > must support 512B sector size, the target only supports 4k sector size. > > Disk layout: > |super|...meta...|..data...| > > Store unit is 4k (a block). Super is 1 block, which stores meta and data size > and compression algorithm. Meta is a bitmap. For each data block, there are 5 > bits meta. > > Data: > Data of a block is compressed. Compressed data is round up to 512B, which is > the payload. In disk, payload is stored at the begining of logical sector of > the block. Let's look at an example. Say we store data to block A, which is in > sector B(A*8), its orginal size is 4k, compressed size is 1500. Compressed > data > (CD) will use 3 sectors (512B). The 3 sectors are the payload. Payload will be > stored at sector B. > > --- > ... | CD1 | CD2 | CD3 | | | | || ... > --- > ^B^B+1 ^B+2 ^B+7 ^B+8 > > For this block, we will not use sector B+3 to B+7 (a hole). We use 4 meta bits > to present payload size. The compressed size (1500) isn't stored in meta > directly. Instead, we store it at the last 32bits of payload. In this example, > we store it at the end of sector B+2. If compressed size + sizeof(32bits) > crosses a sector, payload size will increase one sector. If payload uses 8 > sectors, we store uncompressed data directly. > > If IO size is bigger than one block, we can store the data as an extent. Data > of the whole extent will compressed and stored in the similar way like above. > The first block of the extent is the head, all others are the tail. If extent > is 1 block, the block is head. We have 1 bit of meta to present if a block is > head or tail. If 4 meta bits of head block can't store extent payload size, we > will borrow tail block meta bits to store payload size. Max allowd extent size > is 128k, so we don't compress/decompress too big size data. > > Meta: > Modifying data will modify meta too. Meta will be written(flush) to disk > depending on meta write policy. We support writeback and writethrough mode. In > writeback mode, meta will be written to disk in an interval or a FLUSH > request. > In writethrough mode, data and meta data will be written to disk together. > > Advantages: > 1. simple. Since we store compressed data in-place, we don't need complicated > disk data management. > 2. efficient. For each 4k, we only need 5 bits meta. 1T data will use less > than > 200M meta, so we can load all meta into memory. And actual compression size is > in payload. So if IO doesn't need RMW and we use writeback meta flush, we > don't > need extra IO for meta. > > Disadvantages: > 1. hole. Since we store compressed data in-place, there are a lot of holes (in > above example, B+3 - B+7) Hole can impact IO, because we can't do IO merge. > 2. 1:1 size. Compression doesn't change disk size. If disk is 1T, we can only > store > 1T data even we do compression. > > But this is for SSD only. Generally SSD firmware has a FTL layer to map disk > sectors to flash nand. High end SSD firmware has filesystem-like FTL. > 1. hole. Disk has a lot of holes, but SSD FTL can still store data continuous > in nand. Even we can't do IO merge in OS layer, SSD firmware can do it. > 2. 1:1 size. On one side, we write compressed data to SSD, which means less > data is written to SSD. This will be very helpful to improve SSD garbage > collection, and so write speed and life cycle. So even this is a problem, the > target is still helpful. On the other side, advanced SSD FTL can easily do > thin > provision. For example, if nand is 1T and we let SSD report it as 2T, and use > the SSD as compressed target. In such SSD, we don't have the 1:1 size issue. > > So if SSD FTL can map non-continuous disk sectors to continuous nand and > support thin provision, the compressed target will work very well. > > V2->V3: > Updated with new bio iter API > > V1->V2: > 1. Change name to insitu_comp, cleanup code, add comments and doc > 2. Improve performance (extent locking, dedicated workqueue) > > Signed-off-by: Shaohua Li > --- > Documentation/device-mapper/insitu-comp.txt | 50 > drivers/md/Kconfig |6 > drivers/md/Makefile |1 > drivers/md/dm-insitu-comp.c | 1480 > > drivers/md/dm-insitu-comp.h | 158 ++ > 5 files changed, 1695 insertions(+) > > Index: linux/drivers/md/Kconfig > === > --- linux.orig/drivers/md/Kconfig 2014-02-17 17:34:45.431464714 +0800 > +++ linux/drivers/md/Kconfig 2014-02-17 17:34:45.423464815 +0800 > @@ -295,6 +295,12 @@ config DM_CACHE_CLEANER > A simple cache policy that writes back all data to the >
[patch v3]DM: dm-insitu-comp: a compressed DM target for SSD
This is a simple DM target supporting compression for SSD only. Under layer SSD must support 512B sector size, the target only supports 4k sector size. Disk layout: |super|...meta...|..data...| Store unit is 4k (a block). Super is 1 block, which stores meta and data size and compression algorithm. Meta is a bitmap. For each data block, there are 5 bits meta. Data: Data of a block is compressed. Compressed data is round up to 512B, which is the payload. In disk, payload is stored at the begining of logical sector of the block. Let's look at an example. Say we store data to block A, which is in sector B(A*8), its orginal size is 4k, compressed size is 1500. Compressed data (CD) will use 3 sectors (512B). The 3 sectors are the payload. Payload will be stored at sector B. --- ... | CD1 | CD2 | CD3 | | | | || ... --- ^B^B+1 ^B+2 ^B+7 ^B+8 For this block, we will not use sector B+3 to B+7 (a hole). We use 4 meta bits to present payload size. The compressed size (1500) isn't stored in meta directly. Instead, we store it at the last 32bits of payload. In this example, we store it at the end of sector B+2. If compressed size + sizeof(32bits) crosses a sector, payload size will increase one sector. If payload uses 8 sectors, we store uncompressed data directly. If IO size is bigger than one block, we can store the data as an extent. Data of the whole extent will compressed and stored in the similar way like above. The first block of the extent is the head, all others are the tail. If extent is 1 block, the block is head. We have 1 bit of meta to present if a block is head or tail. If 4 meta bits of head block can't store extent payload size, we will borrow tail block meta bits to store payload size. Max allowd extent size is 128k, so we don't compress/decompress too big size data. Meta: Modifying data will modify meta too. Meta will be written(flush) to disk depending on meta write policy. We support writeback and writethrough mode. In writeback mode, meta will be written to disk in an interval or a FLUSH request. In writethrough mode, data and meta data will be written to disk together. Advantages: 1. simple. Since we store compressed data in-place, we don't need complicated disk data management. 2. efficient. For each 4k, we only need 5 bits meta. 1T data will use less than 200M meta, so we can load all meta into memory. And actual compression size is in payload. So if IO doesn't need RMW and we use writeback meta flush, we don't need extra IO for meta. Disadvantages: 1. hole. Since we store compressed data in-place, there are a lot of holes (in above example, B+3 - B+7) Hole can impact IO, because we can't do IO merge. 2. 1:1 size. Compression doesn't change disk size. If disk is 1T, we can only store 1T data even we do compression. But this is for SSD only. Generally SSD firmware has a FTL layer to map disk sectors to flash nand. High end SSD firmware has filesystem-like FTL. 1. hole. Disk has a lot of holes, but SSD FTL can still store data continuous in nand. Even we can't do IO merge in OS layer, SSD firmware can do it. 2. 1:1 size. On one side, we write compressed data to SSD, which means less data is written to SSD. This will be very helpful to improve SSD garbage collection, and so write speed and life cycle. So even this is a problem, the target is still helpful. On the other side, advanced SSD FTL can easily do thin provision. For example, if nand is 1T and we let SSD report it as 2T, and use the SSD as compressed target. In such SSD, we don't have the 1:1 size issue. So if SSD FTL can map non-continuous disk sectors to continuous nand and support thin provision, the compressed target will work very well. V2->V3: Updated with new bio iter API V1->V2: 1. Change name to insitu_comp, cleanup code, add comments and doc 2. Improve performance (extent locking, dedicated workqueue) Signed-off-by: Shaohua Li --- Documentation/device-mapper/insitu-comp.txt | 50 drivers/md/Kconfig |6 drivers/md/Makefile |1 drivers/md/dm-insitu-comp.c | 1480 drivers/md/dm-insitu-comp.h | 158 ++ 5 files changed, 1695 insertions(+) Index: linux/drivers/md/Kconfig === --- linux.orig/drivers/md/Kconfig 2014-02-17 17:34:45.431464714 +0800 +++ linux/drivers/md/Kconfig2014-02-17 17:34:45.423464815 +0800 @@ -295,6 +295,12 @@ config DM_CACHE_CLEANER A simple cache policy that writes back all data to the origin. Used when decommissioning a dm-cache. +config DM_INSITU_COMPRESSION + tristate "Insitu compression target" + depends on BLK_DEV_DM + ---help--- + Allow volume managers to insitu compress data for SSD. + config DM_MIRROR tristat
