| Message ID | 20200218100740.2228521-1-f.gruenbichler@proxmox.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | mirror: implement incremental and bitmap modes | expand |
Patchew URL: https://patchew.org/QEMU/20200218100740.2228521-1-f.gruenbichler@proxmox.com/ Hi, This series seems to have some coding style problems. See output below for more information: Subject: [RFC qemu 0/6] mirror: implement incremental and bitmap modes Message-id: 20200218100740.2228521-1-f.gruenbichler@proxmox.com Type: series === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/20200218094402.26625-1-philmd@redhat.com -> patchew/20200218094402.26625-1-philmd@redhat.com * [new tag] patchew/20200218100740.2228521-1-f.gruenbichler@proxmox.com -> patchew/20200218100740.2228521-1-f.gruenbichler@proxmox.com Switched to a new branch 'test' 50da7dd mirror: move some checks to QMP 2236985 iotests: add test for bitmap mirror b65aa31 mirror: switch to bdrv_dirty_bitmap_merge_internal c03e05f mirror: add check for bitmap-mode without bitmap 1e909f2 drive-mirror: add support for conditional and always bitmap sync modes e7baad3 drive-mirror: add support for sync=bitmap mode=never === OUTPUT BEGIN === 1/6 Checking commit e7baad3a765b (drive-mirror: add support for sync=bitmap mode=never) 2/6 Checking commit 1e909f289147 (drive-mirror: add support for conditional and always bitmap sync modes) ERROR: Missing Signed-off-by: line(s) total: 1 errors, 0 warnings, 48 lines checked Patch 2/6 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 3/6 Checking commit c03e05f9f733 (mirror: add check for bitmap-mode without bitmap) 4/6 Checking commit b65aa312e64e (mirror: switch to bdrv_dirty_bitmap_merge_internal) 5/6 Checking commit 2236985625be (iotests: add test for bitmap mirror) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #24: new file mode 100755 total: 0 errors, 1 warnings, 3397 lines checked Patch 5/6 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 6/6 Checking commit 50da7ddd187c (mirror: move some checks to QMP) === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20200218100740.2228521-1-f.gruenbichler@proxmox.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
On 2/18/20 5:07 AM, Fabian Grünbichler wrote: > picking up on John's in-progress patch series from last summer, this is > a stab at rebasing and adding test cases for the low-hanging fruits: > > - bitmap mirror mode with always/on-success/never bitmap sync mode > - incremental mirror mode as sugar for bitmap + on-success > > Fabian Grünbichler (4): > mirror: add check for bitmap-mode without bitmap > mirror: switch to bdrv_dirty_bitmap_merge_internal > iotests: add test for bitmap mirror > mirror: move some checks to QMP > > John Snow (2): > drive-mirror: add support for sync=bitmap mode=never > drive-mirror: add support for conditional and always bitmap sync modes > > include/block/block_int.h | 4 +- > block/mirror.c | 96 +- > blockdev.c | 71 +- > tests/test-block-iothread.c | 4 +- > qapi/block-core.json | 29 +- > tests/qemu-iotests/284 | 547 +++++++ > tests/qemu-iotests/284.out | 2846 +++++++++++++++++++++++++++++++++++ > tests/qemu-iotests/group | 1 + > 8 files changed, 3567 insertions(+), 31 deletions(-) > create mode 100755 tests/qemu-iotests/284 > create mode 100644 tests/qemu-iotests/284.out > Hi Fabian! Thanks for picking this up. I'm a bit behind on my mail, but this on my list to look at. (Hint to other maintainers: It might be a while.) --js
On February 25, 2020 10:54 pm, John Snow wrote: > On 2/18/20 5:07 AM, Fabian Grünbichler wrote: >> picking up on John's in-progress patch series from last summer, this is >> a stab at rebasing and adding test cases for the low-hanging fruits: >> >> - bitmap mirror mode with always/on-success/never bitmap sync mode >> - incremental mirror mode as sugar for bitmap + on-success >> >> Fabian Grünbichler (4): >> mirror: add check for bitmap-mode without bitmap >> mirror: switch to bdrv_dirty_bitmap_merge_internal >> iotests: add test for bitmap mirror >> mirror: move some checks to QMP >> >> John Snow (2): >> drive-mirror: add support for sync=bitmap mode=never >> drive-mirror: add support for conditional and always bitmap sync modes >> >> include/block/block_int.h | 4 +- >> block/mirror.c | 96 +- >> blockdev.c | 71 +- >> tests/test-block-iothread.c | 4 +- >> qapi/block-core.json | 29 +- >> tests/qemu-iotests/284 | 547 +++++++ >> tests/qemu-iotests/284.out | 2846 +++++++++++++++++++++++++++++++++++ >> tests/qemu-iotests/group | 1 + >> 8 files changed, 3567 insertions(+), 31 deletions(-) >> create mode 100755 tests/qemu-iotests/284 >> create mode 100644 tests/qemu-iotests/284.out >> > > Hi Fabian! Thanks for picking this up. I'm a bit behind on my mail, but > this on my list to look at. > > (Hint to other maintainers: It might be a while.) > > --js Hi, ping? ;) (it would be great to get some feedback as we'd like to ship this backported to 4.2 or 5.0 with our next point release) kind regards, Fabian
On 18.02.20 11:07, Fabian Grünbichler wrote: [Sorry :/] > picking up on John's in-progress patch series from last summer, this is > a stab at rebasing and adding test cases for the low-hanging fruits: > > - bitmap mirror mode with always/on-success/never bitmap sync mode > - incremental mirror mode as sugar for bitmap + on-success > > Fabian Grünbichler (4): > mirror: add check for bitmap-mode without bitmap > mirror: switch to bdrv_dirty_bitmap_merge_internal > iotests: add test for bitmap mirror > mirror: move some checks to QMP > > John Snow (2): > drive-mirror: add support for sync=bitmap mode=never > drive-mirror: add support for conditional and always bitmap sync modes Looks reasonable to me. I would indeed merge patches 2 through 4 into a single one, and perhaps switch patches 5 and 6. Also, we still need an S-o-b from John on patch 2. I have one question: When the mirror job completes successfully (or is cancelled “successfully”), the bitmap is always fully cleared when the job completes, right? (Unless in “never” mode.) Not that I think we should change the current implementation of “clear sync_bitmap; merge dirty_bitmap into sync_bitmap;”. Just a question for understanding. Soo... What’s the plan? Max
On 8/21/20 9:03 AM, Max Reitz wrote: > On 18.02.20 11:07, Fabian Grünbichler wrote: > > [Sorry :/] > >> picking up on John's in-progress patch series from last summer, this is >> a stab at rebasing and adding test cases for the low-hanging fruits: >> >> - bitmap mirror mode with always/on-success/never bitmap sync mode >> - incremental mirror mode as sugar for bitmap + on-success >> >> Fabian Grünbichler (4): >> mirror: add check for bitmap-mode without bitmap >> mirror: switch to bdrv_dirty_bitmap_merge_internal >> iotests: add test for bitmap mirror >> mirror: move some checks to QMP >> >> John Snow (2): >> drive-mirror: add support for sync=bitmap mode=never >> drive-mirror: add support for conditional and always bitmap sync modes > > Looks reasonable to me. I would indeed merge patches 2 through 4 into a > single one, and perhaps switch patches 5 and 6. > > Also, we still need an S-o-b from John on patch 2. > Whoops! Will do. > I have one question: When the mirror job completes successfully (or is > cancelled “successfully”), the bitmap is always fully cleared when the > job completes, right? (Unless in “never” mode.) > I don't remember personally, it's been a minute ... > Not that I think we should change the current implementation of “clear > sync_bitmap; merge dirty_bitmap into sync_bitmap;”. Just a question for > understanding. > > > Soo... What’s the plan? > > Max >
On August 21, 2020 3:03 pm, Max Reitz wrote: > On 18.02.20 11:07, Fabian Grünbichler wrote: > > [Sorry :/] same, I've been meaning to ping/pick this back up but other stuff got in the way. so thanks for the reminder to get this upstream ;) > >> picking up on John's in-progress patch series from last summer, this is >> a stab at rebasing and adding test cases for the low-hanging fruits: >> >> - bitmap mirror mode with always/on-success/never bitmap sync mode >> - incremental mirror mode as sugar for bitmap + on-success >> >> Fabian Grünbichler (4): >> mirror: add check for bitmap-mode without bitmap >> mirror: switch to bdrv_dirty_bitmap_merge_internal >> iotests: add test for bitmap mirror >> mirror: move some checks to QMP >> >> John Snow (2): >> drive-mirror: add support for sync=bitmap mode=never >> drive-mirror: add support for conditional and always bitmap sync modes > > Looks reasonable to me. I would indeed merge patches 2 through 4 into a > single one, and perhaps switch patches 5 and 6. > > Also, we still need an S-o-b from John on patch 2. > > I have one question: When the mirror job completes successfully (or is > cancelled “successfully”), the bitmap is always fully cleared when the > job completes, right? (Unless in “never” mode.) I have to take a closer look as well, it's been a while ;) IIRC the idea was that failed mirrors would allow re-using the bitmap for a next attempt, unless the mode is always. we are not using that feature (yet) though (see below). > Not that I think we should change the current implementation of “clear > sync_bitmap; merge dirty_bitmap into sync_bitmap;”. Just a question for > understanding. > > > Soo... What’s the plan? I'll rebase, squash as suggested and resend next week! I am not sure how the S-O-B by John is supposed to enter the mix - should I just include it in the squashed patch (which would be partly authored, but not-yet-signed-off by him otherwise?)? do you pick it up once he's replied with one? FWIW, with been running with this for quite a while downstream with no issues, but we are only using the following part: - create bitmap(s) - (incrementally) replicate storage volume(s) out of band (using ZFS) - incrementally drive mirror as part of a live migration of VM - drop bitmap(s) so no fancy semi-permanent bitmap that gets re-used here. I've been toying with implementing some sort of generic replication feature akin to zfs send/recv though, but given that we only have built-in persistent bitmaps with qcow2 and the chance of some other tool or the user messing up other image formats is high, the safe usage scenarios are a bit limited. we do use such long-running bitmaps for our new backup driver though, and it works quite well there!
On 03.09.20 12:13, Fabian Grünbichler wrote: > On August 21, 2020 3:03 pm, Max Reitz wrote: >> On 18.02.20 11:07, Fabian Grünbichler wrote: >> >> [Sorry :/] > > same, I've been meaning to ping/pick this back up but other stuff got in > the way. so thanks for the reminder to get this upstream ;) > >> >>> picking up on John's in-progress patch series from last summer, this is >>> a stab at rebasing and adding test cases for the low-hanging fruits: >>> >>> - bitmap mirror mode with always/on-success/never bitmap sync mode >>> - incremental mirror mode as sugar for bitmap + on-success >>> >>> Fabian Grünbichler (4): >>> mirror: add check for bitmap-mode without bitmap >>> mirror: switch to bdrv_dirty_bitmap_merge_internal >>> iotests: add test for bitmap mirror >>> mirror: move some checks to QMP >>> >>> John Snow (2): >>> drive-mirror: add support for sync=bitmap mode=never >>> drive-mirror: add support for conditional and always bitmap sync modes >> >> Looks reasonable to me. I would indeed merge patches 2 through 4 into a >> single one, and perhaps switch patches 5 and 6. >> >> Also, we still need an S-o-b from John on patch 2. >> >> I have one question: When the mirror job completes successfully (or is >> cancelled “successfully”), the bitmap is always fully cleared when the >> job completes, right? (Unless in “never” mode.) > > I have to take a closer look as well, it's been a while ;) No problem, I’m... *cough* not exactly in a hurry. > IIRC the idea > was that failed mirrors would allow re-using the bitmap for a next > attempt, unless the mode is always. we are not using that feature (yet) > though (see below). > >> Not that I think we should change the current implementation of “clear >> sync_bitmap; merge dirty_bitmap into sync_bitmap;”. Just a question for >> understanding. >> >> >> Soo... What’s the plan? > > I'll rebase, squash as suggested and resend next week! OK :) > I am not sure how > the S-O-B by John is supposed to enter the mix - should I just include > it in the squashed patch (which would be partly authored, but > not-yet-signed-off by him otherwise?)? I’m not too sure on the proceedings, actually. I think it should be fine if you put his S-o-b there, as long as your patch is somehow based on a patch that he sent earlier with his S-o-b underneath. But I’m not sure. > do you pick it up once he's replied with one? Yes, that’s what would be best. > FWIW, with been running with this for quite a while downstream with no > issues, but we are only using the following part: > > - create bitmap(s) > - (incrementally) replicate storage volume(s) out of band (using ZFS) > - incrementally drive mirror as part of a live migration of VM > - drop bitmap(s) > > so no fancy semi-permanent bitmap that gets re-used here. I've been > toying with implementing some sort of generic replication feature akin > to zfs send/recv though, but given that we only have built-in persistent > bitmaps with qcow2 and the chance of some other tool or the user messing > up other image formats is high, the safe usage scenarios are a bit > limited. OK. > we do use such long-running bitmaps for our new backup driver though, > and it works quite well there! Good! :) Max
Am 03.09.2020 um 13:04 hat Max Reitz geschrieben: > On 03.09.20 12:13, Fabian Grünbichler wrote: > > On August 21, 2020 3:03 pm, Max Reitz wrote: > >> On 18.02.20 11:07, Fabian Grünbichler wrote: > > I am not sure how > > the S-O-B by John is supposed to enter the mix - should I just include > > it in the squashed patch (which would be partly authored, but > > not-yet-signed-off by him otherwise?)? > > I’m not too sure on the proceedings, actually. I think it should be > fine if you put his S-o-b there, as long as your patch is somehow based > on a patch that he sent earlier with his S-o-b underneath. But I’m not > sure. Signed-off-by means that John certifies the DCO for the patch (at least the original version that you possibly modified), so you cannot just add it without asking him. John should reply with a Signed-off-by line to the patch in question. Then you (Fabian) can add it in the next version of the series (if I understand correctly, you're going to respin anyway). I see that patch 2 doesn't have any S-o-b at all. It should have both John's and Fabian's. Kevin
On 03.09.20 14:38, Kevin Wolf wrote: > Am 03.09.2020 um 13:04 hat Max Reitz geschrieben: >> On 03.09.20 12:13, Fabian Grünbichler wrote: >>> On August 21, 2020 3:03 pm, Max Reitz wrote: >>>> On 18.02.20 11:07, Fabian Grünbichler wrote: >>> I am not sure how >>> the S-O-B by John is supposed to enter the mix - should I just include >>> it in the squashed patch (which would be partly authored, but >>> not-yet-signed-off by him otherwise?)? >> >> I’m not too sure on the proceedings, actually. I think it should be >> fine if you put his S-o-b there, as long as your patch is somehow based >> on a patch that he sent earlier with his S-o-b underneath. But I’m not >> sure. > > Signed-off-by means that John certifies the DCO for the patch (at least > the original version that you possibly modified), so you cannot just add > it without asking him. But what if you take a patch from someone and heavily modify it – wouldn’t you keep the original S-o-b and explain the modifications in the commit message? Max > John should reply with a Signed-off-by line to the patch in question. > Then you (Fabian) can add it in the next version of the series (if I > understand correctly, you're going to respin anyway). > > I see that patch 2 doesn't have any S-o-b at all. It should have both > John's and Fabian's. > > Kevin >
Am 03.09.2020 um 14:57 hat Max Reitz geschrieben: > On 03.09.20 14:38, Kevin Wolf wrote: > > Am 03.09.2020 um 13:04 hat Max Reitz geschrieben: > >> On 03.09.20 12:13, Fabian Grünbichler wrote: > >>> On August 21, 2020 3:03 pm, Max Reitz wrote: > >>>> On 18.02.20 11:07, Fabian Grünbichler wrote: > >>> I am not sure how > >>> the S-O-B by John is supposed to enter the mix - should I just include > >>> it in the squashed patch (which would be partly authored, but > >>> not-yet-signed-off by him otherwise?)? > >> > >> I’m not too sure on the proceedings, actually. I think it should be > >> fine if you put his S-o-b there, as long as your patch is somehow based > >> on a patch that he sent earlier with his S-o-b underneath. But I’m not > >> sure. > > > > Signed-off-by means that John certifies the DCO for the patch (at least > > the original version that you possibly modified), so you cannot just add > > it without asking him. > > But what if you take a patch from someone and heavily modify it – > wouldn’t you keep the original S-o-b and explain the modifications in > the commit message? Ah, if that patch already had a S-o-b, then yes. You keep it not only to show who touched the patch, but also because your own S-o-b depends on the one from the original author (you only have the rights to contribute it because the original author had them and could pass them on to you). I thought it was based on a patch that came without S-o-b. Kevin
On September 3, 2020 3:23 pm, Kevin Wolf wrote: > Am 03.09.2020 um 14:57 hat Max Reitz geschrieben: >> On 03.09.20 14:38, Kevin Wolf wrote: >> > Am 03.09.2020 um 13:04 hat Max Reitz geschrieben: >> >> On 03.09.20 12:13, Fabian Grünbichler wrote: >> >>> On August 21, 2020 3:03 pm, Max Reitz wrote: >> >>>> On 18.02.20 11:07, Fabian Grünbichler wrote: >> >>> I am not sure how >> >>> the S-O-B by John is supposed to enter the mix - should I just include >> >>> it in the squashed patch (which would be partly authored, but >> >>> not-yet-signed-off by him otherwise?)? >> >> >> >> I’m not too sure on the proceedings, actually. I think it should be >> >> fine if you put his S-o-b there, as long as your patch is somehow based >> >> on a patch that he sent earlier with his S-o-b underneath. But I’m not >> >> sure. >> > >> > Signed-off-by means that John certifies the DCO for the patch (at least >> > the original version that you possibly modified), so you cannot just add >> > it without asking him. >> >> But what if you take a patch from someone and heavily modify it – >> wouldn’t you keep the original S-o-b and explain the modifications in >> the commit message? > > Ah, if that patch already had a S-o-b, then yes. You keep it not only to > show who touched the patch, but also because your own S-o-b depends on > the one from the original author (you only have the rights to contribute > it because the original author had them and could pass them on to you). > > I thought it was based on a patch that came without S-o-b. it is (taken from John's git, with his approval, and implicit admission that S-O-B is just missing because it was a WIP branch/tree that I started from). that was also the reason why I kept that patch unmodified and sent my modifications as patches on-top, to make it easier for John to verify that that one patch is his original WIP one and add this missing S-O-B ;)
Am 03.09.2020 um 15:36 hat Fabian Grünbichler geschrieben: > On September 3, 2020 3:23 pm, Kevin Wolf wrote: > > Am 03.09.2020 um 14:57 hat Max Reitz geschrieben: > >> On 03.09.20 14:38, Kevin Wolf wrote: > >> > Am 03.09.2020 um 13:04 hat Max Reitz geschrieben: > >> >> On 03.09.20 12:13, Fabian Grünbichler wrote: > >> >>> On August 21, 2020 3:03 pm, Max Reitz wrote: > >> >>>> On 18.02.20 11:07, Fabian Grünbichler wrote: > >> >>> I am not sure how > >> >>> the S-O-B by John is supposed to enter the mix - should I just include > >> >>> it in the squashed patch (which would be partly authored, but > >> >>> not-yet-signed-off by him otherwise?)? > >> >> > >> >> I’m not too sure on the proceedings, actually. I think it should be > >> >> fine if you put his S-o-b there, as long as your patch is somehow based > >> >> on a patch that he sent earlier with his S-o-b underneath. But I’m not > >> >> sure. > >> > > >> > Signed-off-by means that John certifies the DCO for the patch (at least > >> > the original version that you possibly modified), so you cannot just add > >> > it without asking him. > >> > >> But what if you take a patch from someone and heavily modify it – > >> wouldn’t you keep the original S-o-b and explain the modifications in > >> the commit message? > > > > Ah, if that patch already had a S-o-b, then yes. You keep it not only to > > show who touched the patch, but also because your own S-o-b depends on > > the one from the original author (you only have the rights to contribute > > it because the original author had them and could pass them on to you). > > > > I thought it was based on a patch that came without S-o-b. > > it is (taken from John's git, with his approval, and implicit admission > that S-O-B is just missing because it was a WIP branch/tree that I > started from). that was also the reason why I kept that patch unmodified > and sent my modifications as patches on-top, to make it easier for John > to verify that that one patch is his original WIP one and add this > missing S-O-B ;) Yeah, then John should just reply to the patch and add the S-o-b. Complications like this are why I always use 'git commit -s' and have it already in my development branches rather than only adding it when preparing the patch emails to send. Kevin
On 03.09.20 15:36, Fabian Grünbichler wrote: > On September 3, 2020 3:23 pm, Kevin Wolf wrote: >> Am 03.09.2020 um 14:57 hat Max Reitz geschrieben: >>> On 03.09.20 14:38, Kevin Wolf wrote: >>>> Am 03.09.2020 um 13:04 hat Max Reitz geschrieben: >>>>> On 03.09.20 12:13, Fabian Grünbichler wrote: >>>>>> On August 21, 2020 3:03 pm, Max Reitz wrote: >>>>>>> On 18.02.20 11:07, Fabian Grünbichler wrote: >>>>>> I am not sure how >>>>>> the S-O-B by John is supposed to enter the mix - should I just include >>>>>> it in the squashed patch (which would be partly authored, but >>>>>> not-yet-signed-off by him otherwise?)? >>>>> >>>>> I’m not too sure on the proceedings, actually. I think it should be >>>>> fine if you put his S-o-b there, as long as your patch is somehow based >>>>> on a patch that he sent earlier with his S-o-b underneath. But I’m not >>>>> sure. >>>> >>>> Signed-off-by means that John certifies the DCO for the patch (at least >>>> the original version that you possibly modified), so you cannot just add >>>> it without asking him. >>> >>> But what if you take a patch from someone and heavily modify it – >>> wouldn’t you keep the original S-o-b and explain the modifications in >>> the commit message? >> >> Ah, if that patch already had a S-o-b, then yes. You keep it not only to >> show who touched the patch, but also because your own S-o-b depends on >> the one from the original author (you only have the rights to contribute >> it because the original author had them and could pass them on to you). >> >> I thought it was based on a patch that came without S-o-b. > > it is (taken from John's git, with his approval, and implicit admission > that S-O-B is just missing because it was a WIP branch/tree that I > started from). that was also the reason why I kept that patch unmodified > and sent my modifications as patches on-top, to make it easier for John > to verify that that one patch is his original WIP one and add this > missing S-O-B ;) OK, I see now. I thought the S-o-b got lost somewhere, but was present originally. Max