Re: [Qemu-devel] [PATCH v2 02/20] block: Handle failure for potentially large allocations

[Kevin Wolf]

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 <address@hidden>
> > ---
> >  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