Re: [Qemu-devel] [RFC PATCH 0/5] qcow: coroutines cleanup
Am 22.07.2011 22:09, schrieb Frediano Ziglio: Il giorno ven, 22/07/2011 alle 12.10 +0200, Kevin Wolf ha scritto: Am 22.07.2011 11:26, schrieb Frediano Ziglio: 2011/7/22 Kevin Wolf kw...@redhat.com: Am 20.07.2011 15:56, schrieb Frediano Ziglio: These patches mostly cleanup some AIO code using coroutines. These patches apply to Kevin's repository, branch coroutine-block. Mostly they use stack instead of allocated AIO structure. Frediano Ziglio (5): qcow: allocate QCowAIOCB structure using stack qcow: QCowAIOCB field cleanup qcow: move some blocks of code to avoid useless variable initialization avoid dandling pointers qcow: small optimization initializing QCowAIOCB block/qcow.c | 210 + block/qcow2.c | 38 +++--- 2 files changed, 102 insertions(+), 146 deletions(-) Most of it looks good now. Did you include the RFC in the subject just because the coroutine work is in RFC state, too, or did you intend to tell me that I shouldn't merge yet? Kevin As these patches are first quite big patches I send (typo or small fixes do not counts) I just want to mark that I could write something really wrong. Just a way to avoid somebody having to send more patches and get more attention. Some projects are quite prone to merge even not that fine ones. I prefer to have some (a bit) pedantic comments and a real fix/improve. Now I removed the RFC from last update. The main reason is that I found your qemu-iotests repository which, I think should be merged to main repository, but it's just my opinion. Oh... qcow fails 004 test (even origin/coroutines-block) with a I/O error. Yup, you're right, I must have messed it up. Care to fix it or should I look into it? Care but I don't know if I'll have time before Thursday. However I found the problem, really strange. bdrv_read returns 0 for errors 0 for success and... bytes read on partial read! Now a qcow image of 128m is 560 bytes so when you read sector 1 you get 48 which is not a problem for qcow code. But if you replace bdrv_read with a bdrv_co_readv (your latest patch on coroutine-block) bdrv_co_readv return -EINVAL on partial read. Oh, that one. I think I have a fix for it somewhere, I'll include it in my series. Kevin
Re: [Qemu-devel] [RFC PATCH 0/5] qcow: coroutines cleanup
Am 20.07.2011 15:56, schrieb Frediano Ziglio: These patches mostly cleanup some AIO code using coroutines. These patches apply to Kevin's repository, branch coroutine-block. Mostly they use stack instead of allocated AIO structure. Frediano Ziglio (5): qcow: allocate QCowAIOCB structure using stack qcow: QCowAIOCB field cleanup qcow: move some blocks of code to avoid useless variable initialization avoid dandling pointers qcow: small optimization initializing QCowAIOCB block/qcow.c | 210 + block/qcow2.c | 38 +++--- 2 files changed, 102 insertions(+), 146 deletions(-) Most of it looks good now. Did you include the RFC in the subject just because the coroutine work is in RFC state, too, or did you intend to tell me that I shouldn't merge yet? Kevin
Re: [Qemu-devel] [RFC PATCH 0/5] qcow: coroutines cleanup
2011/7/22 Kevin Wolf kw...@redhat.com: Am 20.07.2011 15:56, schrieb Frediano Ziglio: These patches mostly cleanup some AIO code using coroutines. These patches apply to Kevin's repository, branch coroutine-block. Mostly they use stack instead of allocated AIO structure. Frediano Ziglio (5): qcow: allocate QCowAIOCB structure using stack qcow: QCowAIOCB field cleanup qcow: move some blocks of code to avoid useless variable initialization avoid dandling pointers qcow: small optimization initializing QCowAIOCB block/qcow.c | 210 + block/qcow2.c | 38 +++--- 2 files changed, 102 insertions(+), 146 deletions(-) Most of it looks good now. Did you include the RFC in the subject just because the coroutine work is in RFC state, too, or did you intend to tell me that I shouldn't merge yet? Kevin As these patches are first quite big patches I send (typo or small fixes do not counts) I just want to mark that I could write something really wrong. Just a way to avoid somebody having to send more patches and get more attention. Some projects are quite prone to merge even not that fine ones. I prefer to have some (a bit) pedantic comments and a real fix/improve. Now I removed the RFC from last update. The main reason is that I found your qemu-iotests repository which, I think should be merged to main repository, but it's just my opinion. Oh... qcow fails 004 test (even origin/coroutines-block) with a I/O error. I must say there are a lot of small hidden things that a developer should know about Qemu, for instance - mailing list follow some LKML rules as CC ML and send to maintainer to get more attention - you can use scripts/checkpatch.pl to check your patches before send I still have also to understand how to use git format-patch/send-email correctly and fluently :) Frediano
Re: [Qemu-devel] [RFC PATCH 0/5] qcow: coroutines cleanup
Am 22.07.2011 11:26, schrieb Frediano Ziglio: 2011/7/22 Kevin Wolf kw...@redhat.com: Am 20.07.2011 15:56, schrieb Frediano Ziglio: These patches mostly cleanup some AIO code using coroutines. These patches apply to Kevin's repository, branch coroutine-block. Mostly they use stack instead of allocated AIO structure. Frediano Ziglio (5): qcow: allocate QCowAIOCB structure using stack qcow: QCowAIOCB field cleanup qcow: move some blocks of code to avoid useless variable initialization avoid dandling pointers qcow: small optimization initializing QCowAIOCB block/qcow.c | 210 + block/qcow2.c | 38 +++--- 2 files changed, 102 insertions(+), 146 deletions(-) Most of it looks good now. Did you include the RFC in the subject just because the coroutine work is in RFC state, too, or did you intend to tell me that I shouldn't merge yet? Kevin As these patches are first quite big patches I send (typo or small fixes do not counts) I just want to mark that I could write something really wrong. Just a way to avoid somebody having to send more patches and get more attention. Some projects are quite prone to merge even not that fine ones. I prefer to have some (a bit) pedantic comments and a real fix/improve. Now I removed the RFC from last update. The main reason is that I found your qemu-iotests repository which, I think should be merged to main repository, but it's just my opinion. Oh... qcow fails 004 test (even origin/coroutines-block) with a I/O error. Yup, you're right, I must have messed it up. Care to fix it or should I look into it? I must say there are a lot of small hidden things that a developer should know about Qemu, for instance - mailing list follow some LKML rules as CC ML and send to maintainer to get more attention - you can use scripts/checkpatch.pl to check your patches before send I still have also to understand how to use git format-patch/send-email correctly and fluently :) That's true. Maybe you can update the wiki page with your findings? The one suggestion I have for your use of git format-patch is --subject-prefix=PATCH v3, so I don't get confused by the different versions ;-) Kevin
Re: [Qemu-devel] [RFC PATCH 0/5] qcow: coroutines cleanup
On Fri, Jul 22, 2011 at 11:10 AM, Kevin Wolf kw...@redhat.com wrote: Am 22.07.2011 11:26, schrieb Frediano Ziglio: - you can use scripts/checkpatch.pl to check your patches before send I updated the SubmitAPatch wiki earlier this week. Stefan
Re: [Qemu-devel] [RFC PATCH 0/5] qcow: coroutines cleanup
2011/7/22 Stefan Hajnoczi stefa...@gmail.com: On Fri, Jul 22, 2011 at 11:10 AM, Kevin Wolf kw...@redhat.com wrote: Am 22.07.2011 11:26, schrieb Frediano Ziglio: - you can use scripts/checkpatch.pl to check your patches before send I updated the SubmitAPatch wiki earlier this week. Stefan Good, now wiki is working (it seems somebody is attacking Qemu sites... yesterday the ML). http://git.qemu.org/git/qemu.git/ is not working so all links give 404. I added some notes, yes checkpatch was already in the page. About git commands to send multiple patches, I use git format-patch --cover-letter -s -M origin/original_branch_name --subject-prefix='PATCH vXX' -o outgoing/ edit manually cover letter and git send-email --to='maintainer@domain' --cc='qemu-devel@nongnu.org' outgoing/* are these command correct or there is a better way? Frediano
Re: [Qemu-devel] [RFC PATCH 0/5] qcow: coroutines cleanup
Am 22.07.2011 15:24, schrieb Frediano Ziglio: 2011/7/22 Stefan Hajnoczi stefa...@gmail.com: On Fri, Jul 22, 2011 at 11:10 AM, Kevin Wolf kw...@redhat.com wrote: Am 22.07.2011 11:26, schrieb Frediano Ziglio: - you can use scripts/checkpatch.pl to check your patches before send I updated the SubmitAPatch wiki earlier this week. Stefan Good, now wiki is working (it seems somebody is attacking Qemu sites... yesterday the ML). http://git.qemu.org/git/qemu.git/ is not working so all links give 404. I added some notes, yes checkpatch was already in the page. About git commands to send multiple patches, I use git format-patch --cover-letter -s -M origin/original_branch_name --subject-prefix='PATCH vXX' -o outgoing/ edit manually cover letter and git send-email --to='maintainer@domain' --cc='qemu-devel@nongnu.org' outgoing/* are these command correct or there is a better way? Looks correct to me. I usually use -s already for commits, so I don't need it in format-patch, and I don't use -M (maybe I should), but otherwise it's more or less the same as I use. Kevin
Re: [Qemu-devel] [RFC PATCH 0/5] qcow: coroutines cleanup
Hi, git send-email --to='maintainer@domain' --cc='qemu-devel@nongnu.org' outgoing/* It is a good idea to make the last argument outgoing/0*.patch, otherwise you risk to send out the -cover-letter.patch~ editor backup file ... Otherwise the procedure looks fine. cheers, Gerd
Re: [Qemu-devel] [RFC PATCH 0/5] qcow: coroutines cleanup
Il giorno ven, 22/07/2011 alle 12.10 +0200, Kevin Wolf ha scritto: Am 22.07.2011 11:26, schrieb Frediano Ziglio: 2011/7/22 Kevin Wolf kw...@redhat.com: Am 20.07.2011 15:56, schrieb Frediano Ziglio: These patches mostly cleanup some AIO code using coroutines. These patches apply to Kevin's repository, branch coroutine-block. Mostly they use stack instead of allocated AIO structure. Frediano Ziglio (5): qcow: allocate QCowAIOCB structure using stack qcow: QCowAIOCB field cleanup qcow: move some blocks of code to avoid useless variable initialization avoid dandling pointers qcow: small optimization initializing QCowAIOCB block/qcow.c | 210 + block/qcow2.c | 38 +++--- 2 files changed, 102 insertions(+), 146 deletions(-) Most of it looks good now. Did you include the RFC in the subject just because the coroutine work is in RFC state, too, or did you intend to tell me that I shouldn't merge yet? Kevin As these patches are first quite big patches I send (typo or small fixes do not counts) I just want to mark that I could write something really wrong. Just a way to avoid somebody having to send more patches and get more attention. Some projects are quite prone to merge even not that fine ones. I prefer to have some (a bit) pedantic comments and a real fix/improve. Now I removed the RFC from last update. The main reason is that I found your qemu-iotests repository which, I think should be merged to main repository, but it's just my opinion. Oh... qcow fails 004 test (even origin/coroutines-block) with a I/O error. Yup, you're right, I must have messed it up. Care to fix it or should I look into it? Care but I don't know if I'll have time before Thursday. However I found the problem, really strange. bdrv_read returns 0 for errors 0 for success and... bytes read on partial read! Now a qcow image of 128m is 560 bytes so when you read sector 1 you get 48 which is not a problem for qcow code. But if you replace bdrv_read with a bdrv_co_readv (your latest patch on coroutine-block) bdrv_co_readv return -EINVAL on partial read. Frediano
[Qemu-devel] [RFC PATCH 0/5] qcow: coroutines cleanup
These patches mostly cleanup some AIO code using coroutines. These patches apply to Kevin's repository, branch coroutine-block. Mostly they use stack instead of allocated AIO structure. Frediano Ziglio (5): qcow: allocate QCowAIOCB structure using stack qcow: QCowAIOCB field cleanup qcow: move some blocks of code to avoid useless variable initialization avoid dandling pointers qcow: small optimization initializing QCowAIOCB block/qcow.c | 210 + block/qcow2.c | 38 +++--- 2 files changed, 102 insertions(+), 146 deletions(-)
