* userspace patches for shared snapshots
@ 2010-02-10 23:59 Mikulas Patocka
2010-02-25 22:13 ` Mike Snitzer
0 siblings, 1 reply; 14+ messages in thread
From: Mikulas Patocka @ 2010-02-10 23:59 UTC (permalink / raw)
To: lvm-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* userspace patches for shared snapshots
2010-02-10 23:59 userspace patches for shared snapshots Mikulas Patocka
@ 2010-02-25 22:13 ` Mike Snitzer
2010-02-26 4:52 ` Mikulas Patocka
0 siblings, 1 reply; 14+ messages in thread
From: Mike Snitzer @ 2010-02-25 22:13 UTC (permalink / raw)
To: lvm-devel
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>
^ permalink raw reply [flat|nested] 14+ messages in thread
* userspace patches for shared snapshots
2010-02-25 22:13 ` Mike Snitzer
@ 2010-02-26 4:52 ` Mikulas Patocka
2010-02-26 21:17 ` Mike Snitzer
0 siblings, 1 reply; 14+ messages in thread
From: Mikulas Patocka @ 2010-02-26 4:52 UTC (permalink / raw)
To: lvm-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* userspace patches for shared snapshots
2010-02-26 4:52 ` Mikulas Patocka
@ 2010-02-26 21:17 ` Mike Snitzer
2010-03-03 22:37 ` Mike Snitzer
0 siblings, 1 reply; 14+ messages in thread
From: Mike Snitzer @ 2010-02-26 21:17 UTC (permalink / raw)
To: lvm-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* userspace patches for shared snapshots
2010-02-26 21:17 ` Mike Snitzer
@ 2010-03-03 22:37 ` Mike Snitzer
2010-03-04 10:11 ` Mikulas Patocka
0 siblings, 1 reply; 14+ messages in thread
From: Mike Snitzer @ 2010-03-03 22:37 UTC (permalink / raw)
To: lvm-devel
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%
^ permalink raw reply [flat|nested] 14+ messages in thread
* userspace patches for shared snapshots
2010-03-03 22:37 ` Mike Snitzer
@ 2010-03-04 10:11 ` Mikulas Patocka
2010-03-04 13:22 ` Mike Snitzer
0 siblings, 1 reply; 14+ messages in thread
From: Mikulas Patocka @ 2010-03-04 10:11 UTC (permalink / raw)
To: lvm-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* userspace patches for shared snapshots
2010-03-04 10:11 ` Mikulas Patocka
@ 2010-03-04 13:22 ` Mike Snitzer
2010-03-05 17:47 ` Mike Snitzer
2010-03-09 3:18 ` userspace patches for shared snapshots Mikulas Patocka
0 siblings, 2 replies; 14+ messages in thread
From: Mike Snitzer @ 2010-03-04 13:22 UTC (permalink / raw)
To: lvm-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* edits for r16 of shared snapshot patches [was: Re: userspace patches for shared snapshots]
2010-03-04 13:22 ` Mike Snitzer
@ 2010-03-05 17:47 ` Mike Snitzer
2010-03-09 3:18 ` userspace patches for shared snapshots Mikulas Patocka
1 sibling, 0 replies; 14+ messages in thread
From: Mike Snitzer @ 2010-03-05 17:47 UTC (permalink / raw)
To: Mikulas Patocka; +Cc: dm-devel, LVM2 development
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* edits for r16 of shared snapshot patches [was: Re: userspace patches for shared snapshots]
@ 2010-03-05 17:47 ` Mike Snitzer
0 siblings, 0 replies; 14+ messages in thread
From: Mike Snitzer @ 2010-03-05 17:47 UTC (permalink / raw)
To: lvm-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* userspace patches for shared snapshots
2010-03-04 13:22 ` Mike Snitzer
2010-03-05 17:47 ` Mike Snitzer
@ 2010-03-09 3:18 ` Mikulas Patocka
2010-03-09 3:38 ` Alasdair G Kergon
1 sibling, 1 reply; 14+ messages in thread
From: Mikulas Patocka @ 2010-03-09 3:18 UTC (permalink / raw)
To: lvm-devel
> > 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* userspace patches for shared snapshots
2010-03-09 3:18 ` userspace patches for shared snapshots Mikulas Patocka
@ 2010-03-09 3:38 ` Alasdair G Kergon
0 siblings, 0 replies; 14+ messages in thread
From: Alasdair G Kergon @ 2010-03-09 3:38 UTC (permalink / raw)
To: lvm-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: edits for r16 of shared snapshot patches [was: Re: userspace patches for shared snapshots]
2010-03-05 17:47 ` Mike Snitzer
@ 2010-03-09 8:41 ` Mikulas Patocka
-1 siblings, 0 replies; 14+ messages in thread
From: Mikulas Patocka @ 2010-03-09 8:41 UTC (permalink / raw)
To: Mike Snitzer; +Cc: dm-devel, LVM2 development
> 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* edits for r16 of shared snapshot patches [was: Re: userspace patches for shared snapshots]
@ 2010-03-09 8:41 ` Mikulas Patocka
0 siblings, 0 replies; 14+ messages in thread
From: Mikulas Patocka @ 2010-03-09 8:41 UTC (permalink / raw)
To: lvm-devel
> 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
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: edits for r16 of shared snapshot patches [was: Re: userspace patches for shared snapshots]
2010-03-09 8:41 ` Mikulas Patocka
(?)
@ 2010-03-10 20:45 ` Mike Snitzer
-1 siblings, 0 replies; 14+ messages in thread
From: Mike Snitzer @ 2010-03-10 20:45 UTC (permalink / raw)
To: Mikulas Patocka; +Cc: dm-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2010-03-10 20:45 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-02-10 23:59 userspace patches for shared snapshots Mikulas Patocka
2010-02-25 22:13 ` Mike Snitzer
2010-02-26 4:52 ` Mikulas Patocka
2010-02-26 21:17 ` Mike Snitzer
2010-03-03 22:37 ` Mike Snitzer
2010-03-04 10:11 ` Mikulas Patocka
2010-03-04 13:22 ` Mike Snitzer
2010-03-05 17:47 ` edits for r16 of shared snapshot patches [was: Re: userspace patches for shared snapshots] Mike Snitzer
2010-03-05 17:47 ` Mike Snitzer
2010-03-09 8:41 ` Mikulas Patocka
2010-03-09 8:41 ` Mikulas Patocka
2010-03-10 20:45 ` Mike Snitzer
2010-03-09 3:18 ` userspace patches for shared snapshots Mikulas Patocka
2010-03-09 3:38 ` Alasdair G Kergon
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.