Am 30.05.2014 um 14:08 hat Benoît Canet geschrieben:
> The Wednesday 28 May 2014 à 16:37:35 (+0200), Kevin Wolf wrote :
> > Some code in the block layer makes potentially huge allocations. Failure
> > is not completely unexpected there, so avoid aborting qemu and handle
> > out-of-memory situations gracefully.
> >
> > This patch addresses bounce buffer allocations in block.c. While at it,
> > convert bdrv_commit() from plain g_malloc() to qemu_try_blockalign().
> >
> > Signed-off-by: Kevin Wolf <kw...@redhat.com>
> > ---
> > block.c | 34 +++++++++++++++++++++++++++-------
> > 1 file changed, 27 insertions(+), 7 deletions(-)
> >
> > diff --git a/block.c b/block.c
> > index dc9bcbd..0e11376 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -2271,7 +2271,14 @@ int bdrv_commit(BlockDriverState *bs)
> > }
> >
> > total_sectors = length >> BDRV_SECTOR_BITS;
> > - buf = g_malloc(COMMIT_BUF_SECTORS * BDRV_SECTOR_SIZE);
> > +
> > + /* qemu_try_blockalign() for bs will choose an alignment that works for
> > + * bs->backing_hd as well, so no need to compare the alignment
> > manually. */
> > + buf = qemu_try_blockalign(bs, COMMIT_BUF_SECTORS * BDRV_SECTOR_SIZE);
> > + if (buf == NULL) {
> > + ret = -ENOMEM;
> > + goto ro_cleanup;
> > + }
> >
> > for (sector = 0; sector < total_sectors; sector += n) {
> > ret = bdrv_is_allocated(bs, sector, COMMIT_BUF_SECTORS, &n);
> > @@ -2309,7 +2316,7 @@ int bdrv_commit(BlockDriverState *bs)
> >
> > ret = 0;
> > ro_cleanup:
> > - g_free(buf);
> > + qemu_vfree(buf);
> >
> > if (ro) {
> > /* ignoring error return here */
> > @@ -2970,7 +2977,12 @@ static int coroutine_fn
> > bdrv_co_do_copy_on_readv(BlockDriverState *bs,
> > cluster_sector_num, cluster_nb_sectors);
> >
> > iov.iov_len = cluster_nb_sectors * BDRV_SECTOR_SIZE;
> > - iov.iov_base = bounce_buffer = qemu_blockalign(bs, iov.iov_len);
> > + iov.iov_base = bounce_buffer = qemu_try_blockalign(bs, iov.iov_len);
> > + if (iov.iov_len && bounce_buffer == NULL) {
> > + ret = -ENOMEM;
> > + goto err;
> > + }
> > +
> > qemu_iovec_init_external(&bounce_qiov, &iov, 1);
> >
> > ret = drv->bdrv_co_readv(bs, cluster_sector_num, cluster_nb_sectors,
> > @@ -3242,7 +3254,11 @@ static int coroutine_fn
> > bdrv_co_do_write_zeroes(BlockDriverState *bs,
> > /* Fall back to bounce buffer if write zeroes is unsupported */
> > iov.iov_len = num * BDRV_SECTOR_SIZE;
> > if (iov.iov_base == NULL) {
> > - iov.iov_base = qemu_blockalign(bs, num * BDRV_SECTOR_SIZE);
> > + iov.iov_base = qemu_try_blockalign(bs, num *
> > BDRV_SECTOR_SIZE);
> Are we sure num never === 0 ? This would render the test false.
Yes, the loop condition is nb_sectors > 0, and num > 0 follows from it.
However, I fixed a few bugs of this kind in v2 and apparently it is too
confusing for all of us to keep track of it. I suppose it's better if I
change qemu_try_blockalign() to never return NULL on success. The way
to achieve this would be something like 'if (!size) size = align' before
calling into qemu_try_memalign(), i.e. always allocating something even
if it wasn't actually requested.
Kevin
