Re: [Qemu-devel] [PATCH 5/7] block/raw-posix: call plain fallocate in handle_aiocb_write_zeroes

[Denis V. Lunev]

On 30/01/15 18:42, Max Reitz wrote:
> On 2015-01-30 at 10:41, Denis V. Lunev wrote:
> > On 30/01/15 17:58, Max Reitz wrote:
> > > On 2015-01-30 at 03:42, Denis V. Lunev wrote:
> > > > There is a possibility that we are extending our image and thus 
> > > > writing
> > > > zeroes beyond the end of the file. In this case we do not need to care
> > > > about the hole to make sure that there is no data in the file under
> > > > this offset (pre-condition to fallocate(0) to work). We could 
> > > > simply call
> > > > fallocate(0).
> > > >
> > > > This improves the performance of writing zeroes even on really old
> > > > platforms which do not have even FALLOC_FL_PUNCH_HOLE.
> > > >
> > > > Before the patch do_fallocate was used when either
> > > > CONFIG_FALLOCATE_PUNCH_HOLE or CONFIG_FALLOCATE_ZERO_RANGE are 
> > > > defined.
> > > > Now the story is different. CONFIG_FALLOCATE is defined when Linux
> > > > fallocate is defined, posix_fallocate is completely different story
> > > > (CONFIG_POSIX_FALLOCATE). CONFIG_FALLOCATE is mandatory prerequite
> > > > for both CONFIG_FALLOCATE_PUNCH_HOLE and CONFIG_FALLOCATE_ZERO_RANGE
> > > > thus we are on the safe side.
> > > >
> > > > Signed-off-by: Denis V. Lunev <address@hidden>
> > > > CC: Max Reitz <address@hidden>
> > > > CC: Kevin Wolf <address@hidden>
> > > > CC: Stefan Hajnoczi <address@hidden>
> > > > CC: Peter Lieven <address@hidden>
> > > > CC: Fam Zheng <address@hidden>
> > > > ---
> > > >   block/raw-posix.c | 14 +++++++++++++-
> > > >   1 file changed, 13 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/block/raw-posix.c b/block/raw-posix.c
> > > > index 5a777e7..1c88ad8 100644
> > > > --- a/block/raw-posix.c
> > > > +++ b/block/raw-posix.c
> > > > @@ -147,6 +147,7 @@ typedef struct BDRVRawState {
> > > >       bool has_discard:1;
> > > >       bool has_write_zeroes:1;
> > > >       bool discard_zeroes:1;
> > > > +    bool has_fallocate;
> > > >       bool needs_alignment;
> > > >   } BDRVRawState;
> > > >   @@ -452,6 +453,7 @@ static int raw_open_common(BlockDriverState 
> > > > *bs, QDict *options,
> > > >       }
> > > >       if (S_ISREG(st.st_mode)) {
> > > >           s->discard_zeroes = true;
> > > > +        s->has_fallocate = true;
> > >
> > > This could be moved upwards where has_discard and has_write_zeroes 
> > > are initialized; but it won't matter in practice, I hope. Thus:
> > >
> > > Reviewed-by: Max Reitz <address@hidden>
> >
> > This does matter as has_discard and has_write_zeroes are bit fields
> > thus I can not insert something useful into the middle of those
> > fields.
>
> Right, but I did not mean the placement inside of the structure but 
> the placement of the initialization statement (s->has_fallocate = 
> true) in raw_open_common().
>
> Max
hmm, you are right. This is possible but I don't want
to have this bit set for block/character etc devices
even if they are not using this bit/code. With my
approach the assignment is made in a way to indicate
application area.

Thank you for a review :) It is somewhat difficult
to obtain feedback here in comparison with Linux
kernel.