[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 3/5] block: introduce preallocate filter
From: |
Stefan Hajnoczi |
Subject: |
Re: [PATCH 3/5] block: introduce preallocate filter |
Date: |
Mon, 13 Jul 2020 12:27:15 +0100 |
On Wed, Jul 08, 2020 at 07:17:52PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 08.07.2020 15:07, Stefan Hajnoczi wrote:
> > On Sat, Jun 20, 2020 at 05:36:47PM +0300, Vladimir Sementsov-Ogievskiy
> > wrote:
> > > It may be used for file-systems with slow allocation.
> > >
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > > ---
> > > qapi/block-core.json | 3 +-
> > > block/preallocate.c | 255 +++++++++++++++++++++++++++++++++++++++++++
> > > block/Makefile.objs | 1 +
> > > 3 files changed, 258 insertions(+), 1 deletion(-)
> > > create mode 100644 block/preallocate.c
> >
> > Please add documentation to docs/system/qemu-block-drivers.rst.inc
> > describing the purpose of this block driver and how to use it.
>
> This implies adding new section "Filters", yes?
Yes, please.
> >
> > Since this filter grows the file I guess it's intended to be below an
> > image format?
>
> Yes, between format and protocol nodes.
Thanks for confirming. Please include this in the documentation. While
reading the code I was thinking about how this can work if the guest is
directly exposed to the filter. Users might wonder about the same thing.
> > > + /* Length should not have changed */
> > > + assert(len == bdrv_getlength(bs->file->bs));
> > > +
> > > + start = write_zero ? MIN(offset, len) : len;
> > > + end = QEMU_ALIGN_UP(offset + bytes + s->prealloc_size,
> > > s->prealloc_align);
> > > +
> > > + ret = bdrv_co_pwrite_zeroes_locked(bs->file, start, end - start,
> > > + BDRV_REQ_NO_FALLBACK, lock);
> > > +
> > > + bdrv_co_range_unlock(lock);
> >
> > Hmm...if this piece of code is the only user of bdrv_co_range_try_lock()
> > then a BDRV_REQ_NO_WAIT flag might be a simpler API.
> >
> > I thought the lock request would be used to perform multiple operations,
> > but if it's just for a single operation then I think it's less code and
> > easier to understand without the lock request.
>
> Hmm, again, I don't remember exact reasons. Firstly, I was afraid of length
> change during try_lock and have a double check for bdrv_getlength(). Then
> I decided that it's impossible and change the check to an assertion.
> Probably, the only reason to leave locked range was "I already have the code,
> it will help with copy-on-read, why not to use it".. OK, I'll try rewrite it
> with help of new flag.
Thanks!
Stefan
signature.asc
Description: PGP signature