| Message ID | 20190920082543.23444-1-vsementsov@virtuozzo.com |
|---|---|
| Headers | show |
| Series | proper locking on bitmap add/remove paths | expand |
On 9/20/19 4:25 AM, Vladimir Sementsov-Ogievskiy wrote: > Hi all! > > We need to lock qcow2 mutex on accessing in-image metadata, especially > on updating this metadata. Let's implement it. > > v3: > 01: add John's r-b > 02: - fix bdrv_remove_persistent_dirty_bitmap return value > - drop extra zeroing of ret in qcow2_remove_persistent_dirty_bitmap > 03: add John's r-b > > Vladimir Sementsov-Ogievskiy (3): > block: move bdrv_can_store_new_dirty_bitmap to block/dirty-bitmap.c > block/dirty-bitmap: return int from > bdrv_remove_persistent_dirty_bitmap > block/qcow2: proper locking on bitmap add/remove paths > > block/qcow2.h | 14 ++--- > include/block/block_int.h | 14 ++--- > include/block/dirty-bitmap.h | 5 +- > block.c | 22 ------- > block/dirty-bitmap.c | 119 +++++++++++++++++++++++++++++++++-- > block/qcow2-bitmap.c | 36 +++++++---- > block/qcow2.c | 5 +- > blockdev.c | 28 +++------ > 8 files changed, 163 insertions(+), 80 deletions(-) > I'll take this; I imagine the return signatures are going to change again with your error propagation series, though ...?
On 9/20/19 4:25 AM, Vladimir Sementsov-Ogievskiy wrote: > Hi all! > > We need to lock qcow2 mutex on accessing in-image metadata, especially > on updating this metadata. Let's implement it. > > v3: > 01: add John's r-b > 02: - fix bdrv_remove_persistent_dirty_bitmap return value > - drop extra zeroing of ret in qcow2_remove_persistent_dirty_bitmap > 03: add John's r-b > > Vladimir Sementsov-Ogievskiy (3): > block: move bdrv_can_store_new_dirty_bitmap to block/dirty-bitmap.c > block/dirty-bitmap: return int from > bdrv_remove_persistent_dirty_bitmap > block/qcow2: proper locking on bitmap add/remove paths > > block/qcow2.h | 14 ++--- > include/block/block_int.h | 14 ++--- > include/block/dirty-bitmap.h | 5 +- > block.c | 22 ------- > block/dirty-bitmap.c | 119 +++++++++++++++++++++++++++++++++-- > block/qcow2-bitmap.c | 36 +++++++---- > block/qcow2.c | 5 +- > blockdev.c | 28 +++------ > 8 files changed, 163 insertions(+), 80 deletions(-) > Thanks, applied to my bitmaps tree: https://github.com/jnsnow/qemu/commits/bitmaps https://github.com/jnsnow/qemu.git --js
26.09.2019 22:01, John Snow wrote: > > > On 9/20/19 4:25 AM, Vladimir Sementsov-Ogievskiy wrote: >> Hi all! >> >> We need to lock qcow2 mutex on accessing in-image metadata, especially >> on updating this metadata. Let's implement it. >> >> v3: >> 01: add John's r-b >> 02: - fix bdrv_remove_persistent_dirty_bitmap return value >> - drop extra zeroing of ret in qcow2_remove_persistent_dirty_bitmap >> 03: add John's r-b >> >> Vladimir Sementsov-Ogievskiy (3): >> block: move bdrv_can_store_new_dirty_bitmap to block/dirty-bitmap.c >> block/dirty-bitmap: return int from >> bdrv_remove_persistent_dirty_bitmap >> block/qcow2: proper locking on bitmap add/remove paths >> >> block/qcow2.h | 14 ++--- >> include/block/block_int.h | 14 ++--- >> include/block/dirty-bitmap.h | 5 +- >> block.c | 22 ------- >> block/dirty-bitmap.c | 119 +++++++++++++++++++++++++++++++++-- >> block/qcow2-bitmap.c | 36 +++++++---- >> block/qcow2.c | 5 +- >> blockdev.c | 28 +++------ >> 8 files changed, 163 insertions(+), 80 deletions(-) >> > > I'll take this; I imagine the return signatures are going to change > again with your error propagation series, though ...? > Thanks a lot! Hmm, I don't think so, as I used to think that returning int for errp-functions is better anyway.. ret = f(..., errp); if (ret < 0) { } vs f(..., errp); if (*errp) { } Hmmm... The latter just looks unfamiliar in comparison with "if (ret < 0)".. But if we anyway going to convert a lot of "if (*local_err)" to "if (*errp)", it will become familiar.. And the latter may save 6 characters in a line with function call, which may save the whole line in some places. So I don't know. returning two errors is not very good, we don't have convention for it actually. if I have int ret = f(..., errp), what should I report? error_report_err_errno(ret, errp), or just error_report_err(errp), assuming errp contains the whole information? Still, sometimes we need to distinguish one error code from another, and we can't check errp for such thing..
On 9/27/19 4:37 AM, Vladimir Sementsov-Ogievskiy wrote: > 26.09.2019 22:01, John Snow wrote: >> >> >> On 9/20/19 4:25 AM, Vladimir Sementsov-Ogievskiy wrote: >>> Hi all! >>> >>> We need to lock qcow2 mutex on accessing in-image metadata, especially >>> on updating this metadata. Let's implement it. >>> >>> v3: >>> 01: add John's r-b >>> 02: - fix bdrv_remove_persistent_dirty_bitmap return value >>> - drop extra zeroing of ret in qcow2_remove_persistent_dirty_bitmap >>> 03: add John's r-b >>> >>> Vladimir Sementsov-Ogievskiy (3): >>> block: move bdrv_can_store_new_dirty_bitmap to block/dirty-bitmap.c >>> block/dirty-bitmap: return int from >>> bdrv_remove_persistent_dirty_bitmap >>> block/qcow2: proper locking on bitmap add/remove paths >>> >>> block/qcow2.h | 14 ++--- >>> include/block/block_int.h | 14 ++--- >>> include/block/dirty-bitmap.h | 5 +- >>> block.c | 22 ------- >>> block/dirty-bitmap.c | 119 +++++++++++++++++++++++++++++++++-- >>> block/qcow2-bitmap.c | 36 +++++++---- >>> block/qcow2.c | 5 +- >>> blockdev.c | 28 +++------ >>> 8 files changed, 163 insertions(+), 80 deletions(-) >>> >> >> I'll take this; I imagine the return signatures are going to change >> again with your error propagation series, though ...? >> > > Thanks a lot! > > Hmm, I don't think so, as I used to think that returning int for errp-functions > is better anyway.. > OK, well, no problem. I'm not very picky about the error propagation paradigm; since you are investing your effort in it lately I'm just going to trust your judgment here. > ret = f(..., errp); > if (ret < 0) { > > } > > vs > > f(..., errp); > if (*errp) { > > } > > Hmmm... The latter just looks unfamiliar in comparison with "if (ret < 0)".. But > if we anyway going to convert a lot of "if (*local_err)" to "if (*errp)", it will > become familiar.. And the latter may save 6 characters in a line with function call, > which may save the whole line in some places. > > So I don't know. > > returning two errors is not very good, we don't have convention for it actually. > > if I have int ret = f(..., errp), what should I report? > > error_report_err_errno(ret, errp), or just error_report_err(errp), assuming errp > contains the whole information? > > Still, sometimes we need to distinguish one error code from another, and we can't > check errp for such thing.. > OK, I just wasn't sure the details of your series, actually -- I just wanted to know if we'd need to make changes, but if not, that's easier :) --js
Hi all! We need to lock qcow2 mutex on accessing in-image metadata, especially on updating this metadata. Let's implement it. v3: 01: add John's r-b 02: - fix bdrv_remove_persistent_dirty_bitmap return value - drop extra zeroing of ret in qcow2_remove_persistent_dirty_bitmap 03: add John's r-b Vladimir Sementsov-Ogievskiy (3): block: move bdrv_can_store_new_dirty_bitmap to block/dirty-bitmap.c block/dirty-bitmap: return int from bdrv_remove_persistent_dirty_bitmap block/qcow2: proper locking on bitmap add/remove paths block/qcow2.h | 14 ++--- include/block/block_int.h | 14 ++--- include/block/dirty-bitmap.h | 5 +- block.c | 22 ------- block/dirty-bitmap.c | 119 +++++++++++++++++++++++++++++++++-- block/qcow2-bitmap.c | 36 +++++++---- block/qcow2.c | 5 +- blockdev.c | 28 +++------ 8 files changed, 163 insertions(+), 80 deletions(-)