Commit every second to prevent too much of a position building up.
 

On Thu, Feb 02 2012 at 11:39am -0500,
Joe Thornber <ejt@redhat.com> wrote:

> ---
> drivers/md/dm-thin.c | 28 ++++++++++++++++++++++++++--
> 1 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
> index 6ef03a2..19de11a 100644
> --- a/drivers/md/dm-thin.c
> +++ b/drivers/md/dm-thin.c
> @@ -23,6 +23,7 @@
> #define DEFERRED_SET_SIZE 64
> #define MAPPING_POOL_SIZE 1024
> #define PRISON_CELLS 1024
> +#define COMMIT_PERIOD HZ
>
> /*
> * The block size of the device holding pool data must be
> @@ -498,8 +499,10 @@ struct pool {
>
> struct workqueue_struct *wq;
> struct work_struct worker;
> + struct delayed_work waker;
>
> unsigned ref_count;
> + unsigned long last_commit_jiffies;
>
> spinlock_t lock;
> struct bio_list deferred_bios;
> @@ -1256,6 +1259,12 @@ static void process_bio(struct thin_c *tc, struct bio *bio)
> }
> }
>
> +static int need_commit_due_to_time(struct pool *pool)
> +{
> + return jiffies < pool->last_commit_jiffies ||
> + jiffies > pool->last_commit_jiffies + COMMIT_PERIOD;
> +}
> +
> static void process_deferred_bios(struct pool *pool)
> {
> unsigned long flags;
> @@ -1297,7 +1306,7 @@ static void process_deferred_bios(struct pool *pool)
> bio_list_init(&pool->deferred_flush_bios);
> spin_unlock_irqrestore(&pool->lock, flags);
>
> - if (bio_list_empty(&bios))
> + if (bio_list_empty(&bios) && !need_commit_due_to_time(pool))
> return;

Shouldn't this be:

if (bio_list_empty(&bios) || !need_commit_due_to_time(pool))
return;

?

Also, should we make the commit interval tunable (akin to ext[34]'s
'commit' mount option)? Or did you defer doing so until it proves
worthwhile?

Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Commit every second to prevent too much of a position building up.
 

On Tue, Feb 07 2012 at 11:53am -0500,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Thu, Feb 02 2012 at 11:39am -0500,
> Joe Thornber <ejt@redhat.com> wrote:
>
> > ---
> > drivers/md/dm-thin.c | 28 ++++++++++++++++++++++++++--
> > 1 files changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
> > index 6ef03a2..19de11a 100644
> > --- a/drivers/md/dm-thin.c
> > +++ b/drivers/md/dm-thin.c
> > @@ -23,6 +23,7 @@
> > #define DEFERRED_SET_SIZE 64
> > #define MAPPING_POOL_SIZE 1024
> > #define PRISON_CELLS 1024
> > +#define COMMIT_PERIOD HZ
> >
> > /*
> > * The block size of the device holding pool data must be
> > @@ -498,8 +499,10 @@ struct pool {
> >
> > struct workqueue_struct *wq;
> > struct work_struct worker;
> > + struct delayed_work waker;
> >
> > unsigned ref_count;
> > + unsigned long last_commit_jiffies;
> >
> > spinlock_t lock;
> > struct bio_list deferred_bios;
> > @@ -1256,6 +1259,12 @@ static void process_bio(struct thin_c *tc, struct bio *bio)
> > }
> > }
> >
> > +static int need_commit_due_to_time(struct pool *pool)
> > +{
> > + return jiffies < pool->last_commit_jiffies ||
> > + jiffies > pool->last_commit_jiffies + COMMIT_PERIOD;
> > +}
> > +
> > static void process_deferred_bios(struct pool *pool)
> > {
> > unsigned long flags;
> > @@ -1297,7 +1306,7 @@ static void process_deferred_bios(struct pool *pool)
> > bio_list_init(&pool->deferred_flush_bios);
> > spin_unlock_irqrestore(&pool->lock, flags);
> >
> > - if (bio_list_empty(&bios))
> > + if (bio_list_empty(&bios) && !need_commit_due_to_time(pool))
> > return;
>
> Shouldn't this be:
>
> if (bio_list_empty(&bios) || !need_commit_due_to_time(pool))
> return;
>
> ?

Hmm, looking closer at the code it is clear that if we'd use || then the
pool->deferred_flush_bios would get dropped in the floor by the
!need_commit_due_to_time(pool) early return.

So ignore that ;)

> Also, should we make the commit interval tunable (akin to ext[34]'s
> 'commit' mount option)? Or did you defer doing so until it proves
> worthwhile?

But this question still stands.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Commit every second to prevent too much of a position building up.
 

On Tue, Feb 07, 2012 at 11:53:25AM -0500, Mike Snitzer wrote:
> Shouldn't this be:
>
> if (bio_list_empty(&bios) || !need_commit_due_to_time(pool))
> return;
>
> ?

I don't think so. It's saying 'if <there's no reason to commit>'. The reasons to commit are bios being non-empty or need_commit_due_to_time.

reason to commit = !bio_list_empty(&bios) || need_commit_due_to_time(pool)
!reason_to_commit = !(!bio_list_empty(&bios) || need_commit_due_to_time(pool))
!reason_to_commit = (bio_list_empty(&bios) && !need_commit_due_to_time(pool))

Agree?

> Also, should we make the commit interval tunable (akin to ext[34]'s
> 'commit' mount option)? Or did you defer doing so until it proves
> worthwhile?

y, tunable is on the list. Every second will do for now though.

- Joe

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Fri Feb 10 17:30:02 2012
Return-path: <devel-bounces@lists.fedoraproject.org>
Envelope-to: tom@linux-archive.org
Delivery-date: Fri, 10 Feb 2012 16:57:48 +0200
Received: from bastion01.fedoraproject.org ([209.132.181.2]:55355 helo=bastion.fedoraproject.org)
by s2.java-tips.org with esmtp (Exim 4.69)
(envelope-from <devel-bounces@lists.fedoraproject.org>)
id 1Rvrv2-0001A6-Nl
for tom@linux-archive.org; Fri, 10 Feb 2012 16:57:48 +0200
Received: from lists.fedoraproject.org (collab03.vpn.fedoraproject.org [192.168.1.70])
by bastion01.phx2.fedoraproject.org (Postfix) with ESMTP id D00942134D;
Fri, 10 Feb 2012 14:57:52 +0000 (UTC)
Received: from collab03.fedoraproject.org (localhost [127.0.0.1])
by lists.fedoraproject.org (Postfix) with ESMTP id 68E6A40AAB;
Fri, 10 Feb 2012 14:57:50 +0000 (UTC)
X-Original-To: devel@lists.fedoraproject.org
Delivered-To: devel@lists.fedoraproject.org
Received: from smtp-mm01.fedoraproject.org (smtp-mm01.fedoraproject.org
[80.239.156.217])
by lists.fedoraproject.org (Postfix) with ESMTP id 0B4AB40A1D
for <devel@lists.fedoraproject.org>;
Fri, 10 Feb 2012 14:57:49 +0000 (UTC)
Received: from mail-bk0-f45.google.com (mail-bk0-f45.google.com
[209.85.214.45])
by smtp-mm01.fedoraproject.org (Postfix) with ESMTP id 4936FC0848
for <devel@lists.fedoraproject.org>;
Fri, 10 Feb 2012 14:57:50 +0000 (UTC)
Received: by bkue19 with SMTP id e19so2677887bku.32
for <devel@lists.fedoraproject.org>;
Fri, 10 Feb 2012 06:57:49 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma;
h=message-id:date:from:user-agent:mime-version:to:subject:references
:in-reply-to:content-type:content-transfer-encoding;
bh=+I7uNpY7n4T60q51cXyW4yjVNc0ExJtEnok2tm+UN8A=;
b=rIwdSd18hbZWDspGLaO7tvCxLcb5gnqQXv68F8YMAIPDwKBM f85UErZSEKOYLLKygn
c+9ec9HJsUdmN2tiBRDAJQjnBAWvmw8sBf4Z39d2qoPqAwCCLt kgoYamjBFVnO7C+KaY
4Jad7o+Xr+3rTcUgwQ3pmHk8IqezzMPgL7Gqw=
Received: by 10.204.13.82 with SMTP id b18mr2704660bka.88.1328885869853;
Fri, 10 Feb 2012 06:57:49 -0800 (PST)
Received: from localhost.localdomain (85-220-55-128.dsl.dynamic.simnet.is.
[85.220.55.128])
by mx.google.com with ESMTPS id t17sm18345638bke.6.2012.02.10.06.57.48
(version=SSLv3 cipher=OTHER); Fri, 10 Feb 2012 06:57:48 -0800 (PST)
Message-ID: <4F353025.80902@gmail.com>
Date: Fri, 10 Feb 2012 14:56:37 +0000
From: =?UTF-8?B?IkrDs2hhbm4gQi4gR3XDsG11bmRzc29uIg==?=
<johannbg@gmail.com>
User-Agent: Mozilla/5.0 (X11; Linux x86_64;
rv:10.0) Gecko/20120131 Thunderbird/10.0
MIME-Version: 1.0
To: devel@lists.fedoraproject.org
Subject: Re: /usrmove?
References: <710f8613-dc50-4697-8da9-f0088863b5fd@zmail13.collab.prod.int.phx2.redhat.c om>
In-Reply-To: <710f8613-dc50-4697-8da9-f0088863b5fd@zmail13.collab.prod.int.phx2.redhat.c om>
X-BeenThere: devel@lists.fedoraproject.org
X-Mailman-Version: 2.1.12
Precedence: list
Reply-To: Development discussions related to Fedora
<devel@lists.fedoraproject.org>
List-Id: Development discussions related to Fedora
<devel.lists.fedoraproject.org>
List-Unsubscribe: <https://admin.fedoraproject.org/mailman/options/devel>,
<mailto:devel-request@lists.fedoraproject.org?subject=unsubscrib e>
List-Archive: <http://lists.fedoraproject.org/pipermail/devel/>
List-Post: <mailto:devel@lists.fedoraproject.org>
List-Help: <mailto:devel-request@lists.fedoraproject.org?subject=help>
List-Subscribe: <https://admin.fedoraproject.org/mailman/listinfo/devel>,
<mailto:devel-request@lists.fedoraproject.org?subject=subscribe>
Content-Transfer-Encoding: base64
Content-Type: text/plain; charset="utf-8"; Format="flowed"
Sender: devel-bounces@lists.fedoraproject.org
Errors-To: devel-bounces@lists.fedoraproject.org

T24gMDIvMTAvMjAxMiAwMTo0NiBQTSwgQWxla3NhbmRhciBLdX J0YWtvdiB3cm90ZToKPiBKb2hh
bm4sCj4gQXJlbid0IHlvdSBwcm92ZW5wYWNrYWdlcj8gSWYgbm 90IHRoaXMgbG9va3MgbGlrZSB0
aGUgYmVzdCB0aGluZyB0byBkbyBzbyB5b3UgcHVzaCB0aGVzZS BjaGFuZ2VzIHlvdXJzZWxmIGFu
ZCBjb25zaWRlcmluZyB0aGF0IHN5c3RlbWQgaXMgdGhlIGluaX RkIHN5c3RlbSBub29uZSBzaG91
bGQgY29tcGxhaW4gYXMgdGhpcyB3YXMgYWxyZWFkeSBhcHByb3 ZlZC4gSSBiZXQgbm90IGV2ZXJ5
IHBhY2thZ2VyIHRoYXQgaGFzbid0IHJlc3BvbmRlZCBpcyB1bn Jlc3BvbnNpdmUgYW5kIGhhdmUg
ZG9uZSBvdGhlciBjaGFuZ2VzIHRoZXkgYXJlIHdlbGwgYXdhcm Ugb2ZmIGFuZCBwb3N0cG9uaW5n
IHRoaXMgdXB0byB0aGUgcG9pbnQgd2hlbiB0aGV5IGNhbiBnZX QgZmFtaWxpYXIgd2l0aCB5b3Vy
IHBhdGNoLgoKSSdtIG5vdCBhIHByb3ZlbnBhY2thZ2VyIGFuZC BoYXZlIG5vIGludGVyZXN0ZWQg
aW4gYmVjb21pbmcgYSBwYWNrYWdlIAptYWludGFpbmVyIGFsbC B0aG91IEkgd291bGQgbm90IG1p
bmQgaGF2ZSB0aGUgYWJpbGl0eSB0byBmaXggdGhpbmdzIGhlcm UgCmFuZCB0aGVyZSBpZiBJIGNh
bWUgYWNyb3NzIHRoZW0uICggSSdtIG5vdCBhd2FyZSBpZiB0aG VyZSBpcyBhIApyb2xlL3Byb2Nl
c3MgaW4gdGhlIGRpc3RyaWJ1dGlvbiBmb3IgcGVvcGxlIHRoYX Qgd2FudCB0byBkbyBqdXN0IHRo
aXMgKQoKQW5kIHRoaXMgcHJvY2VzcyBpcyBub3QgdGhhdCBzaW 1wbGUgYXMgaW4gd2UgY2FudCBi
bGluZGx5IGhhdmUgCnByb3ZlbnBhY2thZ2VycyB0byBwYWNrYW dlIHN1Ym1pdHRlZCB1bml0cyBi
ZWNhdXNlIGluIHNvbWUgY2FzZXMgCnVwc3RyZWFtIGNvZGUgY2 hhbmdlcyBhcmUgbmVlZGVkIG9y
IHRoZSB1bml0cyBhcmVuJ3QgZnVsbHkgY29tcGF0aWJsZSAKd2 l0aCBwcmV2aW91cyBpbml0IHNj
cmlwdCBiZWhhdmlvdXIgZHVlIHRvIHNvbWUgdW5mb3Jlc2VlYW JsZSB1c2FnZSBvZiAKaXQgd2hp
Y2ggb25seSB0aGUgYWN0dWFsIG1haW50YWluZXJzIGFyZSBhd2 FyZSBvZiBoZW5jZSBhIHNvbHV0
aW9uIGZvciAKdGhhdCBuZWVkcyB0byBiZSBmb3VuZC4KCkkgZG lkIGZpbGUgc3BlYyBmaWxlIGNo
YW5nZXMgYWxvbmcgd2l0aCB0aG9zZSB1bml0cyB0byBlYXNlIG 1pZ3JhdGlvbiBhdCAKcmVxdWVz
dCBhbmQgZ3VpZGFuY2Ugb2YgVG9zaGlvIHdoaWNoIGhlIGhpbX NlbGYgb3IgVG9tIHBhY2thZ2Vk
IHRoZSAKdW5pdHMgYW5kIHNoaXBwZWQgdGhlbSB3aGljaCBpcy B3aHkgd2UgbWFuYWdlIHRvIHJl
YWNoIHRoZSBzZXQgZ29hbCBmb3IgCmxhc3QgcmVsZWFzZSBjeW NsZSBob3dldmVyIFRvbSBhbmQg
VG9zaGlvIG9ubHkgdG91Y2hlZCBwYWNrYWdlZCB0aGV5IAphbH JlYWR5IGtuZXcgdGhleSBjb3Vs
ZCB0b3VjaCBpbiBzYWZlIG1hbm5lci4KCkl0J3MgYmVzdCB0aG F0IG1haW50YWluZXJzIHdvdWxk
IGp1c3Qgc2hpcCB0aGUgc3VibWl0dGVkIHVuaXRzICggSSB0aG luayAKdGhlIHdpbmRvdyBpcyB1
cCB0byBiZXRhICkgYXRsZWFzdCB0aGVuIHRoZSBwYWNrYWdpbm cgcGFydCB3YXMgZG9uZSBhbmQg
CnRoZSB1bml0KHMpIHdvdWxkIGJlIGluIHRoZSByZWxlYXNlIG FuZCB0aGV5IGNvdWxkIGZpeCB0
aGVtLCBlaXRoZXIgZnJvbSAKZmVlZGJhY2sgZnJvbSB1c2Vycy BvciBpZi93aGVuIHRoZXkgaGF2
ZSBnb3R0ZW4gZmFtaWxpYXIgd2l0aCBzeXN0ZW1kLgoKRml4ZX MgdG8gc3lzdGVtZCB1bml0cyB1
c3VhbGx5IGFyZW50IG1vcmUgdGhlbiBvbmxpbmVyIGNoYW5nZX MgYW55d2F5IApkdWUgdG8gdGhl
aXIgc2ltcGxpY2l0eS4KCklmIGFuIHVuaXQgYmVjb21lcyBibG 9hdGVkIG9yIGNvbXBsZXggaXQg
dXN1YWxseSBvbmx5IGhpZ2hsaWdodHMgdGhlIApmYWN0IHRoYX QgdGhlIHJlbGV2YW50IGRhZW1v
biBsYWNrcyB0aGUgYWJpbGl0eSB0byBwYXJzZSBjb25maWcgZm lsZS4KCkpCRwotLSAKZGV2ZWwg
bWFpbGluZyBsaXN0CmRldmVsQGxpc3RzLmZlZG9yYXByb2plY3 Qub3JnCmh0dHBzOi8vYWRtaW4u
ZmVkb3JhcHJvamVjdC5vcmcvbWFpbG1hbi9saXN0aW5mby9kZX ZlbA==

Commit every second to prevent too much of a position building up.
 

On Fri, Feb 10 2012 at 9:48am -0500,
Joe Thornber <thornber@redhat.com> wrote:

> On Tue, Feb 07, 2012 at 11:53:25AM -0500, Mike Snitzer wrote:
> > Shouldn't this be:
> >
> > if (bio_list_empty(&bios) || !need_commit_due_to_time(pool))
> > return;
> >
> > ?
>
> I don't think so. It's saying 'if <there's no reason to commit>'. The reasons to commit are bios being non-empty or need_commit_due_to_time.
>
> reason to commit = !bio_list_empty(&bios) || need_commit_due_to_time(pool)
> !reason_to_commit = !(!bio_list_empty(&bios) || need_commit_due_to_time(pool))
> !reason_to_commit = (bio_list_empty(&bios) && !need_commit_due_to_time(pool))
>
> Agree?

Yeap, I followed up saying something like nevermind.

> > Also, should we make the commit interval tunable (akin to ext[34]'s
> > 'commit' mount option)? Or did you defer doing so until it proves
> > worthwhile?
>
> y, tunable is on the list. Every second will do for now though.

OK.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel