From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mikulas Patocka Subject: Re: rebased snapshot-merge patches Date: Sat, 12 Sep 2009 03:54:36 -0400 (EDT) Message-ID: References: <20090831220834.GA15126@redhat.com> <20090911065135.GA3210@redhat.com> <20090912065608.GA1864@redhat.com> Reply-To: device-mapper development Mime-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Return-path: In-Reply-To: <20090912065608.GA1864@redhat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dm-devel-bounces@redhat.com Errors-To: dm-devel-bounces@redhat.com To: Mike Snitzer Cc: device-mapper development , lvm-devel@redhat.com List-Id: dm-devel.ids On Sat, 12 Sep 2009, Mike Snitzer wrote: > On Fri, Sep 11 2009 at 2:51am -0400, > Mike Snitzer wrote: > > > Mikulas, > > > > I ran with your suggestion and it works quite well (results below). > ... > > The changes are detailed at the end of this mail. I still have a "TODO" > > that you'll see in the patch that speaks to needing to have > > snapshot_merge_process() check all origin chunks via __chunk_is_tracked() > > rather than just the one check of the first chunk. Your feedback here > > would be much appreciated. I'm not completely clear on the intent of > > the __chunk_is_tracked() check there and what it now needs to do. > > I believe I've sorted this out. The TODO should've read something like: > snapshot_merge_process() should delay the merging of _all_ chunks > that have in-progress writes; not just the first chunk in the region > that is to be merged. > > The revised patch (relative to Jon's unified snapshot tree) is here: > http://people.redhat.com/msnitzer/patches/snapshot-merge/kernel_unified/2.6.31/dm-snapshot-merge-use-larger-io-when-merging.patch > > Pretty sure it _should_ work; but can't test until we get this unified > snapshot patchset working properly. Yes, that's right. The purpose of the tracking while merging is to exclude pending writes to the region that is being merged --- so we must wait for writes on all chunks. The patch seems fine. Mikulas From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mikulas Patocka Date: Sat, 12 Sep 2009 03:54:36 -0400 (EDT) Subject: Re: rebased snapshot-merge patches In-Reply-To: <20090912065608.GA1864@redhat.com> References: <20090831220834.GA15126@redhat.com> <20090911065135.GA3210@redhat.com> <20090912065608.GA1864@redhat.com> Message-ID: List-Id: To: lvm-devel@redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit On Sat, 12 Sep 2009, Mike Snitzer wrote: > On Fri, Sep 11 2009 at 2:51am -0400, > Mike Snitzer wrote: > > > Mikulas, > > > > I ran with your suggestion and it works quite well (results below). > ... > > The changes are detailed at the end of this mail. I still have a "TODO" > > that you'll see in the patch that speaks to needing to have > > snapshot_merge_process() check all origin chunks via __chunk_is_tracked() > > rather than just the one check of the first chunk. Your feedback here > > would be much appreciated. I'm not completely clear on the intent of > > the __chunk_is_tracked() check there and what it now needs to do. > > I believe I've sorted this out. The TODO should've read something like: > snapshot_merge_process() should delay the merging of _all_ chunks > that have in-progress writes; not just the first chunk in the region > that is to be merged. > > The revised patch (relative to Jon's unified snapshot tree) is here: > http://people.redhat.com/msnitzer/patches/snapshot-merge/kernel_unified/2.6.31/dm-snapshot-merge-use-larger-io-when-merging.patch > > Pretty sure it _should_ work; but can't test until we get this unified > snapshot patchset working properly. Yes, that's right. The purpose of the tracking while merging is to exclude pending writes to the region that is being merged --- so we must wait for writes on all chunks. The patch seems fine. Mikulas