All of lore.kernel.org
 help / color / mirror / Atom feed
From: Joe Thornber <thornber@redhat.com>
To: Akira Hayakawa <ruby.wktk@gmail.com>
Cc: devel@driverdev.osuosl.org, snitzer@redhat.com,
	cesarb@cesarb.net, gregkh@linuxfoundation.org,
	david@fromorbit.com, linux-kernel@vger.kernel.org,
	dm-devel@redhat.com, mpatocka@redhat.com, agk@redhat.com,
	joe@perches.com, akpm@linux-foundation.org, ejt@redhat.com,
	dan.carpenter@oracle.com, m.chehab@samsung.com
Subject: Re: [dm-devel] dm-writeboost testing
Date: Fri, 4 Oct 2013 16:03:14 +0100	[thread overview]
Message-ID: <20131004150313.GB25523@debian> (raw)
In-Reply-To: <524D70DA.8040308@gmail.com>

On Thu, Oct 03, 2013 at 10:27:54PM +0900, Akira Hayakawa wrote:

> > dm-cache doesn't have this problem, if you overwrite the same piece of 
> > data again and again, it goes to the cache device.
> 
> It is not a bug but should/can be optimized.
> 
> Below is the cache hit path for writes.
> writeboost performs very poorly when a partial write hits
> which then turns `needs_cleanup_perv_cache` to true.

Are you using fixed size blocks for caching then?  The whole point of
using a journal/log based disk layout for caching is you can slurp up
all writes irrespective of their size.

What are the scenarios where you out perform dm-cache?

- Joe




> Partial write hits is believed to be unlikely so
> I decided to give up this path to make other likely-paths optimized.
> I think this is just a tradeoff issue of what to be optimized the most.
> 
>         if (found) {
> 
>                 if (unlikely(on_buffer)) {
>                         mutex_unlock(&cache->io_lock);
> 
>                         update_mb_idx = mb->idx;
>                         goto write_on_buffer;
>                 } else {
>                         u8 dirty_bits = atomic_read_mb_dirtiness(seg, mb);
> 
>                         /*
>                          * First clean up the previous cache
>                          * and migrate the cache if needed.
>                          */
>                         bool needs_cleanup_prev_cache =
>                                 !bio_fullsize || !(dirty_bits == 255);
> 
>                         if (unlikely(needs_cleanup_prev_cache)) {
>                                 wait_for_completion(&seg->flush_done);
>                                 migrate_mb(cache, seg, mb, dirty_bits, true);
>                         }
> 
> I checked that the mkfs.ext4 writes only in 4KB size
> so it is not gonna turn the boolean value true for going into the slowpath.
> 
> Problem:
> Problem is that
> it chooses the slowpath even though the bio is full-sized overwrite
> in the test.
> 
> The reason is that the dirty bits is sometimes seen as 0
> and the suspect is the migration daemon.
> 
> I guess you created the writeboost device with the default configuration.
> In that case migration daemon always works and
> some metadata is cleaned up in background.
> 
> If you turns both enable_migration_modulator and allow_migrate to 0
> before beginning the test
> to stop migration at all
> it never goes into the slowpath with the test.
> 
> Solution:
> Changing the code to
> avoid going into the slowpath when the dirty bits is zero
> will solve this problem.
> 
> And done. Please pull the latest one from the repo.
> --- a/Driver/dm-writeboost-target.c
> +++ b/Driver/dm-writeboost-target.c
> @@ -688,6 +688,14 @@ static int writeboost_map(struct dm_target *ti, struct bio *bio
>                         bool needs_cleanup_prev_cache =
>                                 !bio_fullsize || !(dirty_bits == 255);
> 
> +                       /*
> +                        * Migration works in background
> +                        * and may have cleaned up the metablock.
> +                        * If the metablock is clean we need not to migrate.
> +                        */
> +                       if (!dirty_bits)
> +                               needs_cleanup_prev_cache = false;
> +
>                         if (unlikely(needs_cleanup_prev_cache)) {
>                                 wait_for_completion(&seg->flush_done);
>                                 migrate_mb(cache, seg, mb, dirty_bits, true);
> 
> Thanks,
> Akira

WARNING: multiple messages have this Message-ID (diff)
From: Joe Thornber <thornber@redhat.com>
To: Akira Hayakawa <ruby.wktk@gmail.com>
Cc: mpatocka@redhat.com, dm-devel@redhat.com,
	devel@driverdev.osuosl.org, snitzer@redhat.com,
	gregkh@linuxfoundation.org, david@fromorbit.com,
	linux-kernel@vger.kernel.org, dan.carpenter@oracle.com,
	joe@perches.com, akpm@linux-foundation.org, m.chehab@samsung.com,
	ejt@redhat.com, agk@redhat.com, cesarb@cesarb.net
Subject: Re: [dm-devel] dm-writeboost testing
Date: Fri, 4 Oct 2013 16:03:14 +0100	[thread overview]
Message-ID: <20131004150313.GB25523@debian> (raw)
In-Reply-To: <524D70DA.8040308@gmail.com>

On Thu, Oct 03, 2013 at 10:27:54PM +0900, Akira Hayakawa wrote:

> > dm-cache doesn't have this problem, if you overwrite the same piece of 
> > data again and again, it goes to the cache device.
> 
> It is not a bug but should/can be optimized.
> 
> Below is the cache hit path for writes.
> writeboost performs very poorly when a partial write hits
> which then turns `needs_cleanup_perv_cache` to true.

Are you using fixed size blocks for caching then?  The whole point of
using a journal/log based disk layout for caching is you can slurp up
all writes irrespective of their size.

What are the scenarios where you out perform dm-cache?

- Joe




> Partial write hits is believed to be unlikely so
> I decided to give up this path to make other likely-paths optimized.
> I think this is just a tradeoff issue of what to be optimized the most.
> 
>         if (found) {
> 
>                 if (unlikely(on_buffer)) {
>                         mutex_unlock(&cache->io_lock);
> 
>                         update_mb_idx = mb->idx;
>                         goto write_on_buffer;
>                 } else {
>                         u8 dirty_bits = atomic_read_mb_dirtiness(seg, mb);
> 
>                         /*
>                          * First clean up the previous cache
>                          * and migrate the cache if needed.
>                          */
>                         bool needs_cleanup_prev_cache =
>                                 !bio_fullsize || !(dirty_bits == 255);
> 
>                         if (unlikely(needs_cleanup_prev_cache)) {
>                                 wait_for_completion(&seg->flush_done);
>                                 migrate_mb(cache, seg, mb, dirty_bits, true);
>                         }
> 
> I checked that the mkfs.ext4 writes only in 4KB size
> so it is not gonna turn the boolean value true for going into the slowpath.
> 
> Problem:
> Problem is that
> it chooses the slowpath even though the bio is full-sized overwrite
> in the test.
> 
> The reason is that the dirty bits is sometimes seen as 0
> and the suspect is the migration daemon.
> 
> I guess you created the writeboost device with the default configuration.
> In that case migration daemon always works and
> some metadata is cleaned up in background.
> 
> If you turns both enable_migration_modulator and allow_migrate to 0
> before beginning the test
> to stop migration at all
> it never goes into the slowpath with the test.
> 
> Solution:
> Changing the code to
> avoid going into the slowpath when the dirty bits is zero
> will solve this problem.
> 
> And done. Please pull the latest one from the repo.
> --- a/Driver/dm-writeboost-target.c
> +++ b/Driver/dm-writeboost-target.c
> @@ -688,6 +688,14 @@ static int writeboost_map(struct dm_target *ti, struct bio *bio
>                         bool needs_cleanup_prev_cache =
>                                 !bio_fullsize || !(dirty_bits == 255);
> 
> +                       /*
> +                        * Migration works in background
> +                        * and may have cleaned up the metablock.
> +                        * If the metablock is clean we need not to migrate.
> +                        */
> +                       if (!dirty_bits)
> +                               needs_cleanup_prev_cache = false;
> +
>                         if (unlikely(needs_cleanup_prev_cache)) {
>                                 wait_for_completion(&seg->flush_done);
>                                 migrate_mb(cache, seg, mb, dirty_bits, true);
> 
> Thanks,
> Akira

  reply	other threads:[~2013-10-04 15:03 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-10-03  0:15 dm-writeboost testing Mikulas Patocka
2013-10-03  0:15 ` Mikulas Patocka
2013-10-03 13:27 ` [dm-devel] " Akira Hayakawa
2013-10-03 13:27   ` Akira Hayakawa
2013-10-04 15:03   ` Joe Thornber [this message]
2013-10-04 15:03     ` Joe Thornber
2013-10-04  2:28 ` Akira Hayakawa
2013-10-04  2:28   ` Akira Hayakawa
2013-10-04 13:38   ` Mikulas Patocka
2013-10-04 13:38     ` Mikulas Patocka
2013-10-04 14:25     ` Akira Hayakawa
2013-10-04 14:25       ` Akira Hayakawa
2013-10-04 15:56       ` Mikulas Patocka
2013-10-04 15:56         ` [dm-devel] " Mikulas Patocka
2013-10-05  7:05         ` Akira Hayakawa
2013-10-05  7:05           ` Akira Hayakawa
2013-10-06  0:47           ` Mikulas Patocka
2013-10-06  0:47             ` Mikulas Patocka
2013-10-06  2:14             ` Akira Hayakawa

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20131004150313.GB25523@debian \
    --to=thornber@redhat.com \
    --cc=agk@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=cesarb@cesarb.net \
    --cc=dan.carpenter@oracle.com \
    --cc=david@fromorbit.com \
    --cc=devel@driverdev.osuosl.org \
    --cc=dm-devel@redhat.com \
    --cc=ejt@redhat.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=joe@perches.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=m.chehab@samsung.com \
    --cc=mpatocka@redhat.com \
    --cc=ruby.wktk@gmail.com \
    --cc=snitzer@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.