| Message ID | 1422366699-17473-7-git-send-email-den@openvz.org |
|---|---|
| State | New |
| Headers | show |
On 2015-01-27 at 08:51, 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. > > Signed-off-by: Denis V. Lunev <den@openvz.org> > CC: Kevin Wolf <kwolf@redhat.com> > CC: Stefan Hajnoczi <stefanha@redhat.com> > CC: Peter Lieven <pl@kamp.de> > CC: Fam Zheng <famz@redhat.com> > --- > block/raw-posix.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/block/raw-posix.c b/block/raw-posix.c > index c039bef..fa05239 100644 > --- a/block/raw-posix.c > +++ b/block/raw-posix.c > @@ -60,7 +60,7 @@ > #define FS_NOCOW_FL 0x00800000 /* Do not cow file */ > #endif > #endif > -#if defined(CONFIG_FALLOCATE_PUNCH_HOLE) || defined(CONFIG_FALLOCATE_ZERO_RANGE) > +#ifdef CONFIG_FALLOCATE This change doesn't seem right; CONFIG_FALLOCATE is set if posix_fallocate() is available, not for the Linux-specific fallocate() from linux/falloc.h. > #include <linux/falloc.h> > #endif > #if defined (__FreeBSD__) || defined(__FreeBSD_kernel__) > @@ -902,7 +902,7 @@ static int translate_err(int err) > return err; > } > > -#if defined(CONFIG_FALLOCATE_PUNCH_HOLE) || defined(CONFIG_FALLOCATE_ZERO_RANGE) > +#ifdef CONFIG_FALLOCATE Same here. > static int do_fallocate(int fd, int mode, off_t offset, off_t len) > { > do { > @@ -981,6 +981,12 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb) > } > #endif > > +#ifdef CONFIG_FALLOCATE > + if (aiocb->aio_offset >= aiocb->bs->total_sectors << BDRV_SECTOR_BITS) { > + return do_fallocate(s->fd, 0, aiocb->aio_offset, aiocb->aio_nbytes); > + } > +#endif > + This seems fine though, but as I've asked in patch 5: Do we want to have a "has_fallocate"? Other than that, this is the first usage of bs->total_sectors in this file; raw_co_get_block_status() does a similar check, but it uses bdrv_getlength() instead. If bs->total_sectors is correct, bdrv_getlength() will actually do nothing but return bs->total_sectors * BDRV_SECTOR_SIZE; it will only do more (that is, update bs->total_sectors) if it is not correct to use bs->total_sectors (and I feel like it may not be correct because BlockDriver.has_variable_length is true). Max > s->has_write_zeroes = false; > return ret; > }
On 27/01/15 20:57, Max Reitz wrote: > On 2015-01-27 at 08:51, 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. >> >> Signed-off-by: Denis V. Lunev <den@openvz.org> >> CC: Kevin Wolf <kwolf@redhat.com> >> CC: Stefan Hajnoczi <stefanha@redhat.com> >> CC: Peter Lieven <pl@kamp.de> >> CC: Fam Zheng <famz@redhat.com> >> --- >> block/raw-posix.c | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/block/raw-posix.c b/block/raw-posix.c >> index c039bef..fa05239 100644 >> --- a/block/raw-posix.c >> +++ b/block/raw-posix.c >> @@ -60,7 +60,7 @@ >> #define FS_NOCOW_FL 0x00800000 /* Do not cow >> file */ >> #endif >> #endif >> -#if defined(CONFIG_FALLOCATE_PUNCH_HOLE) || >> defined(CONFIG_FALLOCATE_ZERO_RANGE) >> +#ifdef CONFIG_FALLOCATE > > This change doesn't seem right; CONFIG_FALLOCATE is set if > posix_fallocate() is available, not for the Linux-specific fallocate() > from linux/falloc.h. > here is a check for fallocate and posix_fallocate in configure script # check for fallocate fallocate=no cat > $TMPC << EOF #include <fcntl.h> int main(void) { fallocate(0, 0, 0, 0); return 0; } EOF if compile_prog "" "" ; then fallocate=yes fi ... # check for posix_fallocate posix_fallocate=no cat > $TMPC << EOF #include <fcntl.h> int main(void) { posix_fallocate(0, 0, 0); return 0; } EOF if compile_prog "" "" ; then posix_fallocate=yes fi ... if test "$fallocate" = "yes" ; then echo "CONFIG_FALLOCATE=y" >> $config_host_mak fi ... if test "$posix_fallocate" = "yes" ; then echo "CONFIG_POSIX_FALLOCATE=y" >> $config_host_mak fi Thus my check looks correct to me. >> #include <linux/falloc.h> >> #endif >> #if defined (__FreeBSD__) || defined(__FreeBSD_kernel__) >> @@ -902,7 +902,7 @@ static int translate_err(int err) >> return err; >> } >> -#if defined(CONFIG_FALLOCATE_PUNCH_HOLE) || >> defined(CONFIG_FALLOCATE_ZERO_RANGE) >> +#ifdef CONFIG_FALLOCATE > > Same here. > >> static int do_fallocate(int fd, int mode, off_t offset, off_t len) >> { >> do { >> @@ -981,6 +981,12 @@ static ssize_t >> handle_aiocb_write_zeroes(RawPosixAIOData *aiocb) >> } >> #endif >> +#ifdef CONFIG_FALLOCATE >> + if (aiocb->aio_offset >= aiocb->bs->total_sectors << >> BDRV_SECTOR_BITS) { >> + return do_fallocate(s->fd, 0, aiocb->aio_offset, >> aiocb->aio_nbytes); >> + } >> +#endif >> + > > This seems fine though, but as I've asked in patch 5: Do we want to > have a "has_fallocate"? > > Other than that, this is the first usage of bs->total_sectors in this > file; raw_co_get_block_status() does a similar check, but it uses > bdrv_getlength() instead. If bs->total_sectors is correct, > bdrv_getlength() will actually do nothing but return bs->total_sectors > * BDRV_SECTOR_SIZE; it will only do more (that is, update > bs->total_sectors) if it is not correct to use bs->total_sectors (and > I feel like it may not be correct because > BlockDriver.has_variable_length is true). > > Max > ok, will do
On 2015-01-27 at 13:19, Denis V. Lunev wrote: > On 27/01/15 20:57, Max Reitz wrote: >> On 2015-01-27 at 08:51, 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. >>> >>> Signed-off-by: Denis V. Lunev <den@openvz.org> >>> CC: Kevin Wolf <kwolf@redhat.com> >>> CC: Stefan Hajnoczi <stefanha@redhat.com> >>> CC: Peter Lieven <pl@kamp.de> >>> CC: Fam Zheng <famz@redhat.com> >>> --- >>> block/raw-posix.c | 10 ++++++++-- >>> 1 file changed, 8 insertions(+), 2 deletions(-) >>> >>> diff --git a/block/raw-posix.c b/block/raw-posix.c >>> index c039bef..fa05239 100644 >>> --- a/block/raw-posix.c >>> +++ b/block/raw-posix.c >>> @@ -60,7 +60,7 @@ >>> #define FS_NOCOW_FL 0x00800000 /* Do not cow >>> file */ >>> #endif >>> #endif >>> -#if defined(CONFIG_FALLOCATE_PUNCH_HOLE) || >>> defined(CONFIG_FALLOCATE_ZERO_RANGE) >>> +#ifdef CONFIG_FALLOCATE >> >> This change doesn't seem right; CONFIG_FALLOCATE is set if >> posix_fallocate() is available, not for the Linux-specific >> fallocate() from linux/falloc.h. >> > > here is a check for fallocate and posix_fallocate in configure script > > # check for fallocate > fallocate=no > cat > $TMPC << EOF > #include <fcntl.h> > > int main(void) > { > fallocate(0, 0, 0, 0); > return 0; > } > EOF > if compile_prog "" "" ; then > fallocate=yes > fi > ... > # check for posix_fallocate > posix_fallocate=no > cat > $TMPC << EOF > #include <fcntl.h> > > int main(void) > { > posix_fallocate(0, 0, 0); > return 0; > } > EOF > if compile_prog "" "" ; then > posix_fallocate=yes > fi > ... > if test "$fallocate" = "yes" ; then > echo "CONFIG_FALLOCATE=y" >> $config_host_mak > fi > ... > if test "$posix_fallocate" = "yes" ; then > echo "CONFIG_POSIX_FALLOCATE=y" >> $config_host_mak > fi > > Thus my check looks correct to me. Oh, sorry, I somehow mixed those checks. You're right. Very well then; maybe you want to mention this change in the commit message, though? Max > >>> #include <linux/falloc.h> >>> #endif >>> #if defined (__FreeBSD__) || defined(__FreeBSD_kernel__) >>> @@ -902,7 +902,7 @@ static int translate_err(int err) >>> return err; >>> } >>> -#if defined(CONFIG_FALLOCATE_PUNCH_HOLE) || >>> defined(CONFIG_FALLOCATE_ZERO_RANGE) >>> +#ifdef CONFIG_FALLOCATE >> >> Same here. >> >>> static int do_fallocate(int fd, int mode, off_t offset, off_t len) >>> { >>> do { >>> @@ -981,6 +981,12 @@ static ssize_t >>> handle_aiocb_write_zeroes(RawPosixAIOData *aiocb) >>> } >>> #endif >>> +#ifdef CONFIG_FALLOCATE >>> + if (aiocb->aio_offset >= aiocb->bs->total_sectors << >>> BDRV_SECTOR_BITS) { >>> + return do_fallocate(s->fd, 0, aiocb->aio_offset, >>> aiocb->aio_nbytes); >>> + } >>> +#endif >>> + >> >> This seems fine though, but as I've asked in patch 5: Do we want to >> have a "has_fallocate"? >> >> Other than that, this is the first usage of bs->total_sectors in this >> file; raw_co_get_block_status() does a similar check, but it uses >> bdrv_getlength() instead. If bs->total_sectors is correct, >> bdrv_getlength() will actually do nothing but return >> bs->total_sectors * BDRV_SECTOR_SIZE; it will only do more (that is, >> update bs->total_sectors) if it is not correct to use >> bs->total_sectors (and I feel like it may not be correct because >> BlockDriver.has_variable_length is true). >> >> Max >> > ok, will do
On 27/01/15 21:24, Max Reitz wrote: > On 2015-01-27 at 13:19, Denis V. Lunev wrote: >> On 27/01/15 20:57, Max Reitz wrote: >>> On 2015-01-27 at 08:51, 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. >>>> >>>> Signed-off-by: Denis V. Lunev <den@openvz.org> >>>> CC: Kevin Wolf <kwolf@redhat.com> >>>> CC: Stefan Hajnoczi <stefanha@redhat.com> >>>> CC: Peter Lieven <pl@kamp.de> >>>> CC: Fam Zheng <famz@redhat.com> >>>> --- >>>> block/raw-posix.c | 10 ++++++++-- >>>> 1 file changed, 8 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/block/raw-posix.c b/block/raw-posix.c >>>> index c039bef..fa05239 100644 >>>> --- a/block/raw-posix.c >>>> +++ b/block/raw-posix.c >>>> @@ -60,7 +60,7 @@ >>>> #define FS_NOCOW_FL 0x00800000 /* Do not cow >>>> file */ >>>> #endif >>>> #endif >>>> -#if defined(CONFIG_FALLOCATE_PUNCH_HOLE) || >>>> defined(CONFIG_FALLOCATE_ZERO_RANGE) >>>> +#ifdef CONFIG_FALLOCATE >>> >>> This change doesn't seem right; CONFIG_FALLOCATE is set if >>> posix_fallocate() is available, not for the Linux-specific >>> fallocate() from linux/falloc.h. >>> >> >> here is a check for fallocate and posix_fallocate in configure script >> >> # check for fallocate >> fallocate=no >> cat > $TMPC << EOF >> #include <fcntl.h> >> >> int main(void) >> { >> fallocate(0, 0, 0, 0); >> return 0; >> } >> EOF >> if compile_prog "" "" ; then >> fallocate=yes >> fi >> ... >> # check for posix_fallocate >> posix_fallocate=no >> cat > $TMPC << EOF >> #include <fcntl.h> >> >> int main(void) >> { >> posix_fallocate(0, 0, 0); >> return 0; >> } >> EOF >> if compile_prog "" "" ; then >> posix_fallocate=yes >> fi >> ... >> if test "$fallocate" = "yes" ; then >> echo "CONFIG_FALLOCATE=y" >> $config_host_mak >> fi >> ... >> if test "$posix_fallocate" = "yes" ; then >> echo "CONFIG_POSIX_FALLOCATE=y" >> $config_host_mak >> fi >> >> Thus my check looks correct to me. > > Oh, sorry, I somehow mixed those checks. You're right. > > Very well then; maybe you want to mention this change in the commit > message, though? > > Max > no prob
diff --git a/block/raw-posix.c b/block/raw-posix.c index c039bef..fa05239 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -60,7 +60,7 @@ #define FS_NOCOW_FL 0x00800000 /* Do not cow file */ #endif #endif -#if defined(CONFIG_FALLOCATE_PUNCH_HOLE) || defined(CONFIG_FALLOCATE_ZERO_RANGE) +#ifdef CONFIG_FALLOCATE #include <linux/falloc.h> #endif #if defined (__FreeBSD__) || defined(__FreeBSD_kernel__) @@ -902,7 +902,7 @@ static int translate_err(int err) return err; } -#if defined(CONFIG_FALLOCATE_PUNCH_HOLE) || defined(CONFIG_FALLOCATE_ZERO_RANGE) +#ifdef CONFIG_FALLOCATE static int do_fallocate(int fd, int mode, off_t offset, off_t len) { do { @@ -981,6 +981,12 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb) } #endif +#ifdef CONFIG_FALLOCATE + if (aiocb->aio_offset >= aiocb->bs->total_sectors << BDRV_SECTOR_BITS) { + return do_fallocate(s->fd, 0, aiocb->aio_offset, aiocb->aio_nbytes); + } +#endif + s->has_write_zeroes = false; return ret; }
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. Signed-off-by: Denis V. Lunev <den@openvz.org> CC: Kevin Wolf <kwolf@redhat.com> CC: Stefan Hajnoczi <stefanha@redhat.com> CC: Peter Lieven <pl@kamp.de> CC: Fam Zheng <famz@redhat.com> --- block/raw-posix.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)