| Message ID | 33b97a2f2e4860e69e658143d1e63927fa12a92e.1388381026.git.hutao@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On 30.12.2013 06:29, Hu Tao wrote: > In case of do preallocating metadata with a large cluster size, > qcow2_alloc_cluster_offset() can allocate nothing and returns > a NULL l2meta. This patch checks for it and link2 l2 with only > valid l2meta. > > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> > --- > block/qcow2.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/block/qcow2.c b/block/qcow2.c > index 46860d5..380c240 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -1399,18 +1399,20 @@ static int preallocate(BlockDriverState *bs) > offset = 0; > > while (nb_sectors) { > - num = MIN(nb_sectors, INT_MAX >> 9); > + num = MIN(nb_sectors, INT_MAX >> BDRV_SECTOR_BITS); Well, if you're already adjusting this here, you could also replace the other occurrences of 9 and 512 in this function. ;-) > ret = qcow2_alloc_cluster_offset(bs, offset, &num, > &host_offset, &meta); > if (ret < 0) { > return ret; > } > > - ret = qcow2_alloc_cluster_link_l2(bs, meta); > - if (ret < 0) { > - qcow2_free_any_clusters(bs, meta->alloc_offset, meta->nb_clusters, > - QCOW2_DISCARD_NEVER); > - return ret; > + if (meta) { > + ret = qcow2_alloc_cluster_link_l2(bs, meta); > + if (ret < 0) { > + qcow2_free_any_clusters(bs, meta->alloc_offset, > + meta->nb_clusters, QCOW2_DISCARD_NEVER); > + return ret; > + } > } > > /* There are no dependent requests, but we need to remove our request But this doesn't make this patch wrong, so: Reviewed-by: Max Reitz <mreitz@redhat.com>
On Sun, Jan 19, 2014 at 05:18:05PM +0100, Max Reitz wrote: > On 30.12.2013 06:29, Hu Tao wrote: > >In case of do preallocating metadata with a large cluster size, > >qcow2_alloc_cluster_offset() can allocate nothing and returns > >a NULL l2meta. This patch checks for it and link2 l2 with only > >valid l2meta. > > > >Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> > >--- > > block/qcow2.c | 14 ++++++++------ > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > >diff --git a/block/qcow2.c b/block/qcow2.c > >index 46860d5..380c240 100644 > >--- a/block/qcow2.c > >+++ b/block/qcow2.c > >@@ -1399,18 +1399,20 @@ static int preallocate(BlockDriverState *bs) > > offset = 0; > > while (nb_sectors) { > >- num = MIN(nb_sectors, INT_MAX >> 9); > >+ num = MIN(nb_sectors, INT_MAX >> BDRV_SECTOR_BITS); > > Well, if you're already adjusting this here, you could also replace > the other occurrences of 9 and 512 in this function. ;-) > > > ret = qcow2_alloc_cluster_offset(bs, offset, &num, > > &host_offset, &meta); > > if (ret < 0) { > > return ret; > > } > >- ret = qcow2_alloc_cluster_link_l2(bs, meta); > >- if (ret < 0) { > >- qcow2_free_any_clusters(bs, meta->alloc_offset, meta->nb_clusters, > >- QCOW2_DISCARD_NEVER); > >- return ret; > >+ if (meta) { > >+ ret = qcow2_alloc_cluster_link_l2(bs, meta); > >+ if (ret < 0) { > >+ qcow2_free_any_clusters(bs, meta->alloc_offset, > >+ meta->nb_clusters, QCOW2_DISCARD_NEVER); > >+ return ret; > >+ } > > } > > /* There are no dependent requests, but we need to remove our request > > But this doesn't make this patch wrong, so: > > Reviewed-by: Max Reitz <mreitz@redhat.com> Max, Thanks for all of you comments!
Am 20.01.2014 um 04:04 hat Hu Tao geschrieben: > On Sun, Jan 19, 2014 at 05:18:05PM +0100, Max Reitz wrote: > > On 30.12.2013 06:29, Hu Tao wrote: > > >In case of do preallocating metadata with a large cluster size, > > >qcow2_alloc_cluster_offset() can allocate nothing and returns > > >a NULL l2meta. This patch checks for it and link2 l2 with only > > >valid l2meta. > > > > > >Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> > > >--- > > > block/qcow2.c | 14 ++++++++------ > > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > > >diff --git a/block/qcow2.c b/block/qcow2.c > > >index 46860d5..380c240 100644 > > >--- a/block/qcow2.c > > >+++ b/block/qcow2.c > > >@@ -1399,18 +1399,20 @@ static int preallocate(BlockDriverState *bs) > > > offset = 0; > > > while (nb_sectors) { > > >- num = MIN(nb_sectors, INT_MAX >> 9); > > >+ num = MIN(nb_sectors, INT_MAX >> BDRV_SECTOR_BITS); > > > > Well, if you're already adjusting this here, you could also replace > > the other occurrences of 9 and 512 in this function. ;-) > > > > > ret = qcow2_alloc_cluster_offset(bs, offset, &num, > > > &host_offset, &meta); > > > if (ret < 0) { > > > return ret; > > > } > > >- ret = qcow2_alloc_cluster_link_l2(bs, meta); > > >- if (ret < 0) { > > >- qcow2_free_any_clusters(bs, meta->alloc_offset, meta->nb_clusters, > > >- QCOW2_DISCARD_NEVER); > > >- return ret; > > >+ if (meta) { > > >+ ret = qcow2_alloc_cluster_link_l2(bs, meta); > > >+ if (ret < 0) { > > >+ qcow2_free_any_clusters(bs, meta->alloc_offset, > > >+ meta->nb_clusters, QCOW2_DISCARD_NEVER); > > >+ return ret; > > >+ } > > > } > > > /* There are no dependent requests, but we need to remove our request > > > > But this doesn't make this patch wrong, so: > > > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > Max, > > Thanks for all of you comments! The series looks good in general, but I think the comments are worth addressing before we merge it. I would also love to see an qemu-iotests case that tests the cases that would previously crash. Once you post a new version that addresses these points, I'll merge it. Kevin
On Mon, Jan 20, 2014 at 04:17:16PM +0100, Kevin Wolf wrote: > Am 20.01.2014 um 04:04 hat Hu Tao geschrieben: > > On Sun, Jan 19, 2014 at 05:18:05PM +0100, Max Reitz wrote: > > > On 30.12.2013 06:29, Hu Tao wrote: > > > >In case of do preallocating metadata with a large cluster size, > > > >qcow2_alloc_cluster_offset() can allocate nothing and returns > > > >a NULL l2meta. This patch checks for it and link2 l2 with only > > > >valid l2meta. > > > > > > > >Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> > > > >--- > > > > block/qcow2.c | 14 ++++++++------ > > > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > > > > >diff --git a/block/qcow2.c b/block/qcow2.c > > > >index 46860d5..380c240 100644 > > > >--- a/block/qcow2.c > > > >+++ b/block/qcow2.c > > > >@@ -1399,18 +1399,20 @@ static int preallocate(BlockDriverState *bs) > > > > offset = 0; > > > > while (nb_sectors) { > > > >- num = MIN(nb_sectors, INT_MAX >> 9); > > > >+ num = MIN(nb_sectors, INT_MAX >> BDRV_SECTOR_BITS); > > > > > > Well, if you're already adjusting this here, you could also replace > > > the other occurrences of 9 and 512 in this function. ;-) > > > > > > > ret = qcow2_alloc_cluster_offset(bs, offset, &num, > > > > &host_offset, &meta); > > > > if (ret < 0) { > > > > return ret; > > > > } > > > >- ret = qcow2_alloc_cluster_link_l2(bs, meta); > > > >- if (ret < 0) { > > > >- qcow2_free_any_clusters(bs, meta->alloc_offset, meta->nb_clusters, > > > >- QCOW2_DISCARD_NEVER); > > > >- return ret; > > > >+ if (meta) { > > > >+ ret = qcow2_alloc_cluster_link_l2(bs, meta); > > > >+ if (ret < 0) { > > > >+ qcow2_free_any_clusters(bs, meta->alloc_offset, > > > >+ meta->nb_clusters, QCOW2_DISCARD_NEVER); > > > >+ return ret; > > > >+ } > > > > } > > > > /* There are no dependent requests, but we need to remove our request > > > > > > But this doesn't make this patch wrong, so: > > > > > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > > > Max, > > > > Thanks for all of you comments! > > The series looks good in general, but I think the comments are worth > addressing before we merge it. I would also love to see an qemu-iotests > case that tests the cases that would previously crash. Sure. Thanks for review!
On Tue, Jan 21, 2014 at 11:33:18AM +0800, Hu Tao wrote: > On Mon, Jan 20, 2014 at 04:17:16PM +0100, Kevin Wolf wrote: > > Am 20.01.2014 um 04:04 hat Hu Tao geschrieben: > > > On Sun, Jan 19, 2014 at 05:18:05PM +0100, Max Reitz wrote: > > > > On 30.12.2013 06:29, Hu Tao wrote: > > > > >In case of do preallocating metadata with a large cluster size, > > > > >qcow2_alloc_cluster_offset() can allocate nothing and returns > > > > >a NULL l2meta. This patch checks for it and link2 l2 with only > > > > >valid l2meta. > > > > > > > > > >Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> > > > > >--- > > > > > block/qcow2.c | 14 ++++++++------ > > > > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > > > > > > >diff --git a/block/qcow2.c b/block/qcow2.c > > > > >index 46860d5..380c240 100644 > > > > >--- a/block/qcow2.c > > > > >+++ b/block/qcow2.c > > > > >@@ -1399,18 +1399,20 @@ static int preallocate(BlockDriverState *bs) > > > > > offset = 0; > > > > > while (nb_sectors) { > > > > >- num = MIN(nb_sectors, INT_MAX >> 9); > > > > >+ num = MIN(nb_sectors, INT_MAX >> BDRV_SECTOR_BITS); > > > > > > > > Well, if you're already adjusting this here, you could also replace > > > > the other occurrences of 9 and 512 in this function. ;-) > > > > > > > > > ret = qcow2_alloc_cluster_offset(bs, offset, &num, > > > > > &host_offset, &meta); > > > > > if (ret < 0) { > > > > > return ret; > > > > > } > > > > >- ret = qcow2_alloc_cluster_link_l2(bs, meta); > > > > >- if (ret < 0) { > > > > >- qcow2_free_any_clusters(bs, meta->alloc_offset, meta->nb_clusters, > > > > >- QCOW2_DISCARD_NEVER); > > > > >- return ret; > > > > >+ if (meta) { > > > > >+ ret = qcow2_alloc_cluster_link_l2(bs, meta); > > > > >+ if (ret < 0) { > > > > >+ qcow2_free_any_clusters(bs, meta->alloc_offset, > > > > >+ meta->nb_clusters, QCOW2_DISCARD_NEVER); > > > > >+ return ret; > > > > >+ } > > > > > } > > > > > /* There are no dependent requests, but we need to remove our request > > > > > > > > But this doesn't make this patch wrong, so: > > > > > > > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > > > > > Max, > > > > > > Thanks for all of you comments! > > > > The series looks good in general, but I think the comments are worth > > addressing before we merge it. I would also love to see an qemu-iotests > > case that tests the cases that would previously crash. Should I add the test case into an existing file or create a new file?
Am 21.01.2014 um 07:02 hat Hu Tao geschrieben: > On Tue, Jan 21, 2014 at 11:33:18AM +0800, Hu Tao wrote: > > On Mon, Jan 20, 2014 at 04:17:16PM +0100, Kevin Wolf wrote: > > > Am 20.01.2014 um 04:04 hat Hu Tao geschrieben: > > > > On Sun, Jan 19, 2014 at 05:18:05PM +0100, Max Reitz wrote: > > > > > On 30.12.2013 06:29, Hu Tao wrote: > > > > > >In case of do preallocating metadata with a large cluster size, > > > > > >qcow2_alloc_cluster_offset() can allocate nothing and returns > > > > > >a NULL l2meta. This patch checks for it and link2 l2 with only > > > > > >valid l2meta. > > > > > > > > > > > >Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> > > > > > >--- > > > > > > block/qcow2.c | 14 ++++++++------ > > > > > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > > > > > > > > >diff --git a/block/qcow2.c b/block/qcow2.c > > > > > >index 46860d5..380c240 100644 > > > > > >--- a/block/qcow2.c > > > > > >+++ b/block/qcow2.c > > > > > >@@ -1399,18 +1399,20 @@ static int preallocate(BlockDriverState *bs) > > > > > > offset = 0; > > > > > > while (nb_sectors) { > > > > > >- num = MIN(nb_sectors, INT_MAX >> 9); > > > > > >+ num = MIN(nb_sectors, INT_MAX >> BDRV_SECTOR_BITS); > > > > > > > > > > Well, if you're already adjusting this here, you could also replace > > > > > the other occurrences of 9 and 512 in this function. ;-) > > > > > > > > > > > ret = qcow2_alloc_cluster_offset(bs, offset, &num, > > > > > > &host_offset, &meta); > > > > > > if (ret < 0) { > > > > > > return ret; > > > > > > } > > > > > >- ret = qcow2_alloc_cluster_link_l2(bs, meta); > > > > > >- if (ret < 0) { > > > > > >- qcow2_free_any_clusters(bs, meta->alloc_offset, meta->nb_clusters, > > > > > >- QCOW2_DISCARD_NEVER); > > > > > >- return ret; > > > > > >+ if (meta) { > > > > > >+ ret = qcow2_alloc_cluster_link_l2(bs, meta); > > > > > >+ if (ret < 0) { > > > > > >+ qcow2_free_any_clusters(bs, meta->alloc_offset, > > > > > >+ meta->nb_clusters, QCOW2_DISCARD_NEVER); > > > > > >+ return ret; > > > > > >+ } > > > > > > } > > > > > > /* There are no dependent requests, but we need to remove our request > > > > > > > > > > But this doesn't make this patch wrong, so: > > > > > > > > > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > > > > > > > Max, > > > > > > > > Thanks for all of you comments! > > > > > > The series looks good in general, but I think the comments are worth > > > addressing before we merge it. I would also love to see an qemu-iotests > > > case that tests the cases that would previously crash. > > Should I add the test case into an existing file or create a new file? The closest existing case I found is 049, which is however more about option parsing rather than the actual effect of the options. I think a new file might be better. If you add a new case, can you please use 079 as its number? There are several yet unmerged patch series in flight that take the lower numbers. Kevin
On Tue, Jan 21, 2014 at 12:04:47PM +0100, Kevin Wolf wrote: > Am 21.01.2014 um 07:02 hat Hu Tao geschrieben: > > On Tue, Jan 21, 2014 at 11:33:18AM +0800, Hu Tao wrote: > > > On Mon, Jan 20, 2014 at 04:17:16PM +0100, Kevin Wolf wrote: > > > > Am 20.01.2014 um 04:04 hat Hu Tao geschrieben: > > > > > On Sun, Jan 19, 2014 at 05:18:05PM +0100, Max Reitz wrote: > > > > > > On 30.12.2013 06:29, Hu Tao wrote: > > > > > > >In case of do preallocating metadata with a large cluster size, > > > > > > >qcow2_alloc_cluster_offset() can allocate nothing and returns > > > > > > >a NULL l2meta. This patch checks for it and link2 l2 with only > > > > > > >valid l2meta. > > > > > > > > > > > > > >Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> > > > > > > >--- > > > > > > > block/qcow2.c | 14 ++++++++------ > > > > > > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > > > > > > > > > > >diff --git a/block/qcow2.c b/block/qcow2.c > > > > > > >index 46860d5..380c240 100644 > > > > > > >--- a/block/qcow2.c > > > > > > >+++ b/block/qcow2.c > > > > > > >@@ -1399,18 +1399,20 @@ static int preallocate(BlockDriverState *bs) > > > > > > > offset = 0; > > > > > > > while (nb_sectors) { > > > > > > >- num = MIN(nb_sectors, INT_MAX >> 9); > > > > > > >+ num = MIN(nb_sectors, INT_MAX >> BDRV_SECTOR_BITS); > > > > > > > > > > > > Well, if you're already adjusting this here, you could also replace > > > > > > the other occurrences of 9 and 512 in this function. ;-) > > > > > > > > > > > > > ret = qcow2_alloc_cluster_offset(bs, offset, &num, > > > > > > > &host_offset, &meta); > > > > > > > if (ret < 0) { > > > > > > > return ret; > > > > > > > } > > > > > > >- ret = qcow2_alloc_cluster_link_l2(bs, meta); > > > > > > >- if (ret < 0) { > > > > > > >- qcow2_free_any_clusters(bs, meta->alloc_offset, meta->nb_clusters, > > > > > > >- QCOW2_DISCARD_NEVER); > > > > > > >- return ret; > > > > > > >+ if (meta) { > > > > > > >+ ret = qcow2_alloc_cluster_link_l2(bs, meta); > > > > > > >+ if (ret < 0) { > > > > > > >+ qcow2_free_any_clusters(bs, meta->alloc_offset, > > > > > > >+ meta->nb_clusters, QCOW2_DISCARD_NEVER); > > > > > > >+ return ret; > > > > > > >+ } > > > > > > > } > > > > > > > /* There are no dependent requests, but we need to remove our request > > > > > > > > > > > > But this doesn't make this patch wrong, so: > > > > > > > > > > > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > > > > > > > > > Max, > > > > > > > > > > Thanks for all of you comments! > > > > > > > > The series looks good in general, but I think the comments are worth > > > > addressing before we merge it. I would also love to see an qemu-iotests > > > > case that tests the cases that would previously crash. > > > > Should I add the test case into an existing file or create a new file? > > The closest existing case I found is 049, which is however more about > option parsing rather than the actual effect of the options. I think a > new file might be better. > > If you add a new case, can you please use 079 as its number? There are > several yet unmerged patch series in flight that take the lower numbers. OK.
diff --git a/block/qcow2.c b/block/qcow2.c index 46860d5..380c240 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1399,18 +1399,20 @@ static int preallocate(BlockDriverState *bs) offset = 0; while (nb_sectors) { - num = MIN(nb_sectors, INT_MAX >> 9); + num = MIN(nb_sectors, INT_MAX >> BDRV_SECTOR_BITS); ret = qcow2_alloc_cluster_offset(bs, offset, &num, &host_offset, &meta); if (ret < 0) { return ret; } - ret = qcow2_alloc_cluster_link_l2(bs, meta); - if (ret < 0) { - qcow2_free_any_clusters(bs, meta->alloc_offset, meta->nb_clusters, - QCOW2_DISCARD_NEVER); - return ret; + if (meta) { + ret = qcow2_alloc_cluster_link_l2(bs, meta); + if (ret < 0) { + qcow2_free_any_clusters(bs, meta->alloc_offset, + meta->nb_clusters, QCOW2_DISCARD_NEVER); + return ret; + } } /* There are no dependent requests, but we need to remove our request
In case of do preallocating metadata with a large cluster size, qcow2_alloc_cluster_offset() can allocate nothing and returns a NULL l2meta. This patch checks for it and link2 l2 with only valid l2meta. Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- block/qcow2.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)