* [Qemu-devel] [PATCH v2 0/2] implement .bdrv_co_is_allocated
@ 2013-04-22 6:59 Liu Yuan
2013-04-22 6:59 ` [Qemu-devel] [PATCH v2 1/2] sheepdog: use BDRV_SECTOR_SIZE Liu Yuan
2013-04-22 6:59 ` [Qemu-devel] [PATCH v2 2/2] sheepdog: implement .bdrv_co_is_allocated() Liu Yuan
0 siblings, 2 replies; 9+ messages in thread
From: Liu Yuan @ 2013-04-22 6:59 UTC (permalink / raw)
To: qemu-devel; +Cc: sheepdog
From: Liu Yuan <tailai.ly@taobao.com>
v2:
- correct 'end' calculation
- get the longest unallocated area
- replace SECTOR_SIZE with BDRV_SECTOR_SIZE
PATCH 1/2 is ia prepare patch
PATCH 2/2 implement .bdrv_co_is_allocated
Liu Yuan (2):
sheepdog: use BDRV_SECTOR_SIZE
sheepdog: implement .bdrv_co_is_allocated()
block/sheepdog.c | 46 ++++++++++++++++++++++++++++++++++++++++------
1 file changed, 40 insertions(+), 6 deletions(-)
--
1.7.9.5
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v2 1/2] sheepdog: use BDRV_SECTOR_SIZE
2013-04-22 6:59 [Qemu-devel] [PATCH v2 0/2] implement .bdrv_co_is_allocated Liu Yuan
@ 2013-04-22 6:59 ` Liu Yuan
2013-04-22 6:59 ` [Qemu-devel] [PATCH v2 2/2] sheepdog: implement .bdrv_co_is_allocated() Liu Yuan
1 sibling, 0 replies; 9+ messages in thread
From: Liu Yuan @ 2013-04-22 6:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, sheepdog, Stefan Hajnoczi, MORITA Kazutaka
From: Liu Yuan <tailai.ly@taobao.com>
Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Liu Yuan <tailai.ly@taobao.com>
---
block/sheepdog.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/block/sheepdog.c b/block/sheepdog.c
index c099117..0b700a3 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -89,7 +89,6 @@
#define SD_NR_VDIS (1U << 24)
#define SD_DATA_OBJ_SIZE (UINT64_C(1) << 22)
#define SD_MAX_VDI_SIZE (SD_DATA_OBJ_SIZE * MAX_DATA_OBJS)
-#define SECTOR_SIZE 512
#define SD_INODE_SIZE (sizeof(SheepdogInode))
#define CURRENT_VDI_ID 0
@@ -1220,7 +1219,7 @@ static int sd_open(BlockDriverState *bs, const char *filename,
s->min_dirty_data_idx = UINT32_MAX;
s->max_dirty_data_idx = 0;
- bs->total_sectors = s->inode.vdi_size / SECTOR_SIZE;
+ bs->total_sectors = s->inode.vdi_size / BDRV_SECTOR_SIZE;
pstrcpy(s->name, sizeof(s->name), vdi);
qemu_co_mutex_init(&s->lock);
g_free(buf);
@@ -1605,10 +1604,10 @@ static int coroutine_fn sd_co_rw_vector(void *p)
{
SheepdogAIOCB *acb = p;
int ret = 0;
- unsigned long len, done = 0, total = acb->nb_sectors * SECTOR_SIZE;
- unsigned long idx = acb->sector_num * SECTOR_SIZE / SD_DATA_OBJ_SIZE;
+ unsigned long len, done = 0, total = acb->nb_sectors * BDRV_SECTOR_SIZE;
+ unsigned long idx = acb->sector_num * BDRV_SECTOR_SIZE / SD_DATA_OBJ_SIZE;
uint64_t oid;
- uint64_t offset = (acb->sector_num * SECTOR_SIZE) % SD_DATA_OBJ_SIZE;
+ uint64_t offset = (acb->sector_num * BDRV_SECTOR_SIZE) % SD_DATA_OBJ_SIZE;
BDRVSheepdogState *s = acb->common.bs->opaque;
SheepdogInode *inode = &s->inode;
AIOReq *aio_req;
@@ -1727,7 +1726,7 @@ static coroutine_fn int sd_co_writev(BlockDriverState *bs, int64_t sector_num,
int ret;
if (bs->growable && sector_num + nb_sectors > bs->total_sectors) {
- ret = sd_truncate(bs, (sector_num + nb_sectors) * SECTOR_SIZE);
+ ret = sd_truncate(bs, (sector_num + nb_sectors) * BDRV_SECTOR_SIZE);
if (ret < 0) {
return ret;
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v2 2/2] sheepdog: implement .bdrv_co_is_allocated()
2013-04-22 6:59 [Qemu-devel] [PATCH v2 0/2] implement .bdrv_co_is_allocated Liu Yuan
2013-04-22 6:59 ` [Qemu-devel] [PATCH v2 1/2] sheepdog: use BDRV_SECTOR_SIZE Liu Yuan
@ 2013-04-22 6:59 ` Liu Yuan
2013-04-22 12:00 ` Stefan Hajnoczi
1 sibling, 1 reply; 9+ messages in thread
From: Liu Yuan @ 2013-04-22 6:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, sheepdog, Stefan Hajnoczi, MORITA Kazutaka
From: Liu Yuan <tailai.ly@taobao.com>
Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Liu Yuan <tailai.ly@taobao.com>
---
block/sheepdog.c | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 0b700a3..0dbf039 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2137,6 +2137,39 @@ static coroutine_fn int sd_co_discard(BlockDriverState *bs, int64_t sector_num,
return acb->ret;
}
+static coroutine_fn int
+sd_co_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
+ int *pnum)
+{
+ BDRVSheepdogState *s = bs->opaque;
+ SheepdogInode *inode = &s->inode;
+ unsigned long start = sector_num * BDRV_SECTOR_SIZE / SD_DATA_OBJ_SIZE, idx,
+ end = DIV_ROUND_UP((sector_num + nb_sectors) *
+ BDRV_SECTOR_SIZE, SD_DATA_OBJ_SIZE);
+ int ret = 1;
+
+ for (idx = start; idx <= end; idx++) {
+ if (inode->data_vdi_id[idx] == 0) {
+ break;
+ }
+ }
+ if (idx == start) {
+ /* Get te longest length of unallocated sectors */
+ ret = 0;
+ for (idx = start + 1; idx <= end; idx++) {
+ if (inode->data_vdi_id[idx] != 0) {
+ break;
+ }
+ }
+ }
+
+ *pnum = (idx - start) * SD_DATA_OBJ_SIZE / BDRV_SECTOR_SIZE;
+ if (*pnum > nb_sectors) {
+ *pnum = nb_sectors;
+ }
+ return ret;
+}
+
static QEMUOptionParameter sd_create_options[] = {
{
.name = BLOCK_OPT_SIZE,
@@ -2170,6 +2203,7 @@ static BlockDriver bdrv_sheepdog = {
.bdrv_co_writev = sd_co_writev,
.bdrv_co_flush_to_disk = sd_co_flush_to_disk,
.bdrv_co_discard = sd_co_discard,
+ .bdrv_co_is_allocated = sd_co_is_allocated,
.bdrv_snapshot_create = sd_snapshot_create,
.bdrv_snapshot_goto = sd_snapshot_goto,
@@ -2196,6 +2230,7 @@ static BlockDriver bdrv_sheepdog_tcp = {
.bdrv_co_writev = sd_co_writev,
.bdrv_co_flush_to_disk = sd_co_flush_to_disk,
.bdrv_co_discard = sd_co_discard,
+ .bdrv_co_is_allocated = sd_co_is_allocated,
.bdrv_snapshot_create = sd_snapshot_create,
.bdrv_snapshot_goto = sd_snapshot_goto,
@@ -2222,6 +2257,7 @@ static BlockDriver bdrv_sheepdog_unix = {
.bdrv_co_writev = sd_co_writev,
.bdrv_co_flush_to_disk = sd_co_flush_to_disk,
.bdrv_co_discard = sd_co_discard,
+ .bdrv_co_is_allocated = sd_co_is_allocated,
.bdrv_snapshot_create = sd_snapshot_create,
.bdrv_snapshot_goto = sd_snapshot_goto,
--
1.7.9.5
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] sheepdog: implement .bdrv_co_is_allocated()
2013-04-22 6:59 ` [Qemu-devel] [PATCH v2 2/2] sheepdog: implement .bdrv_co_is_allocated() Liu Yuan
@ 2013-04-22 12:00 ` Stefan Hajnoczi
2013-04-22 12:10 ` Liu Yuan
0 siblings, 1 reply; 9+ messages in thread
From: Stefan Hajnoczi @ 2013-04-22 12:00 UTC (permalink / raw)
To: Liu Yuan
Cc: Kevin Wolf, MORITA Kazutaka, sheepdog, qemu-devel, Stefan Hajnoczi
On Mon, Apr 22, 2013 at 02:59:10PM +0800, Liu Yuan wrote:
> +static coroutine_fn int
> +sd_co_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
> + int *pnum)
> +{
> + BDRVSheepdogState *s = bs->opaque;
> + SheepdogInode *inode = &s->inode;
> + unsigned long start = sector_num * BDRV_SECTOR_SIZE / SD_DATA_OBJ_SIZE, idx,
> + end = DIV_ROUND_UP((sector_num + nb_sectors) *
> + BDRV_SECTOR_SIZE, SD_DATA_OBJ_SIZE);
Please put the variable declarations on separate lines, it's very easy
to miss "idx".
> + int ret = 1;
> +
> + for (idx = start; idx <= end; idx++) {
Should this be idx < end? Otherwise you are checking one beyond the
last SD_DATA_OBJ_SIZE.
> + if (inode->data_vdi_id[idx] == 0) {
> + break;
> + }
> + }
> + if (idx == start) {
> + /* Get te longest length of unallocated sectors */
s/te/the/
> + ret = 0;
> + for (idx = start + 1; idx <= end; idx++) {
> + if (inode->data_vdi_id[idx] != 0) {
> + break;
> + }
> + }
> + }
Here is a more concise way of implementing these two loops:
int ret = !!inode->data_vdi_id[idx];
for (idx = start + 1; idx < end; idx++) {
if (!!inode->data_vdi_id[idx] != ret) {
break;
}
}
I like this better because it avoids code duplication, but it's a
question of style. Feel free to stick to your approach if you like.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] sheepdog: implement .bdrv_co_is_allocated()
2013-04-22 12:00 ` Stefan Hajnoczi
@ 2013-04-22 12:10 ` Liu Yuan
2013-04-22 15:03 ` Stefan Hajnoczi
0 siblings, 1 reply; 9+ messages in thread
From: Liu Yuan @ 2013-04-22 12:10 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Kevin Wolf, MORITA Kazutaka, sheepdog, qemu-devel, Stefan Hajnoczi
On 04/22/2013 08:00 PM, Stefan Hajnoczi wrote:
> On Mon, Apr 22, 2013 at 02:59:10PM +0800, Liu Yuan wrote:
>> +static coroutine_fn int
>> +sd_co_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
>> + int *pnum)
>> +{
>> + BDRVSheepdogState *s = bs->opaque;
>> + SheepdogInode *inode = &s->inode;
>> + unsigned long start = sector_num * BDRV_SECTOR_SIZE / SD_DATA_OBJ_SIZE, idx,
>> + end = DIV_ROUND_UP((sector_num + nb_sectors) *
>> + BDRV_SECTOR_SIZE, SD_DATA_OBJ_SIZE);
>
> Please put the variable declarations on separate lines, it's very easy
> to miss "idx".
Okay.
>
>> + int ret = 1;
>> +
>> + for (idx = start; idx <= end; idx++) {
>
> Should this be idx < end? Otherwise you are checking one beyond the
> last SD_DATA_OBJ_SIZE.
No. the end is index of last object, not index of last object + 1.
>
>> + if (inode->data_vdi_id[idx] == 0) {
>> + break;
>> + }
>> + }
>> + if (idx == start) {
>> + /* Get te longest length of unallocated sectors */
>
> s/te/the/
>
>> + ret = 0;
>> + for (idx = start + 1; idx <= end; idx++) {
>> + if (inode->data_vdi_id[idx] != 0) {
>> + break;
>> + }
>> + }
>> + }
>
> Here is a more concise way of implementing these two loops:
>
> int ret = !!inode->data_vdi_id[idx];
> for (idx = start + 1; idx < end; idx++) {
> if (!!inode->data_vdi_id[idx] != ret) {
> break;
> }
> }
>
> I like this better because it avoids code duplication, but it's a
> question of style. Feel free to stick to your approach if you like.
The trick of your code looks fantastic to me and I like your idea to
reduce the duplicated code as much as possible but the sacrifice of code
readability for the resulted code is somewhat too high, so I think not
worth of it and I'll stick to my stupid but more clear version in V3.
Thanks,
Yuan
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] sheepdog: implement .bdrv_co_is_allocated()
2013-04-22 12:10 ` Liu Yuan
@ 2013-04-22 15:03 ` Stefan Hajnoczi
2013-04-22 15:18 ` Liu Yuan
0 siblings, 1 reply; 9+ messages in thread
From: Stefan Hajnoczi @ 2013-04-22 15:03 UTC (permalink / raw)
To: Liu Yuan
Cc: Kevin Wolf, Stefan Hajnoczi, sheepdog, qemu-devel, MORITA Kazutaka
On Mon, Apr 22, 2013 at 08:10:58PM +0800, Liu Yuan wrote:
> On 04/22/2013 08:00 PM, Stefan Hajnoczi wrote:
> > On Mon, Apr 22, 2013 at 02:59:10PM +0800, Liu Yuan wrote:
> >> +static coroutine_fn int
> >> +sd_co_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
> >> + int *pnum)
> >> +{
> >> + BDRVSheepdogState *s = bs->opaque;
> >> + SheepdogInode *inode = &s->inode;
> >> + unsigned long start = sector_num * BDRV_SECTOR_SIZE / SD_DATA_OBJ_SIZE, idx,
> >> + end = DIV_ROUND_UP((sector_num + nb_sectors) *
> >> + BDRV_SECTOR_SIZE, SD_DATA_OBJ_SIZE);
> >
> > Please put the variable declarations on separate lines, it's very easy
> > to miss "idx".
>
> Okay.
>
> >
> >> + int ret = 1;
> >> +
> >> + for (idx = start; idx <= end; idx++) {
> >
> > Should this be idx < end? Otherwise you are checking one beyond the
> > last SD_DATA_OBJ_SIZE.
>
> No. the end is index of last object, not index of last object + 1.
Imagine sector_num = 0 and nb_sectors = SD_DATA_OBJ_SIZE / BDRV_SECTOR_SIZE.
start = 0
end = 1
You don't want object 1, only object 0.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] sheepdog: implement .bdrv_co_is_allocated()
2013-04-22 15:03 ` Stefan Hajnoczi
@ 2013-04-22 15:18 ` Liu Yuan
2013-04-22 20:46 ` Stefan Hajnoczi
0 siblings, 1 reply; 9+ messages in thread
From: Liu Yuan @ 2013-04-22 15:18 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Kevin Wolf, Stefan Hajnoczi, sheepdog, qemu-devel, MORITA Kazutaka
On 04/22/2013 11:03 PM, Stefan Hajnoczi wrote:
> Imagine sector_num = 0 and nb_sectors = SD_DATA_OBJ_SIZE / BDRV_SECTOR_SIZE.
>
> start = 0
> end = 1
>
> You don't want object 1, only object 0.
Hmm, math, ouch. So nb_sectors include sector_num? What I mean is
If [start, end] mean a range of sectors,so
1. nb_sectors = end - start + 1 (I assume this one)
2. nb_sectors = end - start (you meant this one?)
which one is correct for .co_is_allocated?
Thanks,
Yuan
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] sheepdog: implement .bdrv_co_is_allocated()
2013-04-22 15:18 ` Liu Yuan
@ 2013-04-22 20:46 ` Stefan Hajnoczi
2013-04-23 5:55 ` Liu Yuan
0 siblings, 1 reply; 9+ messages in thread
From: Stefan Hajnoczi @ 2013-04-22 20:46 UTC (permalink / raw)
To: Liu Yuan
Cc: Kevin Wolf, MORITA Kazutaka, sheepdog, qemu-devel, Stefan Hajnoczi
On Mon, Apr 22, 2013 at 5:18 PM, Liu Yuan <namei.unix@gmail.com> wrote:
> On 04/22/2013 11:03 PM, Stefan Hajnoczi wrote:
>> Imagine sector_num = 0 and nb_sectors = SD_DATA_OBJ_SIZE / BDRV_SECTOR_SIZE.
>>
>> start = 0
>> end = 1
>>
>> You don't want object 1, only object 0.
>
> Hmm, math, ouch. So nb_sectors include sector_num? What I mean is
>
> If [start, end] mean a range of sectors,so
>
> 1. nb_sectors = end - start + 1 (I assume this one)
> 2. nb_sectors = end - start (you meant this one?)
>
> which one is correct for .co_is_allocated?
The first sector is included in nb_sectors. Mathematically the range
is defined as [sector_num, sector_num + nb_sectors). Notice the
half-open interval, sector_num + nb_sectors is excluded. The last
included sector is sector_num + nb_sectors - 1.
You can look at qcow2's implementation, especially
count_contiguous_clusters() to double-check.
Stefan
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] sheepdog: implement .bdrv_co_is_allocated()
2013-04-22 20:46 ` Stefan Hajnoczi
@ 2013-04-23 5:55 ` Liu Yuan
0 siblings, 0 replies; 9+ messages in thread
From: Liu Yuan @ 2013-04-23 5:55 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Kevin Wolf, MORITA Kazutaka, sheepdog, qemu-devel, Stefan Hajnoczi
On 04/23/2013 04:46 AM, Stefan Hajnoczi wrote:
> The first sector is included in nb_sectors. Mathematically the range
> is defined as [sector_num, sector_num + nb_sectors). Notice the
> half-open interval, sector_num + nb_sectors is excluded. The last
> included sector is sector_num + nb_sectors - 1.
>
> You can look at qcow2's implementation, especially
> count_contiguous_clusters() to double-check.
Okay, thanks for you explanation. I'll update it in v4.
Yuan
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-04-23 5:55 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-04-22 6:59 [Qemu-devel] [PATCH v2 0/2] implement .bdrv_co_is_allocated Liu Yuan
2013-04-22 6:59 ` [Qemu-devel] [PATCH v2 1/2] sheepdog: use BDRV_SECTOR_SIZE Liu Yuan
2013-04-22 6:59 ` [Qemu-devel] [PATCH v2 2/2] sheepdog: implement .bdrv_co_is_allocated() Liu Yuan
2013-04-22 12:00 ` Stefan Hajnoczi
2013-04-22 12:10 ` Liu Yuan
2013-04-22 15:03 ` Stefan Hajnoczi
2013-04-22 15:18 ` Liu Yuan
2013-04-22 20:46 ` Stefan Hajnoczi
2013-04-23 5:55 ` Liu Yuan
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.