* 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
* 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
* 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
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.