userspace patches for shared snapshots

userspace patches for shared snapshots

[Mikulas Patocka]

Hi

I uploaded the current version of userspace shared snapshots here:
http://people.redhat.com/mpatocka/patches/userspace/new-snapshots/lvm-2.02.60/

Mikulas

userspace patches for shared snapshots

[Mike Snitzer]

On Wed, Feb 10 2010 at  6:59pm -0500,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> Hi
> 
> I uploaded the current version of userspace shared snapshots here:
> http://people.redhat.com/mpatocka/patches/userspace/new-snapshots/lvm-2.02.60/

I've refreshed these patches to apply against the latest upstream lvm2:
http://people.redhat.com/msnitzer/patches/multisnap/lvm2/LVM2-2.02.62/

Seems these lvm2 patches can be cleaned up a bit to use wrappers much
like was done for the snapshot-merge support:
http://sources.redhat.com/git/gitweb.cgi?p=lvm2.git;a=commit;h=cad03afc54f565c

And I'm wondering whether we _really_ need a distinct 'shared_snapshot'
in 'struct logical_volume'.  I was able to remove 'merging_snapshot'
from the lvm2 snapshot-merge support:
http://sources.redhat.com/git/gitweb.cgi?p=lvm2.git;a=commit;h=fa684a97dae2e36

But I'll be reviewing the code (both kernel and lvm2) much closer in the
coming days.  Will go bottom up (kernel <-> lvm2 metadata -> lvm2).


I've done minimal testing to verify the basics are working; I have some
initial things/questions that I NOTE'd below:

# lvcreate -L 1G --sharedstore mikulas -s test/testlv1
  Logical volume "testlv1-shared" created

# dmsetup targets | grep multisnap
multisnap-snap   v1.0.0
multisnapshot    v1.0.0

# dmsetup ls
test-testlv1    (253, 0)
test-testlv1-cow        (253, 2)
test-testlv1-real       (253, 1)

# lvs
  LV             VG   Attr   LSize Origin  Snap%  Move Log Copy%  Convert
  testlv1        test owi-a- 4.00g                                       
  testlv1-shared test swi--- 1.00g testlv1 100.00                        

NOTE: strikes me as odd that the testlv1-shared Snap% is 100%.  I've
fixed the same with the snapshot-merge code before; will dig deeper in a
bit.


# lvcreate -L 128M -s -n testlv1_snap test/testlv1
  Logical volume "testlv1_snap" created

# dmsetup ls
test-testlv1    (253, 0)
test-testlv1_snap       (253, 3)
test-testlv1-cow        (253, 2)
test-testlv1-real       (253, 1)

# lvs
  LV             VG   Attr   LSize Origin  Snap%  Move Log Copy%  Convert
  testlv1        test owi-a- 4.00g                                       
  testlv1-shared test swi--- 1.00g testlv1   0.01                        
  testlv1_snap   test swi-a- 4.00g testlv1                               

NOTE: how can we claim the snapshot is 4G when the shared snap store is
only 1G?  Also, why isn't 'testlv1-shared' (a)ctive?


# lvremove -f test/testlv1_snap
  Logical volume "testlv1_snap" successfully removed
# lvremove test/testlv1-shared
  Logical volume "testlv1-shared" successfully removed

# dmsetup ls
test-testlv1    (253, 0)
test-testlv1-cow        (253, 2)

NOTE: 'test-testlv1-cow' is left dangling.  This is certainly a
side-effect of relying on info-by-name to remove 'test-testlv-cow'.
We recently switched to only using info-by-uuid.  I had to adapt the
snapshot-merge deptree code to work with info-by-uuid:
http://sources.redhat.com/git/gitweb.cgi?p=lvm2.git;a=commit;h=eaef92b02f968

test-testlv1-cow has the following uuid:
LVM-bS8vdfSDcqfY0Q2KQDJtKD3UAXbp4cmyEBeX5HhmQ50dqK3tlLAdcATFavfhFM0f-cow

But the attached lvremove -vvvv log shows we're only looking to remove
based on an origin-based uuid (we no longer try to remove by name):
LVM-bS8vdfSDcqfY0Q2KQDJtKD3UAXbp4cmy0R0pTH5hWG79ZDsnhmXTV093McEMF52v-cow

Given that with shared snapshots there is only ever one store: AFAIK it
_should_ be valid to create test-testlv1-cow using the origin's uuid
rather than a cow uuid.  It's somewhat hackish but...


# lvcreate -L 1G --sharedstore mikulas -s test/testlv1
  device-mapper: create ioctl failed: Device or resource busy
  Failed to load snapshot driver for testlv1-shared
  Logical volume "testlv1-shared" successfully removed

NOTE: manually removing 'test-testlv1-cow' obviously fixes this
-------------- next part --------------
A non-text attachment was scrubbed...
Name: remove_shared.log.gz
Type: application/x-gzip
Size: 2825 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20100225/0e850bcc/attachment.bin>

userspace patches for shared snapshots

[Mikulas Patocka]

Hi

On Thu, 25 Feb 2010, Mike Snitzer wrote:

> On Wed, Feb 10 2010 at  6:59pm -0500,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > Hi
> > 
> > I uploaded the current version of userspace shared snapshots here:
> > http://people.redhat.com/mpatocka/patches/userspace/new-snapshots/lvm-2.02.60/
> 
> I've refreshed these patches to apply against the latest upstream lvm2:
> http://people.redhat.com/msnitzer/patches/multisnap/lvm2/LVM2-2.02.62/
> 
> Seems these lvm2 patches can be cleaned up a bit to use wrappers much
> like was done for the snapshot-merge support:
> http://sources.redhat.com/git/gitweb.cgi?p=lvm2.git;a=commit;h=cad03afc54f565c

Yes. I did it at some places, but you can find more where it could be 
done.

> And I'm wondering whether we _really_ need a distinct 'shared_snapshot'
> in 'struct logical_volume'.  I was able to remove 'merging_snapshot'
> from the lvm2 snapshot-merge support:
> http://sources.redhat.com/git/gitweb.cgi?p=lvm2.git;a=commit;h=fa684a97dae2e36

We don't need "shared_snapshot" entry, the pointer could be definitely 
stuffed into some existing structure entry, but I wouldn't do it.

If you use one structure entry for multiple things, you are increasing the 
possibility of bugs (you write it as one thing and read it as another 
thing).

In my opinion, it is not worth trying to save 8 bytes per logical volume 
at the cost of increasing bug possibilities.

> But I'll be reviewing the code (both kernel and lvm2) much closer in the
> coming days.  Will go bottom up (kernel <-> lvm2 metadata -> lvm2).
> 
> 
> I've done minimal testing to verify the basics are working; I have some
> initial things/questions that I NOTE'd below:
> 
> # lvcreate -L 1G --sharedstore mikulas -s test/testlv1
>   Logical volume "testlv1-shared" created
> 
> # dmsetup targets | grep multisnap
> multisnap-snap   v1.0.0
> multisnapshot    v1.0.0
> 
> # dmsetup ls
> test-testlv1    (253, 0)
> test-testlv1-cow        (253, 2)
> test-testlv1-real       (253, 1)
> 
> # lvs
>   LV             VG   Attr   LSize Origin  Snap%  Move Log Copy%  Convert
>   testlv1        test owi-a- 4.00g                                       
>   testlv1-shared test swi--- 1.00g testlv1 100.00                        
> 
> NOTE: strikes me as odd that the testlv1-shared Snap% is 100%.  I've
> fixed the same with the snapshot-merge code before; will dig deeper in a
> bit.

This is actually bug in the kernel, it starts with the smallest possible 
size and extends the internal data structures when the first operation is 
performed. So, if you ask for status without performing any operation, it 
reports 100%.

Thanks for finding it, I overlooked it. I'l fix that.

> # lvcreate -L 128M -s -n testlv1_snap test/testlv1
>   Logical volume "testlv1_snap" created

That "-L" argument is ignored because it is a shared store. If you want to 
extend the shared store, extend "testlv1-shared" (you can't shrink it).

> # dmsetup ls
> test-testlv1    (253, 0)
> test-testlv1_snap       (253, 3)
> test-testlv1-cow        (253, 2)
> test-testlv1-real       (253, 1)
> 
> # lvs
>   LV             VG   Attr   LSize Origin  Snap%  Move Log Copy%  Convert
>   testlv1        test owi-a- 4.00g                                       
>   testlv1-shared test swi--- 1.00g testlv1   0.01                        
>   testlv1_snap   test swi-a- 4.00g testlv1                               
> 
> NOTE: how can we claim the snapshot is 4G when the shared snap store is
> only 1G?

4G is the virtual size of the snapshot. It is the same as the origin size 
(but it can be shrunk or extended). The physical size of the shared store 
is reported testlv1-shared. The physical size of individual snapshots is 
not reported because the store is shared.

> Also, why isn't 'testlv1-shared' (a)ctive?

It is not active deliberately because it shouldn't be exported in /dev/vg/ 
and the user can't do much with this LV anyway. The only thing you can do 
with this LV is to look at the size or status or extend it.

> # lvremove -f test/testlv1_snap
>   Logical volume "testlv1_snap" successfully removed
> # lvremove test/testlv1-shared
>   Logical volume "testlv1-shared" successfully removed
> 
> # dmsetup ls
> test-testlv1    (253, 0)
> test-testlv1-cow        (253, 2)
> 
> NOTE: 'test-testlv1-cow' is left dangling.  This is certainly a
> side-effect of relying on info-by-name to remove 'test-testlv-cow'.
>
> We recently switched to only using info-by-uuid.  I had to adapt the
> snapshot-merge deptree code to work with info-by-uuid:
> http://sources.redhat.com/git/gitweb.cgi?p=lvm2.git;a=commit;h=eaef92b02f968
> 
> test-testlv1-cow has the following uuid:
> LVM-bS8vdfSDcqfY0Q2KQDJtKD3UAXbp4cmyEBeX5HhmQ50dqK3tlLAdcATFavfhFM0f-cow
> 
> But the attached lvremove -vvvv log shows we're only looking to remove
> based on an origin-based uuid (we no longer try to remove by name):
> LVM-bS8vdfSDcqfY0Q2KQDJtKD3UAXbp4cmy0R0pTH5hWG79ZDsnhmXTV093McEMF52v-cow
> 
> Given that with shared snapshots there is only ever one store: AFAIK it
> _should_ be valid to create test-testlv1-cow using the origin's uuid
> rather than a cow uuid.  It's somewhat hackish but...
> 
> 
> # lvcreate -L 1G --sharedstore mikulas -s test/testlv1
>   device-mapper: create ioctl failed: Device or resource busy
>   Failed to load snapshot driver for testlv1-shared
>   Logical volume "testlv1-shared" successfully removed
> 
> NOTE: manually removing 'test-testlv1-cow' obviously fixes this

That dangling device doesn't happen for me on 2.02.60, so it was likely 
introduced when porting to 2.02.62.

I don't know how this info-by-uuid works. I haven't yet looked at it.

Mikulas

userspace patches for shared snapshots

[Mike Snitzer]

On Thu, Feb 25 2010 at 11:52pm -0500,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> Hi
> 
> On Thu, 25 Feb 2010, Mike Snitzer wrote:
> 
> > On Wed, Feb 10 2010 at  6:59pm -0500,
> > Mikulas Patocka <mpatocka@redhat.com> wrote:
> > 
> > > Hi
> > > 
> > > I uploaded the current version of userspace shared snapshots here:
> > > http://people.redhat.com/mpatocka/patches/userspace/new-snapshots/lvm-2.02.60/
> > 
> > I've refreshed these patches to apply against the latest upstream lvm2:
> > http://people.redhat.com/msnitzer/patches/multisnap/lvm2/LVM2-2.02.62/
> > 
> > Seems these lvm2 patches can be cleaned up a bit to use wrappers much
> > like was done for the snapshot-merge support:
> > http://sources.redhat.com/git/gitweb.cgi?p=lvm2.git;a=commit;h=cad03afc54f565c
> 
> Yes. I did it at some places, but you can find more where it could be 
> done.

Yeap, my recent patch posting adds find_shared_cow() and
lv_is_shared_cow().  This helps clean up the code to be more clear.
 
> > And I'm wondering whether we _really_ need a distinct 'shared_snapshot'
> > in 'struct logical_volume'.  I was able to remove 'merging_snapshot'
> > from the lvm2 snapshot-merge support:
> > http://sources.redhat.com/git/gitweb.cgi?p=lvm2.git;a=commit;h=fa684a97dae2e36
> 
> We don't need "shared_snapshot" entry, the pointer could be definitely 
> stuffed into some existing structure entry, but I wouldn't do it.
> 
> If you use one structure entry for multiple things, you are increasing the 
> possibility of bugs (you write it as one thing and read it as another 
> thing).
> 
> In my opinion, it is not worth trying to save 8 bytes per logical volume 
> at the cost of increasing bug possibilities.

I was of the same opinion when Alasdair suggested I do the same
(removing 'merging_snapshot').  But it actually doesn't pose a risk once
you have proper wrappers in place.  Do you agree? (see my patch#3 in my
recent patch series).

> > # lvs
> >   LV             VG   Attr   LSize Origin  Snap%  Move Log Copy%  Convert
> >   testlv1        test owi-a- 4.00g                                       
> >   testlv1-shared test swi--- 1.00g testlv1 100.00                        
> > 
> > NOTE: strikes me as odd that the testlv1-shared Snap% is 100%.  I've
> > fixed the same with the snapshot-merge code before; will dig deeper in a
> > bit.
> 
> This is actually bug in the kernel, it starts with the smallest possible 
> size and extends the internal data structures when the first operation is 
> performed. So, if you ask for status without performing any operation, it 
> reports 100%.
> 
> Thanks for finding it, I overlooked it. I'l fix that.

Sure, I'll be interested to see your fix.  I'm not clear on what you're
referring to.

> > # lvcreate -L 128M -s -n testlv1_snap test/testlv1
> >   Logical volume "testlv1_snap" created
> 
> That "-L" argument is ignored because it is a shared store. If you want to 
> extend the shared store, extend "testlv1-shared" (you can't shrink it).

OK, but if we can extend and reduce the virtual size (as you shared
below) shouldn't we honor the requested size (-L) as the virtual
snapshot's size?

> > # dmsetup ls
> > test-testlv1    (253, 0)
> > test-testlv1_snap       (253, 3)
> > test-testlv1-cow        (253, 2)
> > test-testlv1-real       (253, 1)
> > 
> > # lvs
> >   LV             VG   Attr   LSize Origin  Snap%  Move Log Copy%  Convert
> >   testlv1        test owi-a- 4.00g                                       
> >   testlv1-shared test swi--- 1.00g testlv1   0.01                        
> >   testlv1_snap   test swi-a- 4.00g testlv1                               
> > 
> > NOTE: how can we claim the snapshot is 4G when the shared snap store is
> > only 1G?
> 
> 4G is the virtual size of the snapshot. It is the same as the origin size 
> (but it can be shrunk or extended). The physical size of the shared store 
> is reported testlv1-shared. The physical size of individual snapshots is 
> not reported because the store is shared.

Mike

userspace patches for shared snapshots

[Mike Snitzer]

On Fri, Feb 26 2010 at  4:17pm -0500,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Thu, Feb 25 2010 at 11:52pm -0500,
> Mikulas Patocka <mpatocka@redhat.com> wrote:

> > > # lvs
> > >   LV             VG   Attr   LSize Origin  Snap%  Move Log Copy%  Convert
> > >   testlv1        test owi-a- 4.00g                                       
> > >   testlv1-shared test swi--- 1.00g testlv1 100.00                        
> > > 
> > > NOTE: strikes me as odd that the testlv1-shared Snap% is 100%.  I've
> > > fixed the same with the snapshot-merge code before; will dig deeper in a
> > > bit.
> > 
> > This is actually bug in the kernel, it starts with the smallest possible 
> > size and extends the internal data structures when the first operation is 
> > performed. So, if you ask for status without performing any operation, it 
> > reports 100%.
> > 
> > Thanks for finding it, I overlooked it. I'l fix that.
> 
> Sure, I'll be interested to see your fix.  I'm not clear on what you're
> referring to.

BTW, the patches I posted earlier change this behaviour, in particular
this patch:
http://people.redhat.com/msnitzer/patches/multisnap/lvm2/LVM2-2.02.62/lvm-shared-eliminate-shared_snapshot-in-lv.patch

Now the -shared store is hidden:

# lvs
  LV      VG   Attr   LSize Origin Snap%  Move Log Copy%  Convert
  testlv1 test owi-a- 4.00g                                      

# lvs -a
  LV               VG   Attr   LSize Origin  Snap%  Move Log Copy%  Convert
  testlv1          test owi-a- 4.00g                                       
  [testlv1-shared] test swi--- 1.00g testlv1   0.00                        

You'll also note that Snap% is no longer 100%

userspace patches for shared snapshots

[Mikulas Patocka]

On Wed, 3 Mar 2010, Mike Snitzer wrote:

> On Fri, Feb 26 2010 at  4:17pm -0500,
> Mike Snitzer <snitzer@redhat.com> wrote:
> 
> > On Thu, Feb 25 2010 at 11:52pm -0500,
> > Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > > > # lvs
> > > >   LV             VG   Attr   LSize Origin  Snap%  Move Log Copy%  Convert
> > > >   testlv1        test owi-a- 4.00g                                       
> > > >   testlv1-shared test swi--- 1.00g testlv1 100.00                        
> > > > 
> > > > NOTE: strikes me as odd that the testlv1-shared Snap% is 100%.  I've
> > > > fixed the same with the snapshot-merge code before; will dig deeper in a
> > > > bit.
> > > 
> > > This is actually bug in the kernel, it starts with the smallest possible 
> > > size and extends the internal data structures when the first operation is 
> > > performed. So, if you ask for status without performing any operation, it 
> > > reports 100%.
> > > 
> > > Thanks for finding it, I overlooked it. I'l fix that.
> > 
> > Sure, I'll be interested to see your fix.  I'm not clear on what you're
> > referring to.

Each time someone locks the exception store, it checks the device size. If 
the size of the device has grown, the exception store extends itself.

So the problem was, that if you queried the percentage the first time 
before any locking operation, it wasn't extended yet and it reported 100%. 
I fixed it by adding a test for extension just after initialization.

> BTW, the patches I posted earlier change this behaviour, in particular
> this patch:
> http://people.redhat.com/msnitzer/patches/multisnap/lvm2/LVM2-2.02.62/lvm-shared-eliminate-shared_snapshot-in-lv.patch
> 
> Now the -shared store is hidden:
> 
> # lvs
>   LV      VG   Attr   LSize Origin Snap%  Move Log Copy%  Convert
>   testlv1 test owi-a- 4.00g                                      
> 
> # lvs -a
>   LV               VG   Attr   LSize Origin  Snap%  Move Log Copy%  Convert
>   testlv1          test owi-a- 4.00g                                       
>   [testlv1-shared] test swi--- 1.00g testlv1   0.00                        
> 
> You'll also note that Snap% is no longer 100%

No, it didn't fix that. See above.

I went through your kernel patch and uploaded a new version at 
http://people.redhat.com/mpatocka/patches/kernel/new-snapshots/r16/ . It 
contains fix for this 100% usage. It contains most of your changes (I 
rolled back that cycle change).

Mikulas

userspace patches for shared snapshots

[Mike Snitzer]

On Thu, Mar 04 2010 at  5:11am -0500,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> 
> 
> On Wed, 3 Mar 2010, Mike Snitzer wrote:
> 
> > On Fri, Feb 26 2010 at  4:17pm -0500,
> > Mike Snitzer <snitzer@redhat.com> wrote:
> > 
> > > On Thu, Feb 25 2010 at 11:52pm -0500,
> > > Mikulas Patocka <mpatocka@redhat.com> wrote:
> > 
> > > > > # lvs
> > > > >   LV             VG   Attr   LSize Origin  Snap%  Move Log Copy%  Convert
> > > > >   testlv1        test owi-a- 4.00g                                       
> > > > >   testlv1-shared test swi--- 1.00g testlv1 100.00                        
> > > > > 
> > > > > NOTE: strikes me as odd that the testlv1-shared Snap% is 100%.  I've
> > > > > fixed the same with the snapshot-merge code before; will dig deeper in a
> > > > > bit.
> > > > 
> > > > This is actually bug in the kernel, it starts with the smallest possible 
> > > > size and extends the internal data structures when the first operation is 
> > > > performed. So, if you ask for status without performing any operation, it 
> > > > reports 100%.
> > > > 
> > > > Thanks for finding it, I overlooked it. I'l fix that.
> > > 
> > > Sure, I'll be interested to see your fix.  I'm not clear on what you're
> > > referring to.
> 
> Each time someone locks the exception store, it checks the device size. If 
> the size of the device has grown, the exception store extends itself.
> 
> So the problem was, that if you queried the percentage the first time 
> before any locking operation, it wasn't extended yet and it reported 100%. 
> I fixed it by adding a test for extension just after initialization.
> 
> > BTW, the patches I posted earlier change this behaviour, in particular
> > this patch:
> > http://people.redhat.com/msnitzer/patches/multisnap/lvm2/LVM2-2.02.62/lvm-shared-eliminate-shared_snapshot-in-lv.patch
> > 
> > Now the -shared store is hidden:
> > 
> > # lvs
> >   LV      VG   Attr   LSize Origin Snap%  Move Log Copy%  Convert
> >   testlv1 test owi-a- 4.00g                                      
> > 
> > # lvs -a
> >   LV               VG   Attr   LSize Origin  Snap%  Move Log Copy%  Convert
> >   testlv1          test owi-a- 4.00g                                       
> >   [testlv1-shared] test swi--- 1.00g testlv1   0.00                        
> > 
> > You'll also note that Snap% is no longer 100%
> 
> No, it didn't fix that. See above.

Interesting, yeap you're right.

> I went through your kernel patch and uploaded a new version at 
> http://people.redhat.com/mpatocka/patches/kernel/new-snapshots/r16/ . It 
> contains fix for this 100% usage. It contains most of your changes (I 
> rolled back that cycle change).

OK, I have made further edits that layered ontop of my other changes
(primarily just making use of __func__ rather than hardcoding the
function name in ERROR messages).  I'll have a look at your r16 and
hopefully they'll apply there too.

As for the 'goto midcycle', why do you want to keep that?  IMO it's
quite ugly.  What was wrong with what I did?

@@ -638,15 +641,7 @@ dispatch_write:
        }
 
        i = 0;
-       goto midcycle;
        for (; i < DM_MULTISNAP_MAX_CHUNKS_TO_REMAP; i++) {
-               r = s->store->query_next_remap(s->p, chunk);
-               if (unlikely(r < 0))
-                       goto free_err_endio;
-               if (likely(!r))
-                       break;
-
-midcycle:
                s->store->add_next_remap(s->p, &pe->desc[i], &new_chunk);
                if (unlikely(dm_multisnap_has_error(s)))
                        goto free_err_endio;
@@ -654,6 +649,14 @@ midcycle:
                dests[i].bdev = s->snapshot->bdev;
                dests[i].sector = chunk_to_sector(s, new_chunk);
                dests[i].count = s->chunk_size >> SECTOR_SHIFT;
+
+               r = s->store->query_next_remap(s->p, chunk);
+               if (unlikely(r < 0))
+                       goto free_err_endio;
+               if (likely(!r)) {
+                       i++;
+                       break;
+               }
        }
 
        dispatch_kcopyd(s, pe, 0, chunk, bio, dests, i);


Is it that I made the loop less logically ideal (where ideal is:
query_next_remap then add_next_remap)?  Problem is you already did the
first query_next_remap.  Anyway, I'd be very surprised if that goto
makes it past Alasdair.. but either way its not a big deal to me.

Mike

edits for r16 of shared snapshot patches [was: Re: userspace patches for shared snapshots]

[Mike Snitzer]

On Thu, Mar 04 2010 at  8:22am -0500,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Thu, Mar 04 2010 at  5:11am -0500,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > I went through your kernel patch and uploaded a new version at 
> > http://people.redhat.com/mpatocka/patches/kernel/new-snapshots/r16/ . It 
> > contains fix for this 100% usage. It contains most of your changes (I 
> > rolled back that cycle change).
> 
> OK, I have made further edits that layered ontop of my other changes
> (primarily just making use of __func__ rather than hardcoding the
> function name in ERROR messages).  I'll have a look at your r16 and
> hopefully they'll apply there too.

I've uploaded my edits to r16 here:
http://people.redhat.com/msnitzer/patches/multisnap/kernel/2.6.33/r16a/

You can see the edits here:
http://people.redhat.com/msnitzer/patches/multisnap/kernel/2.6.33/r16a/r16_edits.patch

Boils down to:
* use __func__ rather than hardcoding the function name
  - this fixed ~3 inconsistencies (incorrect function names) and should
    help if/when we do any function renaming in later phases of review
* s/Tomonorig/Tomonori/
* a few more typo fixups
* a few more whitespace cleanups.

I'd appreciate it if you pull these in for the basis of any follow-on
release you make.

Also, I'm working to improve Documentation/device-mapper/dm-multisnapshot.txt
I'll share this a bit later (hopefully today).

Mike

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

edits for r16 of shared snapshot patches [was: Re: userspace patches for shared snapshots]

[Mike Snitzer]

On Thu, Mar 04 2010 at  8:22am -0500,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Thu, Mar 04 2010 at  5:11am -0500,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > I went through your kernel patch and uploaded a new version at 
> > http://people.redhat.com/mpatocka/patches/kernel/new-snapshots/r16/ . It 
> > contains fix for this 100% usage. It contains most of your changes (I 
> > rolled back that cycle change).
> 
> OK, I have made further edits that layered ontop of my other changes
> (primarily just making use of __func__ rather than hardcoding the
> function name in ERROR messages).  I'll have a look at your r16 and
> hopefully they'll apply there too.

I've uploaded my edits to r16 here:
http://people.redhat.com/msnitzer/patches/multisnap/kernel/2.6.33/r16a/

You can see the edits here:
http://people.redhat.com/msnitzer/patches/multisnap/kernel/2.6.33/r16a/r16_edits.patch

Boils down to:
* use __func__ rather than hardcoding the function name
  - this fixed ~3 inconsistencies (incorrect function names) and should
    help if/when we do any function renaming in later phases of review
* s/Tomonorig/Tomonori/
* a few more typo fixups
* a few more whitespace cleanups.

I'd appreciate it if you pull these in for the basis of any follow-on
release you make.

Also, I'm working to improve Documentation/device-mapper/dm-multisnapshot.txt
I'll share this a bit later (hopefully today).

Mike

userspace patches for shared snapshots

[Mikulas Patocka]

> > I went through your kernel patch and uploaded a new version at 
> > http://people.redhat.com/mpatocka/patches/kernel/new-snapshots/r16/ . It 
> > contains fix for this 100% usage. It contains most of your changes (I 
> > rolled back that cycle change).
> 
> OK, I have made further edits that layered ontop of my other changes
> (primarily just making use of __func__ rather than hardcoding the
> function name in ERROR messages).  I'll have a look at your r16 and
> hopefully they'll apply there too.
> 
> As for the 'goto midcycle', why do you want to keep that?  IMO it's
> quite ugly.  What was wrong with what I did?
> 
> @@ -638,15 +641,7 @@ dispatch_write:
>         }
>  
>         i = 0;
> -       goto midcycle;
>         for (; i < DM_MULTISNAP_MAX_CHUNKS_TO_REMAP; i++) {
> -               r = s->store->query_next_remap(s->p, chunk);
> -               if (unlikely(r < 0))
> -                       goto free_err_endio;
> -               if (likely(!r))
> -                       break;
> -
> -midcycle:
>                 s->store->add_next_remap(s->p, &pe->desc[i], &new_chunk);
>                 if (unlikely(dm_multisnap_has_error(s)))
>                         goto free_err_endio;
> @@ -654,6 +649,14 @@ midcycle:
>                 dests[i].bdev = s->snapshot->bdev;
>                 dests[i].sector = chunk_to_sector(s, new_chunk);
>                 dests[i].count = s->chunk_size >> SECTOR_SHIFT;
> +
> +               r = s->store->query_next_remap(s->p, chunk);
> +               if (unlikely(r < 0))
> +                       goto free_err_endio;
> +               if (likely(!r)) {
> +                       i++;
> +                       break;
> +               }
>         }
>  
>         dispatch_kcopyd(s, pe, 0, chunk, bio, dests, i);
> 
> 
> Is it that I made the loop less logically ideal (where ideal is:
> query_next_remap then add_next_remap)?  Problem is you already did the
> first query_next_remap.  Anyway, I'd be very surprised if that goto
> makes it past Alasdair.. but either way its not a big deal to me.
> 
> Mike

The patch causes one more excess call to s->store->query_next_remap (it 
wouldn't matter much).

But more importantly, the patch makes the code harder to understand, not 
easier:

The code should be written to follow the thinking of the programmer. So, 
if the logic is that we first ask for the chunk to remap and then perform 
that remapping --- then asking for the remap should be before performing 
that remap in the source code (your patch reversed that).

In my code, since we asked for the first remap already (before calling 
dm_multisnap_alloc_pending_exception), the first ask inside the cycle is 
skipped with goto. I don't see anything wrong with it.

Just don't follow dummy Dijsktra's rules.

Mikulas

userspace patches for shared snapshots

[Alasdair G Kergon]

A forward goto is unlikely to get through without a decent name (I don't
think 'midcycle' tells me much useful) and (very brief) explanation of the
non-standard code structure in the comment: it'll need people like me to
understand that any other way of structuring it (including moving some
of the code out to a function or 'rotating' the loop or having an ugly
'first time' variable) would be significantly (not merely slightly) more
convoluted or harder to understand.

Alasdair

Re: edits for r16 of shared snapshot patches [was: Re: userspace patches for shared snapshots]

[Mikulas Patocka]

> You can see the edits here:
> http://people.redhat.com/msnitzer/patches/multisnap/kernel/2.6.33/r16a/r16_edits.patch
> 
> Boils down to:
> * use __func__ rather than hardcoding the function name
>   - this fixed ~3 inconsistencies (incorrect function names) and should
>     help if/when we do any function renaming in later phases of review

It consumes one more stack word for every function where this conversion 
was performed ... I don't know if it's worth it ... likely it doesn't 
matter.

The problem here is that GCC is preallocating the space for all outgoing 
arguments in the function prologue, so if the function has something like 
this
if (bug_happened) {
	printk("%s: bug happened: %d %d %d %d %d %d %d %d", __func__, a, 
b, c, d, e, f, g, h, i);
}
the whole function always wastes 10 stack words, even if the bug doesn't 
happen :(

But that one word because of __func__ maybe doesn't matter (there are much 
more wasted already in these printks). Or do you think that it's better to 
not do this __func__ conversion?

Mikulas

> * s/Tomonorig/Tomonori/
> * a few more typo fixups
> * a few more whitespace cleanups.
> 
> I'd appreciate it if you pull these in for the basis of any follow-on
> release you make.
> 
> Also, I'm working to improve Documentation/device-mapper/dm-multisnapshot.txt
> I'll share this a bit later (hopefully today).
> 
> Mike
> 

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

edits for r16 of shared snapshot patches [was: Re: userspace patches for shared snapshots]

[Mikulas Patocka]

> You can see the edits here:
> http://people.redhat.com/msnitzer/patches/multisnap/kernel/2.6.33/r16a/r16_edits.patch
> 
> Boils down to:
> * use __func__ rather than hardcoding the function name
>   - this fixed ~3 inconsistencies (incorrect function names) and should
>     help if/when we do any function renaming in later phases of review

It consumes one more stack word for every function where this conversion 
was performed ... I don't know if it's worth it ... likely it doesn't 
matter.

The problem here is that GCC is preallocating the space for all outgoing 
arguments in the function prologue, so if the function has something like 
this
if (bug_happened) {
	printk("%s: bug happened: %d %d %d %d %d %d %d %d", __func__, a, 
b, c, d, e, f, g, h, i);
}
the whole function always wastes 10 stack words, even if the bug doesn't 
happen :(

But that one word because of __func__ maybe doesn't matter (there are much 
more wasted already in these printks). Or do you think that it's better to 
not do this __func__ conversion?

Mikulas

> * s/Tomonorig/Tomonori/
> * a few more typo fixups
> * a few more whitespace cleanups.
> 
> I'd appreciate it if you pull these in for the basis of any follow-on
> release you make.
> 
> Also, I'm working to improve Documentation/device-mapper/dm-multisnapshot.txt
> I'll share this a bit later (hopefully today).
> 
> Mike
> 

Re: edits for r16 of shared snapshot patches [was: Re: userspace patches for shared snapshots]

[Mike Snitzer]

On Tue, Mar 09 2010 at  3:41am -0500,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> > You can see the edits here:
> > http://people.redhat.com/msnitzer/patches/multisnap/kernel/2.6.33/r16a/r16_edits.patch
> > 
> > Boils down to:
> > * use __func__ rather than hardcoding the function name
> >   - this fixed ~3 inconsistencies (incorrect function names) and should
> >     help if/when we do any function renaming in later phases of review
> 
> It consumes one more stack word for every function where this conversion 
> was performed ... I don't know if it's worth it ... likely it doesn't 
> matter.
> 
> The problem here is that GCC is preallocating the space for all outgoing 
> arguments in the function prologue, so if the function has something like 
> this
> if (bug_happened) {
> 	printk("%s: bug happened: %d %d %d %d %d %d %d %d", __func__, a, 
> b, c, d, e, f, g, h, i);
> }
> the whole function always wastes 10 stack words, even if the bug doesn't 
> happen :(
> 
> But that one word because of __func__ maybe doesn't matter (there are much 
> more wasted already in these printks). Or do you think that it's better to 
> not do this __func__ conversion?

I think the use of __func__ reduces the maintenance cost of keeping the
printk updated if the code were to change.  Like I said, I found ~3
remaining function name inconsistencies.  Plus I had already fixed a few
others in my previous large whitespace patch.

Anyway, I understand your point on the extra word associated with using
__func__ but feel using it is more helpful.

Mike